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.
libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js
Outdated
Show resolved
Hide resolved
libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js
Outdated
Show resolved
Hide resolved
Datadog ReportAll test runs ✅ 13 Total Test Services: 0 Failed, 13 Passed Test ServicesThis report shows up to 10 services
|
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.
libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16476 +/- ##
==========================================
+ Coverage 36.53% 36.61% +0.07%
==========================================
Files 6888 6877 -11
Lines 143642 143058 -584
Branches 40926 40723 -203
==========================================
- Hits 52483 52384 -99
+ Misses 91159 90674 -485
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 31 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js (2)
6-12: Add documentation for client type filteringConsider adding a comment explaining why only 'web', 'native', and 'spa' client types are selected for default scopes.
+ // Only web, native, and spa clients should receive default scopes const clients = await queryInterface.sequelize.query(
50-55: Improve documentation for empty down methodWhile it's intentional to not implement rollback, the comment should explain why removing default scopes would be undesirable.
async down(queryInterface, Sequelize) { - /** - * There is no need to go back - * - */ + /** + * Rollback is not implemented as removing default scopes could break + * existing client functionality that depends on these permissions. + */ },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js (1 hunks)
🧰 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 (1)
libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js (1)
19-29: LGTM! Efficient implementation using Set for lookupsThe use of Set with composite keys provides efficient lookup capabilities, and the query is properly parameterized.
libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js
Show resolved
Hide resolved
libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js
Outdated
Show resolved
Hide resolved
libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js (1 hunks)
🧰 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)
libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js
Show resolved
Hide resolved
libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js (1)
49-53: Enhance error logging for better debugging
While error handling is implemented correctly, adding detailed error logging would help with debugging production issues.
} catch (err) {
+ console.error('Failed to add default scopes:', {
+ error: err.message,
+ stack: err.stack,
+ migration: '20241018150418-add-default-scopes-to-client-allowed-scope-table'
+ })
await transaction.rollback()
throw err
}📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js (1 hunks)
🧰 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)
1-1: Remove redundant 'use strict' directive
The 'use strict' directive is unnecessary as JavaScript modules are automatically in strict mode.
-'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)
37-43: Consider batch processing for large datasets
For better memory management with large datasets, consider processing clients in batches.
-for (const clientId of clientIds) {
+const BATCH_SIZE = 1000;
+for (let i = 0; i < clientIds.length; i += BATCH_SIZE) {
+ const batchClientIds = clientIds.slice(i, i + BATCH_SIZE);
+ for (const clientId of batchClientIds) {
rows.push(
...DEFAULT_SCOPES
.filter((scope) => {
return !existingScopesLookup.has(`${clientId}-${scope}`)
})
.map((scope) => ({ client_id: clientId, scope_name: scope })),
)
+ }
}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js (3)
49-51: Add logging for affected rows
Consider adding logging to track the number of scopes added, which would be helpful for monitoring and verification.
await queryInterface.bulkInsert('client_allowed_scope', rows, {
transaction,
})
+ console.log(`Added ${rows.length} default scopes to ${new Set(rows.map(r => r.client_id)).size} clients`)52-55: Use more descriptive error variable name
Rename 'err' to 'error' for better clarity and consistency with JavaScript conventions.
- } catch (err) {
+ } catch (error) {
await transaction.rollback()
- throw err
+ throw error
}58-63: Document the rationale for skipping down migration
While the comment indicates there's no need to go back, it would be helpful to document the reasoning behind this decision. Consider adding a more detailed comment explaining why rolling back these default scopes is not necessary or desirable.
async down(queryInterface, Sequelize) {
/**
- * There is no need to go back
- *
- */
+ * Down migration is intentionally not implemented because:
+ * 1. These scopes are considered fundamental to client functionality
+ * 2. Removing them could break existing client integrations
+ * 3. The change is considered forward-only
+ */
},📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js (1 hunks)
🧰 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)
libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js
Show resolved
Hide resolved
…into feat/default-scopes
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)
49-51: Add logging for migration progress
Consider adding logging statements to track the migration progress:
await queryInterface.bulkInsert('client_allowed_scope', rows, {
transaction,
})
+console.log(`Successfully inserted ${rows.length} new scope entries for ${clientIds.length} clients`);📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js (1 hunks)
🧰 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 (1)
libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js (1)
59-64: Verify the decision to skip down migration
The comment suggests that rollback is not needed. However, please confirm if this aligns with your team's migration strategy. Consider:
- What happens if the feature needs to be rolled back?
- How would you handle scope removal if needed in the future?
✅ Verification successful
Empty down migration is appropriate in this case
The up migration only adds default scopes ('email', 'address', 'phone') to existing clients of specific types. Implementing a down migration is unnecessary because:
- The migration is additive and only affects new default scopes
- These scopes are fundamental to client operation and removing them could break existing functionality
- If scope removal is needed in the future, it should be handled through a new forward migration with proper business logic
The decision to skip the down migration aligns with best practices for migrations that introduce essential default data.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are other migrations that implement scope removal
rg -l "DELETE.*FROM.*client_allowed_scope" libs/auth-api-lib/migrations/
Length of output: 74
Script:
#!/bin/bash
# Let's examine the up migration to understand what changes are being made
cat libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js
# Check for any other migrations related to client_allowed_scope table
rg -l "client_allowed_scope" libs/auth-api-lib/migrations/
# Check if there are any migrations with similar scope-related operations
rg -l "scope" libs/auth-api-lib/migrations/
Length of output: 4905
libs/auth-api-lib/migrations/20241018150418-add-default-scopes-to-client-allowed-scope-table.js
Show resolved
Hide resolved
|
@GunnlaugurG I checked with Eiki, we are no longer creating spa clients, that is the reason the type is not in the switch case. |
magnearun
added
the
deprecated:automerge
magnearun
removed
the
deprecated:automerge
magnearun
added
the
deprecated:automerge
* Adds migration for inserting default scopes to existing clients * Adds phone, address and email as default scopes when creating clients * chore: nx format:write update dirty files * small refactoring * chore: nx format:write update dirty files * wrap in try/catch and use transaction * chore: nx format:write update dirty files * fix missing transaction commit --------- Co-authored-by: andes-it <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* Adds migration for inserting default scopes to existing clients * Adds phone, address and email as default scopes when creating clients * chore: nx format:write update dirty files * small refactoring * chore: nx format:write update dirty files * wrap in try/catch and use transaction * chore: nx format:write update dirty files * fix missing transaction commit --------- Co-authored-by: andes-it <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
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