Validate that a custom role is not associated with any users before allowing deletion

We should not allow a custom role (MemberRole) to be deleted until it is not associated with any users/memberships.(require all users associated with a custom role to be disassociated from custom role before it can be deleted)

From @hsutor :

In an ideal world, we could give some kind of error that would say:

"Guest+1 cannot be deleted because [User X] is assigned."

Then give them a choice:

• Assign a different role to User X
• Ok/Acknowledge

This issue originally began as a follow-up to an MR discussion about a database constraint that was believed to delete custom roles if any of the membership records for that custom role is deleted. That was tested out and seems not to be the case. But the opposite is true: deleting a custom role removes all memberships associated with that custom role. To avoid any unintended member removals, we should add this validation/warning step.

Because custom roles are currently only destroy-able via the API, the above suggestion on assign/ok is not going to work. For now, the validation should just prevent the deletion and advice the Owner to remove all custom role users before deleting the custom role.

added devopsmanage feature featureaddition groupauthentication and authorization [DEPRECATED] sectiondev typefeature labels

assigned to @ifarkas

added to epic &8408 (closed)

added typebug label and removed typefeature label

removed featureaddition label

mentioned in merge request !94937 (merged)

this would mean a member_roles record will be deleted on the delete of its associated member record,

Testing this locally, it seems the direction is correct. Deleting member records does not delete member_roles records, only the other way around.

@hsutor, to verify that is the expected behaviour, what should happen to the members associated with a customizable role when the role gets deleted?

• 1. the members should be removed
• 2. the members should revert back the access level that was used as a template when the role was created

@ifarkas Sorry for the delay.

This is a good question - do we necessarily need to delete a user who doesn't have a role? Can we make them inactive instead?

I think customers would not like any automatic movement of users into roles (with exception of SAML group links), due to security reasons, so I'd like to think of another solution.

@hsutor @ifarkas Here is a scenario to help us think through this case:

1. User A is part of Group B as a Guest+1 user (can read code)
2. Guest+1 role is deleted from Group B (owner of Group B no longer wants this custom role)

Options for how to handle this scenario:

• User A becomes a regular Guest on Group B
• User A is removed from Group B
• We do not allow Guest+1 role to be deleted until Guest A is no long a Guest+1 user (require all users associated with a custom role to be disassociated from custom role before it can be deleted)

@jessieay @ifarkas Thanks for the example, makes it much easier for me to understand

I kind of like this option:

We do not allow Guest+1 role to be deleted until Guest A is no long a Guest+1 user (require all users associated with a custom role to be disassociated from custom role before it can be deleted)

In an ideal world, we could give some kind of error that would say:

"Guest+1 cannot be deleted because [User X] is assigned."

Then give them a choice:

• Assign a different role to User X
• Ok/Acknowledge

The list of users may may be very long, however, I supposed in an API response we can return all of the users who have the custom role, but I'm not sure how we'd display this in the UI

For MVC, we can just let them not delete it because it's associated to a user. And then we need a way for an admin/group owner to see all users assigned to a certain role.

great @hsutor - I've updated this issue description so that it reflects your suggested direction

Now that we've changed the direction here, I think that we actually need to tackle this issue first: #387769 (closed)

Right now, the only way to change the permissions of user with a custom role is the delete the custom role, which deletes their membership from the group.

If we remove the ability to delete custom roles that are associated with users right now, they will effectively become undeletable because there is no way to de-associate a user.

@hsutor @adil.farrukh are you OK with swapping this issue for #387769 (closed) for %15.9? I think that they are pretty similar in weight.

@jessieay You are welcome to swap them out. If we'd rather complete core implementation (with this issue + De-associate custom role from membership via API (#387769 - closed)) we can push [Consolidate permissions] View pipeline + View ... (#382094 - closed) to 15.10 as well as that's likely more effort. (You can update the labels either way as preferred)

@adil.farrukh that would be good. I just moved [Consolidate permissions] View pipeline + View ... (#382094 - closed) to %15.10 and prioritizing De-associate custom role from membership via API (#387769 - closed) for %15.9

mentioned in issue gitlab-org/manage/general-discussion#17589

changed milestone to %15.8

added Deliverable label

mentioned in issue gitlab-com/Product#5183 (closed)

set weight to 2

changed milestone to %15.9

mentioned in issue gitlab-org/manage/general-discussion#17597

Setting health status to on track as the milestone has just begun.

Issue participants are welcome to override this by setting the health status to another value.

changed health status to on track

changed title from Fix foreign key cascading delete on Member to MemberRole to Validate that a custom role is not associated with any users before allowing deletion

changed title from Validate that a custom role is not associated with any users before allowing deletion to Validate/warn that a custom role is not associated with any users before allowing deletion

changed the description

changed title from Validate/warn that a custom role is not associated with any users before allowing deletion to Validate that a custom role is not associated with any users before allowing deletion

Also side note to @hsutor: in its latest incarnation, this issue is now a duplicate of #385328 (closed). Should we close one of them?

marked this issue as related to #385328 (closed)

mentioned in issue #385328 (closed)

added typefeature label and removed typebug label

mentioned in merge request !104322 (merged)

changed epic to &9360 (closed)

This issue is scheduled for completion in this milestone but is not yet in development. Changing health status to 'needs attention'.

Issue participants are welcome to override this by setting the health status to another value.

changed health status to needs attention

added workflowin dev label

mentioned in merge request !111793 (merged)

This issue is scheduled for completion in this milestone but is in an early development stage. Changing health status to 'at risk'.

Issue participants are welcome to override this by setting the health status to another value.

changed health status to at risk

added workflowin review label and removed workflowin dev label

closed

added workflowproduction label and removed workflowin review label