Merge branch '267568-user-admin-approval-default-for-new-instances' into 'master'

User admin approval - Default for new instances

See merge request gitlab-org/gitlab!46937

app/models/application_setting_implementation.rb

@@ -120,7 +120,7 @@ module ApplicationSettingImplementation
         repository_checks_enabled: true,
         repository_storages_weighted: { default: 100 },
         repository_storages: ['default'],
-        require_admin_approval_after_user_signup: false,
+        require_admin_approval_after_user_signup: true,
         require_two_factor_authentication: false,
         restricted_visibility_levels: Settings.gitlab['restricted_visibility_levels'],
         rsa_key_restriction: 0,

changelogs/unreleased/267568-user-admin-approval-default-for-new-instances.yml

+---
+title: Admin approval required on user registration by default
+merge_request: 46937
+author:
+type: changed

db/migrate/20201107032257_add_default_true_require_admin_approval_after_user_signup_to_application_settings.rb

+# frozen_string_literal: true
+
+class AddDefaultTrueRequireAdminApprovalAfterUserSignupToApplicationSettings < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    change_column_default :application_settings, :require_admin_approval_after_user_signup, from: false, to: true
+  end
+end

db/schema_migrations/20201107032257

+07160ee3c92e68273042df979640c3927abbb187f79e1a4645471e28061e1c2c
\ No newline at end of file

db/structure.sql

