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

ocrvs-7698 / ocrvs-7691 Stops local sys admin from creating national level staff #7996

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

euanmillar
Copy link
Collaborator

Stop sys admins from de-activating themselves and stop local sys admins. from creating national level users

Also stops sys admins from being able to delete themselves in the UI:

Screenshot 2024-11-15 at 08 56 27

A National Sys Admin will see the correct roles available to them:

Screenshot 2024-11-15 at 08 45 49 Screenshot 2024-11-15 at 08 45 26

A local system admin will not see national level roles available to them:
Screenshot 2024-11-15 at 08 44 58

Screenshot 2024-11-15 at 08 44 32

This comment has been minimized.

@euanmillar euanmillar changed the title ocrvs-7698 Stops local sys admin from creating national level staff ocrvs-7698 / ocrvs-7691 Stops local sys admin from creating national level staff Nov 15, 2024
Comment on lines +44 to +46
if (!getValidRoles({}, 'asc', 'creationDate', token, user.systemRole)) {
return h.response().code(400)
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's have the access management in gateway where we have some parts of it already

Comment on lines +23 to +46
export const sysAdminAccessMap: Map<string, string[]> = new Map([
[
'LOCAL_SYSTEM_ADMIN',
[
'FIELD_AGENT',
'LOCAL_REGISTRAR',
'LOCAL_SYSTEM_ADMIN',
'PERFORMANCE_MANAGEMENT',
'REGISTRATION_AGENT'
]
],
[
'NATIONAL_SYSTEM_ADMIN',
[
'FIELD_AGENT',
'LOCAL_REGISTRAR',
'LOCAL_SYSTEM_ADMIN',
'NATIONAL_REGISTRAR',
'NATIONAL_SYSTEM_ADMIN',
'PERFORMANCE_MANAGEMENT',
'REGISTRATION_AGENT'
]
]
])
Copy link
Member

Choose a reason for hiding this comment

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

This I think can just be a regular object literal, right?

export const ROLE_CAN_MODIFY_ROLE = {
  LOCAL_SYSTEM_ADMIN: [
    "FIELD_AGENT",
    "LOCAL_REGISTRAR",
    "LOCAL_SYSTEM_ADMIN",
    "PERFORMANCE_MANAGEMENT",
    "REGISTRATION_AGENT",
  ],
  NATIONAL_SYSTEM_ADMIN: [
    "FIELD_AGENT",
    "LOCAL_REGISTRAR",
    "LOCAL_SYSTEM_ADMIN",
    "NATIONAL_REGISTRAR",
    "NATIONAL_SYSTEM_ADMIN",
    "PERFORMANCE_MANAGEMENT",
    "REGISTRATION_AGENT",
  ],
};

@@ -57,3 +62,35 @@ export const getUserId = (authHeader: IAuthHeader): string => {
const tokenPayload = getTokenPayload(authHeader.Authorization.split(' ')[1])
return tokenPayload.sub
}

export async function getValidRoles(
Copy link
Member

Choose a reason for hiding this comment

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

This function I struggle to understand 🤔

@@ -34,6 +33,7 @@ export default async function getSystemRoles(
sortOrder = 'asc'
} = request.payload as IVerifyPayload
let criteria = {}
const token = request.headers.authorization
Copy link
Member

Choose a reason for hiding this comment

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

Let's this function as generic as possible and do the visual filtering of the list per user role in the frontend. This will be removed as part of the user scopes PR, I think.

Comment on lines +117 to +119
export const canDeactivateUser = (id: string, userDetails: UserDetails) => {
return id !== userDetails.id ? true : false
}
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/opencrvs/opencrvs-core/blob/develop/packages/gateway/src/features/user/root-resolvers.ts#L500-L511

Should we add the same condition to the backend endpoint? Or maybe it's enough just to prevent it from happening in the UI? Guess it's not that big of a deal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants