fix(download-service): Update usage of accesstoken in jwt strategy and download service controllers #16216
Conversation
…avaliable in body. Also updated all controllers in download service to get the access token from the user context instead
WalkthroughThe changes involve the removal of multiple data transfer objects (DTOs) that contained an Changes
Possibly related PRs
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
apps/download-service/src/app/modules/finance-documents/document.controller.ts (1)
69-71: LGTM with a minor suggestion: Response header updatesThe changes to the response headers maintain the same functional intent while using a more modern and consistent formatting. This is a good improvement.
Consider combining the Cache-Control headers for even cleaner code:
- res.header('Cache-Control', 'no-cache') - res.header('Cache-Control', 'nmax-age=0') + res.header('Cache-Control', 'no-cache, max-age=0')apps/download-service/src/app/modules/health/payment-overview-documents.controller.ts (1)
38-38: Response handling improvements with a minor suggestionThe changes in response handling are good:
- The updated
AuthMiddlewareusage aligns with the new authorization approach.- The addition of 'Content-Type', 'Pragma', and 'Cache-Control' headers improves the response handling.
However, there's a minor redundancy in the 'Cache-Control' header setting.
Consider combining the 'Cache-Control' headers into a single line for clarity:
- res.header('Cache-Control', 'no-cache') - res.header('Cache-Control', 'nmax-age=0') + res.header('Cache-Control', 'no-cache, max-age=0')Also applies to: 65-68
libs/auth-nest-tools/src/lib/jwt.strategy.ts (1)
49-54: Approve changes with a minor suggestion for clarityThe changes effectively implement the new token handling mechanism, aligning with the PR objectives and preparing for the BFF architecture. The prioritization of the token from headers enhances security.
Consider adding a brief comment explaining the prioritization logic for better clarity:
// Prioritize token from headers, fallback to body for backward compatibility const authorization = request.headers.authorization || (bodyAuthorization && `Bearer ${bodyAuthorization}`) || ''apps/download-service/src/app/modules/education-documents/education-document.controller.ts (1)
75-78: LGTM: Response headers updated for consistencyThe changes to the response headers improve consistency and readability. The
Content-Typeheader is now set using the same method as other headers, which is good.Consider combining the two
Cache-Controlheaders for efficiency:- res.header('Cache-Control', 'no-cache') - res.header('Cache-Control', 'nmax-age=0') + res.header('Cache-Control', 'no-cache, max-age=0')This change would reduce the number of header operations while maintaining the same caching behavior.
apps/download-service/src/app/modules/regulation-documents/regulation-documents.controller.ts (1)
81-81: Cache-Control header syntax correctedThe update to the 'Cache-Control' header syntax is correct and important for ensuring proper caching behavior.
For improved clarity and to future-proof the header, consider using a more specific directive:
- res.header('Cache-Control', 'no-cache') + res.header('Cache-Control', 'no-store, must-revalidate')This ensures that the response is not stored in any cache and must be revalidated with the server on each request.
apps/download-service/src/app/modules/vehicles-documents/vehicle-document.controller.ts (2)
Line range hint
38-55: LGTM: Authentication flow updated and response headers improvedThe changes in the
getVehicleHistoryPdfmethod successfully implement the new authentication flow, aligning with the PR objectives. The removal of theresourceparameter and direct use of theuserobject withAuthMiddlewareenhance security by not exposing tokens to the client side.The response header updates improve consistency and security. However, there's a minor formatting issue in one header.
Consider updating the
Content-Typeheader to use the same format as other headers:- res.header('Content-Type: application/pdf') + res.header('Content-Type', 'application/pdf')
Line range hint
1-102: Excellent implementation of new authentication flow and security improvementsThe changes in this file successfully implement the new authentication flow, aligning perfectly with the PR objectives. Key improvements include:
- Removal of the
resourceparameter, enhancing security by not exposing tokens to the client side.- Direct use of the
userobject withAuthMiddleware, simplifying the authentication process.- Consistent formatting of response headers across both methods.
- Effective use of TypeScript for improved type safety.
The overall structure adheres to NextJS best practices for API routes, and the code remains well-organized and maintainable. These changes contribute significantly to preparing for the Backend for Frontend (BFF) architecture and enhancing overall security.
Consider documenting these changes in the project's architecture documentation to ensure that other developers understand the new authentication flow and its implications for the BFF architecture.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
- apps/download-service/src/app/modules/education-documents/dto/getEducationGraduationDocument.ts (0 hunks)
- apps/download-service/src/app/modules/education-documents/education-document.controller.ts (2 hunks)
- apps/download-service/src/app/modules/finance-documents/document.controller.ts (3 hunks)
- apps/download-service/src/app/modules/finance-documents/dto/getFinanceDocument.dto.ts (1 hunks)
- apps/download-service/src/app/modules/health/dto/getHealthPaymentDocument.dto.ts (0 hunks)
- apps/download-service/src/app/modules/health/payment-overview-documents.controller.ts (3 hunks)
- apps/download-service/src/app/modules/regulation-documents/dto/getRegulationDraftDocument.dto.ts (0 hunks)
- apps/download-service/src/app/modules/regulation-documents/regulation-documents.controller.ts (3 hunks)
- apps/download-service/src/app/modules/vehicles-documents/dto/getVehicleHistoryDocument.dto.ts (0 hunks)
- apps/download-service/src/app/modules/vehicles-documents/vehicle-document.controller.ts (4 hunks)
- apps/download-service/src/app/modules/work-machines-documents/dto/getWorkMachineCollectionDocument.dto.ts (0 hunks)
- apps/download-service/src/app/modules/work-machines-documents/work-machines-documents.controller.ts (3 hunks)
- libs/auth-nest-tools/src/lib/jwt.strategy.ts (1 hunks)
💤 Files with no reviewable changes (5)
- apps/download-service/src/app/modules/education-documents/dto/getEducationGraduationDocument.ts
- apps/download-service/src/app/modules/health/dto/getHealthPaymentDocument.dto.ts
- apps/download-service/src/app/modules/regulation-documents/dto/getRegulationDraftDocument.dto.ts
- apps/download-service/src/app/modules/vehicles-documents/dto/getVehicleHistoryDocument.dto.ts
- apps/download-service/src/app/modules/work-machines-documents/dto/getWorkMachineCollectionDocument.dto.ts
🧰 Additional context used
📓 Path-based instructions (8)
apps/download-service/src/app/modules/education-documents/education-document.controller.ts (1)
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/download-service/src/app/modules/finance-documents/document.controller.ts (1)
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/download-service/src/app/modules/finance-documents/dto/getFinanceDocument.dto.ts (1)
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/download-service/src/app/modules/health/payment-overview-documents.controller.ts (1)
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/download-service/src/app/modules/regulation-documents/regulation-documents.controller.ts (1)
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/download-service/src/app/modules/vehicles-documents/vehicle-document.controller.ts (1)
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/download-service/src/app/modules/work-machines-documents/work-machines-documents.controller.ts (1)
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/auth-nest-tools/src/lib/jwt.strategy.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 (22)
apps/download-service/src/app/modules/finance-documents/dto/getFinanceDocument.dto.ts (2)
1-2: LGTM: Import statements are correct and concise.The import statements are appropriate for the decorators used in the class. They follow a good practice of importing only what's necessary.
Line range hint
4-9: LGTM: DTO simplification aligns with PR objectives.The removal of the
__accessTokenproperty fromGetFinanceDocumentDtoaligns well with the PR objectives to prepare for the BFF architecture and enhance security. The remainingannualDocproperty is correctly decorated and typed.To ensure consistency across the codebase, please verify that all controllers using this DTO have been updated accordingly. Run the following script to check for any remaining references to
__accessToken:apps/download-service/src/app/modules/work-machines-documents/work-machines-documents.controller.ts (4)
1-1: LGTM: Improved type import for UserThe change to import
Useras a type is a good TypeScript practice. It ensures that the import is used only for type checking and not at runtime, which can lead to better performance and clearer code intentions.
31-35: LGTM: Improved method signature and securityThe removal of the
resourceparameter from the method signature is a positive change. It aligns with the PR objectives of not exposing tokens in the request body and simplifies the method interface. The direct use of theuserparameter for authentication enhances security by relying on the authenticated user information provided by the@CurrentUser()decorator.
58-60: Minor improvement: Response header formattingThe changes to the response headers improve the formatting consistency. The removal of colons in the header names is correct, as the
headermethod expects separate name and value parameters.
Line range hint
1-64: LGTM: Good TypeScript usage and compatibility with NextJSThe code demonstrates good use of TypeScript, with proper type annotations and imports. While this is a NestJS controller, its structure and practices are compatible with NextJS API routes. The use of decorators for route and parameter handling, along with proper typing of request and response objects, aligns well with both NestJS and NextJS best practices for API development.
apps/download-service/src/app/modules/finance-documents/document.controller.ts (4)
1-9: LGTM: Enhanced type safety and securityThe changes to the import statements and the addition of the
IdsUserGuardandScopesGuardalign well with the PR objectives. The specific API scopes in the@Scopesdecorator improve the security model by clearly defining the required permissions for accessing finance-related endpoints.Also applies to: 25-26
48-55: LGTM: Simplified authorization logicThe changes in the financeService method calls align perfectly with the PR objectives. By directly passing the
userobject instead of creating a separateauthUserobject, the code becomes cleaner and more straightforward. This change effectively updates the usage of access tokens as intended.
Line range hint
1-77: Overall assessment: Well-implemented changesThe modifications in this file successfully address the PR objectives by updating the usage of access tokens and preparing for the Backend for Frontend (BFF) architecture. The changes enhance security, improve code clarity, and maintain backwards compatibility. The use of TypeScript for type safety and adherence to NextJS best practices is commendable.
A few minor suggestions have been made, but overall, the implementation is solid and ready for the next steps in the development process.
Line range hint
41-47: Verify internal logic changesWhile the method signature remains unchanged, which is good for maintaining backwards compatibility, the AI summary indicates that there have been internal logic changes. Let's verify these changes to ensure they align with the PR objectives.
apps/download-service/src/app/modules/health/payment-overview-documents.controller.ts (3)
9-14: Import changes look goodThe changes in imports align well with the PR objectives. The addition of
ApiScopeandAuditServicesupports enhanced security and auditing. The removal ofBodyimport is consistent with the updated method signature.
33-37: Method signature update aligns with PR objectivesThe removal of the
@Body() resource: GetGetHealthPaymentDocumentDtoparameter from thegetHealthPaymentOverviewPdfmethod signature is a positive change. It aligns with the PR objectives of extracting the access token using JWT strategy instead of including it in the request body, enhancing security by not exposing tokens to the client side.
Line range hint
1-72: Good TypeScript usage, but clarification on framework contextThe use of TypeScript for type safety throughout the file is commendable and aligns with the coding guidelines. However, it's important to note that this file is part of a NestJS backend application, not a NextJS frontend application. The NextJS-specific best practices mentioned in the coding guidelines don't apply in this context.
libs/auth-nest-tools/src/lib/jwt.strategy.ts (2)
61-61: Approve the update to the Auth objectThe change correctly uses the newly constructed
authorizationvariable, ensuring consistency with the earlier modifications and proper token handling.
Line range hint
1-76: Overall approval: Changes align with PR objectives and enhance securityThe modifications to the
JwtStrategyclass effectively implement the new token handling mechanism, aligning perfectly with the PR objectives. Key points:
- The changes prepare the system for the Backend for Frontend (BFF) architecture.
- Security is enhanced by prioritizing tokens from request headers over those in the body.
- Backward compatibility is maintained through the fallback mechanism.
- The code adheres to TypeScript best practices and maintains reusability, as required for files in the
libsdirectory.These changes successfully address the goal of updating how the access token is handled within the download service controllers, moving away from including tokens in POST request bodies.
apps/download-service/src/app/modules/education-documents/education-document.controller.ts (2)
Line range hint
46-53: LGTM: Method signature updated correctlyThe removal of the
resourceparameter and its@Body()decorator aligns with the PR objectives. The method now relies on the@CurrentUser()decorator to obtain the user information, which is the correct approach for handling authorization in the new implementation.
54-54: LGTM: User object correctly utilized for authorizationThe direct use of the
userobject in thegetStudentTrackPdf()method call is consistent with the PR objectives. This change effectively utilizes the JWT strategy to handle authorization, eliminating the need for a separate access token in the request body.apps/download-service/src/app/modules/regulation-documents/regulation-documents.controller.ts (3)
8-10: Import changes look goodThe updated imports align well with the PR objectives. The addition of
AdminPortalScope,RegulationsService, andRegulationsAdminClientServicesupports the new implementation. The removal ofBodyfrom NestJS imports and the inferred removal ofGetRegulationDraftDocumentDtoare consistent with the changes in the method signature.Also applies to: 15-17
47-48: Method signature update improves securityThe removal of the
resourceparameter and direct use of theuserobject for authorization aligns perfectly with the PR objectives. This change enhances security by eliminating the need to pass the access token in the request body, which is a step towards the Backend for Frontend (BFF) architecture mentioned in the PR summary.
Line range hint
1-89: Overall changes align with NestJS best practicesWhile the coding guidelines mention NextJS best practices, it's important to note that this file is a NestJS controller, not a NextJS component. The changes made are appropriate for a NestJS application and follow NestJS best practices, including:
- Proper use of decorators for route handling and parameter extraction.
- Correct implementation of dependency injection in the constructor.
- Appropriate error handling and response management.
The updates to the controller improve its security and align well with the PR objectives.
apps/download-service/src/app/modules/vehicles-documents/vehicle-document.controller.ts (2)
1-3: LGTM: Imports updated to support new authentication flowThe new imports for
User,AuthMiddleware, andApiScopeare appropriate for the updated authentication mechanism. This change aligns well with the PR objectives and maintains good TypeScript practices.
Line range hint
77-98: LGTM: Consistent implementation of new authentication flowThe changes in the
getVehicleOwnershipmethod mirror those ingetVehicleHistoryPdf, maintaining consistency across the controller. The updated authentication flow and response header formatting are well-implemented and align with the PR objectives.The removal of the
resourceparameter and the use ofAuthMiddlewarewith theuserobject enhance security by not exposing tokens to the client side. The response headers are now consistently formatted, improving code quality.
Datadog ReportAll test runs ✅ 74 Total Test Services: 0 Failed, 72 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (24)
|
Yes I have done the test now and this should be solid :) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/download-service/src/app/modules/occupational-licenses/occupational-license.controller.ts (1)
56-59: LGTM: Improved HTTP header settingsThe changes to the HTTP header settings are correct and improve the code quality:
- The 'Content-Type' and 'Pragma' headers are now set correctly without the erroneous colon.
- The 'Cache-Control' headers have been reformatted for consistency.
These changes align with best practices for setting HTTP headers.
Consider combining the two 'Cache-Control' headers into a single line for better readability:
res.header('Cache-Control', 'no-cache, max-age=0')This change would maintain the same behavior while slightly reducing code duplication.
libs/auth-nest-tools/src/lib/jwt.strategy.spec.ts (1)
117-139: LGTM! Consider adding a negative test case.The new test case effectively verifies the handling of the
__accessTokenfield in the request body. It follows the established Arrange-Act-Assert pattern and adheres to the coding guidelines for TypeScript usage.To further improve test coverage, consider adding a negative test case where the
__accessTokenis not present in the request body. This would ensure that theauthorizationproperty is not set when the token is missing.Here's an example of how you could add a negative test case:
it('does not set authorization when __accessToken is missing', async () => { // Arrange const payload: JwtPayload = { nationalId: '1234567890', scope: ['test-scope-1'], client_id: 'test-client', } const request = { headers: { 'user-agent': 'test user agent', 'x-forwarded-for': '2.2.2.2, 3.3.3.3', }, body: {}, } as unknown as Request // Act const user = await jwtStrategy.validate(request, payload) // Assert expect(user.authorization).toBeUndefined() })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
- apps/download-service/src/app/modules/documents/document.controller.ts (0 hunks)
- apps/download-service/src/app/modules/documents/dto/getDocument.dto.ts (0 hunks)
- apps/download-service/src/app/modules/education-documents/education-document.controller.ts (3 hunks)
- apps/download-service/src/app/modules/finance-documents/document.controller.ts (3 hunks)
- apps/download-service/src/app/modules/finance-documents/dto/getFinanceDocument.dto.ts (0 hunks)
- apps/download-service/src/app/modules/occupational-licenses/occupational-license.controller.ts (1 hunks)
- libs/api/domains/finance/src/lib/api-domains-finance.resolver.ts (1 hunks)
- libs/auth-nest-tools/src/lib/jwt.strategy.spec.ts (1 hunks)
- libs/auth-nest-tools/src/lib/jwt.strategy.ts (1 hunks)
💤 Files with no reviewable changes (3)
- apps/download-service/src/app/modules/documents/document.controller.ts
- apps/download-service/src/app/modules/documents/dto/getDocument.dto.ts
- apps/download-service/src/app/modules/finance-documents/dto/getFinanceDocument.dto.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/download-service/src/app/modules/education-documents/education-document.controller.ts
- apps/download-service/src/app/modules/finance-documents/document.controller.ts
- libs/auth-nest-tools/src/lib/jwt.strategy.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/download-service/src/app/modules/occupational-licenses/occupational-license.controller.ts (1)
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/api/domains/finance/src/lib/api-domains-finance.resolver.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/auth-nest-tools/src/lib/jwt.strategy.spec.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 (2)
apps/download-service/src/app/modules/occupational-licenses/occupational-license.controller.ts (1)
Line range hint
23-26: Verify the removal of DistrictCommissionersLicensesServiceThe
DistrictCommissionersLicensesServicehas been removed from the constructor. This aligns with the PR objectives to update the usage of access tokens. However, please ensure that:
- All functionalities previously using this service have been properly migrated or are no longer needed.
- There are no remaining references to this service in the class methods.
To confirm there are no lingering references, run the following script:
If the script returns any results, please review and remove any remaining references.
✅ Verification successful
DistrictCommissionersLicensesService Removal Verified
All references to
DistrictCommissionersLicensesServicehave been successfully removed fromoccupational-license.controller.ts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to DistrictCommissionersLicensesService in the file # Test: Search for any usage of DistrictCommissionersLicensesService rg --type typescript "DistrictCommissionersLicensesService" apps/download-service/src/app/modules/occupational-licenses/occupational-license.controller.tsLength of output: 189
Script:
#!/bin/bash # Description: Check for any remaining references to DistrictCommissionersLicensesService in the file # Test: Search for any usage of DistrictCommissionersLicensesService with correct file type rg --type ts "DistrictCommissionersLicensesService" apps/download-service/src/app/modules/occupational-licenses/occupational-license.controller.tsLength of output: 146
Script:
#!/bin/bash # Description: Check for any remaining references to DistrictCommissionersLicensesService in the file without specifying file type # Test: Search for any usage of DistrictCommissionersLicensesService in the specified file rg "DistrictCommissionersLicensesService" apps/download-service/src/app/modules/occupational-licenses/occupational-license.controller.tsLength of output: 136
libs/api/domains/finance/src/lib/api-domains-finance.resolver.ts (1)
67-70: LGTM: Addition ofdownloadServiceURLto the response ingetFinanceStatus.The inclusion of the
downloadServiceURLproperty enhances the response by providing a direct link for finance-related downloads. The construction of the URL usingthis.downloadServiceConfig.baseUrlappears correct.
snaerseljan
added
the
deprecated:automerge
snaerseljan
force-pushed
the
fix/download-service-token-usage
branch
from
9aec04e to
c984ae6
Compare
…d download service controllers (#16216) * Update jsw strategy to return access token in authorisation field if avaliable in body. Also updated all controllers in download service to get the access token from the user context instead * fix optional * Update test * Remove unused import * Update test because of unwanted new line * Remove/update body from finance controller and update headers * Remove unused import --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Fix download service token usage
What
Instead of including the accessToken in the POST request body for the download-service controllers, we now extract it via the JWT strategy. The strategy pulls the token from the request headers and provides it through the authorization field in the user context.
Why
Preparation for BFF since no tokens will be exposed in the client.
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Chores
These changes enhance user experience by simplifying access to documents while maintaining functionality.