feat: support the OAuth2 device flow with GitHub for signing in #16585

hugodutka · 2025-02-16T21:22:36Z

First PR in a series to address #16230.

Introduces support for logging in via the GitHub 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/device page, 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_FLOW env variable or a corresponding config setting.

hugodutka · 2025-02-19T16:20:13Z

@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?

Emyrk

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

Emyrk · 2025-02-19T16:53:37Z

cli/server.go

@@ -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) {


We should really make these args a struct soon. Not necessary in this PR, just a ton of options that can be misorderd.

Added a TODO

Emyrk · 2025-02-19T16:57:07Z

cli/server.go

+	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,
+		}
+	}


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, }

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.

I did not have a strong opinion either way. Just curious 👍

Emyrk · 2025-02-19T17:05:30Z

coderd/httpmw/oauth2.go

+				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
+				}


Is this error comparison correct?

Comparing to the usage in the oauth lib, it is checking the ErrorCode field of the RetreiveError.

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

We actually don't use the DeviceAccessToken method from the oauth lib. We have a custom implementation of ExchangeDeviceCode which we use for the device flow. That implementation calls codersdk.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's DeviceAccessToken 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.

Emyrk · 2025-02-19T17:07:46Z

coderd/userauth.go

+	if !c.DeviceFlowEnabled {
+		return c.OAuth2Config.AuthCodeURL(state, opts...)
+	}
+	return "/login/device?state=" + state


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 AuthCodeURL in a regular Oauth flow is a link to an external domain. So it's not obvious imo.

Emyrk · 2025-02-19T17:12:22Z

coderd/userauth.go

+		// In the device flow, the redirect is handled client-side.
+		httpapi.Write(ctx, rw, http.StatusOK, codersdk.OAuth2DeviceFlowCallbackResponse{
+			RedirectURL: redirect,
+		})


Emyrk · 2025-02-19T17:16:27Z

coderd/userauth_test.go

@@ -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) {


Just sharing some info. Our oidctest.FakeIDP supports device auth flow.

Here is an example external auth test:

coder/coderd/externalauth_test.go

Lines 269 to 287 in dab3105

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 for ExchangeDeviceCode and AuthorizeDevice. It does not test any of the oauth logic behind that.

site/src/components/GitDeviceAuth/GitDeviceAuth.tsx

site/src/pages/LoginOAuthDevicePage/LoginOAuthDevicePageView.tsx

jaaydenh · 2025-02-19T21:07:53Z

@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 · 2025-02-19T21:38:18Z

@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?

It would make sense to add storybook tests for the pageview that has been added:
LoginOAuthDevicePageView.tsx

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?

hugodutka · 2025-02-20T17:31:04Z

@jaaydenh

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.

I indeed just copy pasted and modified site/src/pages/ExternalAuthPage. I'd rather keep the refactoring to tailwind effort in a separate PR.

Both of these could be done in a separate PR to keep things moving.

Great, I'll do that in a separate PR.

Is there a simple enough way to test the Github device flow locally?

Yes, run the server like this:

export CODER_OAUTH2_GITHUB_DEVICE_FLOW=true
export CODER_OAUTH2_GITHUB_CLIENT_ID=Iv23liSBHklRMBNx5lk9
export CODER_OAUTH2_GITHUB_ALLOW_SIGNUPS=true
export CODER_OAUTH2_GITHUB_ALLOW_EVERYONE=true
go run cmd/coder/main.go server

hugodutka · 2025-02-21T17:42:07Z

Thank you for the reviews @jaaydenh @Emyrk! I'll add storybook tests, a fakeIDP test, and - if I find a good way to do it - an E2E test in a follow up PR.

github-actions bot assigned hugodutka Feb 16, 2025

hugodutka force-pushed the hugodutka/github-oauth2-device-flow branch 12 times, most recently from f4a8918 to dab3105 Compare February 19, 2025 16:06

hugodutka requested review from Emyrk and jaaydenh February 19, 2025 16:19

hugodutka marked this pull request as ready for review February 19, 2025 16:19

Emyrk approved these changes Feb 19, 2025

View reviewed changes