fix(auth-api): Add audit logs to index with alsoLog set to true for server logs #16203
Conversation
…es to users delegations records in the index.
WalkthroughThe changes in this pull request involve updates to several classes and methods across different files, primarily focusing on the inclusion of additional parameters, specifically Changes
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (12)
apps/services/auth/delegation-api/src/app/delegations/delegation-index.controller.ts (2)
96-103: LGTM! Consider destructuring for improved readability.The addition of the
authparameter to thecreateOrUpdateDelegationRecordmethod call is appropriate and aligns with the PR objectives to enhance auditing capabilities. This change maintains consistency with the updated method signature and adheres to NestJS best practices.For improved readability, consider destructuring the object passed to
createOrUpdateDelegationRecord. Here's a suggested refactor:this.delegationIndexService.createOrUpdateDelegationRecord( - { - ...parsedDelegationInfo, - provider: auth.delegationProvider, - validTo: body.validTo, - }, - auth, + { + ...parsedDelegationInfo, + provider: auth.delegationProvider, + validTo: body.validTo, + auth, + }, ),This change combines the two arguments into a single object, which can make the code more maintainable if additional parameters are added in the future.
132-138: LGTM! Consider destructuring for improved readability.The addition of the
authparameter to theremoveDelegationRecordmethod call is appropriate and aligns with the PR objectives to enhance auditing capabilities. This change maintains consistency with the updated method signature and adheres to NestJS best practices.For improved readability and consistency with the previous suggestion, consider destructuring the object passed to
removeDelegationRecord. Here's a suggested refactor:this.delegationIndexService.removeDelegationRecord( - { - ...parsedDelegationInfo, - provider: auth.delegationProvider, - }, - auth, + { + ...parsedDelegationInfo, + provider: auth.delegationProvider, + auth, + }, ),This change combines the two arguments into a single object, which can make the code more maintainable if additional parameters are added in the future and maintains consistency with the
createOrUpdateDelegationRecordmethod.apps/services/auth/personal-representative/src/app/modules/personalRepresentatives/personalRepresentatives.controller.ts (2)
139-139: LGTM! Consider adding a comment for clarity.The addition of the
userparameter toindexRepresentativeDelegationsaligns with the PR objectives and enhances the auditing capabilities. This change provides more context for indexing operations, which is a good practice.Consider adding a brief comment explaining the purpose of passing the
userparameter, for consistency with other parts of the codebase and to improve maintainability. For example:// Pass user context for audit logging during indexing void this.delegationIndexService.indexRepresentativeDelegations( deletedPersonalRepresentative.nationalIdPersonalRepresentative, user, )
208-208: LGTM! Consider adding a comment for clarity.The addition of the
userparameter toindexRepresentativeDelegationsin thecreatemethod is consistent with the change in thedeletemethod. This maintains consistency across the controller and enhances the auditing capabilities as per the PR objectives.For consistency and improved maintainability, consider adding a brief comment explaining the purpose of passing the
userparameter, similar to the suggestion for thedeletemethod. For example:// Pass user context for audit logging during indexing void this.delegationIndexService.indexRepresentativeDelegations( createdPersonalRepresentative.nationalIdPersonalRepresentative, user, )libs/auth-api-lib/src/lib/delegations/delegations-incoming.service.ts (1)
305-313: LGTM! Consider adding a comment for clarity.The addition of the
userparameter to theremoveDelegationRecordmethod call is a good improvement. It aligns with the PR objectives of enhancing auditing capabilities by providing more context when removing delegation records.Consider adding a brief comment explaining why the
userparameter is now included, to improve code readability and maintain consistency with the rest of the codebase. For example:// Include user information for audit logging purposes void this.delegationsIndexService.removeDelegationRecord( { fromNationalId, toNationalId: user.nationalId, type: AuthDelegationType.LegalRepresentative, provider: AuthDelegationProvider.DistrictCommissionersRegistry, }, user, )libs/auth-api-lib/src/lib/delegations/delegations-outgoing.service.ts (5)
276-276: LGTM! Consider adding a comment for consistency.The addition of the
userparameter toindexCustomDelegationsis appropriate and aligns with the PR objectives. It enhances the auditing capabilities by providing user context during indexing.For consistency with other asynchronous operations in this file, consider adding a comment explaining why we're using
voidhere:// Fire and forget: Index custom delegations for the toNationalId void this.delegationIndexService.indexCustomDelegations( createDelegation.toNationalId, user, )
422-422: LGTM! Consider adding a comment for consistency.The addition of the
userparameter toindexCustomDelegationsin thepatchmethod is appropriate and consistent with the previous change in thecreatemethod. It further enhances the auditing capabilities by providing user context during indexing for patch operations.For consistency, consider adding a comment explaining the use of
voidhere as well:// Fire and forget: Index custom delegations for the toNationalId void this.delegationIndexService.indexCustomDelegations( delegation.toNationalId, user, )
427-428: LGTM! Consider adding a comment for clarity.The addition of
voidbeforethis.notifyDelegationUpdateis consistent with the treatment of other asynchronous operations in this file. The blank line improves readability by separating this operation from the previous code block.For clarity and consistency with other asynchronous operations, consider adding a comment explaining the use of
void:// Fire and forget: Notify delegation update void this.notifyDelegationUpdate(user, delegation, hasExistingScopes)
463-463: LGTM! Consider adding a comment for consistency.The addition of the
userparameter toindexCustomDelegationsin thedeletemethod is appropriate and consistent with the previous changes in thecreateandpatchmethods. It further enhances the auditing capabilities by providing user context during indexing for delete operations.For consistency, consider adding a comment explaining the use of
voidhere as well:// Fire and forget: Index custom delegations for the toNationalId void this.delegationIndexService.indexCustomDelegations( delegation.toNationalId, user, )
Line range hint
1-515: Overall, the changes enhance auditing capabilities and maintain consistency.The modifications to the
DelegationsOutgoingServiceclass successfully implement the PR objectives by adding user context to theindexCustomDelegationsmethod calls in thecreate,patch, anddeletemethods. This enhancement improves the auditing capabilities of the system during delegation operations.The changes adhere to the coding guidelines for files in the
libsdirectory, maintaining reusability and consistency across different NextJS apps. The use of TypeScript for defining props and exporting types is preserved.To further improve the code:
- Consider extracting the
indexCustomDelegationscalls into a separate private method within the service to reduce code duplication and improve maintainability.- If the
userparameter is consistently required for auditing purposes, consider updating theindexCustomDelegationsmethod signature in theDelegationIndexServiceto include it as a mandatory parameter.libs/auth-api-lib/src/lib/delegations/delegation-scope.service.ts (2)
325-333: Include error details in logs for improved debuggingIn the
catchblock, the logged error message does not include the actual error details. Including the error object can provide more context and aid in debugging issues.Apply this diff to include the error in the log:
} catch (error) { - logger.error( - `Failed checking if delegation exists at provider '${AuthDelegationProvider.DistrictCommissionersRegistry}'`, - ) + logger.error( + `Failed checking if delegation exists at provider '${AuthDelegationProvider.DistrictCommissionersRegistry}': ${error.message}`, + { error }, + ) return [] }
313-333: Update function documentation to reflect new parameterThe method
findDistrictCommissionersRegistryScopesTonow accepts aUserobject instead of atoNationalIdstring. Ensure that any accompanying documentation or comments are updated to reflect this change, enhancing code readability and maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- apps/services/auth/delegation-api/src/app/delegations/delegation-index.controller.ts (2 hunks)
- apps/services/auth/delegation-api/src/app/delegations/test/delegation-index/delegation-index.service.spec.ts (3 hunks)
- apps/services/auth/personal-representative/src/app/modules/personalRepresentatives/personalRepresentatives.controller.ts (3 hunks)
- libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (4 hunks)
- libs/auth-api-lib/src/lib/delegations/delegation-scope.service.ts (3 hunks)
- libs/auth-api-lib/src/lib/delegations/delegations-incoming.service.ts (1 hunks)
- libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts (7 hunks)
- libs/auth-api-lib/src/lib/delegations/delegations-outgoing.service.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
apps/services/auth/delegation-api/src/app/delegations/delegation-index.controller.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/auth/delegation-api/src/app/delegations/test/delegation-index/delegation-index.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/auth/personal-representative/src/app/modules/personalRepresentatives/personalRepresentatives.controller.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/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.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-api-lib/src/lib/delegations/delegation-scope.service.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-api-lib/src/lib/delegations/delegations-incoming.service.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-api-lib/src/lib/delegations/delegations-index.service.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-api-lib/src/lib/delegations/delegations-outgoing.service.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 (11)
apps/services/auth/delegation-api/src/app/delegations/delegation-index.controller.ts (1)
Line range hint
1-142: Overall assessment: Changes are well-implemented and align with project goals.The modifications to the
DelegationIndexControllerclass successfully enhance the auditing capabilities of the system by including theauthparameter in relevant method calls. The changes are consistent with the PR objectives and adhere to NestJS architecture and best practices.Key points:
- Both
createOrUpdateDelegationRecordandremoveDelegationRecordmethods have been updated to include theauthparameter.- The changes maintain the existing architecture and dependency injection patterns.
- Minor suggestions for code readability have been provided to further improve the implementation.
The implementation effectively addresses the goal of adding audit logs during specific operations related to indexing, improving the overall transparency and accountability of the system's operations.
apps/services/auth/personal-representative/src/app/modules/personalRepresentatives/personalRepresentatives.controller.ts (1)
Line range hint
1-214: Overall, the changes look good. Verify the impact on related components.The modifications to both the
deleteandcreatemethods enhance the auditing capabilities by passing theusercontext to theindexRepresentativeDelegationsmethod. These changes align well with the PR objectives and follow NestJS best practices.To ensure the changes don't have unintended consequences, please run the following verification script:
This script will help ensure that the
DelegationsIndexServicehas been updated to handle the newuserparameter and that all calls toindexRepresentativeDelegationsacross the codebase include theuserparameter.apps/services/auth/delegation-api/src/app/delegations/test/delegation-index/delegation-index.service.spec.ts (4)
454-454: LGTM: User parameter added to indexCustomDelegations callThe addition of the
userparameter to theindexCustomDelegationsmethod call is consistent with the broader update to include user context in delegation operations. This change enhances the method's capability to handle user-specific information during custom delegation indexing.
483-486: LGTM: User parameter added and improved readabilityThe addition of the
userparameter to theindexRepresentativeDelegationsmethod call is consistent with the previous changes. The multi-line format improves readability and makes it easier to add more parameters in the future if needed. This change enhances the method's capability to handle user-specific information during representative delegation indexing.
631-631: LGTM: Consistent application of user parameterThe addition of the
userparameter to theindexCustomDelegationsmethod call in this test case is consistent with the previous changes. This demonstrates a thorough update across different test scenarios, ensuring that the new user context is properly tested in various situations.
Line range hint
1-631: Consider adding specific tests for the new user parameter functionalityThe changes to include the
userparameter in various method calls have been consistently applied throughout the test file. This is a good practice and aligns with the PR objectives. However, to ensure comprehensive test coverage, consider adding specific test cases that verify the behavior of these methods with different user contexts.To help identify areas where additional tests might be beneficial, you can run the following script:
This script will help you identify methods that now accept a user parameter and compare them with existing test cases, potentially revealing areas where additional tests could be beneficial.
✅ Verification successful
To proceed with verifying the need for additional tests, please run the following script:
User Parameter Functionality is Adequately Tested
The methods that accept the
userparameter are appropriately covered by existing test cases. No additional tests are necessary at this time.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Identify methods that now accept a user parameter but may lack specific tests for this new functionality. # Search for method definitions that include a user parameter echo "Methods that now accept a user parameter:" rg --type typescript "async.*\(.*user.*\)" apps/services/auth/delegation-api/src/app/delegations # Search for existing test cases that use these methods with the user parameter echo "\nExisting test cases using methods with user parameter:" rg --type typescript "await.*\(.*user.*\)" apps/services/auth/delegation-api/src/app/delegations/test # Compare the results to identify methods that might need additional test coverageLength of output: 488
Script:
#!/bin/bash # Description: Identify methods that now accept a user parameter but may lack specific tests for this new functionality. # Search for method definitions that include a user parameter echo "Methods that now accept a user parameter:" rg --type ts "async.*\(.*user.*\)" apps/services/auth/delegation-api/src/app/delegations # Search for existing test cases that use these methods with the user parameter echo "\nExisting test cases using methods with user parameter:" rg --type ts "await.*\(.*user.*\)" apps/services/auth/delegation-api/src/app/delegations/testLength of output: 3921
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (2)
8-8: Importing 'Auth' and 'User' typesThe addition of
AuthandUserimports is appropriate, as these types are utilized in the updated method signatures.
270-275: Ensure correct usage of 'auth' parameter in 'indexDelegations' methodThe
indexDelegationsmethod now acceptsauth: Authand passes it to the delegation indexing services. Confirm that whenindexDelegationsis called elsewhere, the providedauthparameter contains the necessary authentication information required bydelegationIndexService.libs/auth-api-lib/src/lib/delegations/delegation-scope.service.ts (3)
31-31: Import 'User' type for updated method signaturesThe import statement correctly includes the
Usertype from'@island.is/auth-nest-tools', which is necessary for the updated method signatures that now accept aUserobject.
313-333: Confirm TypeScript types for function parametersUsing TypeScript for defining function parameters is a good practice. Confirm that the
Usertype correctly represents the expected structure and that all properties used (e.g.,user.nationalId) are properly typed.
436-436: Verify all calls are updated to pass 'user' instead of 'toNationalId'Ensure that all calls to
findDistrictCommissionersRegistryScopesTohave been updated to pass theuserobject instead oftoNationalIdas the first parameter. This is crucial to maintain consistency after changing the method signature.Run the following script to check for any outdated calls:
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts
Show resolved
Hide resolved
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16203 +/- ##
=======================================
Coverage 36.70% 36.71%
=======================================
Files 6778 6776 -2
Lines 139601 139599 -2
Branches 39687 39686 -1
=======================================
+ Hits 51243 51248 +5
+ Misses 88358 88351 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 13 Total Test Services: 0 Failed, 13 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (1)
|
…erver logs (#16203) * Adding audit log to delegationIndex saveToIndex method to track changes to users delegations records in the index. * Add audit to create and remove in index service. --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
Adding audit logs when
Why
To be able to track changes made to the index during multiple scenarios of updates.
Screenshots / Gifs
N/A
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
These changes improve the overall functionality and user experience when managing delegation records.