fix(auth-admin): Allow delegation-admin into domains controller #17125
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (3)
apps/services/auth/delegation-api/src/app/domains/domains.controller.ts (3)
95-98: Consider security implications of scope changesThe changes appropriately enhance access control by:
- Maintaining authentication through
IdsUserGuard- Providing alternative access paths via
delegationSystemscope- Preserving admin access via
delegationSystemAdminscopeHowever, please ensure that:
- The
delegationSystemscope provides appropriate access level for non-admin users- The change doesn't unintentionally expose sensitive domain information
Line range hint
63-94: Update method documentation to reflect new access patternsThe
@Documentationdecorator should be updated to reflect that the endpoint is now accessible via multiple scope combinations. This will help API consumers understand the various access paths.@Documentation({ description: `Get all domains. Provides query parameters to filter domains - delegation support and/or specific delegation direction.`, + delegation support and/or specific delegation direction. Accessible with + either delegation system access or delegation admin access.`, request: {
Controller scopes are indeed too restrictive for read-only operations
The current controller uses
delegationScopeswhich includes@island.is/auth/delegations:writescope. This is overly restrictive for endpoints that only perform read operations. The scope should be adjusted to match the actual requirements of the endpoints.
- Consider creating separate read-only scopes for delegation lookups
- Update the controller to use appropriate read scopes instead of write scopes
- If write operations exist in the controller, consider splitting the endpoints into separate controllers with different scope requirements
🔗 Analysis chain
Line range hint
43-44: Verify if base controller scopes are too restrictiveThe controller-level
@Scopes(...delegationScopes)might be too restrictive given the PR objective to allow broader access to delegation lookups.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the contents of delegationScopes to ensure it's not overly restrictive rg -A 5 "export const delegationScopes.*="Length of output: 411
Script:
#!/bin/bash # Check the definition of AuthScope.delegations and AdminPortalScope.delegations rg -A 2 "export (enum|const) (AuthScope|AdminPortalScope)" libs/auth/scopes/src/lib/auth.scope.tsLength of output: 234
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/services/auth/delegation-api/src/app/domains/domains.controller.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/services/auth/delegation-api/src/app/domains/domains.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."
🔇 Additional comments (2)
apps/services/auth/delegation-api/src/app/domains/domains.controller.ts (2)
18-18: LGTM: Import addition is appropriate
The addition of AdminPortalScope import aligns with the PR objective to enhance delegation access control.
95-98: Verify scope hierarchy and implications
While the addition of both scopes (delegationSystemAdmin and delegationSystem) aligns with the PR objective, we should verify:
- The scope hierarchy - is
delegationSystemAdmina superset ofdelegationSystem? - The interaction with controller-level
delegationScopes
✅ Verification successful
Scopes are correctly layered and not redundant
Based on the findings:
- The scopes are distinct and hierarchical:
delegationSystem = '@admin.island.is/delegation-system'delegationSystemAdmin = '@admin.island.is/delegation-system:admin'
- The controller has class-level
@Scopes(...delegationScopes)and method-level scopes working together - The
:adminsuffix indelegationSystemAdminindicates a higher privilege level, following standard naming convention
The implementation follows secure authorization patterns by:
- Using both
IdsUserGuardandScopesGuard - Properly layering scopes at both class and method levels
- Following the established scope naming convention
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check scope definitions and hierarchy
rg -A 10 "export enum AdminPortalScope"
# Check for any scope documentation or tests
fd -e ts -e md . | xargs rg -l "delegationSystemAdmin.*delegationSystem|scope.*hierarchy"
Length of output: 1280
Script:
#!/bin/bash
# Let's search for the specific scopes in the enum and their usage
rg "delegationSystem(Admin)?" libs/auth/scopes/src/lib/admin-portal.scope.ts
# Let's also check for any documentation about these scopes
fd "README|CONTRIBUTING" -e md | xargs rg -l "delegation.*scope"
# Let's check for any tests that might reveal the scope hierarchy
fd "(test|spec)" -e ts | xargs rg -l "delegationSystem"
# Let's check the controller class for any class-level scopes
rg -B 5 "class DomainsController" apps/services/auth/delegation-api/src/app/domains/domains.controller.ts
Length of output: 583
Script:
#!/bin/bash
# Let's check for any class-level scopes on the controller
rg -B 10 "@UseGuards|@Scopes" apps/services/auth/delegation-api/src/app/domains/domains.controller.ts
# Let's also check the findAll method implementation to understand the context
ast-grep --pattern $'findAll($$$) {
$$$
}' apps/services/auth/delegation-api/src/app/domains/domains.controller.ts
# Let's check if there are any other methods using these scopes
rg "delegationSystem|delegationSystemAdmin" apps/services/auth/delegation-api/src/app/domains/domains.controller.ts
Length of output: 977
Datadog ReportAll test runs ❌ 8 Total Test Services: 1 Failed, 7 Passed Test Services
❌ Failed Tests (19)
🔻 Code Coverage Decreases vs Default Branch (1)
|
GunnlaugurG
added
the
deprecated:automerge
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17125 +/- ##
=======================================
Coverage 35.71% 35.72%
=======================================
Files 6920 6920
Lines 147498 147498
Branches 42001 42001
=======================================
+ Hits 52686 52689 +3
+ Misses 94812 94809 -3
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.
|
* allow delegation admin into domain controller * fix scopes for find all --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
…) (#17130) * allow delegation admin into domain controller * fix scopes for find all --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
When fetching delegations for delegation lookups, those who did not have
@admin.island.is/delegationsscope could not look it up since that was required to get the domain name for the delegation.Why
So all users with delegation system access can lookup delegations.
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit