feat(auth-admin): Delegation-admin webhook auditlog #16335
Conversation
WalkthroughThe changes in this pull request enhance the delegation management system by introducing an audit decorator in the 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: 0
🧹 Outside diff range and nitpick comments (1)
apps/services/auth/admin-api/src/app/v2/delegations/delegation-admin.controller.ts (1)
Line range hint
1-145: Consider adding audit decorators to other methods for consistencyThe addition of the
@Auditdecorator to thecreateByZendeskIdmethod is a good improvement. For consistency and comprehensive auditing, consider adding similar@Auditdecorators to other methods in this controller that perform create, update, or delete operations, if they're not already covered by the class-level@Audit({ namespace })decorator.For example, the
createanddeletemethods might benefit from method-level audit decorators to capture more specific details about these operations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- apps/services/auth/admin-api/src/app/v2/delegations/delegation-admin.controller.ts (1 hunks)
- apps/services/auth/delegation-api/src/app/delegations/delegation-index.controller.ts (2 hunks)
- libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/services/auth/admin-api/src/app/v2/delegations/delegation-admin.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/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."
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."
🔇 Additional comments (7)
apps/services/auth/admin-api/src/app/v2/delegations/delegation-admin.controller.ts (1)
102-102: LGTM: Audit decorator added correctlyThe addition of the
@Auditdecorator to thecreateByZendeskIdmethod aligns well with the PR objective of implementing audit logs for the delegation-admin Zendesk route. This change enhances the auditing capabilities of the controller.A few observations:
- The decorator is correctly placed above the method definition.
- It uses a generic type
<{ id: string }>which matches the expected input.- The
resourcesfunction extracts theidfrom the body, which is appropriate for tracking the affected resource.This implementation adheres to the NestJS architecture and follows good practices for adding cross-cutting concerns like auditing.
apps/services/auth/delegation-api/src/app/delegations/delegation-index.controller.ts (3)
91-91: Improved audit logging with namespaceThe addition of the
namespaceparameter to theauditService.auditPromisecall enhances the audit logging by providing more specific context. This change will make it easier to filter and analyze logs related to delegation index operations.
127-127: Consistent audit logging improvementThe addition of the
namespaceparameter to theauditService.auditPromisecall in theremoveDelegationRecordmethod is consistent with the previous change. This ensures uniform audit logging across different operations in the controller.
Line range hint
1-143: Overall analysis: Adherence to NestJS best practices and improved audit loggingThe changes in this file are well-implemented and consistent with NestJS best practices:
- The controller follows NestJS architecture, including proper use of decorators, guards, and dependency injection.
- Authentication and authorization are handled correctly using
@UseGuardsand@Scopesdecorators.- The file includes appropriate documentation using Swagger decorators.
- The audit logging improvements are consistent across methods and enhance the overall functionality.
The changes adhere to the coding guidelines for NestJS applications and improve the audit logging capabilities without introducing any new issues or inconsistencies.
libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts (3)
313-314: Improved audit logging for createOrUpdateDelegationRecordThe changes to the audit logging in this method enhance clarity and consistency:
- The
actionstring has been simplified, making it more readable.- The addition of the
namespaceproperty provides better context for the audit log entry.These improvements align well with the overall goal of refining the logging strategy.
337-338: Consistent audit logging improvements in removeDelegationRecordThe changes in this method mirror those in
createOrUpdateDelegationRecord:
- The
actionstring has been simplified for better readability.- The
namespaceproperty has been added, maintaining consistency across methods.This consistent approach to audit logging across different methods is commendable and improves the overall logging strategy.
442-443: Completed audit logging improvements in saveToIndexThe changes in this method complete the audit logging improvements across all relevant methods:
- The
actionstring has been simplified to 'save-to-index'.- The
namespaceproperty has been added, maintaining consistency with other methods.This change ensures that all entry points for indexing have consistent and clear audit logging, which is crucial as
saveToIndexis used by multiple entry points when indexing.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16335 +/- ##
=======================================
Coverage 36.75% 36.76%
=======================================
Files 6807 6807
Lines 140972 140959 -13
Branches 40145 40122 -23
=======================================
+ Hits 51811 51817 +6
+ Misses 89161 89142 -19
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 26 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)
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
libs/auth-api-lib/src/lib/delegations/dto/delegation.dto.ts (1)
48-50: LGTM! Consider adding a description to the @ApiPropertyOptional decorator.The addition of the
createdByNationalIdproperty is well-implemented and follows the existing coding style. It's correctly typed and marked as optional.To improve documentation and make the DTO more self-explanatory, consider adding a description to the
@ApiPropertyOptional()decorator. This can help API consumers understand the purpose of this field.Here's a suggested improvement:
- @ApiPropertyOptional() + @ApiPropertyOptional({ description: 'National ID of the user who created the delegation' }) createdByNationalId?: stringlibs/auth-api-lib/src/lib/delegations/models/delegation.model.ts (1)
144-144: LGTM! Consider adding a comment for consistency.The addition of
createdByNationalIdto the DTO is appropriate and aligns with the class structure. It enhances the information provided by the DTO, which can be useful for audit purposes.For consistency with other properties in the class, consider adding a brief comment explaining the purpose of
createdByNationalId, similar to the comment forreferenceId. For example:/** * National ID of the user who created this delegation */ @Column({ type: DataType.STRING, allowNull: true, }) createdByNationalId?: string;This would improve code readability and maintain consistency throughout the class.
apps/services/auth/admin-api/src/app/v2/delegations/delegation-admin.controller.ts (1)
109-126: Improved audit logging with potential for refinementThe use of
auditService.auditPromisesignificantly enhances the audit trail by capturing detailed information about the delegation operation. This is a positive change that improves system observability.However, consider the following suggestions:
- The use of 'Unknown' as a fallback for undefined values might not be ideal. Consider using a more descriptive placeholder or omitting the field entirely if it's undefined.
- The complexity of the
resourcesfunction might make it harder to maintain. Consider extracting it to a separate, testable function for better readability and maintainability.Here's a suggested refactoring to address these points:
const formatAuditResources = (res: DelegationDTO) => { const fields = [ ['id', res.id], ['toNationalId', res.toNationalId], ['fromNationalId', res.fromNationalId], ['createdByNationalId', res.createdByNationalId] ]; return fields .filter(([_, value]) => value !== undefined) .map(([key, value]) => `${key}: ${value}`) .join(', '); }; await this.auditService.auditPromise( { system: true, namespace, action: 'createByZendeskId', resources: formatAuditResources, meta: { id }, }, this.delegationAdminService.createDelegationByZendeskId(id), );This refactoring improves readability and allows for easier unit testing of the
formatAuditResourcesfunction.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- apps/services/auth/admin-api/src/app/v2/delegations/delegation-admin.controller.ts (1 hunks)
- libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (2 hunks)
- libs/auth-api-lib/src/lib/delegations/dto/delegation.dto.ts (1 hunks)
- libs/auth-api-lib/src/lib/delegations/models/delegation.model.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/services/auth/admin-api/src/app/v2/delegations/delegation-admin.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/dto/delegation.dto.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/models/delegation.model.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 (3)
apps/services/auth/admin-api/src/app/v2/delegations/delegation-admin.controller.ts (1)
102-102: Audit decorator enhances traceabilityThe addition of the
@Auditdecorator improves the traceability of thecreateByZendeskIdoperation. This aligns well with the NestJS architecture and enhances the overall security and accountability of the system.libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (2)
Line range hint
1-368: Approve overall file structure and adherence to guidelinesThe file structure and implementation adhere to the coding guidelines for
libs/**/*files. Specifically:
- The code demonstrates good reusability practices, with the
DelegationAdminCustomServicepotentially being used across different NextJS apps.- TypeScript is used effectively for defining types and method signatures.
- The code structure supports effective tree-shaking and bundling practices.
Line range hint
156-192: Approve changes and verify impact on codebaseThe changes to the
createDelegationByZendeskIdmethod improve its functionality by returning the created delegation data. This is a positive change that provides more information to the caller.To ensure this change doesn't introduce any breaking changes, please run the following script to check for any usage of this method in the codebase:
If any usages are found, please ensure they are updated to handle the new return value appropriately.
apps/services/auth/admin-api/src/app/v2/delegations/delegation-admin.controller.ts
Outdated
Show resolved
Hide resolved
# Conflicts: # libs/auth-api-lib/src/lib/delegations/models/delegation.model.ts
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.auth.spec.ts (1)
164-173: LGTM: Improved mock implementation with a suggestion.The updated mock implementation for
createDelegationByZendeskIdnow returns a more detailedDelegationDTOobject, which improves the fidelity of the test data. This change enhances type safety and better represents the actual service response.Suggestion: Consider using a factory function or a fixture to generate the mock
DelegationDTOobject. This would make it easier to maintain and reuse across different tests.Example:
const createMockDelegationDTO = (): DelegationDTO => ({ id: '123', fromNationalId: '1234567890', fromName: 'Test', toNationalId: '0987654321', toName: 'Test', createdByNationalId: '0101010101', }); jest .spyOn(delegationAdminService, 'createDelegationByZendeskId') .mockImplementation(() => Promise.resolve(createMockDelegationDTO()));This approach would make it easier to maintain consistent mock data across your test suite.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- apps/services/auth/admin-api/src/app/v2/delegations/delegation-admin.controller.ts (1 hunks)
- apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.auth.spec.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/services/auth/admin-api/src/app/v2/delegations/delegation-admin.controller.ts
🧰 Additional context used
📓 Path-based instructions (1)
apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.auth.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."
📓 Learnings (1)
apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.auth.spec.ts (2)
Learnt from: saevarma PR: island-is/island.is#16257 File: apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.auth.spec.ts:178-201 Timestamp: 2024-10-04T07:46:37.013Z Learning: In `apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.auth.spec.ts`, ensure that test case names accurately describe the scenario being tested, particularly in cases involving invalid request signatures.Learnt from: saevarma PR: island-is/island.is#16257 File: apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.auth.spec.ts:178-201 Timestamp: 2024-10-08T15:39:04.351Z Learning: In `apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.auth.spec.ts`, ensure that test case names accurately describe the scenario being tested, particularly in cases involving invalid request signatures.
🔇 Additional comments (2)
apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.auth.spec.ts (2)
14-18: LGTM: Import statement reorganization.The reorganization of imports improves code readability by grouping related entities from the
@island.is/auth-api-libmodule. This change aligns with TypeScript and NestJS best practices for import management.
Line range hint
1-238: Overall assessment: Well-structured and comprehensive test suite.This test file demonstrates adherence to NestJS best practices for testing, including:
- Proper use of dependency injection and service encapsulation.
- Comprehensive coverage of authentication and authorization scenarios.
- Use of
setupAppandsetupAppWithoutAuthfor different test contexts.- Clear separation of arrange, act, and assert phases in test cases.
The recent changes have improved the mock data fidelity and import organization without introducing any issues. The test suite remains robust and well-structured.
* audit log delegation-admin webhook * log the response in resource and request in meta * remove audit in decorator * make createdBy optional * attempt to fix pipe * attempt to fix pipe * merge with main * fix tests --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
) * feat(auth-admin): Delegation-admin webhook auditlog (#16335) * audit log delegation-admin webhook * log the response in resource and request in meta * remove audit in decorator * make createdBy optional * attempt to fix pipe * attempt to fix pipe * merge with main * fix tests --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(service-portal): Delete and view delegation in service portal form me list (#16400) * fixes wrong validTo date in DelegationViewModal and show delete button for outgoing delegation * remove log * chore: nx format:write update dirty files --------- Co-authored-by: andes-it <builders@andes.is> --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> Co-authored-by: Magnea Rún Vignisdóttir <magneavignisdottir@gmail.com> Co-authored-by: andes-it <builders@andes.is>
What
Audit logs for delegation-admin Zendesk route
Why
☝️
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes