Merge branch 'dblessing_trigger_saml_group_sync' into 'master'

Trigger Group SAML Group Sync on sign-in

See merge request gitlab-org/gitlab!46700

ee/app/models/saml_group_link.rb

@@ -9,6 +9,7 @@ class SamlGroupLink < ApplicationRecord
   validates :saml_group_name, presence: true, uniqueness: { scope: [:group_id] }, length: { maximum: 255 }
 
   scope :by_id_and_group_id, ->(id, group_id) { where(id: id, group_id: group_id) }
+  scope :by_saml_group_name, -> (name) { where(saml_group_name: name) }
   scope :by_group_id, ->(group_id) { where(group_id: group_id) }
   scope :preload_group, -> { preload(group: :route) }
 end

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

+# frozen_string_literal: true
+
+module Gitlab
+  module Auth
+    module GroupSaml
+      class AuthHash < Gitlab::Auth::Saml::AuthHash
+        def groups
+          Array.wrap(get_raw('groups') || get_raw('Groups'))
+        end
+      end
+    end
+  end
+end

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

@@ -31,7 +31,7 @@ module Gitlab
         # rubocop: enable CodeReuse/ActiveRecord
 
         def update_group_membership
-          MembershipUpdater.new(current_user, saml_provider).execute
+          MembershipUpdater.new(current_user, saml_provider, oauth).execute
         end
       end
     end

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

@@ -4,16 +4,26 @@ module Gitlab
   module Auth
     module GroupSaml
       class MembershipUpdater
-        attr_reader :user, :saml_provider
+        include Gitlab::Utils::StrongMemoize
+
+        attr_reader :user, :saml_provider, :auth_hash
 
         delegate :group, :default_membership_role, to: :saml_provider
 
-        def initialize(user, saml_provider)
+        def initialize(user, saml_provider, auth_hash)
           @user = user
           @saml_provider = saml_provider
+          @auth_hash = AuthHash.new(auth_hash)
         end
 
         def execute
+          update_default_membership
+          enqueue_group_sync if sync_groups?
+        end
+
+        private
+
+        def update_default_membership
           return if group.member?(user)
 
           member = group.add_user(user, default_membership_role)
@@ -21,7 +31,32 @@ module Gitlab
           log_audit_event(member: member)
         end
 
