Skip to content

Commit

Permalink
Don't return 401 if refresh token is invalid, continue like unauthorized
Browse files Browse the repository at this point in the history
The user don't care if their refresh token expired, the right approach
is to treat them like unauthorized and either allow anonymous access or
redirect to the authorization endpoint.
  • Loading branch information
jirutka committed Oct 4, 2023
1 parent 00cee68 commit a74c19b
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 6 deletions.
5 changes: 4 additions & 1 deletion integration-tests/authorize.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,10 @@ describe('Authorize', () => {

when("I make a request to a secured page")

then("the response status should be {status}", 401)
then("the proxy should redirect me to $oidc_server_url/authorize", ({ resp, oauthServerUrl }) => {
assert(resp.statusCode === 303)
assert(resp.headers.location!.split('?')[0] === `${oauthServerUrl}/authorize`)
})

and("session variable {varName} should be cleared", Session.RefreshToken)
})
Expand Down
16 changes: 13 additions & 3 deletions src/handlers/auth-access.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,20 @@ export const auth_access: RequestHandler = async (ctx) => {
const refreshToken = vars[Session.RefreshToken]
if (refreshToken) {
log.info?.(`authorize: refreshing token for user ${getCookie(Cookie.Username)}`)
const { idToken } = await refreshTokens(ctx, refreshToken)

exposeClaims(ctx, idToken)
return authorizeAccess(ctx, idToken, conf)
const tokenSet = await refreshTokens(ctx, refreshToken).catch(err => {
if (err.status === 401) {
// The refresh token probably just expired, so let's act like the user
// is unauthenticated.
log.info?.(`authorize: invalid refresh token: ${err.detail ?? err.message}`)
} else {
throw err
}
})
if (tokenSet) {
exposeClaims(ctx, tokenSet.idToken)
return authorizeAccess(ctx, tokenSet.idToken, conf)
}
}

if (isAnonymousAllowed(conf)) {
Expand Down
14 changes: 12 additions & 2 deletions src/handlers/auth-pages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,19 @@ export const auth_pages: RequestHandler = async (ctx) => {
const refreshToken = vars[Session.RefreshToken]
if (refreshToken) {
log.info?.(`authorize: refreshing token for user ${getCookie(Cookie.Username)}`)
const { idToken } = await refreshTokens(ctx, refreshToken)

return await authorizeAccess(ctx, idToken, accessRule)
const tokenSet = await refreshTokens(ctx, refreshToken).catch(err => {
if (err.status === 401) {
// The refresh token probably just expired, so let's act like the user
// is unauthenticated.
log.info?.(`authorize: invalid refresh token: ${err.detail ?? err.message}`)
} else {
throw err
}
})
if (tokenSet) {
return await authorizeAccess(ctx, tokenSet.idToken, accessRule)
}
}

if (isAnonymousAllowed(accessRule)) {
Expand Down

0 comments on commit a74c19b

Please sign in to comment.