feat(ids-api): Default scopes for clients #16476
Conversation
WalkthroughThis pull request introduces a migration that adds default scopes to 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: 2
🧹 Outside diff range and nitpick comments (2)
libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js (2)
1-1: Remove redundant 'use strict' directiveThe 'use strict' directive is unnecessary in JavaScript modules as they are automatically in strict mode. Consider removing this line to adhere to modern JavaScript best practices.
Apply this change:
-'use strict';🧰 Tools
🪛 Biome
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
35-40: Consider implementing a rollback methodWhile it may not be necessary to remove the default scopes, it's generally a good practice to provide rollback functionality in migrations. This allows for more flexibility in managing database states and can be crucial for testing or recovering from failed deployments.
Consider implementing a
downmethod that removes the default scopes added by this migration. Here's a potential implementation:async down (queryInterface, Sequelize) { const defaultScopes = ['email', 'address', 'phone']; const clients = await queryInterface.sequelize.query( 'SELECT "client_id" FROM "client" WHERE "client_type" IN (:clientTypes);', { type: queryInterface.sequelize.QueryTypes.SELECT, replacements: { clientTypes: ['web', 'native', 'spa'] } } ); const clientIds = clients.map(client => client.client_id); if (clientIds.length) { await queryInterface.sequelize.query( 'DELETE FROM "client_allowed_scope" WHERE "client_id" IN (:clientIds) AND "scope_name" IN (:scopes)', { replacements: { clientIds, scopes: defaultScopes } } ); } }This implementation would remove the default scopes added by the
upmethod, providing a way to revert the changes if necessary.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js (1 hunks)
- libs/auth-api-lib/src/lib/clients/admin/admin-clients.service.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.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/src/lib/clients/admin/admin-clients.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."
🪛 Biome
libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (3)
libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js (2)
12-17: LGTM: Efficient retrieval of existing scopesThe query to fetch existing scopes is well-structured and uses parameterized input. The use of a Set for lookup is an efficient approach for checking existing scopes.
32-32: LGTM: Efficient bulk insert of new scopesThe use of
bulkInsertis an efficient way to insert multiple rows at once. This approach is suitable for potentially large datasets and helps minimize database round-trips.libs/auth-api-lib/src/lib/clients/admin/admin-clients.service.ts (1)
634-646: Approve changes with suggestions for improvementThe implementation correctly adds the new default scopes (email, phone, and address) for web and native client types, aligning with the PR objectives. However, consider the following improvements:
- Fix the indentation of the new code block to match the rest of the file.
- Add a comment explaining the rationale behind adding these specific scopes by default.
Here's a suggested revision with improved formatting and a comment:
switch (client.clientType) { case ClientType.web: case ClientType.native: - scopes.push(...[{ - clientId: client.clientId, - scopeName: 'profile', - }, { - clientId: client.clientId, - scopeName: 'email', - }, { - clientId: client.clientId, - scopeName: 'phone', - }, { - clientId: client.clientId, - scopeName: 'address', - }]) + // Add default scopes for web and native clients to ensure + // access to essential user information + scopes.push( + ...[ + { clientId: client.clientId, scopeName: 'profile' }, + { clientId: client.clientId, scopeName: 'email' }, + { clientId: client.clientId, scopeName: 'phone' }, + { clientId: client.clientId, scopeName: 'address' }, + ] + ) }Is a database migration planned to add these new default scopes for existing web and native clients? If not, consider creating one to ensure consistency across all clients.
| const clients = await queryInterface.sequelize.query(`SELECT "client_id" FROM "client" WHERE "client_type" IN ('web', 'native', 'spa');`, { type: queryInterface.sequelize.QueryTypes.SELECT }); | ||
| const clientIds = clients.map(client => client.client_id); | ||
|
|
||
| if (!clientIds.length) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using parameterized query for better security
While the current query is not directly vulnerable to SQL injection (as it doesn't use user input), it's a good practice to use parameterized queries consistently. Consider refactoring to use query parameters.
Here's a suggested refactoring:
const clients = await queryInterface.sequelize.query(
'SELECT "client_id" FROM "client" WHERE "client_type" IN (:clientTypes);',
{
type: queryInterface.sequelize.QueryTypes.SELECT,
replacements: { clientTypes: ['web', 'native', 'spa'] }
}
);This approach is more secure and consistent with best practices for database queries.
| const rows = []; | ||
| const defaultScopes = ['email', 'address', 'phone'] | ||
|
|
||
| for (const clientId of clientIds) { | ||
| rows.push(...defaultScopes.filter((scope => { | ||
| return !existingScopesLookup.has(`${clientId}-${scope}`); | ||
| })).map(scope => ({ client_id: clientId, scope_name: scope }))); | ||
| } | ||
|
|
||
| if (!rows.length) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting default scopes to a constant
The default scopes array is defined within the method. For better maintainability and potential reuse, consider extracting it to a constant at the module level.
Apply this change at the top of the file:
const DEFAULT_SCOPES = ['email', 'address', 'phone'];
module.exports = {
async up(queryInterface, Sequelize) {
// ... existing code ...
// Replace line 20 with:
// const defaultScopes = DEFAULT_SCOPES;
// ... rest of the code ...
},
// ... down method ...
};This change improves code readability and makes it easier to update default scopes in the future if needed.
Datadog ReportAll test runs ✅ 9 Total Test Services: 0 Failed, 9 Passed Test 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 (1)
libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js (1)
1-3: Remove redundant 'use strict' directiveThe 'use strict' directive on line 1 is redundant in ES modules, as they are always in strict mode. Consider removing this line to improve code cleanliness.
Apply this change:
-'use strict' module.exports = {🧰 Tools
🪛 Biome
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js (1 hunks)
- libs/auth-api-lib/src/lib/clients/admin/admin-clients.service.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/auth-api-lib/src/lib/clients/admin/admin-clients.service.ts
🧰 Additional context used
📓 Path-based instructions (1)
libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.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."
🪛 Biome
libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (2)
libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js (2)
4-45: LGTM: Well-implemented migration logicThe
upmethod is well-structured and follows good practices:
- It uses parameterized queries for security.
- It efficiently retrieves and processes data.
- It includes early returns to avoid unnecessary processing.
Good job on the implementation!
47-52: LGTM: Appropriate handling of down migrationThe
downmethod is intentionally left empty with a clear comment explaining that no rollback is needed. This is an acceptable approach when the migration doesn't require reversal.
| ) | ||
|
|
||
| const rows = [] | ||
| const defaultScopes = ['email', 'address', 'phone'] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting default scopes to a constant
For better maintainability and potential reuse, consider extracting the default scopes array to a constant at the module level.
Apply this change at the top of the file:
const DEFAULT_SCOPES = ['email', 'address', 'phone'];
module.exports = {
async up(queryInterface, Sequelize) {
// ... existing code ...
// Replace line 28 with:
const defaultScopes = DEFAULT_SCOPES;
// ... rest of the code ...
},
// ... down method ...
};This change improves code readability and makes it easier to update default scopes in the future if needed.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16476 +/- ##
==========================================
+ Coverage 36.77% 36.80% +0.03%
==========================================
Files 6847 6839 -8
Lines 141873 141440 -433
Branches 40424 40257 -167
==========================================
- Hits 52167 52051 -116
+ Misses 89706 89389 -317
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 44 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
What
Adds phone, email and address as default scopes when creating clients
Adds migration that inserts these default scopes for existing clients
Why
These scopes should be granted by default
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit