Conversation
The PKCE grant exchanged an auth code for a session without checking whether the user is banned. Banning a user only sets banned_until and does not remove pending flow states, so an auth code issued before the ban could still be exchanged afterwards for a new session and access token. Check IsBanned() after the code verifier is validated, returning the same user_banned error as the password grant, and add a test that bans the user between issuing and exchanging the auth code.
Author
|
this is ready for review let me know if there is need to make change. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
The PKCE grant (
POST /token?grant_type=pkce) never checks whether the user is banned.Every step that issues an auth code already rejects banned users (
/verify, the external provider callback, SAML ACS), and so do the password and refresh token grants. ButUser.Ban()only setsbanned_until; it does not remove the user's pending flow states. So an auth code issued before the ban can still be exchanged afterwards, within the flow state expiry (300s by default). That exchange creates a new session and returns an access token and a refresh token.The refresh token is rejected on first use, because the refresh grant checks
IsBanned(). The access token, however, stays valid until it expires (JWT_EXP, 3600s by default).What is the new behavior?
PKCE()returns400witherror_code: user_banned("User is banned") when the user is banned, matching the password grant. No session is created.The check runs after
VerifyPKCE, so a request that has the auth code but not the code verifier still getsbad_code_verifierand learns nothing about the account's ban status.Additional context
TestTokenPKCEGrantBannedUser: it starts a PKCE magic link sign in, follows/verifyto obtain the auth code, bans the user, then exchanges the code. Onmasterthe exchange returns200with tokens. With this change it returns400 user_banned, the session count is unchanged, and a wrong code verifier still returnsbad_code_verifier.go test ./internal/api/...(all packages pass),gofmt -s,go vet,staticcheck.