fix/session management using refresh token #1476
Conversation
Signed-off-by: sujitaw <[email protected]>
WalkthroughThe changes refactor session and token management across the API gateway and user services. They introduce new token data structures, modify JWT strategy to remove runtime token expiration checks, add session token update methods to the repository, and restructure the refresh token flow to use refresh tokens as the expiration source. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UserService
participant UserRepository
participant AuthService
participant TokenProvider
Client->>UserService: refreshTokenDetails(refreshToken)
rect rgb(230, 245, 255)
Note over UserService: Validate & Decode Phase
UserService->>UserService: decode(refreshToken)
UserService->>UserService: validate exp & sid
alt Token Expired
UserService->>UserRepository: deleteSession(sid)
UserService-->>Client: ❌ Unauthorized (expired)
end
end
rect rgb(240, 250, 230)
Note over UserService: Token Generation & Session Update Phase
UserService->>AuthService: requestNewAccessToken()
AuthService-->>UserService: newAccessToken
UserService->>UserRepository: getSession(sid)
UserRepository-->>UserService: existingSession
UserService->>UserService: decode(newAccessToken)
UserService->>UserService: derive expiresAt
UserService->>UserRepository: updateSessionToken(sid, tokenData)
UserRepository-->>UserService: ✓ updatedSession
end
UserService-->>Client: ✓ tokenResponse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (1)apps/user/src/user.service.ts (4)
🔇 Additional comments (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/client-registration/src/client-registration.service.ts (1)
178-181: Stop logging tokens, secrets, and sensitive payloads.Logs at these sites include client_secret, access/refresh tokens, or whole payloads/responses. This is a security and compliance risk.
- Remove or mask sensitive fields (client_secret, access_token, refresh_token).
- Prefer structured logs with redaction.
Example masking pattern:
this.logger.debug('Token response received', { endpoint: 'SAT', has_access_token: !!tokenResponse?.access_token, // do not log token values });Also applies to: 345-347, 695-696
apps/user/src/user.service.ts (1)
484-496: Guard decode of refresh_token to avoid NPE and fallback on refresh_expires_injwt.decode can return null/string; exp may be missing. Add a safe fallback.
Apply this diff:
- const decodedToken: any = jwt.decode(tokenDetails?.refresh_token); - const expiresAt = new Date(decodedToken.exp * 1000); + const decodedRefresh = jwt.decode(tokenDetails?.refresh_token) as jwt.JwtPayload | null; + const expiresAt = + decodedRefresh?.exp + ? new Date(decodedRefresh.exp * 1000) + : new Date(Date.now() + (tokenDetails?.refresh_expires_in ?? 0) * 1000);
🧹 Nitpick comments (8)
libs/client-registration/src/client-registration.service.ts (1)
21-23: DTO naming consistency (optional).accessTokenPayloadDto and userTokenPayloadDto are constructed via new but use lowerCamelCase. Nest/TS conventions prefer PascalCase for classes/DTOs (e.g., AccessTokenPayloadDto).
If feasible, rename classes/files to PascalCase to align with codebase conventions.
libs/common/src/interfaces/user.interface.ts (1)
9-9: Clarify units for refresh_expires_in.Is this seconds (Keycloak default) or milliseconds? Ensure it matches expires_in units and document to avoid misuse.
Add a brief JSDoc comment indicating the unit.
apps/user/interfaces/user.interface.ts (1)
305-310: Disambiguate expiry fields in ITokenData.Having both expires: number and expiresAt: Date is redundant and can drift. Define one source of truth or clearly document:
- expires: seconds since epoch?
- expiresAt: absolute Date?
Consider keeping only expiresAt (Date) or renaming expires → expiresInSeconds with clear units.
apps/api-gateway/common/interface.ts (2)
1-2: Avoid Prisma type leakage in gateway.Importing Prisma just for JsonValue couples the gateway to ORM internals.
Use a local JsonValue type (see apps/api-gateway/src/issuance/utils/helper.ts) or a shared union in a common lib.
16-28: DRY: ISession duplication across services.This ISession mirrors apps/user/interfaces/user.interface.ts. Duplicated contracts will drift.
Extract ISession into a shared lib (e.g., @credebl/common/interfaces) and import from there. Also replace Prisma.JsonValue with a shared JsonValue.
apps/user/src/user.controller.ts (1)
98-101: LGTM; add DTO validation for payload.Handler is fine. Consider a DTO with class-validator to enforce refreshToken presence/type.
Example:
export class GenerateAccessTokenUsingRefreshTokenDto { @IsString() refreshToken: string; }Then type payload accordingly.
apps/user/repositories/user.repository.ts (2)
732-745: Minimize exposure: select only needed fields for session cleanupThis method returns full session records “without any restriction,” including tokens. Prefer selecting only fields required by callers to reduce accidental leakage.
Apply this diff:
- async fetchUserSessionDetails(userId: string): Promise<ISession[]> { + async fetchUserSessionDetails(userId: string): Promise<ISession[]> { try { - const userSessionCount = await this.prisma.session.findMany({ - where: { - userId - } - }); - return userSessionCount; + const sessions = await this.prisma.session.findMany({ + where: { userId }, + select: { id: true, refreshToken: true, expiresAt: true } + }); + return sessions as unknown as ISession[]; } catch (error) { this.logger.error(`Error in getting user session details: ${error.message} `); throw error; } }
1005-1011: Good P2025 handling; align other methods for consistencyCatching Prisma P2025 and mapping to NotFound is good. Mirror this pattern in deleteSessionBySessionId for consistent behavior across repository methods.
Suggested change outside this range (deleteSessionBySessionId):
} catch (error) { if (error instanceof Prisma.PrismaClientKnownRequestError && error.code === 'P2025') { throw new RpcException(new NotFoundException(`Session not found for userId: ${userId}`)); } this.logger.error(`Error in Deleting Session: ${error.message}`); throw error; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/api-gateway/common/interface.ts(2 hunks)apps/api-gateway/src/authz/jwt.strategy.ts(0 hunks)apps/api-gateway/src/user/user.service.ts(2 hunks)apps/user/interfaces/user.interface.ts(1 hunks)apps/user/repositories/user.repository.ts(4 hunks)apps/user/src/user.controller.ts(1 hunks)apps/user/src/user.service.ts(3 hunks)libs/client-registration/src/client-registration.service.ts(1 hunks)libs/common/src/interfaces/user.interface.ts(1 hunks)libs/common/src/response-messages/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/api-gateway/src/authz/jwt.strategy.ts
🧰 Additional context used
🧬 Code graph analysis (5)
apps/api-gateway/src/user/user.service.ts (3)
apps/api-gateway/src/authz/authz.controller.ts (1)
refreshToken(328-337)apps/api-gateway/src/authz/authz.service.ts (1)
refreshToken(80-82)libs/common/src/interfaces/user.interface.ts (1)
ISignInUser(1-10)
apps/user/src/user.controller.ts (1)
libs/common/src/interfaces/user.interface.ts (1)
ISignInUser(1-10)
apps/api-gateway/common/interface.ts (2)
apps/user/interfaces/user.interface.ts (1)
ISession(185-197)apps/api-gateway/src/issuance/utils/helper.ts (1)
JsonValue(5-5)
apps/user/src/user.service.ts (5)
apps/api-gateway/src/authz/authz.controller.ts (2)
refreshToken(328-337)sessionDetails(223-241)apps/api-gateway/src/authz/authz.service.ts (1)
refreshToken(80-82)apps/api-gateway/src/authz/jwt-payload.interface.ts (1)
JwtPayload(1-13)libs/common/src/response-messages/index.ts (1)
ResponseMessages(1-519)libs/common/src/interfaces/user.interface.ts (1)
ISignInUser(1-10)
apps/user/repositories/user.repository.ts (3)
apps/api-gateway/common/interface.ts (1)
ISession(16-28)apps/user/interfaces/user.interface.ts (2)
ISession(185-197)ITokenData(305-310)libs/logger/src/logger.service.ts (1)
error(51-54)
🔇 Additional comments (3)
apps/api-gateway/common/interface.ts (1)
11-14: OK: ExceptionResponse typing tightened.apps/api-gateway/src/user/user.service.ts (2)
10-10: Import update looks good.
77-81: LGTM; confirm subject naming across services.Method correctly proxies to user service. Double-check we need both flows:
- refresh-token-details (existing)
- generate-accessToken-using-refresh-token (new)
If both remain, document their distinct purposes to avoid caller confusion.
Signed-off-by: sujitaw <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
libs/common/src/response-messages/index.ts (1)
76-76: Typo fix already flagged in previous review.The typo correction from
userSeesionNotFoundtouserSessionNotFoundwas already identified in a previous review comment.apps/user/src/user.service.ts (1)
540-583: Critical: Multiple security and reliability issues in refresh token flow.This method has several critical issues that remain from previous reviews:
- Line 540: Unsafe
jwt.decodewithout validation - can crash if token is malformed- Lines 548, 554: Logging PII (user details) and sensitive data (tokens, client secrets)
- Line 544: Session deletion not wrapped in try-catch - could mask
refreshTokenExpirederror- Line 577: Using
tokenResponse.session_stateinstead ofsessionDetails.idfor update - may fail or update wrong session- Lines 569-570: Unsafe decode of new refresh token without validation
Apply this comprehensive diff to address all issues:
- const data = jwt.decode(refreshToken) as jwt.JwtPayload; - const refreshTokenExp = new Date(data.exp * 1000); + const payload = jwt.decode(refreshToken) as jwt.JwtPayload | null; + if (!payload || typeof payload !== 'object' || !payload.exp || !payload.sid || !payload.sub) { + throw new UnauthorizedException(ResponseMessages.user.error.invalidRefreshToken); + } + const refreshTokenExp = new Date(payload.exp * 1000); const currentTime = new Date(); if (refreshTokenExp < currentTime) { - await this.userRepository.deleteSession(data?.sid); + // Best-effort cleanup; ignore not-found errors + try { + await this.userRepository.deleteSession(payload.sid as string); + } catch (err) { + // Ignore deletion errors (e.g., session already deleted) + } throw new UnauthorizedException(ResponseMessages.user.error.refreshTokenExpired); } - const userByKeycloakId = await this.userRepository.getUserByKeycloakId(data?.sub); - this.logger.debug(`User details::;${JSON.stringify(userByKeycloakId)}`); + const userByKeycloakId = await this.userRepository.getUserByKeycloakId(payload.sub as string); const tokenResponse = await this.clientRegistrationService.getAccessToken( refreshToken, userByKeycloakId?.['clientId'], userByKeycloakId?.['clientSecret'] ); - this.logger.debug(`tokenResponse::::${JSON.stringify(tokenResponse)}`); // Fetch the details from account table based on userid and refresh token const userAccountDetails = await this.userRepository.checkAccountDetails(userByKeycloakId?.['id']); // Update the account details with latest access token, refresh token and exp date if (!userAccountDetails) { throw new NotFoundException(ResponseMessages.user.error.userAccountNotFound); } // Fetch session details - - const sessionDetails = await this.userRepository.getSession(data.sid); + const sessionDetails = await this.userRepository.getSession(payload.sid as string); if (!sessionDetails) { - throw new NotFoundException(ResponseMessages.user.error.userSessionNotFound); + throw new NotFoundException(ResponseMessages.user.error.userSessionNotFound); } - - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const decodedToken: any = jwt.decode(tokenResponse?.refresh_token); - const expiresAt = new Date(decodedToken.exp * 1000); + const decodedRefresh = jwt.decode(tokenResponse?.refresh_token) as jwt.JwtPayload | null; + const expiresAt = + decodedRefresh?.exp + ? new Date(decodedRefresh.exp * 1000) + : new Date(Date.now() + (tokenResponse?.refresh_expires_in ?? 0) * 1000); const sessionData = { sessionToken: tokenResponse.access_token, expires: tokenResponse.expires_in, refreshToken: tokenResponse.refresh_token, expiresAt }; - const addSessionDetails = await this.userRepository.updateSessionToken(tokenResponse.session_state, sessionData); + const addSessionDetails = await this.userRepository.updateSessionToken(sessionDetails.id, sessionData); if (!addSessionDetails) { throw new InternalServerErrorException(ResponseMessages.user.error.errorInSessionCreation); } return tokenResponse;Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/user/repositories/user.repository.ts(4 hunks)apps/user/src/user.service.ts(3 hunks)libs/common/src/response-messages/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/user/repositories/user.repository.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/user/src/user.service.ts (3)
apps/api-gateway/src/authz/authz.controller.ts (1)
refreshToken(328-337)apps/api-gateway/src/authz/authz.service.ts (1)
refreshToken(80-82)apps/api-gateway/src/authz/jwt-payload.interface.ts (1)
JwtPayload(1-13)
🔇 Additional comments (2)
libs/common/src/response-messages/index.ts (1)
77-77: LGTM —refreshTokenExpiredis referenced in apps/user/src/user.service.ts:545.apps/user/src/user.service.ts (1)
455-455: deleteInactiveSessions correctly uses stored expiresAt
The repository implementation deletes sessions whereexpiresAt < new Date(), so it relies on the stored expiry timestamp rather than decoding tokens.
Signed-off-by: sujitaw <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
apps/user/src/user.service.ts (4)
542-548: Critical: Validate decoded refresh token before accessing claims.The code directly accesses
data.expanddata.sidwithout validating thatjwt.decodereturned a valid payload. If the token is malformed, this will crash.Apply this diff to add validation:
- const data = jwt.decode(refreshToken) as jwt.JwtPayload; - const refreshTokenExp = new Date(data.exp * 1000); + const payload = jwt.decode(refreshToken) as jwt.JwtPayload | null; + if (!payload || typeof payload !== 'object' || !payload.exp || !payload.sid || !payload.sub) { + throw new UnauthorizedException(ResponseMessages.user.error.invalidRefreshToken); + } + const refreshTokenExp = new Date(payload.exp * 1000); const currentTime = new Date(); if (refreshTokenExp < currentTime) { - await this.userRepository.deleteSession(data?.sid); + try { + await this.userRepository.deleteSession(payload.sid as string); + } catch (_) { + // Best-effort cleanup; ignore not-found errors + } throw new UnauthorizedException(ResponseMessages.user.error.refreshTokenExpired); } - const userByKeycloakId = await this.userRepository.getUserByKeycloakId(data?.sub); + const userByKeycloakId = await this.userRepository.getUserByKeycloakId(payload.sub as string);
550-550: Remove PII from logs.Logging the full user object exposes personally identifiable information (PII). Remove this debug log.
Apply this diff:
- this.logger.debug(`User details::;${JSON.stringify(userByKeycloakId)}`);
556-556: Remove sensitive token data from logs.Logging
tokenResponseexposes access tokens, refresh tokens, and client secrets. Remove this debug log.Apply this diff:
- this.logger.debug(`tokenResponse::::${JSON.stringify(tokenResponse)}`);
570-579: Critical: Validate decoded refresh token and use correct session ID.Two issues here:
- The decoded token is typed as
anyand accessed without validation.- Line 579 uses
tokenResponse.session_stateinstead ofsessionDetails.idto update the session.Apply this diff:
- // eslint-disable-next-line @typescript-eslint/no-explicit-any - const decodedToken: any = jwt.decode(tokenResponse?.refresh_token); - const expiresAt = new Date(decodedToken.exp * 1000); + const decodedRefresh = jwt.decode(tokenResponse?.refresh_token) as jwt.JwtPayload | null; + const expiresAt = + decodedRefresh?.exp + ? new Date(decodedRefresh.exp * 1000) + : new Date(Date.now() + (tokenResponse?.refresh_expires_in ?? 0) * 1000); const sessionData = { sessionToken: tokenResponse.access_token, expires: tokenResponse.expires_in, refreshToken: tokenResponse.refresh_token, expiresAt }; - const addSessionDetails = await this.userRepository.updateSessionToken(tokenResponse.session_state, sessionData); + const addSessionDetails = await this.userRepository.updateSessionToken(sessionDetails.id, sessionData);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/user/src/user.service.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/user/src/user.service.ts (4)
libs/common/src/response-messages/index.ts (1)
ResponseMessages(1-519)apps/api-gateway/src/authz/authz.controller.ts (1)
refreshToken(328-337)apps/api-gateway/src/authz/authz.service.ts (1)
refreshToken(80-82)apps/api-gateway/src/authz/jwt-payload.interface.ts (1)
JwtPayload(1-13)
🔇 Additional comments (1)
apps/user/src/user.service.ts (1)
565-568: Use consistent error message key.The error message key
userSessionNotFoundshould match the response messages definition. Based on the relevant code snippets, the key isuserSessionNotFound(notuserSeesionNotFound).
Signed-off-by: sujitaw <[email protected]>
Signed-off-by: sujitaw <[email protected]>
|
What
Summary by CodeRabbit
New Features
Bug Fixes
Improvements