chore(ids-auth-api): Change handling of alsoForDelegatedUser in createScope helper #16046
Conversation
… set value to false for one existing scope.
WalkthroughThis pull request introduces a database migration script that modifies 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
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
|
jonarnarbriem
added
the
deprecated:automerge
![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
Outside diff range and nitpick comments (1)
libs/auth-api-lib/seeders/20240917153226-set-also-for-delegated-user-false.js (1)
5-7: Avoid Hardcoding Scope Names for MaintainabilityHardcoding the scope name
'@island.is/signature-collection'reduces flexibility and can lead to maintenance issues if the scope name changes.Consider defining the scope name as a constant at the top of the module:
+const scopeName = '@island.is/signature-collection'; module.exports = { up(queryInterface) { return queryInterface.sequelize.transaction(async (transaction) => { await queryInterface.sequelize.query( ` UPDATE api_scope SET also_for_delegated_user = false - WHERE name = '@island.is/signature-collection' + WHERE name = '${scopeName}' `, { transaction }, ) }) },
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- libs/auth-api-lib/seeders/20240917153226-set-also-for-delegated-user-false.js (1 hunks)
- libs/auth-api-lib/seeders/data/helpers/createScope.ts (2 hunks)
Additional context used
Path-based instructions (2)
libs/auth-api-lib/seeders/20240917153226-set-also-for-delegated-user-false.js (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/seeders/data/helpers/createScope.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 not posted (3)
libs/auth-api-lib/seeders/data/helpers/createScope.ts (3)
51-54: Well-Documented Addition ofalsoForDelegatedUserPropertyThe introduction of the
alsoForDelegatedUseroptional property in theScopeOptionsinterface is clear and follows TypeScript best practices. The JSDoc comment accurately describes its purpose and default behavior.
63-63: Consistent Default Value LogicThe use of the nullish coalescing operator (
??) foralso_for_delegated_userensures thatfalseis the default value whenalsoForDelegatedUserisundefined. This maintains consistency with the documented default behavior.
51-54: Ensure Consistency in Optional Delegation PropertiesNow that
alsoForDelegatedUseris optional and defaults tofalse, verify that this property is intentionallytrueonly where necessary. This helps prevent unintended exposure of scopes to delegated users.To check where
alsoForDelegatedUseris set totrue, you can run:Verification successful
Verified:
alsoForDelegatedUserUsage is Intentional and NecessaryThe
alsoForDelegatedUser: trueproperty is used appropriately in thefindActorApiScopesmethod ofresources.service.ts. This method filters API scopes to include only those explicitly marked for delegated users, aligning with the property's intended purpose. No other instances of setting this property totruewere found, indicating it's not being overused elsewhere in the codebase.
- Location:
libs/auth-api-lib/src/lib/resources/resources.service.ts- Context: Used in
findActorApiScopesmethod to filter API scopes for delegated usersScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all instances where alsoForDelegatedUser is explicitly set to true. # Search through seeders for alsoForDelegatedUser: true rg --type ts 'alsoForDelegatedUser:\s*true'Length of output: 138
Script:
#!/bin/bash # Description: View the context of alsoForDelegatedUser: true usage in resources.service.ts # Display the relevant part of the file with context rg --type ts -C 10 'alsoForDelegatedUser:\s*true' libs/auth-api-lib/src/lib/resources/resources.service.ts # Look for any comments mentioning alsoForDelegatedUser in the same file rg --type ts '(\/\/|\/\*|\*).*alsoForDelegatedUser' libs/auth-api-lib/src/lib/resources/resources.service.tsLength of output: 910
libs/auth-api-lib/seeders/20240917153226-set-also-for-delegated-user-false.js
Show resolved
Hide resolved
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)
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16046 +/- ##
========================================
Coverage 36.73% 36.74%
========================================
Files 6735 6733 -2
Lines 138280 138179 -101
Branches 39319 39284 -35
========================================
- Hits 50797 50772 -25
+ Misses 87483 87407 -76
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 20 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
alsoForDelegatedUser is now set to false by default
added option to set to true if wanted
seeder script to set value to false for one existing scope
...
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