Merge branch 'dblessing_group_saml_controller_performance' into 'master'

Async SAML membership updates

See merge request gitlab-org/gitlab!83619

app/models/group.rb

@@ -351,14 +351,16 @@ class Group < Namespace
     )
   end
 
-  def add_user(user, access_level, current_user: nil, expires_at: nil, ldap: false)
-    Members::Groups::CreatorService.new(self, # rubocop:disable CodeReuse/ServiceClass
-                                        user,
-                                        access_level,
-                                        current_user: current_user,
-                                        expires_at: expires_at,
-                                        ldap: ldap)
-                                   .execute
+  def add_user(user, access_level, current_user: nil, expires_at: nil, ldap: false, blocking_refresh: true)
+    Members::Groups::CreatorService.new( # rubocop:disable CodeReuse/ServiceClass
+      self,
+      user,
+      access_level,
+      current_user: current_user,
+      expires_at: expires_at,
+      ldap: ldap,
+      blocking_refresh: blocking_refresh
+    ).execute
   end
 
   def add_guest(user, current_user = nil)

app/models/member.rb

@@ -22,6 +22,7 @@ class Member < ApplicationRecord
   STATE_AWAITING = 1
 
   attr_accessor :raw_invite_token
+  attr_writer :blocking_refresh
 
   belongs_to :created_by, class_name: "User"
   belongs_to :user
@@ -201,7 +202,7 @@ class Member < ApplicationRecord
   after_save :log_invitation_token_cleanup
 
   after_commit on: [:create, :update], unless: :importing? do
-    refresh_member_authorized_projects(blocking: true)
+    refresh_member_authorized_projects(blocking: blocking_refresh)
   end
 
   after_commit on: [:destroy], unless: :importing? do
@@ -513,6 +514,13 @@ class Member < ApplicationRecord
     error = StandardError.new("Invitation token is present but invite was already accepted!")
     Gitlab::ErrorTracking.track_exception(error, attributes.slice(%w["invite_accepted_at created_at source_type source_id user_id id"]))
   end
+
+  def blocking_refresh
+    return true unless Feature.enabled?(:allow_non_blocking_member_refresh, default_enabled: :yaml)
+    return true if @blocking_refresh.nil?
+
+    @blocking_refresh
+  end
 end
 
 Member.prepend_mod_with('Member')

app/services/members/creator_service.rb

@@ -101,6 +101,8 @@ module Members
                 else
                   source.members.build(invite_email: user)
                 end
+
+      @member.blocking_refresh = args[:blocking_refresh]
     end
 
     # This method is used to find users that have been entered into the "Add members" field.

config/feature_flags/development/allow_non_blocking_member_refresh.yml

+---
+name: allow_non_blocking_member_refresh
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/83619
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/357281
+milestone: '14.10'
+type: development
+group: group::authentication and authorization
+default_enabled: false

ee/lib/gitlab/auth/group_saml/membership_updater.rb

@@ -26,7 +26,7 @@ module Gitlab
         def update_default_membership
           return if group.member?(user)
 
-          member = group.add_user(user, default_membership_role)
+          member = group.add_user(user, default_membership_role, blocking_refresh: false)
 
           log_audit_event(member: member)
         end

ee/spec/lib/gitlab/auth/group_saml/membership_updater_spec.rb

@@ -23,7 +23,9 @@ RSpec.describe Gitlab::Auth::GroupSaml::MembershipUpdater do
   end
 
   it 'adds the member with the specified `default_membership_role`' do
-    subject
+    expect(group).to receive(:add_user).with(user, Gitlab::Access::DEVELOPER, blocking_refresh: false).and_call_original
+
+    update_membership
 
     created_member = group.members.find_by(user: user)
     expect(created_member.access_level).to eq(Gitlab::Access::DEVELOPER)

spec/models/group_spec.rb

@@ -843,11 +843,23 @@ RSpec.describe Group do
   describe '#add_user' do
     let(:user) { create(:user) }
 
-    before do
+    it 'adds the user with a blocking refresh by default' do
+      expect_next_instance_of(GroupMember) do |member|
+        expect(member).to receive(:refresh_member_authorized_projects).with(blocking: true)
+      end
+
       group.add_user(user, GroupMember::MAINTAINER)
+
+      expect(group.group_members.maintainers.map(&:user)).to include(user)
     end
 
-    it { expect(group.group_members.maintainers.map(&:user)).to include(user) }
+    it 'passes the blocking refresh value to member' do
+      expect_next_instance_of(GroupMember) do |member|
+        expect(member).to receive(:refresh_member_authorized_projects).with(blocking: false)
+      end
+
+      group.add_user(user, GroupMember::MAINTAINER, blocking_refresh: false)
+    end
   end
 
   describe '#add_users' do

spec/models/member_spec.rb

@@ -682,14 +682,46 @@ RSpec.describe Member do
       member.accept_invite!(user)
     end
 
-    it "refreshes user's authorized projects", :delete do
-      project = member.source
+    context 'authorized projects' do
+      let(:project) { member.source }
 
-      expect(user.authorized_projects).not_to include(project)
+      before do
+        expect(user.authorized_projects).not_to include(project)
+      end
 
-      member.accept_invite!(user)
+      it 'successfully completes a blocking refresh', :delete do
+        expect(member).to receive(:refresh_member_authorized_projects).with(blocking: true).and_call_original
+
+        member.accept_invite!(user)
+
+        expect(user.authorized_projects.reload).to include(project)
+      end
+
+      it 'successfully completes a non-blocking refresh', :delete, :sidekiq_inline do
+        member.blocking_refresh = false
+
+        expect(member).to receive(:refresh_member_authorized_projects).with(blocking: false).and_call_original
+
+        member.accept_invite!(user)
 
-      expect(user.authorized_projects.reload).to include(project)
+        expect(user.authorized_projects.reload).to include(project)
+      end
+
+      context 'when the feature flag is disabled' do
+        before do
+          stub_feature_flags(allow_non_blocking_member_refresh: false)
+        end
+
+        it 'successfully completes a blocking refresh', :delete, :sidekiq_inline do
+          member.blocking_refresh = false
+
+          expect(member).to receive(:refresh_member_authorized_projects).with(blocking: true).and_call_original
+
+          member.accept_invite!(user)
+
+          expect(user.authorized_projects.reload).to include(project)
+        end
+      end
     end
 
     it 'does not accept the invite if saving a new user fails' do