Allow super admin to reset passwords for community admin users - Alternate method #905
Conversation
| @@ -34,11 +34,11 @@ def create | |||
| end | |||
|
|
|||
| def show | |||
| @user = authorize community.users.find(params[:id]) | |||
| @user = authorize User.find(params[:id]) | |||
There was a problem hiding this comment.
one thing that doing the find off a Community's users association is that it was implicitly a permissions check too, because community is getting pulled from the current_user here
If you remember from our discussion, the reason we have to remove it here is that a super admin doesn't belong to a single community, so if we remove this implicit community check, we'd probably have to add an explicit one in UserPolicy
There was a problem hiding this comment.
Oh sorry, I wasn't clear what the consequence of this is. I believe without this implicit community check, an admin of any community can access these routes for other communities.
There was a problem hiding this comment.
Yep, you're right! Added logic and tests to confirm the admin is within the same community.
| @@ -1,7 +1,7 @@ | |||
| # For details on the DSL available within this file, see http://guides.rubyonrails.org/routing.html | |||
| Rails.application.routes.draw do | |||
| scope '(:locale)', locale: /#{I18n.available_locales.join("|")}/ do | |||
| scope '/member', module: 'dashboard', constraints: RoleRoutingConstraint.new { |user| !user.super_admin } do | |||
| scope '/member', module: 'dashboard', constraints: RoleRoutingConstraint.new do | |||
There was a problem hiding this comment.
Just want to call out that this change makes the assumption that a super_admin should be able to access every page, which seems reasonable to me but maybe was intentionally restricted in the past for a reason we're not aware of.
rails/app/policies/user_policy.rb
Outdated
| current_user.super_admin? || (current_user.admin? && (current_user.community_id == user.community_id)) | ||
| end | ||
|
|
||
| def update? | ||
| edit? | ||
| end | ||
|
|
||
| def show? | ||
| current_user.admin? | ||
| edit? |
There was a problem hiding this comment.
Hm, that's surprising that a user couldn't call show on themselves though 🤔
DuldR
force-pushed
the
802-super-admin-password-same-flow
branch
from
802b73f to
1998a75
Compare
|
Closing this PR in favor of the implementation in #904 |
Fixes #802
Related PR: #904
For this functionality, I've changed the authorization logic to allow for a super admin to touch the user edit, show and update methods. Instead of creating it's own super_admin route, this hooks into the existing route available.
I think this methodology is a bit cleaner as the
super_adminhits the same edit page as an admin and user. So if we ever update the logic anywhere in the pipeline, we'll only need to make a single change instead of keeping two flows in sync.Super Admin Panel
I ensured that a user cannot update any other user with this implementation. An unauthorized error is raised on any attempt to go to another user's page.
