EnxDev commented on issue #37938: URL: https://github.com/apache/superset/issues/37938#issuecomment-4386599009
Hey @jon-coffey, thanks for raising this and sorry for the confusion! Let me share some context. The Admin-only restriction on these views actually predates the React migration. It's enforced in the backend via [`ADMIN_ONLY_VIEW_MENUS`](https://github.com/apache/superset/blob/master/superset/security/manager.py#L439) in `SupersetSecurityManager`, which strips the relevant PVMs (`List Users, List Roles, List Groups, UsersListView, RoleModelView,` etc.) from Alpha/Gamma roles during `superset init`. The original FAB views were already gated this way through `@has_access ` they weren't accessible to non-admins via granular permissions either. The granular permissions still exist as PVM rows in the DB, but they're effectively unassignable to non-admin roles because of that filter. What [PR #32432](https://github.com/apache/superset/pull/32432) (and the follow-up Users/Groups migrations) added was a frontend `isAdmin` guard at routes.tsx#L351 for route registration, mirroring the existing backend behavior. The new Python views still gate correctly via `@has_access` + `permission_name("read")` on class_permission_name = "security", so the backend story is consistent but you're absolutely right that the user-facing behavior is "Admin-only." To enable what you're describing (and what @rusackas suggested), we'd need changes in two places: 1. **Backend** — selectively remove only the read PVMs from ADMIN_ONLY_VIEW_MENUS / ADMIN_ONLY_PERMISSIONS, keeping all write/mutation PVMs admin-only: - Allow `can read` on `User`, `Role`, `Group`, `UsersListView`, `RoleModelView`, `UserGroupModelView` to be assignable to non-admin roles (so a "User Manager" role can list users/roles/groups). - **Keep admin-only**: `can write`, `can edit`, `can delete`, `can add`, `can update_role`, `update_roles_users`, `list_roles` on these same view-menus, plus the REST API write endpoints. Without constraint logic preventing privilege escalation (e.g. a sub-admin granting permissions they don't hold, or assigning the Admin role), write access stays gated. - Add the now-removable read PVMs to `GAMMA_EXCLUDED_PVMS` (and the alpha equivalent) so they aren't auto-granted to Alpha/Gamma on `superset ini`t — they should only land on roles where an admin explicitly assigns them. 2. **Frontend** — replace the `isAdmin` route guard in `routes.tsx#L351` with a permission check like `findPermission('can_read', 'security', user)` (or whichever granularity we land on with maintainers). Same swap in the page-level guards in UsersList/index.tsx, RolesList/index.tsx, GroupsList/index.tsx. Crucially, the page components also need to conditionally hide write actions (Add Role, Edit, Delete, Add User, etc.) based on the corresponding can_write / can_edit permissions so a read-only user manager sees the lists but no mutation buttons. Out of scope for this first pass (worth flagging in the PR description): full write access for sub-admins. That requires new constraint logic in `SupersetSecurityManager` e.g. preventing a non-admin from assigning permissions they don't themselves hold, or from granting the Admin role — which is a separate design discussion. Happy to collaborate on a PR if you want to take a stab at it I'd be glad to review. We should probably also align with maintainers e.g. @dpgaspar on whether `can_read` on security is the right granularity, or whether we want separate perms for users / roles / groups views. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
