Merge branch '266946-minimal-access-members-are-not-listed-in-saml' into 'master'

Resolve "Minimal Access - Members are not listed in SAML"

See merge request gitlab-org/gitlab!46238

changelogs/unreleased/266946-minimal-access-members-are-not-listed-in-saml.yml

+---
+title: Add minimal access users to group members api endpoints
+merge_request: 46238
+author:
+type: changed

ee/lib/ee/api/helpers/members_helpers.rb

@@ -28,6 +28,14 @@ module EE
 
           members
         end
+
+        override :source_members
+        def source_members(source)
+          return super if source.is_a?(Project)
+          return super unless source.minimal_access_role_allowed?
+
+          source.all_group_members
+        end
         # rubocop: enable CodeReuse/ActiveRecord
 
         def can_view_group_identity?(members_source)

ee/spec/requests/api/members_spec.rb

@@ -3,6 +3,117 @@
 require 'spec_helper'
 
 RSpec.describe API::Members do
+  context 'group members endpoints for group with minimal access feature' do
+    let_it_be(:group) { create(:group) }
+    let_it_be(:minimal_access_member) { create(:group_member, :minimal_access, source: group) }
+    let_it_be(:owner) { create(:user) }
+
+    before do
+      group.add_owner(owner)
+    end
+
+    describe "GET /groups/:id/members" do
+      subject do
+        get api("/groups/#{group.id}/members", owner)
+        json_response
+      end
+
+      it 'returns user with minimal access when feature is available' do
+        stub_licensed_features(minimal_access_role: true)
+
+        expect(subject.map { |u| u['id'] }).to match_array [owner.id, minimal_access_member.user_id]
+      end
+
+      it 'does not return user with minimal access when feature is unavailable' do
+        stub_licensed_features(minimal_access_role: false)
+
+        expect(subject.map { |u| u['id'] }).not_to include(minimal_access_member.user_id)
+      end
+    end
+
+    describe 'POST /groups/:id/members' do
+      let(:stranger) { create(:user) }
+
+      subject do
+        post api("/groups/#{group.id}/members", owner),
+             params: { user_id: stranger.id, access_level: Member::MINIMAL_ACCESS }
+      end
+
+      context 'when minimal access role is not available' do
+        it 'does not create a member' do
+          expect do
+            subject
+          end.not_to change { group.all_group_members.count }
+
+          expect(response).to have_gitlab_http_status(:bad_request)
+          expect(json_response['message']).to eq({ 'access_level' => ['is not included in the list'] })
+        end
+      end
+
+      context 'when minimal access role is available' do
+        it 'creates a member' do
+          stub_licensed_features(minimal_access_role: true)
+
+          expect do
+            subject
+          end.to change { group.all_group_members.count }.by(1)
+
+          expect(response).to have_gitlab_http_status(:created)
+          expect(json_response['id']).to eq(stranger.id)
+        end
+      end
+    end
+
+    describe 'PUT /groups/:id/members/:user_id' do
+      let(:expires_at) { 2.days.from_now.to_date }
+
+      context 'when minimal access role is available' do
+        it 'updates the member' do
+          stub_licensed_features(minimal_access_role: true)
+
+          put api("/groups/#{group.id}/members/#{minimal_access_member.user_id}", owner),
+              params: {  expires_at: expires_at, access_level: Member::MINIMAL_ACCESS }
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(json_response['id']).to eq(minimal_access_member.user_id)
+          expect(json_response['expires_at']).to eq(expires_at.to_s)
+        end
+      end
+
+      context 'when minimal access role is not available' do
+        it 'does not update the member' do
+          put api("/groups/#{group.id}/members/#{minimal_access_member.user_id}", owner),
+              params: {  expires_at: expires_at, access_level: Member::MINIMAL_ACCESS }
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+      end
+    end
+
+    describe 'DELETE /groups/:id/members/:user_id' do
+      context 'when minimal access role is available' do
+        it 'deletes the member' do
+          stub_licensed_features(minimal_access_role: true)
+          expect do
+            delete api("/groups/#{group.id}/members/#{minimal_access_member.user_id}", owner)
+          end.to change { group.all_group_members.count }.by(-1)
+
+          expect(response).to have_gitlab_http_status(:no_content)
+        end
+      end
+
+      context 'when minimal access role is not available' do
+        it 'does not delete the member' do
+          expect do
+            delete api("/groups/#{group.id}/members/#{minimal_access_member.id}", owner)
+
+            expect(response).to have_gitlab_http_status(:not_found)
+          end.not_to change { group.all_group_members.count }
+        end
+      end
+    end
+  end
+
   context 'group members endpoint for group managed accounts' do
     let(:group) { create(:group) }
     let(:owner) { create(:user) }

lib/api/helpers/members_helpers.rb

@@ -20,12 +20,16 @@ module API
 
       # 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 = deep ? find_all_members(source) : source_members(source).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
+
+      def source_members(source)
+        source.members
+      end
       # rubocop: enable CodeReuse/ActiveRecord
 
       def find_all_members(source)

lib/api/members.rb

@@ -136,7 +136,7 @@ module API
           source = find_source(source_type, params.delete(:id))
           authorize_admin_source!(source_type, source)
 
-          member = source.members.find_by!(user_id: params[:user_id])
+          member = source_members(source).find_by!(user_id: params[:user_id])
           updated_member =
             ::Members::UpdateService
               .new(current_user, declared_params(include_missing: false))
@@ -159,7 +159,7 @@ module API
         # rubocop: disable CodeReuse/ActiveRecord
         delete ":id/members/:user_id" do
           source = find_source(source_type, params[:id])
-          member = source.members.find_by!(user_id: params[:user_id])
+          member = source_members(source).find_by!(user_id: params[:user_id])
 
           destroy_conditionally!(member) do
             ::Members::DestroyService.new(current_user).execute(member, unassign_issuables: params[:unassign_issuables])

spec/requests/api/members_spec.rb

@@ -7,6 +7,7 @@ RSpec.describe API::Members do
   let(:developer) { create(:user) }
   let(:access_requester) { create(:user) }
   let(:stranger) { create(:user) }
+  let(:user_with_minimal_access) { create(:user) }
 
   let(:project) do
     create(:project, :public, creator_id: maintainer.id, namespace: maintainer.namespace) do |project|
@@ -20,6 +21,7 @@ RSpec.describe API::Members do
     create(:group, :public) do |group|
       group.add_developer(developer)
       group.add_owner(maintainer)
+      create(:group_member, :minimal_access, source: group, user: user_with_minimal_access)
       group.request_access(access_requester)
     end
   end