Add latest changes from gitlab-org/gitlab@13-12-stable-eev13.12.0-rc42

1 files changed, 130 insertions, 114 deletions

diff --git a/spec/services/users/build_service_spec.rb b/spec/services/users/build_service_spec.rb
index b2a7d349ce6..e8786c677d1 100644
--- a/spec/services/users/build_service_spec.rb
+++ b/spec/services/users/build_service_spec.rb
@@ -6,105 +6,76 @@ RSpec.describe Users::BuildService do
   using RSpec::Parameterized::TableSyntax
 
   describe '#execute' do
-    let(:params) { build_stubbed(:user).slice(:first_name, :last_name, :username, :email, :password) }
-
-    context 'with an admin user' do
-      let(:params) { build_stubbed(:user).slice(:name, :username, :email, :password) }
+    let_it_be(:current_user) { nil }
 
-      let(:admin_user) { create(:admin) }
-      let(:service) { described_class.new(admin_user, ActionController::Parameters.new(params).permit!) }
+    let(:params) { build_stubbed(:user).slice(:first_name, :last_name, :username, :email, :password) }
+    let(:service) { described_class.new(current_user, params) }
 
-      it 'returns a valid user' do
-        expect(service.execute).to be_valid
-      end
+    shared_examples_for 'common build items' do
+      it { is_expected.to be_valid }
 
       it 'sets the created_by_id' do
-        expect(service.execute.created_by_id).to eq(admin_user.id)
+        expect(user.created_by_id).to eq(current_user&.id)
       end
 
-      context 'calls the UpdateCanonicalEmailService' do
-        specify do
-          expect(Users::UpdateCanonicalEmailService).to receive(:new).and_call_original
+      it 'calls UpdateCanonicalEmailService' do
+        expect(Users::UpdateCanonicalEmailService).to receive(:new).and_call_original
 
-          service.execute
-        end
+        user
       end
 
-      context 'allowed params' do
-        let(:params) do
-          {
-            access_level: 1,
-            admin: 1,
-            avatar: anything,
-            bio: 1,
-            can_create_group: 1,
-            color_scheme_id: 1,
-            email: 1,
-            external: 1,
-            force_random_password: 1,
-            hide_no_password: 1,
-            hide_no_ssh_key: 1,
-            linkedin: 1,
-            name: 1,
-            password: 1,
-            password_automatically_set: 1,
-            password_expires_at: 1,
-            projects_limit: 1,
-            remember_me: 1,
-            skip_confirmation: 1,
-            skype: 1,
-            theme_id: 1,
-            twitter: 1,
-            username: 1,
-            website_url: 1,
-            private_profile: 1,
-            organization: 1,
-            location: 1,
-            public_email: 1
-          }
-        end
+      context 'when user_type is provided' do
+        context 'when project_bot' do
+          before do
+            params.merge!({ user_type: :project_bot })
+          end
 
-        it 'sets all allowed attributes' do
-          admin_user # call first so the admin gets created before setting `expect`
+          it { expect(user.project_bot?).to be true }
+        end
 
-          expect(User).to receive(:new).with(hash_including(params)).and_call_original
+        context 'when not a project_bot' do
+          before do
+            params.merge!({ user_type: :alert_bot })
+          end
 
-          service.execute
+          it { expect(user).to be_human }
         end
       end
+    end
 
+    shared_examples_for 'current user not admin' do
       context 'with "user_default_external" application setting' do
         where(:user_default_external, :external, :email, :user_default_internal_regex, :result) do
           true  | nil   | 'fl@example.com'        | nil                     | true
           true  | true  | 'fl@example.com'        | nil                     | true
-          true  | false | 'fl@example.com'        | nil                     | false
+          true  | false | 'fl@example.com'        | nil                     | true # admin difference
 
           true  | nil   | 'fl@example.com'        | ''                      | true
           true  | true  | 'fl@example.com'        | ''                      | true
