- Notifications
You must be signed in to change notification settings - Fork914
feat: support the OAuth2 device flow with GitHub for signing in#16585
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Uh oh!
There was an error while loading.Please reload this page.
Merged
Changes fromall commits
Commits
Show all changes
5 commits Select commitHold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
31 changes: 29 additions & 2 deletionscli/server.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -677,12 +677,13 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. | ||
| } | ||
| } | ||
| if vals.OAuth2.Github.ClientSecret != ""|| vals.OAuth2.Github.DeviceFlow.Value(){ | ||
| options.GithubOAuth2Config, err = configureGithubOAuth2( | ||
| oauthInstrument, | ||
| vals.AccessURL.Value(), | ||
| vals.OAuth2.Github.ClientID.String(), | ||
| vals.OAuth2.Github.ClientSecret.String(), | ||
| vals.OAuth2.Github.DeviceFlow.Value(), | ||
| vals.OAuth2.Github.AllowSignups.Value(), | ||
| vals.OAuth2.Github.AllowEveryone.Value(), | ||
| vals.OAuth2.Github.AllowedOrgs, | ||
| @@ -1831,8 +1832,10 @@ func configureCAPool(tlsClientCAFile string, tlsConfig *tls.Config) error { | ||
| return nil | ||
| } | ||
| // TODO: convert the argument list to a struct, it's easy to mix up the order of the arguments | ||
| // | ||
| //nolint:revive // Ignore flag-parameter: parameter 'allowEveryone' seems to be a control flag, avoid control coupling (revive) | ||
| func configureGithubOAuth2(instrument *promoauth.Factory, accessURL *url.URL, clientID, clientSecret string,deviceFlow,allowSignups, allowEveryone bool, allowOrgs []string, rawTeams []string, enterpriseBaseURL string) (*coderd.GithubOAuth2Config, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. We should really make these args a struct soon. Not necessary in this PR, just a ton of options that can be misorderd. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Added a TODO | ||
| redirectURL, err := accessURL.Parse("/api/v2/users/oauth2/github/callback") | ||
| if err != nil { | ||
| return nil, xerrors.Errorf("parse github oauth callback url: %w", err) | ||
| @@ -1898,6 +1901,17 @@ func configureGithubOAuth2(instrument *promoauth.Factory, accessURL *url.URL, cl | ||
| return github.NewClient(client), nil | ||
| } | ||
| var deviceAuth *externalauth.DeviceAuth | ||
| if deviceFlow { | ||
| deviceAuth = &externalauth.DeviceAuth{ | ||
| Config: instrumentedOauth, | ||
| ClientID: clientID, | ||
| TokenURL: endpoint.TokenURL, | ||
| Scopes: []string{"read:user", "read:org", "user:email"}, | ||
| CodeURL: endpoint.DeviceAuthURL, | ||
| } | ||
| } | ||
| return &coderd.GithubOAuth2Config{ | ||
| OAuth2Config: instrumentedOauth, | ||
| AllowSignups: allowSignups, | ||
| @@ -1941,6 +1955,19 @@ func configureGithubOAuth2(instrument *promoauth.Factory, accessURL *url.URL, cl | ||
| team, _, err := api.Teams.GetTeamMembershipBySlug(ctx, org, teamSlug, username) | ||
| return team, err | ||
| }, | ||
| DeviceFlowEnabled: deviceFlow, | ||
| ExchangeDeviceCode: func(ctx context.Context, deviceCode string) (*oauth2.Token, error) { | ||
| if !deviceFlow { | ||
| return nil, xerrors.New("device flow is not enabled") | ||
| } | ||
| return deviceAuth.ExchangeDeviceCode(ctx, deviceCode) | ||
| }, | ||
| AuthorizeDevice: func(ctx context.Context) (*codersdk.ExternalAuthDevice, error) { | ||
| if !deviceFlow { | ||
| return nil, xerrors.New("device flow is not enabled") | ||
| } | ||
| return deviceAuth.AuthorizeDevice(ctx) | ||
| }, | ||
| }, nil | ||
| } | ||
3 changes: 3 additions & 0 deletionscli/testdata/coder_server_--help.golden
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
3 changes: 3 additions & 0 deletionscli/testdata/server-config.yaml.golden
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
28 changes: 28 additions & 0 deletionscoderd/apidoc/docs.go
Some generated files are not rendered by default. Learn more abouthow customized files appear on GitHub.
Oops, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
24 changes: 24 additions & 0 deletionscoderd/apidoc/swagger.json
Some generated files are not rendered by default. Learn more abouthow customized files appear on GitHub.
Oops, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
1 change: 1 addition & 0 deletionscoderd/coderd.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
13 changes: 10 additions & 3 deletionscoderd/httpmw/oauth2.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -167,9 +167,16 @@ func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, authURLOp | ||
| oauthToken, err := config.Exchange(ctx, code) | ||
| if err != nil { | ||
| errorCode := http.StatusInternalServerError | ||
| detail := err.Error() | ||
| if detail == "authorization_pending" { | ||
| // In the device flow, the token may not be immediately | ||
| // available. This is expected, and the client will retry. | ||
| errorCode = http.StatusBadRequest | ||
| } | ||
Comment on lines +172 to +176 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Is this error comparison correct? Comparing to the usage in the oauth lib, it is checking the Usage:https://github.com/golang/oauth2/blob/master/deviceauth.go#L189-L190 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. We actually don't use the For context, we use a custom implementation instead of the lib's | ||
| httpapi.Write(ctx, rw, errorCode, codersdk.Response{ | ||
| Message: "Failed exchanging Oauth code.", | ||
| Detail: detail, | ||
| }) | ||
| return | ||
| } | ||
76 changes: 75 additions & 1 deletioncoderd/userauth.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -748,12 +748,32 @@ type GithubOAuth2Config struct { | ||
| ListOrganizationMemberships func(ctx context.Context, client *http.Client) ([]*github.Membership, error) | ||
| TeamMembership func(ctx context.Context, client *http.Client, org, team, username string) (*github.Membership, error) | ||
| DeviceFlowEnabled bool | ||
| ExchangeDeviceCode func(ctx context.Context, deviceCode string) (*oauth2.Token, error) | ||
| AuthorizeDevice func(ctx context.Context) (*codersdk.ExternalAuthDevice, error) | ||
| AllowSignups bool | ||
| AllowEveryone bool | ||
| AllowOrganizations []string | ||
| AllowTeams []GithubOAuth2Team | ||
| } | ||
| func (c *GithubOAuth2Config) Exchange(ctx context.Context, code string, opts ...oauth2.AuthCodeOption) (*oauth2.Token, error) { | ||
| if !c.DeviceFlowEnabled { | ||
| return c.OAuth2Config.Exchange(ctx, code, opts...) | ||
| } | ||
| return c.ExchangeDeviceCode(ctx, code) | ||
| } | ||
| func (c *GithubOAuth2Config) AuthCodeURL(state string, opts ...oauth2.AuthCodeOption) string { | ||
| if !c.DeviceFlowEnabled { | ||
| return c.OAuth2Config.AuthCodeURL(state, opts...) | ||
| } | ||
| // This is an absolute path in the Coder app. The device flow is orchestrated | ||
| // by the Coder frontend, so we need to redirect the user to the device flow page. | ||
| return "/login/device?state=" + state | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Is this a relative url to our Coder app? Not an external auth url? Can you just leave a comment that mentions this. I say that becaused | ||
| } | ||
| // @Summary Get authentication methods | ||
| // @ID get-authentication-methods | ||
| // @Security CoderSessionToken | ||
| @@ -786,6 +806,53 @@ func (api *API) userAuthMethods(rw http.ResponseWriter, r *http.Request) { | ||
| }) | ||
| } | ||
| // @Summary Get Github device auth. | ||
| // @ID get-github-device-auth | ||
| // @Security CoderSessionToken | ||
| // @Produce json | ||
| // @Tags Users | ||
| // @Success 200 {object} codersdk.ExternalAuthDevice | ||
| // @Router /users/oauth2/github/device [get] | ||
| func (api *API) userOAuth2GithubDevice(rw http.ResponseWriter, r *http.Request) { | ||
| var ( | ||
| ctx = r.Context() | ||
| auditor = api.Auditor.Load() | ||
| aReq, commitAudit = audit.InitRequest[database.APIKey](rw, &audit.RequestParams{ | ||
| Audit: *auditor, | ||
| Log: api.Logger, | ||
| Request: r, | ||
| Action: database.AuditActionLogin, | ||
| }) | ||
| ) | ||
| aReq.Old = database.APIKey{} | ||
| defer commitAudit() | ||
| if api.GithubOAuth2Config == nil { | ||
| httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ | ||
| Message: "Github OAuth2 is not enabled.", | ||
| }) | ||
| return | ||
| } | ||
| if !api.GithubOAuth2Config.DeviceFlowEnabled { | ||
| httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ | ||
| Message: "Device flow is not enabled for Github OAuth2.", | ||
| }) | ||
| return | ||
| } | ||
| deviceAuth, err := api.GithubOAuth2Config.AuthorizeDevice(ctx) | ||
| if err != nil { | ||
| httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ | ||
| Message: "Failed to authorize device.", | ||
| Detail: err.Error(), | ||
| }) | ||
| return | ||
| } | ||
| httpapi.Write(ctx, rw, http.StatusOK, deviceAuth) | ||
| } | ||
| // @Summary OAuth 2.0 GitHub Callback | ||
| // @ID oauth-20-github-callback | ||
| // @Security CoderSessionToken | ||
| @@ -1016,7 +1083,14 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { | ||
| } | ||
| redirect = uriFromURL(redirect) | ||
| if api.GithubOAuth2Config.DeviceFlowEnabled { | ||
| // In the device flow, the redirect is handled client-side. | ||
| httpapi.Write(ctx, rw, http.StatusOK, codersdk.OAuth2DeviceFlowCallbackResponse{ | ||
| RedirectURL: redirect, | ||
| }) | ||
Comment on lines +1087 to +1090 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. 👍 | ||
| } else { | ||
| http.Redirect(rw, r, redirect, http.StatusTemporaryRedirect) | ||
| } | ||
| } | ||
| type OIDCConfig struct { | ||
87 changes: 87 additions & 0 deletionscoderd/userauth_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -22,6 +22,7 @@ import ( | ||||||||||||||||||||||||||||||||||||||||
| "github.com/prometheus/client_golang/prometheus" | ||||||||||||||||||||||||||||||||||||||||
| "github.com/stretchr/testify/assert" | ||||||||||||||||||||||||||||||||||||||||
| "github.com/stretchr/testify/require" | ||||||||||||||||||||||||||||||||||||||||
| "golang.org/x/oauth2" | ||||||||||||||||||||||||||||||||||||||||
| "golang.org/x/xerrors" | ||||||||||||||||||||||||||||||||||||||||
| "cdr.dev/slog" | ||||||||||||||||||||||||||||||||||||||||
| @@ -882,6 +883,92 @@ func TestUserOAuth2Github(t *testing.T) { | ||||||||||||||||||||||||||||||||||||||||
| require.Equal(t, user.ID, userID, "user_id is different, a new user was likely created") | ||||||||||||||||||||||||||||||||||||||||
| require.Equal(t, user.Email, newEmail) | ||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||
| t.Run("DeviceFlow", func(t *testing.T) { | ||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Just sharing some info. Our Here is an example external auth test: coder/coderd/externalauth_test.go Lines 269 to 287 indab3105
So we could write a test against a "live" idp server. This test uses mocked out responses for | ||||||||||||||||||||||||||||||||||||||||
| t.Parallel() | ||||||||||||||||||||||||||||||||||||||||
| client := coderdtest.New(t, &coderdtest.Options{ | ||||||||||||||||||||||||||||||||||||||||
| GithubOAuth2Config: &coderd.GithubOAuth2Config{ | ||||||||||||||||||||||||||||||||||||||||
| OAuth2Config: &testutil.OAuth2Config{}, | ||||||||||||||||||||||||||||||||||||||||
| AllowOrganizations: []string{"coder"}, | ||||||||||||||||||||||||||||||||||||||||
| AllowSignups: true, | ||||||||||||||||||||||||||||||||||||||||
| ListOrganizationMemberships: func(_ context.Context, _ *http.Client) ([]*github.Membership, error) { | ||||||||||||||||||||||||||||||||||||||||
| return []*github.Membership{{ | ||||||||||||||||||||||||||||||||||||||||
| State: &stateActive, | ||||||||||||||||||||||||||||||||||||||||
| Organization: &github.Organization{ | ||||||||||||||||||||||||||||||||||||||||
| Login: github.String("coder"), | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| }}, nil | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| AuthenticatedUser: func(_ context.Context, _ *http.Client) (*github.User, error) { | ||||||||||||||||||||||||||||||||||||||||
| return &github.User{ | ||||||||||||||||||||||||||||||||||||||||
| ID: github.Int64(100), | ||||||||||||||||||||||||||||||||||||||||
| Login: github.String("testuser"), | ||||||||||||||||||||||||||||||||||||||||
| Name: github.String("The Right Honorable Sir Test McUser"), | ||||||||||||||||||||||||||||||||||||||||
| }, nil | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| ListEmails: func(_ context.Context, _ *http.Client) ([]*github.UserEmail, error) { | ||||||||||||||||||||||||||||||||||||||||
| return []*github.UserEmail{{ | ||||||||||||||||||||||||||||||||||||||||
| Email: github.String("testuser@coder.com"), | ||||||||||||||||||||||||||||||||||||||||
| Verified: github.Bool(true), | ||||||||||||||||||||||||||||||||||||||||
| Primary: github.Bool(true), | ||||||||||||||||||||||||||||||||||||||||
| }}, nil | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| DeviceFlowEnabled: true, | ||||||||||||||||||||||||||||||||||||||||
| ExchangeDeviceCode: func(_ context.Context, _ string) (*oauth2.Token, error) { | ||||||||||||||||||||||||||||||||||||||||
| return &oauth2.Token{ | ||||||||||||||||||||||||||||||||||||||||
| AccessToken: "access_token", | ||||||||||||||||||||||||||||||||||||||||
| RefreshToken: "refresh_token", | ||||||||||||||||||||||||||||||||||||||||
| Expiry: time.Now().Add(time.Hour), | ||||||||||||||||||||||||||||||||||||||||
| }, nil | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| AuthorizeDevice: func(_ context.Context) (*codersdk.ExternalAuthDevice, error) { | ||||||||||||||||||||||||||||||||||||||||
| return &codersdk.ExternalAuthDevice{ | ||||||||||||||||||||||||||||||||||||||||
| DeviceCode: "device_code", | ||||||||||||||||||||||||||||||||||||||||
| UserCode: "user_code", | ||||||||||||||||||||||||||||||||||||||||
| }, nil | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||
| client.HTTPClient.CheckRedirect = func(*http.Request, []*http.Request) error { | ||||||||||||||||||||||||||||||||||||||||
| return http.ErrUseLastResponse | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| // Ensure that we redirect to the device login page when the user is not logged in. | ||||||||||||||||||||||||||||||||||||||||
| oauthURL, err := client.URL.Parse("/api/v2/users/oauth2/github/callback") | ||||||||||||||||||||||||||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||||||||||||||||||||||||||
| req, err := http.NewRequestWithContext(context.Background(), "GET", oauthURL.String(), nil) | ||||||||||||||||||||||||||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||||||||||||||||||||||||||
| res, err := client.HTTPClient.Do(req) | ||||||||||||||||||||||||||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||||||||||||||||||||||||||
| defer res.Body.Close() | ||||||||||||||||||||||||||||||||||||||||
| require.Equal(t, http.StatusTemporaryRedirect, res.StatusCode) | ||||||||||||||||||||||||||||||||||||||||
| location, err := res.Location() | ||||||||||||||||||||||||||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||||||||||||||||||||||||||
| require.Equal(t, "/login/device", location.Path) | ||||||||||||||||||||||||||||||||||||||||
| query := location.Query() | ||||||||||||||||||||||||||||||||||||||||
| require.NotEmpty(t, query.Get("state")) | ||||||||||||||||||||||||||||||||||||||||
| // Ensure that we return a JSON response when the code is successfully exchanged. | ||||||||||||||||||||||||||||||||||||||||
| oauthURL, err = client.URL.Parse("/api/v2/users/oauth2/github/callback?code=hey&state=somestate") | ||||||||||||||||||||||||||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||||||||||||||||||||||||||
| req, err = http.NewRequestWithContext(context.Background(), "GET", oauthURL.String(), nil) | ||||||||||||||||||||||||||||||||||||||||
| req.AddCookie(&http.Cookie{ | ||||||||||||||||||||||||||||||||||||||||
| Name: "oauth_state", | ||||||||||||||||||||||||||||||||||||||||
| Value: "somestate", | ||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||||||||||||||||||||||||||
| res, err = client.HTTPClient.Do(req) | ||||||||||||||||||||||||||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||||||||||||||||||||||||||
| defer res.Body.Close() | ||||||||||||||||||||||||||||||||||||||||
| require.Equal(t, http.StatusOK, res.StatusCode) | ||||||||||||||||||||||||||||||||||||||||
| var resp codersdk.OAuth2DeviceFlowCallbackResponse | ||||||||||||||||||||||||||||||||||||||||
| require.NoError(t, json.NewDecoder(res.Body).Decode(&resp)) | ||||||||||||||||||||||||||||||||||||||||
| require.Equal(t, "/", resp.RedirectURL) | ||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| // nolint:bodyclose | ||||||||||||||||||||||||||||||||||||||||
Oops, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
Oops, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.