Adjust group members API to include group saml info

If a group has SAML provider configured and enabled
we expose group SAML identities for group members
to group owners. It should be used at members page
to display group SAML identity for each member

ee/app/models/ee/group_member.rb

@@ -45,6 +45,16 @@ module EE
       errors.add(:invite_email, email_no_match_email_domain(invite_email))
     end
 
+    def group_saml_identity
+      return unless source.saml_provider
+
+      if user.group_saml_identities.loaded?
+        user.group_saml_identities.detect { |i| i.saml_provider_id == source.saml_provider.id }
+      else
+        user.group_saml_identities.find_by(saml_provider: source.saml_provider)
+      end
+    end
+
     private
 
     def email_no_match_email_domain(email)

ee/app/policies/ee/group_policy.rb

@@ -45,6 +45,10 @@ module EE
         @subject.feature_available?(:cluster_deployments)
       end
 
+      condition(:group_saml_enabled) do
+        @subject.saml_provider&.enabled?
+      end
+
       rule { reporter }.policy do
         enable :admin_list
         enable :admin_board
@@ -135,6 +139,10 @@ module EE
       rule { ip_enforcement_prevents_access & ~owner }.policy do
         prevent :read_group
       end
+
+      rule { owner & group_saml_enabled }.policy do
+        enable :read_group_saml_identity
+      end
     end
 
     override :lookup_access_level!

ee/changelogs/unreleased/35308-list-connected-group-saml-identities.yml

+---
+title: Adjust group members API to include group SAML info
+merge_request: 20357
+author:
+type: added

ee/lib/ee/api/entities.rb

@@ -79,6 +79,16 @@ module EE
         end
       end
 
+      module Member
+        extend ActiveSupport::Concern
+
+        prepended do
+          expose :group_saml_identity,
+                 using: ::API::Entities::Identity,
+                 if:  -> (member, options) { Ability.allowed?(options[:current_user], :read_group_saml_identity, member.source) }
+        end
+      end
+
       module ProtectedRefAccess
         extend ActiveSupport::Concern
 

ee/lib/ee/api/members.rb

+# frozen_string_literal: true
+
+module EE
+  module API
+    module Members
+      extend ActiveSupport::Concern
+
+      prepended do
+        helpers do
+          # rubocop: disable CodeReuse/ActiveRecord
+          def retrieve_members(source, *args)
+            super.tap do |members|
+              members.includes(user: :group_saml_identities) if can_view_group_identity?(source)
+            end
+          end
+          # rubocop: enable CodeReuse/ActiveRecord
+
+          def can_view_group_identity?(members_source)
+            can?(current_user, :read_group_saml_identity, members_source)
+          end
+        end
+      end
+    end
+  end
+end

ee/spec/lib/ee/api/entities/member_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe API::Entities::Member do
+  subject(:entity_representation) { described_class.new(member).as_json }
+
+  let(:member) { build_stubbed(:group_member) }
+  let(:group_saml_identity) { build_stubbed(:group_saml_identity, extern_uid: 'TESTIDENTITY') }
+
+  before do
+    allow(member).to receive(:group_saml_identity).and_return(group_saml_identity)
+  end
+
+  context 'when current user is allowed to read group saml identity' do
+    before do
+      allow(Ability).to receive(:allowed?).with(anything, :read_group_saml_identity, member.source).and_return(true)
+    end
+
+    it 'exposes group_saml_identity' do
+      expect(entity_representation[:group_saml_identity]).to include(extern_uid: 'TESTIDENTITY')
+    end
+  end
+
+  context 'when current user is not allowed to read group saml identity' do
+    before do
+      allow(Ability).to receive(:allowed?).with(anything, :read_group_saml_identity, member.source).and_return(false)
+    end
+
+    it 'does not expose group saml identity' do
+      expect(entity_representation.keys).not_to include(:group_saml_identity)
+    end
+  end
+end

ee/spec/models/group_member_spec.rb

@@ -9,8 +9,8 @@ describe GroupMember do
   describe 'validations' do
     describe 'group domain limitations' do
       let(:group) { create(:group) }
-      let(:user) { create(:user, email: 'test@gitlab.com')}
-      let(:user_2) { create(:user, email: 'test@gmail.com')}
+      let(:user) { create(:user, email: 'test@gitlab.com') }
+      let(:user_2) { create(:user, email: 'test@gmail.com') }
 
       before do
         create(:allowed_email_domain, group: group)
