diff --git a/app/controllers/api/v1/users_controller.rb b/app/controllers/api/v1/users_controller.rb index 3632789df67588e33572daa9e5a77f3ad50f00ac..95b7a69e72887b5b6601fb42bc39e09e6eedbc4f 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 c6aefd2b7a30874ec05b524bfe889dfb550be20f..7d524695b8df4216b58e8a3f75f02b575cc264f5 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 ff071d219961b955489ccfa0f5cf6575904ee1e5..d37e2120b3369146ad9a7c8639304416ae6520cb 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 cb97aa817166795af406d6ccf0d081696c72b5b5..e498784b77657f6751b5942c0d79c3a420fa6191 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