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

Deleting the last admin should not be possible #3713

Closed
ScharfViktor opened this issue May 6, 2022 · 8 comments · Fixed by #4615
Closed

Deleting the last admin should not be possible #3713

ScharfViktor opened this issue May 6, 2022 · 8 comments · Fixed by #4615
Assignees
Labels
Priority:p2-high Escalation, on top of current planning, release blocker Type:Bug

Comments

@ScharfViktor
Copy link
Contributor

ocis local:
branch: v2.0.0-alpha1

Actual: as admin I can delete myself, even if I am the last admin in the system.

Expected: similar to the logic in project spaces. Admin can delete yourself if at least one admin remains in the system

@ScharfViktor ScharfViktor added Type:Bug Priority:p2-high Escalation, on top of current planning, release blocker labels May 6, 2022
@micbar micbar added Priority:p3-medium Normal priority and removed Priority:p2-high Escalation, on top of current planning, release blocker labels May 11, 2022
@jvillafanez
Copy link
Member

I don't think we can use the same solution here.

As far I know, we can't list who is admin. We can get all the roles and we can get the roles for a specific user, but we'll have to check each user one by one if we want to know who is admin.
This might work for space because we don't expect a high number of users / groups in the space, but it won't work if we have to deal with thousands of users.

A simpler alternative could be to prevent deleting yourself, so another admin would need to do that for you, which ensures there is an additional admin. It also ensures that the new admin can manage the system (he can access with his account and perform admin's operations), which is something the might not happen if you remove yourself (new admin might lose his password)
In terms of performance, it will be kept constant regardless of the number of users, and the operation is expected to be fast because we're expected to perform just a couple of grpc calls, one of them cached already.

Unless we have a use case where an admin needs to remove himself from the system, I think it's better to prevent that action in all circumstances.

@C0rby
Copy link
Contributor

C0rby commented Jun 14, 2022

@micbar, this sounds like PM needs to make a decision. Should an admin be able to delete himself or should another admin do that?

I like @jvillafanez's proposal for the benefits he already stated.

@micbar
Copy link
Contributor

micbar commented Jun 22, 2022

yes i like it too

A simpler alternative could be to prevent deleting yourself, so another admin would need to do that for you, which ensures there is an additional admin. It also ensures that the new admin can manage the system (he can access with his account and perform admin's operations), which is something the might not happen if you remove yourself (new admin might lose his password)

Good proposal. Fine from my POV

@kulmann
Copy link
Member

kulmann commented Jun 22, 2022

The issue spans a bit further: for role assignments we'd also need to make sure that the last admin can't get a role downgrade unless another user gets the admin role assigned. Opinions?

@kobergj
Copy link
Collaborator

kobergj commented Jun 22, 2022

The issue spans a bit further: for role assignments we'd also need to make sure that the last admin can't get a role downgrade unless another user gets the admin role assigned. Opinions?

Wouldn't it be sufficient to just block editing your own roles?

@jvillafanez
Copy link
Member

I'd vote for blocking the edition too. I don't know if it will possible to have an intermediate role just to manage the accounts (not admin, but maybe accountManager), but in that case it might also be possible to increase your own permissions. It will be also more difficult to handle these cases.

By blocking the edition of your role, this case is prevented. You also depend on another person to approve your role change because he'll be the one changing your role.

@pmaier1
Copy link
Contributor

pmaier1 commented Jun 22, 2022

There must always be at least one "admin" account. Downgrading an admin to another role (if another admin exists) should have a security question, IMO.

Keep in mind that roles will be configurable in the future. Actually we'd need to define the set of permissions that at least one user must have.

@michl19
Copy link
Contributor

michl19 commented Jun 27, 2022

Learning from refinement: disable both (edit own role, delet own account)

@micbar micbar added this to the 2.0.0 General Availability milestone Jul 13, 2022
@micbar micbar added GA-Blocker Priority:p2-high Escalation, on top of current planning, release blocker and removed Priority:p3-medium Normal priority labels Sep 14, 2022
@rhafer rhafer self-assigned this Sep 14, 2022
rhafer added a commit to rhafer/ocis that referenced this issue Sep 19, 2022
And admin user is no longer allows to edit/remove its own assignments.
This to prevent admins from locking themselves out.

Fixes: owncloud#3713
rhafer added a commit to rhafer/ocis that referenced this issue Sep 19, 2022
And admin user is no longer allows to edit/remove its own assignments.
This to prevent admins from locking themselves out.

Fixes: owncloud#3713
rhafer added a commit to rhafer/ocis that referenced this issue Sep 20, 2022
And admin user is no longer allowed to edit/remove its own assignments.
This to prevent admins from locking themselves out.

Fixes: owncloud#3713
rhafer added a commit to rhafer/ocis that referenced this issue Sep 20, 2022
And admin user is no longer allowed to remove its own user account.
This to prevent admins from locking themselves out.

Fixes: owncloud#3713
rhafer added a commit to rhafer/ocis that referenced this issue Sep 23, 2022
And admin user is no longer allowed to edit/remove its own assignments.
This to prevent admins from locking themselves out.

Fixes: owncloud#3713
rhafer added a commit to rhafer/ocis that referenced this issue Sep 23, 2022
And admin user is no longer allowed to remove its own user account.
This to prevent admins from locking themselves out.

Fixes: owncloud#3713
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:p2-high Escalation, on top of current planning, release blocker Type:Bug
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

9 participants