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
56 changes: 55 additions & 1 deletion 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,59 @@
}
})

upgradeFuncs.push({
description: 'Create default roles with permissions and update user groups',
func: async () => {
try {
const channels = await ChannelModel.find()
const existingRoles = JSON.parse(JSON.stringify(roles)) // Deep clone the roles object

const updateRolePermissions = (role, permissionType, channelName) => {
if (!existingRoles[role]) {
existingRoles[role] = { permissions: {} }

Check warning on line 309 in src/upgradeDB.js

View check run for this annotation

Codecov / codecov/patch

src/upgradeDB.js#L309

Added line #L309 was not covered by tests
}
if (!existingRoles[role].permissions[permissionType]) {
existingRoles[role].permissions[permissionType] = []
}
existingRoles[role].permissions[permissionType].push(channelName)
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
}

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))
}
})
}
})
Comment on lines +320 to +334
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 17, 2024

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the allow array holds the groups/roles allowed to access a channel

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.


// Create or update roles
await Promise.all(Object.entries(existingRoles).map(([roleName, roleData]) =>
RoleModel.findOneAndUpdate(
{ name: roleName },
{ name: roleName, permissions: roleData.permissions },
{ upsert: true, new: true }
)
))

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

Check warning on line 348 in src/upgradeDB.js

View check run for this annotation

Codecov / codecov/patch

src/upgradeDB.js#L347-L348

Added lines #L347 - L348 were not covered by tests
}
}
})

if (process.env.NODE_ENV === 'test') {
exports.upgradeFuncs = upgradeFuncs
exports.dedupName = dedupName
Expand All @@ -306,7 +361,6 @@
(await DbVersionModel.findOne()) ||
new DbVersionModel({version: 0, lastUpdated: new Date()})
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 existingRoles = await RoleModel.find()
existingRoles.length.should.be.exactly(Object.keys(roles).length)

const adminRoles = existingRoles.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.

})
})