-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Add authentication via GitHub #920
Conversation
Codecov Report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick comments added. I would also like to try it out locally and test it. Is it ready for me to do that? One of the things I will test is: Does it support linking my existing Sourcegraph account to GitHub? (The auth provider API supports this, but there might be nuances.)
Other things (not necessary if you just want to merge it and try it out behind a feature flag):
- Add docs for this auth provider. Probably the best way is to add the main doc section to the doc/integration/github.md file and link to it from the auth providers doc file (instead of vice versa).
- Give guidance on what to use as the OAuth app name and URL when creating the OAuth client application on GitHub so that users know it is the Sourcegraph app but it is created and owned by their org's site admin--it does not leak the info to Sourcegraph itself.
Questions:
- Why use GitHub OAuth and not GitHub Apps (the new way GitHub prefers you to integrate)?
- Will we be able to reuse a lot of this OAuth code when we want to support GitLab OAuth and other OAuth providers? I think it's better to call this auth provider
type: "github"
and nottype: "oauth"
even we end up supporting general OAuth because this is probably going to be an extremely common way of using OAuth.
schema/site.schema.json
Outdated
}, | ||
"clientID": { | ||
"type": "string", | ||
"description": "The Client ID of the GitHub OAuth app, accessible from https://github.com/settings/developers" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Full sentences (end with period).
and when you give a github.com URL, always say something to indicate that GHE is also supported, like "accessible from https://github.com/whatever (or the same path on GitHub Enterprise)."
schema/site.schema.json
Outdated
@@ -1092,6 +1093,32 @@ | |||
} | |||
} | |||
}, | |||
"GitHubAuthProvider": { | |||
"description": "Configures the GitHub OAuth authentication provider for SSO. In addition to specifying this configuration object, you must also create a OAuth App on your GitHub instance: https://developer.github.com/apps/building-oauth-apps/creating-an-oauth-app/. On log-in, Sourcegraph will request `repo` scope access.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the first sentence this to mention GHE:
Configures the GitHub (or GitHub Enterprise) OAuth user authentication provider for SSO.
We and GitHub both call it "sign in" (re: On log-in
).
Also, make the last sentence clearer. Something like:
When a user signs into Sourcegraph or links their GitHub account to their existing Sourcegraph account, GitHub will prompt the user for the
repo
scope.
Yes, please test locally. It links your existing account the same way the other SSO integrations do so (using verified emails).
Will do.
The GitHub app docs state that even after an app is installed, users are still required to go through the OAuth flow to grant user-level permissions and obtain a user-specific token. Such a token is required, because the only way AFAICT that the GitHub API permits querying user repo permissions is through the "show me affiliated repositories for the currently authenticated user" endpoint. It's possible there's a simpler way, but I exhausted my patience after a few hours. OAuth apps suffice for our purposes and I'd like to get through all the auth stuff ASAP so we can focus on building new features.
Likely, yes. The library this uses for the OAuth handlers is https://github.com/dghubble/gologin. It currently doesn't have explicit support for GitLab, but I imagine it will be straightforward. I agree that the type should be "github", rather than "oauth". There are enough flavors of OAuth-based authentication out there that I think it would be folly to try to have a single handler for all of them (OpenIDConnect, after all is such a flavor). The auth provider type is already called "github". |
e6e01ee
to
b44a8ed
Compare
Merging this behind a feature flag w/ Quinn's approval. Will update the docs and authz support in a subsequent PR. |
Removes usage of the endpoint deprecated in https://github.com/sourcegraph/sourcegraph/pull/46299, related to sourcegraph/customer#1525
Adds a GitHub authentication provider that enables signing in via GitHub (both GitHub.com and enterprise). Requires creating an OAuth app on the GitHub instance.
Authentication requests
repo
scope from users. This is to permit us to use the OAuth token to query repository permissions (to be supported in a subsequent PR).