-        private
+        def enqueue_group_sync
+          GroupSamlGroupSyncWorker.perform_async(user.id, group.id, group_link_ids)
+        end
+
+        def sync_groups?
+          return false unless user && group.saml_group_sync_available?
+
+          group_names_from_saml.any? && group_link_ids.any?
+        end
+
+        # rubocop:disable CodeReuse/ActiveRecord
+        def group_link_ids
+          strong_memoize(:group_link_ids) do
+            SamlGroupLink
+              .by_saml_group_name(group_names_from_saml)
+              .by_group_id(group.self_and_descendants.select(:id))
+              .pluck(:id)
+          end
+        end
+        # rubocop:enable CodeReuse/ActiveRecord
+
+        def group_names_from_saml
+          strong_memoize(:group_names_from_saml) do
+            auth_hash.groups
+          end
+        end
 
         def log_audit_event(member:)
           ::AuditEventService.new(

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

@@ -38,7 +38,7 @@ module Gitlab
         def update_group_membership
           return unless user_from_identity
 
-          MembershipUpdater.new(user_from_identity, saml_provider).execute
+          MembershipUpdater.new(user_from_identity, saml_provider, auth_hash).execute
         end
       end
     end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Auth::GroupSaml::AuthHash do
+  let(:raw_info_attr) { { group_attribute => %w(Developers Owners) } }
+  let(:omniauth_auth_hash) do
+    OmniAuth::AuthHash.new(extra: { raw_info: OneLogin::RubySaml::Attributes.new(raw_info_attr) } )
+  end
+
+  subject(:saml_auth_hash) { described_class.new(omniauth_auth_hash) }
+
+  describe '#groups' do
+    context 'with a lowercase groups attribute' do
+      let(:group_attribute) { 'groups' }
+
+      it 'returns array of groups' do
+        expect(saml_auth_hash.groups).to eq(%w(Developers Owners))
+      end
+    end
+
+    context 'with a capitalized Groups attribute' do
+      let(:group_attribute) { 'Groups' }
+
+      it 'returns array of groups' do
+        expect(saml_auth_hash.groups).to eq(%w(Developers Owners))
+      end
+    end
+
+    context 'when no groups are present in the auth hash' do
+      let(:raw_info_attr) { {} }
+
+      it 'returns an empty array' do
+        expect(saml_auth_hash.groups).to match_array([])
+      end
+    end
+  end
+end

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

@@ -6,8 +6,13 @@ RSpec.describe Gitlab::Auth::GroupSaml::MembershipUpdater do
   let(:user) { create(:user) }
   let(:saml_provider) { create(:saml_provider, default_membership_role: Gitlab::Access::DEVELOPER) }
   let(:group) { saml_provider.group }
+  let(:omniauth_auth_hash) do
+    OmniAuth::AuthHash.new(extra: {
+      raw_info: OneLogin::RubySaml::Attributes.new('groups' => %w(Developers Owners))
+    })
+  end
 
-  subject { described_class.new(user, saml_provider).execute }
+  subject(:update_membership) { described_class.new(user, saml_provider, omniauth_auth_hash).execute }
 
   it 'adds the user to the group' do
     subject
@@ -45,4 +50,53 @@ RSpec.describe Gitlab::Auth::GroupSaml::MembershipUpdater do
 
     expect(AuditEvent.last.details).to include(add: 'user_access', target_details: user.name, as: 'Developer')
   end
+
+  it 'does not enqueue group sync' do
+    expect(GroupSamlGroupSyncWorker).not_to receive(:perform_async)
+
+    update_membership
+  end
+
+  context 'when SAML group links exist' do
+    def stub_saml_group_sync_available(enabled)
+      allow(group).to receive(:saml_group_sync_available?).and_return(enabled)
+    end
+
+    let(:group_link) { create(:saml_group_link, saml_group_name: 'Owners', group: group) }
+    let(:subgroup_link) { create(:saml_group_link, saml_group_name: 'Developers', group: create(:group, parent: group)) }
+
+    context 'when group sync is not available' do
+      before do
+        stub_saml_group_sync_available(false)
+      end
+
+      it 'does not enqueue group sync' do
+        expect(GroupSamlGroupSyncWorker).not_to receive(:perform_async)
+      end
+    end
+
+    context 'when group sync is available' do
+      before do
+        stub_saml_group_sync_available(true)
+      end
+
+      it 'enqueues group sync' do
+        expect(GroupSamlGroupSyncWorker).to receive(:perform_async).with(user.id, group.id, match_array([group_link.id, subgroup_link.id]))
+
+        update_membership
+      end
+
+      context 'with a group link outside the top-level group' do
+        before do
+          create(:saml_group_link, saml_group_name: 'Developers', group: create(:group))
+        end
+
+        it 'enqueues group sync without the outside group' do
+          expect(GroupSamlGroupSyncWorker).to receive(:perform_async).with(user.id, group.id, match_array([group_link.id, subgroup_link.id]))
+
+          update_membership
+        end
+      end
+    end
+  end
 end

ee/spec/models/saml_group_link_spec.rb

@@ -50,4 +50,26 @@ RSpec.describe SamlGroupLink do
       end
     end
   end
+
+  describe '.by_saml_group_name' do
+    let_it_be(:group) { create(:group) }
+    let_it_be(:group_link) { create(:saml_group_link, group: group) }
+
+    it 'finds the group link' do
+      results = described_class.by_saml_group_name(group_link.saml_group_name)
+
+      expect(results).to match_array([group_link])
+    end
+
+    context 'with multiple groups and group links' do
+      let_it_be(:group2) { create(:group) }
+      let_it_be(:group_link2) { create(:saml_group_link, group: group2) }
+
+      it 'finds group links within the given groups' do
+        results = described_class.by_saml_group_name([group_link.saml_group_name, group_link2.saml_group_name])
+
+        expect(results).to match_array([group_link, group_link2])
+      end
+    end
+  end
 end

ee/spec/services/group_saml/identity/destroy_service_spec.rb

@@ -8,11 +8,7 @@ RSpec.describe GroupSaml::Identity::DestroyService do
   subject { described_class.new(identity) }
 
   before do
-    link_group_membership
-  end
-
-  def link_group_membership
-    Gitlab::Auth::GroupSaml::MembershipUpdater.new(identity.user, identity.saml_provider).execute
+    identity.saml_provider.group.add_guest(identity.user)
   end
 
   it "prevents future Group SAML logins" do

ee/spec/services/groups/sync_service_spec.rb

@@ -179,6 +179,61 @@ RSpec.describe Groups::SyncService do
             .to eq(::Gitlab::Access::REPORTER)
         end
       end
+
+      context 'but should no longer be a member' do
+        shared_examples 'removes the member' do
+          before do
+            group2.add_user(user, ::Gitlab::Access::DEVELOPER)
+          end
+
+          it 'reduces group member count by 1' do
+            expect { sync }.to change { group2.members.count }.by(-1)
+          end
+
+          it 'removes the matching user' do
+            sync
+
+            expect(group2.members.pluck(:user_id)).not_to include(user.id)
+          end
+        end
+
+        context 'when manage_group_ids is present' do
+          let_it_be(:manage_group_ids) { [group2.id] }
+
+          include_examples 'removes the member'
+        end
+
+        context 'when manage_group_ids is empty' do
+          let_it_be(:manage_group_ids) { [] }
+
+          include_examples 'removes the member'
+        end
+
+        context 'when manage_groups_ids is nil' do
+          let_it_be(:manage_group_ids) { nil }
+
+          include_examples 'removes the member'
+        end
+      end
+
+      context 'in a group that is not managed' do
+        let_it_be(:manage_group_ids) { [top_level_group.id, group1.id] }
+
+        before do
+          group2.add_user(user, ::Gitlab::Access::REPORTER)
+        end
+
+        it 'does not change the group member count' do
+          expect { sync }.not_to change { group2.members.count }
+        end
+
+        it 'retains the correct access level' do
+          sync
+
+          expect(group2.members.find_by(user_id: user.id).access_level)
+            .to eq(::Gitlab::Access::REPORTER)
+        end
+      end
     end
   end
 end