-          true  | false | 'fl@example.com'        | ''                      | false
+          true  | false | 'fl@example.com'        | ''                      | true # admin difference
 
           true  | nil   | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false
-          true  | true  | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | true
+          true  | true  | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false # admin difference
           true  | false | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false
 
           true  | nil   | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | true
           true  | true  | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | true
-          true  | false | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | false
+          true  | false | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | true # admin difference
 
           false | nil   | 'fl@example.com'        | nil                     | false
-          false | true  | 'fl@example.com'        | nil                     | true
+          false | true  | 'fl@example.com'        | nil                     | false # admin difference
           false | false | 'fl@example.com'        | nil                     | false
 
           false | nil   | 'fl@example.com'        | ''                      | false
-          false | true  | 'fl@example.com'        | ''                      | true
+          false | true  | 'fl@example.com'        | ''                      | false # admin difference
           false | false | 'fl@example.com'        | ''                      | false
 
           false | nil   | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false
-          false | true  | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | true
+          false | true  | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false # admin difference
           false | false | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false
 
           false | nil   | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | false
-          false | true  | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | true
+          false | true  | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | false # admin difference
           false | false | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | false
         end
 
@@ -116,40 +87,11 @@ RSpec.describe Users::BuildService do
             params.merge!({ external: external, email: email }.compact)
           end
 
-          subject(:user) { service.execute }
-
-          it 'correctly sets user.external' do
+          it 'sets the value of Gitlab::CurrentSettings.user_default_external' do
             expect(user.external).to eq(result)
           end
         end
       end
-    end
-
-    context 'with non admin user' do
-      let(:user) { create(:user) }
-      let(:service) { described_class.new(user, params) }
-
-      it 'raises AccessDeniedError exception' do
-        expect { service.execute }.to raise_error Gitlab::Access::AccessDeniedError
-      end
-
-      context 'when authorization is skipped' do
-        subject(:built_user) { service.execute(skip_authorization: true) }
-
-        it { is_expected.to be_valid }
-
-        it 'sets the created_by_id' do
-          expect(built_user.created_by_id).to eq(user.id)
-        end
-      end
-    end
-
-    context 'with nil user' do
-      let(:service) { described_class.new(nil, params) }
-
-      it 'returns a valid user' do
-        expect(service.execute).to be_valid
-      end
 
       context 'when "send_user_confirmation_email" application setting is true' do
         before do
@@ -157,7 +99,7 @@ RSpec.describe Users::BuildService do
         end
 
         it 'does not confirm the user' do
-          expect(service.execute).not_to be_confirmed
+          expect(user).not_to be_confirmed
         end
       end
 
@@ -167,27 +109,103 @@ RSpec.describe Users::BuildService do
         end
 
         it 'confirms the user' do
-          expect(service.execute).to be_confirmed
+          expect(user).to be_confirmed
         end
       end
 
-      context 'when user_type is provided' do
-        subject(:user) { service.execute }
+      context 'with allowed params' do
+        let(:params) do
+          {
+            email: 1,
+            name: 1,
+            password: 1,
+            password_automatically_set: 1,
+            username: 1,
+            user_type: 'project_bot'
+          }
+        end
 
-        context 'when project_bot' do
-          before do
-            params.merge!({ user_type: :project_bot })
-          end
+        it 'sets all allowed attributes' do
+          expect(User).to receive(:new).with(hash_including(params)).and_call_original
 
-          it { expect(user.project_bot?).to be true}
+          user
         end
+      end
+    end
 
-        context 'when not a project_bot' do
-          before do
-            params.merge!({ user_type: :alert_bot })
-          end
+    context 'with nil current_user' do
+      subject(:user) { service.execute }
+
+      it_behaves_like 'common build items'
+      it_behaves_like 'current user not admin'
+    end
+
+    context 'with non admin current_user' do
+      let_it_be(:current_user) { create(:user) }
+
+      let(:service) { described_class.new(current_user, params) }
+
+      subject(:user) { service.execute(skip_authorization: true) }
+
+      it 'raises AccessDeniedError exception when authorization is not skipped' do
+        expect { service.execute }.to raise_error Gitlab::Access::AccessDeniedError
+      end
 
-          it { expect(user.user_type).to be nil }
+      it_behaves_like 'common build items'
+      it_behaves_like 'current user not admin'
+    end
+
+    context 'with an admin current_user' do
+      let_it_be(:current_user) { create(:admin) }
+
+      let(:params) { build_stubbed(:user).slice(:name, :username, :email, :password) }
+      let(:service) { described_class.new(current_user, ActionController::Parameters.new(params).permit!) }
+
+      subject(:user) { service.execute }
+
+      it_behaves_like 'common build items'
+
+      context 'with allowed params' do
+        let(:params) do
+          {
+            access_level: 1,
+            admin: 1,
+            avatar: anything,
+            bio: 1,
+            can_create_group: 1,
+            color_scheme_id: 1,
+            email: 1,
+            external: 1,
+            force_random_password: 1,
+            hide_no_password: 1,
+            hide_no_ssh_key: 1,
+            linkedin: 1,
+            name: 1,
+            password: 1,
+            password_automatically_set: 1,
+            password_expires_at: 1,
+            projects_limit: 1,
+            remember_me: 1,
+            skip_confirmation: 1,
+            skype: 1,
+            theme_id: 1,
+            twitter: 1,
+            username: 1,
+            website_url: 1,
+            private_profile: 1,
+            organization: 1,
+            location: 1,
+            public_email: 1,
+            user_type: 'project_bot',
+            note: 1,
+            view_diffs_file_by_file: 1
+          }
+        end
+
+        it 'sets all allowed attributes' do
+          expect(User).to receive(:new).with(hash_including(params)).and_call_original
+
+          service.execute
         end
       end
 
@@ -195,34 +213,34 @@ RSpec.describe Users::BuildService do
         where(:user_default_external, :external, :email, :user_default_internal_regex, :result) do
           true  | nil   | 'fl@example.com'        | nil                     | true
           true  | true  | 'fl@example.com'        | nil                     | true
-          true  | false | 'fl@example.com'        | nil                     | true
+          true  | false | 'fl@example.com'        | nil                     | false # admin difference
 
           true  | nil   | 'fl@example.com'        | ''                      | true
           true  | true  | 'fl@example.com'        | ''                      | true
-          true  | false | 'fl@example.com'        | ''                      | true
+          true  | false | 'fl@example.com'        | ''                      | false # admin difference
 
           true  | nil   | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false
-          true  | true  | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false
+          true  | true  | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | true # admin difference
           true  | false | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false
 
           true  | nil   | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | true
           true  | true  | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | true
-          true  | false | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | true
+          true  | false | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | false # admin difference
 
           false | nil   | 'fl@example.com'        | nil                     | false
-          false | true  | 'fl@example.com'        | nil                     | false
+          false | true  | 'fl@example.com'        | nil                     | true # admin difference
           false | false | 'fl@example.com'        | nil                     | false
 
           false | nil   | 'fl@example.com'        | ''                      | false
-          false | true  | 'fl@example.com'        | ''                      | false
+          false | true  | 'fl@example.com'        | ''                      | true # admin difference
           false | false | 'fl@example.com'        | ''                      | false
 
           false | nil   | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false
-          false | true  | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false
+          false | true  | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | true # admin difference
           false | false | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false
 
           false | nil   | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | false
-          false | true  | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | false
+          false | true  | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | true # admin difference
           false | false | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | false
         end
 
@@ -234,8 +252,6 @@ RSpec.describe Users::BuildService do
             params.merge!({ external: external, email: email }.compact)
           end
 
-          subject(:user) { service.execute }
-
           it 'sets the value of Gitlab::CurrentSettings.user_default_external' do
             expect(user.external).to eq(result)
           end