Skip to content

Commit e827278

Browse files
EmyrkEmyrk
authored
feat: failed update refresh should redirect to login (#9442)
* chore: update refresh oauth token message * chore: unauthorized -> forbidden for non authentication failures * redirect to login on all 401 responses * add unit test to verify 401 on expired refresh
1 parent b9fbc54 commit e827278

File tree

9 files changed

+58
-19
lines changed

9 files changed

+58
-19
lines changed

‎coderd/httpmw/apikey.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -296,7 +296,7 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon
296296
}).Token()
297297
if err != nil {
298298
return write(http.StatusUnauthorized, codersdk.Response{
299-
Message: "Could not refresh expired Oauth token.",
299+
Message: "Could not refresh expired Oauth token. Try re-authenticating to resolve this issue.",
300300
Detail: err.Error(),
301301
})
302302
}

‎coderd/workspaceapps/apptest/apptest.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
125125
resp, err := requestWithRetries(ctx, t, appDetails.AppClient(t), http.MethodGet, appDetails.PathAppURL(appDetails.Apps.Owner).String(), nil)
126126
require.NoError(t, err)
127127
defer resp.Body.Close()
128-
require.Equal(t, http.StatusUnauthorized, resp.StatusCode)
128+
require.Equal(t, http.StatusForbidden, resp.StatusCode)
129129
body, err := io.ReadAll(resp.Body)
130130
require.NoError(t, err)
131131
require.Contains(t, string(body), "Path-based applications are disabled")

‎coderd/workspaceapps/proxy.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -256,8 +256,8 @@ func (s *Server) handleAPIKeySmuggling(rw http.ResponseWriter, r *http.Request,
256256
func (s *Server) workspaceAppsProxyPath(rw http.ResponseWriter, r *http.Request) {
257257
if s.DisablePathApps {
258258
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
259-
Status: http.StatusUnauthorized,
260-
Title: "Unauthorized",
259+
Status: http.StatusForbidden,
260+
Title: "Forbidden",
261261
Description: "Path-based applications are disabled on this Coder deployment by the administrator.",
262262
RetryEnabled: false,
263263
DashboardURL: s.DashboardURL.String(),

‎coderd/workspaces.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
359359
}
360360

361361
if organization.ID != template.OrganizationID {
362-
httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{
362+
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
363363
Message: fmt.Sprintf("Template is not in organization %q.", organization.Name),
364364
})
365365
return

‎coderd/workspaces_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -447,7 +447,7 @@ func TestPostWorkspacesByOrganization(t *testing.T) {
447447
require.Error(t, err)
448448
var apiErr *codersdk.Error
449449
require.ErrorAs(t, err, &apiErr)
450-
require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode())
450+
require.Equal(t, http.StatusForbidden, apiErr.StatusCode())
451451
})
452452

453453
t.Run("AlreadyExists", func(t *testing.T) {

‎enterprise/coderd/userauth_test.go‎

Lines changed: 49 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,14 @@
11
package coderd_test
22

33
import (
4+
"context"
45
"net/http"
56
"regexp"
67
"testing"
78

89
"github.com/golang-jwt/jwt/v4"
910
"github.com/stretchr/testify/require"
11+
"golang.org/x/xerrors"
1012

1113
"github.com/coder/coder/v2/coderd"
1214
"github.com/coder/coder/v2/coderd/coderdtest"
@@ -357,6 +359,40 @@ func TestUserOIDC(t *testing.T) {
357359
runner.ForceRefresh(t, client, claims)
358360
}
359361
})
362+
363+
t.Run("FailedRefresh", func(t *testing.T) {
364+
t.Parallel()
365+
366+
runner := setupOIDCTest(t, oidcTestConfig{
367+
FakeOpts: []oidctest.FakeIDPOpt{
368+
oidctest.WithRefreshHook(func(_ string) error {
369+
// Always "expired" refresh token.
370+
return xerrors.New("refresh token is expired")
371+
}),
372+
},
373+
Config: func(cfg *coderd.OIDCConfig) {
374+
cfg.AllowSignups = true
375+
},
376+
})
377+
378+
claims := jwt.MapClaims{
379+
"email": "alice@coder.com",
380+
}
381+
// Login a new client that signs up
382+
client, resp := runner.Login(t, claims)
383+
require.Equal(t, http.StatusOK, resp.StatusCode)
384+
385+
// Expire the token, cause a refresh
386+
runner.ExpireOauthToken(t, client)
387+
388+
// This should fail because the oauth token refresh should fail.
389+
_, err := client.User(context.Background(), codersdk.Me)
390+
require.Error(t, err)
391+
var apiError *codersdk.Error
392+
require.ErrorAs(t, err, &apiError)
393+
require.Equal(t, http.StatusUnauthorized, apiError.StatusCode())
394+
require.ErrorContains(t, apiError, "refresh")
395+
})
360396
})
361397
}
362398

