Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cu 86c0bkuxc add a mechanism to migrate users from the old roles to new rbac roles #1234

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
1 change: 1 addition & 0 deletions src/model/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,4 @@ export * from './users'
export * from './visualizer'
export * from './metrics'
export * from './passport'
export * from './role'
2 changes: 1 addition & 1 deletion src/model/role.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ const RoleSchema = new Schema({
export const RoleModelAPI = connectionAPI.model('Role', RoleSchema)
export const RoleModel = connectionDefault.model('Role', RoleSchema)

const roles = {
export const roles = {
admin: {
name: 'admin',
permissions: {
Expand Down
41 changes: 41 additions & 0 deletions src/upgradeDB.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import {UserModel} from './model/users'
import {VisualizerModel} from './model/visualizer'
import {PassportModel} from './model/passport'
import {RoleModel, roles} from './model/role'

function dedupName(name, names, num) {
let newName
Expand Down Expand Up @@ -295,6 +296,46 @@
}
})

upgradeFuncs.push({
description: 'Create default roles with permissions and update user groups',
func() {
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';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

txViewAcl: [String],
txViewFullAcl: [String],
txRerunAcl: [String],

We should map these to a new role with the same name and the new channel permissions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if ((user.groups && user.groups.includes('admin')) || user.superUser) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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)

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);

Check warning on line 333 in src/upgradeDB.js

View check run for this annotation

Codecov / codecov/patch

src/upgradeDB.js#L332-L333

Added lines #L332 - L333 were not covered by tests
}
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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)

}
});

if (process.env.NODE_ENV === 'test') {
exports.upgradeFuncs = upgradeFuncs
exports.dedupName = dedupName
Expand Down
154 changes: 153 additions & 1 deletion test/unit/upgradeDBTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ import {
KeystoreModel,
PassportModel,
UserModel,
VisualizerModel
VisualizerModel,
RoleModel
} from '../../src/model'

describe('Upgrade DB Tests', () => {
Expand Down Expand Up @@ -546,4 +547,155 @@ describe('Upgrade DB Tests', () => {
passports.length.should.eql(2)
})
})

describe(`updateFunction4 - Create default roles with permissions and update user groups`, () => {
const upgradeFunc = originalUpgradeFuncs[4].func

beforeEach(async () => {
await RoleModel.deleteMany({})
await UserModel.deleteMany({})
})

afterEach(async () => {
await RoleModel.deleteMany({})
await UserModel.deleteMany({})
})

it('should create default roles if they do not exist', async () => {
await upgradeFunc()

const roles = await RoleModel.find()
roles.length.should.be.exactly(3)

const roleNames = roles.map(r => r.name)
roleNames.should.containEql('manager')
roleNames.should.containEql('admin')
roleNames.should.containEql('operator')
})

it('should not create duplicate roles if they already exist', async () => {
await new RoleModel({name: 'admin', permissions: {}}).save()

await upgradeFunc()

const roles = await RoleModel.find()
roles.length.should.be.exactly(3)

const adminRoles = roles.filter(r => r.name === 'admin')
adminRoles.length.should.be.exactly(1)
})

it('should set correct permissions for each role', async () => {
await upgradeFunc()

const managerRole = await RoleModel.findOne({name: 'manager'})
const adminRole = await RoleModel.findOne({name: 'admin'})
const operatorRole = await RoleModel.findOne({name: 'operator'})

// Helper function to check permissions
const checkPermissions = (role, expectedPermissions) => {
console.log(`Checking permissions for role: ${role.name}`)
Object.entries(expectedPermissions).forEach(([key, value]) => {
should(role.permissions[key]).equal(value)
})
}

// Admin role permissions
checkPermissions(adminRole, {
'channel-view-all': true,
'channel-manage-all': true,
'client-view-all': true,
'client-manage-all': true,
'transaction-view-all': true,
'transaction-view-body-all': true,
'transaction-rerun-all': true,
'user-view': true,
'user-manage': true,
'visualizer-manage': true,
'visualizer-view': true
// Add other admin permissions as needed
})

// Manager role permissions
checkPermissions(managerRole, {
'channel-view-all': true,
'channel-manage-all': true,
'client-view-all': true,
'client-manage-all': true,
'transaction-view-all': true,
'transaction-view-body-all': true,
'transaction-rerun-all': true,
'user-view': true,
'visualizer-manage': true,
'visualizer-view': true
// Add other manager permissions as needed
})

// Operator role permissions
checkPermissions(operatorRole, {
'channel-view-all': true,
'transaction-view-all': true,
'transaction-view-body-all': true,
'transaction-rerun-all': true
// Add other operator permissions as needed
})

// Check that operator doesn't have certain permissions
should(operatorRole.permissions['user-manage']).be.false()
should(operatorRole.permissions['client-manage-all']).be.false()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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()

})

it('should update user groups to admin for superUsers', async () => {
const superUser = new UserModel({
email: '[email protected]',
groups: ['admin'],
firstname: 'Super',
surname: 'User'
})
await superUser.save()

await upgradeFunc()

const updatedUser = await UserModel.findOne({email: '[email protected]'})
updatedUser.groups.should.eql(['admin'])
})

it('should handle mixed user types correctly', async () => {
const users = [
new UserModel({
email: '[email protected]',
groups: ['user'],
firstname: 'Regular',
surname: 'User'
}),
new UserModel({
email: '[email protected]',
groups: ['user', 'admin'],
firstname: 'Admin',
surname: 'User'
}),
new UserModel({
email: '[email protected]',
groups: ['admin'],
firstname: 'Super',
surname: 'User'
}),
new UserModel({
email: '[email protected]',
groups: ['operator'],
firstname: 'Another',
surname: 'User'
})
]
await Promise.all(users.map(user => user.save()))

await upgradeFunc()

const updatedUsers = await UserModel.find().sort('email')
updatedUsers[0].groups.should.eql(['admin']) // [email protected]
updatedUsers[1].groups.should.eql(['manager']) // [email protected]
updatedUsers[2].groups.should.eql(['manager']) // [email protected]
updatedUsers[3].groups.should.eql(['admin']) // [email protected]
})
})
})