Cu 86c0bkuxc add a mechanism to migrate users from the old roles to new rbac roles #1234
Conversation
WalkthroughThis pull request introduces several changes to enhance role management within the application. It adds an export statement in Changes
Possibly related PRs
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1234 +/- ##
=======================================
Coverage 87.07% 87.08%
=======================================
Files 91 91
Lines 5988 6015 +27
=======================================
+ Hits 5214 5238 +24
- Misses 774 777 +3 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
…ers-from-the-old-roles-to-new-RBAC-roles
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
src/model/role.js (1)
Line range hint
140-220: Approved: Roles export enhances modularity. Consider adding JSDoc comments.The export of the
rolesobject is a good change that improves modularity and allows for easier reuse of role definitions across the application. The role permissions are well-defined and follow the principle of least privilege.Consider adding JSDoc comments to describe the structure and purpose of the
rolesobject. This would improve code documentation and make it easier for other developers to understand and use this exported constant. For example:/** * @typedef {Object} RolePermissions * @property {boolean} channel-view-all * @property {boolean} channel-manage-all * ... (other permissions) */ /** * @typedef {Object} Role * @property {string} name - The name of the role * @property {RolePermissions} permissions - The permissions associated with the role */ /** * Defines the default roles and their associated permissions * @type {Object.<string, Role>} */ export const roles = { // ... (role definitions) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/model/index.js (1 hunks)
- src/model/role.js (1 hunks)
- src/upgradeDB.js (2 hunks)
- test/unit/upgradeDBTest.js (2 hunks)
🧰 Additional context used
🪛 Biome
src/upgradeDB.js
[error] 302-335: Promise executor functions should not be
async.(lint/suspicious/noAsyncPromiseExecutor)
[error] 318-318: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
src/model/index.js (1)
23-23: LGTM! The new export aligns with the existing pattern and PR objectives.The addition of
export * from './role'is consistent with the module's existing export pattern. This change supports the PR objective of implementing a mechanism to migrate users from old roles to new RBAC roles by making the role module accessible.A few points to consider:
- Ensure that the './role' module exists and is correctly implemented.
- Be aware that this exports all named exports from the role module. If there are any naming conflicts with other exported entities, they should be resolved.
- If fine-grained control over exported entities is needed in the future, consider using named exports instead of the wildcard export.
To verify the existence of the './role' module and its exports, you can run the following script:
✅ Verification successful
**** The
export * from './role'statement successfully re-exports the necessary components from the./rolemodule. No naming conflicts detected, and this change aligns with the PR objectives of implementing RBAC role migrations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and exports of the './role' module # Check if the role.js file exists if [ -f "src/model/role.js" ]; then echo "The './role' module exists." # Display the exports from the role module echo "Exports from the role module:" grep -E "^export" src/model/role.js else echo "Warning: The './role' module does not exist in the expected location." fiLength of output: 422
src/model/role.js (1)
Line range hint
222-236: LGTM: createDefaultRoles function is well-implementedThe
createDefaultRolesfunction is efficiently implemented using async/await and Promise.all for concurrent role creation. It properly handles the creation of default roles and logs the results. The error handling is also in place.test/unit/upgradeDBTest.js (1)
17-17: Importing 'RoleModel'The
RoleModelis correctly imported, enabling role management functionalities in the tests.
src/upgradeDB.js
Outdated
|
|
||
| const userPromises = users.map(async (user) => { | ||
| let newGroup = 'manager'; | ||
| if ((user.groups && user.groups.includes('admin')) || user.superUser) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify condition using optional chaining
You can simplify the condition by using optional chaining to improve readability.
Apply this diff:
const userPromises = users.map(async (user) => {
let newGroup = 'manager';
- if ((user.groups && user.groups.includes('admin')) || user.superUser) {
+ if (user.groups?.includes('admin') || user.superUser) {
newGroup = 'admin';
}
// Update user's groups
user.groups = [newGroup];
return user.save();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ((user.groups && user.groups.includes('admin')) || user.superUser) { | |
| if (user.groups?.includes('admin') || user.superUser) { |
🧰 Tools
🪛 Biome
[error] 318-318: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/upgradeDB.js
Outdated
| return new Promise(async (resolve, reject) => { | ||
| try { | ||
| // Create default roles with permissions | ||
| for (const [roleName, roleData] of Object.entries(roles)) { | ||
| await RoleModel.findOneAndUpdate( | ||
| { name: roleName }, | ||
| roleData, | ||
| { upsert: true, new: true } | ||
| ); | ||
| logger.info(`Role ${roleName} created or updated`); | ||
| } | ||
|
|
||
| const users = await UserModel.find(); | ||
|
|
||
| const userPromises = users.map(async (user) => { | ||
| let newGroup = 'manager'; | ||
| if ((user.groups && user.groups.includes('admin')) || user.superUser) { | ||
| newGroup = 'admin'; | ||
| } | ||
| // Update user's groups | ||
| user.groups = [newGroup]; | ||
|
|
||
| return user.save(); | ||
| }); | ||
|
|
||
| await Promise.all(userPromises); | ||
|
|
||
| logger.info('Successfully updated user groups based on new role model'); | ||
| resolve(); | ||
| } catch (err) { | ||
| logger.error(`Error updating user groups: ${err}`); | ||
| reject(err); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Avoid using async functions as Promise executor functions
Using async functions as Promise executor functions can lead to unexpected behavior because exceptions thrown inside the async function may not be caught by the Promise's catch block. Instead, define the func as an async function and remove the explicit new Promise.
Apply this diff to fix the issue:
upgradeFuncs.push({
description: 'Create default roles with permissions and update user groups',
- func() {
- return new Promise(async (resolve, reject) => {
+ async func() {
try {
// Create default roles with permissions
for (const [roleName, roleData] of Object.entries(roles)) {
await RoleModel.findOneAndUpdate(
{ name: roleName },
roleData,
{ upsert: true, new: true }
);
logger.info(`Role ${roleName} created or updated`);
}
const users = await UserModel.find();
const userPromises = users.map(async (user) => {
let newGroup = 'manager';
- if ((user.groups && user.groups.includes('admin')) || user.superUser) {
+ if (user.groups?.includes('admin') || user.superUser) {
newGroup = 'admin';
}
// Update user's groups
user.groups = [newGroup];
return user.save();
});
await Promise.all(userPromises);
logger.info('Successfully updated user groups based on new role model');
- resolve();
} catch (err) {
logger.error(`Error updating user groups: ${err}`);
- reject(err);
+ throw err;
}
- });
}
});This change ensures proper error handling and cleaner code structure.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return new Promise(async (resolve, reject) => { | |
| try { | |
| // Create default roles with permissions | |
| for (const [roleName, roleData] of Object.entries(roles)) { | |
| await RoleModel.findOneAndUpdate( | |
| { name: roleName }, | |
| roleData, | |
| { upsert: true, new: true } | |
| ); | |
| logger.info(`Role ${roleName} created or updated`); | |
| } | |
| const users = await UserModel.find(); | |
| const userPromises = users.map(async (user) => { | |
| let newGroup = 'manager'; | |
| if ((user.groups && user.groups.includes('admin')) || user.superUser) { | |
| newGroup = 'admin'; | |
| } | |
| // Update user's groups | |
| user.groups = [newGroup]; | |
| return user.save(); | |
| }); | |
| await Promise.all(userPromises); | |
| logger.info('Successfully updated user groups based on new role model'); | |
| resolve(); | |
| } catch (err) { | |
| logger.error(`Error updating user groups: ${err}`); | |
| reject(err); | |
| } | |
| }); | |
| return { | |
| description: 'Create default roles with permissions and update user groups', | |
| async func() { | |
| try { | |
| // Create default roles with permissions | |
| for (const [roleName, roleData] of Object.entries(roles)) { | |
| await RoleModel.findOneAndUpdate( | |
| { name: roleName }, | |
| roleData, | |
| { upsert: true, new: true } | |
| ); | |
| logger.info(`Role ${roleName} created or updated`); | |
| } | |
| const users = await UserModel.find(); | |
| const userPromises = users.map(async (user) => { | |
| let newGroup = 'manager'; | |
| if (user.groups?.includes('admin') || user.superUser) { | |
| newGroup = 'admin'; | |
| } | |
| // Update user's groups | |
| user.groups = [newGroup]; | |
| return user.save(); | |
| }); | |
| await Promise.all(userPromises); | |
| logger.info('Successfully updated user groups based on new role model'); | |
| } catch (err) { | |
| logger.error(`Error updating user groups: ${err}`); | |
| throw err; | |
| } | |
| } | |
| }; |
🧰 Tools
🪛 Biome
[error] 302-335: Promise executor functions should not be
async.(lint/suspicious/noAsyncPromiseExecutor)
[error] 318-318: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
test/unit/upgradeDBTest.js
Outdated
| const roles = await RoleModel.find() | ||
| roles.length.should.be.exactly(4) | ||
|
|
||
| const roleNames = roles.map(r => r.name) | ||
| roleNames.should.containEql('manager') | ||
| roleNames.should.containEql('admin') | ||
| roleNames.should.containEql('operator') | ||
| }) |
There was a problem hiding this comment.
Inconsistent number of expected roles
In the test should create default roles if they do not exist, the assertion expects roles.length to be exactly 4, but only three role names ('manager', 'admin', 'operator') are checked. This suggests there might be a fourth default role that is either missing from the assertions or the expected count is incorrect.
Suggested Fix:
If there are only three default roles, adjust the expected length:
- roles.length.should.be.exactly(4)
+ roles.length.should.be.exactly(3)Alternatively, if there is a fourth role, include it in your checks:
roleNames.should.containEql('yourFourthRoleName')Ensure that all default roles are accounted for in both the role count and the role name assertions.
test/unit/upgradeDBTest.js
Outdated
| should(operatorRole.permissions['user-manage']).be.false() | ||
| should(operatorRole.permissions['client-manage-all']).be.false() |
There was a problem hiding this comment.
Assertion may fail when permission is undefined
When verifying that the 'operator' role doesn't have certain permissions, using should(...).be.false() could lead to a test failure if the permission is undefined, since undefined is not strictly equal to false.
Suggested Fix:
Modify the assertions to check that the permission is not true, covering both false and undefined cases:
- should(operatorRole.permissions['user-manage']).be.false()
- should(operatorRole.permissions['client-manage-all']).be.false()
+ should(operatorRole.permissions['user-manage']).not.be.true()
+ should(operatorRole.permissions['client-manage-all']).not.be.true()This ensures the test passes if the permission is either false or undefined, accurately reflecting that the operator should not have these permissions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| should(operatorRole.permissions['user-manage']).be.false() | |
| should(operatorRole.permissions['client-manage-all']).be.false() | |
| should(operatorRole.permissions['user-manage']).not.be.true() | |
| should(operatorRole.permissions['client-manage-all']).not.be.true() |
drizzentic
force-pushed
the
CU-86c0bkuxc_Add-a-mechanism-to-migrate-users-from-the-old-roles-to-new-RBAC-roles
branch
from
2513df2
to
424337a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
test/unit/upgradeDBTest.js (1)
597-597: Remove unnecessary console.log statement in testsThe
console.logstatement inside thecheckPermissionshelper function can clutter test output when running the test suite. It's best practice to avoid unnecessary logging in unit tests to keep the output clean.Apply this diff to remove the console log:
- console.log(`Checking permissions for role: ${role.name}`)
src/upgradeDB.js
Outdated
| const users = await UserModel.find(); | ||
|
|
||
| const userPromises = users.map(async (user) => { | ||
| let newGroup = 'manager'; |
There was a problem hiding this comment.
Unfortunately, we cannot just update a user group to manager as this might give them more privileges than they originally had. A channel object has a few properties which allow users or groups to access transaction of that channel without being an admin. See:
openhim-core-js/src/model/channels.js
Lines 180 to 182 in d259d5d
We should map these to a new role with the same name and the new channel permissions.
There was a problem hiding this comment.
Hey @rcrichton would it be right to assume that roles not linked to any channels should be discarded? I have made a few tweaks to the upgrade function which now relies on the roles defined in the allow array in the channels model.
…nnels and roles defined in channels?
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
src/upgradeDB.js (1)
307-307: Consider using a more reliable method for deep cloning objectsUsing
JSON.parse(JSON.stringify(roles))for deep cloning can lead to issues if therolesobject contains functions,Dateobjects, or other non-serializable properties. Consider using a utility likelodash.cloneDeep()or the structured cloning algorithm (structuredClone()in modern environments) for a more robust solution.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/upgradeDB.js (3 hunks)
- test/unit/upgradeDBTest.js (2 hunks)
🧰 Additional context used
🪛 Biome
src/upgradeDB.js
[error] 303-378: Promise executor functions should not be
async.(lint/suspicious/noAsyncPromiseExecutor)
src/upgradeDB.js
Outdated
| channels.forEach(channel => { | ||
| if (Array.isArray(channel.allow)) { | ||
| if (channel.txViewAcl && channel.txViewAcl.length > 0) { | ||
| channel.txViewAcl.forEach(role => { | ||
| if (!existingRoles[role]) { | ||
| existingRoles[role] = { permissions: {} } | ||
| } | ||
| if (!existingRoles[role].permissions['transaction-view-specified']) { | ||
| existingRoles[role].permissions['transaction-view-specified'] = [] | ||
| } | ||
| existingRoles[role].permissions['transaction-view-specified'].push(channel.name) | ||
| if (!existingRoles[role].permissions['channel-view-specified']) { | ||
| existingRoles[role].permissions['channel-view-specified'] = [] | ||
| } | ||
| existingRoles[role].permissions['channel-view-specified'].push(channel.name) | ||
| existingRoles[role].permissions['client-view-all'] = true | ||
| }) | ||
| } | ||
| if (channel.txRerunAcl && channel.txRerunAcl.length > 0) { | ||
| channel.txRerunAcl.forEach(role => { | ||
| if (!existingRoles[role]) { | ||
| existingRoles[role] = { permissions: {} } | ||
| } | ||
| if (!existingRoles[role].permissions['transaction-rerun-specified']) { | ||
| existingRoles[role].permissions['transaction-rerun-specified'] = [] | ||
| } | ||
| existingRoles[role].permissions['transaction-rerun-specified'].push(channel.name) | ||
| if (!existingRoles[role].permissions['channel-view-specified']) { | ||
| existingRoles[role].permissions['channel-view-specified'] = [] | ||
| } | ||
| existingRoles[role].permissions['channel-view-specified'].push(channel.name) | ||
| existingRoles[role].permissions['client-view-all'] = true | ||
| }) | ||
| } | ||
| if (channel.txViewFullAcl && channel.txViewFullAcl.length > 0) { | ||
| channel.txViewFullAcl.forEach(role => { | ||
| if (!existingRoles[role]) { | ||
| existingRoles[role] = { permissions: {} } | ||
| } | ||
| if (!existingRoles[role].permissions['transaction-view-body-specified']) { | ||
| existingRoles[role].permissions['transaction-view-body-specified'] = [] | ||
| } | ||
| existingRoles[role].permissions['transaction-view-body-specified'].push(channel.name) | ||
| if (!existingRoles[role].permissions['channel-view-specified']) { | ||
| existingRoles[role].permissions['channel-view-specified'] = [] | ||
| } | ||
| existingRoles[role].permissions['channel-view-specified'].push(channel.name) | ||
| existingRoles[role].permissions['client-view-all'] = true | ||
| }) | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated code for handling ACLs into a helper function
The code blocks processing txViewAcl, txRerunAcl, and txViewFullAcl are similar, resulting in code duplication. Refactoring these into a helper function will improve maintainability and reduce the potential for errors when making future changes.
Define a helper function outside the loop (this code is added outside the selected line ranges):
function processAcl(aclRoles, permissionKey, channelName, existingRoles) {
aclRoles.forEach((role) => {
if (!existingRoles[role]) {
existingRoles[role] = { permissions: {} };
}
if (!existingRoles[role].permissions[permissionKey]) {
existingRoles[role].permissions[permissionKey] = [];
}
existingRoles[role].permissions[permissionKey].push(channelName);
if (!existingRoles[role].permissions['channel-view-specified']) {
existingRoles[role].permissions['channel-view-specified'] = [];
}
existingRoles[role].permissions['channel-view-specified'].push(channelName);
existingRoles[role].permissions['client-view-all'] = true;
});
}Apply this diff to refactor the code within the loop:
channels.forEach((channel) => {
- if (Array.isArray(channel.allow)) {
- if (channel.txViewAcl && channel.txViewAcl.length > 0) {
- channel.txViewAcl.forEach(role => {
- if (!existingRoles[role]) {
- existingRoles[role] = { permissions: {} }
- }
- if (!existingRoles[role].permissions['transaction-view-specified']) {
- existingRoles[role].permissions['transaction-view-specified'] = []
- }
- existingRoles[role].permissions['transaction-view-specified'].push(channel.name)
- if (!existingRoles[role].permissions['channel-view-specified']) {
- existingRoles[role].permissions['channel-view-specified'] = []
- }
- existingRoles[role].permissions['channel-view-specified'].push(channel.name)
- existingRoles[role].permissions['client-view-all'] = true
- })
- }
- if (channel.txRerunAcl && channel.txRerunAcl.length > 0) {
- // similar code block
- }
- if (channel.txViewFullAcl && channel.txViewFullAcl.length > 0) {
- // similar code block
- }
- }
+ const aclMappings = [
+ { aclKey: 'txViewAcl', permissionKey: 'transaction-view-specified' },
+ { aclKey: 'txRerunAcl', permissionKey: 'transaction-rerun-specified' },
+ { aclKey: 'txViewFullAcl', permissionKey: 'transaction-view-body-specified' },
+ ];
+
+ aclMappings.forEach(({ aclKey, permissionKey }) => {
+ const aclRoles = channel[aclKey];
+ if (aclRoles && aclRoles.length > 0) {
+ processAcl(aclRoles, permissionKey, channel.name, existingRoles);
+ }
+ });
});
src/upgradeDB.js
Outdated
| const existingRoles = JSON.parse(JSON.stringify(roles)) // Deep clone the roles object | ||
|
|
||
| channels.forEach(channel => { | ||
| if (Array.isArray(channel.allow)) { |
There was a problem hiding this comment.
Remove unnecessary check on channel.allow
The condition if (Array.isArray(channel.allow)) is unnecessary because the subsequent code does not depend on channel.allow; it operates on channel.txViewAcl, channel.txRerunAcl, and channel.txViewFullAcl. Removing this condition will simplify the code without affecting functionality.
Apply this diff to remove the unnecessary condition:
- if (Array.isArray(channel.allow)) {
if (channel.txViewAcl && channel.txViewAcl.length > 0) {
// existing code
}
// other ACL checks
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (Array.isArray(channel.allow)) { | |
| if (channel.txViewAcl && channel.txViewAcl.length > 0) { | |
| // existing code | |
| } | |
| // other ACL checks |
test/unit/upgradeDBTest.js
Outdated
| const roles = await RoleModel.find() | ||
| roles.length.should.be.exactly(Object.keys(roles).length) | ||
|
|
There was a problem hiding this comment.
Rename local 'roles' variable to prevent conflict with imported 'roles'
In the test 'should not create duplicate roles if they already exist', the local variable roles is assigned the result of await RoleModel.find(), which shadows the imported roles constant. This can cause confusion and may lead to incorrect references.
To enhance code readability and prevent potential bugs, rename the local variable:
- const roles = await RoleModel.find()
- roles.length.should.be.exactly(Object.keys(roles).length)
+ const existingRoles = await RoleModel.find()
+ existingRoles.length.should.be.exactly(Object.keys(roles).length)This makes it clear when you are referring to the imported roles constant versus the roles retrieved from the database.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const roles = await RoleModel.find() | |
| roles.length.should.be.exactly(Object.keys(roles).length) | |
| const existingRoles = await RoleModel.find() | |
| existingRoles.length.should.be.exactly(Object.keys(roles).length) | |
| existingRoles.length.should.be.exactly(Object.keys(roles).length) | ||
|
|
||
| const roleNames = existingRoles.map(r => r.name) | ||
| Object.keys(roles).forEach(roleName => { | ||
| roleNames.should.containEql(roleName) | ||
| }) |
There was a problem hiding this comment.
Avoid variable shadowing of 'roles' variable
The local variable roles is being used in a way that shadows the imported roles constant from '../../src/model'. This can lead to confusion and potential errors in the test assertions.
To improve clarity and prevent any unintended behavior, consider renaming the local variables. Here's an example:
- const existingRoles = await RoleModel.find()
- existingRoles.length.should.be.exactly(Object.keys(roles).length)
- const roleNames = existingRoles.map(r => r.name)
- Object.keys(roles).forEach(roleName => {
+ const defaultRoleNames = Object.keys(roles)
+ existingRoles.length.should.be.exactly(defaultRoleNames.length)
+ const roleNames = existingRoles.map(r => r.name)
+ defaultRoleNames.forEach(roleName => {
roleNames.should.containEql(roleName)
})This change avoids confusion between the imported roles constant and the locally scoped variables.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
src/upgradeDB.js (2)
303-318: LGTM: Initial setup and helper functionThe initial setup, including retrieving channels and cloning the roles object, is well-implemented. The
updateRolePermissionshelper function is a good practice for code organization.However, consider using a more robust deep cloning method for
existingRoles:- const existingRoles = JSON.parse(JSON.stringify(roles)) // Deep clone the roles object + const existingRoles = structuredClone(roles) // Deep clone the roles objectThis change uses the
structuredClonefunction, which is more efficient and handles a wider range of object types correctly.
336-345: LGTM: Efficient role update logicThe role creation and update logic is well-implemented, using Promise.all for efficient parallel operations and findOneAndUpdate with upsert for flexibility.
Consider adding more detailed logging:
- logger.info('Successfully updated roles') + logger.info(`Successfully updated ${Object.keys(existingRoles).length} roles`)This change provides more specific information about the number of roles updated.
test/unit/upgradeDBTest.js (1)
577-587: LGTM with a minor suggestion: Avoid variable shadowingThe test case correctly verifies that duplicate roles are not created. However, to maintain consistency with the previous suggestion and improve clarity:
- const existingRoles = await RoleModel.find() - existingRoles.length.should.be.exactly(Object.keys(roles).length) + const createdRoles = await RoleModel.find() + createdRoles.length.should.be.exactly(Object.keys(roles).length) - const adminRoles = existingRoles.filter(r => r.name === 'admin') + const adminRoles = createdRoles.filter(r => r.name === 'admin')This change maintains consistency with the previous suggestion and avoids potential confusion with the imported
rolesconstant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/upgradeDB.js (2 hunks)
- test/unit/upgradeDBTest.js (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
src/upgradeDB.js (3)
12-13: LGTM: New imports and function declarationThe new imports for RoleModel, roles, and ChannelModel are correctly added. The new upgrade function is properly declared with an accurate description.
Also applies to: 300-302
346-349: LGTM: Proper error handlingThe error handling is well-implemented. Errors are logged for debugging purposes and then re-thrown to allow for proper error propagation.
300-351: Overall LGTM: Well-implemented role migration functionalityThis new upgrade function successfully implements the mechanism to migrate users from old roles to new RBAC roles, as outlined in the PR objectives. The implementation is thorough, processing channel ACLs to create and update roles with appropriate permissions.
Key strengths:
- Efficient use of async/await and Promise.all for performance.
- Good error handling and logging practices.
- Well-structured code with helper functions for better organization.
The minor suggestions provided in previous comments will further enhance the code quality, but overall, this implementation effectively achieves the goal of the pull request.
test/unit/upgradeDBTest.js (2)
16-19: LGTM: New imports added for role management testsThe new imports (RoleModel, ChannelModel, roles) are correctly added and are necessary for the new tests related to role management.
552-563: LGTM: Proper test setup for updateFunction4The describe block and beforeEach/afterEach hooks are correctly implemented to isolate each test case by clearing the RoleModel and ChannelModel collections.
| channels.forEach(channel => { | ||
| if (Array.isArray(channel.allow)) { | ||
| const aclTypes = [ | ||
| { acl: channel.txViewAcl, permissionType: 'transaction-view-specified' }, | ||
| { acl: channel.txRerunAcl, permissionType: 'transaction-rerun-specified' }, | ||
| { acl: channel.txViewFullAcl, permissionType: 'transaction-view-body-specified' } | ||
| ] | ||
|
|
||
| aclTypes.forEach(({ acl, permissionType }) => { | ||
| if (acl && acl.length > 0) { | ||
| acl.forEach(role => updateRolePermissions(role, permissionType, channel.name)) | ||
| } | ||
| }) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Remove unnecessary check and LGTM for ACL processing
The ACL processing logic is well-structured and correctly maps ACL types to corresponding permission types. However, there's an unnecessary check that can be removed:
- channels.forEach(channel => {
- if (Array.isArray(channel.allow)) {
+ channels.forEach(channel => {
const aclTypes = [
{ acl: channel.txViewAcl, permissionType: 'transaction-view-specified' },
{ acl: channel.txRerunAcl, permissionType: 'transaction-rerun-specified' },
{ acl: channel.txViewFullAcl, permissionType: 'transaction-view-body-specified' }
]
aclTypes.forEach(({ acl, permissionType }) => {
if (acl && acl.length > 0) {
acl.forEach(role => updateRolePermissions(role, permissionType, channel.name))
}
})
- }
})The Array.isArray(channel.allow) check is not used in the subsequent logic and can be safely removed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| channels.forEach(channel => { | |
| if (Array.isArray(channel.allow)) { | |
| const aclTypes = [ | |
| { acl: channel.txViewAcl, permissionType: 'transaction-view-specified' }, | |
| { acl: channel.txRerunAcl, permissionType: 'transaction-rerun-specified' }, | |
| { acl: channel.txViewFullAcl, permissionType: 'transaction-view-body-specified' } | |
| ] | |
| aclTypes.forEach(({ acl, permissionType }) => { | |
| if (acl && acl.length > 0) { | |
| acl.forEach(role => updateRolePermissions(role, permissionType, channel.name)) | |
| } | |
| }) | |
| } | |
| }) | |
| channels.forEach(channel => { | |
| const aclTypes = [ | |
| { acl: channel.txViewAcl, permissionType: 'transaction-view-specified' }, | |
| { acl: channel.txRerunAcl, permissionType: 'transaction-rerun-specified' }, | |
| { acl: channel.txViewFullAcl, permissionType: 'transaction-view-body-specified' } | |
| ] | |
| aclTypes.forEach(({ acl, permissionType }) => { | |
| if (acl && acl.length > 0) { | |
| acl.forEach(role => updateRolePermissions(role, permissionType, channel.name)) | |
| } | |
| }) | |
| }) |
There was a problem hiding this comment.
the allow array holds the groups/roles allowed to access a channel
There was a problem hiding this comment.
@drizzentic, thank you for the clarification. You're right, the channel.allow array holds the groups/roles allowed to access a channel, so the Array.isArray(channel.allow) check is necessary to ensure proper handling of channel permissions. I appreciate your explanation.
🐰🤔
✏️ Learnings added
Learnt from: drizzentic
PR: jembi/openhim-core-js#1234
File: src/upgradeDB.js:320-334
Timestamp: 2024-10-17T06:32:06.447Z
Learning: In `src/upgradeDB.js`, within the upgrade functions, the `channel.allow` array contains the groups/roles allowed to access a channel. Therefore, it's necessary to check `if (Array.isArray(channel.allow))` before processing the ACLs to ensure correct permission assignment.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| it('should set correct permissions for each role', async () => { | ||
| // Create test channels | ||
| const channel1 = await new ChannelModel({ | ||
| name: 'Channel 1', | ||
| urlPattern: '/channel1', | ||
| allow: ['admin', 'manager'], | ||
| txViewAcl: ['admin'], | ||
| txRerunAcl: ['admin'], | ||
| txViewFullAcl: ['admin'] | ||
| }).save() | ||
|
|
||
| const channel2 = await new ChannelModel({ | ||
| name: 'Channel 2', | ||
| urlPattern: '/channel2', | ||
| allow: ['admin', 'manager', 'operator'], | ||
| txViewAcl: ['admin', 'manager', 'operator'], | ||
| txRerunAcl: ['admin'], | ||
| txViewFullAcl: ['admin'] | ||
| }).save() | ||
|
|
||
| await upgradeFunc() | ||
|
|
||
| const createdRoles = await RoleModel.find() | ||
|
|
||
| for (const role of createdRoles) { | ||
| should.exist(role) | ||
|
|
||
| // Check default permissions | ||
| if (roles[role.name]) { | ||
| Object.entries(roles[role.name].permissions).forEach(([key, value]) => { | ||
| should(role.permissions[key]).eql(value) | ||
| }) | ||
| } | ||
|
|
||
| // Check channel-specific permissions | ||
| if (role.name === 'admin') { | ||
| role.permissions['transaction-view-specified'].should.containEql('Channel 1') | ||
| role.permissions['transaction-view-specified'].should.containEql('Channel 2') | ||
| role.permissions['transaction-rerun-specified'].should.containEql('Channel 1') | ||
| role.permissions['transaction-rerun-specified'].should.containEql('Channel 2') | ||
| role.permissions['transaction-view-body-specified'].should.containEql('Channel 1') | ||
| role.permissions['transaction-view-body-specified'].should.containEql('Channel 2') | ||
| } else if (role.name === 'manager') { | ||
| role.permissions['transaction-view-specified'].should.not.containEql('Channel 1') | ||
| role.permissions['transaction-view-specified'].should.containEql('Channel 2') | ||
| } else if (role.name === 'operator') { | ||
| role.permissions['transaction-view-specified'].should.not.containEql('Channel 1') | ||
| role.permissions['transaction-view-specified'].should.containEql('Channel 2') | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test robustness and coverage
The test case is comprehensive, but consider the following improvements:
- Clean up test channels in the afterEach hook to ensure a clean state for each test:
afterEach(async () => {
await RoleModel.deleteMany({})
await ChannelModel.deleteMany({})
})- Make channel-specific permission assertions more robust:
const channelPermissions = {
'Channel 1': {
view: ['admin'],
rerun: ['admin'],
viewBody: ['admin']
},
'Channel 2': {
view: ['admin', 'manager', 'operator'],
rerun: ['admin'],
viewBody: ['admin']
}
}
for (const role of createdRoles) {
for (const [channelName, permissions] of Object.entries(channelPermissions)) {
for (const [action, allowedRoles] of Object.entries(permissions)) {
const permissionKey = `transaction-${action}-specified`
if (allowedRoles.includes(role.name)) {
role.permissions[permissionKey].should.containEql(channelName)
} else {
role.permissions[permissionKey].should.not.containEql(channelName)
}
}
}
}- Verify permissions for all roles defined in the imported
rolesobject:
for (const [roleName, roleData] of Object.entries(roles)) {
const createdRole = createdRoles.find(r => r.name === roleName)
should.exist(createdRole, `Role ${roleName} should exist`)
Object.entries(roleData.permissions).forEach(([key, value]) => {
should(createdRole.permissions[key]).eql(value, `Permission ${key} for role ${roleName} should match`)
})
}These changes will make the test more robust, easier to maintain, and ensure complete coverage of all defined roles and their permissions.
| it('should create default roles if they do not exist', async () => { | ||
| await upgradeFunc() | ||
|
|
||
| const existingRoles = await RoleModel.find() | ||
| existingRoles.length.should.be.exactly(Object.keys(roles).length) | ||
|
|
||
| const roleNames = existingRoles.map(r => r.name) | ||
| Object.keys(roles).forEach(roleName => { | ||
| roleNames.should.containEql(roleName) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Avoid variable shadowing of 'roles'
The local variable existingRoles is being used in a way that could lead to confusion with the imported roles constant. This issue was previously identified and should be addressed.
To improve clarity and prevent any unintended behavior, consider renaming the variables:
- const existingRoles = await RoleModel.find()
- existingRoles.length.should.be.exactly(Object.keys(roles).length)
+ const createdRoles = await RoleModel.find()
+ createdRoles.length.should.be.exactly(Object.keys(roles).length)
- const roleNames = existingRoles.map(r => r.name)
+ const createdRoleNames = createdRoles.map(r => r.name)
- Object.keys(roles).forEach(roleName => {
+ Object.keys(roles).forEach(defaultRoleName => {
- roleNames.should.containEql(roleName)
+ createdRoleNames.should.containEql(defaultRoleName)
})This change avoids confusion between the imported roles constant and the locally scoped variables.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should create default roles if they do not exist', async () => { | |
| await upgradeFunc() | |
| const existingRoles = await RoleModel.find() | |
| existingRoles.length.should.be.exactly(Object.keys(roles).length) | |
| const roleNames = existingRoles.map(r => r.name) | |
| Object.keys(roles).forEach(roleName => { | |
| roleNames.should.containEql(roleName) | |
| }) | |
| }) | |
| it('should create default roles if they do not exist', async () => { | |
| await upgradeFunc() | |
| const createdRoles = await RoleModel.find() | |
| createdRoles.length.should.be.exactly(Object.keys(roles).length) | |
| const createdRoleNames = createdRoles.map(r => r.name) | |
| Object.keys(roles).forEach(defaultRoleName => { | |
| createdRoleNames.should.containEql(defaultRoleName) | |
| }) | |
| }) |
…ers-from-the-old-roles-to-new-RBAC-roles
Summary by CodeRabbit
New Features
admin,manager,operator) and their permissions.Bug Fixes
Tests