feat: add APIs for session listing and deletion #1449
Conversation
Signed-off-by: sujitaw <[email protected]>
Signed-off-by: sujitaw <[email protected]>
WalkthroughCapture client metadata (IP, raw UA, parsed OS/browser/device via express-useragent) during login and persist it to session.clientInfo; add JWT‑protected endpoints to list and delete user sessions; introduce Prisma schema migration, DTO/interface updates, repository/service/controller methods, and an express-useragent dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant G as API Gateway (AuthzController)
participant UA as express-useragent
participant S as AuthzService
participant UMS as User Service
participant DB as Prisma
U->>G: POST /auth/login { email, password }
G->>UA: parse User-Agent (middleware populates req.useragent)
UA-->>G: req.useragent (os, browser, device)
G->>S: login({ clientInfo: { ip, rawUA, useragent } }, email, password)
S->>UMS: create-login/session request (includes clientInfo)
UMS->>DB: create session { expiresAt, clientInfo }
DB-->>UMS: session
UMS-->>S: tokens + session info
S-->>G: response
G-->>U: 200 OK { tokens, user, session }
sequenceDiagram
autonumber
actor U as User
participant G as API Gateway
participant S as AuthzService
participant UMS as User Service
participant DB as Prisma
rect rgb(235,245,255)
note over U,G: Fetch Sessions
U->>G: GET /auth/:userId/sessions (JWT)
G->>S: userSessions(userId)
S->>UMS: session-details-by-userId
UMS->>DB: findMany(session by userId)
DB-->>UMS: IRestrictedUserSession[]
UMS-->>S: IRestrictedUserSession[]
S-->>G: IRestrictedUserSession[]
G-->>U: 200 OK
end
rect rgb(245,235,235)
note over U,G: Delete Session
U->>G: DELETE /auth/:sessionId/sessions (JWT)
G->>S: deleteSession(sessionId, req.user.id)
S->>UMS: delete-session-by-sessionId { sessionId, userId }
UMS->>DB: deleteMany({ id, userId })
alt deleted
DB-->>UMS: count>0
UMS-->>S: { message: "Session deleted Successfully" }
S-->>G: 200 OK
G-->>U: 200 OK
else not found
DB-->>UMS: count=0
UMS-->>S: RpcException(NotFound)
S-->>G: HttpException 404
G-->>U: 404 Not Found
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
Signed-off-by: sujitaw <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (10)
package.json (1)
117-117: Pin or constrain ua-parser-js to avoid unexpected breaking changesUsing a caret can pull in minor-breaking updates. Consider pinning or using a tilde and rely on Renovate/Dependabot for controlled upgrades.
- "ua-parser-js": "^2.0.5", + "ua-parser-js": "~2.0.5",libs/prisma-service/prisma/schema.prisma (1)
70-71: Storing clientInfo (JSON) — add privacy guardrails and plan indexing
- This likely includes PII (IP/UA). Define retention/redaction policy and avoid storing raw IPs where possible.
- If you’ll filter/group by keys (e.g., device, os), plan a GIN index via a raw SQL migration for jsonb to keep queries fast.
libs/common/src/response-messages/index.ts (1)
29-31: Normalize message casing for consistencyUse sentence case like existing messages.
- fetchAllSession: 'User Sessions fetched Successfully', - sessionDelete: 'Session deleted Successfully' + fetchAllSession: 'User sessions fetched successfully', + sessionDelete: 'Session deleted successfully'apps/user/repositories/user.repository.ts (2)
691-693: Default clientInfo sentinel may be confusing; consider null to mean “unknown”Storing a sentinel object can complicate downstream analytics. Prefer null when absent.
- ...(tokenDetails.clientInfo ? { clientInfo: tokenDetails.clientInfo } : { clientInfo: { clientToken: true } }) + ...(tokenDetails.clientInfo ? { clientInfo: tokenDetails.clientInfo } : { clientInfo: null })
979-994: Good ownership guard on delete; return message is fineFiltering by both sessionId and userId prevents cross-user deletion. Consider returning count for observability.
- async deleteSessionBySessionId(sessionId: string, userId: string): Promise<{ message: string }> { + async deleteSessionBySessionId(sessionId: string, userId: string): Promise<{ message: string; count: number }> { ... - return { message: 'Session deleted successfully' }; + return { message: 'Session deleted successfully', count: result.count };apps/user/dtos/login-user.dto.ts (1)
25-28: Type and example for clientInfoUse Prisma.JsonValue and provide a realistic JSON example.
- @ApiProperty({ example: 'false' }) + @ApiProperty({ example: { ip: '203.0.113.1', ua: 'Mozilla/5.0', os: 'macOS' } }) @IsOptional() - clientInfo?: JsonValue; + clientInfo?: Prisma.JsonValue;apps/api-gateway/src/authz/authz.service.ts (2)
54-57: Signature change: verify all call sites updated; prefer an object param to avoid ordering bugsPassing four positional args is brittle. Consider a single object param.
- async login(clientInfo: JsonValue, email: string, password?: string, isPasskey = false): Promise<ISignInUser> { - const payload = { email, password, isPasskey, clientInfo }; + async login(clientInfo: Prisma.JsonValue, email: string, password?: string, isPasskey = false): Promise<ISignInUser> { + const payload = { email, password, isPasskey, clientInfo };Optionally:
- async login(clientInfo: Prisma.JsonValue, email: string, password?: string, isPasskey = false) + async login(params: { clientInfo: Prisma.JsonValue; email: string; password?: string; isPasskey?: boolean })
84-96: Good error mapping to HttpException; consider logging for traceabilityKeep as-is; optionally add minimal logging for audit trails.
- } catch (error) { + } catch (error) { + // this.logger?.error?.('deleteSession failed', { sessionId, userId, error });apps/user/src/user.service.ts (1)
591-599: Consistent error mapping: wrap errors in RpcExceptionOther methods rethrow
RpcException; this one rethrows the raw error.Apply:
async deleteSession(sessionId: string, userId: string): Promise<{ message: string }> { try { return this.userRepository.deleteSessionBySessionId(sessionId, userId); } catch (error) { this.logger.error(`delete session by session id: ${JSON.stringify(error)}`); - throw error; + throw new RpcException(error.response ? error.response : error); } }apps/api-gateway/src/authz/authz.controller.ts (1)
403-433: Delete session endpoint — good ownership enforcement; minor nitsOwnership is enforced by passing
reqUser.idto the service. Consider:
- Returning 204 No Content on successful deletion.
- Route shape consistency:
/sessions/:sessionIdreads more RESTfully than/:sessionId/sessions. Not blocking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
apps/api-gateway/src/authz/authz.controller.ts(5 hunks)apps/api-gateway/src/authz/authz.service.ts(4 hunks)apps/user/dtos/login-user.dto.ts(1 hunks)apps/user/interfaces/user.interface.ts(3 hunks)apps/user/repositories/user.repository.ts(3 hunks)apps/user/src/user.controller.ts(2 hunks)apps/user/src/user.service.ts(4 hunks)libs/common/src/response-messages/index.ts(1 hunks)libs/prisma-service/prisma/schema.prisma(1 hunks)package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
apps/user/interfaces/user.interface.ts (1)
apps/api-gateway/src/issuance/utils/helper.ts (1)
JsonValue(5-5)
apps/user/dtos/login-user.dto.ts (3)
apps/api-gateway/src/user/dto/login-user.dto.ts (1)
LoginUserDto(7-20)libs/common/src/cast.helper.ts (1)
trim(39-43)apps/api-gateway/src/issuance/utils/helper.ts (1)
JsonValue(5-5)
apps/api-gateway/src/authz/authz.service.ts (2)
apps/api-gateway/src/issuance/utils/helper.ts (1)
JsonValue(5-5)libs/common/src/interfaces/user.interface.ts (1)
ISignInUser(1-9)
apps/user/repositories/user.repository.ts (1)
libs/logger/src/logger.service.ts (1)
error(51-54)
apps/api-gateway/src/authz/authz.controller.ts (3)
apps/user/dtos/login-user.dto.ts (1)
LoginUserDto(7-28)apps/api-gateway/src/user/dto/login-user.dto.ts (1)
LoginUserDto(7-20)libs/common/src/interfaces/response.interface.ts (1)
IResponse(8-13)
apps/user/src/user.service.ts (1)
libs/logger/src/logger.service.ts (1)
error(51-54)
🔇 Additional comments (8)
apps/user/interfaces/user.interface.ts (1)
197-198: LGTM on session typingOptional Prisma.JsonValue on ISession matches schema (Json?) and usage.
apps/user/repositories/user.repository.ts (1)
34-34: LGTM: RpcException import for microservice error propagationapps/user/src/user.controller.ts (2)
26-26: LGTM: Using Prisma session type for strong typing
92-100: Ensure authorization at the gateway for user-scoped session operationsConfirm the API gateway enforces userId === auth subject and that admins follow a separate path to avoid IDOR.
Would you like a quick verification script to grep the gateway controller/guards for this check?
apps/api-gateway/src/authz/authz.service.ts (1)
80-82: LGTM: Pass-through to user service for session listingapps/user/src/user.service.ts (2)
29-29: Importing Prismasessiontype — OKMatches the new usage downstream. No action needed.
438-481: Normalize and defensively-parse clientInfo before persisting (avoid storing double-encoded JSON)The controller may send clientInfo as a JSON string — parse it only if it’s a string, fall back to undefined on parse errors, and save a normalized object to the DB instead of the raw string.
Apply:
- async login(loginUserDto: LoginUserDto): Promise<ISignInUser> { - const { email, password, isPasskey, clientInfo } = loginUserDto; + async login(loginUserDto: LoginUserDto): Promise<ISignInUser> { + const { email, password, isPasskey, clientInfo } = loginUserDto; + // Normalize clientInfo (can arrive as string or object) + let normalizedClientInfo: unknown = undefined; + if (clientInfo) { + if (typeof clientInfo === 'string') { + try { + normalizedClientInfo = JSON.parse(clientInfo); + } catch { + // ignore parse error; fall back to undefined to avoid bad data in DB + normalizedClientInfo = undefined; + } + } else { + normalizedClientInfo = clientInfo; + } + }And:
const sessionData = { id: decodedToken.sid, sessionToken: tokenDetails?.access_token, userId: userData?.id, expires: tokenDetails?.expires_in, refreshToken: tokenDetails?.refresh_token, sessionType: SessionType.USER_SESSION, - expiresAt, - clientInfo + expiresAt, + clientInfo: normalizedClientInfo };Also confirm whether both expires (seconds) and expiresAt (Date) are required by your schema/queries; remove one if redundant to avoid divergence.
Would you like me to scan the repo for all Session consumers to confirm which field(s) they actually read?
apps/api-gateway/src/authz/authz.controller.ts (1)
21-30: Swagger decorators — good coverageBearer/Unauthorized/Forbidden annotations improve API docs. Looks good.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
libs/prisma-service/prisma/migrations/20250911122855_add_client_info_column_for_sessions_table/migration.sql (4)
2-2: Define what goes into clientInfo (privacy, PII, and exposure)If this stores IP, UA, device IDs, etc., confirm:
- Only minimal fields are persisted and IPs are truncated/hashed if possible.
- Retention policy exists (e.g., TTL/cleanup job) and RLS/API serializers avoid exposing raw clientInfo via “SELECT *”.
- Data export/deletion flows cover this column for DSR/CCPA/GDPR.
2-2: Add a DB comment for schema clarity (optional)Helps tooling and future readers.
ALTER TABLE "session" ADD COLUMN "clientInfo" JSONB; +COMMENT ON COLUMN "session"."clientInfo" IS 'Client metadata (e.g., UA, device, IP/subnet). Nullable. Keep minimal; see privacy policy.';
2-2: Naming convention note (optional)CamelCase column names force quoting in SQL. If your convention elsewhere is snake_case, consider client_info for consistency in future migrations. If Prisma maps consistently to quoted names, ignore.
2-2: Plan indexing only if you’ll filter on JSON keys — no JSON-key filters found in repoRepo contains clientInfo (Prisma model + migration + writes) but no usages that filter JSON keys (no -> / ->> or Prisma JSON path filters).
ALTER TABLE "session" ADD COLUMN "clientInfo" JSONB;
- If you will query by fields inside clientInfo (e.g., browser, deviceType), add either:
- Functional index for common keys:
CREATE INDEX session_client_browser_idx ON "session" ((clientInfo->>'browser'));- GIN index for ad‑hoc querying:
CREATE INDEX session_clientinfo_gin_idx ON "session" USING GIN ("clientInfo");- Do not run CREATE INDEX ... CONCURRENTLY inside Prisma-managed migrations (Prisma wraps migrations in a transaction); run index creation outside that transaction.
Key locations: libs/prisma-service/prisma/schema.prisma (clientInfo), libs/prisma-service/prisma/migrations/20250911122855_add_client_info_column_for_sessions_table/migration.sql, apps/user/repositories/user.repository.ts:692, apps/api-gateway/src/authz/authz.controller.ts:188-189, apps/user/interfaces/user.interface.ts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/prisma-service/prisma/migrations/20250911122855_add_client_info_column_for_sessions_table/migration.sql(1 hunks)
🔇 Additional comments (1)
libs/prisma-service/prisma/migrations/20250911122855_add_client_info_column_for_sessions_table/migration.sql (1)
1-2: LGTM: backward-compatible column addAdding a nullable JSONB column is safe and aligns with Prisma
Json?. No data rewrite; minimal lock time on Postgres.
Signed-off-by: sujitaw <[email protected]>
Signed-off-by: sujitaw <[email protected]>
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 (1)
apps/user/interfaces/user.interface.ts (1)
178-183: Align IUserSignIn with DTO: make clientInfo optionalDTO has clientInfo optional; interface requires it. This mismatch will cause typing friction across services.
export interface IUserSignIn { email: string; password: string; isPasskey?: boolean; - clientInfo: Prisma.JsonValue; + clientInfo?: Prisma.JsonValue; }
♻️ Duplicate comments (2)
apps/user/dtos/login-user.dto.ts (1)
16-19: Make password conditionally required when not using passkeyEnsure non-passkey logins can’t submit empty passwords; add ValidateIf and tighten validation.
- import { IsBoolean, IsEmail, IsNotEmpty, IsOptional, IsString } from 'class-validator'; + import { IsBoolean, IsEmail, IsNotEmpty, IsOptional, IsString, ValidateIf } from 'class-validator'; @@ - @ApiProperty({ example: 'Password@1' }) - @IsOptional() - @IsString({ message: 'password should be string' }) - password?: string; + @ApiProperty({ example: 'Password@1' }) + @ValidateIf((o) => !o.isPasskey) + @IsNotEmpty({ message: 'Password is required.' }) + @IsString({ message: 'Password should be a string' }) + password?: string;Also applies to: 1-1
apps/user/src/user.service.ts (1)
583-590: Don’t risk leaking tokens in session listing; project safe fields at the service layer (defense-in-depth).Even if the repository is fixed, TypeScript types don’t enforce runtime shape. Map to safe fields here to guarantee no
sessionToken/refreshTokenescape. Also consider pagination.Apply this diff to enforce a safe projection:
- async userSessions(userId: string): Promise<IRestrictedUserSession[]> { - try { - return await this.userRepository.fetchUserSessions(userId); - } catch (error) { - this.logger.error(`get user sessions: ${JSON.stringify(error)}`); - throw new RpcException(error.response ? error.response : error); - } - } + async userSessions(userId: string): Promise<IRestrictedUserSession[]> { + try { + const sessions = await this.userRepository.fetchUserSessions(userId); + // Defense-in-depth: drop any unexpected fields from repository rows + return sessions.map((s: any) => ({ + id: s.id, + userId: s.userId, + createdAt: s.createdAt, + expiresAt: s.expiresAt, + clientInfo: s.clientInfo ?? null, + sessionType: s.sessionType, + })); + } catch (error) { + this.logger.error(`get user sessions: ${JSON.stringify(error)}`); + throw new RpcException(error.response ? error.response : error); + } + }Optional: extend the signature to
userSessions(userId: string, page = 1, pageSize = 20)and pass through to the repository for pagination and ordering bycreatedAt DESC.Run to confirm the repository selects only non-sensitive fields and supports pagination:
#!/bin/bash set -euo pipefail echo "---- fetchUserSessions implementation ----" rg -nP 'fetchUserSessions\s*\(' -C6 apps/user/repositories/user.repository.ts || true echo echo "---- Ensure no token fields are selected/returned ----" rg -n --hidden --no-ignore -C2 -S 'sessionToken|refreshToken' apps/user/repositories/user.repository.ts apps/user/src/user.controller.ts || true echo echo "---- Check controller route for pagination params ----" rg -nP 'userSessions\(' -C4 apps/user/src/user.controller.ts || true
🧹 Nitpick comments (6)
apps/user/dtos/login-user.dto.ts (1)
26-28: Fix Swagger example/type for clientInfoExample is boolean but type is JSON. Provide an object example and mark schema correctly.
- @ApiProperty({ example: 'false' }) + @ApiProperty({ + required: false, + type: 'object', + example: { ip: '203.0.113.1', ua: 'Mozilla/5.0', os: 'Windows', browser: 'Chrome', device: 'Desktop' } + }) @IsOptional() clientInfo?: Prisma.JsonValue;apps/user/src/user.controller.ts (1)
93-101: Consider DTOs for message payloads + validationDefine DTOs for userId/sessionId payloads and enable validation to harden the microservice boundary. Also consider standardizing on object payloads for both queries (future-proofing).
apps/user/repositories/user.repository.ts (2)
703-716: Order sessions for deterministic UXReturn latest sessions first.
- const userSessionCount = await this.prisma.session.findMany({ + const userSessionCount = await this.prisma.session.findMany({ where: { userId }, select: { id: true, userId: true, expiresAt: true, createdAt: true, clientInfo: true, sessionType: true - } + }, + orderBy: { createdAt: 'desc' } });
988-1003: Include sessionId in NotFound to aid debuggingImproves traceability without exposing sensitive data.
- if (0 === result.count) { - throw new RpcException(new NotFoundException(`Session not found for userId: ${userId}`)); - } + if (0 === result.count) { + throw new RpcException(new NotFoundException(`Session ${sessionId} not found for userId: ${userId}`)); + }apps/api-gateway/src/authz/authz.service.ts (1)
19-23: Use type-only Prisma imports; remove unusedsessionafter type changeAvoids pulling Prisma at runtime.
-import { session, user } from '@prisma/client'; +import type { user } from '@prisma/client';apps/user/src/user.service.ts (1)
592-599: Repository already enforces ownership; optional: normalize service error to RpcException
- Repository: apps/user/repositories/user.repository.ts — deleteSessionBySessionId uses prisma.session.deleteMany({ where: { id: sessionId, userId } }) and throws an RpcException when nothing is deleted (lines ~989–996). Ownership guard present; no repo change needed.
- Service: apps/user/src/user.service.ts — deleteSession currently logs and rethrows the original error (lines ~592–599). Optional: wrap non-Rpc errors in RpcException for consistent RPC error shapes.
Suggested diff (optional):
async deleteSession(sessionId: string, userId: string): Promise<{ message: string }> { try { return await this.userRepository.deleteSessionBySessionId(sessionId, userId); } catch (error) { this.logger.error(`delete session by session id: ${JSON.stringify(error)}`); - throw error; + throw new RpcException(error.response ? error.response : error); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/api-gateway/src/authz/authz.controller.ts(5 hunks)apps/api-gateway/src/authz/authz.service.ts(4 hunks)apps/user/dtos/login-user.dto.ts(1 hunks)apps/user/interfaces/user.interface.ts(3 hunks)apps/user/repositories/user.repository.ts(5 hunks)apps/user/src/user.controller.ts(2 hunks)apps/user/src/user.service.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api-gateway/src/authz/authz.controller.ts
🧰 Additional context used
🧬 Code graph analysis (6)
apps/user/interfaces/user.interface.ts (1)
apps/api-gateway/src/issuance/utils/helper.ts (1)
JsonValue(5-5)
apps/user/src/user.controller.ts (1)
apps/user/interfaces/user.interface.ts (1)
IRestrictedUserSession(296-303)
apps/api-gateway/src/authz/authz.service.ts (2)
libs/common/src/interfaces/user.interface.ts (1)
ISignInUser(1-9)apps/user/interfaces/user.interface.ts (1)
IRestrictedUserSession(296-303)
apps/user/dtos/login-user.dto.ts (2)
apps/api-gateway/src/user/dto/login-user.dto.ts (1)
LoginUserDto(7-20)libs/common/src/cast.helper.ts (1)
trim(39-43)
apps/user/repositories/user.repository.ts (2)
apps/user/interfaces/user.interface.ts (1)
IRestrictedUserSession(296-303)libs/logger/src/logger.service.ts (1)
error(51-54)
apps/user/src/user.service.ts (1)
apps/user/interfaces/user.interface.ts (1)
IRestrictedUserSession(296-303)
🔇 Additional comments (5)
apps/user/dtos/login-user.dto.ts (1)
4-4: Good: switched to Prisma.JsonValue (public API)apps/user/interfaces/user.interface.ts (1)
296-303: New IRestrictedUserSession shape looks solidMinimal fields, nullable clientInfo; aligns with repository select.
apps/api-gateway/src/authz/authz.service.ts (1)
54-56: Verified — call sites pass clientInfo first; no action required.
authz.controller.ts:191 calls authzService.login(clientInfo, loginUserDto.email, loginUserDto.password).apps/user/src/user.service.ts (2)
29-29: No action — import change looks harmless.Importing Prisma types (including
session) is fine here. Nothing to change in this hunk.
45-47: Good: returning restricted session shape.Using
IRestrictedUserSessionis the right move to avoid leaking tokens by type contract.
Signed-off-by: sujitaw <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/api-gateway/src/authz/authz.service.ts (1)
59-62: Bug: spreading a string sessionId creates a char-keyed object.
Payload must be{ sessionId }, not{ ...sessionId }. Also add the missing type on the parameter.- async getSession(sessionId): Promise<ISessionDetails> { - const payload = { ...sessionId }; + async getSession(sessionId: string): Promise<ISessionDetails> { + const payload = { sessionId }; return this.natsClient.sendNatsMessage(this.authServiceProxy, 'fetch-session-details', payload); }
🧹 Nitpick comments (2)
apps/api-gateway/src/authz/authz.service.ts (2)
84-96: Normalize error handling to avoid leaking internals and ensure consistency.
Currentcatchrethrows arbitrary errors, potentially exposing internals. Prefer mapping to HttpException (or rethrow HttpException as-is).Apply:
async deleteSession(sessionId: string, userId: string): Promise<{ message: string }> { try { return await this.natsClient.sendNatsMessage(this.authServiceProxy, 'delete-session-by-sessionId', { sessionId, userId }); } catch (error) { - if (error?.response && error?.status) { - throw new HttpException(error.response, error.status); - } - throw error; + if (error instanceof HttpException) throw error; + const status = (error as any)?.status ?? HttpStatus.BAD_GATEWAY; + const response = (error as any)?.response ?? 'Failed to delete session'; + throw new HttpException(response, status); } }And add to imports:
-import { Injectable, Inject, HttpException } from '@nestjs/common'; +import { Injectable, Inject, HttpException, HttpStatus } from '@nestjs/common';
54-56: Make clientInfo nullable or switch to a single DTO; update one call site if you pick the DTO option.Found one authzService.login caller: apps/api-gateway/src/authz/authz.controller.ts:191 (other .login calls are to userService and unrelated).
Minimal (safe) fix:
- async login(clientInfo: Prisma.JsonValue, email: string, password?: string, isPasskey = false): Promise<ISignInUser> { + async login(clientInfo: Prisma.JsonValue | null, email: string, password?: string, isPasskey = false): Promise<ISignInUser> { const payload = { email, password, isPasskey, clientInfo };Optional (clearer, future-proof) DTO:
- async login(clientInfo: Prisma.JsonValue | null, email: string, password?: string, isPasskey = false): Promise<ISignInUser> { - const payload = { email, password, isPasskey, clientInfo }; - return this.natsClient.sendNatsMessage(this.authServiceProxy, 'user-holder-login', payload); - } + async login(params: { email: string; password?: string; isPasskey?: boolean; clientInfo?: Prisma.JsonValue | null }): Promise<ISignInUser> { + const { email, password, isPasskey = false, clientInfo = null } = params; + return this.natsClient.sendNatsMessage(this.authServiceProxy, 'user-holder-login', { email, password, isPasskey, clientInfo }); + }If you adopt the DTO, update the caller at apps/api-gateway/src/authz/authz.controller.ts:191.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api-gateway/src/authz/authz.service.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api-gateway/src/authz/authz.service.ts (2)
libs/common/src/interfaces/user.interface.ts (1)
ISignInUser(1-9)apps/user/interfaces/user.interface.ts (1)
IRestrictedUserSession(296-303)
🔇 Additional comments (3)
apps/api-gateway/src/authz/authz.service.ts (3)
1-1: LGTM: HttpException import matches usage in deleteSession.
No issues with the added symbol.
22-22: Good: type-only Prisma import.
Prevents bundling Prisma at runtime here.
80-82: Return type aligned to restricted session view.
Signature matches downstream response shape. Looks good.
Signed-off-by: sujitaw <[email protected]>
Signed-off-by: sujitaw <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/api-gateway/src/authz/authz.controller.ts (1)
180-193: Don’t stringify clientInfo; harden IP extraction and reuse req.useragent from middlewareDouble‑encoding clientInfo (stringifying) will store a JSON string instead of JSON, and XFF handling misses array cases. Also, since express-useragent middleware is registered, prefer req.useragent over re‑parsing.
Apply:
- async login(@Req() req: Request, @Body() loginUserDto: LoginUserDto, @Res() res: Response): Promise<Response> { + async login(@Req() req: Request, @Body() loginUserDto: LoginUserDto, @Res() res: Response): Promise<Response> { if (loginUserDto.email) { - const ip = (req.headers['x-forwarded-for'] as string)?.split(',')[0] || req.socket.remoteAddress; - const ua = req.headers['user-agent']; - const expressUa = useragent.parse(ua); - const device = { - browser: `${expressUa.browser} ${expressUa.version ?? ''}`.trim(), - os: expressUa.platform, - deviceType: expressUa.isDesktop ? 'desktop' : 'mobile' - }; - - const clientInfo = JSON.stringify({ ...device, rawDetail: ua, ip }); - const userData = await this.authzService.login(clientInfo, loginUserDto.email, loginUserDto.password); + const xff = req.headers['x-forwarded-for']; + const ip = + Array.isArray(xff) ? xff[0] : (xff as string)?.split(',')[0]?.trim() ?? req.ip ?? req.socket.remoteAddress; + const ua = req.headers['user-agent'] ?? ''; + const parsed = (req as any).useragent ?? useragent.parse(ua); + const device = { + browser: [parsed.browser, parsed.version].filter(Boolean).join(' ').trim(), + os: parsed.os || parsed.platform || 'unknown', + deviceType: parsed.isDesktop ? 'desktop' : parsed.isTablet ? 'tablet' : parsed.isMobile ? 'mobile' : 'unknown' + }; + const clientInfo = { ...device, rawDetail: ua, ip }; + const userData = await this.authzService.login(clientInfo, loginUserDto.email, loginUserDto.password);
🧹 Nitpick comments (1)
apps/api-gateway/src/main.ts (1)
19-19: Only keep express-useragent middleware if you actually read req.useragent; otherwise parse ad hoc on the login routeRight now the login controller reparses the UA via useragent.parse(ua), so the global middleware adds overhead to every request without a consumer. Either:
- use req.useragent in the controller, or
- drop the global middleware and keep ad‑hoc parsing on the login route.
Additionally, set trust proxy so req.ip is accurate behind a LB; this also simplifies IP extraction downstream.
Add (outside the shown hunk, after expressApp.set('x-powered-by', false)):
expressApp.set('trust proxy', true);Also applies to: 51-51
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
apps/api-gateway/src/authz/authz.controller.ts(5 hunks)apps/api-gateway/src/main.ts(2 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api-gateway/src/authz/authz.controller.ts (4)
apps/user/dtos/login-user.dto.ts (1)
LoginUserDto(8-29)apps/api-gateway/src/user/dto/login-user.dto.ts (1)
LoginUserDto(7-20)libs/common/src/interfaces/response.interface.ts (1)
IResponse(8-13)libs/common/src/response-messages/index.ts (1)
ResponseMessages(1-518)
🔇 Additional comments (3)
apps/api-gateway/src/authz/authz.controller.ts (3)
369-403: Ownership check + JWT guard on list sessions looks goodIDOR is addressed via reqUser.id === userId and proper 401/403 docs.
410-440: Scoped deletion via (sessionId, userId) is correctJWT required, user context enforced in service call; response contract is fine.
34-34: Resolved: default export IResponseType exists — import is correct
libs/common/src/interfaces/response.interface.ts exports a default interface IResponseType, so the import is valid.
Signed-off-by: sujitaw <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
apps/api-gateway/src/authz/authz.service.ts (3)
59-62: Fix incorrect spread operator usage for sessionId parameter.Spreading a string creates an object with character indices as keys, which is incorrect.
async getSession(sessionId): Promise<ISessionDetails> { - const payload = { ...sessionId }; + const payload = { sessionId }; return this.natsClient.sendNatsMessage(this.authServiceProxy, 'fetch-session-details', payload); }
84-86: LGTM! User sessions method properly implemented.The return type correctly uses
IRestrictedUserSession[]to avoid exposing sensitive data.
20-20: Move session interfaces to shared library to avoid cross-app dependencies.Importing from
apps/usercreates a layering violation and build coupling. These interfaces should be in a shared library.apps/user/src/user.service.ts (2)
439-439: Validate and sanitize clientInfo before persisting to prevent oversized or malformed JSON.The
clientInfoparameter is user-controlled and should be validated to prevent storing arbitrary data or PII.- const { email, password, isPasskey, clientInfo } = loginUserDto; + const { email, password, isPasskey, clientInfo: rawClientInfo } = loginUserDto;Before using clientInfo, add validation:
const clientInfo = sanitizeClientInfo(rawClientInfo); // Helper function to add outside the method function sanitizeClientInfo(input: unknown): Prisma.JsonValue | null { if (!input || typeof input !== 'object') return null; const i = input as Record<string, unknown>; // Only allow specific fields with length limits const sanitized: any = {}; if (typeof i.ip === 'string') sanitized.ip = i.ip.slice(0, 64); if (typeof i.ua === 'string') sanitized.ua = i.ua.slice(0, 512); // Sanitize nested objects if (i.browser && typeof i.browser === 'object') { const b = i.browser as any; sanitized.browser = { name: typeof b.name === 'string' ? b.name.slice(0, 128) : undefined, version: typeof b.version === 'string' ? b.version.slice(0, 64) : undefined }; } if (i.os && typeof i.os === 'object') { const o = i.os as any; sanitized.os = { name: typeof o.name === 'string' ? o.name.slice(0, 128) : undefined, version: typeof o.version === 'string' ? o.version.slice(0, 64) : undefined }; } if (i.device && typeof i.device === 'object') { const d = i.device as any; sanitized.device = { type: typeof d.type === 'string' ? d.type.slice(0, 64) : undefined, vendor: typeof d.vendor === 'string' ? d.vendor.slice(0, 64) : undefined, model: typeof d.model === 'string' ? d.model.slice(0, 64) : undefined }; } return sanitized; }
593-600: Add pagination and ensure sensitive fields are not exposed in session listing.The
fetchUserSessionsmethod should not return sensitive fields likesessionTokenandrefreshToken. Also, consider adding pagination to prevent unbounded responses.Let me verify what fields are actually being returned:
#!/bin/bash # Check the fetchUserSessions implementation in the repository echo "=== Checking fetchUserSessions implementation ===" rg -A 20 "fetchUserSessions.*\(" apps/user/repositories/user.repository.ts # Check if IRestrictedUserSession excludes sensitive fields echo -e "\n=== Checking IRestrictedUserSession interface ===" ast-grep --pattern 'interface IRestrictedUserSession { $$$ }' # Check the Prisma schema for session model echo -e "\n=== Checking Prisma session model ===" rg -A 15 "model session" libs/prisma-service/prisma/schema.prisma
🧹 Nitpick comments (2)
apps/api-gateway/src/authz/jwt.strategy.ts (1)
65-77: Improve error logging and message for session validation failures.The error log message has a typo and could be more descriptive. Also, consider differentiating between session not found vs. other errors.
if (sessionId) { try { sessionDetails = await this.authzService.checkSession(sessionId); } catch (error) { - this.logger.log('Error in JWT Stratergy while fetching session details', JSON.stringify(error, null, 2)); + this.logger.error('Error in JWT Strategy while fetching session details', error); } if (!sessionDetails) { throw new UnauthorizedException(ResponseMessages.user.error.invalidAccessToken); } }apps/api-gateway/src/authz/authz.service.ts (1)
64-66: Add type annotation for sessionId parameter.- async checkSession(sessionId): Promise<ISessionDetails> { + async checkSession(sessionId: string): Promise<ISessionDetails> { return this.natsClient.sendNatsMessage(this.authServiceProxy, 'check-session-details', sessionId); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/api-gateway/src/authz/authz.service.ts(5 hunks)apps/api-gateway/src/authz/jwt-payload.interface.ts(1 hunks)apps/api-gateway/src/authz/jwt.strategy.ts(1 hunks)apps/user/repositories/user.repository.ts(6 hunks)apps/user/src/user.controller.ts(2 hunks)apps/user/src/user.service.ts(6 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 (4)
apps/user/src/user.service.ts (1)
apps/user/interfaces/user.interface.ts (2)
ISessionDetails(206-210)IRestrictedUserSession(296-303)
apps/api-gateway/src/authz/jwt.strategy.ts (2)
apps/api-gateway/src/authz/authz.controller.ts (1)
sessionDetails(222-240)libs/common/src/response-messages/index.ts (1)
ResponseMessages(1-518)
apps/api-gateway/src/authz/authz.service.ts (2)
libs/common/src/interfaces/user.interface.ts (1)
ISignInUser(1-9)apps/user/interfaces/user.interface.ts (2)
ISessionDetails(206-210)IRestrictedUserSession(296-303)
apps/user/src/user.controller.ts (1)
apps/user/interfaces/user.interface.ts (2)
ISessionDetails(206-210)IRestrictedUserSession(296-303)
🔇 Additional comments (8)
apps/api-gateway/src/authz/jwt-payload.interface.ts (1)
12-12: LGTM! Session ID added to JWT payload interface.The addition of the
sidfield properly enables session-based validation in the JWT strategy.apps/user/src/user.controller.ts (3)
88-91: LGTM! Session check endpoint properly implemented.The method correctly delegates to the service layer for session validation.
98-101: LGTM! User sessions endpoint properly implemented.The method returns restricted session information, which is appropriate for security.
103-106: LGTM! Session deletion endpoint properly implemented.The method correctly requires both sessionId and userId for authorization.
apps/api-gateway/src/authz/authz.service.ts (2)
54-57: LGTM! Login method updated with client info tracking.The addition of clientInfo parameter enables proper session metadata tracking.
88-100: LGTM! Session deletion with proper error handling.The error handling correctly preserves HTTP exception details when available.
apps/user/src/user.service.ts (2)
528-536: LGTM! Session check method properly implemented.The method correctly fetches session details by ID without any transformations.
602-609: LGTM! Session deletion properly implemented.The method correctly delegates to the repository and maintains error propagation.
Signed-off-by: sujitaw <[email protected]>
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/user/repositories/user.repository.ts (1)
988-1002: Correct P2025 error handling for session deletion.The implementation properly handles the P2025 error code which occurs when a record is not found, converting it to an appropriate
NotFoundExceptionwrapped inRpcExceptionfor microservice communication. The error handling is consistent with Prisma best practices.However, there's a potential issue with the delete operation:
- await this.prisma.session.delete({ - where: { id: sessionId, userId } - }); + const result = await this.prisma.session.deleteMany({ + where: { id: sessionId, userId } + }); + + if (result.count === 0) { + throw new RpcException(new NotFoundException(`Session not found for userId: ${userId}`)); + }Using
deleteManyinstead ofdeletewould avoid the P2025 exception entirely and provide cleaner control flow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/user/repositories/user.repository.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/user/repositories/user.repository.ts (2)
apps/user/interfaces/user.interface.ts (1)
IRestrictedUserSession(296-303)libs/logger/src/logger.service.ts (1)
error(51-54)
🔇 Additional comments (5)
apps/user/repositories/user.repository.ts (5)
5-6: LGTM!The import of
IRestrictedUserSessioninterface properly supports the new session listing functionality.
35-35: LGTM!The
RpcExceptionimport is necessary for proper microservice error propagation in the session deletion functionality.
140-151: The getSession method now properly awaits the database call.This change fixes the previous issue where a Promise was being returned directly instead of the resolved session data.
692-694: Don't write placeholder clientInfo; omit or store null.Persisting
{ clientToken: true }when clientInfo is absent pollutes data. The P2025 error code occurs when a record is not found in the database. Prefer omitting or explicit null.
703-723: LGTM! Session listing with proper field restriction.The method correctly implements the
IRestrictedUserSessioninterface by selecting only the necessary fields and returning proper typing. The selective projection prevents exposing sensitive session data.
* feat/api to fetch all sessions and delete session Signed-off-by: sujitaw <[email protected]> * feat/update signin api to store session details Signed-off-by: sujitaw <[email protected]> * feat/added prisma migration file for clientInfo column Signed-off-by: sujitaw <[email protected]> * fix/sonarcube comment Signed-off-by: sujitaw <[email protected]> * fix/code rabbit comments Signed-off-by: sujitaw <[email protected]> * fix/rabbit comments Signed-off-by: sujitaw <[email protected]> * fix/pr comments Signed-off-by: sujitaw <[email protected]> * fix/changed the package from uaparser to express-parser Signed-off-by: sujitaw <[email protected]> * feat/update jwt logic to throw 401 if session is deleted Signed-off-by: sujitaw <[email protected]> * fix/resolve pr comments Signed-off-by: sujitaw <[email protected]> --------- Signed-off-by: sujitaw <[email protected]> Signed-off-by: Ankita Patidar <[email protected]>
What
Summary by CodeRabbit
New Features
Documentation
Chores