Skip to content

Commit b6878c2

Browse files
authored
fix: reject AT from banned users (#2642)
Reject requests from from banned users with still-valid access tokens. The following endpoints will be affected: ### User & session | Method | Path | Handler | |---|---|---| | `GET` | `/user` | `UserGet` | | `PUT` | `/user` | `UserUpdate` | | `GET` | `/reauthenticate` | `Reauthenticate` | | `POST` | `/logout` | `Logout` (now also blocked for banned users) | ### Identities & OAuth grants | Method | Path | Handler | |---|---|---| | `GET` | `/user/identities/authorize` | `LinkIdentity` | | `DELETE` | `/user/identities/{identity_id}` | `DeleteIdentity` | | `GET` | `/user/oauth/grants` | `UserListOAuthGrants` | | `DELETE` | `/user/oauth/grants` | `UserRevokeOAuthGrant` | | `POST` | `/token?grant_type=id_token` (with `link_identity=true`) | `IdTokenGrant` (via direct `requireAuthentication` call) | ### MFA / factors | Method | Path | Handler | |---|---|---| | `POST` | `/factors` | `EnrollFactor` | | `POST` | `/factors/{factor_id}/challenge` | `ChallengeFactor` | | `POST` | `/factors/{factor_id}/verify` | `VerifyFactor` | | `DELETE` | `/factors/{factor_id}` | `UnenrollFactor` | ### Passkeys | Method | Path | Handler | |---|---|---| | `POST` | `/passkeys/registration/options` | `PasskeyRegistrationOptions` | | `POST` | `/passkeys/registration/verify` | `PasskeyRegistrationVerify` | | `GET` | `/passkeys` | `PasskeyList` | | `PATCH` | `/passkeys/{passkey_id}` | `PasskeyUpdate` | | `DELETE` | `/passkeys/{passkey_id}` | `PasskeyDelete` | ### OAuth server (OIDC) | Method | Path | Handler | |---|---|---| | `GET` | `/oauth/userinfo` | `OAuthUserInfo` | | `GET` | `/oauth/authorizations/{authorization_id}` | `OAuthServerGetAuthorization` | | `POST` | `/oauth/authorizations/{authorization_id}/consent` | `OAuthServerConsent` |
1 parent 0fde049 commit b6878c2

2 files changed

Lines changed: 62 additions & 1 deletion

File tree

internal/api/auth.go

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,13 @@ func (a *API) requireAuthentication(w http.ResponseWriter, r *http.Request) (con
3232
if err != nil {
3333
return ctx, err
3434
}
35-
return ctx, err
35+
36+
// Reject banned users who still hold an access token issued before the ban
37+
if user := getUser(ctx); user != nil && user.IsBanned() {
38+
return ctx, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned")
39+
}
40+
41+
return ctx, nil
3642
}
3743

3844
func (a *API) requireNotAnonymous(w http.ResponseWriter, r *http.Request) (context.Context, error) {

internal/api/auth_test.go

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"net/http"
77
"net/http/httptest"
88
"testing"
9+
"time"
910

1011
"github.com/gofrs/uuid"
1112
jwt "github.com/golang-jwt/jwt/v5"
@@ -87,6 +88,9 @@ func TestExtractBearerTokenCaseInsensitive(t *testing.T) {
8788
}
8889

8990
func (ts *AuthTestSuite) TestParseJWTClaims() {
91+
originalJWT := ts.Config.JWT
92+
defer func() { ts.Config.JWT = originalJWT }()
93+
9094
cases := []struct {
9195
desc string
9296
key map[string]interface{}
@@ -312,3 +316,54 @@ func (ts *AuthTestSuite) TestMaybeLoadUserOrSession() {
312316
})
313317
}
314318
}
319+
320+
// TestRequireAuthenticationBannedUser verifies that a banned user holding a
321+
// still-valid access token is rejected by requireAuthentication, while
322+
// non-banned users and users whose ban has expired are allowed through.
323+
func (ts *AuthTestSuite) TestRequireAuthenticationBannedUser() {
324+
u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud)
325+
require.NoError(ts.T(), err)
326+
327+
newAuthedRequest := func() *http.Request {
328+
claims := &AccessTokenClaims{
329+
RegisteredClaims: jwt.RegisteredClaims{
330+
Subject: u.ID.String(),
331+
},
332+
Role: "authenticated",
333+
}
334+
userJwt, err := jwt.NewWithClaims(jwt.SigningMethodHS256, claims).SignedString([]byte(ts.Config.JWT.Secret))
335+
require.NoError(ts.T(), err)
336+
337+
req := httptest.NewRequest(http.MethodGet, "http://localhost", nil)
338+
req.Header.Set("Authorization", "Bearer "+userJwt)
339+
return req
340+
}
341+
342+
ts.Run("Banned user is rejected", func() {
343+
require.NoError(ts.T(), u.Ban(ts.API.db, time.Hour))
344+
345+
_, err := ts.API.requireAuthentication(httptest.NewRecorder(), newAuthedRequest())
346+
require.Error(ts.T(), err)
347+
require.Equal(ts.T(),
348+
apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned").Error(),
349+
err.Error())
350+
})
351+
352+
ts.Run("Expired ban is allowed", func() {
353+
pastBan := time.Now().Add(-time.Hour)
354+
u.BannedUntil = &pastBan
355+
require.NoError(ts.T(), ts.API.db.UpdateOnly(u, "banned_until"))
356+
357+
ctx, err := ts.API.requireAuthentication(httptest.NewRecorder(), newAuthedRequest())
358+
require.NoError(ts.T(), err)
359+
require.NotNil(ts.T(), getUser(ctx))
360+
})
361+
362+
ts.Run("Non-banned user is allowed", func() {
363+
require.NoError(ts.T(), u.Ban(ts.API.db, 0))
364+
365+
ctx, err := ts.API.requireAuthentication(httptest.NewRecorder(), newAuthedRequest())
366+
require.NoError(ts.T(), err)
367+
require.NotNil(ts.T(), getUser(ctx))
368+
})
369+
}

0 commit comments

Comments
 (0)