Commit 9f8777d1 authored by Drew Blessing's avatar Drew Blessing

Add SAML default membership asynchronously

Previously when a new SAML user signed in the default membership
was added synchronously. This caused performance issues for very
large groups as the project authorizations were inserted for
each project. The default membership is now managed asynchronously.
There's a small chance users won't see all projects in their list
immediately upon first sign in, but the Sidekiq jobs have high
priority and should finish quickly.

Changelog: fixed
parent 40b78d62
......@@ -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)
......
......@@ -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')
......@@ -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.
......
---
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
......@@ -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
......
......@@ -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)
......
......@@ -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
......
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment