Added logs for login and user read API#588
Added logs for login and user read API#588Tusharmahajan12 wants to merge 9 commits intotekdi:aspire-leadersfrom
Conversation
WalkthroughAuthController.login now accepts and forwards the Express Request to AuthService; AuthService and UserController gained request-aware, structured logging (User-Agent, tenant/JWT context) and expanded error classification; LoggerUtil made non-blocking with file rotation and failure-safe behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthController
participant AuthService
participant AuthAdapter
participant Logger
Note over Client,AuthController: Login request with credentials + headers
Client->>AuthController: POST /auth/login (body, headers)
AuthController->>AuthService: login(authDto, request, response)
AuthService->>Logger: log attempt (User-Agent, tenantId/JWT if present)
AuthService->>AuthAdapter: validate credentials / fetch user/tokens
alt Adapter returns success
AuthService->>Logger: log success (User-Agent, OK)
AuthService->>Client: respond 200 (tokens/user)
else Adapter returns client/server error
AuthService->>Logger: log failure (mapped failureReason / IssueType)
AuthService->>Client: respond mapped status (4xx/5xx)
end
sequenceDiagram
participant Client
participant UserController
participant UserAdapter
participant Logger
Client->>UserController: GET /user?tenantId=... (Authorization header)
UserController->>Logger: log attempt (tenantId, redacted fieldvalue)
UserController->>UserController: decode JWT -> requester identity (fallback Unknown)
UserController->>UserAdapter: fetch user (tenantId, query)
alt Adapter returns 2xx
UserController->>Logger: log success (tenantId)
UserController->>Client: respond 200 (user)
else Adapter returns non-2xx
UserController->>Logger: log failure (mapped failureReason / IssueType, tenantId)
UserController->>Client: respond original status with error metadata
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 4
♻️ Duplicate comments (1)
src/auth/auth.service.ts (1)
150-165: DuplicategetClientIpimplementation.As noted in the UserController review, this helper is duplicated. Extract to a shared utility for consistency.
🧹 Nitpick comments (3)
src/user/user.controller.ts (1)
219-234: DuplicategetClientIpimplementation - extract to shared utility.This
getClientIphelper is duplicated insrc/auth/auth.service.ts(lines 150-165). Both implementations are identical. Extract this to a shared utility (e.g., insrc/common/utils/) to follow DRY principle and ensure consistent IP extraction logic across the codebase.🔎 Example shared utility
Create
src/common/utils/request.utils.ts:import { Request } from 'express'; export function getClientIp(request: Request): string { const forwarded = request.headers['x-forwarded-for']; if (forwarded) { const ips = Array.isArray(forwarded) ? forwarded[0] : forwarded; return ips.split(',')[0].trim(); } const realIp = request.headers['x-real-ip']; if (realIp) { return Array.isArray(realIp) ? realIp[0] : realIp; } return request.ip || request.socket?.remoteAddress || 'Unknown'; }Then import and use in both files instead of duplicating the method.
src/auth/auth.service.ts (2)
226-238: Fragile error classification based on string matching.Categorizing errors by matching substrings in error messages (e.g.,
e.message?.includes('token')) is fragile and locale-dependent. Error messages can change between library versions or vary based on environment.Consider using error types/names, error codes, or custom exception classes for more reliable error classification.
🔎 Example improvement
// More reliable error classification if (e instanceof JsonWebTokenError || e.name === 'JsonWebTokenError') { httpStatus = HttpStatus.UNAUTHORIZED; failureReason = 'INVALID_TOKEN'; issueType = 'CLIENT_ERROR'; } else if (e instanceof NotFoundException) { httpStatus = HttpStatus.NOT_FOUND; failureReason = 'USER_NOT_FOUND'; issueType = 'CLIENT_ERROR'; } else if (e instanceof UnauthorizedException || e instanceof ForbiddenException) { httpStatus = HttpStatus.FORBIDDEN; failureReason = 'UNAUTHORIZED'; issueType = 'CLIENT_ERROR'; }
167-208: Excessive logging in getUserByAuth - consider consolidating.The method logs three separate info entries during a single successful request (attempt at line 178, processing at line 191, success at line 203). This could be consolidated into a single log entry at completion to reduce log volume while maintaining auditability.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
src/auth/auth.controller.tssrc/auth/auth.service.tssrc/common/logger/LoggerUtil.tssrc/user/user.controller.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
⚙️ CodeRabbit configuration file
**/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
Files:
src/common/logger/LoggerUtil.tssrc/user/user.controller.tssrc/auth/auth.service.tssrc/auth/auth.controller.ts
🧬 Code graph analysis (3)
src/user/user.controller.ts (2)
src/common/logger/LoggerUtil.ts (2)
LoggerUtil(3-141)error(85-106)src/utils/response.ts (1)
error(32-58)
src/auth/auth.service.ts (2)
src/common/utils/api-id.config.ts (1)
APIID(21-127)src/common/logger/LoggerUtil.ts (2)
LoggerUtil(3-141)error(85-106)
src/auth/auth.controller.ts (1)
src/auth/dto/auth-dto.ts (1)
AuthDto(4-24)
🔇 Additional comments (7)
src/common/logger/LoggerUtil.ts (3)
25-29: Console transport disables exception/rejection handling.Setting
handleExceptions: falseandhandleRejections: falsemeans uncaught exceptions and unhandled promise rejections won't be logged to console. If your file transports also don't handle them (they don't by default), these critical events may go unlogged. Verify this is intentional for your production environment.
57-80: LGTM - Non-blocking log method with proper error handling.The use of
process.nextTickto defer logging is a valid approach for non-blocking I/O. The try/catch ensures logging failures don't affect the calling code.
85-106: Consistent non-blocking pattern across all log levels.The error, warn, and debug methods follow the same non-blocking pattern with proper error handling. Implementation is consistent.
Also applies to: 111-123, 128-140
src/auth/auth.controller.ts (1)
46-52: LGTM - Request object properly injected and forwarded.The login method now correctly receives the Express Request via
@Req()decorator and passes it to the service for IP extraction and logging. The implementation follows NestJS patterns.src/user/user.controller.ts (2)
120-125: Consider privacy implications of logging user identifiers.The logs include usernames, user IDs, and IP addresses. Depending on your jurisdiction (GDPR, CCPA), logging PII may require:
- Appropriate data retention policies
- User consent or legitimate interest documentation
- Secure log storage and access controls
Verify these logs comply with your organization's privacy requirements.
Also applies to: 159-164
149-212: Comprehensive error handling with proper logging.The try/catch wrapper around the adapter call provides good error handling with:
- Status code mapping for different error types
- Detailed error logging
- Structured error responses
The implementation appropriately distinguishes between success (2xx), client errors (4xx), and server errors (5xx).
src/auth/auth.service.ts (1)
30-44: LGTM - Login method properly enhanced with request context.The login method signature update and initial logging implementation are correct. Request information is properly extracted for audit logging purposes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/auth/auth.service.ts (2)
248-255: Error response label still doesn't match actual status code.The
APIResponse.errorcall uses'Internal Server Error'as the error label even when the detected status could be 401, 403, or 404. While the comment says "Keep original API response behavior", this creates inconsistent responses where the logged status differs from the returned status.If maintaining backward compatibility is required, consider adding a TODO or tracking issue to address this in a future version.
120-120: UsingNotFoundExceptionfor invalid credentials returns incorrect HTTP status.A 401 Unauthorized response indicates invalid credentials, but
NotFoundExceptionreturns 404 Not Found. This is semantically incorrect and confuses API consumers expecting proper HTTP status codes.🔎 Suggested fix
- throw new NotFoundException('Invalid username or password'); + throw new UnauthorizedException('Invalid username or password');
🧹 Nitpick comments (6)
src/common/logger/LoggerUtil.ts (3)
37-37: Inaccurate comment fortailableoption.The comment states "Oldest logs are deleted when maxFiles is reached", but this describes
maxFilesbehavior, nottailable. Thetailable: trueoption ensures the base log filename (e.g.,error.log) always contains the most recent logs, enablingtail -fto work correctly. Rotation creates numbered files (error.log.1, etc.) for older logs.🔎 Suggested fix
- tailable: true, // Oldest logs are deleted when maxFiles is reached + tailable: true, // Keep base filename for most recent logs (tail -f compatible)
45-45: Same inaccurate comment repeated.Same issue as above - the comment describes
maxFilesbehavior, nottailable.🔎 Suggested fix
- tailable: true, // Oldest logs are deleted when maxFiles is reached + tailable: true, // Keep base filename for most recent logs (tail -f compatible)
21-22: Consider making log level configurable.The logger level is hardcoded to
'info', which meansdebuglevel logs (line 131-142) will never be emitted unless the configuration is changed. Consider reading from an environment variable for flexibility.🔎 Suggested enhancement
this.logger = winston.createLogger({ - level: 'info', + level: process.env.LOG_LEVEL || 'info', format: winston.format.combine(winston.format.timestamp(), customFormat),src/user/user.controller.ts (1)
223-238: ExtractgetClientIpto a shared utility.This helper is duplicated in
src/auth/auth.service.ts. Extract it to a shared location (e.g.,src/common/utils/request.util.ts) to follow DRY principles and ensure consistent IP extraction across the codebase.🔎 Suggested approach
Create a shared utility:
// src/common/utils/request.util.ts import { Request } from 'express'; export function getClientIp(request: Request): string { const forwarded = request.headers['x-forwarded-for']; if (forwarded) { const ips = Array.isArray(forwarded) ? forwarded[0] : forwarded; return ips.split(',')[0].trim(); } const realIp = request.headers['x-real-ip']; if (realIp) { return Array.isArray(realIp) ? realIp[0] : realIp; } return request.ip || request.socket?.remoteAddress || 'Unknown'; }Then import and use in both controllers/services.
src/auth/auth.service.ts (2)
150-165: DuplicategetClientIpimplementation.This helper is identical to the one in
src/user/user.controller.ts. Extract to a shared utility as mentioned in the UserController review.
226-238: Fragile error detection using string matching.The error categorization relies on checking
e.messagefor substrings like 'token', 'jwt', 'not found', etc. This is fragile because:
- Error messages may change across library versions
- Localized messages won't match
- Partial matches could produce false positives
Consider using error types/classes or error codes instead of message content for reliable categorization.
🔎 Suggested improvement
- if (e.name === 'JsonWebTokenError' || e.message?.includes('token') || e.message?.includes('jwt')) { + if (e.name === 'JsonWebTokenError' || e.name === 'InvalidTokenError') { detectedStatus = HttpStatus.UNAUTHORIZED; failureReason = 'INVALID_TOKEN'; issueType = 'CLIENT_ERROR'; - } else if (e.message?.includes('not found') || e.message?.includes('does not exist')) { + } else if (e instanceof NotFoundException || e.status === HttpStatus.NOT_FOUND) { detectedStatus = HttpStatus.NOT_FOUND; failureReason = 'USER_NOT_FOUND'; issueType = 'CLIENT_ERROR'; - } else if (e.message?.includes('unauthorized') || e.message?.includes('forbidden')) { + } else if (e instanceof UnauthorizedException || e instanceof ForbiddenException) { detectedStatus = HttpStatus.FORBIDDEN; failureReason = 'UNAUTHORIZED'; issueType = 'CLIENT_ERROR'; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
src/auth/auth.service.tssrc/common/logger/LoggerUtil.tssrc/user/user.controller.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
⚙️ CodeRabbit configuration file
**/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
Files:
src/common/logger/LoggerUtil.tssrc/auth/auth.service.tssrc/user/user.controller.ts
🧬 Code graph analysis (1)
src/user/user.controller.ts (1)
src/common/logger/LoggerUtil.ts (2)
LoggerUtil(3-144)error(88-109)
🔇 Additional comments (7)
src/common/logger/LoggerUtil.ts (2)
48-50: Previous review comment addressed.The misleading comment about
exitOnErrorhas been corrected. The comment now accurately describes that logging failures won't crash the application.
66-82: Non-blocking logging implementation looks good.The
process.nextTickapproach with try-catch fallback provides resilience. However, note that winston's file transports are already asynchronous, so the primary benefit here is exception isolation rather than non-blocking I/O.One consideration: if the application exits immediately after logging, some logs queued via
process.nextTickmay be lost since they execute after the current operation completes but before the next event loop tick.src/user/user.controller.ts (3)
106-119: Good improvement - JWT decode errors now logged.The empty catch block from the previous review has been addressed. The code now logs a warning with relevant context (IP, error message) before continuing with fallback values.
153-196: Robust response handling with appropriate logging.The status code categorization (2xx success, 4xx client error, 5xx server error) and structured logging with failure reasons is well-implemented. This will aid debugging and monitoring.
59-59: The import syntaximport jwt_decode from 'jwt-decode'is correct for version 3.1.2. No changes needed.src/auth/auth.service.ts (2)
30-44: Good enhancement - login now includes request context.Adding the
Requestparameter enables comprehensive logging of login attempts with client IP and User-Agent. This is valuable for security monitoring and audit trails.
62-68: Structured logging for failed login attempts.The logging includes failure reason categorization (USER_NOT_FOUND vs USER_INACTIVE) and issue type classification. This structured approach will facilitate log analysis and alerting.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/auth/auth.service.ts (2)
247-253: Error response label inconsistency with detected status.The API always returns
'Internal Server Error'as the label, even whendetectedStatusindicates a 401, 403, or 404. This was previously flagged. The comment indicates this is intentional to "keep original API response behavior," but this creates inconsistent API responses.
119-119: UsingNotFoundExceptionfor invalid credentials returns incorrect HTTP status.This throws a 404 Not Found for invalid credentials, but the correct response should be 401 Unauthorized. This was previously flagged.
🔎 Suggested fix
- throw new NotFoundException('Invalid username or password'); + throw new UnauthorizedException('Invalid username or password');
🧹 Nitpick comments (2)
src/auth/auth.service.ts (2)
171-172: Unused variables:usernameanduserIdare assigned but never used.These variables are declared and assigned from the decoded JWT token but are never used in the code. If they're excluded from logging for legal compliance, consider removing these assignments to avoid unnecessary processing.
🔎 Suggested fix
// Extract request information for logging const userAgent = request.headers['user-agent'] || 'Unknown'; - let username = 'Unknown'; - let userId = 'Unknown'; try { // Log API call attempt (username and IP excluded for legal compliance) LoggerUtil.log( `GetUserByAuth attempt - User-Agent: ${userAgent}, TenantId: ${tenantId || 'Not provided'}`, 'AuthService', undefined, // Username excluded for legal compliance 'info' ); - // Decode JWT token to get username const decoded: any = jwt_decode(request.headers.authorization); - username = decoded.preferred_username || 'Unknown'; - userId = decoded.sub || 'Unknown'; + const username = decoded.preferred_username || 'Unknown'; - // Log with username after decoding (username, userId, and IP excluded for legal compliance) + // Log processing (username excluded for legal compliance) LoggerUtil.log( `GetUserByAuth processing - TenantId: ${tenantId || 'Not provided'}`, 'AuthService', undefined, // Username excluded for legal compliance 'info' );Also applies to: 185-186
224-236: Error detection relies on fragile string matching.The error type detection uses substring matching on error messages (e.g.,
includes('token'),includes('not found')), which is brittle and may produce false positives/negatives. Consider using error types or codes from the underlying libraries instead of message parsing.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
src/auth/auth.service.tssrc/user/user.controller.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/user/user.controller.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
⚙️ CodeRabbit configuration file
**/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
Files:
src/auth/auth.service.ts
🧬 Code graph analysis (1)
src/auth/auth.service.ts (2)
src/common/utils/api-id.config.ts (1)
APIID(21-127)src/common/logger/LoggerUtil.ts (2)
LoggerUtil(3-144)error(88-109)
🔇 Additional comments (2)
src/auth/auth.service.ts (2)
14-15: LGTM!The imports for
RequestandLoggerUtilare appropriate for the logging functionality being added.
57-67: LGTM!The error logging for failed login attempts provides comprehensive details (status code, failure reason, message, issue type) while appropriately excluding sensitive user information.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/auth/auth.service.ts (1)
161-167: Response always returns 500 regardless of detected error type.The error handling correctly categorizes failures (lines 128-151) and logs the appropriate status, but the API response always returns
HttpStatus.INTERNAL_SERVER_ERRORwith label'Internal Server Error'. This creates inconsistent behavior where a 4xx client error from an upstream service is masked as a 500.Consider using the detected
httpStatusand a matching label for more accurate error propagation:🔎 Suggested fix
+ const errorLabel = httpStatus >= 500 ? 'Internal Server Error' : + httpStatus === 400 ? 'Bad Request' : + httpStatus === 403 ? 'Forbidden' : + httpStatus === 404 ? 'Not Found' : + httpStatus === 429 ? 'Too Many Requests' : + 'Client Error'; + return APIResponse.error( response, apiId, - 'Internal Server Error', + errorLabel, `Error : ${errorMessage}`, - HttpStatus.INTERNAL_SERVER_ERROR + httpStatus );
♻️ Duplicate comments (2)
src/auth/auth.service.ts (2)
111-120: UsingNotFoundExceptionfor 401 invalid credentials is semantically incorrect.This was flagged in a previous review. The logging correctly identifies this as a 401 Unauthorized case, but line 120 throws
NotFoundExceptionwhich returns a 404 status code. This creates an inconsistency between the logged status and the actual response.
253-259: Error response label doesn't match detected status code.This is a duplicate of a past review comment. The logs show the actual detected status (e.g., 401, 404), but the API response always returns 500 with
'Internal Server Error'. While the comment on line 252 explains this preserves original behavior, it creates misleading responses for clients.
🧹 Nitpick comments (3)
src/auth/auth.service.ts (3)
177-178: Remove unusedusernameanduserIdvariables.These variables are extracted from the JWT but never used in the code. The comments state they are "excluded for legal compliance" but they are still assigned to variables. Either remove them entirely or use them for internal logic (if needed) without logging.
🔎 Suggested fix
// Extract request information for logging const userAgent = request.headers['user-agent'] || 'Unknown'; - let username = 'Unknown'; - let userId = 'Unknown'; try { // Log API call attempt (username and IP excluded for legal compliance) LoggerUtil.log( `GetUserByAuth attempt - User-Agent: ${userAgent}, TenantId: ${tenantId || 'Not provided'}`, 'AuthService', undefined, // Username excluded for legal compliance 'info' ); // Decode JWT token to get username const decoded: any = jwt_decode(request.headers.authorization); - username = decoded.preferred_username || 'Unknown'; - userId = decoded.sub || 'Unknown'; + const username = decoded.preferred_username || 'Unknown'; // Log with username after decoding (username, userId, and IP excluded for legal compliance)Also applies to: 191-192
128-151: Consider extracting failure reason mapping to a helper function.The status-to-reason mapping logic here (lines 128-151) is similar to pattern that could be reused elsewhere. Consider extracting this to a private helper method for maintainability and to reduce duplication if similar mappings are needed in other methods.
🔎 Suggested refactor
private getFailureReasonFromStatus(httpStatus: number): string { const statusReasonMap: Record<number, string> = { }; if (httpStatus >= 400 && httpStatus < 500) { return statusReasonMap[httpStatus] || 'CLIENT_ERROR'; } if (httpStatus >= 500) { return statusReasonMap[httpStatus] || 'INTERNAL_SERVER_ERROR'; } return 'INTERNAL_SERVER_ERROR'; }
230-242: Error detection based on message string matching is fragile.Relying on
e.message?.includes('token')ore.message?.includes('not found')is brittle—error messages can change between library versions or be localized. Consider checkinge.name,e.code, or using typed error classes fromjwt-decodefor more reliable detection.🔎 Suggested approach
- if (e.name === 'JsonWebTokenError' || e.message?.includes('token') || e.message?.includes('jwt')) { + if (e.name === 'InvalidTokenError' || e.name === 'JsonWebTokenError') { detectedStatus = HttpStatus.UNAUTHORIZED; failureReason = 'INVALID_TOKEN'; issueType = 'CLIENT_ERROR'; - } else if (e.message?.includes('not found') || e.message?.includes('does not exist')) { + } else if (e.response?.status === 404) { detectedStatus = HttpStatus.NOT_FOUND; failureReason = 'USER_NOT_FOUND'; issueType = 'CLIENT_ERROR'; - } else if (e.message?.includes('unauthorized') || e.message?.includes('forbidden')) { + } else if (e.response?.status === 403 || e.response?.status === 401) { detectedStatus = HttpStatus.FORBIDDEN; failureReason = 'UNAUTHORIZED'; issueType = 'CLIENT_ERROR'; }jwt-decode version 3 error types and exception names
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/auth/auth.service.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
⚙️ CodeRabbit configuration file
**/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
Files:
src/auth/auth.service.ts
🧬 Code graph analysis (1)
src/auth/auth.service.ts (3)
src/auth/dto/auth-dto.ts (1)
AuthDto(4-24)src/common/utils/api-id.config.ts (1)
APIID(21-127)src/common/logger/LoggerUtil.ts (2)
LoggerUtil(3-144)error(88-109)
🔇 Additional comments (5)
src/auth/auth.service.ts (5)
14-16: LGTM!The new imports are correctly structured and necessary for the enhanced logging functionality. The
AuthDtoimport properly supports the typed method signature.
31-44: LGTM!The method signature now properly includes the
AuthDtotype annotation as recommended. User-Agent extraction with a sensible fallback and the initial logging pattern is well-structured and follows the non-blocking logging approach.
53-77: LGTM!The error categorization with
USER_NOT_FOUNDandUSER_INACTIVEfailure reasons provides good observability. The log message structure is comprehensive and the response label correctly matches the HTTP status code.
96-110: LGTM!The success logging follows a consistent pattern with the rest of the logging implementation and appropriately logs the outcome without sensitive information.
202-220: LGTM!The success flow correctly logs the outcome and returns the appropriate response with consistent patterns.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/auth/auth.service.ts (3)
158-164: API response doesn't match detected error status.The code correctly detects and logs
httpStatus(which may be 4xx), but the API response always returnsHttpStatus.INTERNAL_SERVER_ERRORwith hardcoded label'Internal Server Error'. This masks client errors as server errors.🔎 Suggested fix
+ const errorLabel = httpStatus === HttpStatus.BAD_REQUEST ? 'Bad Request' : + httpStatus === HttpStatus.UNAUTHORIZED ? 'Unauthorized' : + httpStatus === HttpStatus.FORBIDDEN ? 'Forbidden' : + httpStatus === HttpStatus.NOT_FOUND ? 'Not Found' : + 'Internal Server Error'; + return APIResponse.error( response, apiId, - 'Internal Server Error', + errorLabel, `Error : ${errorMessage}`, - HttpStatus.INTERNAL_SERVER_ERROR + httpStatus );
263-270: API response always returns 500 regardless of detected error type.Similar to the login method, the error handling logs the detected status (
detectedStatus) but always returnsHttpStatus.INTERNAL_SERVER_ERRORto the client. This was flagged in a previous review. Consider returning the appropriate status code to match the error type.
117-117:NotFoundExceptionfor invalid credentials returns wrong HTTP status.This throws a 404 Not Found when the error is actually a 401 Unauthorized condition. Use
UnauthorizedExceptioninstead.🔎 Suggested fix
- throw new NotFoundException('Invalid username or password'); + throw new UnauthorizedException('Invalid username or password');
🧹 Nitpick comments (4)
src/auth/auth.service.ts (2)
169-199: Unused variables and weak typing ingetUserByAuth.Two issues:
request: anyshould be typed asRequestfrom express for consistency and type safety.
usernameanduserId(lines 174-175, 190-191) are extracted from the JWT but never used. Since they're excluded from logs for compliance, consider removing these dead assignments.🔎 Suggested fix
- public async getUserByAuth(request: any, tenantId, response: Response) { + public async getUserByAuth(request: Request, tenantId: string, response: Response) { const apiId = APIID.USER_AUTH; // Extract request information for logging const userAgent = request.headers['user-agent'] || 'Unknown'; - let username = 'Unknown'; - let userId = 'Unknown'; try { // Log API call attempt (username and IP excluded for legal compliance) LoggerUtil.log( `GetUserByAuth attempt - User-Agent: ${userAgent}, TenantId: ${ tenantId || 'Not provided' }`, 'AuthService', undefined, // Username excluded for legal compliance 'info' ); - // Decode JWT token to get username const decoded: any = jwt_decode(request.headers.authorization); - username = decoded.preferred_username || 'Unknown'; - userId = decoded.sub || 'Unknown'; + const username = decoded.preferred_username || 'Unknown';
229-251: Error classification uses case-sensitive string matching.The error message checks (
e.message?.includes('token'), etc.) are case-sensitive and may miss errors with different casing. Consider using case-insensitive matching for more robust classification.🔎 Suggested improvement
+ const errorMessageLower = e.message?.toLowerCase() || ''; + if ( e.name === 'JsonWebTokenError' || - e.message?.includes('token') || - e.message?.includes('jwt') + errorMessageLower.includes('token') || + errorMessageLower.includes('jwt') ) {src/user/user.controller.ts (2)
100-115:requesterUsernameis extracted but never used.The variable is assigned from the JWT but excluded from all logs per compliance. Consider removing the extraction entirely if not needed, to avoid dead code.
🔎 Suggested fix
- // Extract request information for logging - let requesterUsername = 'Unknown'; - try { // Extract requester info from JWT token if available if (request.headers.authorization) { - const decoded: any = jwt_decode(request.headers.authorization); - requesterUsername = decoded.preferred_username || 'Unknown'; + // JWT validation happens via JwtAuthGuard; decode only if needed for logging + jwt_decode(request.headers.authorization); // Validate token format } } catch (e) { - // If token decode fails, log the error and continue with Unknown values (IP excluded for legal compliance) + // If token decode fails, log the error LoggerUtil.warn( `Failed to decode JWT token for getUser request - Error: ${e?.message || 'Unknown error'}`, 'UserController' ); }Alternatively, if the purpose is just to validate the token can be decoded, and you don't need the extracted values, you could remove this block entirely since
JwtAuthGuardalready handles authentication.
149-193: Consider defensive check for undefined result.If
getUsersDetailsByIdreturnsundefinedornull, accessingresult.statusCodeon line 154 would throw aTypeError. While the adapter likely always returns an object, a defensive check improves robustness.🔎 Suggested fix
const result = await this.userAdapter .buildUserAdapter() .getUsersDetailsById(userData, response); + if (!result) { + LoggerUtil.error( + `GetUser failed - StatusCode: 500, Reason: EMPTY_RESPONSE, Message: Adapter returned empty result, IssueType: SERVER_ERROR, TenantId: ${tenantId}`, + 'Empty adapter response', + 'UserController', + undefined + ); + return response.status(500).json({ + statusCode: 500, + error: 'Internal Server Error', + message: 'An unexpected error occurred', + }); + } + const statusCode = result.statusCode || 200;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
src/auth/auth.service.tssrc/user/user.controller.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
⚙️ CodeRabbit configuration file
**/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
Files:
src/user/user.controller.tssrc/auth/auth.service.ts
🧬 Code graph analysis (1)
src/auth/auth.service.ts (3)
src/auth/dto/auth-dto.ts (1)
AuthDto(4-24)src/common/utils/api-id.config.ts (1)
APIID(21-127)src/common/logger/LoggerUtil.ts (2)
LoggerUtil(3-144)error(88-109)
🔇 Additional comments (5)
src/auth/auth.service.ts (3)
13-15: LGTM - Type annotation and imports properly added.The
AuthDtotype annotation on theloginmethod parameter and the corresponding import address the previous review feedback. TheRequestandLoggerUtilimports support the new logging functionality.Also applies to: 30-30
33-43: Request-context logging implemented appropriately.The user-agent extraction and non-blocking logging with explicit compliance exclusions follows a good pattern. The fallback to
'Unknown'handles missing headers gracefully.
57-65: Structured error logging with clear failure categorization.The differentiation between
USER_NOT_FOUNDandUSER_INACTIVEfailure reasons provides useful diagnostic information while maintaining compliance by excluding PII.src/user/user.controller.ts (2)
21-21: Top-level import for jwt-decode properly implemented.The
jwt_decodeimport moved to the top of the file addresses the previous review feedback about avoiding inlinerequire(). This is more idiomatic TypeScript/NestJS.Also applies to: 59-59
117-138: Proper request validation and logging for missing tenantId.The structured logging with
StatusCode,Reason,Message, andIssueTypefollows the pattern established in the auth service. The 400 response for missing tenantId is appropriate.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/adapters/postgres/cohort-adapter.ts (1)
336-378: UseSELECT *to maintain consistency and reduce schema coupling.The query at line 280 in the same file uses
SELECT DISTINCT ON (fv."fieldId", fv."itemId") fv.*successfully, proving explicit column enumeration is unnecessary. UsingSELECT *here maintains consistency, reduces maintenance burden, and prevents future bugs if columns are added to the FieldValues table.Change the inner subquery to:
SELECT DISTINCT ON (fv."fieldId", fv."itemId") fv.* FROM public."FieldValues" fv WHERE fv."itemId" = ANY($1) ORDER BY fv."fieldId", fv."itemId", fv."createdAt" DESC, fv."fieldValuesId" DESC
🧹 Nitpick comments (8)
src/adapters/postgres/cohort-adapter.ts (2)
1736-1847: Simplify date validation logic and remove redundant checks.The
normalizeDateStringmethod has redundant validation:
- Lines 1756-1763 validate the date using
Dateconstructor- Lines 1811-1843 manually validate date components (month, day, hour, etc.)
The manual validation at lines 1824-1843 is redundant since
new Date()already validates parseability. Additionally, the manual range checks don't account for actual days in each month (e.g., it would accept day=31 for February if somehow the Date constructor passes).♻️ Proposed simplification
Remove the redundant manual validation block (lines 1811-1843) since the Date constructor already performs these checks:
} } - // Additional validation: check if the date components are within valid ranges - // Extract date parts to validate manually - const dateMatch = normalized.match( - /^(\d{4})-(\d{2})-(\d{2})T(\d{2}):(\d{2}):(\d{2})/ - ); - if (dateMatch) { - const [, year, month, day, hour, minute, second] = dateMatch; - const monthNum = parseInt(month, 10); - const dayNum = parseInt(day, 10); - const hourNum = parseInt(hour, 10); - const minuteNum = parseInt(minute, 10); - const secondNum = parseInt(second, 10); - - // Validate ranges - if ( - monthNum < 1 || - monthNum > 12 || - dayNum < 1 || - dayNum > 31 || - hourNum < 0 || - hourNum > 23 || - minuteNum < 0 || - minuteNum > 59 || - secondNum < 0 || - secondNum > 59 - ) { - LoggerUtil.warn( - `Invalid date component values: ${dateString} (month: ${monthNum}, day: ${dayNum}, hour: ${hourNum}, minute: ${minuteNum}, second: ${secondNum})`, - 'COHORT_DATE_NORMALIZATION' - ); - return null; - } - } - // Return the normalized string (validated and ready for storage) return normalized; }
1675-1725: Consider adding cross-field validation for date ranges.The
normalizeCohortDatesmethod validatescohort_startDateandcohort_endDateindependently, but doesn't verify that the end date is greater than or equal to the start date. This could allow invalid cohort date ranges to be stored.♻️ Add date range validation
dto.cohort_endDate = normalizedEndDate; } } +// Validate that endDate >= startDate if both are present +if ( + dto.cohort_startDate && + dto.cohort_endDate && + dto.cohort_startDate !== null && + dto.cohort_endDate !== null +) { + const startDate = new Date(dto.cohort_startDate); + const endDate = new Date(dto.cohort_endDate); + if (endDate < startDate) { + return APIResponse.error( + res, + apiId, + 'cohort_endDate must be greater than or equal to cohort_startDate', + 'Invalid date range', + HttpStatus.BAD_REQUEST + ); + } +} + return null; // No errorsrc/user/user.controller.ts (2)
137-137: Add defensive check for result.If
getUsersDetailsByIdreturnsundefinedornull, accessingresult.statusCodewill throw a TypeError. Consider adding a guard check.♻️ Suggested fix
- const statusCode = result.statusCode || 200; + const statusCode = result?.statusCode ?? 200; + + if (!result) { + LoggerUtil.error( + `GetUser failed - StatusCode: 500, Reason: EMPTY_RESPONSE, Message: Adapter returned empty result, IssueType: SERVER_ERROR, TenantId: ${tenantId}`, + 'Adapter returned empty result', + 'UserController' + ); + return response.status(500).json({ + statusCode: 500, + error: 'Unexpected empty response from adapter', + }); + }
150-165: Consider extracting status code classification to a shared utility.This failure reason and issue type classification logic is duplicated in
auth.service.ts(lines 124-146). Extracting it into a shared utility (e.g., inLoggerUtilor a newHttpErrorClassifier) would improve maintainability.♻️ Example utility
// In a shared utility file, e.g., src/common/utils/http-error-classifier.ts export function classifyHttpError(statusCode: number): { failureReason: string; issueType: string } { let failureReason = 'UNKNOWN_ERROR'; let issueType = statusCode >= 500 ? 'SERVER_ERROR' : 'CLIENT_ERROR'; if (statusCode === 400) failureReason = 'BAD_REQUEST'; else if (statusCode === 401 || statusCode === 403) failureReason = 'UNAUTHORIZED'; else if (statusCode === 404) failureReason = 'USER_NOT_FOUND'; else if (statusCode === 429) failureReason = 'RATE_LIMIT_EXCEEDED'; else if (statusCode >= 500) failureReason = 'INTERNAL_SERVER_ERROR'; return { failureReason, issueType }; }src/auth/auth.service.ts (4)
38-43: Minor: Inconsistent indentation.The indentation on lines 40-42 is inconsistent with the rest of the file (extra leading spaces).
♻️ Suggested fix
LoggerUtil.log( `Login attempt initiated - User-Agent: ${userAgent}`, - 'AuthService', - undefined, - 'info' + 'AuthService', + undefined, + 'info' );
115-115: Consider usingUnauthorizedExceptioninstead ofNotFoundExceptionfor invalid credentials.Throwing
NotFoundExceptionfor invalid username/password is semantically incorrect. The user/resource exists but the credentials are wrong, which is an authorization failure, not a "not found" scenario. This may also confuse API consumers expecting a 404 for not-found vs 401 for auth failures.♻️ Suggested fix
- throw new NotFoundException('Invalid username or password'); + throw new UnauthorizedException('Invalid username or password');
185-185: Remove unused variable.The
usernamevariable is declared but never used. Since it's excluded for compliance, consider removing the declaration entirely.♻️ Suggested fix
// Decode JWT token to get username const decoded: any = jwt_decode(request.headers.authorization); - const username = decoded.preferred_username || 'Unknown';
223-245: Note: String-based error classification is fragile.The error classification relies on matching substrings in error messages (
'token','jwt','not found', etc.). If upstream error messages change, this detection will silently fail. Consider supplementing withinstanceofchecks where possible (e.g.,e instanceof JsonWebTokenError), or accept this trade-off for diagnostic logging purposes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
src/adapters/postgres/cohort-adapter.tssrc/auth/auth.service.tssrc/user/user.controller.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
⚙️ CodeRabbit configuration file
**/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
Files:
src/auth/auth.service.tssrc/user/user.controller.tssrc/adapters/postgres/cohort-adapter.ts
🧬 Code graph analysis (2)
src/auth/auth.service.ts (3)
src/auth/dto/auth-dto.ts (1)
AuthDto(4-24)src/common/utils/api-id.config.ts (1)
APIID(21-127)src/common/logger/LoggerUtil.ts (2)
LoggerUtil(3-144)error(88-109)
src/user/user.controller.ts (2)
src/common/logger/LoggerUtil.ts (2)
LoggerUtil(3-144)error(88-109)src/utils/response.ts (1)
error(32-58)
🔇 Additional comments (8)
src/user/user.controller.ts (4)
21-21: LGTM!The
HttpStatusimport is correctly added and used for consistent HTTP status code handling in the error responses.
100-109: LGTM!The logging correctly captures the API call attempt with relevant context (tenantId, fieldValue) while properly excluding sensitive information for compliance.
111-121: LGTM with a minor observation.The error logging is correctly added. Note that the response format here (
{ statusCode, error }) differs from the standardServerResponseformat used elsewhere (e.g., viaAPIResponse.error()), but this is pre-existing behavior and outside the scope of this PR's logging enhancements.
178-196: LGTM!The exception handling block correctly captures comprehensive error details, classifies the issue type, and returns a structured error response. The use of optional chaining for
error?.messageanderror?.stackprovides safe property access.src/auth/auth.service.ts (4)
13-15: LGTM!The imports are correctly added and all are utilized in the updated method signatures and logging calls.
57-64: LGTM!The failure logging correctly distinguishes between user not found and inactive user scenarios, providing clear diagnostic information while maintaining compliance.
92-98: LGTM!Success logging correctly captures the outcome with relevant metadata while excluding sensitive information.
30-30: All callers correctly pass therequestparameter.Verified:
auth.controller.tsis the sole caller ofauthService.login()and properly passes all three parameters (authDto,request,response). Therequestparameter is correctly used in the service method for logging User-Agent information. Type safety is improved with theAuthDtoannotation.
1 participant
Summary by CodeRabbit
Bug Fixes
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.