Merge branch 'jej/group-saml-sso-badges' into 'master'

Group SAML status badges on members page

Closes #4515

See merge request gitlab-org/gitlab-ee!5807

app/controllers/concerns/members_presentation.rb

@@ -12,12 +12,7 @@ module MembersPresentation
     ).fabricate!
   end
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def preload_associations(members)
-    ActiveRecord::Associations::Preloader.new.preload(members, :user)
-    ActiveRecord::Associations::Preloader.new.preload(members, :source)
-    ActiveRecord::Associations::Preloader.new.preload(members.map(&:user), :status)
-    ActiveRecord::Associations::Preloader.new.preload(members.map(&:user), :u2f_registrations)
+    MembersPreloader.new(members).preload_all
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 end

app/models/members_preloader.rb

+# frozen_string_literal: true
+
+class MembersPreloader
+  prepend EE::MembersPreloader
+
+  attr_reader :members
+
+  def initialize(members)
+    @members = members
+  end
+
+  def preload_all
+    ActiveRecord::Associations::Preloader.new.preload(members, :user)
+    ActiveRecord::Associations::Preloader.new.preload(members, :source)
+    ActiveRecord::Associations::Preloader.new.preload(members.map(&:user), :status)
+    ActiveRecord::Associations::Preloader.new.preload(members.map(&:user), :u2f_registrations)
+  end
+end

app/views/shared/members/_member.html.haml

@@ -14,6 +14,8 @@
         = user_status(user)
         %span.cgray= user.to_reference
 
+        = render_if_exists 'shared/members/ee/sso_badge', member: member
+
         - if user == current_user
           %span.badge.badge-success.prepend-left-5 It's you
 

ee/app/models/ee/members_preloader.rb

+# frozen_string_literal: true
+
+module EE
+  module MembersPreloader
+    extend ::Gitlab::Utils::Override
+
+    override :preload_all
+    def preload_all
+      super
+
+      ActiveRecord::Associations::Preloader.new.preload(members.map(&:user), group_saml_identities: :saml_provider)
+    end
+  end
+end

ee/app/models/ee/user.rb

@@ -38,6 +38,8 @@ module EE
       has_many :users_ops_dashboard_projects
       has_many :ops_dashboard_projects, through: :users_ops_dashboard_projects, source: :project
 
+      has_many :group_saml_identities, -> { where.not(saml_provider_id: nil) }, source: :identities, class_name: ::Identity
+
       # Protected Branch Access
       has_many :protected_branch_merge_access_levels, dependent: :destroy, class_name: ::ProtectedBranch::MergeAccessLevel # rubocop:disable Cop/ActiveRecordDependent
       has_many :protected_branch_push_access_levels, dependent: :destroy, class_name: ::ProtectedBranch::PushAccessLevel # rubocop:disable Cop/ActiveRecordDependent
@@ -185,5 +187,15 @@ module EE
     def admin_unsubscribe!
       update_column :admin_email_unsubscribed_at, Time.now
     end
+
+    def group_sso?(group)
+      return false unless group
+
+      if group_saml_identities.loaded?
+        group_saml_identities.any? { |identity| identity.saml_provider.group_id == group.id }
+      else
+        group_saml_identities.where(saml_provider: group.saml_provider).any?
+      end
+    end
   end
 end

ee/app/presenters/ee/group_member_presenter.rb

 module EE
   module GroupMemberPresenter
+    def group_sso?
+      member.user.group_sso?(source)
+    end
+
     private
 
     def override_member_permission

ee/app/presenters/ee/project_member_presenter.rb

 module EE
   module ProjectMemberPresenter
+    def group_sso?
+      false
+    end
+
     private
 
     def override_member_permission

ee/app/views/shared/members/ee/_sso_badge.html.haml

+- if member.group_sso?
+  %span.badge.badge-info.prepend-left-5 SAML

ee/changelogs/unreleased/jej-group-saml-sso-badges.yml

+---
+title: Group SAML status badges on members page
+merge_request: 5807
+author:
+type: changed

