Merge branch '35308-list-connected-group-saml-identities' into 'master'

Adjust group members API to include group SAML info

Closes #35308

See merge request gitlab-org/gitlab!20357

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')