Merge branch '24605-allow-admins-to-disable-users-ability-to-change-profile-name' into 'master'

Resolve "Allow admins to disable users ability to change profile name"

See merge request gitlab-org/gitlab!21987

app/helpers/application_settings_helper.rb

@@ -283,6 +283,7 @@ module ApplicationSettingsHelper
       :unique_ips_limit_enabled,
       :unique_ips_limit_per_user,
       :unique_ips_limit_time_window,
+      :updating_name_disabled_for_users,
       :usage_ping_enabled,
       :instance_statistics_visibility_private,
       :user_default_external,

app/policies/user_policy.rb

@@ -13,6 +13,11 @@ class UserPolicy < BasePolicy
   desc "The user is blocked"
   condition(:blocked_user, scope: :subject, score: 0) { @subject.blocked? }
 
+  condition(:updating_name_disabled_for_users) do
+    ::Gitlab::CurrentSettings.current_application_settings
+      .updating_name_disabled_for_users
+  end
+
   rule { ~restricted_public_level }.enable :read_user
   rule { ~anonymous }.enable :read_user
 
@@ -22,6 +27,8 @@ class UserPolicy < BasePolicy
     enable :update_user_status
   end
 
+  rule { can?(:update_user) & ( admin | ~updating_name_disabled_for_users ) }.enable :update_name
+
   rule { default }.enable :read_user_profile
   rule { (private_profile | blocked_user) & ~(user_is_self | admin) }.prevent :read_user_profile
 end

app/services/users/update_service.rb

@@ -17,6 +17,8 @@ module Users
       yield(@user) if block_given?
 
       user_exists = @user.persisted?
+
+      discard_read_only_attributes
       assign_attributes
       assign_identity
 
@@ -50,13 +52,28 @@ module Users
       success
     end
 
-    def assign_attributes
+    def discard_read_only_attributes
+      discard_synced_attributes
+      discard_name unless name_updatable?
+    end
+
+    def discard_synced_attributes
       if (metadata = @user.user_synced_attributes_metadata)
         read_only = metadata.read_only_attributes
 
         params.reject! { |key, _| read_only.include?(key.to_sym) }
       end
+    end
 
+    def discard_name
+      params.delete(:name)
+    end
+
+    def name_updatable?
+      can?(current_user, :update_name, @user)
+    end
+
+    def assign_attributes
       @user.assign_attributes(params.except(*identity_attributes)) unless params.empty?
     end
 

app/views/admin/application_settings/_account_and_limit.html.haml

@@ -51,6 +51,12 @@
         = f.check_box :user_show_add_ssh_key_message, class: 'form-check-input'
         = f.label :user_show_add_ssh_key_message, class: 'form-check-label' do
           = _("Inform users without uploaded SSH keys that they can't push over SSH until one is added")
+    .form-group
+      = f.label :updating_name_disabled_for_users, _('User restrictions'), class: 'label-bold'
+      .form-check
+        = f.check_box :updating_name_disabled_for_users, class: 'form-check-input'
+        = f.label :updating_name_disabled_for_users, class: 'form-check-label' do
+          = _("Prevent users from changing their profile name")
 
     = render_if_exists 'admin/application_settings/availability_on_namespace_setting', form: f
 

app/views/profiles/_name.html.haml

+- if user.read_only_attribute?(:name)
+  = form.text_field :name, required: true, readonly: true, wrapper: { class: 'col-md-9 qa-full-name rspec-full-name' },
+  help: s_("Profiles|Your name was automatically set based on your %{provider_label} account, so people you know can recognize you") % { provider_label: attribute_provider_label(:name) }
+- elsif can?(current_user, :update_name, user)
+  = form.text_field :name, label: s_('Profiles|Full name'), required: true, title: s_("Profiles|Using emojis in names seems fun, but please try to set a status message instead"), wrapper: { class: 'col-md-9 qa-full-name rspec-full-name' }, help: s_("Profiles|Enter your name, so people you know can recognize you")
+- else
+  = form.text_field :name, required: true, readonly: true, wrapper: { class: 'col-md-9 qa-full-name rspec-full-name' },
+  help: s_("Profiles|The ability to update your name has been disabled by your administrator.")

