feat(service-bff): Add support screen for Bff double session #17255
Conversation
Implements the BffDoubleSessionModal to handle cases where a user has multiple active sessions. This modal provides options to switch users or keep the current session. Enhances error handling in the AuthService by replacing generic error messages with more specific ones and adding a new method to manage missing login attempts, improving user experience during login failures.
Update the BffError class to use a number type for the statusCode instead of a string. Modify the constructor and related methods to ensure consistent handling of status codes as numbers. This change improves type safety and prevents potential issues with string-to-number conversions.
WalkthroughThis pull request introduces enhancements to the authentication and session management system across multiple files. The changes focus on improving error handling, session detection, and user experience during login processes. A new mechanism for detecting and handling multiple active sessions has been implemented, with updates to both backend and frontend components. The modifications include refactoring error types, updating token caching logic, and introducing a new modal for managing conflicting login attempts. Changes
Sequence DiagramsequenceDiagram
participant User
participant BffProvider
participant AuthService
participant BffDoubleSessionModal
User->>BffProvider: Attempt Login
BffProvider->>AuthService: Check Login
alt Active Session Exists
AuthService-->>BffProvider: Multiple Sessions Detected
BffProvider->>BffDoubleSessionModal: Show Modal
BffDoubleSessionModal->>User: Prompt to Switch or Keep Session
User->>BffDoubleSessionModal: Choose Action
BffDoubleSessionModal->>BffProvider: Handle User Choice
else No Active Session
AuthService-->>BffProvider: Login Successful
BffProvider->>User: Complete Login
end
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17255 +/- ##
==========================================
- Coverage 35.68% 35.68% -0.01%
==========================================
Files 6928 6931 +3
Lines 148752 148782 +30
Branches 42501 42509 +8
==========================================
+ Hits 53088 53096 +8
- Misses 95664 95686 +22 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 10 Total Test Services: 0 Failed, 10 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (4) |
Refactor error handling introducing a new enum for BFF error codes. Update error messages and status codes for clarity. Enhance user session management by ensuring proper handling of multiple active sessions. Implement a method to remove BFF error parameters from the URL to improve user experience. Adjust button click handlers to use arrow functions for consistency.
Enhance the cache retrieval logic in the AuthController test to improve readability. Update the documentation in AuthService to provide clearer explanations of scenarios where a login attempt cache entry may not be found. This includes specifying the conditions for cache expiration and the recovery process for login attempts, ensuring better understanding for future developers.
|
Affected services are: air-discount-scheme-api,air-discount-scheme-backend,api,application-system-api,download-service,financial-aid-api,financial-aid-backend,financial-aid-open-api,github-actions-cache,icelandic-names-registry-backend,judicial-system-api,judicial-system-digital-mailbox-api,judicial-system-backend,judicial-system-message-handler,judicial-system-scheduler,judicial-system-xrd-api,license-api,reference-backend,regulations-admin-backend,services-auth-admin-api,services-auth-ids-api,services-auth-delegation-api,services-auth-personal-representative,services-auth-personal-representative-public,services-auth-public-api,services-bff,services-contentful-entry-tagger,services-documents,services-endorsements-api,services-form-system,services-search-indexer,services-sessions,services-university-gateway,services-user-notification,services-user-profile,services-xroad-collector,skilavottord-ws,air-discount-scheme-web,auth-admin-web,consultation-portal,contentful-apps,financial-aid-web-osk,financial-aid-web-veita,judicial-system-web,skilavottord-web,web,application-system-form,island-ui-storybook,portals-admin,service-portal,system-e2e, Deployed services: application-system-api,application-system-form,service-portal,service-portal-api,portals-admin,consultation-portal,api,web,xroad-collector,skilavottord-web,skilavottord-ws,services-documents,island-ui-storybook,download-service,icelandic-names-registry-backend,endorsement-system-api,air-discount-scheme-web,air-discount-scheme-backend,air-discount-scheme-api,regulations-admin-backend,application-system-api-worker,license-api,services-sessions,services-sessions-worker,services-sessions-cleanup,services-university-gateway,services-university-gateway-worker,services-bff-portals-admin,services-bff-portals-my-pages. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
apps/services/bff/src/app/utils/create-error-query-str.ts (3)
1-4: LGTM! Consider adding type documentation.The type definition changes improve error handling flexibility with optional parameters. Consider adding JSDoc comments to document the type properties.
+/** + * Arguments for creating an error query string + * @property statusCode - HTTP status code of the error + * @property message - Optional error message + * @property code - Optional error code + */ export type CreateErrorQueryStrArgs = { statusCode: number message?: string code?: string }
8-9: Enhance function documentation.The current documentation could be more descriptive about the function's purpose, parameters, and return value.
-/** - * This utility function creates a query string with the error details - */ +/** + * Creates a URL-encoded query string containing error details for BFF error handling + * @param {CreateErrorQueryStrArgs} args - Error details including status code and optional message/code + * @returns {string} URL-encoded query string with bff_error_* parameters + * @example + * createErrorQueryStr({ statusCode: 401, message: "Unauthorized", code: "AUTH_001" }) + * // Returns: "bff_error_status_code=401&bff_error_message=Unauthorized&bff_error_code=AUTH_001" + */
10-21: LGTM! Clean implementation with consistent error parameter handling.The implementation properly handles optional parameters and maintains consistent naming with the 'bff_' prefix. The use of URLSearchParams ensures proper URL encoding.
Consider documenting these error parameter names (bff_error_*) in a central location since they seem to be part of the BFF's error handling contract with clients.
libs/react-spa/bff/src/lib/BffError.ts (2)
1-5: Consider exporting the BffErrorCodes enum for reuse.The enum provides well-structured parameter keys, but it should be exported to allow reuse across different NextJS applications that need to handle BFF errors.
-enum BffErrorCodes { +export enum BffErrorCodes {
22-33: Consider using readonly properties for immutability.The class implementation is well-structured with proper TypeScript types. Consider making the properties readonly since they should not change after initialization.
export class BffError extends Error { - code?: string - statusCode?: number + readonly code?: string + readonly statusCode?: numberlibs/react-spa/bff/src/lib/BffProvider.tsx (2)
42-42: Address the TODO comment for dynamic error handlingThe TODO comment suggests making the error screen display dynamic depending on the error code. Consider implementing this logic now to ensure proper error handling before merging.
Do you need assistance implementing dynamic error handling? I can help with code suggestions or open a GitHub issue to track this task.
210-218: Remove debugconsole.logstatementThe
console.logstatement appears to be used for debugging purposes. It's recommended to remove it to prevent unintended logging in production environments.apps/services/bff/src/app/modules/auth/auth.service.ts (1)
245-299: Add unit tests for the newhandleMissingLoginAttemptmethodTo ensure robust error handling and session management, consider adding unit tests for the
handleMissingLoginAttemptmethod. This will help validate its logic under different scenarios and improve code reliability.libs/react-spa/bff/src/lib/BffDoubleSession.tsx (1)
5-8: Consider enhancing props interfaceThe props interface could be improved by:
- Adding JSDoc comments for better documentation
- Consider adding an optional
onClosehandler for better modal controltype BffDoubleSessionModalProps = { + /** Callback when user chooses to switch to new session */ onSwitchUser(): void + /** Callback when user chooses to keep current session */ onKeepCurrentUser(): void + /** Optional callback when modal is dismissed */ + onClose?(): void }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/services/bff/src/app/modules/auth/auth.controller.spec.ts(9 hunks)apps/services/bff/src/app/modules/auth/auth.service.ts(12 hunks)apps/services/bff/src/app/modules/auth/auth.types.ts(1 hunks)apps/services/bff/src/app/modules/auth/token-refresh.service.ts(1 hunks)apps/services/bff/src/app/utils/create-error-query-str.ts(1 hunks)libs/react-spa/bff/src/lib/BffDoubleSession.css.ts(1 hunks)libs/react-spa/bff/src/lib/BffDoubleSession.tsx(1 hunks)libs/react-spa/bff/src/lib/BffError.ts(1 hunks)libs/react-spa/bff/src/lib/BffProvider.tsx(8 hunks)libs/react-spa/bff/src/lib/bff.state.ts(3 hunks)libs/shared/constants/src/index.ts(1 hunks)libs/shared/constants/src/lib/errorCodes.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/shared/constants/src/lib/errorCodes.ts
👮 Files not reviewed due to content moderation or server errors (2)
- apps/services/bff/src/app/modules/auth/token-refresh.service.ts
- apps/services/bff/src/app/modules/auth/auth.controller.spec.ts
🧰 Additional context used
📓 Path-based instructions (11)
libs/react-spa/bff/src/lib/BffDoubleSession.css.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
apps/services/bff/src/app/modules/auth/token-refresh.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/shared/constants/src/index.ts (2)
Pattern libs/shared/**/*: "Confirm that the code adheres to the following:
- Cross-application utility functions, types, and constants.
- TypeScript best practices to ensure type safety and reusability.
- Documentation and examples for library consumers."
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/react-spa/bff/src/lib/bff.state.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
apps/services/bff/src/app/utils/create-error-query-str.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/auth/auth.types.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/react-spa/bff/src/lib/BffDoubleSession.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
apps/services/bff/src/app/modules/auth/auth.controller.spec.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/react-spa/bff/src/lib/BffProvider.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
apps/services/bff/src/app/modules/auth/auth.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/react-spa/bff/src/lib/BffError.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (8)
libs/react-spa/bff/src/lib/BffError.ts (1)
1-67: Verify integration with session handling logic.
While the error handling implementation is solid, we should verify its integration with the session management system described in the PR objectives.
✅ Verification successful
BffError implementation correctly handles session-related scenarios
The verification confirms that BffError is properly integrated with session handling:
- It's used in BffProvider for handling session-related errors, including the double session scenario (code: LOGIN_ATTEMPT_FAILED_ACTIVE_SESSION)
- The error parameters are properly managed through URL parameters and cleaned up after processing
- The implementation aligns with the auth flow in the BFF service, which handles session cookies and unauthorized states
- The error class is used to communicate session state changes across browser tabs/windows through the BffPoller
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of BffError in session-related code
echo "Searching for session-related error handling..."
rg -l "LOGIN_ATTEMPT_FAILED_ACTIVE_SESSION" | xargs rg "BffError"
# Look for any existing error handling in auth controllers
echo "Checking auth controller implementations..."
fd -e ts -e js "auth.controller" | xargs rg "throw.*Error"
Length of output: 780
Script:
#!/bin/bash
# Let's search for session-related error handling patterns
echo "Searching for session-related constants and error codes..."
rg -A 3 "const.*SESSION|enum.*SESSION|type.*SESSION"
# Look for BffError usage in authentication flows
echo "Checking BffError usage in authentication..."
rg -A 3 "new BffError.*SESSION|throw.*BffError"
# Search for any session-related error handling
echo "Looking for session error handling patterns..."
rg -A 5 "session.*error|error.*session" -i
Length of output: 18433
libs/react-spa/bff/src/lib/BffProvider.tsx (1)
169-169: Verify the BffError instantiation for error dispatch
Ensure that new BffError(error.message) provides all necessary information required by the BffError constructor (e.g., code, statusCode). This will ensure proper error handling and consistent error messages.
libs/react-spa/bff/src/lib/BffDoubleSession.css.ts (1)
1-6: New CSS module looks good
The styles are correctly defined using @vanilla-extract/css, and the theme spacing is appropriately applied.
libs/shared/constants/src/index.ts (1)
3-8: Verify consistency of export statements
Please ensure that the export statements for chargeItemCode, errorCodes, and locale are consistent and not duplicated or unintentionally removed elsewhere. This helps maintain the integrity of shared constants across the application.
apps/services/bff/src/app/modules/auth/auth.types.ts (2)
30-30: Verify impact of removed originUrl property
The change to make targetLinkUri required improves type safety, but we should verify the removal of originUrl doesn't break existing code.
✅ Verification successful
Removal of originUrl property is safe
The verification shows that originUrl is only used in a test file (user.controller.spec.ts) and not in any actual implementation code. The LoginAttemptData type is used in several places but none of them reference the removed originUrl property. All usages are compatible with the updated type that only requires targetLinkUri and codeVerifier.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to originUrl
rg "originUrl.*LoginAttemptData" -t typescript
Length of output: 83
Script:
#!/bin/bash
# Search for any remaining references to originUrl in typescript files
rg "originUrl" --type ts -A 2
# Also search for LoginAttemptData usage
rg "LoginAttemptData" --type ts -A 2
Length of output: 2408
23-26: LGTM: Well-structured type for handling multiple login attempts
The addition of loginAttemptData as an optional map of LoginAttemptData is a clean approach to track multiple sessions.
Let's verify the usage of this new type:
✅ Verification successful
Type changes are properly implemented and used consistently
The new loginAttemptData property is well integrated into the codebase:
- Used in auth.service.ts for managing multiple login attempts
- Properly accessed with null checks (using optional chaining)
- Correctly implements the type safety for targetLinkUri access
- No breaking changes found in the implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for places where loginAttemptData is accessed or modified
rg "loginAttemptData" -t typescript
Length of output: 70
Script:
#!/bin/bash
# Search for places where loginAttemptData is accessed or modified, using ripgrep without file type filter
rg "loginAttemptData"
Length of output: 1149
Script:
#!/bin/bash
# Find TypeScript files and search for loginAttemptData
fd -e ts -e tsx -x rg "loginAttemptData" {}
Length of output: 562
Script:
#!/bin/bash
# Use ast-grep to find object property access patterns
ast-grep --pattern 'loginAttemptData'
Length of output: 825
libs/react-spa/bff/src/lib/bff.state.ts (2)
27-27: LGTM: Improved type safety with BffError
Using BffError instead of generic Error provides better type safety and error handling capabilities.
61-61: Verify error handling implementation
The Action type now uses BffError, but we should verify the error handling implementation in components that dispatch ERROR actions.
✅ Verification successful
Error handling implementation is properly implemented
The verification shows that the BffError is correctly used in the error handling flow:
BffProvider.tsxdispatches ERROR actions withBffErrorpayloads in two scenarios:- When the BFF server is unavailable
- When errors are detected from URL parameters
- The error state and actions are properly typed in
bff.state.ts - A dedicated
BffErrorclass is implemented with proper error handling utilities
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ERROR action dispatches
rg "type:\s*ActionType.ERROR" -t typescript
Length of output: 80
Script:
#!/bin/bash
# Search for ERROR action dispatches with ripgrep
rg "type.*ERROR" --type ts
# Also search for BffError usage
rg "BffError" --type ts
Length of output: 4920
Remove unnecessary console logs during login redirection and directly set the window location for a cleaner flow. Enhance the button wrapper styles for better responsiveness and alignment in the BffDoubleSession component.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
libs/react-spa/bff/src/lib/BffDoubleSession.css.ts (1)
4-15: Consider enhancing maintainability and documentation.While the implementation is solid and follows the design system, consider these improvements:
- Add JSDoc comments to document the component's purpose and usage
- Consider extracting the media query pattern if it's reused across components
Here's a suggested enhancement:
+/** + * Styles for the double session modal button wrapper + * Provides responsive layout switching from column to inline at medium breakpoint + */ export const buttonWrapper = style({ marginTop: theme.spacing[2], display: 'inline-flex', flexDirection: 'column', alignItems: 'center', '@media': { [`screen and (min-width: ${theme.breakpoints.md}px)`]: { display: 'inline', }, }, })libs/react-spa/bff/src/lib/BffProvider.tsx (4)
144-144: Consider localizing error messagesGood improvement using structured BffError instead of generic Error. However, consider extracting the error message to a localization constant for better maintainability and internationalization support.
- throw new BffError(BFF_SERVER_UNAVAILABLE, res.status) + throw new BffError(ERRORS.SERVER_UNAVAILABLE, res.status) // In a separate constants file: + export const ERRORS = { + SERVER_UNAVAILABLE: 'BFF_SERVER_UNAVAILABLE', + // Add other error messages here + } as const;Also applies to: 169-169
221-231: Consider adding error boundary protectionThe error handling logic is good, but consider wrapping the component in an error boundary to gracefully handle any uncaught errors during the effect execution.
// In a separate ErrorBoundary component: class BffErrorBoundary extends React.Component { componentDidCatch(error: Error) { // Handle unexpected errors dispatch({ type: ActionType.ERROR, payload: new BffError(error.message), }) } render() { return this.props.children } } // Usage: <BffErrorBoundary> <BffProvider {...props} /> </BffErrorBoundary>
247-261: Extract HTTP status code to constantThe double session handling logic is well-implemented, but the HTTP status code 409 should be extracted to a named constant for better maintainability and readability.
+ const HTTP_STATUS = { + CONFLICT: 409, + // Add other status codes as needed + } as const; if ( showErrorScreen && - error?.statusCode === 409 && + error?.statusCode === HTTP_STATUS.CONFLICT && error?.code === LOGIN_ATTEMPT_FAILED_ACTIVE_SESSION )
Line range hint
1-300: Architecture aligns with coding guidelinesThe implementation successfully meets the coding guidelines for libs/**/* files:
- ✅ Component is reusable across NextJS apps
- ✅ Proper TypeScript usage for props and exports
- ✅ Clean tree-shaking support with proper exports
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/react-spa/bff/src/lib/BffDoubleSession.css.ts(1 hunks)libs/react-spa/bff/src/lib/BffDoubleSession.tsx(1 hunks)libs/react-spa/bff/src/lib/BffProvider.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/react-spa/bff/src/lib/BffDoubleSession.tsx
🧰 Additional context used
📓 Path-based instructions (2)
libs/react-spa/bff/src/lib/BffDoubleSession.css.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/react-spa/bff/src/lib/BffProvider.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (3)
libs/react-spa/bff/src/lib/BffDoubleSession.css.ts (1)
1-2: LGTM! Clean imports and proper usage of design system.
The file correctly imports the theme from the design system and vanilla-extract utilities.
libs/react-spa/bff/src/lib/BffProvider.tsx (2)
5-5: LGTM: New imports support double session handling
The added imports properly support the new double session feature while maintaining clean module organization.
Also applies to: 7-8
107-107: LGTM: Enhanced URL security
Good security practice to clean error parameters from URLs before redirect. This prevents potential information leakage through URL parameters.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/services/bff/src/app/modules/cache/cache.service.ts (1)
Missing test coverage for cache service key handling methods
The cache service is heavily used by the auth module, but there is no direct test coverage for the key handling methods (
keyWithoutSid,createKeyError,createSessionKeyType). While these methods are indirectly tested through the auth module's tests, dedicated unit tests would ensure better maintainability and catch potential edge cases.Key areas needing test coverage:
keyWithoutSidmethod for different key formats and edge casescreateKeyErrormethod for error message formattingcreateSessionKeyTypemethod for both 'attempt' and 'current' types🔗 Analysis chain
Line range hint
1-96: Verify test coverage for new cache key handling.While the implementation follows NestJS architectural patterns and dependency injection practices, ensure that unit tests are added to cover:
- Session ID removal from different key formats
- Error handling scenarios
- Edge cases in key processing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for test coverage of the cache service # Look for test files fd "cache.service.spec.ts" "apps/services/bff/src" # Check test content if file exists rg "keyWithoutSid|createKeyError|createSessionKeyType" "apps/services/bff/src/**/*.spec.ts"Length of output: 217
Script:
#!/bin/bash # Let's verify if there are any test files in the bff service directory fd ".spec.ts$" "apps/services/bff/src" # Let's also check if there's a __tests__ directory fd -t d "__tests__" "apps/services/bff/src" # Let's check for any test files that might reference the cache service rg -l "CacheService" "apps/services/bff/src/**/*.spec.ts" "apps/services/bff/src/**/*.test.ts" || trueLength of output: 759
Script:
#!/bin/bash # Let's check the content of specific test files that might use cache service cat "apps/services/bff/src/app/modules/auth/auth.controller.spec.ts" cat "apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts" # Let's also check for cache service usage in the implementation files rg "CacheService" "apps/services/bff/src/app/modules/auth/"Length of output: 26863
🧹 Nitpick comments (21)
apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts (1)
Line range hint
176-182: Consider adding test coverage for session ID logging.While the error message has been simplified, it would be valuable to add a separate test case that verifies whether the session ID is properly handled in error scenarios, especially given the context of the PR's focus on double session management.
Here's a suggested additional test case:
it('should include session details in debug logs for errors', async () => { // Arrange const error = new Error('Refresh token failed') jest.spyOn(idsService, 'refreshToken').mockRejectedValueOnce(error) // Act await service.refreshToken({ sid: testSid, encryptedRefreshToken: testRefreshToken, }) // Assert expect(mockLogger.debug).toHaveBeenCalledWith( expect.stringContaining(testSid), expect.any(Error) ) })apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts (2)
14-14: Add test coverage for encryption edge casesWhile the setup for CryptoService is good, consider adding test cases to verify:
- Encryption actually transforms the session ID
- Error handling when encryption fails
- Decryption produces the original value
// Add these test cases it('should properly encrypt session ID', () => { expect(encryptedSid).not.toEqual(SID_VALUE) expect(cryptoService.decrypt(encryptedSid)).toEqual(SID_VALUE) }) it('should handle encryption errors', () => { expect(() => cryptoService.encrypt(null)).toThrow() })Also applies to: 64-89
134-134: Consider grouping session-related test casesThe test cases thoroughly cover session handling, but could be better organized. Consider grouping related test cases using describe blocks:
describe('session handling', () => { // Group tests related to session validation // e.g., unauthorized access, session encryption }) describe('token refresh', () => { // Group tests related to token refresh // e.g., expired tokens, polling })Also applies to: 146-146, 152-152, 180-180, 185-185, 198-198, 203-203, 228-228, 233-233, 262-262, 267-267, 308-308, 313-313
apps/services/bff/src/app/modules/auth/auth.controller.spec.ts (3)
92-94: Consider extracting session encryption setup to a helper function.The session ID encryption setup could be moved to a helper function to improve test maintainability and reusability.
+ function getEncryptedSessionId(cryptoService: CryptoService, sessionId: string): string { + return cryptoService.encrypt(sessionId, true) + } - // Encrypt the session ID for cookie - encryptedSid = cryptoService.encrypt(SID_VALUE, true) + encryptedSid = getEncryptedSessionId(cryptoService, SID_VALUE)
546-597: Enhance test readability and maintainability.While the test case thoroughly covers the double session scenario, consider these improvements:
- Extract cache key construction to constants or helper functions
- Make the test data setup more explicit with descriptive variable names
+ const createCacheKey = (type: 'attempt' | 'current', name: string, sid: string) => + `${type}::${name}::${sid}` - const firstAttemptKey = `attempt::${mockConfig.name}::${SID_VALUE}` - const currentKey = `current::${mockConfig.name}::${SID_VALUE}` + const firstAttemptKey = createCacheKey('attempt', mockConfig.name, SID_VALUE) + const currentKey = createCacheKey('current', mockConfig.name, SID_VALUE) + // Explicitly define the structure of cached session data + interface CachedSessionData { + loginAttemptData: { + [key: string]: { + codeVerifier: string + targetLinkUri: string + } + } + } - const cachedData = { + const existingSessionData = { ...tokensResponse, loginAttemptData: { [firstAttemptKey]: { codeVerifier: 'test_verifier', targetLinkUri: baseUrlWithKey, }, }, }
550-554: Consider adding error response type checking.The error response structure should be type-checked to ensure consistency across the application.
+ interface BffErrorResponse { + bff_error_status_code: string + bff_error_message: string + bff_error_code?: string + } - const searchParams = new URLSearchParams({ + const errorResponse: BffErrorResponse = { bff_error_status_code: '409', bff_error_message: 'Multiple sessions detected!', bff_error_code: LOGIN_ATTEMPT_FAILED_ACTIVE_SESSION, }) + const searchParams = new URLSearchParams(errorResponse)apps/services/bff/src/app/modules/cache/cache.service.ts (3)
10-10: Consider using a readonly class constant for the separator.Since the separator is a constant value used throughout the class, it would be more appropriate to declare it as a readonly class constant.
- private separator = '::' + private static readonly SEPARATOR = '::'
20-37: LGTM! Consider performance optimization for key processing.The implementation effectively prevents session ID leakage in logs and handles invalid formats gracefully. However, for better performance, consider using a regex pattern instead of multiple string splits and joins.
private keyWithoutSid(key: string) { - if (key.split(this.separator).length !== 3) { + const pattern = new RegExp(`^([^${this.separator}]+${this.separator}[^${this.separator}]+)${this.separator}.*$`); + const match = key.match(pattern); + if (!match) { return key; } - return key.split(this.separator).slice(0, 2).join(this.separator) + return match[1]; }
40-40: LGTM! Consider using custom error types.The error handling effectively prevents session ID leakage. To improve error handling further, consider implementing custom error types for cache-related errors.
Example implementation:
export class CacheKeyNotFoundError extends Error { constructor(key: string) { super(`Cache key "${key}" not found.`); this.name = 'CacheKeyNotFoundError'; } } export class CacheDeleteError extends Error { constructor(key: string) { super(`Failed to delete key "${key}" from cache.`); this.name = 'CacheDeleteError'; } }Also applies to: 92-94
apps/services/bff/src/app/modules/auth/auth.module.ts (1)
4-4: LGTM! Consider documenting the module's enhanced session management capabilities.The addition of SessionCookieService follows NestJS best practices for dependency injection and module organization. This change appropriately supports the new double session detection feature.
Consider adding a module-level JSDoc comment describing the enhanced session management capabilities, as this module now plays a crucial role in handling double sessions.
Also applies to: 11-17
apps/services/bff/src/app/modules/proxy/proxy.module.ts (1)
4-4: Consider creating a SharedModule for session management.The SessionCookieService is now used across multiple modules (Auth, User, and Proxy). While the current implementation is correct, consider creating a SharedModule to centralize session management services.
This refactoring would:
- Reduce code duplication in module declarations
- Centralize session management configuration
- Make the session management feature more maintainable
Example structure:
@Module({ providers: [SessionCookieService], exports: [SessionCookieService], }) export class SharedModule {}Also applies to: 20-20
apps/services/bff/src/app/services/sessionCookie.service.ts (3)
20-32: Consider adding error type safety.While the error handling is good, consider defining a custom error type or using a specific error class for better type safety.
+ type CookieEncryptionError = { + message: string; + code: 'ENCRYPTION_FAILED' | 'COOKIE_SET_FAILED'; + }; set({ res, value }: { res: Response; value: string }): void { try { const encryptedValue = this.cryptoService.encrypt(value, true) res.cookie(SESSION_COOKIE_NAME, encryptedValue, getCookieOptions()) } catch (error) { - this.logger.error('Error setting encrypted session cookie: ', { + const typedError = error as CookieEncryptionError; + this.logger.error('Error setting encrypted session cookie: ', { - message: error.message, + message: typedError.message, + code: typedError.code, }) throw error } }
40-54: Consider making the return type more specific.The method could benefit from a more specific return type to better indicate the expected format of the decrypted value.
- get(req: Request): string | undefined { + get(req: Request): SessionId | undefined { + type SessionId = string;
1-62: Consider extracting cookie configuration to a separate injectable service.The cookie configuration could be moved to a separate injectable service for better maintainability and reusability.
This would allow for:
- Centralized cookie configuration management
- Easier testing through dependency injection
- More flexible cookie options based on environment
apps/services/bff/src/app/modules/user/user.service.ts (1)
Line range hint
44-49: Consider adding specific error message for missing session.The error handling could be more informative when the session is missing.
const sid = this.sessionCookieService.get(req) if (!sid) { - throw new UnauthorizedException() + throw new UnauthorizedException('No active session found') }apps/services/bff/src/app/services/error.service.ts (1)
82-85: Consider adding order-dependent operation handling.The session clearing and cache deletion operations should be handled with proper error handling and potentially in a specific order.
- this.sessionCookieService.clear(res) - await this.cacheService.delete(tokenResponseKey) + try { + // Clear session first to prevent user actions with invalid cache + this.sessionCookieService.clear(res) + await this.cacheService.delete(tokenResponseKey) + } catch (cleanupError) { + this.logger.error('Failed to cleanup session:', cleanupError) + // Still throw UnauthorizedException as the session is invalid + } throw new UnauthorizedException()apps/services/bff/src/app/services/crypto.service.spec.ts (1)
93-142: LGTM! Well-structured test suite with comprehensive coverage.The test suite effectively covers the new
skipAlgorithmPrefixfunctionality with clear arrange/act/assert patterns and good edge cases.Consider making the test descriptions more specific:
- it('should encrypt and decrypt with algorithm prefix (default behavior)', () => { + it('should include algorithm prefix in encrypted text and successfully decrypt when skipAlgorithmPrefix is false', () => {apps/services/bff/src/app/services/crypto.service.ts (1)
Line range hint
35-69: LGTM! Secure implementation with good documentation.The encryption implementation properly handles the new
skipAlgorithmPrefixparameter while maintaining backward compatibility.Consider adding specific error types for different failure scenarios:
- throw new Error('Failed to encrypt the text.') + throw new Error(`Failed to encrypt the text: ${error.message}`)apps/services/bff/src/app/modules/proxy/proxy.service.ts (1)
87-87: LGTM! Clean integration of session cookie service.The change properly uses the new service while maintaining existing security measures.
Consider enhancing error logging when session ID is missing:
- if (!sid) { + if (!sid) { + this.logger.warn('No session ID found in request') throw new UnauthorizedException() }apps/services/bff/src/app/modules/user/user.controller.spec.ts (2)
89-89: Consider adding encryption failure test casesWhile the encryption setup is well-implemented, consider adding test cases for encryption failure scenarios to ensure robust error handling.
Example test case to consider:
it('should handle encryption failures gracefully', async () => { // Mock encryption failure jest.spyOn(cryptoService, 'encrypt').mockImplementation(() => { throw new Error('Encryption failed'); }); const res = await server.get('/user') .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]); expect(res.status).toEqual(HttpStatus.INTERNAL_SERVER_ERROR); });Also applies to: 92-94
122-122: Consider refactoring test matrix for better maintainabilityThe test coverage is comprehensive and properly validates the encrypted session handling. However, the test matrix could be refactored for better maintainability.
Consider extracting the test matrix into a separate helper:
interface TokenRefreshTestCase { name: string; exists: boolean; expired: boolean; refresh: boolean; expectedStatus: number; shouldCallRefresh: boolean; } const TOKEN_REFRESH_TEST_CASES: TokenRefreshTestCase[] = [ { name: 'should refresh when token exists, is expired, and refresh=true', exists: true, expired: true, refresh: true, expectedStatus: HttpStatus.OK, shouldCallRefresh: true, }, // ... other cases ]; // Then use in test: for (const testCase of TOKEN_REFRESH_TEST_CASES) { it(testCase.name, async () => { // Test implementation using testCase parameters }); }Also applies to: 139-139, 147-147, 172-172, 189-189, 214-214, 231-231, 253-253, 270-270, 337-337
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/services/bff/src/app/modules/auth/auth.controller.spec.ts(17 hunks)apps/services/bff/src/app/modules/auth/auth.module.ts(1 hunks)apps/services/bff/src/app/modules/auth/auth.service.ts(15 hunks)apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts(1 hunks)apps/services/bff/src/app/modules/auth/token-refresh.service.ts(3 hunks)apps/services/bff/src/app/modules/cache/cache.service.ts(4 hunks)apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts(10 hunks)apps/services/bff/src/app/modules/proxy/proxy.module.ts(2 hunks)apps/services/bff/src/app/modules/proxy/proxy.service.ts(3 hunks)apps/services/bff/src/app/modules/user/user.controller.spec.ts(12 hunks)apps/services/bff/src/app/modules/user/user.module.ts(2 hunks)apps/services/bff/src/app/modules/user/user.service.ts(3 hunks)apps/services/bff/src/app/services/crypto.service.spec.ts(2 hunks)apps/services/bff/src/app/services/crypto.service.ts(3 hunks)apps/services/bff/src/app/services/error.service.ts(3 hunks)apps/services/bff/src/app/services/sessionCookie.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/services/bff/src/app/modules/auth/token-refresh.service.ts
- apps/services/bff/src/app/modules/auth/auth.service.ts
🧰 Additional context used
📓 Path-based instructions (14)
apps/services/bff/src/app/modules/proxy/proxy.module.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/services/error.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/user/user.module.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/auth/auth.module.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/proxy/proxy.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/user/user.controller.spec.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/user/user.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/services/sessionCookie.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/services/crypto.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/auth/auth.controller.spec.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/services/crypto.service.spec.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/cache/cache.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (12)
apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts (1)
181-181: LGTM! Simplified error message expectation.
The change to use a simpler error message in the test aligns with good logging practices by focusing on the essential information.
apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts (1)
Line range hint 1-320: Test implementation follows NestJS best practices
The test file demonstrates good practices:
- Proper dependency injection and service mocking
- Comprehensive test coverage including error scenarios
- Clean setup and teardown
- Good separation of concerns
apps/services/bff/src/app/modules/auth/auth.controller.spec.ts (1)
Line range hint 1-16: LGTM! Import changes align with double session handling requirements.
The addition of LOGIN_ATTEMPT_FAILED_ACTIVE_SESSION constant and the use of CryptoService enhance the security and error handling capabilities.
apps/services/bff/src/app/modules/user/user.module.ts (1)
4-4: LGTM! Verify circular dependency prevention.
The SessionCookieService integration follows NestJS best practices. Since both UserModule and AuthModule now use SessionCookieService, ensure circular dependencies are prevented.
Let's verify the dependency structure:
Also applies to: 20-20
✅ Verification successful
No circular dependency detected - implementation follows best practices
The SessionCookieService is correctly shared between AuthModule and UserModule as a common service, with proper dependency management:
- SessionCookieService is defined in a shared services directory (
../../services/) - AuthModule exports only AuthService, not SessionCookieService
- Both modules import SessionCookieService independently from the shared location
- UserModule imports specific services from auth (TokenRefreshService) without creating circular references
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential circular dependencies between AuthModule and UserModule
rg -A 5 "import.*from.*auth" apps/services/bff/src/app/modules/user/
rg -A 5 "import.*from.*user" apps/services/bff/src/app/modules/auth/
Length of output: 1979
Script:
#!/bin/bash
# Let's check the module declarations and dependencies in both modules
ast-grep --pattern 'class AuthModule {
$$$
}'
ast-grep --pattern '@Module({
$$$
})
export class AuthModule'
ast-grep --pattern '@Module({
$$$
})
export class UserModule'
# Check if SessionCookieService is used in auth module
rg -A 5 "SessionCookieService" apps/services/bff/src/app/modules/auth/
Length of output: 3476
apps/services/bff/src/app/services/sessionCookie.service.ts (1)
9-15: LGTM! Proper dependency injection setup.
The service follows NestJS best practices with proper dependency injection and decorator usage.
apps/services/bff/src/app/modules/user/user.service.ts (1)
7-7: LGTM! Clean integration of SessionCookieService.
The service is properly imported and injected following NestJS patterns.
Also applies to: 19-19
apps/services/bff/src/app/services/error.service.ts (1)
6-6: LGTM! Clean integration of SessionCookieService.
The service is properly imported and injected following NestJS patterns.
Also applies to: 36-36
apps/services/bff/src/app/services/crypto.service.spec.ts (1)
Line range hint 6-31: LGTM! Robust test configuration.
The test setup properly mocks crypto functionality and includes both valid and invalid configurations for thorough testing.
apps/services/bff/src/app/services/crypto.service.ts (1)
81-109: LGTM! Well-documented decryption logic with proper validation.
The decryption implementation includes good input validation and clear documentation of the format parsing logic.
Let's verify that all current usages of decrypt method are updated with the correct skipAlgorithmPrefix parameter:
✅ Verification successful
Verified: All decrypt method usages are properly handled
The codebase scan shows that there is only one direct usage of the modified cryptoService.decrypt() method with the new skipAlgorithmPrefix parameter in sessionCookie.service.ts, where it's correctly set to true. All other usages of decrypt are either:
- Different implementations (CryptoJS.AES.decrypt, crypto.subtle.decrypt)
- Test cases in crypto.service.spec.ts that properly test both parameter variations
- Calls to cryptoService.decrypt() without the optional parameter, which defaults to false as intended
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for decrypt method calls to ensure they're properly updated
ast-grep --pattern 'cryptoService.decrypt($_)'
Length of output: 48
Script:
#!/bin/bash
# Search for decrypt method calls with different patterns to catch various usage scenarios
rg -l "decrypt\(" --type ts
# Then let's examine the content of these files
for file in $(rg -l "decrypt\(" --type ts); do
echo "=== $file ==="
rg -A 2 "decrypt\(" "$file"
done
Length of output: 6490
apps/services/bff/src/app/modules/proxy/proxy.service.ts (1)
Line range hint 22-72: LGTM! Good separation of concerns.
The integration of SessionCookieService properly centralizes session cookie management and follows NestJS dependency injection patterns.
apps/services/bff/src/app/modules/user/user.controller.spec.ts (2)
14-14: LGTM: Clean implementation of crypto service setup
The addition of CryptoService and related variables follows NestJS best practices and proper TypeScript typing.
Also applies to: 74-75
Line range hint 1-353: Verify NestJS architecture compliance
The test file follows NestJS best practices and guidelines, including:
- Proper use of dependency injection
- Comprehensive test coverage
- Clear separation of concerns
✅ Verification successful
Let me gather more information about the controller implementation to ensure all methods are properly tested.
Let me try one more verification to check the test coverage specifically for the getUser endpoint.
NestJS architecture compliance verified
The test file demonstrates excellent compliance with NestJS architecture and testing best practices:
- Proper controller structure with a single
getUserendpoint - Comprehensive test coverage for the endpoint including:
- Authentication checks
- Token refresh scenarios
- Error cases
- Success cases
- Proper dependency injection and mocking
- Clear test organization using describe/it blocks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify test coverage and related service files
# Check for corresponding service file
fd --type f "user.controller.ts" "apps/services/bff/src/app/modules/user/"
# Check for proper module structure
fd --type f "user.module.ts" "apps/services/bff/src/app/modules/user/"
# Look for potential missing test cases by finding all public methods
ast-grep --pattern 'export class UserController {
$$$
$method($_) {
$$$
}
$$$
}'
Length of output: 348
Script:
#!/bin/bash
# Get the controller implementation to check test coverage
cat apps/services/bff/src/app/modules/user/user.controller.ts
# Check test file structure for all controller methods
rg "describe\(.*it\(" apps/services/bff/src/app/modules/user/user.controller.spec.ts
Length of output: 959
Script:
#!/bin/bash
# Search for test cases specifically related to the getUser endpoint
rg -A 2 "describe\('GET /user" apps/services/bff/src/app/modules/user/user.controller.spec.ts
# Check if UserService is properly mocked in tests
rg "UserService" apps/services/bff/src/app/modules/user/user.controller.spec.ts
Length of output: 315
snaerth
force-pushed
the
bff-double-session
branch
2 times, most recently
from
73fcd2e to
8ba56e1
Compare
Removes unnecessary cache checks for existing sessions in the authentication process. The code now directly returns a conflict response when a session ID is present, streamlining the logic. Additionally, it eliminates the `loginAttemptData` from the `updateTokenCache` method, as it is no longer needed, reducing complexity and improving maintainability.
Refactor the `onRetry` function to use `useCallback` for better performance and prevent unnecessary re-renders. Introduce a new `onKeepCurrentUser` function that removes BFF error parameters from the URL and calls `onRetry`.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/services/bff/src/app/modules/auth/auth.service.ts (4)
90-96: Consider enhancing type safety for error handlingThe error handling could be improved by using an enum or const object for status codes to prevent magic numbers and ensure type safety.
+const enum ErrorStatus { + INTERNAL_ERROR = 500, + BAD_REQUEST = 400, + CONFLICT = 409, +} -const statusCode = args?.statusCode || 500 +const statusCode = args?.statusCode || ErrorStatus.INTERNAL_ERROR
243-281: Enhance error logging for better debuggingThe method handles the double session scenario well, but could benefit from more detailed logging to aid in debugging production issues.
if (sid) { + this.logger.warn( + `Multiple sessions detected - Existing session: ${sid}, Attempt key: ${loginAttemptCacheKey}` + ) return this.redirectWithError(res, { statusCode: 409, code: LOGIN_ATTEMPT_FAILED_ACTIVE_SESSION, message: 'Multiple sessions detected!', }) }
354-356: Improve error logging for cache deletion failuresThe catch block should include more context about the failed operation.
-this.cacheService.delete(loginAttemptCacheKey).catch((err) => { - this.logger.warn(err) -}) +this.cacheService.delete(loginAttemptCacheKey).catch((err) => { + this.logger.warn(`Failed to delete login attempt cache key: ${loginAttemptCacheKey}`, err) +})
Line range hint
1-582: LGTM! Excellent NestJS architectural implementationThe service demonstrates proper:
- Dependency injection patterns
- Service encapsulation
- Error handling and logging practices
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/services/bff/src/app/modules/auth/auth.service.ts(11 hunks)apps/services/bff/src/app/modules/auth/auth.types.ts(1 hunks)libs/react-spa/bff/src/lib/BffProvider.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/services/bff/src/app/modules/auth/auth.types.ts
- libs/react-spa/bff/src/lib/BffProvider.tsx
🧰 Additional context used
📓 Path-based instructions (1)
apps/services/bff/src/app/modules/auth/auth.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (1)
apps/services/bff/src/app/modules/auth/auth.service.ts (1)
308-309: LGTM! Consistent error handling implementation
The standardized error handling approach across different methods improves code maintainability and user experience.
Also applies to: 321-321, 440-441
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/services/bff/src/app/modules/auth/auth.controller.spec.ts (1)
534-572: Enhance double session test coverageWhile the test case effectively validates the double session detection, consider these improvements:
- Add cache cleanup after the test to prevent potential side effects
- Add assertions to verify the cache state after the test
it('should detect double session when attempt data exists in current session', async () => { // Arrange const code = 'testcode' const getCacheSpy = jest.spyOn(mockCacheManagerValue, 'get') const searchParams = new URLSearchParams({ bff_error_status_code: '409', bff_error_message: 'Multiple sessions detected!', bff_error_code: LOGIN_ATTEMPT_FAILED_ACTIVE_SESSION, }) const errorUrl = `${baseUrlWithKey}?${searchParams.toString()}` // First login - create initial session with attempt data await server.get('/login') const currentKey = `current::${mockConfig.name}::${SID_VALUE}` mockCacheManagerValue.set(currentKey, tokensResponse) getCacheSpy.mockImplementation((key) => { if (key === currentKey) { return Promise.resolve(tokensResponse) } return Promise.resolve(null) }) // Second login attempt (user pressed back) await server.get('/login') // Act - Try to complete second login const res = await server .get('/callbacks/login') .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) .query({ code, state: SID_VALUE }) // Assert expect(res.status).toEqual(HttpStatus.FOUND) expect(res.headers.location).toEqual(errorUrl) + // Verify cache state + const cachedSession = await mockCacheManagerValue.get(currentKey) + expect(cachedSession).toEqual(tokensResponse) + + // Cleanup + await mockCacheManagerValue.del(currentKey) })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/services/bff/src/app/modules/auth/auth.controller.spec.ts(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/services/bff/src/app/modules/auth/auth.controller.spec.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (2)
apps/services/bff/src/app/modules/auth/auth.controller.spec.ts (2)
120-120: LGTM: Proper fallback handling for target link URI
The implementation correctly handles the target link URI fallback to baseUrlWithKey when undefined, and the test cases thoroughly cover both scenarios.
Also applies to: 319-319
172-173: Verify error response field changes in controller implementation
The test cases have been updated to use the new error response structure (bff_error_status_code and bff_error_message). Let's ensure these changes are reflected in the controller implementation.
Also applies to: 249-250, 268-269, 380-381
✅ Verification successful
Error response field changes are correctly implemented
The changes in the test file match the actual implementation. The createErrorQueryStr utility function in create-error-query-str.ts uses the new field names (bff_error_status_code and bff_error_message), and these are consistently used throughout the auth service's error handling in redirectWithError method. The changes are properly synchronized between tests and implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error response field changes in controller implementation
# Search for old and new error field names in the controller file
echo "Searching for old error field names..."
rg -A 2 "bff_error_code|bff_error_description" "apps/services/bff/src/app/modules/auth/auth.controller.ts"
echo "Searching for new error field names..."
rg -A 2 "bff_error_status_code|bff_error_message" "apps/services/bff/src/app/modules/auth/auth.controller.ts"
Length of output: 390
Script:
#!/bin/bash
# Let's try a broader search to understand the error response structure
# First, let's check if the controller file exists
echo "Checking controller file existence..."
fd "auth.controller.ts" apps/services/bff/src/app/modules/auth/
echo "Searching for error response patterns in controller..."
rg -A 5 "throw new|return.*error" apps/services/bff/src/app/modules/auth/auth.controller.ts
# Let's also check for any error response interfaces or types
echo "Searching for error response types..."
rg -A 5 "interface.*Error|type.*Error" apps/services/bff/src/app/modules/auth/
Length of output: 585
Script:
#!/bin/bash
# Let's try to find any error-related code using ast-grep
echo "Searching for error response construction patterns..."
ast-grep --pattern 'return {
$$$
bff_error_$_: $_,
$$$
}'
# Let's also check the content of the controller file
echo "Checking controller file content..."
cat apps/services/bff/src/app/modules/auth/auth.controller.ts
Length of output: 1857
Script:
#!/bin/bash
# Let's check the auth service since the controller delegates to it
echo "Checking auth service content..."
cat apps/services/bff/src/app/modules/auth/auth.service.ts
# Also check for any error-related types or interfaces
echo "Checking for error types in the module..."
fd -e ts . apps/services/bff/src/app/modules/auth/ --exec rg -l "interface|type.*Error"
Length of output: 18458
Script:
#!/bin/bash
# Let's check the error query string utility that's being used
echo "Checking create-error-query-str utility..."
cat apps/services/bff/src/app/utils/create-error-query-str.ts
# Also check for any error-related constants
echo "Checking for error-related constants..."
rg "error|Error" apps/services/bff/src/app/constants/
Length of output: 790
Feature Bff double session
What
This PR implements a modal that handles cases where a user inadvertently creates two active sessions. This scenario occurs when:
The modal provides users with two options:
On the api side
To detect double sessions, we store both the current and attempt session data in the cache. During normal authentication flow, the attempt session is deleted upon successful login. However, if a user presses the back button and logs in again, we'll have both an active current session and a new attempt session simultaneously. By combining both session data in our response, we can detect this scenario and notify the client application that a double session exists, allowing it to display the appropriate warning modal to the user.
Mobile
**
**
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style
Chores