From 9a553dfadc19e02b1bc8e075b2ec90aad8c0c281 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 19 Jan 2024 11:51:48 -0600 Subject: [PATCH 1/5] chore: work on unit test for device auth --- coderd/externalauth_test.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/coderd/externalauth_test.go b/coderd/externalauth_test.go index 17adfac69dcd7..fcc95415f3e5e 100644 --- a/coderd/externalauth_test.go +++ b/coderd/externalauth_test.go @@ -264,6 +264,32 @@ func TestExternalAuthManagement(t *testing.T) { func TestExternalAuthDevice(t *testing.T) { t.Parallel() + // This is an example test on how to do device auth flow using our fake idp. + 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) + device, err := client.ExternalAuthDeviceByID(context.Background(), externalID) + require.NoError(t, err) + + ctx := testutil.Context(t, testutil.WaitShort) + resp, err := client.Request(ctx, http.MethodPost, device.VerificationURI, nil) + require.NoError(t, err) + fmt.Println(resp.StatusCode) + + extAuth, err := client.ExternalAuthByID(context.Background(), externalID) + require.NoError(t, err) + require.True(t, extAuth.Authenticated) + }) + t.Run("NotSupported", func(t *testing.T) { t.Parallel() client := coderdtest.New(t, &coderdtest.Options{ From 33541e680cab378f15c8854a6397bed0edaf155b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 19 Jan 2024 12:08:04 -0600 Subject: [PATCH 2/5] implement test using fake idp --- coderd/coderdtest/oidctest/idp.go | 25 +++++++++++++++++++++++++ coderd/externalauth_test.go | 9 ++------- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/coderd/coderdtest/oidctest/idp.go b/coderd/coderdtest/oidctest/idp.go index 0238c41e5a0da..55963bdf8cfbb 100644 --- a/coderd/coderdtest/oidctest/idp.go +++ b/coderd/coderdtest/oidctest/idp.go @@ -41,6 +41,7 @@ import ( "github.com/coder/coder/v2/coderd/promoauth" "github.com/coder/coder/v2/coderd/util/syncmap" "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/testutil" ) type token struct { @@ -484,6 +485,30 @@ func (f *FakeIDP) ExternalLogin(t testing.TB, client *codersdk.Client, opts ...f _ = res.Body.Close() } +// DeviceLogin does the oauth2 device flow for external auth providers. +func (f *FakeIDP) DeviceLogin(t testing.TB, client *codersdk.Client, externalAuthID string) { + // First we need to initiate the device flow. This will have Coder hit the + // fake IDP and get a device code. + device, err := client.ExternalAuthDeviceByID(context.Background(), externalAuthID) + require.NoError(t, err) + + // Now the user needs to go to the fake IDP page and click "allow" and enter + // the device code input. For our purposes, we just send an http request to + // the verification url. No additional user input is needed. + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + _, err = client.Request(ctx, http.MethodPost, device.VerificationURI, nil) + require.NoError(t, err) + + // Now we need to exchange the device code for an access token. We do this + // in this method because it is the user that does the polling for the device + // auth flow, not the backend. + err = client.ExternalAuthDeviceExchange(context.Background(), externalAuthID, codersdk.ExternalAuthDeviceExchange{ + DeviceCode: device.DeviceCode, + }) + require.NoError(t, err) +} + // CreateAuthCode emulates a user clicking "allow" on the IDP page. When doing // unit tests, it's easier to skip this step sometimes. It does make an actual // request to the IDP, so it should be equivalent to doing this "manually" with diff --git a/coderd/externalauth_test.go b/coderd/externalauth_test.go index fcc95415f3e5e..bed1c79c7eccf 100644 --- a/coderd/externalauth_test.go +++ b/coderd/externalauth_test.go @@ -277,13 +277,8 @@ func TestExternalAuthDevice(t *testing.T) { ExternalAuthConfigs: []*externalauth.Config{cfg}, }) coderdtest.CreateFirstUser(t, client) - device, err := client.ExternalAuthDeviceByID(context.Background(), externalID) - require.NoError(t, err) - - ctx := testutil.Context(t, testutil.WaitShort) - resp, err := client.Request(ctx, http.MethodPost, device.VerificationURI, nil) - require.NoError(t, err) - fmt.Println(resp.StatusCode) + // Login! + fake.DeviceLogin(t, client, externalID) extAuth, err := client.ExternalAuthByID(context.Background(), externalID) require.NoError(t, err) From 7bf5f671daf59398cc11ca70afb2424c89dbaef5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 19 Jan 2024 12:27:18 -0600 Subject: [PATCH 3/5] improve error handling around device code failure --- coderd/externalauth/externalauth.go | 32 +++++++++++++++++++++++------ coderd/externalauth_test.go | 25 ++++++++++++++++++++++ 2 files changed, 51 insertions(+), 6 deletions(-) diff --git a/coderd/externalauth/externalauth.go b/coderd/externalauth/externalauth.go index 282c0d8a722b7..42c1f5306662b 100644 --- a/coderd/externalauth/externalauth.go +++ b/coderd/externalauth/externalauth.go @@ -6,9 +6,11 @@ import ( "encoding/json" "fmt" "io" + "mime" "net/http" "net/url" "regexp" + "strconv" "strings" "time" @@ -321,13 +323,31 @@ func (c *DeviceAuth) AuthorizeDevice(ctx context.Context) (*codersdk.ExternalAut } err = json.NewDecoder(resp.Body).Decode(&r) if err != nil { - // Some status codes do not return json payloads, and we should - // return a better error. - switch resp.StatusCode { - case http.StatusTooManyRequests: - return nil, xerrors.New("rate limit hit, unable to authorize device. please try again later") + mediaType, _, err := mime.ParseMediaType(resp.Header.Get("Content-Type")) + if err != nil { + mediaType = "unknown" + } + + // If the json fails to decode, do a best effort to return a better error. + switch { + case resp.StatusCode == http.StatusTooManyRequests: + retryIn := "please try again later" + resetIn := resp.Header.Get("x-ratelimit-reset") + if resetIn != "" { + // Best effort to tell the user exactly how long they need + // to wait for. + unix, err := strconv.ParseInt(resetIn, 10, 64) + if err == nil { + waitFor := time.Unix(unix, 0).Sub(time.Now().Truncate(time.Second)) + retryIn = fmt.Sprintf(" retry after %s", waitFor.Truncate(time.Second)) + } + } + // 429 returns a plaintext payload with a message. + return nil, xerrors.New(fmt.Sprintf("rate limit hit, unable to authorize device. %s", retryIn)) + case mediaType == "application/x-www-form-urlencoded": + return nil, xerrors.Errorf("%s payload response is form-url encoded, expected a json payload", resp.StatusCode) default: - return nil, fmt.Errorf("status_code=%d: %w", resp.StatusCode, err) + return nil, fmt.Errorf("status_code=%d, mediaType=%s: %w", resp.StatusCode, mediaType, err) } } if r.ErrorDescription != "" { diff --git a/coderd/externalauth_test.go b/coderd/externalauth_test.go index bed1c79c7eccf..db40ccf38a554 100644 --- a/coderd/externalauth_test.go +++ b/coderd/externalauth_test.go @@ -5,6 +5,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "net/url" "regexp" "strings" "testing" @@ -384,6 +385,30 @@ func TestExternalAuthDevice(t *testing.T) { _, err := client.ExternalAuthDeviceByID(context.Background(), "test") require.ErrorContains(t, err, "rate limit hit") }) + + // If we forget to add the accept header, we get a form encoded body instead. + t.Run("FormEncodedBody", func(t *testing.T) { + t.Parallel() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/x-www-form-urlencoded") + _, _ = w.Write([]byte(url.Values{"access_token": {"hey"}}.Encode())) + })) + defer srv.Close() + client := coderdtest.New(t, &coderdtest.Options{ + ExternalAuthConfigs: []*externalauth.Config{{ + ID: "test", + DeviceAuth: &externalauth.DeviceAuth{ + ClientID: "test", + CodeURL: srv.URL, + Scopes: []string{"repo"}, + }, + }}, + }) + coderdtest.CreateFirstUser(t, client) + _, err := client.ExternalAuthDeviceByID(context.Background(), "test") + require.Error(t, err) + require.ErrorContains(t, err, "is form-url encoded") + }) } // nolint:bodyclose From 2cd35843fd8ffce9375dea7874bfab4e0442be55 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 19 Jan 2024 12:46:56 -0600 Subject: [PATCH 4/5] linting --- coderd/coderdtest/oidctest/idp.go | 3 ++- coderd/externalauth/externalauth.go | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/coderd/coderdtest/oidctest/idp.go b/coderd/coderdtest/oidctest/idp.go index 55963bdf8cfbb..ff0abc267b376 100644 --- a/coderd/coderdtest/oidctest/idp.go +++ b/coderd/coderdtest/oidctest/idp.go @@ -497,8 +497,9 @@ func (f *FakeIDP) DeviceLogin(t testing.TB, client *codersdk.Client, externalAut // the verification url. No additional user input is needed. ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) defer cancel() - _, err = client.Request(ctx, http.MethodPost, device.VerificationURI, nil) + resp, err := client.Request(ctx, http.MethodPost, device.VerificationURI, nil) require.NoError(t, err) + defer resp.Body.Close() // Now we need to exchange the device code for an access token. We do this // in this method because it is the user that does the polling for the device diff --git a/coderd/externalauth/externalauth.go b/coderd/externalauth/externalauth.go index 42c1f5306662b..0c936743a0df5 100644 --- a/coderd/externalauth/externalauth.go +++ b/coderd/externalauth/externalauth.go @@ -345,7 +345,7 @@ func (c *DeviceAuth) AuthorizeDevice(ctx context.Context) (*codersdk.ExternalAut // 429 returns a plaintext payload with a message. return nil, xerrors.New(fmt.Sprintf("rate limit hit, unable to authorize device. %s", retryIn)) case mediaType == "application/x-www-form-urlencoded": - return nil, xerrors.Errorf("%s payload response is form-url encoded, expected a json payload", resp.StatusCode) + return nil, xerrors.Errorf("status_code=%d, payload response is form-url encoded, expected a json payload", resp.StatusCode) default: return nil, fmt.Errorf("status_code=%d, mediaType=%s: %w", resp.StatusCode, mediaType, err) } From 54113e4e891b7bbac8c353ba396a97c21d48047b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 19 Jan 2024 12:58:42 -0600 Subject: [PATCH 5/5] linting --- coderd/coderdtest/oidctest/idp.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/coderdtest/oidctest/idp.go b/coderd/coderdtest/oidctest/idp.go index ff0abc267b376..4a68eb3ea0b54 100644 --- a/coderd/coderdtest/oidctest/idp.go +++ b/coderd/coderdtest/oidctest/idp.go @@ -486,7 +486,7 @@ func (f *FakeIDP) ExternalLogin(t testing.TB, client *codersdk.Client, opts ...f } // DeviceLogin does the oauth2 device flow for external auth providers. -func (f *FakeIDP) DeviceLogin(t testing.TB, client *codersdk.Client, externalAuthID string) { +func (*FakeIDP) DeviceLogin(t testing.TB, client *codersdk.Client, externalAuthID string) { // First we need to initiate the device flow. This will have Coder hit the // fake IDP and get a device code. device, err := client.ExternalAuthDeviceByID(context.Background(), externalAuthID)