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
87 changes: 85 additions & 2 deletions src/upgradeDB.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
import {UserModel} from './model/users'
import {VisualizerModel} from './model/visualizer'
import {PassportModel} from './model/passport'
import {RoleModel, roles} from './model/role'
import {ChannelModel} from './model/channels'

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

upgradeFuncs.push({
description: 'Create default roles with permissions and update user groups',
func() {
return new Promise(async (resolve, reject) => {
try {
// Fetch channels and get the role names with their associated channels
const channels = await ChannelModel.find()
const existingRoles = JSON.parse(JSON.stringify(roles)) // Deep clone the roles object

channels.forEach(channel => {
if (Array.isArray(channel.allow)) {
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

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.

Suggested change
if (Array.isArray(channel.allow)) {
if (channel.txViewAcl && channel.txViewAcl.length > 0) {
// existing code
}
// other ACL checks

if (channel.txViewAcl && channel.txViewAcl.length > 0) {
channel.txViewAcl.forEach(role => {
if (!existingRoles[role]) {
existingRoles[role] = { permissions: {} }

Check warning on line 314 in src/upgradeDB.js

View check run for this annotation

Codecov / codecov/patch

src/upgradeDB.js#L314

Added line #L314 was not covered by tests
}
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: {} }

Check warning on line 330 in src/upgradeDB.js

View check run for this annotation

Codecov / codecov/patch

src/upgradeDB.js#L330

Added line #L330 was not covered by tests
}
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'] = []

Check warning on line 337 in src/upgradeDB.js

View check run for this annotation

Codecov / codecov/patch

src/upgradeDB.js#L337

Added line #L337 was not covered by tests
}
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: {} }

Check warning on line 346 in src/upgradeDB.js

View check run for this annotation

Codecov / codecov/patch

src/upgradeDB.js#L346

Added line #L346 was not covered by tests
}
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'] = []

Check warning on line 353 in src/upgradeDB.js

View check run for this annotation

Codecov / codecov/patch

src/upgradeDB.js#L353

Added line #L353 was not covered by tests
}
existingRoles[role].permissions['channel-view-specified'].push(channel.name)
existingRoles[role].permissions['client-view-all'] = true
})
}
}
})
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

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


// Create or update roles
for (const [roleName, roleData] of Object.entries(existingRoles)) {
await RoleModel.findOneAndUpdate(
{ name: roleName },
{ name: roleName, permissions: roleData.permissions },
{ upsert: true, new: true }
)
logger.info(`Role ${roleName} created or updated with permissions`)
}

logger.info('Successfully updated roles')
resolve()
} catch (err) {
logger.error(`Error updating roles: ${err}`)
reject(err)

Check warning on line 376 in src/upgradeDB.js

View check run for this annotation

Codecov / codecov/patch

src/upgradeDB.js#L375-L376

Added lines #L375 - L376 were not covered by tests
}
})
}
})

if (process.env.NODE_ENV === 'test') {
exports.upgradeFuncs = upgradeFuncs
exports.dedupName = dedupName
Expand All @@ -305,8 +389,7 @@
const dbVer =
(await DbVersionModel.findOne()) ||
new DbVersionModel({version: 0, lastUpdated: new Date()})
const upgradeFuncsToRun = upgradeFuncs.slice(dbVer.version)

const upgradeFuncsToRun = upgradeFuncs.slice(dbVer.version)
for (const upgradeFunc of upgradeFuncsToRun) {
await upgradeFunc.func()
dbVer.version++
Expand Down
96 changes: 94 additions & 2 deletions test/unit/upgradeDBTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,11 @@ import {
KeystoreModel,
PassportModel,
UserModel,
VisualizerModel
VisualizerModel,
RoleModel,
ChannelModel,
roles
} from '../../src/model'

describe('Upgrade DB Tests', () => {
const originalUpgradeFuncs = [...upgradeDB.upgradeFuncs]
upgradeDB.upgradeFuncs.length = 0
Expand Down Expand Up @@ -546,4 +548,94 @@ describe('Upgrade DB Tests', () => {
passports.length.should.eql(2)
})
})

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

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

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

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)
})
Comment on lines +569 to +574
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 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.

})
Comment on lines +565 to +575
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 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.

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


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(Object.keys(roles).length)

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

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.

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

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

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')
}
}
})
Comment on lines +589 to +639
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

Enhance test robustness and coverage

The test case is comprehensive, but consider the following improvements:

  1. Clean up test channels in the afterEach hook to ensure a clean state for each test:
afterEach(async () => {
  await RoleModel.deleteMany({})
  await ChannelModel.deleteMany({})
})
  1. 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)
      }
    }
  }
}
  1. Verify permissions for all roles defined in the imported roles object:
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.

})
})