roadmap icon indicating copy to clipboard operation
roadmap copied to clipboard

admin_grant_permissions and admin_update_permissions have conflicting ideas about assignable permissions

Open nicolasfranck opened this issue 3 years ago • 2 comments

Please complete the following fields as applicable:

What version of the DMPRoadmap code are you running? (e.g. v2.2.0)

3.1.1

Expected behaviour:

You should be able to assign one of the listed permissions to user as shown on the modal "Editing privileges for " (See also UsersController#admin_grant_permissions)

Actual behaviour:

While the modal shows all permissions (when you are a super admin), the controller UsersController#admin_update_permissions only adds those permissions that the current user also has.

e.g. I am super admin, but "can use api" is not checked. Consequentially I cannot give someone else this permission.

In short:

  • UsersController#admin_grant_permissions loads ALL permissions when you are super admin
  • UsersController#admin_update_permissions only takes permissions into account that the current user also has
  • This is confusing.

Steps to reproduce:

  • make sure your user has one of the super admin permissions, and login.
  • make sure that you do NOT have Perm.use_api
  • go to http://localhost:3000/org/admin/users/admin_index and click on an "edit" button in the column "Current privileges"
  • If that user does not have have "can use api" yet, try to assign it, and click save
  • You get a "User was saved" message
  • Reload page, and try to bring that modal up: the permission "can use api" was never assigned.

Solution?

Not sure. Use Perm.all instead of current_user.perms when that user is super_admin? That would also mean that you can gain any permission once you have one of the three super admin permissions..

nicolasfranck avatar May 31 '22 15:05 nicolasfranck

Thanks for creating this issue @nicolasfranck. The original codebase had only 3 levels of permissions: super admin, org admin and regular user. At some point later on, it was decided that we should break those down into more granular permissions. In practice though, I wonder if we need the granular permissions at all.

If you look at the User model and it's helper methods like can_super_admin? it returns true if ANY of the 3 'super admin' permissions are true. I wonder if there is a good reason why a 'super' admin wouldn't have permission to do everything all the time?

@DMPRoadmap/devs do any of your installations have a use case where a super admin would not have all 3 of those permissions?

briri avatar Jun 02 '22 15:06 briri

For the DMP Assistant, the super admin has all permissions.

pengyin-shan avatar Jun 02 '22 15:06 pengyin-shan