Merge pull request #13779 from opf/bugfix/50121-prevent-non-admin-fro…

…m-changing-admin-email

Prevent non admin from changing admin attributes

.rubocop.yml

@@ -239,6 +239,7 @@ RSpec/ContextWording:
   Prefixes:
     - and
     - as
+    - even
     - for
     - having
     - if

app/contracts/users/update_contract.rb

@@ -30,17 +30,31 @@ module Users
   class UpdateContract < BaseContract
     validate :user_allowed_to_update
 
-    private
-
     ##
     # Users can only be updated when
     # - the user is editing herself
-    # - the user has the global manage_user permission
     # - the user is an admin
+    # - the user has the global manage_user permission and is not editing an admin
+    def allowed_to_update?
+      editing_themself? || can_manage_user?
+    end
+
+    private
+
     def user_allowed_to_update
-      unless user == model || user.allowed_to_globally?(:manage_user)
+      unless allowed_to_update?
         errors.add :base, :error_unauthorized
       end
     end
+
+    def editing_themself?
+      user == model
+    end
+
+    # Only admins can edit other admins
+    # Only users with manage_user permission can edit other users
+    def can_manage_user?
+      user.allowed_to_globally?(:manage_user) && (user.admin? || !model.admin?)
+    end
   end
 end

app/controllers/users_controller.rb

@@ -156,7 +156,7 @@ def update
   def change_status_info
     @status_change = params[:change_action].to_sym
 
-    return render_400 unless %i(activate lock unlock).include? @status_change
+    render_400 unless %i(activate lock unlock).include? @status_change
   end
 
   def change_status

app/helpers/tabs_helper.rb

@@ -43,7 +43,7 @@ def selected_tab(tabs)
 
   # Render tabs from the ui/extensible tabs manager
   def render_extensible_tabs(key, params = {})
-    tabs = ::OpenProject::Ui::ExtensibleTabs.enabled_tabs(key).map do |tab|
+    tabs = ::OpenProject::Ui::ExtensibleTabs.enabled_tabs(key, params.reverse_merge(current_user:)).map do |tab|
       path = tab[:path].respond_to?(:call) ? instance_exec(params, &tab[:path]) : tab[:path]
       tab.dup.merge(path:)
     end

docs/system-admin-guide/users-permissions/roles-permissions/README.md

@@ -28,7 +28,7 @@ A user can have one or more roles which grant permissions on different levels.
 
 | Scope of the role                                            | Permission examples                                          | Customization options                                        |
 | ------------------------------------------------------------ | ------------------------------------------------------------ | ------------------------------------------------------------ |
-| Application-level: Full control of all aspects of the application | - Assign administration privileges to other users<br />- Create and restore backups in the web interface<br />- Create and configure an OAuth app<br />- Configure custom fields<br />- Archive projects/restore projects<br />- Configure global roles<br />- Configure project roles | Cannot be changed | 
+| Application-level: Full control of all aspects of the application | - Assign administration privileges to other users<br />- Create and restore backups in the web interface<br />- Create and configure an OAuth app<br />- Configure custom fields<br />- Archive projects/restore projects<br />- Configure global roles<br />- Configure project roles | Cannot be changed |
 
 ### Global role
 