ee/spec/features/groups/members/list_members_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe 'Groups > Members > List members' do
+  let(:user1) { create(:user, name: 'John Doe') }
+  let(:user2) { create(:user, name: 'Mary Jane') }
+  let(:group) { create(:group) }
+
+  before do
+    group.add_developer(user1)
+
+    sign_in(user1)
+  end
+
+  context 'with Group SAML identity linked for a user' do
+    let(:saml_provider) { create(:saml_provider) }
+    let(:group) { saml_provider.group }
+
+    before do
+      group.add_guest(user2)
+      user2.identities.create!(provider: :group_saml,
+                               saml_provider: saml_provider,
+                               extern_uid: 'user2@example.com')
+    end
+
+    it 'shows user with SSO status badge' do
+      visit group_group_members_path(group)
+
+      member = GroupMember.find_by(user: user2, group: group)
+      expect(find("#group_member_#{member.id}").find('.badge-info')).to have_content('SAML')
+    end
+  end
+end

ee/spec/models/ee/members_preloader_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe EE::MembersPreloader do
+  describe '#preload_all' do
+    let(:group) { create(:group) }
+    let(:saml_provider) { create(:saml_provider, group: group) }
+
+    def group_sso_with_preload(members)
+      MembersPreloader.new(members).preload_all
+      MembersPresenter.new(members, current_user: nil).map(&:group_sso?)
+    end
+
+    it 'preloads SAML identities to avoid N+1 queries in MembersPresenter' do
+      member = create(:group_member, group: group)
+      create(:identity, :group_saml, user: member.user, saml_provider: saml_provider)
+      control = ActiveRecord::QueryRecorder.new { group_sso_with_preload([member]) }
+
+      members = create_list(:group_member, 3, group: group)
+      create(:identity, :group_saml, user: members.first.user, saml_provider: saml_provider)
+      create(:identity, :group_saml, user: members.last.user, saml_provider: saml_provider)
+
+      expect { group_sso_with_preload(members) }.not_to exceed_query_limit(control)
+    end
+  end
+end

ee/spec/models/ee/user_spec.rb

@@ -236,4 +236,47 @@ describe EE::User do
       end
     end
   end
+
+  describe '#group_sso?' do
+    subject(:user) { create(:user) }
+
+    it 'is false without a saml_provider' do
+      expect(subject.group_sso?(nil)).to be_falsey
+      expect(subject.group_sso?(create(:group))).to be_falsey
+    end
+
+    context 'with linked identity' do
+      let!(:identity) { create(:identity, :group_saml, user: user) }
+      let(:saml_provider) { identity.saml_provider }
+      let(:group) { saml_provider.group }
+
+      context 'without preloading' do
+        it 'returns true' do
+          expect(subject.group_sso?(group)).to be_truthy
+        end
+
+        it 'does not cause ActiveRecord to loop through identites' do
+          create(:identity, :group_saml, user: user)
+
+          expect(Identity).not_to receive(:instantiate)
+
+          subject.group_sso?(group)
+        end
+      end
+
+      context 'when identities and saml_providers pre-loaded' do
+        before do
+          ActiveRecord::Associations::Preloader.new.preload(subject, group_saml_identities: :saml_provider)
+        end
+
+        it 'returns true' do
+          expect(subject.group_sso?(group)).to be_truthy
+        end
+
+        it 'does not trigger additional database queries' do
+          expect { subject.group_sso?(group) }.not_to exceed_query_limit(0)
+        end
+      end
+    end
+  end
 end

ee/spec/presenters/group_member_presenter_spec.rb

@@ -3,9 +3,20 @@ require 'spec_helper'
 describe GroupMemberPresenter do
   let(:user) { double(:user) }
   let(:group) { double(:group) }
-  let(:group_member) { double(:group_member, source: group) }
+  let(:group_member) { double(:group_member, source: group, user: user) }
   let(:presenter) { described_class.new(group_member, current_user: user) }
 
+  describe '#group_sso?' do
+    let(:saml_provider) { double(:saml_provider) }
+    let(:group) { double(:group) }
+
+    it 'calls through to User#group_sso?' do
+      expect(user).to receive(:group_sso?).with(group).and_return(true)
+
+      expect(presenter.group_sso?).to eq true
+    end
+  end
+
   describe '#can_update?' do
     context 'when user cannot update_group_member but can override_group_member' do
       before do

spec/factories/identities.rb

@@ -2,5 +2,11 @@ FactoryBot.define do
   factory :identity do
     provider 'ldapmain'
     extern_uid 'my-ldap-id'
+
+    trait :group_saml do
+      provider 'group_saml'
+      extern_uid { generate(:username) }
+      saml_provider
+    end
   end
 end