Skip to content

feat: failed update refresh should redirect to login #9442

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Aug 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion coderd/httpmw/apikey.go
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon
}).Token()
if err != nil {
return write(http.StatusUnauthorized, codersdk.Response{
Message: "Could not refresh expired Oauth token.",
Message: "Could not refresh expired Oauth token. Try re-authenticating to resolve this issue.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How will they reauthenticate other than manually wiping their cookies?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The frontend will intercept the "Unauthorized" status code and redirect to the login page. They shouldn't ever even see this message.

If they're using the CLI, they can just run coder login again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, duh lol

Detail: err.Error(),
})
}
Expand Down
2 changes: 1 addition & 1 deletion coderd/workspaceapps/apptest/apptest.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
resp, err := requestWithRetries(ctx, t, appDetails.AppClient(t), http.MethodGet, appDetails.PathAppURL(appDetails.Apps.Owner).String(), nil)
require.NoError(t, err)
defer resp.Body.Close()
require.Equal(t, http.StatusUnauthorized, resp.StatusCode)
require.Equal(t, http.StatusForbidden, resp.StatusCode)
body, err := io.ReadAll(resp.Body)
require.NoError(t, err)
require.Contains(t, string(body), "Path-based applications are disabled")
Expand Down
4 changes: 2 additions & 2 deletions coderd/workspaceapps/proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -256,8 +256,8 @@ func (s *Server) handleAPIKeySmuggling(rw http.ResponseWriter, r *http.Request,
func (s *Server) workspaceAppsProxyPath(rw http.ResponseWriter, r *http.Request) {
if s.DisablePathApps {
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
Status: http.StatusUnauthorized,
Title: "Unauthorized",
Status: http.StatusForbidden,
Title: "Forbidden",
Description: "Path-based applications are disabled on this Coder deployment by the administrator.",
RetryEnabled: false,
DashboardURL: s.DashboardURL.String(),
Expand Down
2 changes: 1 addition & 1 deletion coderd/workspaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
}

if organization.ID != template.OrganizationID {
httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
Message: fmt.Sprintf("Template is not in organization %q.", organization.Name),
})
return
Expand Down
2 changes: 1 addition & 1 deletion coderd/workspaces_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,7 @@ func TestPostWorkspacesByOrganization(t *testing.T) {
require.Error(t, err)
var apiErr *codersdk.Error
require.ErrorAs(t, err, &apiErr)
require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode())
require.Equal(t, http.StatusForbidden, apiErr.StatusCode())
})

t.Run("AlreadyExists", func(t *testing.T) {
Expand Down
55 changes: 49 additions & 6 deletions enterprise/coderd/userauth_test.go
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
package coderd_test

import (
"context"
"net/http"
"regexp"
"testing"

"github.com/golang-jwt/jwt/v4"
"github.com/stretchr/testify/require"
"golang.org/x/xerrors"

"github.com/coder/coder/v2/coderd"
"github.com/coder/coder/v2/coderd/coderdtest"
Expand Down Expand Up @@ -357,6 +359,40 @@ func TestUserOIDC(t *testing.T) {
runner.ForceRefresh(t, client, claims)
}
})

t.Run("FailedRefresh", func(t *testing.T) {
t.Parallel()

runner := setupOIDCTest(t, oidcTestConfig{
FakeOpts: []oidctest.FakeIDPOpt{
oidctest.WithRefreshHook(func(_ string) error {
// Always "expired" refresh token.
return xerrors.New("refresh token is expired")
}),
},
Config: func(cfg *coderd.OIDCConfig) {
cfg.AllowSignups = true
},
})

claims := jwt.MapClaims{
"email": "alice@coder.com",
}
// Login a new client that signs up
client, resp := runner.Login(t, claims)
require.Equal(t, http.StatusOK, resp.StatusCode)

// Expire the token, cause a refresh
runner.ExpireOauthToken(t, client)

// This should fail because the oauth token refresh should fail.
_, err := client.User(context.Background(), codersdk.Me)
require.Error(t, err)
var apiError *codersdk.Error
require.ErrorAs(t, err, &apiError)
require.Equal(t, http.StatusUnauthorized, apiError.StatusCode())
require.ErrorContains(t, apiError, "refresh")
})
})
}

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

type oidcTestConfig struct {
Userinfo jwt.MapClaims

// Config allows modifying the Coderd OIDC configuration.
Config func(cfg *coderd.OIDCConfig)
Config func(cfg *coderd.OIDCConfig)
FakeOpts []oidctest.FakeIDPOpt
}

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

fake := oidctest.NewFakeIDP(t,
oidctest.WithStaticUserInfo(settings.Userinfo),
oidctest.WithLogging(t, nil),
// Run fake IDP on a real webserver
oidctest.WithServing(),
append([]oidctest.FakeIDPOpt{
oidctest.WithStaticUserInfo(settings.Userinfo),
oidctest.WithLogging(t, nil),
// Run fake IDP on a real webserver
oidctest.WithServing(),
}, settings.FakeOpts...)...,
)

ctx := testutil.Context(t, testutil.WaitMedium)
Expand Down Expand Up @@ -665,5 +705,8 @@ func setupOIDCTest(t *testing.T, settings oidcTestConfig) *oidcTestRunner {
ForceRefresh: func(t *testing.T, client *codersdk.Client, idToken jwt.MapClaims) {
helper.ForceRefresh(t, api.Database, client, idToken)
},
ExpireOauthToken: func(t *testing.T, client *codersdk.Client) {
helper.ExpireOauthToken(t, api.Database, client)
},
}
}
6 changes: 1 addition & 5 deletions site/src/components/RequireAuth/RequireAuth.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { embedRedirect } from "../../utils/redirect"
import { FullScreenLoader } from "../Loader/FullScreenLoader"
import { DashboardProvider } from "components/Dashboard/DashboardProvider"
import { ProxyProvider } from "contexts/ProxyContext"
import { getErrorDetail } from "api/errors"

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

Expand Down
2 changes: 1 addition & 1 deletion site/src/pages/UsersPage/UsersPage.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,7 @@ describe("UsersPage", () => {
server.use(
rest.put(`/api/v2/users/${MockUser.id}/roles`, (req, res, ctx) => {
return res(
ctx.status(401),
ctx.status(400),
ctx.json({ message: "message from the backend" }),
)
}),
Expand Down
2 changes: 1 addition & 1 deletion site/src/xServices/auth/authXService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ export type AuthEvent =
| { type: "UPDATE_PROFILE"; data: TypesGen.UpdateUserProfileRequest }

export const authMachine =
/** @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 */
/** @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 */
createMachine(
{
id: "authState",
Expand Down