Merge branch...

Merge branch '215697-allow-groups-to-disable-2fa-requirement-for-subgroups-update-subgroups' into 'master'

Allow groups to disable 2FA requirement for subgroups - changing subgroups settings

See merge request gitlab-org/gitlab!44337

app/services/groups/transfer_service.rb

@@ -38,6 +38,7 @@ module Groups
     # Overridden in EE
     def post_update_hooks(updated_project_ids)
       refresh_project_authorizations
+      refresh_descendant_groups if @new_parent_group
     end
 
     def ensure_allowed_transfer
@@ -101,6 +102,8 @@ module Groups
         @group.visibility_level = @new_parent_group.visibility_level
       end
 
+      update_two_factor_authentication if @new_parent_group
+
       @group.parent = @new_parent_group
       @group.clear_memoization(:self_and_ancestors_ids)
 
@@ -129,8 +132,26 @@ module Groups
       projects_to_update
         .update_all(visibility_level: @new_parent_group.visibility_level)
     end
+
+    def update_two_factor_authentication
+      return if namespace_parent_allows_two_factor_auth
+
+      @group.require_two_factor_authentication = false
+    end
+
+    def refresh_descendant_groups
+      return if namespace_parent_allows_two_factor_auth
+
+      if @group.descendants.where(require_two_factor_authentication: true).any?
+        DisallowTwoFactorForSubgroupsWorker.perform_async(@group.id)
+      end
+    end
     # rubocop: enable CodeReuse/ActiveRecord
 
+    def namespace_parent_allows_two_factor_auth
+      @new_parent_group.namespace_settings.allow_mfa_for_subgroups
+    end
+
     def ensure_ownership
       return if @new_parent_group
       return unless @group.owners.empty?

app/services/groups/update_service.rb

@@ -4,6 +4,8 @@ module Groups
   class UpdateService < Groups::BaseService
     include UpdateVisibilityLevel
 
+    SETTINGS_PARAMS = [:allow_mfa_for_subgroups].freeze
+
     def execute
       reject_parent_id!
       remove_unallowed_params
@@ -21,6 +23,8 @@ module Groups
 
       return false unless update_shared_runners
 
+      handle_changes
+
       before_assignment_hook(group, params)
 
       handle_namespace_settings
@@ -89,6 +93,19 @@ module Groups
         # don't enqueue immediately to prevent todos removal in case of a mistake
         TodosDestroyer::GroupPrivateWorker.perform_in(Todo::WAIT_FOR_DELETE, group.id)
       end
+
+      update_two_factor_requirement_for_subgroups
+    end
+
+    def update_two_factor_requirement_for_subgroups
+      settings = group.namespace_settings
+
+      return if settings.allow_mfa_for_subgroups
+
+      if settings.previous_changes.include?(:allow_mfa_for_subgroups)
+        # enque in batches members update
+        DisallowTwoFactorForSubgroupsWorker.perform_async(group.id)
+      end
     end
 
     def reject_parent_id!
@@ -101,6 +118,21 @@ module Groups
       params.delete(:default_branch_protection) unless can?(current_user, :update_default_branch_protection, group)
     end
 
+    def handle_changes
+      handle_settings_update
+    end
+
+    def handle_settings_update
+      settings_params = params.slice(*allowed_settings_params)
+      allowed_settings_params.each { |param| params.delete(param) }
+
+      ::NamespaceSettings::UpdateService.new(current_user, group, settings_params).execute
+    end
+
+    def allowed_settings_params
+      SETTINGS_PARAMS
+    end
+
     def valid_share_with_group_lock_change?
       return true unless changing_share_with_group_lock?
       return true if can?(current_user, :change_share_with_group_lock, group)

app/workers/all_queues.yml

@@ -1409,6 +1409,22 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: disallow_two_factor_for_group
+  :feature_category: :subgroups
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
+- :name: disallow_two_factor_for_subgroups
+  :feature_category: :subgroups
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: email_receiver
   :feature_category: :issue_tracking
   :has_external_dependencies:

app/workers/disallow_two_factor_for_group_worker.rb

+# frozen_string_literal: true
+
+class DisallowTwoFactorForGroupWorker
+  include ApplicationWorker
+  include ExceptionBacktrace
+
+  feature_category :subgroups
+  idempotent!
+
+  def perform(group_id)
+    begin
+      group = Group.find(group_id)
+    rescue ActiveRecord::RecordNotFound
+      return
+    end
+
+    group.update!(require_two_factor_authentication: false)
+  end
+end

app/workers/disallow_two_factor_for_subgroups_worker.rb

+# frozen_string_literal: true
+
+class DisallowTwoFactorForSubgroupsWorker
+  include ApplicationWorker
+  include ExceptionBacktrace
+
+  INTERVAL = 2.seconds.to_i
+
+  feature_category :subgroups
+  idempotent!
+
+  def perform(group_id)
+    begin
+      group = Group.find(group_id)
+    rescue ActiveRecord::RecordNotFound
+      return
+    end
+
+    # rubocop: disable CodeReuse/ActiveRecord
+    subgroups = group.descendants.where(require_two_factor_authentication: true) # rubocop: disable CodeReuse/ActiveRecord
+    subgroups.find_each(batch_size: 100).with_index do |subgroup, index|
+      delay = index * INTERVAL
+
+      with_context(namespace: subgroup) do
+        DisallowTwoFactorForGroupWorker.perform_in(delay, subgroup.id)
+      end
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
+  end
+end

config/sidekiq_queues.yml

@@ -86,6 +86,10 @@
   - 1
 - - detect_repository_languages
   - 1