@@ -59,4 +59,40 @@ describe GroupMember do
       end
     end
   end
+
+  describe '#group_saml_identity' do
+    subject(:group_saml_identity) { member.group_saml_identity }
+
+    let!(:member) { create :group_member }
+
+    context 'without saml_provider' do
+      it { is_expected.to eq nil }
+    end
+
+    context 'with saml_provider enabled' do
+      let!(:saml_provider) { create(:saml_provider, group: member.group) }
+
+      context 'when member has no connected identity' do
+        it { is_expected.to eq nil }
+      end
+
+      context 'when member has connected identity' do
+        let!(:group_related_identity) do
+          create(:group_saml_identity, user: member.user, saml_provider: saml_provider)
+        end
+
+        it 'returns related identity' do
+          expect(group_saml_identity).to eq group_related_identity
+        end
+      end
+
+      context 'when member has connected identity of different group' do
+        before do
+          create(:group_saml_identity, user: member.user)
+        end
+
+        it { is_expected.to eq nil }
+      end
+    end
+  end
 end

ee/spec/policies/group_policy_spec.rb

@@ -445,4 +445,30 @@ describe GroupPolicy do
   describe 'view_type_of_work_charts' do
     include_examples 'analytics policy', :view_type_of_work_charts
   end
+
+  describe '#read_group_saml_identity' do
+    let_it_be(:saml_provider) { create(:saml_provider, group: group, enabled: true) }
+
+    context 'for owner' do
+      let(:current_user) { owner }
+
+      it { is_expected.to be_allowed(:read_group_saml_identity) }
+
+      context 'without Group SAML enabled' do
+        before do
+          saml_provider.update(enabled: false)
+        end
+
+        it { is_expected.to be_disallowed(:read_group_saml_identity) }
+      end
+    end
+
+    %w[maintainer developer reporter guest].each do |role|
+      context "for #{role}" do
+        let(:current_user) { public_send(role) }
+
+        it { is_expected.to be_disallowed(:read_group_saml_identity) }
+      end
+    end
+  end
 end

ee/spec/requests/api/members_spec.rb

@@ -3,18 +3,19 @@
 require 'spec_helper'
 
 describe API::Members do
-  let(:user) { create(:user) }
+  let(:group) { create(:group) }
+  let(:owner) { create(:user) }
+
+  before do
+    group.add_owner(owner)
+  end
 
   describe 'POST /projects/:id/members' do
     context 'group membership locked' do
-      let(:owner) { create(:user) }
+      let(:user) { create(:user) }
       let(:group) { create(:group, membership_lock: true)}
       let(:project) { create(:project, group: group) }
 
-      before do
-        group.add_owner(owner)
-      end
-
       context 'project in a group' do
         it 'returns a 405 method not allowed error when group membership lock is enabled' do
           post api("/projects/#{project.id}/members", owner),
@@ -25,4 +26,37 @@ describe API::Members do
       end
     end
   end
+
+  describe 'GET /groups/:id/members' do
+    context 'when a group has SAML provider configured' do
+      before do
+        saml_provider = create :saml_provider, group: group
+        create :group_saml_identity, user: owner, saml_provider: saml_provider
+      end
+
+      context 'and current_user is group owner' do
+        it 'returns a list of users with group SAML identities info' do
+          get api("/groups/#{group.to_param}/members", owner)
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(json_response.first['group_saml_identity']).to match(kind_of(Hash))
+        end
+      end
+
+      context 'and current_user is not an owner' do
+        let(:maintainer) do
+          create(:user).tap do |user|
+            group.add_maintainer(user)
+          end
+        end
+
+        it 'returns a list of users with group SAML identities info' do
+          get api("/groups/#{group.to_param}/members", maintainer)
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(json_response.map(&:keys).flatten).not_to include('group_saml_identity')
+        end
+      end
+    end
+  end
 end

lib/api/entities.rb

@@ -1838,6 +1838,7 @@ end
 ::API::Entities::Issue.prepend_if_ee('EE::API::Entities::Issue')
 ::API::Entities::List.prepend_if_ee('EE::API::Entities::List')
 ::API::Entities::MergeRequestBasic.prepend_if_ee('EE::API::Entities::MergeRequestBasic', with_descendants: true)
+::API::Entities::Member.prepend_if_ee('EE::API::Entities::Member', with_descendants: true)
 ::API::Entities::Namespace.prepend_if_ee('EE::API::Entities::Namespace')
 ::API::Entities::Project.prepend_if_ee('EE::API::Entities::Project', with_descendants: true)
 ::API::Entities::ProtectedRefAccess.prepend_if_ee('EE::API::Entities::ProtectedRefAccess')

