Open
Conversation
WalkthroughAdds user-based token revocation across the JWT stack: new Service.RevokeByUser, repository implementation to mark non-expired tokens revoked, metrics adjustments, a no-op disabled implementation, and wiring into the users service (revokes tokens, then updates password). Changes
Sequence DiagramsequenceDiagram
participant Client
participant UserService as User Service
participant JWTService as JWT Service
participant Repository
participant Database
participant Metrics
Client->>UserService: ChangePassword(userID, oldPwd, newPwd)
UserService->>JWTService: RevokeByUser(ctx, userID)
JWTService->>Repository: RevokeByUser(ctx, userID)
Repository->>Database: UPDATE tokens SET revoked_at = NOW() WHERE user_id=... AND revoked_at IS NULL AND expires_at > NOW()
Database-->>Repository: rows_affected
Repository-->>JWTService: count, error
JWTService->>Metrics: IncrementTokensRevoked("revoked", count)
Metrics-->>JWTService: ok
JWTService-->>UserService: error (if any)
alt no error
UserService->>Database: Update user password hash
Database-->>UserService: ok
UserService->>Client: success
else error
UserService->>Client: error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can customize the tone of the review comments and chat replies.Configure the |
🤖 Pull request artifacts
|
capcom6
force-pushed
the
codex/plan-jwt-token-invalidation-on-password-change
branch
2 times, most recently
from
e11005d to
bb1638a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/sms-gateway/jwt/repository.go (1)
112-114: Consider index coverage for this bulk-update predicate.This path can become hot on password changes. Ensure an index supports
user_id,revoked_at, andexpires_atto avoid table scans under load.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sms-gateway/jwt/repository.go` around lines 112 - 114, The bulk-update in repository.go using r.db.WithContext(...).Model((*tokenModel)(nil)).Where("user_id = ? and revoked_at is null and expires_at > NOW()", userID).Update(...) can cause table scans; add a supporting DB index for tokenModel to cover the predicate. Add a composite index on (user_id, revoked_at, expires_at) via your migrations/schema changes (or a partial index on user_id WHERE revoked_at IS NULL if your DB supports partial indexes) so the Update query can use an index and avoid full table scans under high load.internal/sms-gateway/jwt/metrics.go (1)
115-118: Consider stricter API design for counter increments to prevent misuse.The
IncrementTokensRevokedfunction accepts variadicintinput and forwards directly toCounter.Add(float64(...)). Since PrometheusCounter.Addpanics on negative values, the variadic signature creates a weak API surface.Current call sites are safe—they pass
RowsAffected(a database row count, always non-negative)—but defensive improvements would prevent future misuse. Consider either:
- Removing the variadic parameter entirely and requiring explicit calls to
Inc()orAdd(value)at the call site, or- Validating or rejecting negative values explicitly rather than silently dropping them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sms-gateway/jwt/metrics.go` around lines 115 - 118, Change the weak variadic API of Metrics.IncrementTokensRevoked to require an explicit non-variadic integer and validate it before calling the Prometheus counter: replace func (m *Metrics) IncrementTokensRevoked(status string, value ...int) with func (m *Metrics) IncrementTokensRevoked(status string, value int), then check value >= 0 and return or log/error if negative (do not call Counter.Add with negative), and finally call m.tokensRevokedCounter.WithLabelValues(status).Add(float64(value)); update call sites that previously passed RowsAffected to use the new signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/sms-gateway/jwt/service.go`:
- Around line 268-272: The error-path metric currently increments StatusError by
int(revoked) which is often zero and undercounts failures; change the logic to
compute the actual failed count (e.g., failed := attemptedTotal - revoked where
attemptedTotal is the total number of tokens you tried to revoke such as
len(ids) or the total variable used in the surrounding function) and call
s.metrics.IncrementTokensRevoked(StatusError, int(failed)) on err != nil, while
keeping s.metrics.IncrementTokensRevoked(StatusSuccess, int(revoked)) for the
success path; update the code around s.metrics.IncrementTokensRevoked,
StatusError, StatusSuccess, and the revoked variable to use the computed failed
value.
In `@internal/sms-gateway/users/service.go`:
- Around line 113-119: The password update and JWT revocation are non-atomic: if
s.users.UpdatePassword succeeds but s.jwtSvc.RevokeByUser fails the old tokens
remain valid; change the flow so the operation fails closed or becomes atomic —
either call s.jwtSvc.RevokeByUser(ctx, username) before committing the new
password and only persist the new hash if revocation succeeds, or perform both
steps inside a single transactional operation (e.g., add a transactional method
that invokes s.users.UpdatePassword and token revocation together and rolls back
the password update on revocation failure); ensure any temporary/transactional
API surfaces clearly reference s.users.UpdatePassword and s.jwtSvc.RevokeByUser
so failures return an error and do not leave old tokens valid.
---
Nitpick comments:
In `@internal/sms-gateway/jwt/metrics.go`:
- Around line 115-118: Change the weak variadic API of
Metrics.IncrementTokensRevoked to require an explicit non-variadic integer and
validate it before calling the Prometheus counter: replace func (m *Metrics)
IncrementTokensRevoked(status string, value ...int) with func (m *Metrics)
IncrementTokensRevoked(status string, value int), then check value >= 0 and
return or log/error if negative (do not call Counter.Add with negative), and
finally call m.tokensRevokedCounter.WithLabelValues(status).Add(float64(value));
update call sites that previously passed RowsAffected to use the new signature.
In `@internal/sms-gateway/jwt/repository.go`:
- Around line 112-114: The bulk-update in repository.go using
r.db.WithContext(...).Model((*tokenModel)(nil)).Where("user_id = ? and
revoked_at is null and expires_at > NOW()", userID).Update(...) can cause table
scans; add a supporting DB index for tokenModel to cover the predicate. Add a
composite index on (user_id, revoked_at, expires_at) via your migrations/schema
changes (or a partial index on user_id WHERE revoked_at IS NULL if your DB
supports partial indexes) so the Update query can use an index and avoid full
table scans under high load.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 52449b7d-080b-49a0-8258-2341f3ce7dc3
📒 Files selected for processing (6)
internal/sms-gateway/jwt/disabled.gointernal/sms-gateway/jwt/jwt.gointernal/sms-gateway/jwt/metrics.gointernal/sms-gateway/jwt/repository.gointernal/sms-gateway/jwt/service.gointernal/sms-gateway/users/service.go
capcom6
force-pushed
the
codex/plan-jwt-token-invalidation-on-password-change
branch
from
3bdbe3f to
ac00753
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/sms-gateway/jwt/disabled.go (1)
39-43: Inconsistent error behavior withRevokeToken.
RevokeByUserreturnsnilwhileRevokeToken(Line 37) returnsErrDisabled. If this is intentional to allow password changes when JWT is disabled, consider adding a comment explaining the design rationale. Otherwise, align the behavior with other methods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sms-gateway/jwt/disabled.go` around lines 39 - 43, The disabled JWT service has inconsistent behavior: RevokeByUser (method on type disabled) returns nil while RevokeToken returns ErrDisabled; either make RevokeByUser return ErrDisabled to match RevokeToken or, if the nil return is intentional to permit password changes when JWT is disabled, add an explanatory comment above the RevokeByUser method documenting that design choice and why it differs from RevokeToken. Ensure you reference the disabled type, RevokeByUser, RevokeToken, and ErrDisabled when making the change so behavior is consistent or clearly documented.internal/sms-gateway/jwt/repository.go (1)
109-119: Implementation looks correct.The query logic properly targets active tokens (non-revoked, non-expired) for the specified user. The pattern is consistent with the existing
Revokemethod.Consider adding a composite index on
(user_id, revoked_at, expires_at)to optimize this query if users accumulate many tokens. The current schema has separate indexes which may require index merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sms-gateway/jwt/repository.go` around lines 109 - 119, Add a composite index covering the tokenModel fields used in RevokeByUser (user_id, revoked_at, expires_at) to avoid index-merge overhead: update the tokenModel schema (or add a migration) to create an index on (user_id, revoked_at, expires_at) — e.g., add a GORM index tag on tokenModel or write a migration that creates idx_user_revoked_expires on those three columns and ensure it runs during schema migrations so the WHERE clause in RevokeByUser benefits from the composite index.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/sms-gateway/jwt/disabled.go`:
- Around line 39-43: The disabled JWT service has inconsistent behavior:
RevokeByUser (method on type disabled) returns nil while RevokeToken returns
ErrDisabled; either make RevokeByUser return ErrDisabled to match RevokeToken
or, if the nil return is intentional to permit password changes when JWT is
disabled, add an explanatory comment above the RevokeByUser method documenting
that design choice and why it differs from RevokeToken. Ensure you reference the
disabled type, RevokeByUser, RevokeToken, and ErrDisabled when making the change
so behavior is consistent or clearly documented.
In `@internal/sms-gateway/jwt/repository.go`:
- Around line 109-119: Add a composite index covering the tokenModel fields used
in RevokeByUser (user_id, revoked_at, expires_at) to avoid index-merge overhead:
update the tokenModel schema (or add a migration) to create an index on
(user_id, revoked_at, expires_at) — e.g., add a GORM index tag on tokenModel or
write a migration that creates idx_user_revoked_expires on those three columns
and ensure it runs during schema migrations so the WHERE clause in RevokeByUser
benefits from the composite index.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: afb15f67-4417-4dcb-81ce-6dd80b878f38
📒 Files selected for processing (6)
internal/sms-gateway/jwt/disabled.gointernal/sms-gateway/jwt/jwt.gointernal/sms-gateway/jwt/metrics.gointernal/sms-gateway/jwt/repository.gointernal/sms-gateway/jwt/service.gointernal/sms-gateway/users/service.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/sms-gateway/jwt/service.go
- internal/sms-gateway/jwt/jwt.go
capcom6
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac00753ce2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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.
Motivation
Description
RevokeAllByUser(ctx, userID string)to thejwt.Serviceinterface and a repository implementation that updatesrevoked_atfor active tokens matchinguser_id.service.RevokeAllByUserto forward to the repository with metrics and a no-op implementation in the disabled service.TokenRevokerinterface, accepting it inusers.NewService, and callingtokenRevoker.RevokeAllByUserafter updating the password.docs/jwt-password-change-invalidation-plan.mddescribing the approach, SQL snippet, trade-offs, observability, and testing plan.Testing
Codex Task
Summary by CodeRabbit