@@ -9324,7 +9324,7 @@ CREATE TABLE application_settings (
     gitpod_enabled boolean DEFAULT false NOT NULL,
     gitpod_url text DEFAULT 'https://gitpod.io/'::text,
     abuse_notification_email character varying,
-    require_admin_approval_after_user_signup boolean DEFAULT false NOT NULL,
+    require_admin_approval_after_user_signup boolean DEFAULT true NOT NULL,
     help_page_documentation_base_url text,
     automatic_purchased_storage_allocation boolean DEFAULT false NOT NULL,
     encrypted_ci_jwt_signing_key text,

doc/user/admin_area/settings/sign_up_restrictions.md

@@ -27,9 +27,10 @@ To disable sign ups:
 
 ## Require administrator approval for new sign ups
 
-> [Introduced](https://gitlab.com/groups/gitlab-org/-/epics/4491) in GitLab 13.5.
+> - [Introduced](https://gitlab.com/groups/gitlab-org/-/epics/4491) in GitLab 13.5.
+> - [Enabled by default](https://gitlab.com/gitlab-org/gitlab/-/issues/267568) in GitLab 13.6.
 
-When this setting is enabled, any user visiting your GitLab domain and signing up for a new account must be explicitly [approved](../approving_users.md#approving-a-user) by an administrator before they can start using their account. This setting is only applicable if sign ups are enabled.
+When this setting is enabled, any user visiting your GitLab domain and signing up for a new account must be explicitly [approved](../approving_users.md#approving-a-user) by an administrator before they can start using their account. This setting is enabled by default for newly created instances. This setting is only applicable if sign ups are enabled.
 
 To require administrator approval for new sign ups:
 

ee/spec/features/invites_spec.rb

@@ -10,6 +10,7 @@ RSpec.describe 'Group or Project invitations' do
   let(:dev_env_or_com) { true }
 
   before do
+    stub_application_setting(require_admin_approval_after_user_signup: false)
     allow(::Gitlab).to receive(:dev_env_or_com?).and_return(dev_env_or_com)
 
     visit invite_path(group_invite.raw_invite_token)
@@ -55,4 +56,17 @@ RSpec.describe 'Group or Project invitations' do
       expect(page).not_to have_content('My company or team')
     end
   end
+
+  context 'with admin approval on sign-up enabled' do
+    before do
+      stub_application_setting(require_admin_approval_after_user_signup: true)
+    end
+
+    it 'does not sign the user in' do
+      fill_in_sign_up_form(new_user)
+
+      expect(current_path).to eq(new_user_session_path)
+      expect(page).to have_content('You have signed up successfully. However, we could not sign you in because your account is awaiting approval from your GitLab administrator.')
+    end
+  end
 end

ee/spec/features/signup_spec.rb

@@ -5,6 +5,10 @@ require 'spec_helper'
 RSpec.describe 'Signup on EE' do
   let(:new_user) { build_stubbed(:user) }
 
+  before do
+    stub_application_setting(require_admin_approval_after_user_signup: false)
+  end
+
   def fill_in_signup_form
     fill_in 'new_user_username', with: new_user.username
     fill_in 'new_user_email', with: new_user.email

ee/spec/features/trial_registrations/signup_spec.rb

@@ -5,6 +5,10 @@ require 'spec_helper'
 RSpec.describe 'Trial Sign Up' do
   let(:user_attrs) { attributes_for(:user, first_name: 'GitLab', last_name: 'GitLab') }
 
+  before do
+    stub_application_setting(require_admin_approval_after_user_signup: false)
+  end
+
   describe 'on GitLab.com' do
     before do
       allow(Gitlab).to receive(:com?).and_return(true).at_least(:once)

qa/qa/page/main/sign_up.rb

@@ -18,15 +18,13 @@ module QA
         end
 
         def sign_up!(user)
-          fill_element :new_user_first_name_field, user.first_name
-          fill_element :new_user_last_name_field, user.last_name
-          fill_element :new_user_username_field, user.username
-          fill_element :new_user_email_field, user.email
-          fill_element :new_user_password_field, user.password
-
-          signed_in = retry_until do
+          signed_in = retry_until(raise_on_failure: false) do
+            fill_element :new_user_first_name_field, user.first_name
+            fill_element :new_user_last_name_field, user.last_name
+            fill_element :new_user_username_field, user.username
+            fill_element :new_user_email_field, user.email
+            fill_element :new_user_password_field, user.password
             click_element :new_user_register_button if has_element?(:new_user_register_button)
-
             click_element :get_started_button if has_element?(:get_started_button)
 
             Page::Main::Menu.perform(&:has_personal_area?)

qa/qa/runtime/application_settings.rb

@@ -33,7 +33,7 @@ module QA
 
         def api_client
           @api_client ||= Runtime::API::Client.as_admin
-        rescue AuthorizationError => e
+        rescue API::Client::AuthorizationError => e
           raise "Administrator access is required to set application settings. #{e.message}"
         end
       end

qa/qa/runtime/env.rb

@@ -8,7 +8,7 @@ module QA
     module Env
       extend self
 
-      attr_writer :personal_access_token, :ldap_username, :ldap_password
+      attr_writer :personal_access_token
 
       ENV_VARIABLES = Gitlab::QA::Runtime::Env::ENV_VARIABLES
 
@@ -293,6 +293,11 @@ module QA
         @ldap_username ||= ENV['GITLAB_LDAP_USERNAME']
       end
 
+      def ldap_username=(ldap_username)
+        @ldap_username = ldap_username # rubocop:disable Gitlab/ModuleWithInstanceVariables
+        ENV['GITLAB_LDAP_USERNAME'] = ldap_username
+      end
+
       def ldap_password
         @ldap_password ||= ENV['GITLAB_LDAP_PASSWORD']
       end

qa/qa/specs/features/browser_ui/1_manage/login/register_spec.rb

@@ -13,11 +13,39 @@ module QA
     end
   end
 
-  RSpec.describe 'Manage', :skip_signup_disabled do
+  RSpec.describe 'Manage', :skip_signup_disabled, :requires_admin do
+    describe 'while LDAP is enabled', :orchestrated, :ldap_no_tls, testcase: 'https://gitlab.com/gitlab-org/quality/testcases/-/issues/935' do
+      before do
+        # When LDAP is enabled, a previous test might have created a token for the LDAP 'tanuki' user who is not an admin
+        # So we need to set it to nil in order to create a new token for admin user so that we are able to set_application_settings
+        # Also, when GITLAB_LDAP_USERNAME is provided, it is used to create a token. This also needs to be set to nil temporarily
+        # for the same reason as above.
+
+        @personal_access_token = Runtime::Env.personal_access_token
+        Runtime::Env.personal_access_token = nil
+        ldap_username = Runtime::Env.ldap_username
+        Runtime::Env.ldap_username = nil
+
+        disable_require_admin_approval_after_user_signup
+
+        Runtime::Env.ldap_username = ldap_username
+      end
+
+      it_behaves_like 'registration and login'
+
+      after do
+        Runtime::Env.personal_access_token = @personal_access_token
+      end
+    end
+
     describe 'standard', testcase: 'https://gitlab.com/gitlab-org/quality/testcases/-/issues/936' do
+      before(:all) do
+        disable_require_admin_approval_after_user_signup
+      end
+
       it_behaves_like 'registration and login'
 
-      context 'when user account is deleted', :requires_admin do
+      context 'when user account is deleted' do
         let(:user) do
           Resource::User.fabricate_via_api! do |resource|
             resource.api_client = admin_api_client
@@ -61,11 +89,10 @@ module QA
         end
       end
     end
-  end
 
-  RSpec.describe 'Manage', :orchestrated, :ldap_no_tls, :skip_signup_disabled do
-    describe 'while LDAP is enabled', testcase: 'https://gitlab.com/gitlab-org/quality/testcases/-/issues/935' do
-      it_behaves_like 'registration and login'
+    def disable_require_admin_approval_after_user_signup
+      Runtime::ApplicationSettings.set_application_settings(require_admin_approval_after_user_signup: false)
+      sleep 10 # It takes a moment for the setting to come into effect
     end
   end
 end

spec/controllers/registrations_controller_spec.rb

@@ -7,6 +7,7 @@ RSpec.describe RegistrationsController do
 
   before do
     stub_feature_flags(invisible_captcha: false)
+    stub_application_setting(require_admin_approval_after_user_signup: false)
   end
 
   describe '#new' do
@@ -76,10 +77,6 @@ RSpec.describe RegistrationsController do
       end
 
       context 'when the `require_admin_approval_after_user_signup` setting is turned off' do
-        before do
-          stub_application_setting(require_admin_approval_after_user_signup: false)
-        end
-
         it 'signs up the user in `active` state' do
           subject
           created_user = User.find_by(email: 'new@user.com')

spec/features/invites_spec.rb

@@ -10,6 +10,7 @@ RSpec.describe 'Group or Project invitations', :aggregate_failures do
   let(:group_invite) { group.group_members.invite.last }
 
   before do
+    stub_application_setting(require_admin_approval_after_user_signup: false)
     project.add_maintainer(owner)
     group.add_owner(owner)
     group.add_developer('user@example.com', owner)
@@ -97,6 +98,21 @@ RSpec.describe 'Group or Project invitations', :aggregate_failures do
         click_link 'Register now'
       end
 
+      context 'with admin appoval required enabled' do
+        before do
+          stub_application_setting(require_admin_approval_after_user_signup: true)
+        end
+
+        let(:send_email_confirmation) { true }
+
+        it 'does not sign the user in' do
+          fill_in_sign_up_form(new_user)
+
+          expect(current_path).to eq(new_user_session_path)
+          expect(page).to have_content('You have signed up successfully. However, we could not sign you in because your account is awaiting approval from your GitLab administrator')
+        end
+      end
+
       context 'email confirmation disabled' do
         let(:send_email_confirmation) { false }
 

spec/features/users/signup_spec.rb

@@ -43,6 +43,10 @@ end
 RSpec.describe 'Signup' do
   include TermsHelper
 
+  before do
+    stub_application_setting(require_admin_approval_after_user_signup: false)
+  end
+
   let(:new_user) { build_stubbed(:user) }
 
   def fill_in_signup_form
@@ -228,6 +232,22 @@ RSpec.describe 'Signup' do
         expect(current_path).to eq users_sign_up_welcome_path
       end
     end
+
+    context 'with required admin approval enabled' do
+      before do
+        stub_application_setting(require_admin_approval_after_user_signup: true)
+      end
+
+      it 'creates the user but does not sign them in' do
+        visit new_user_registration_path
+
+        fill_in_signup_form
+
+        expect { click_button 'Register' }.to change { User.count }.by(1)
+        expect(current_path).to eq new_user_session_path
+        expect(page).to have_content("You have signed up successfully. However, we could not sign you in because your account is awaiting approval from your GitLab administrator")
+      end
+    end
   end
 
   context 'with errors' do

spec/requests/api/settings_spec.rb

@@ -41,7 +41,7 @@ RSpec.describe API::Settings, 'Settings' do
       expect(json_response['spam_check_endpoint_enabled']).to be_falsey
       expect(json_response['spam_check_endpoint_url']).to be_nil
       expect(json_response['wiki_page_max_content_bytes']).to be_a(Integer)
-      expect(json_response['require_admin_approval_after_user_signup']).to eq(false)
+      expect(json_response['require_admin_approval_after_user_signup']).to eq(true)
     end
   end