lib/api/helpers/members_helpers.rb

@@ -13,10 +13,19 @@ module API
         authorize! :"admin_#{source_type}", source
       end
 
-      def find_all_members(source_type, source)
-        members = source_type == 'project' ? find_all_members_for_project(source) : find_all_members_for_group(source)
-        members.non_invite
-          .non_request
+      # rubocop: disable CodeReuse/ActiveRecord
+      def retrieve_members(source, params:, deep: false)
+        members = deep ? find_all_members(source) : source.members.where.not(user_id: nil)
+        members = members.includes(:user)
+        members = members.references(:user).merge(User.search(params[:query])) if params[:query].present?
+        members = members.where(user_id: params[:user_ids]) if params[:user_ids].present?
+        members
+      end
+      # rubocop: enable CodeReuse/ActiveRecord
+
+      def find_all_members(source)
+        members = source.is_a?(Project) ? find_all_members_for_project(source) : find_all_members_for_group(source)
+        members.non_invite.non_request
       end
 
       def find_all_members_for_project(project)
@@ -26,6 +35,10 @@ module API
       def find_all_members_for_group(group)
         GroupMembersFinder.new(group).execute
       end
+
+      def present_members(members)
+        present members, with: Entities::Member, current_user: current_user
+      end
     end
   end
 end

lib/api/members.rb

@@ -21,18 +21,14 @@ module API
           optional :user_ids, type: Array[Integer], desc: 'Array of user ids to look up for membership'
           use :pagination
         end
-        # rubocop: disable CodeReuse/ActiveRecord
+
         get ":id/members" do
           source = find_source(source_type, params[:id])
 
-          members = source.members.where.not(user_id: nil).includes(:user)
-          members = members.joins(:user).merge(User.search(params[:query])) if params[:query].present?
-          members = members.where(user_id: params[:user_ids]) if params[:user_ids].present?
-          members = paginate(members)
+          members = paginate(retrieve_members(source, params: params))
 
-          present members, with: Entities::Member
+          present_members members
         end
-        # rubocop: enable CodeReuse/ActiveRecord
 
         desc 'Gets a list of group or project members viewable by the authenticated user, including those who gained membership through ancestor group.' do
           success Entities::Member
@@ -42,18 +38,14 @@ module API
           optional :user_ids, type: Array[Integer], desc: 'Array of user ids to look up for membership'
           use :pagination
         end
-        # rubocop: disable CodeReuse/ActiveRecord
+
         get ":id/members/all" do
           source = find_source(source_type, params[:id])
 
-          members = find_all_members(source_type, source)
-          members = members.includes(:user).references(:user).merge(User.search(params[:query])) if params[:query].present?
-          members = members.where(user_id: params[:user_ids]) if params[:user_ids].present?
-          members = paginate(members)
+          members = paginate(retrieve_members(source, params: params, deep: true))
 
-          present members, with: Entities::Member
+          present_members members
         end
-        # rubocop: enable CodeReuse/ActiveRecord
 
         desc 'Gets a member of a group or project.' do
           success Entities::Member
@@ -68,7 +60,7 @@ module API
           members = source.members
           member = members.find_by!(user_id: params[:user_id])
 
-          present member, with: Entities::Member
+          present_members member
         end
         # rubocop: enable CodeReuse/ActiveRecord
 
@@ -82,10 +74,10 @@ module API
         get ":id/members/all/:user_id" do
           source = find_source(source_type, params[:id])
 
-          members = find_all_members(source_type, source)
+          members = find_all_members(source)
           member = members.find_by!(user_id: params[:user_id])
 
-          present member, with: Entities::Member
+          present_members member
         end
         # rubocop: enable CodeReuse/ActiveRecord
 
@@ -113,7 +105,7 @@ module API
           if !member
             not_allowed! # This currently can only be reached in EE
           elsif member.persisted? && member.valid?
-            present member, with: Entities::Member
+            present_members member
           else
             render_validation_error!(member)
           end
@@ -140,7 +132,7 @@ module API
               .execute(member)
 
           if updated_member.valid?
-            present updated_member, with: Entities::Member
+            present_members updated_member
           else
             render_validation_error!(updated_member)
           end
@@ -165,3 +157,5 @@ module API
     end
   end
 end
+
+API::Members.prepend_if_ee('EE::API::Members')