Skip to content
74 changes: 72 additions & 2 deletions src/services/ResourceService.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,20 @@ const prisma = require('../common/prisma').getClient()

const payloadFields = ['id', 'challengeId', 'memberId', 'memberHandle', 'roleId', 'phaseChangeNotifications', 'created', 'createdBy', 'updated', 'updatedBy']

// Restricted roles that cannot be combined with submitter role
const RESTRICTED_ROLE_NAMES = [
'manager',
'copilot',
'reviewer',
'iterative reviewer',
'screener',
'checkpoint screener',
'checkpoint reviewer',
'approver'
]

let copilotResourceRoleIdsCache
let restrictedRoleIdsCache

async function getCopilotResourceRoleIds () {
if (copilotResourceRoleIdsCache) {
Expand All @@ -34,11 +47,42 @@ async function getCopilotResourceRoleIds () {
return copilotResourceRoleIdsCache
}

/**
* Get resource role IDs that are restricted from being combined with submitter role.
* These include: Manager, Copilot, Reviewer, Iterative Reviewer, Screener,
* Checkpoint Screener, Checkpoint Reviewer, and Approver.
* @returns {Promise<Array<String>>} Array of restricted role IDs
*/
async function getRestrictedRoleIds () {
if (restrictedRoleIdsCache) {
Copy link

Choose a reason for hiding this comment

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

[⚠️ maintainability]
The caching mechanism for restrictedRoleIdsCache does not account for changes in the database. If roles are updated externally, the cache might become stale. Consider implementing a cache invalidation strategy or a mechanism to refresh the cache periodically.

return restrictedRoleIdsCache
}
const roles = await prisma.resourceRole.findMany({
where: {
nameLower: {
in: RESTRICTED_ROLE_NAMES
}
},
select: {
id: true,
nameLower: true
}
})
restrictedRoleIdsCache = roles.map(role => role.id)
return restrictedRoleIdsCache
}

/**
* Check whether the user can access resources
* @param {Array} resources resources of current user for specified challenge id
*/
async function checkAccess (currentUserResources) {
const copilotRoleIds = await getCopilotResourceRoleIds()
const hasCopilotRole = _.some(currentUserResources, r => copilotRoleIds.includes(r.roleId))
Copy link

Choose a reason for hiding this comment

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

[💡 performance]
The use of _.some to check for copilot roles could be replaced with Array.prototype.some for better readability and performance, as native methods are generally faster.

if (hasCopilotRole) {
return
}

const list = await prisma.resourceRole.findMany({})
const fullAccessRoles = []
_.each(list, e => {
Expand Down Expand Up @@ -291,6 +335,12 @@ async function init (currentUser, challengeId, resource, isCreated) {
const { memberId, email } = memberInfoFromDb
handle = memberInfoFromDb.handle
const userResources = allResources.filter((r) => _.toLower(r.memberHandle) === _.toLower(handle))

let restrictedRoleIds
if (isCreated) {
restrictedRoleIds = await getRestrictedRoleIds()
}

// Retrieve the constraint - Allowed Registrants
if (isCreated && resource.roleId === config.SUBMITTER_RESOURCE_ROLE_ID) {
const allowedRegistrants = _.get(challenge, 'constraints.allowedRegistrants')
Expand All @@ -307,12 +357,20 @@ async function init (currentUser, challengeId, resource, isCreated) {
`User ${handle} is not allowed to register.`
)
}
if (!_.get(challenge, 'task.isTask', false) && (_.toLower(challenge.createdBy) === _.toLower(handle) ||
_.some(userResources, r => r.roleId === config.REVIEWER_RESOURCE_ROLE_ID || r.roleId === config.ITERATIVE_REVIEWER_RESOURCE_ROLE_ID))) {
// Prevent challenge creator from registering as submitter (for non-tasks)
if (!_.get(challenge, 'task.isTask', false) && _.toLower(challenge.createdBy) === _.toLower(memberId)) {
throw new errors.BadRequestError(
`User ${handle} is not allowed to register.`
)
}
// Check if user already has a restricted role (Manager, Copilot, Reviewer, etc.)
const existingRestrictedRole = _.find(userResources, r => restrictedRoleIds.includes(r.roleId))
Copy link

Choose a reason for hiding this comment

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

[⚠️ maintainability]
The error message construction using RESTRICTED_ROLE_NAMES assumes that the list will not change. If roles are added or removed, the message might become inaccurate. Consider dynamically constructing the message based on the actual roles retrieved from the database.

if (existingRestrictedRole) {
const roleNamesList = RESTRICTED_ROLE_NAMES.slice(0, -1).join(', ') + ', or ' + RESTRICTED_ROLE_NAMES.slice(-1)
throw new errors.BadRequestError(
`User ${handle} is already assigned a restricted role (${roleNamesList}) and cannot be registered as a submitter.`
)
}
}

// Prevent from creating more than 1 submitter resources on tasks
Expand Down Expand Up @@ -344,6 +402,18 @@ async function init (currentUser, challengeId, resource, isCreated) {
// ensure resource role existed
const resourceRole = await getResourceRole(resource.roleId, isCreated)

// Check if user is trying to assign a restricted role and already has submitter role
if (isCreated) {
if (restrictedRoleIds.includes(resource.roleId)) {
const existingSubmitterRole = _.find(userResources, r => r.roleId === config.SUBMITTER_RESOURCE_ROLE_ID)
Copy link

Choose a reason for hiding this comment

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

[⚠️ maintainability]
The logic for checking if a user is trying to assign a restricted role while already having a submitter role is duplicated in multiple places. Consider refactoring this logic into a separate function to improve maintainability and reduce code duplication.

if (existingSubmitterRole) {
throw new errors.BadRequestError(
`User ${handle} is already registered as a submitter and cannot be assigned a ${resourceRole.name} role.`
)
}
}
}

// Verify the member has agreed to the challenge terms
if (isCreated) {
await helper.checkAgreedTerms(memberId, _.filter(_.get(challenge, 'terms', []), t => t.roleId === resourceRole.id))
Expand Down
34 changes: 34 additions & 0 deletions test/unit/createResource.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const { v4: uuid } = require('uuid')
const service = require('../../src/services/ResourceService')
const ResourceRolePhaseDependencyService = require('../../src/services/ResourceRolePhaseDependencyService')
const prisma = require('../../src/common/prisma').getClient()
const helper = require('../../src/common/helper')
const ResourceRoleService = require('../../src/services/ResourceRoleService')
const { requestBody, user } = require('../common/testData')
const { assertValidationError, assertError, assertResource, getRoleIds, clearDependencies } = require('../common/testHelper')
Expand Down Expand Up @@ -264,6 +265,39 @@ module.exports = describe('Create resource', () => {
await assertResource(ret.id, ret)
})

it('copilot can manage resources without full access flags', async () => {
const originalRole = await helper.getById('ResourceRole', copilotRoleId)
Copy link

Choose a reason for hiding this comment

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

[⚠️ correctness]
Consider adding error handling for the helper.getById call. If this call fails or returns an unexpected result, it could cause the test to behave unpredictably.

await ResourceRoleService.updateResourceRole(user.admin, copilotRoleId, {
name: originalRole.name,
fullReadAccess: false,
fullWriteAccess: false,
isActive: originalRole.isActive,
selfObtainable: originalRole.selfObtainable
})

const entity = resources.createBody('diazz', reviewerRoleId, challengeId2)
let createdResource
try {
createdResource = await service.createResource(user.phead, entity)
should.equal(createdResource.roleId, entity.roleId)
should.equal(createdResource.memberHandle.toLowerCase(), entity.memberHandle.toLowerCase())
await assertResource(createdResource.id, createdResource)
} finally {
if (createdResource && createdResource.id) {
Copy link

Choose a reason for hiding this comment

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

[⚠️ maintainability]
The cleanup logic in the finally block is crucial for maintaining test isolation. Ensure that any errors during the prisma.resource.deleteMany call are logged or handled to avoid silent failures.

await prisma.resource.deleteMany({
where: { id: createdResource.id }
})
}
await ResourceRoleService.updateResourceRole(user.admin, copilotRoleId, {
name: originalRole.name,
fullReadAccess: originalRole.fullReadAccess,
fullWriteAccess: originalRole.fullWriteAccess,
isActive: originalRole.isActive,
selfObtainable: originalRole.selfObtainable
})
}
})

it('create resource for user ghostar 1', async () => {
const entity = resources.createBody('ghostar', reviewerRoleId, challengeId2)
const ret = await service.createResource(user.m2m, entity)
Expand Down
Loading