@@ -52,7 +52,7 @@ A user can have one or more roles which grant permissions on different levels.
 
 **Non member** is the default role of users of your OpenProject instance who have not been added to a project. This only applies if the project has been set as **[public](../../../user-guide/projects/#set-a-project-to-public)** in the project settings.<br />
 
-**Note:** The *Non-member* role cannot be deleted. 
+**Note:** The *Non-member* role cannot be deleted.
 
 
 | Scope of the role                                            | Permission examples                                          | Customization options                                        |
@@ -70,7 +70,7 @@ OpenProject allows to share project information with **anonymous** users which a
 | ------------------------------------------------------------ | ------------------------------------------------------------ | ------------------------------------------------------------ |
 | Project-level: Permissions scoped to individual projects for users which are <u>not</u> logged in | - View work packages for users that are not logged in        | Assign different permissions to the role *Anonymous*         |
 
-## Customize roles with individual permissions 
+## Customize roles with individual permissions
 
 Administrators can add new roles with custom permissions or configure existing ones in *Administration* > *Users and permissions* > *Roles and permissions*.
 
@@ -105,7 +105,7 @@ Administrators can create new global roles in *Administration* > *Users and perm
 
 - **[Edit users](../users/)**
 
-	> **Note:** This allows the *Administrator* to delegate the administration of users to other people that should not have full control of the entire OpenProject installation (Administrator).  
+	> **Note:** This allows the *Administrator* to delegate the administration of users to other people that should not have full control of the entire OpenProject installation (Administrator). These users can edit attributes of any users, except administrators. This means they are able to impersonate another user by changing email address to match theirs. This is a security risk and should be considered with caution.
 
 - **[Create, edit, and delete placeholder users](../placeholder-users/)**
 
@@ -115,7 +115,7 @@ Administrators can create new global roles in *Administration* > *Users and perm
 
 To edit an existing role, click on the role name in the roles overview table. Make your changes and save the update by clicking on the *Save* button at the bottom of the overview page.
 
-To delete an existing role click on the **delete icon** next to a role in the list. 
+To delete an existing role click on the **delete icon** next to a role in the list.
 
 > **Note:**  Roles that are assigned to a user cannot be deleted.
 
@@ -125,11 +125,15 @@ To delete an existing role click on the **delete icon** next to a role in the li
 
 No, only Administrators can delete other users.
 
+### Can a user with "Edit users" global permission change administrators attributes?
+
+No, only Administrators can update other Administrators attributes like name or email. This is to prevent the possibility of a user with "Edit users" global permission impersonating an Administrator by changing the email address to match theirs.
+
 ### Can I set a default role for a user that creates a new project?
 
 You can set a [default role](../../system-settings/project-system-settings/#settings-for-new-projects) that users with this permission will have in a project they created.
 
-### Users do not see the action *Create project* in the main navigation even though they have the create project permission? 
+### Users do not see the action *Create project* in the main navigation even though they have the create project permission?
 
 This is UX bug tracked in [#50123](https://community.openproject.org/wp/50123).
 

lib/open_project/ui/extensible_tabs.rb

@@ -54,13 +54,17 @@ def add(key, **entry)
 
         private
 
+        # rubocop:disable Metrics/AbcSize
         def core_user_tabs
           [
             {
               name: 'general',
               partial: 'users/general',
               path: ->(params) { edit_user_path(params[:user], tab: :general) },
-              label: :label_general
+              label: :label_general,
+              only_if: ->(context) {
+                         Users::UpdateContract.new(context[:user], context[:current_user]).allowed_to_update?
+                       }
             },
             {
               name: 'memberships',
@@ -96,6 +100,7 @@ def core_user_tabs
             }
           ]
         end
+        # rubocop:enable Metrics/AbcSize
 
         def core_placeholder_user_tabs
           [

spec/contracts/users/update_contract_spec.rb

@@ -41,7 +41,9 @@
         firstname: user_firstname,
         lastname: user_lastname,
         login: user_login,
-        mail: user_mail
+        mail: user_mail,
+        password: nil,
+        password_confirmation: nil
       }
     end
 
@@ -50,14 +52,17 @@
 
       describe 'can lock the user' do
         before do
-          # needed for the stub
-          user.password = user.password_confirmation = nil
-
           user.status = Principal.statuses[:locked]
         end
 
         it_behaves_like 'contract is valid'
       end
+
+      describe 'cannot update an administrator' do
+        let(:user) { build_stubbed(:admin, attributes) }
+
+        it_behaves_like 'contract is invalid'
+      end
     end
 
     context "when updated user is current user" do
@@ -68,9 +73,6 @@
 
       context 'when setting status' do
         before do
-          # needed for the stub
-          user.password = user.password_confirmation = nil
-
           user.status = Principal.statuses[:locked]
         end