- 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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
f4a8918 todab3105Compare@jaaydenh I've never made any changes to the frontend before. Would you like to see some storybooks or e2e tests added in this PR? |
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.
Overall LG. Some nits.
I did not look at the backend code.
I would like to see a test with the oidctest.FakeIDP, but not sure how difficult that would actually be to plug in.
Someone else should approve the FE
| @@ -1832,7 +1833,7 @@ func configureCAPool(tlsClientCAFile string, tlsConfig *tls.Config) error { | |||
| } | |||
| //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, allowSignups, allowEveryone bool, allowOrgs []string, rawTeams []string, enterpriseBaseURL string) (*coderd.GithubOAuth2Config, error) { | |||
| 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 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
cli/server.go Outdated
| createDeviceAuth := func() *externalauth.DeviceAuth { | ||
| return &externalauth.DeviceAuth{ | ||
| Config: instrumentedOauth, | ||
| ClientID: clientID, | ||
| TokenURL: endpoint.TokenURL, | ||
| Scopes: []string{"read:user", "read:org", "user:email"}, | ||
| CodeURL: endpoint.DeviceAuthURL, | ||
| } | ||
| } |
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.
Do you know if this has to be a new instance for each auth flow? If so, can you explain in a comment here?
As opposed to just
deviceAuth:=&externalauth.DeviceAuth{Config:instrumentedOauth,ClientID:clientID,TokenURL:endpoint.TokenURL,Scopes: []string{"read:user","read:org","user:email"},CodeURL:endpoint.DeviceAuthURL,}
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.
I think it doesn't have to be a new instance. I mirrored what we were doing for the GitHub client without understanding why we did it. I changed it to your suggestion now.
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.
I did not have a strong opinion either way. Just curious 👍
| 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 | ||
| } |
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.
Is this error comparison correct?
Comparing to the usage in the oauth lib, it is checking theErrorCode field of theRetreiveError.
Usage:https://github.com/golang/oauth2/blob/master/deviceauth.go#L189-L190
Error type:https://github.com/golang/oauth2/blob/master/token.go#L187-L198
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 actually don't use theDeviceAccessToken method from the oauth lib. We have a custom implementation of ExchangeDeviceCodewhich we use for the device flow. That implementationcallscodersdk.ReadBodyAsError on an HTTP response it gets from GitHub, which populates the detail field with"authorization_pending". I verified that the check is valid.
For context, we use a custom implementation instead of the lib'sDeviceAccessToken because the latter keeps retrying until the code is successfully exchanged. We keep the retry logic on the frontend. That way you don't have an HTTP call that hangs for what can be minutes until the user authenticates with GitHub.
| if !c.DeviceFlowEnabled { | ||
| return c.OAuth2Config.AuthCodeURL(state, opts...) | ||
| } | ||
| return "/login/device?state=" + state |
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.
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 becausedAuthCodeURL in a regular Oauth flow is a link to an external domain. So it's not obvious imo.
| // In the device flow, the redirect is handled client-side. | ||
| httpapi.Write(ctx, rw, http.StatusOK, codersdk.OAuth2DeviceFlowCallbackResponse{ | ||
| RedirectURL: redirect, | ||
| }) |
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.
👍
| @@ -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 comment
The reason will be displayed to describe this comment to others.Learn more.
Just sharing some info. Ouroidctest.FakeIDP supports device auth flow.
Here is an example external auth test:
coder/coderd/externalauth_test.go
Lines 269 to 287 indab3105
| t.Run("WithFakeIDP",func(t*testing.T) { | |
| t.Parallel() | |
| fake:=oidctest.NewFakeIDP(t,oidctest.WithServing()) | |
| externalID:="fake-idp" | |
| cfg:=fake.ExternalAuthConfig(t,externalID,&oidctest.ExternalAuthConfigOptions{ | |
| UseDeviceAuth:true, | |
| }) | |
| client:=coderdtest.New(t,&coderdtest.Options{ | |
| ExternalAuthConfigs: []*externalauth.Config{cfg}, | |
| }) | |
| coderdtest.CreateFirstUser(t,client) | |
| // Login! | |
| fake.DeviceLogin(t,client,externalID) | |
| extAuth,err:=client.ExternalAuthByID(context.Background(),externalID) | |
| require.NoError(t,err) | |
| require.True(t,extAuth.Authenticated) | |
| }) |
So we could write a test against a "live" idp server. This test uses mocked out responses forExchangeDeviceCode andAuthorizeDevice. It does not test any of the oauth logic behind that.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
@hugodutka Regarding some of the comments about the FE regarding Tailwind and Material UI components. It looks you were modifying existing code more than adding new code. I would say its preferable to make the changes but it makes sense to not do it now if you feel your are just modifying existing code. |
jaaydenh commentedFeb 19, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
It would make sense to add storybook tests for the pageview that has been added: Playwright e2e tests would make sense as well if you have a good way to test this. Both of these could be done in a separate PR to keep things moving. Is there a simple enough way to test the Github device flow locally? |
I indeed just copy pasted and modified
Great, I'll do that in a separate PR.
Yes, run the server like this: |
dab3105 tobe8cca1Comparebe8cca1 tocfc3645Compare8c5e700 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
First PR in a series to address#16230.
Introduces support for logging in via theGitHub OAuth2 Device Flow.
It's previously been possible to configure external auth with the device flow, but it's not been possible to use it for logging in. This PR builds on the existing support we had to extend it to sign ins.
When a user clicks "sign in with GitHub" when device auth is configured, they are redirected to the new
/login/devicepage, which makes the flow possible from the client's side. The recording below shows the full flow.Screen.Recording.2025-02-17.at.19.49.52.mov
I've also manually tested that it works for converting from password-based auth to oauth.
Device auth can be enabled by a deployment's admin by setting the
CODER_OAUTH2_GITHUB_DEVICE_FLOWenv variable or a corresponding config setting.