Repository navigation
feat(auth): add active sessions / device management - #59
Conversation
Adds a server-side Session table so refresh-token sessions can be listed and individually revoked, instead of being purely stateless JWTs with no visibility or per-device revocation. - Both access and refresh JWTs now carry a sid (session id) claim, minted once at register/login and reused across refreshes (TouchSessionUseCase slides the session's expiry forward on each refresh, mirroring the refresh JWT's own sliding expiry) - New sessions query and revokeSession/revokeOtherSessions mutations, backed by CreateSessionUseCase/TouchSessionUseCase/ListSessionsUseCase/ RevokeSessionUseCase/RevokeOtherSessionsUseCase - logout now best-effort revokes the current session server-side (in addition to clearing cookies), so a cleared-from-browser refresh token can't be replayed - New Active sessions section in account.tsx: device/IP/last-active per session, a "This device" badge, per-row revoke, and "Sign out other sessions" - Verified end-to-end against a live server with an isolated throwaway DB: registered from one device, logged in again from a second device, listed both sessions with correct device/current flags, revoked the second session, confirmed its refreshToken call now fails with UNAUTHORIZED while the first session's refresh still succeeds
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (36)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
mankatcheung
left a comment
There was a problem hiding this comment.
Overview
Adds a server-side Session table so refresh tokens can be listed/revoked per device, threads a sid claim through both access and refresh JWTs, and exposes sessions / revokeSession / revokeOtherSessions in GraphQL plus a new "Active sessions" panel in the account page. Clean Architecture layering is followed correctly: domain entity (Session) → port (ISessionRepository) → 5 focused use cases → PrismaSessionRepository → SessionMapper/SessionType → resolvers/schema fields → DI wiring in container.ts. Test coverage is strong (use-case, repository-against-real-sqlite, mapper, and updated AuthResolver/FastifyJwtTokenService tests).
Code Quality
- Layering and file size are consistent with project conventions — each use case is single-purpose and small.
SESSION.TTL_MSderiving fromCOOKIE_MAX_AGE_S.REFRESH_TOKENto keep the DB-tracked expiry and the refresh JWT's sliding expiry in lockstep is a nice touch — avoids a whole class of drift bugs.logoutrevoking the session server-side (best-effort, swallowed failure) in addition to clearing cookies is the right call and is explicitly tested.- Minor:
PrismaSessionRepositoryhand-rolls aPrismaSessionrow type identical to the generated Prisma model shape — could just importPrisma.Sessionfrom@prisma/clientto avoid drift if the schema changes, but not a real issue today.
Issues & Risks
Authorization / IDOR — no issues found. revokeSession uses findByIdAndUserId(sessionId, userId) before revoking (404s if the session isn't owned by the caller — covered by a test), revokeOtherSessions scopes to ctx.user.sub, and the sessions query only ever lists the caller's own sessions. All three correctly derive the user id from the verified JWT context, never from client input.
Revocation is enforced server-side, but only at the refresh-token boundary. TouchSessionUseCase checks the Session row (revokedAt/expiresAt) on every refreshToken call and rejects with UNAUTHORIZED if revoked — so a revoked session can no longer mint new tokens, confirmed by the manual e2e test in the PR description. However, the existing 15-minute access token for a just-revoked session is not itself checked against the DB (app.ts's context just verifies the JWT signature/expiry) — it remains valid for up to its natural 15m TTL after revocation. This is a standard/acceptable stateless-access + stateful-refresh tradeoff given the short access TTL, but worth calling out explicitly since "revoke this device now" is the exact feature being built — a user revoking a stolen/lost device's session should ideally understand there's up to a 15-minute window where that device can still act. Not blocking, but consider a one-line note in the UI copy or PR/changelog.
Migration edge case: pre-existing refresh tokens have no sid claim. Any refresh token issued before this deploy will decode with sid: undefined (the TS type on ITokenService.verifyRefresh/JwtUser claims sid is always a string, but that's not runtime-enforced for tokens signed by the old code path). AuthResolver.refreshToken then calls touchSessionUseCase.execute(undefined) → sessionRepository.findById(undefined). TouchSessionUseCase only guards on !session, so this depends on Prisma treating an undefined unique-lookup key as "not found" rather than throwing a validation error; if it throws, the caller gets a generic INTERNAL_ERROR (confirmed via formatError.ts/AppError.ts — no sensitive data leaks either way) instead of a clean UNAUTHORIZED/"please log in again". Practically this only affects the one-time cutover (every currently-logged-in user's next refresh after this deploy), and the failure mode is "ugly error, user re-logs in" rather than anything security-sensitive — but it's untested and worth an explicit guard:
if (!sessionId) throw Object.assign(new Error('Session revoked or expired'), { code: ERROR_CODES.UNAUTHORIZED });at the top of TouchSessionUseCase.execute, with a test for the "no sid" case.
Suggestions
- Add the
sessionIdguard above and a corresponding test (missing/undefinedsid→ cleanUNAUTHORIZED, not an uncaught error). - Consider a thin resolver/schema-level test for
sessionQueries.ts/sessionMutations.ts(e.g. asserting the unauthenticatedctx.user === nullpath throwsUNAUTHORIZED, and thatrevokeOtherSessionsthrows whenctx.user.sidis absent for API-token auth) — currently only exercised indirectly via use-case tests andAuthResolvertests, not the schema fields themselves. PrismaSessionRepository's hand-written row type could reference the generated Prisma model type instead, to stay in sync automatically.- Cosmetic: the account page shows the current session's device string and a static "This device" label but no visual distinction if two sessions share the same
userAgent(e.g. same browser on two machines) beyond the IP — fine for a v1, just worth knowing the device fingerprint is coarse.
Test Coverage
Thorough at the use-case and repository layers: all 5 session use cases (including revoked/expired/not-found branches), PrismaSessionRepository against a real in-memory SQLite DB (including cross-user isolation), SessionMapper, and the sid-aware FastifyJwtTokenService/AuthResolver changes (including the "session revoked mid-refresh" case). Web-side AccountPage tests cover current-vs-other-session rendering, individual revoke, and "sign out other sessions". Gaps are limited to the schema/resolver-boundary auth checks and the legacy-token migration path noted above.
Verdict
No IDOR, and revocation is genuinely enforced server-side where it matters (refresh-token issuance), not just a DB flag ignored by the auth layer. The two issues raised are a real but self-healing migration edge case and a documented, industry-standard short access-token revocation-lag — neither rises to a merge blocker. Approving, with the TouchSessionUseCase guard + test as a strongly recommended fast-follow (or fold into this PR before merging if convenient).
Summary
Sessiontable so refresh-token sessions can be listed and individually revoked — previously refresh tokens were purely stateless JWTs with zero visibility or per-device revocationsid(session id) claim, minted once at register/login and reused across refreshes —TouchSessionUseCaseslides the session'sexpiresAtforward on each refresh, mirroring the refresh JWT's own sliding expiry, so the two never drift out of syncsessionsquery andrevokeSession/revokeOtherSessionsmutations, backed byCreateSessionUseCase/TouchSessionUseCase/ListSessionsUseCase/RevokeSessionUseCase/RevokeOtherSessionsUseCaselogoutnow best-effort revokes the current session server-side in addition to clearing cookies, so a cleared-from-browser refresh token can't be replayedaccount.tsx: device/IP/last-active per session, a "This device" badge, per-row revoke, and "Sign out other sessions"Fixes JEF-19
Test plan
pnpm typecheck(api + web)pnpm lint(api + web)pnpm test— 426 API tests + 77 web tests passing, including new coverage for all 5 session use cases,PrismaSessionRepository,SessionMapper, updatedAuthResolver/FastifyJwtTokenService, and the new Active sessions UIpnpm build(all packages)Device-A), logged in again from a second device (Device-B), confirmedsessionslists both with correct device/IP/currentflags, calledrevokeSessionon the second session, confirmed its subsequentrefreshTokencall now fails withUNAUTHORIZED("Session revoked or expired") while the first session'srefreshTokenstill succeedsNote for reviewers
This changes
ITokenService.sign()'s signature (now requires asessionIdargument) and makesAuthResolver.refreshTokenasync. Like #58, if another in-flight branch also touchesAuthResolver/authMutations.ts, there will be a small merge conflict to resolve.🤖 Generated with Claude Code
https://claude.ai/code/session_01DdEiNRTcUnM6kE5AdFQ3n8