-
Notifications
You must be signed in to change notification settings - Fork 914
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 { | ||
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ | ||
Message: "Internal error exchanging Oauth code.", | ||
Detail: err.Error(), | ||
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 | ||
} | ||
|
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) | ||
http.Redirect(rw, r, redirect, http.StatusTemporaryRedirect) | ||
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 { | ||
|
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 in dab3105
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 | ||||||||||||||||||||||||||||||||||||||||
|
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Added a TODO