Skip to content

[org] Disallow logins with organizational Git Auth #16874

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 16, 2023
Merged

[org] Disallow logins with organizational Git Auth #16874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions components/server/src/auth/auth-provider-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export class AuthProviderService {
host: oap.host.toLowerCase(),
verified: oap.status === "verified",
builtin: false,
disallowLogin: !!oap.organizationId,
// hiddenOnDashboard: true, // i.e. show only if it's used
loginContextMatcher: `https://${oap.host}/`,
oauth: {
Expand Down
9 changes: 9 additions & 0 deletions components/server/src/auth/authenticator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,15 @@ export class Authenticator {
res.redirect(this.getSorryUrl(`Bad request: missing parameters.`));
return;
}
// Logins with organizational Git Auth is not permitted
if (authProvider.info.organizationId) {
log.info({ sessionId: req.sessionID }, `Login with "${host}" is not permitted.`, {
"authorize-flow": true,
ap: authProvider.info,
});
res.redirect(this.getSorryUrl(`Login with "${host}" is not permitted.`));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better call to action we can include? What should the user do when this happens?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question.
The response here is what's done on bad requests in that area. The legacy sorry page is used as a sink where we're lacking proper rending on the dashboard.
Why calling that a bad request? Those organizational Git Auth providers won't be available on the Login screen, they are not meant to be included there. Here we are guarding from forged requests to the /api/login endpoint parameterized with the host of an organizational Git Auth provider.
Given that one cannot reach this point by navigating any link, I don't think it make sense to provide any CTA. But that's definitely biased, so if find anything helpful, let's do that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with this, given that it's a not a case a user can end up with in a regular case.

☁️ To better highlight this, we could also just return a 400 (Bad Request). But I think it's more cosmetics.

return;
}
if (this.config.disableDynamicAuthProviderLogin && !authProvider.params.builtin) {
log.info({ sessionId: req.sessionID }, `Auth Provider is not allowed.`, { ap: authProvider.info });
res.redirect(this.getSorryUrl(`Login with ${authProvider.params.host} is not allowed.`));
Expand Down