@@ -576,14 +612,16 @@ type oidcTestRunner struct {
576612
// ForceRefresh will use an authenticated codersdk.Client, and force their
577613
// OIDC token to be expired and require a refresh. The refresh will use the claims provided.
578614
// It just calls the /users/me endpoint to trigger the refresh.
579-
ForceRefresh func(t *testing.T, client *codersdk.Client, idToken jwt.MapClaims)
615+
ForceRefresh func(t *testing.T, client *codersdk.Client, idToken jwt.MapClaims)
616+
ExpireOauthToken func(t *testing.T, client *codersdk.Client)
580617
}
581618

582619
type oidcTestConfig struct {
583620
Userinfo jwt.MapClaims
584621

585622
// Config allows modifying the Coderd OIDC configuration.
586-
Config func(cfg *coderd.OIDCConfig)
623+
Config func(cfg *coderd.OIDCConfig)
624+
FakeOpts []oidctest.FakeIDPOpt
587625
}
588626

589627
func (r *oidcTestRunner) AssertRoles(t *testing.T, userIdent string, roles []string) {
@@ -633,10 +671,12 @@ func setupOIDCTest(t *testing.T, settings oidcTestConfig) *oidcTestRunner {
633671
t.Helper()
634672

635673
fake := oidctest.NewFakeIDP(t,
636-
oidctest.WithStaticUserInfo(settings.Userinfo),
637-
oidctest.WithLogging(t, nil),
638-
// Run fake IDP on a real webserver
639-
oidctest.WithServing(),
674+
append([]oidctest.FakeIDPOpt{
675+
oidctest.WithStaticUserInfo(settings.Userinfo),
676+
oidctest.WithLogging(t, nil),
677+
// Run fake IDP on a real webserver
678+
oidctest.WithServing(),
679+
}, settings.FakeOpts...)...,
640680
)
641681

642682
ctx := testutil.Context(t, testutil.WaitMedium)
@@ -665,5 +705,8 @@ func setupOIDCTest(t *testing.T, settings oidcTestConfig) *oidcTestRunner {
665705
ForceRefresh: func(t *testing.T, client *codersdk.Client, idToken jwt.MapClaims) {
666706
helper.ForceRefresh(t, api.Database, client, idToken)
667707
},
708+
ExpireOauthToken: func(t *testing.T, client *codersdk.Client) {
709+
helper.ExpireOauthToken(t, api.Database, client)
710+
},
668711
}
669712
}

‎site/src/components/RequireAuth/RequireAuth.tsx‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import { embedRedirect } from "../../utils/redirect"
66
import { FullScreenLoader } from "../Loader/FullScreenLoader"
77
import { DashboardProvider } from "components/Dashboard/DashboardProvider"
88
import { ProxyProvider } from "contexts/ProxyContext"
9-
import { getErrorDetail } from "api/errors"
109

1110
export const RequireAuth: FC = () => {
1211
const [authState, authSend] = useAuth()
@@ -23,10 +22,7 @@ export const RequireAuth: FC = () => {
2322
// 401 Unauthorized
2423
// If we encountered an authentication error, then our token is probably
2524
// invalid and we should update the auth state to reflect that.
26-
if (
27-
error.response.status === 401 &&
28-
getErrorDetail(error)?.startsWith("API key expired")
29-
) {
25+
if (error.response.status === 401) {
3026
authSend("SIGN_OUT")
3127
}
3228

‎site/src/pages/UsersPage/UsersPage.test.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -447,7 +447,7 @@ describe("UsersPage", () => {
447447
server.use(
448448
rest.put(`/api/v2/users/${MockUser.id}/roles`, (req, res, ctx) => {
449449
return res(
450-
ctx.status(401),
450+
ctx.status(400),
451451
ctx.json({ message: "message from the backend" }),
452452
)
453453
}),

‎site/src/xServices/auth/authXService.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,7 @@ export type AuthEvent =
192192
| { type: "UPDATE_PROFILE"; data: TypesGen.UpdateUserProfileRequest }
193193

194194
export const authMachine =
195-
/** @xstate-layout N4IgpgJg5mDOIC5QEMCuAXAFgZXc9YAdADYD2yEAlgHZQCS1l6lyxAghpgCL7IDEEUtSI0AbqQDWRNFlz4iZCjXqNmrDlh54EY0gGN8lIQG0ADAF0z5xKAAOpWEyPUbIAB6IAjAFZThACwATAAcAGwAzOGewZ7hoYH+ADQgAJ6Igaam3oSeGf7BcbnBAJyBAL5lyTI4eAQk5FS0DE7qnFr8gsKEulKE1XJ1io0qLextvDrU4gbMJhbGntZIIPaOsy7LHgg+fkFhkdGx8Ump6bEA7ITn56HnkaFFpRVVnAMKDcrNamOavAJCIimkmkr1q7yUTVULB+3AmuhmzisxkCSzsDicQlcWx2ARCESiMTiCWSaQQgUC5124UCxQKDxCT0qIH6YPqEJG3w0sLwfDAACc+aQ+YRbMR8AAzIUAWz6oPkbOGX2hXPak2mhjmlgsrlWGI2oGxvlx+wJR2JpzJ-lM4UIphKgXuj3KTJZ8scUGEEAA8hg+Ng6ABxAByAH06EGrDr0essV5TOdslloqZPKZ-Od-NESV4Hjb-OESqFvAyEudgs9mXK6u7GJD-l0ekQawxI8tdTHNl5ybtPKnPKFin3ivns2TU3mi1FrmFSuEK67q5QPZ9qLyBUKRWL0JK+TLm9RW2i1s5Y9tu4Q8ZksiFgmnR93PLbad5h6FMgm5y6q02l56GH7A1DL0AFUABVDxWaMT07BAogHQhgmCfwBxvbwiwKe87WyYIM1ibxPHzG5PxeWRWRrSAGBFQVxUoYgRAgOi+GAgAFLg2FAgBRENmIAJS9AAxOgABkOIg9toINdIi0fcJzn7dMshfXtR2ibx-EIUJkOKbwiVw4p52-QhyIgSjbGo2iiFQWwIEMWhmPMxjOkBcRegXH8PQo6gqNIGi6MIKybOYOyHLANV9A1A95m1NsoMxGDPGfHIiPCfxvDSwcCJUwsci0nT4j0gzSLdX9PO83zLOs2yoHsnyLLXQVhVFCVpVlIrFw8kyvLM2q-ICqqavKsKEU1MTYv1dwvESzxktS9LexOUlonyHKBzyilM30r82vc2soB9dB62c4EjN-fbRuPOLJNghK-HJckX2KFLimHFTSkCQhZIKUwByQotnRImpiuXWh9sO7ogV6GszsWKMLvGrY4n8dSbn8bTfFyXx01e8JsLU580unG5CsB9rdtB-kGs3ZrdxOj0zuio89VPWTTHenHTEegpwm+nDwkwzSPuKIjEPOckkOJt5CD0IQaKgVA+WUUDMDAfjKD5WB0GA2B+QA4MwwjBnILh09b1CHIOdTN8Uoe+9pve0WhZKHHNJRiomWoUgIDgVw3NhpmYIAWm8YIAhxule1zfNilCFTi2yTno8pfICwpCXWSGFdRhVXg-Y7K6A9TUPg8KftYijmOLUOdTQhiDJShuVMEjToHPX23OJIm2CExDuT7QiLJEKy76L3wpDrmteJEOb0mV3by7O+jvwUodEI7hw-MK9JQIjgvGlSkQyI01Caeds8uf4a8fDiiuSk7VmpTMsr24bQIlDAluc5HruE-ab-LqQvPqeHwtwb6ZCQmlB+C0vA3D8C7FMCYdJxG8D-YypkQrdAYmAQBMFt7pg+nJOBc1PBZTUhpBS000r3GQVtEmp9OplQshgvyHsOLrj5Ngq629tL4PkpSIhr0ggrW0rpTMOEUElXod1cqTCiAUyFBwzuXDsiyV4YpDKmFC6v2EflUR5xxEdTQT1CqgVlADQsgo7EDxsjjzvhAjKUDtjZS0WtAqNDJY1mUG3GKxsYLLxtHcGkxwCIpnyPzNmelpoDnyHotxrJpbUFlvLRWytVbq01trdh3j-ZXTSn4BKyE7Q3hrsEbwttRZXBpGWR6aF0yaTdmUIAA */
195+
/** @xstate-layout N4IgpgJg5mDOIC5QEMCuAXAFgZXc9YAdADYD2yEAlgHZQCS1l6lyxAghpgCL7IDEEUtSI0AbqQDWRNFlz4iZCjXqNmrDlh54EY0gGN8lIQG0ADAF0z5xKAAOpWEyPUbIAB6IAjAFZThACwATAAcAGwAzOGewZ7hoYH+ADQgAJ6Igaam3oSeGf7BcbnBAJyBAL5lyTI4eAQk5FS0DE7qnFr8gsKEulKE1XJ1io0qLextvDrU4gbMJhbGntZIIPaOsy7LHgg+fkFhkdGx8Ump6bEA7ITn56HnkaFFpRVVnAMKDcrNamOavAJCIimkmkr1q7yUTVULB+3AmuhmzisxkCSzsDicQlcWx2ARCESiMTiCWSaQQgUC5124UCxQKDxCT0qIH6YPqEJG3w0sLwfDAACc+aQ+YRbMR8AAzIUAWz6oPkbOGX2hXPak2mhjmlgsrlWGI2oGxvlx+wJR2JpzJ-lM4UIphKgXuj3KTJZ8scUGEEAA8hg+Ng6ABxAByAH06EGrDr0essV5TOdslloqZPKZ-Od-NESV4Hjb-OESqFvAyEudgs9mXK6u7GJD-l0ekQawxI8tdTHNl5ybtPKnPKFin3ivns2TU3mi1FrmFSuEK67q5QPZ9qLyBUKRWL0JK+TLm9RW2i1s5Y9tu4Q8ZksiFgmnR93PLbad5h6FMgm5y6q02l56GH7A1DL0AFUABVDxWaMT07BAogHQhgmCfwBxvbwiwKe87WyYIM1ibxPHzG5PxeWRWRrSAGBFQVxUoYgRAgOi+GAgAFLg2FAgBRENmIAJS9AAxOgABkOIg9toINdIi0fcJzn7dMshfXtR2ibx-EIUJkOKbwiVw4p52-QhyIgSjbGo2iiFQWwIEMWhmPMxjOkBcRegXH8PQo6gqNIGi6MIKybOYOyHLANV9A1A95m1NsoMxGDPGfHIiPCfxvDSwcCJUwsci0nT4j0gzSLdX9PO83zLOs2yoHsnyLLXQVhVFCVpVlIrFw8kyvLM2q-ICqqavKsKEU1MTYv1dwvESzxktS9LexOUlonyHKBzyilM30r82vc2soB9dB62c4EjN-fbRuPOLJNghK-HJckX2KFLimHFTSkCQhZIKUwByQotnRImpiuXWh9sO7ogV6GszsWKMLvGrY4n8dSbn8bTfFyXx01e8JsLU580unG5CsB9rdtB-kGs3ZrdxOj0zuio89VPWTTHenHTEegpwm+nDwkwzSPuKIjEPOckkOJt5CD0IQaKgVA+WUUDMDAfjKD5WB0GA2B+QA4MwwjBnILh09b1CHIOdTN8Uoe+9pve0WhZKHHNJRiomWoUgIDgVw3NhpmYIAWlCFS3w04cCwyDmKWpYjK22hUV1GFVeD9jsroD1MAhxule1zfNimDi1DnU0IYgyUoblTBIJbIkrvQwVOJIm2CE2CK5olKCIskQrLvovfCkOua14kQmugd2hhG8u5uC78FKHRCO4cPzQvSUCI4LxpUpEMiNNQjH0nPKn+GvDiM3KW52dcO+vmLQSNuEvzRC0uQtDZIPnbSu68rj9PAiCKuNaKOslMw31HJEbIA5874SvOEbSH9aZ-i6iFboDEwC-3igXM28RfB2kCH9I4o4gg2kflaeSalyShH3ltEmn9OplQsqgvyHsOLrj5Bgq669tIfTki7RSGVXpBBWtpXSmYcIIOMqZFBlA0GEApkKDhzcuHZFkvJSkc1PCYUzgRVaojojnAkXXKRPUKqBWUANCyijsQPGyEPO0s0lKZSLtlHRIj8obUMcDPaDcYrGxgvPG0dwaTHAIimfI-M2Z6WmlA8RNDJbS2oLLeWitlaq3VprbW7DfH+yumlPwj83zBBvKXYI3hbaiyuDSMsj00Lpk0m7MoQA */
196196
createMachine(
197197
{
198198
id: "authState",

0 commit comments

Comments
 (0)