Skip to content

Commit 9bc29db

Browse files
committed
chore: improve error handling for device code exchange failure (#11708)
* implement test using fake idp device flow * improve error handling around device code failure
1 parent aaca806 commit 9bc29db

File tree

3 files changed

+98
-6
lines changed

3 files changed

+98
-6
lines changed

coderd/coderdtest/oidctest/idp.go

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ import (
4141
"github.com/coder/coder/v2/coderd/promoauth"
4242
"github.com/coder/coder/v2/coderd/util/syncmap"
4343
"github.com/coder/coder/v2/codersdk"
44+
"github.com/coder/coder/v2/testutil"
4445
)
4546

4647
type token struct {
@@ -484,6 +485,31 @@ func (f *FakeIDP) ExternalLogin(t testing.TB, client *codersdk.Client, opts ...f
484485
_ = res.Body.Close()
485486
}
486487

488+
// DeviceLogin does the oauth2 device flow for external auth providers.
489+
func (*FakeIDP) DeviceLogin(t testing.TB, client *codersdk.Client, externalAuthID string) {
490+
// First we need to initiate the device flow. This will have Coder hit the
491+
// fake IDP and get a device code.
492+
device, err := client.ExternalAuthDeviceByID(context.Background(), externalAuthID)
493+
require.NoError(t, err)
494+
495+
// Now the user needs to go to the fake IDP page and click "allow" and enter
496+
// the device code input. For our purposes, we just send an http request to
497+
// the verification url. No additional user input is needed.
498+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
499+
defer cancel()
500+
resp, err := client.Request(ctx, http.MethodPost, device.VerificationURI, nil)
501+
require.NoError(t, err)
502+
defer resp.Body.Close()
503+
504+
// Now we need to exchange the device code for an access token. We do this
505+
// in this method because it is the user that does the polling for the device
506+
// auth flow, not the backend.
507+
err = client.ExternalAuthDeviceExchange(context.Background(), externalAuthID, codersdk.ExternalAuthDeviceExchange{
508+
DeviceCode: device.DeviceCode,
509+
})
510+
require.NoError(t, err)
511+
}
512+
487513
// CreateAuthCode emulates a user clicking "allow" on the IDP page. When doing
488514
// unit tests, it's easier to skip this step sometimes. It does make an actual
489515
// request to the IDP, so it should be equivalent to doing this "manually" with

coderd/externalauth/externalauth.go

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,11 @@ import (
66
"encoding/json"
77
"fmt"
88
"io"
9+
"mime"
910
"net/http"
1011
"net/url"
1112
"regexp"
13+
"strconv"
1214
"strings"
1315
"time"
1416

@@ -321,13 +323,31 @@ func (c *DeviceAuth) AuthorizeDevice(ctx context.Context) (*codersdk.ExternalAut
321323
}
322324
err = json.NewDecoder(resp.Body).Decode(&r)
323325
if err != nil {
324-
// Some status codes do not return json payloads, and we should
325-
// return a better error.
326-
switch resp.StatusCode {
327-
case http.StatusTooManyRequests:
328-
return nil, xerrors.New("rate limit hit, unable to authorize device. please try again later")
326+
mediaType, _, err := mime.ParseMediaType(resp.Header.Get("Content-Type"))
327+
if err != nil {
328+
mediaType = "unknown"
329+
}
330+
331+
// If the json fails to decode, do a best effort to return a better error.
332+
switch {
333+
case resp.StatusCode == http.StatusTooManyRequests:
334+
retryIn := "please try again later"
335+
resetIn := resp.Header.Get("x-ratelimit-reset")
336+
if resetIn != "" {
337+
// Best effort to tell the user exactly how long they need
338+
// to wait for.
339+
unix, err := strconv.ParseInt(resetIn, 10, 64)
340+
if err == nil {
341+
waitFor := time.Unix(unix, 0).Sub(time.Now().Truncate(time.Second))
342+
retryIn = fmt.Sprintf(" retry after %s", waitFor.Truncate(time.Second))
343+
}
344+
}
345+
// 429 returns a plaintext payload with a message.
346+
return nil, xerrors.New(fmt.Sprintf("rate limit hit, unable to authorize device. %s", retryIn))
347+
case mediaType == "application/x-www-form-urlencoded":
348+
return nil, xerrors.Errorf("status_code=%d, payload response is form-url encoded, expected a json payload", resp.StatusCode)
329349
default:
330-
return nil, fmt.Errorf("status_code=%d: %w", resp.StatusCode, err)
350+
return nil, fmt.Errorf("status_code=%d, mediaType=%s: %w", resp.StatusCode, mediaType, err)
331351
}
332352
}
333353
if r.ErrorDescription != "" {

coderd/externalauth_test.go

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"fmt"
66
"net/http"
77
"net/http/httptest"
8+
"net/url"
89
"regexp"
910
"strings"
1011
"testing"
@@ -264,6 +265,27 @@ func TestExternalAuthManagement(t *testing.T) {
264265

265266
func TestExternalAuthDevice(t *testing.T) {
266267
t.Parallel()
268+
// This is an example test on how to do device auth flow using our fake idp.
269+
t.Run("WithFakeIDP", func(t *testing.T) {
270+
t.Parallel()
271+
fake := oidctest.NewFakeIDP(t, oidctest.WithServing())
272+
externalID := "fake-idp"
273+
cfg := fake.ExternalAuthConfig(t, externalID, &oidctest.ExternalAuthConfigOptions{
274+
UseDeviceAuth: true,
275+
})
276+
277+
client := coderdtest.New(t, &coderdtest.Options{
278+
ExternalAuthConfigs: []*externalauth.Config{cfg},
279+
})
280+
coderdtest.CreateFirstUser(t, client)
281+
// Login!
282+
fake.DeviceLogin(t, client, externalID)
283+
284+
extAuth, err := client.ExternalAuthByID(context.Background(), externalID)
285+
require.NoError(t, err)
286+
require.True(t, extAuth.Authenticated)
287+
})
288+
267289
t.Run("NotSupported", func(t *testing.T) {
268290
t.Parallel()
269291
client := coderdtest.New(t, &coderdtest.Options{
@@ -363,6 +385,30 @@ func TestExternalAuthDevice(t *testing.T) {
363385
_, err := client.ExternalAuthDeviceByID(context.Background(), "test")
364386
require.ErrorContains(t, err, "rate limit hit")
365387
})
388+
389+
// If we forget to add the accept header, we get a form encoded body instead.
390+
t.Run("FormEncodedBody", func(t *testing.T) {
391+
t.Parallel()
392+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
393+
w.Header().Set("Content-Type", "application/x-www-form-urlencoded")
394+
_, _ = w.Write([]byte(url.Values{"access_token": {"hey"}}.Encode()))
395+
}))
396+
defer srv.Close()
397+
client := coderdtest.New(t, &coderdtest.Options{
398+
ExternalAuthConfigs: []*externalauth.Config{{
399+
ID: "test",
400+
DeviceAuth: &externalauth.DeviceAuth{
401+
ClientID: "test",
402+
CodeURL: srv.URL,
403+
Scopes: []string{"repo"},
404+
},
405+
}},
406+
})
407+
coderdtest.CreateFirstUser(t, client)
408+
_, err := client.ExternalAuthDeviceByID(context.Background(), "test")
409+
require.Error(t, err)
410+
require.ErrorContains(t, err, "is form-url encoded")
411+
})
366412
}
367413

368414
// nolint:bodyclose

0 commit comments

Comments
 (0)