app/views/profiles/show.html.haml

@@ -88,11 +88,7 @@
           = s_("Profiles|Some options are unavailable for LDAP accounts")
     .col-lg-8
       .row
-        - if @user.read_only_attribute?(:name)
-          = f.text_field :name, required: true, readonly: true, wrapper: { class: 'col-md-9 qa-full-name rspec-full-name' },
-          help: s_("Profiles|Your name was automatically set based on your %{provider_label} account, so people you know can recognize you") % { provider_label: attribute_provider_label(:name) }
-        - else
-          = f.text_field :name, label: s_('Profiles|Full name'), required: true, title: s_("Profiles|Using emojis in names seems fun, but please try to set a status message instead"), wrapper: { class: 'col-md-9 qa-full-name rspec-full-name' }, help: s_("Profiles|Enter your name, so people you know can recognize you")
+        = render 'profiles/name', form: f, user: @user
         = f.text_field :id, readonly: true, label: s_('Profiles|User ID'), wrapper: { class: 'col-md-3' }
       = f.select :role, ::User.roles.keys.map { |role| [role.titleize, role] }, { prompt: _('Select your role') }, required: true, class: 'input-md'
 

changelogs/unreleased/24605-allow-admins-to-disable-users-ability-to-change-profile-name.yml

+---
+title: Allow admins to disable users ability to change profile name
+merge_request: 21987
+author:
+type: added

db/migrate/20191218084115_add_updating_name_disabled_for_users_to_application_settings.rb

+# frozen_string_literal: true
+
+class AddUpdatingNameDisabledForUsersToApplicationSettings < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default(:application_settings, :updating_name_disabled_for_users,
+                            :boolean,
+                            default: false,
+                            allow_null: false)
+  end
+
+  def down
+    remove_column(:application_settings, :updating_name_disabled_for_users)
+  end
+end

db/schema.rb

@@ -364,6 +364,7 @@ ActiveRecord::Schema.define(version: 2020_01_08_233040) do
     t.string "encrypted_slack_app_secret_iv", limit: 255
     t.text "encrypted_slack_app_verification_token"
     t.string "encrypted_slack_app_verification_token_iv", limit: 255
+    t.boolean "updating_name_disabled_for_users", default: false, null: false
     t.index ["custom_project_templates_group_id"], name: "index_application_settings_on_custom_project_templates_group_id"
     t.index ["file_template_project_id"], name: "index_application_settings_on_file_template_project_id"
     t.index ["instance_administration_project_id"], name: "index_applicationsettings_on_instance_administration_project_id"

doc/user/admin_area/settings/account_and_limit_settings.md

@@ -116,3 +116,17 @@ Once a lifetime for personal access tokens is set, GitLab will:
 - After three hours, revoke old tokens with no expiration date or with a lifetime longer than the
   allowed lifetime. Three hours is given to allow administrators to change the allowed lifetime,
   or remove it, before revocation takes place.
+
+## Disabling user profile name changes **(CORE ONLY)**
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/24605) in GitLab 12.7.
+
+To maintain integrity of user details in [Audit Events](../../../administration/audit_events.md), GitLab administrators can choose to disable a user's ability to change their profile name.
+
+To do this:
+
+1. Navigate to **Admin Area > Settings > General**, then expand **Account and Limit**.
+1. Check the **Prevent users from changing their profile name** checkbox.
+
+NOTE: **Note:**
+When this ability is disabled, GitLab administrators will still be able to update the name of any user in their instance via the [Admin UI](../index.md#administering-users) or the [API](../../../api/users.md#user-modification)

lib/api/settings.rb

@@ -142,6 +142,7 @@ module API
         requires :sourcegraph_url, type: String, desc: 'The configured Sourcegraph instance URL'
       end
       optional :terminal_max_session_time, type: Integer, desc: 'Maximum time for web terminal websocket connection (in seconds). Set to 0 for unlimited time.'
+      optional :updating_name_disabled_for_users, type: Boolean, desc: 'Flag indicating if users are permitted to update their profile name'
       optional :usage_ping_enabled, type: Boolean, desc: 'Every week GitLab will report license usage back to GitLab, Inc.'
       optional :instance_statistics_visibility_private, type: Boolean, desc: 'When set to `true` Instance statistics will only be available to admins'
       optional :local_markdown_version, type: Integer, desc: 'Local markdown version, increase this value when any cached markdown should be invalidated'

locale/gitlab.pot

@@ -13449,6 +13449,9 @@ msgstr ""
 msgid "Prevent environment from auto-stopping"
 msgstr ""
 
+msgid "Prevent users from changing their profile name"
+msgstr ""
+
 msgid "Preview"
 msgstr ""
 
@@ -13752,6 +13755,9 @@ msgstr ""
 msgid "Profiles|Tell us about yourself in fewer than 250 characters"
 msgstr ""
 
+msgid "Profiles|The ability to update your name has been disabled by your administrator."
+msgstr ""
+
 msgid "Profiles|The maximum file size allowed is 200KB."
 msgstr ""
 
@@ -19932,6 +19938,9 @@ msgstr ""
 msgid "User pipeline minutes were successfully reset."
 msgstr ""
 
+msgid "User restrictions"
+msgstr ""
+
 msgid "User settings"
 msgstr ""
 

spec/controllers/admin/application_settings_controller_spec.rb

@@ -102,6 +102,13 @@ describe Admin::ApplicationSettingsController do
       expect(ApplicationSetting.current.minimum_password_length).to eq(10)
     end
 
+    it 'updates updating_name_disabled_for_users setting' do
+      put :update, params: { application_setting: { updating_name_disabled_for_users: true } }
+
+      expect(response).to redirect_to(admin_application_settings_path)
+      expect(ApplicationSetting.current.updating_name_disabled_for_users).to eq(true)
+    end
+
     context 'external policy classification settings' do
       let(:settings) do
         {

spec/controllers/admin/users_controller_spec.rb

@@ -257,6 +257,28 @@ describe Admin::UsersController do
   end
 
   describe 'POST update' do
+    context 'updating name' do
+      context 'when the ability to update their name is disabled for users' do
+        before do
+          stub_application_setting(updating_name_disabled_for_users: true)
+        end
+
+        it 'updates the name' do
+          params = {
+            id: user.to_param,
+            user: {
+              name: 'New Name'
+            }
+          }
+
+          put :update, params: params
+
+          expect(response).to redirect_to(admin_user_path(user))
+          expect(user.reload.name).to eq('New Name')
+        end
+      end
+    end
+
     context 'when the password has changed' do
       def update_password(user, password, password_confirmation = nil)
         params = {

spec/controllers/profiles_controller_spec.rb

@@ -81,6 +81,54 @@ describe ProfilesController, :request_store do
       expect(ldap_user.location).to eq('City, Country')
     end
 
+    context 'updating name' do
+      subject { put :update, params: { user: { name: 'New Name' } } }
+
+      context 'when the ability to update thier name is not disabled for users' do
+        before do
+          stub_application_setting(updating_name_disabled_for_users: false)
+          sign_in(user)
+        end
+
+        it 'updates the name' do
+          subject
+
+          expect(response.status).to eq(302)
+          expect(user.reload.name).to eq('New Name')
+        end
+      end
+
+      context 'when the ability to update their name is disabled for users' do
+        before do
+          stub_application_setting(updating_name_disabled_for_users: true)
+        end
+
+        context 'as a regular user' do
+          it 'does not update the name' do
+            sign_in(user)
+
+            subject
+
+            expect(response.status).to eq(302)
+            expect(user.reload.name).not_to eq('New Name')
+          end
+        end
+
+        context 'as an admin user' do
+          it 'updates the name' do
+            admin = create(:admin)
+
+            sign_in(admin)
+
+            subject
+
+            expect(response.status).to eq(302)
+            expect(admin.reload.name).to eq('New Name')
+          end
+        end
+      end
+    end
+
     it 'allows setting a user status' do
       sign_in(user)
 

spec/policies/user_policy_spec.rb

@@ -48,4 +48,36 @@ describe UserPolicy do
   describe "updating a user" do
     it_behaves_like 'changing a user', :update_user
   end
+
+  describe "updating a user's name" do
+    context 'when the ability to update their name is not disabled for users' do
+      before do
+        stub_application_setting(updating_name_disabled_for_users: false)
+      end
+
+      it_behaves_like 'changing a user', :update_name
+    end
+
+    context 'when the ability to update their name is disabled for users' do
+      before do
+        stub_application_setting(updating_name_disabled_for_users: true)
+      end
+
+      context 'for a regular user' do
+        it { is_expected.not_to be_allowed(:update_name) }
+      end
+
+      context 'for a ghost user' do
+        let(:current_user) { create(:user, :ghost) }
+
+        it { is_expected.not_to be_allowed(:update_name) }
+      end
+
+      context 'for an admin user' do
+        let(:current_user) { create(:admin) }
+
+        it { is_expected.to be_allowed(:update_name) }
+      end
+    end
+  end
 end

spec/requests/api/settings_spec.rb

@@ -136,6 +136,14 @@ describe API::Settings, 'Settings' do
       expect(json_response['performance_bar_allowed_group_id']).to eq(group.id)
     end
 
+    it "supports updating_name_disabled_for_users" do
+      put api("/application/settings", admin),
+        params: { updating_name_disabled_for_users: true }
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response['updating_name_disabled_for_users']).to eq(true)
+    end
+
     it "supports legacy performance_bar_enabled" do
       put api("/application/settings", admin),
         params: {

spec/requests/api/users_spec.rb

@@ -645,6 +645,21 @@ describe API::Users do
       expect(response).to have_gitlab_http_status(200)
     end
 
+    context 'updating name' do
+      context 'when the ability to update their name is disabled for users' do
+        before do
+          stub_application_setting(updating_name_disabled_for_users: true)
+        end
+
+        it 'updates the user with new name' do
+          put api("/users/#{user.id}", admin), params: { name: 'New Name' }
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(json_response['name']).to eq('New Name')
+        end
+      end
+    end
+
     it "updates user with new bio" do
       put api("/users/#{user.id}", admin), params: { bio: 'new test bio' }
 

spec/services/users/update_service_spec.rb

@@ -6,12 +6,45 @@ describe Users::UpdateService do
   let(:user) { create(:user) }
 
   describe '#execute' do
+    context 'updating name' do
+      context 'when the ability to update their name is not disabled for users' do
+        before do
+          stub_application_setting(updating_name_disabled_for_users: false)
+        end
+
         it 'updates the name' do
           result = update_user(user, name: 'New Name')
 
           expect(result).to eq(status: :success)
           expect(user.name).to eq('New Name')
         end
+      end
+
+      context 'when the ability to update their name is disabled for users' do
+        before do
+          stub_application_setting(updating_name_disabled_for_users: true)
+        end
+
+        context 'executing as a regular user' do
+          it 'does not update the name' do
+            result = update_user(user, name: 'New Name')
+
+            expect(result).to eq(status: :success)
+            expect(user.name).not_to eq('New Name')
+          end
+        end
+
+        context 'executing as an admin user' do
+          it 'updates the name' do
+            admin = create(:admin)
+            result = described_class.new(admin, { user: user, name: 'New Name' }).execute
+
+            expect(result).to eq(status: :success)
+            expect(user.name).to eq('New Name')
+          end
+        end
+      end
+    end
 
     it 'updates time preferences' do
       result = update_user(user, timezone: 'Europe/Warsaw', time_display_relative: true, time_format_in_24h: false)