Skip to content

feat(auth): implement JWT logout with token revocation#58

Open
Gitkbc wants to merge 2 commits into
OneBusAway:mainfrom
Gitkbc:feature/logout-endpoint
Open

feat(auth): implement JWT logout with token revocation#58
Gitkbc wants to merge 2 commits into
OneBusAway:mainfrom
Gitkbc:feature/logout-endpoint

Conversation

@Gitkbc

@Gitkbc Gitkbc commented Mar 15, 2026

Copy link
Copy Markdown
Contributor

This PR adds logout support to the JWT authentication system by introducing token revocation. Building on the authentication foundation from PR #29, this update ensures tokens can be invalidated immediately upon logout rather than remaining active for their full 24-hour lifespan.

To achieve this, a jti claim was added to all issued JWTs, and a revoked_tokens database migration was introduced to act as a server-side blocklist. A new POST /api/v1/auth/logout endpoint handles the revocation, supported by new TokenChecker and TokenRevoker interfaces. The existing requireAuth middleware was also updated to intercept and reject any tokens found on this blocklist.

Test cases were also added to cover successful logouts and ensure revoked tokens are properly rejected.

- add jti claim to issued JWTs for unique token identification
- introduce revoked_tokens table via migration
- implement store methods for token revocation
- add POST /api/v1/auth/logout endpoint
- update requireAuth middleware to reject revoked tokens
- add TokenChecker and TokenRevoker interfaces
- extend authentication tests to cover logout and revoked token scenarios
@Gitkbc

Gitkbc commented Mar 15, 2026

Copy link
Copy Markdown
Contributor Author

While working on the authentication flow I noticed that the system currently supports login and logout, but there does not appear to be an API endpoint for creating users.

Would it make sense to add a user creation or signup endpoint in the future (for example POST /api/v1/auth/signup)?

Also regarding user creation, would you prefer a single endpoint where the role (e.g. driver, admin) is provided as a field in the request, or separate role-specific creation flows?

Another observation I had while reviewing the authentication flow: would introducing a username field (as an additional unique identifier alongside email) make sense for this system? In some cases users may have multiple emails and remembering a short username could be easier for login.

Happy to implement this in a follow-up PR if it aligns with the design.

@Gitkbc

Gitkbc commented Mar 16, 2026

Copy link
Copy Markdown
Contributor Author

While reviewing the auth implementation again, I noticed that auth.go was still using the standard log package while most of the codebase uses log/slog for structured logging.

I pushed a small refactor commit to align the auth logging with the structured logging approach, following the logging consistency discussion in #52.

@aaronbrethorst aaronbrethorst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Chaitanya, solid approach to token revocation here. The jti claim + database blocklist is the standard pattern for JWT logout, and you've wired it through cleanly — the TokenChecker and TokenRevoker interfaces keep the middleware testable, the ON CONFLICT DO NOTHING in RevokeToken makes double-logout idempotent, and the TestLoginLogoutRevokeFlow integration test walks through the full lifecycle convincingly. The log.Printfslog cleanup is a nice bonus.

Critical

  1. Migration number conflict (migrations/000006_add_revoked_tokens.{up,down}.sql). The branch is based on an older main that only had migrations 000001–000003. Since then, 000004_add_vehicle_received_index has landed on main. Your migration jumps straight to 000006, skipping 000004 and 000005. golang-migrate requires sequential numbering with no gaps. Please rebase on the latest main and renumber your migration to 000005.

Important

  1. No mechanism to clean up expired revoked tokens (migrations/000006_add_revoked_tokens.up.sql, store_revocation.go). The revoked_tokens table stores an expires_at column but nothing ever deletes rows past their expiry. Since every logout adds a row that's never removed, this table will grow indefinitely. You don't need to solve this in this PR, but the expires_at column is inert right now — please add a code comment in store_revocation.go noting that a periodic cleanup job (e.g. DELETE FROM revoked_tokens WHERE expires_at < NOW()) is needed as a follow-up.

  2. Revocation check silently skipped for tokens without jti (auth.go:162). If the jti claim is missing or empty, the if block at line 162 is skipped entirely and the request proceeds. This is fine for backwards compatibility with pre-existing tokens, but it means any token issued without a jti (e.g. by a test, a script, or a future code path that forgets to include it) can never be revoked. Please add a comment explaining that this is intentional for backwards compatibility with tokens issued before this change.

Suggestions

  1. Consider 204 No Content for logout (auth.go:208). The current 200 OK with {"message": "logged out successfully"} works, but logout is a void operation — the client doesn't need a response body. 204 No Content is the more conventional choice and saves the client from parsing a response it doesn't use. Not a blocker.

Strengths

  • Clean interface design: TokenChecker and TokenRevoker are minimal single-purpose interfaces that compose well with the existing UserFetcher pattern.
  • Thorough test suite: The TestLoginLogoutRevokeFlow test is particularly good — it walks through valid-token → logout → rejected-token in sequence, exactly how a real client would use this.
  • Idempotent revocation: ON CONFLICT (jti) DO NOTHING means double-logout doesn't error, which is the right behavior.
  • Structured logging migration: Replacing log.Printf with slog.Error/slog.Warn brings the auth code in line with the rest of the codebase.

Recommended Action

Request changes. Rebase on latest main and renumber migration to 000005. Add the comments noted in items 2 and 3.

@Gitkbc

Gitkbc commented Mar 26, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback, I’m working on the migration renumbering and the improvements suggested above.

I had one quick follow-up: would you be open to adding a username field (unique, alongside email) for login in a future PR?

I was thinking this could help reduce login friction a bit, especially in cases where users might not remember the exact email they used.From an implementation side, we could simply restrict usernames from containing @, so we can clearly decide whether to query by email or username. That way we still keep lookups efficient (indexed, O(log n)) and avoid OR-based queries.

It should also stay predictable from a security perspective since both fields would remain unique and unambiguous.
If this sounds reasonable, would you prefer keeping email as the primary identifier with username optional, or allowing login via either?

Happy to go with whatever direction you think fits best.

@aaronbrethorst

Copy link
Copy Markdown
Member

no, just email. please fix the merge conflicts when you have a chance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants