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

Admins are now forbidden from promoting users to Super Admin. #440

Open
wants to merge 24 commits into
base: staging
Choose a base branch
from

Conversation

JohnServo24
Copy link
Contributor

@JohnServo24 JohnServo24 commented May 18, 2023

  • GET /api/groups will still return the Super Admin group even if a user doesn't have the promote_to_super_admin capability.
  • From my understanding, if a user has the read_groups capability, they should be able to read all of the groups. Please correct me if I'm wrong here.
  • This is why the filtering of the groups is done on the frontend, mainly on AddToGroupsForm.js. This component is used to add/edit the groups of a user.
    • If the user has promote_to_super_admin, show the Super Admin checkbox.
    • Otherwise, do not show the Super Admin checkbox.
  • However, if a user has the read_groups capability, but they don't have the promote_to_super_admin capability, the CMS will forbid them from adding themselves or other users to the Super Admin group.

@JohnServo24 JohnServo24 changed the title Admins are now forbidden to promote users to Super Admin Admins are now forbidden from promoting users to Super Admin. May 18, 2023
@JohnServo24 JohnServo24 self-assigned this May 18, 2023
@@ -8,7 +8,11 @@ export default function AddToGroupsForm({ groups }) {
const { values, errors, touched } = useFormikContext();
const { capabilities } = useContext(CacheContext);

const systemSupplied = groups.filter((group) => group.isSystemSupplied);
const systemSupplied = groups.filter((group) => {
if (group.id === SUPER_ADMIN_ID && !capabilities.includes(promoteToSuperAdmin)) return;
Copy link
Member

Choose a reason for hiding this comment

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

We do not check group membership. We do not deal with groups. The concept of groups is an illusion. It does not exist. We only deal with capabilities.

@@ -16,21 +16,34 @@ export default function AddToGroupsForm({ groups }) {
<h4 className="mb-4">Groups</h4>
<h5>System supplied</h5>
<div className="groups__container">
{capabilities.includes(promoteToSuperAdmin) && <div className="groups__item" >
Copy link
Member

Choose a reason for hiding this comment

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

I'm not comfortable giving 'Super Admin' special treatment.

I suggest we use the parameterized capabilities, so we have something like:

promote_to(super_admin)
promote_to(admin)
promote_to(registered_user)
etc.

Copy link
Contributor Author

@JohnServo24 JohnServo24 May 31, 2023

Choose a reason for hiding this comment

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

The promoteToSuperAdmin variable is somewhat similar to that. It's a variable that can be found from the constants folder

It is equivalent to this: const promoteToSuperAdmin = '${assignToGroup}(${SUPER_ADMIN_ID})';

Where assignToGroup is the assign_to_group capability and the SUPER_ADMIN_ID is the id # of the Super Admin group, therefore it is equal to assign_to_group(1)

Should I specifically change this to capabilities.includes('promote_to(super_admin)'), where promote_to(super_admin) is a string? (or maybe capabilities.includes('assign_to_group(1)')

Copy link
Member

Choose a reason for hiding this comment

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

🤔

@angeli-mercedes angeli-mercedes added staging PR to Staging security Security Fix labels Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Security Fix staging PR to Staging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants