feat(ids-api): Filter out accessControlled scopes #16175
Conversation
WalkthroughThe changes involve updating 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)
libs/auth-api-lib/src/lib/delegations/delegations-incoming-custom.service.ts (1)
190-195: LGTM! Consider enhancing readability with a separate variable.The changes effectively filter out access-controlled scopes, aligning with the PR objective. The implementation maintains type safety and doesn't affect tree-shaking or reusability.
To improve readability, consider extracting the filter condition into a separate variable:
const isAllowedScope = (s: ApiScopeInfo) => !s.isAccessControlled && s.supportedDelegationTypes?.some( (dt) => dt.delegationType === AuthDelegationType.GeneralMandate ); const customApiScopes = clientAllowedApiScopes.filter(isAllowedScope);This separation of concerns makes the code more self-documenting and easier to maintain.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- libs/auth-api-lib/src/lib/delegations/delegations-incoming-custom.service.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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."
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16175 +/- ##
==========================================
+ Coverage 36.69% 36.70% +0.01%
==========================================
Files 6778 6776 -2
Lines 139671 139571 -100
Branches 39719 39695 -24
==========================================
- Hits 51247 51236 -11
+ Misses 88424 88335 -89
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 14 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 (2) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
libs/auth-api-lib/src/lib/delegations/delegations-incoming-custom.service.ts (1)
208-214: LGTM: Enhanced filtering logic in findAllAvailableGeneralMandateThe updated filtering logic in the
findAllAvailableGeneralMandatemethod aligns well with the PR objective of filtering out access-controlled scopes. The implementation looks correct and effectively uses the newly addedfilterByCustomScopeRulemethod.Consider the performance impact of the multiple filtering conditions, especially for large datasets. If performance becomes an issue, you might want to explore optimizing this filtering process.
If performance becomes a concern, consider refactoring the filtering logic into a separate method for better readability and potential optimization:
private filterCustomApiScopes(scopes: ApiScopeInfo[]): ApiScopeInfo[] { return scopes.filter( (s) => !s.isAccessControlled && this.filterByCustomScopeRule(s) && s.supportedDelegationTypes?.some( (dt) => dt.delegationType === AuthDelegationType.GeneralMandate, ), ); }Then use it in the
findAllAvailableGeneralMandatemethod:const customApiScopes = this.filterCustomApiScopes(clientAllowedApiScopes);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- libs/auth-api-lib/src/lib/delegations/DelegationConfig.ts (3 hunks)
- libs/auth-api-lib/src/lib/delegations/delegations-incoming-custom.service.ts (4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/auth-api-lib/src/lib/delegations/DelegationConfig.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."
🔇 Additional comments (10)
libs/auth-api-lib/src/lib/delegations/DelegationConfig.ts (7)
5-5: Excellent use of shared types!The import of
AuthDelegationTypefrom a shared types module aligns well with our coding guidelines. This change promotes reusability across different NextJS apps and supports effective tree-shaking and bundling practices.
24-24: Consistent type usage maintainedThe refinement check has been correctly updated to use
AuthDelegationType. This change maintains type safety and is consistent with the import modification. It's a good example of proper TypeScript usage for defining props and types.
45-45: Consistent enumeration usage in configurationThe
onlyForDelegationTypefields have been correctly updated to useAuthDelegationType.ProcurationHolder. This change maintains consistency with the import and schema modifications, ensuring type safety throughout the configuration.Also applies to: 49-49
54-55: Correct enumeration usage with multiple delegation typesThe
onlyForDelegationTypefield forApiScope.samradsgatthas been properly updated to useAuthDelegationType.ProcurationHolderandAuthDelegationType.Custom. This change maintains consistency with previous modifications while preserving the inclusion of multiple delegation types.
61-62: Consistent enumeration updates across multiple scopesThe
onlyForDelegationTypefields for bothApiScope.financeSalaryandApiScope.companyhave been correctly updated to useAuthDelegationType.ProcurationHolderandAuthDelegationType.Custom. These changes maintain consistency across multiple scope configurations while preserving the inclusion of multiple delegation types.Also applies to: 68-69
Line range hint
1-105: Overall assessment: Excellent adherence to coding guidelinesThe changes in this file consistently update the delegation type from
DelegationTypetoAuthDelegationTypeacross all relevant configurations. These modifications:
- Maintain type safety throughout the file.
- Promote reusability by using shared types from a common module.
- Support effective tree-shaking and bundling practices.
- Properly utilize TypeScript for defining props and exporting types.
The overall structure and purpose of the configuration remain unchanged, ensuring that the existing functionality is preserved while improving type consistency.
76-77: Consistent updates for custom scopes and migration noteThe
onlyForDelegationTypefields for custom scopes (including '@akureyri.is/service-portal', '@island.is/applications/orkusjodur', and '@skagafjordur.is/ibuagatt') have been correctly updated to useAuthDelegationType.ProcurationHolderandAuthDelegationType.Custom. These changes maintain consistency across all scope configurations.Note: There's a TODO comment regarding a scope migration for '@island.is/applications/orkusjodur'. Please ensure this is addressed in future updates.
To verify the TODO comment and its relevance, run the following script:
Also applies to: 85-86, 93-94
✅ Verification successful
Action Required: Update Verification Scripts Due to Unrecognized File Type
The initial verification scripts encountered issues with the
rgcommand not recognizing thetypescriptfile type. Please run the updated scripts below to ensure accurate verification:
Action Required: Update Verification Scripts with Correct File Extensions
Please run the updated verification scripts below to ensure accurate search results:
Verification Successful: TODO Comment and Branch Confirmed
The TODO comment regarding the scope migration for
@island.is/applications/orkusjodurhas been located inDelegationConfig.ts, and the branchauth-api/custom-delegation-scope-ruleexists in the repository. Additionally, related mentions inlibs/auth/scopes/src/lib/api.scope.tshave been identified, ensuring comprehensive coverage of the migration effort.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the presence of the TODO comment and related branch # Test 1: Search for the TODO comment echo "Searching for TODO comment:" rg --type typescript "Todo: add this to the scope migration of the branch" libs/auth-api-lib/src/lib/delegations/DelegationConfig.ts # Test 2: Check if the mentioned branch exists echo "Checking for the existence of the auth-api/custom-delegation-scope-rule branch:" git ls-remote --exit-code --heads origin auth-api/custom-delegation-scope-rule # Test 3: Search for any mentions of '@island.is/applications/orkusjodur' in other files echo "Searching for mentions of '@island.is/applications/orkusjodur' in other files:" rg --type typescript "@island.is/applications/orkusjodur" --glob '!libs/auth-api-lib/src/lib/delegations/DelegationConfig.ts'Length of output: 908
Script:
#!/bin/bash # Description: Check for the presence of the TODO comment and related branch # Test 1: Search for the TODO comment echo "Searching for TODO comment:" rg "Todo: add this to the scope migration of the branch" libs/auth-api-lib/src/lib/delegations/DelegationConfig.ts # Test 2: Check if the mentioned branch exists echo "Checking for the existence of the auth-api/custom-delegation-scope-rule branch:" git ls-remote --exit-code --heads origin auth-api/custom-delegation-scope-rule # Test 3: Search for any mentions of '@island.is/applications/orkusjodur' in other files echo "Searching for mentions of '@island.is/applications/orkusjodur' in other files:" rg "@island.is/applications/orkusjodur" --glob '!libs/auth-api-lib/src/lib/delegations/DelegationConfig.ts' -t ts -t tsxLength of output: 906
Script:
#!/bin/bash # Description: Check for the presence of the TODO comment and related branch # Test 1: Search for the TODO comment echo "Searching for TODO comment:" rg "Todo: add this to the scope migration of the branch" libs/auth-api-lib/src/lib/delegations/DelegationConfig.ts # Test 2: Check if the mentioned branch exists echo "Checking for the existence of the auth-api/custom-delegation-scope-rule branch:" git ls-remote --exit-code --heads origin auth-api/custom-delegation-scope-rule # Test 3: Search for any mentions of '@island.is/applications/orkusjodur' in other files echo "Searching for mentions of '@island.is/applications/orkusjodur' in other files:" rg "@island.is/applications/orkusjodur" --glob '*.ts' --glob '*.tsx' --glob '!libs/auth-api-lib/src/lib/delegations/DelegationConfig.ts'Length of output: 986
libs/auth-api-lib/src/lib/delegations/delegations-incoming-custom.service.ts (3)
3-3: LGTM: Import of ConfigType enhances type safetyThe addition of the ConfigType import from @nestjs/config is a good practice. It suggests that configuration types are being used, which enhances type safety and improves the overall robustness of the code.
61-62: LGTM: DelegationConfig injection enhances configurabilityThe addition of the DelegationConfig injection in the constructor is a good practice. It allows for better configurability of the service and follows NestJS dependency injection patterns. This change will likely improve the flexibility and maintainability of the code.
189-201: LGTM: New filterByCustomScopeRule method enhances scope filteringThe new
filterByCustomScopeRulemethod is a good addition that aligns with the PR objective of filtering out access-controlled scopes. The implementation looks correct and follows TypeScript best practices.However, please verify if the default behavior (returning true when no custom scope rule is found) is intentional and aligns with the expected filtering logic.
kodiakhq
bot
deleted the
feat/filter-delegations-by-access-controled-scopes
branch
* filter access control * filter out cusotmScopeRules --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
...
Attach a link to issue if relevant
What
Specify what you're trying to achieve
Why
Specify why you need to achieve this
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes