fix(auth-delegations): Fix missing domains for legal representative delegations #17304
Conversation
WalkthroughThis pull request introduces enhancements to the delegation management system across multiple services. The changes focus on improving scope filtering for delegations, introducing a new utility function Changes
Sequence DiagramsequenceDiagram
participant Service as DelegationScopeService
participant Utility as filterByCustomScopeRule
participant Model as ApiScopeInfo
Service->>Utility: Filter scopes
Utility-->>Model: Check scope rules
Utility-->>Service: Return filtered scopes
Service->>Service: Map valid scopes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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: 1
🧹 Nitpick comments (1)
libs/auth-api-lib/src/lib/resources/delegation-resources.service.ts (1)
317-317: Update comment to reflect new logic.
This revised comment clarifies that delegations for individuals are now allowed under certain circumstances. Good improvement for maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
libs/auth-api-lib/src/lib/delegations/delegation-scope.service.ts(2 hunks)libs/auth-api-lib/src/lib/delegations/delegations-incoming-custom.service.ts(2 hunks)libs/auth-api-lib/src/lib/delegations/utils/filterByScopeCustomScopeRule.ts(1 hunks)libs/auth-api-lib/src/lib/resources/delegation-resources.service.ts(3 hunks)libs/auth-api-lib/src/lib/resources/resources.module.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
libs/auth-api-lib/src/lib/resources/resources.module.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/utils/filterByScopeCustomScopeRule.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/resources/delegation-resources.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-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."
🔇 Additional comments (13)
libs/auth-api-lib/src/lib/resources/delegation-resources.service.ts (4)
28-28: No issues with the newly imported model.
The import of ApiScopeDelegationType aligns with the expanded functionality for supporting legal representative delegations.
46-47: Constructor injection of ApiScopeDelegationType model is consistent.
Injecting apiScopeDelegationTypeModel here extends the service’s domain logic for new delegation types. Ensure that naming remains consistent and descriptive to minimize confusion.
319-340: New logic for handling LegalRepresentative delegation.
- The fallback to a ForbiddenException is correctly placed to enforce restricted access for non-authorized individuals.
- Additional condition to fetch scopes from apiScopeDelegationTypeModel is appropriate, ensuring only valid scopes are returned.
- The or-clause properly merges multiple delegation type conditions.
Consider verifying test coverage for these changes to ensure all legal representative use cases are tested.
✅ Verification successful
Test coverage for LegalRepresentative delegation is comprehensive
The test files show adequate coverage for LegalRepresentative delegation scenarios:
-
delegations-filters.spec.tsincludes specific test cases for LegalRepresentative delegations, including:- Verification of legal representative access
- Filtering of delegations based on legal representative type
- Both positive and negative test cases for verification endpoints
-
Test cases cover:
- Proper authorization checks
- Scope filtering for legal representatives
- Verification of delegation types
- Integration with necessary services (National Registry, RSK)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Checking test coverage for delegationTypeFilter
rg -A 4 'delegationTypeFilter' | rg 'test'
Length of output: 40
Script:
#!/bin/bash
# Let's search for test files related to delegation resources
fd "delegation.*test" --type f
# Let's also check for test files containing LegalRepresentative
rg -l "LegalRepresentative.*test|test.*LegalRepresentative"
# Let's look for actual test cases involving legal representatives
rg -B 2 -A 4 "LegalRepresentative" "**/*test*"
Length of output: 702
Script:
#!/bin/bash
# Let's examine the test cases in the identified files
cat apps/services/auth/ids-api/src/app/delegations/test/delegations-filters.spec.ts
# Let's also check the delegation test cases
cat apps/services/auth/delegation-api/src/app/delegations/test/delegation-index/delegation-index-test-cases.ts
cat apps/services/auth/delegation-api/src/app/delegations/test/delegations-controller/delegations.controller.test-cases.ts
Length of output: 21347
310-310: Delegation type check for user.delegationType usage.
Reusing user.delegationType ensures that the filter logic is consistent. Make sure all calling methods provide user objects with the delegationType property assigned.
Run this script to confirm all code paths properly instantiate user.delegationType:
✅ Verification successful
Based on the code analysis, I can now provide a final response:
The delegationType property is a critical part of the User interface and is properly handled in the codebase.
The check for user.delegationType is valid as:
- The User interface in multiple locations (
libs/shared/types/src/lib/user.ts,libs/auth-nest-tools/src/lib/auth.ts) definesdelegationTypeas an optional array of AuthDelegationType. - The
delegationTypeFiltermethod indelegation-resources.service.tsproperly handles the case whendelegationTypeis undefined. - The property is consistently used across the codebase for authorization and delegation type checks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
rg -A 6 $'new User\\('
Length of output: 22
Script:
#!/bin/bash
# Search for User type definitions and interfaces
ast-grep --pattern 'interface User {
$$$
}'
# Search for User class definitions
ast-grep --pattern 'class User {
$$$
}'
# Search for delegationType assignments or checks
rg -A 3 "delegationType"
Length of output: 65653
libs/auth-api-lib/src/lib/delegations/utils/filterByScopeCustomScopeRule.ts (2)
1-2: Import statements align with usage.
The imports used (AuthDelegationType, ApiScopeInfo) are relevant for custom scope rule checks and properly typed.
4-20: New function for scope filtering.
- Exported as a default function, which is consistent for a small utility.
- The logic returns “true” when no matching rule is found, preserving existing default functionality.
- Appropriately checks onlyForDelegationType array for the GeneralMandate. If future needs arise, consider a more flexible or dynamic approach, e.g. passing in the required delegation type at runtime.
No issues found with correctness or clarity.
libs/auth-api-lib/src/lib/resources/resources.module.ts (2)
29-29: New import of ApiScopeDelegationType model is consistent.
This import ensures that the tracking of delegation types is unified within the ORM context.
43-43: Adding ApiScopeDelegationType to SequelizeModule.forFeature.
Including ApiScopeDelegationType in forFeature ensures the model is recognized by Sequelize. Good step for legal representative delegations.
libs/auth-api-lib/src/lib/delegations/delegation-scope.service.ts (2)
30-30: ApiScopeInfo import is valid for referencing scope details.
Importing this type clarifies the scope structure and contributes to type safety.
33-33: Introduced filterByCustomScopeRule.
Centralizing scope filtering logic in a dedicated utility function makes the code more maintainable and testable.
libs/auth-api-lib/src/lib/delegations/delegations-incoming-custom.service.ts (3)
29-29: LGTM! Good modularization.
Moving the scope rule filtering logic to a separate utility function improves code reusability and maintainability.
Line range hint 1-450: Compliant with coding guidelines.
The service implementation follows the required guidelines for libs/**/* files:
- Uses TypeScript effectively with proper type definitions
- Maintains modularity for reuse across different apps
- Follows import/export patterns that support tree-shaking
223-223: Verify the impact on existing general mandate delegations.
The change looks correct and aligns with the PR objectives to fix permission issues. However, let's verify that existing general mandate delegations continue to work as expected.
✅ Verification successful
Let me gather more information about the implementation and tests.
The changes to filterByCustomScopeRule are safe and well-tested
The implementation is correct and the tests demonstrate that general mandate delegations work as expected:
- The function correctly filters scopes based on delegation types
- Comprehensive test coverage exists for general mandate scenarios including:
- Valid general mandate delegations
- Expired delegations
- Multiple delegation type support
- Scope filtering based on delegation types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other usages of filterByCustomScopeRule to ensure consistent implementation
# Check for all usages of the function
rg -A 5 "filterByCustomScopeRule"
# Check for tests covering general mandate functionality
fd -e spec.ts -e test.ts | xargs rg -l "GeneralMandate"
Length of output: 3631
Script:
#!/bin/bash
# Check the implementation of filterByCustomScopeRule
cat libs/auth-api-lib/src/lib/delegations/utils/filterByScopeCustomScopeRule.ts
# Check general mandate related tests
rg -A 5 "GeneralMandate" apps/services/auth/ids-api/src/app/delegations/test/delegations.controller.spec.ts
Length of output: 3595
libs/auth-api-lib/src/lib/delegations/delegation-scope.service.ts
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17304 +/- ##
==========================================
- Coverage 35.69% 35.67% -0.02%
==========================================
Files 6927 6927
Lines 148705 148646 -59
Branches 42493 42472 -21
==========================================
- Hits 53080 53033 -47
+ Misses 95625 95613 -12 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 13 files 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 (1)
|
GunnlaugurG
added
the
deprecated:automerge
…elegations (#17304) * attempt to fix delegation bugs * add filterOutForDelegationType to utils func * Utils func changes --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
/domainroute was getting a 403 when trying to create a delegation as a Legal representative.Why
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
LegalRepresentativedelegation type.Bug Fixes
Documentation
Chores