+- - disallow_two_factor_for_group
+  - 1
+- - disallow_two_factor_for_subgroups
+  - 1
 - - elastic_commit_indexer
   - 1
 - - elastic_delete_project

ee/app/services/ee/groups/update_service.rb

@@ -4,6 +4,7 @@ module EE
   module Groups
     module UpdateService
       extend ::Gitlab::Utils::Override
+      EE_SETTINGS_PARAMS = [:prevent_forking_outside_group].freeze
 
       override :execute
       def execute
@@ -90,10 +91,11 @@ module EE
         end
       end
 
+      override :handle_changes
       def handle_changes
         handle_allowed_email_domains_update
         handle_ip_restriction_update
-        handle_settings_update
+        super
       end
 
       def handle_ip_restriction_update
@@ -112,11 +114,9 @@ module EE
         AllowedEmailDomains::UpdateService.new(current_user, group, comma_separated_domains).execute
       end
 
-      def handle_settings_update
-        settings_params = params.slice(:prevent_forking_outside_group)
-        params.delete(:prevent_forking_outside_group)
-
-        ::NamespaceSettings::UpdateService.new(current_user, group, settings_params).execute
+      override :allowed_settings_params
+      def allowed_settings_params
+        @allowed_settings_params ||= super + EE_SETTINGS_PARAMS
       end
 
       def log_audit_event

spec/factories/groups.rb

@@ -23,7 +23,7 @@ FactoryBot.define do
     end
 
     trait :internal do
-      visibility_level {Gitlab::VisibilityLevel::INTERNAL }
+      visibility_level { Gitlab::VisibilityLevel::INTERNAL }
     end
 
     trait :private do

spec/services/groups/transfer_service_spec.rb

@@ -567,6 +567,39 @@ RSpec.describe Groups::TransferService do
         end
       end
 
+      context 'when transferring a group with two factor authentication switched on' do
+        before do
+          TestEnv.clean_test_path
+          create(:group_member, :owner, group: new_parent_group, user: user)
+          create(:group, :private, parent: group, require_two_factor_authentication: true)
+          group.update!(require_two_factor_authentication: true)
+        end
+
+        it 'does not update group two factor authentication setting' do
+          transfer_service.execute(new_parent_group)
+
+          expect(group.require_two_factor_authentication).to eq(true)
+        end
+
+        context 'when new parent disallows two factor authentication switched on for descendants' do
+          before do
+            new_parent_group.namespace_settings.update!(allow_mfa_for_subgroups: false)
+          end
+
+          it 'updates group two factor authentication setting' do
+            transfer_service.execute(new_parent_group)
+
+            expect(group.require_two_factor_authentication).to eq(false)
+          end
+
+          it 'schedules update of group two factor authentication setting for descendants' do
+            expect(DisallowTwoFactorForSubgroupsWorker).to receive(:perform_async).with(group.id)
+
+            transfer_service.execute(new_parent_group)
+          end
+        end
+      end
+
       context 'when updating the group goes wrong' do
         let!(:subgroup1) { create(:group, :public, parent: group) }
         let!(:subgroup2) { create(:group, :public, parent: group) }

spec/services/groups/update_service_spec.rb

@@ -308,6 +308,25 @@ RSpec.describe Groups::UpdateService do
     end
   end
 
+  context 'changes allowing subgroups to establish own 2FA' do
+    let(:group) { create(:group) }
+    let(:params) { { allow_mfa_for_subgroups: false } }
+
+    subject { described_class.new(group, user, params).execute }
+
+    it 'changes settings' do
+      subject
+
+      expect(group.namespace_settings.reload.allow_mfa_for_subgroups).to eq(false)
+    end
+
+    it 'enqueues update subgroups and its members' do
+      expect(DisallowTwoFactorForSubgroupsWorker).to receive(:perform_async).with(group.id)
+
+      subject
+    end
+  end
+
   def update_group(group, user, opts)
     Groups::UpdateService.new(group, user, opts).execute
   end

spec/workers/disallow_two_factor_for_group_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe DisallowTwoFactorForGroupWorker do
+  let_it_be(:group) { create(:group, require_two_factor_authentication: true) }
+  let_it_be(:user) { create(:user, require_two_factor_authentication_from_group: true) }
+
+  it "updates group" do
+    described_class.new.perform(group.id)
+
+    expect(group.reload.require_two_factor_authentication).to eq(false)
+  end
+
+  it "updates group members" do
+    group.add_user(user, GroupMember::DEVELOPER)
+
+    described_class.new.perform(group.id)
+
+    expect(user.reload.require_two_factor_authentication_from_group).to eq(false)
+  end
+end

spec/workers/disallow_two_factor_for_subgroups_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe DisallowTwoFactorForSubgroupsWorker do
+  let_it_be(:group) { create(:group) }
+  let_it_be(:subgroup_with_2fa) { create(:group, parent: group, require_two_factor_authentication: true) }
+  let_it_be(:subgroup_without_2fa) { create(:group, parent: group, require_two_factor_authentication: false) }
+  let_it_be(:subsubgroup_with_2fa) { create(:group, parent: subgroup_with_2fa, require_two_factor_authentication: true) }
+
+  it "schedules updating subgroups" do
+    expect(DisallowTwoFactorForGroupWorker).to receive(:perform_in).with(0, subgroup_with_2fa.id)
+    expect(DisallowTwoFactorForGroupWorker).to receive(:perform_in).with(2, subsubgroup_with_2fa.id)
+
+    described_class.new.perform(group.id)
+  end
+end