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

@@ -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)

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();

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 = {

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