Merge branch 'exclude_external_users_from_invite_members_dropdown_for_saml_groups' into 'master'

Filter suggested users by saml provider in invite modal dropdown list [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!63565

app/assets/javascripts/invite_members/components/invite_members_modal.vue

@@ -16,7 +16,12 @@ import Api from '~/api';
 import ExperimentTracking from '~/experimentation/experiment_tracking';
 import { BV_SHOW_MODAL } from '~/lib/utils/constants';
 import { s__, sprintf } from '~/locale';
-import { INVITE_MEMBERS_IN_COMMENT, GROUP_FILTERS, MEMBER_AREAS_OF_FOCUS } from '../constants';
+import {
+  INVITE_MEMBERS_IN_COMMENT,
+  GROUP_FILTERS,
+  USERS_FILTER_ALL,
+  MEMBER_AREAS_OF_FOCUS,
+} from '../constants';
 import eventHub from '../event_hub';
 import {
   responseMessageFromError,
@@ -72,6 +77,16 @@ export default {
       required: false,
       default: null,
     },
+    usersFilter: {
+      type: String,
+      required: false,
+      default: USERS_FILTER_ALL,
+    },
+    filterId: {
+      type: Number,
+      required: false,
+      default: null,
+    },
     helpLink: {
       type: String,
       required: true,
@@ -384,6 +399,8 @@ export default {
           class="gl-mb-2"
           :validation-state="validationState"
           :aria-labelledby="$options.membersTokenSelectLabelId"
+          :users-filter="usersFilter"
+          :filter-id="filterId"
           @clear="handleMembersTokenSelectClear"
         />
         <group-select

app/assets/javascripts/invite_members/components/members_token_select.vue

@@ -3,7 +3,7 @@ import { GlTokenSelector, GlAvatar, GlAvatarLabeled, GlIcon, GlSprintf } from '@
 import { debounce } from 'lodash';
 import { __ } from '~/locale';
 import { getUsers } from '~/rest_api';
-import { SEARCH_DELAY } from '../constants';
+import { SEARCH_DELAY, USERS_FILTER_ALL, USERS_FILTER_SAML_PROVIDER_ID } from '../constants';
 
 export default {
   components: {
@@ -26,6 +26,16 @@ export default {
     validationState: {
       type: Boolean,
       required: false,
+      default: false,
+    },
+    usersFilter: {
+      type: String,
+      required: false,
+      default: USERS_FILTER_ALL,
+    },
+    filterId: {
+      type: Number,
+      required: false,
       default: null,
     },
   },
@@ -51,6 +61,15 @@ export default {
       }
       return '';
     },
+    queryOptions() {
+      if (this.usersFilter === USERS_FILTER_SAML_PROVIDER_ID) {
+        return {
+          saml_provider_id: this.filterId,
+          ...this.$options.defaultQueryOptions,
+        };
+      }
+      return this.$options.defaultQueryOptions;
+    },
   },
   methods: {
     handleTextInput(query) {
@@ -60,7 +79,7 @@ export default {
       this.retrieveUsers(query);
     },
     retrieveUsers: debounce(function debouncedRetrieveUsers() {
-      return getUsers(this.query, this.$options.queryOptions)
+      return getUsers(this.query, this.queryOptions)
         .then((response) => {
           this.users = response.data.map((token) => ({
             id: token.id,
@@ -98,7 +117,7 @@ export default {
       this.$emit('clear');
     },
   },
-  queryOptions: { exclude_internal: true, active: true },
+  defaultQueryOptions: { exclude_internal: true, active: true },
   i18n: {
     inviteTextMessage: __('Invite "%{email}" by email'),
   },

app/assets/javascripts/invite_members/constants.js

@@ -17,3 +17,5 @@ export const GROUP_FILTERS = {
 export const API_MESSAGES = {
   EMAIL_ALREADY_INVITED: __('Invite email has already been taken'),
 };
+export const USERS_FILTER_ALL = 'all';
+export const USERS_FILTER_SAML_PROVIDER_ID = 'saml_provider_id';

app/assets/javascripts/invite_members/init_invite_members_modal.js

@@ -25,6 +25,8 @@ export default function initInviteMembersModal() {
           groupSelectParentId: parseInt(el.dataset.parentId, 10),
           areasOfFocusOptions: JSON.parse(el.dataset.areasOfFocusOptions),
           noSelectionAreasOfFocus: JSON.parse(el.dataset.noSelectionAreasOfFocus),
+          usersFilter: el.dataset.usersFilter,
+          filterId: parseInt(el.dataset.filterId, 10),
         },
       }),
   });

app/helpers/invite_members_helper.rb

@@ -78,4 +78,9 @@ module InviteMembersHelper
       }
     ]
   end
+
+  # Overridden in EE
+  def users_filter_data(group)
+    {}
+  end
 end

app/views/groups/_invite_members_modal.html.haml

@@ -2,4 +2,5 @@
 
 .js-invite-members-modal{ data: { is_project: 'false',
   access_levels: GroupMember.access_level_roles.to_json,
-  help_link: help_page_url('user/permissions') }.merge(group_select_data(group)).merge(common_invite_modal_dataset(group)) }
+  default_access_level: Gitlab::Access::GUEST,
+  help_link: help_page_url('user/permissions') }.merge(group_select_data(group)).merge(common_invite_modal_dataset(group)).merge(users_filter_data(group)) }

app/views/projects/_invite_members_modal.html.haml

@@ -2,4 +2,4 @@
 
 .js-invite-members-modal{ data: { is_project: 'true',
   access_levels: ProjectMember.access_level_roles.to_json,
-  help_link: help_page_url('user/permissions') }.merge(common_invite_modal_dataset(project)) }
+  help_link: help_page_url('user/permissions') }.merge(common_invite_modal_dataset(project)).merge(users_filter_data(project.group)) }

ee/app/helpers/ee/invite_members_helper.rb

+# frozen_string_literal: true
+
+module EE
+  module InviteMembersHelper
+    def users_filter_data(group)
+      root_group = group&.root_ancestor
+
+      return {} unless root_group&.enforced_sso? && root_group.saml_provider&.id
+
+      { users_filter: 'saml_provider_id', filter_id: root_group.saml_provider.id }
+    end
+  end
+end

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

@@ -4,21 +4,20 @@ require 'spec_helper'
 RSpec.describe 'Groups > Members > List members' do
   include Spec::Support::Helpers::Features::MembersHelpers
 
-  let(:user1) { create(:user, name: 'John Doe') }
-  let(:user2) { create(:user, name: 'Mary Jane') }
-  let(:group) { create(:group) }
+  let_it_be(:user1) { create(:user, name: 'John Doe') }
+  let_it_be(:user2) { create(:user, name: 'Mary Jane') }
+  let_it_be(:group) { create(:group) }
 
   context 'with Group SAML identity linked for a user' do
-    let(:saml_provider) { create(:saml_provider) }
+    let_it_be(:saml_provider) { create(:saml_provider) }
+
     let(:group) { saml_provider.group }
 
     before do
       sign_in(user1)
       group.add_developer(user1)
       group.add_guest(user2)
-      user2.identities.create!(provider: :group_saml,
-                               saml_provider: saml_provider,
-                               extern_uid: 'user2@example.com')
+      create(:identity, saml_provider: saml_provider, user: user2)
     end
 
     it 'shows user with SSO status badge', :js do
@@ -44,45 +43,40 @@ RSpec.describe 'Groups > Members > List members' do
   end
 
   context 'with SAML and enforced SSO' do
-    let(:saml_provider) { create(:saml_provider, group: group, enabled: true, enforced_sso: true) }
-    let(:user3) { create(:user, name: 'Amy with different SAML provider') }
-    let(:user4) { create(:user, name: 'Bob without SAML') }
-    let(:session) { { active_group_sso_sign_ins: { saml_provider.id => DateTime.now } } }
+    let_it_be(:saml_provider) { create(:saml_provider, group: group, enabled: true, enforced_sso: true) }
+    let_it_be(:user3) { create(:user, name: 'Amy with different SAML provider') }
+    let_it_be(:user4) { create(:user, name: 'Bob without SAML') }
+    let_it_be(:session) { { active_group_sso_sign_ins: { saml_provider.id => DateTime.now } } }
 
     before do
       stub_licensed_features(group_saml: true)
       allow(Gitlab::Session).to receive(:current).and_return(session)
-
-      create(:identity, saml_provider: saml_provider, user: user1)
-
-      group.add_owner(user1)
       sign_in(user1)
     end
 
-    it 'returns only users with SAML in autocomplete', :js do
-      create(:identity, saml_provider: saml_provider, user: user2)
+    before_all do
+      create(:identity, provider: 'group_saml1', saml_provider_id: saml_provider.id, user: user1)
+      create(:identity, provider: 'group_saml1', saml_provider_id: saml_provider.id, user: user2)
       create(:identity, user: user3)
+      group.add_owner(user1)
+    end
 
+    it 'returns only users with SAML in autocomplete', :js do
       visit group_group_members_path(group)
 
-      wait_for_requests
-
       click_on 'Invite members'
 
       page.within '#invite-members-modal' do
-        [user1, user2].each do |user_with_saml|
-          find('[data-testid="members-token-select-input"]').set(user_with_saml.name)
-          wait_for_requests
-
-          expect(page).to have_content(user_with_saml.name)
-        end
+        field = find('[data-testid="members-token-select-input"]')
+        field.native.send_keys :tab
+        field.click
 
-        [user3, user4].each do |user_without_saml|
-          find('[data-testid="members-token-select-input"]').set(user_without_saml.name)
-          wait_for_requests
+        wait_for_requests
 
-          expect(page).not_to have_content(user_without_saml.name)
-        end
+        expect(page).to have_content(user1.name)
+        expect(page).to have_content(user2.name)
+        expect(page).not_to have_content(user3.name)
+        expect(page).not_to have_content(user4.name)
       end
     end
 
@@ -92,9 +86,6 @@ RSpec.describe 'Groups > Members > List members' do
       end
 
       it 'returns only users with SAML in autocomplete', :js do
-        create(:identity, saml_provider: saml_provider, user: user2)
-        create(:identity, user: user3)
-
         visit group_group_members_path(group)
 
         wait_for_requests

ee/spec/helpers/ee/invite_members_helper_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+RSpec.describe EE::InviteMembersHelper do
+  describe '.users_filter_data' do
+    let_it_be(:group) { create(:group) }
+    let_it_be(:saml_provider) { create(:saml_provider, group: group) }
+
+    let!(:group2) { create(:group) }
+
+    context 'when the group has enforced sso' do
+      before do
+        allow(group).to receive(:enforced_sso?).and_return(true)
+      end
+
+      context 'when there is a group with a saml provider' do
+        it 'returns user filter data' do
+          expected = { users_filter: 'saml_provider_id', filter_id: saml_provider.id }
+
+          expect(helper.users_filter_data(group)).to eq expected
+        end
+      end
+
+      context 'when there is a group without a saml provider' do
+        it 'does not return user filter data' do
+          expect(helper.users_filter_data(group2)).to eq({})
+        end
+      end
+    end
+
+    context 'when group has enforced sso disabled' do
+      before do
+        allow(group).to receive(:enforced_sso?).and_return(false)
+      end
+
+      context 'when there is a group with a saml provider' do
+        it 'does not return user filter data' do
+          expect(helper.users_filter_data(group)).to eq({})
+        end
+      end
+
+      context 'when there is a group without a saml provider' do
+        it 'does not return user filter data' do
+          expect(helper.users_filter_data(group2)).to eq({})
+        end
+      end
+    end
+  end
+end

spec/frontend/invite_members/components/members_token_select_spec.js

@@ -12,11 +12,12 @@ const user1 = { id: 1, name: 'John Smith', username: 'one_1', avatar_url: '' };
 const user2 = { id: 2, name: 'Jane Doe', username: 'two_2', avatar_url: '' };
 const allUsers = [user1, user2];
 
-const createComponent = () => {
+const createComponent = (props) => {
   return shallowMount(MembersTokenSelect, {
     propsData: {
       ariaLabelledby: label,
       placeholder,
+      ...props,
     },
     stubs: {
       GlTokenSelector: stubComponent(GlTokenSelector),
@@ -27,11 +28,6 @@ const createComponent = () => {
 describe('MembersTokenSelect', () => {
   let wrapper;
 
-  beforeEach(() => {
-    jest.spyOn(UserApi, 'getUsers').mockResolvedValue({ data: allUsers });
-    wrapper = createComponent();
-  });
-
   afterEach(() => {
     wrapper.destroy();
     wrapper = null;
@@ -41,6 +37,8 @@ describe('MembersTokenSelect', () => {
 
   describe('rendering the token-selector component', () => {
     it('renders with the correct props', () => {
+      wrapper = createComponent();
+
       const expectedProps = {
         ariaLabelledby: label,
         placeholder,
@@ -51,6 +49,11 @@ describe('MembersTokenSelect', () => {
   });
 
   describe('users', () => {
+    beforeEach(() => {
+      jest.spyOn(UserApi, 'getUsers').mockResolvedValue({ data: allUsers });
+      wrapper = createComponent();
+    });
+
     describe('when input is focused for the first time (modal auto-focus)', () => {
       it('does not call the API', async () => {
         findTokenSelector().vm.$emit('focus');
@@ -90,10 +93,10 @@ describe('MembersTokenSelect', () => {
 
         await waitForPromises();
 
-        expect(UserApi.getUsers).toHaveBeenCalledWith(
-          searchParam,
-          wrapper.vm.$options.queryOptions,
-        );
+        expect(UserApi.getUsers).toHaveBeenCalledWith(searchParam, {
+          active: true,
+          exclude_internal: true,
+        });
         expect(tokenSelector.props('hideDropdownWithNoItems')).toBe(false);
       });
 
@@ -134,6 +137,8 @@ describe('MembersTokenSelect', () => {
 
   describe('when text input is blurred', () => {
     it('clears text input', async () => {
+      wrapper = createComponent();
+
       const tokenSelector = findTokenSelector();
 
       tokenSelector.vm.$emit('blur');
@@ -143,4 +148,33 @@ describe('MembersTokenSelect', () => {
       expect(tokenSelector.props('hideDropdownWithNoItems')).toBe(false);
     });
   });
+
+  describe('when component is mounted for a group using a saml provider', () => {
+    const searchParam = 'name';
+    const samlProviderId = 123;
+    let resolveApiRequest;
+
+    beforeEach(() => {
+      jest.spyOn(UserApi, 'getUsers').mockImplementation(
+        () =>
+          new Promise((resolve) => {
+            resolveApiRequest = resolve;
+          }),
+      );
+
+      wrapper = createComponent({ filterId: samlProviderId, usersFilter: 'saml_provider_id' });
+
+      findTokenSelector().vm.$emit('text-input', searchParam);
+    });
+
+    it('calls the API with the saml provider ID param', () => {
+      resolveApiRequest({ data: allUsers });
+
+      expect(UserApi.getUsers).toHaveBeenCalledWith(searchParam, {
+        active: true,
+        exclude_internal: true,
+        saml_provider_id: samlProviderId,
+      });
+    });
+  });
 });