From 11bc3731bacfe5c20ffb17120e7babea129e1afe Mon Sep 17 00:00:00 2001
From: Samuel Couillard <43917914+scouillard@users.noreply.github.com>
Date: Mon, 10 Jul 2023 14:17:47 -0400
Subject: [PATCH] Add restriction on users trying to change their own role
 (#5318)

* Add restriction on users trying to change their own role

* Add check for role_id in params

* rubo
---
 app/controllers/api/v1/users_controller.rb    |  7 ++++---
 .../users/user/forms/UpdateUserForm.jsx       |  5 ++---
 config/application.rb                         |  3 ++-
 spec/controllers/users_controller_spec.rb     | 21 +++++++++++++++++--
 4 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/app/controllers/api/v1/users_controller.rb b/app/controllers/api/v1/users_controller.rb
index 3632789d..95b7a69e 100644
--- a/app/controllers/api/v1/users_controller.rb
+++ b/app/controllers/api/v1/users_controller.rb
@@ -98,9 +98,10 @@ module Api
       # Updates the values of a user
       def update
         user = User.find(params[:id])
-        # user is updating themselves
-        if current_user.id == params[:id] && !PermissionsChecker.new(permission_names: 'ManageUsers', current_user:, current_provider:).call
-          params[:user].delete(:role_id)
+
+        # User can't change their own role
+        if params[:user][:role_id].present? && current_user == user && params[:user][:role_id] != user.role_id
+          return render_error errors: Rails.configuration.custom_error_msgs[:unauthorized], status: :forbidden
         end
 
         if user.update(update_user_params)
diff --git a/app/javascript/components/users/user/forms/UpdateUserForm.jsx b/app/javascript/components/users/user/forms/UpdateUserForm.jsx
index c6aefd2b..7d524695 100644
--- a/app/javascript/components/users/user/forms/UpdateUserForm.jsx
+++ b/app/javascript/components/users/user/forms/UpdateUserForm.jsx
@@ -36,9 +36,8 @@ export default function UpdateUserForm({ user }) {
   const { t } = useTranslation();
   const currentUser = useAuth();
 
-  // Remove the display of role input field if the user is a super admin trying to update their own role
-  const isSuperAdminEditOwnRole = user === currentUser && currentUser.isSuperAdmin;
-  const canUpdateRole = PermissionChecker.hasManageUsers(currentUser) && !isSuperAdminEditOwnRole;
+  // User with ManageUsers permission can update any user except themselves
+  const canUpdateRole = PermissionChecker.hasManageUsers(currentUser) && currentUser.id !== user.id;
 
   const { data: roles } = useRoles({ enabled: canUpdateRole });
   const { data: locales } = useLocales();
diff --git a/config/application.rb b/config/application.rb
index ff071d21..d37e2120 100644
--- a/config/application.rb
+++ b/config/application.rb
@@ -51,7 +51,8 @@ module Greenlight
       pending_user: 'PendingUser',
       banned_user: 'BannedUser',
       unverified_user: 'UnverifiedUser',
-      external_signup_error: 'SignupError'
+      external_signup_error: 'SignupError',
+      unauthorized: 'Unauthorized'
     }
 
     config.uploads = {
diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb
index cb97aa81..e498784b 100644
--- a/spec/controllers/users_controller_spec.rb
+++ b/spec/controllers/users_controller_spec.rb
@@ -353,15 +353,32 @@ RSpec.describe Api::V1::UsersController, type: :controller do
       expect(user.role_id).not_to eq(updated_params[:role_id])
     end
 
-    it 'allows a user with ManageUser permissions to edit their own role' do
+    it 'allows a user to change their own name' do
+      updated_params = {
+        name: 'New Awesome Name'
+      }
+
+      patch :update, params: { id: user.id, user: updated_params }
+
+      user.reload
+
+      expect(user.name).to eq(updated_params[:name])
+    end
+
+    it 'doesnt allow a user with ManageUser permissions to edit their own role' do
       sign_in_user(user_with_manage_users_permission)
+
+      old_role_id = user_with_manage_users_permission.role_id
       updated_params = {
         role_id: create(:role, name: 'New Role').id
       }
+
       patch :update, params: { id: user_with_manage_users_permission.id, user: updated_params }
 
       user_with_manage_users_permission.reload
-      expect(user_with_manage_users_permission.role_id).to eq(updated_params[:role_id])
+      expect(response).to have_http_status(:forbidden)
+      expect(user_with_manage_users_permission.role_id).to eq(old_role_id)
+      expect(user_with_manage_users_permission.role_id).not_to eq(updated_params[:role_id])
     end
 
     it 'allows a user with ManageUser permissions to edit another users role' do
-- 
GitLab