From 31f2b0db0a930f2847d4c8b1c202fc248cd8dbcf Mon Sep 17 00:00:00 2001 From: Jackie Fraser Date: Tue, 13 Jul 2021 14:58:37 -0400 Subject: [PATCH 1/4] Remove non SAML users from suggested invite list Adds the option to limit the list of existing users to users created by the same saml group provider. --- .../components/invite_members_modal.vue | 14 ++++- .../components/members_token_select.vue | 25 +++++++-- .../javascripts/invite_members/constants.js | 2 + .../init_invite_members_modal.js | 2 + app/helpers/invite_members_helper.rb | 9 ++++ .../groups/_invite_members_modal.html.haml | 3 +- .../projects/_invite_members_modal.html.haml | 2 +- .../groups/members/list_members_spec.rb | 37 +++++-------- .../components/members_token_select_spec.js | 54 +++++++++++++++---- 9 files changed, 108 insertions(+), 40 deletions(-) diff --git a/app/assets/javascripts/invite_members/components/invite_members_modal.vue b/app/assets/javascripts/invite_members/components/invite_members_modal.vue index 40eccefd7e0e0a..cab70ac11b7a09 100644 --- a/app/assets/javascripts/invite_members/components/invite_members_modal.vue +++ b/app/assets/javascripts/invite_members/components/invite_members_modal.vue @@ -16,7 +16,7 @@ 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 +72,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 +394,8 @@ export default { class="gl-mb-2" :validation-state="validationState" :aria-labelledby="$options.membersTokenSelectLabelId" + :users-filter="usersFilter" + :filter-id="filterId" @clear="handleMembersTokenSelectClear" /> { 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'), }, diff --git a/app/assets/javascripts/invite_members/constants.js b/app/assets/javascripts/invite_members/constants.js index 01b35f2a656acc..d7daf83e26b044 100644 --- a/app/assets/javascripts/invite_members/constants.js +++ b/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'; diff --git a/app/assets/javascripts/invite_members/init_invite_members_modal.js b/app/assets/javascripts/invite_members/init_invite_members_modal.js index db7e2ca4a71918..c1dfaa25dc7e67 100644 --- a/app/assets/javascripts/invite_members/init_invite_members_modal.js +++ b/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), }, }), }); diff --git a/app/helpers/invite_members_helper.rb b/app/helpers/invite_members_helper.rb index 3274ea15b8b556..2cf105236f30e1 100644 --- a/app/helpers/invite_members_helper.rb +++ b/app/helpers/invite_members_helper.rb @@ -78,4 +78,13 @@ def member_areas_of_focus_options } ] end + + def users_filter_data(group) + root_group = group&.root_ancestor + if root_group&.enforced_sso? && root_group&.saml_provider&.id + { users_filter: 'saml_provider_id', filter_id: root_group.saml_provider.id } + else + {} + end + end end diff --git a/app/views/groups/_invite_members_modal.html.haml b/app/views/groups/_invite_members_modal.html.haml index 8801ad98b8cdee..a9e1184f956976 100644 --- a/app/views/groups/_invite_members_modal.html.haml +++ b/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)) } diff --git a/app/views/projects/_invite_members_modal.html.haml b/app/views/projects/_invite_members_modal.html.haml index 16964d2154a305..3304c9adc33a0a 100644 --- a/app/views/projects/_invite_members_modal.html.haml +++ b/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)) } diff --git a/ee/spec/features/groups/members/list_members_spec.rb b/ee/spec/features/groups/members/list_members_spec.rb index 646092c76c821c..9d22885714f669 100644 --- a/ee/spec/features/groups/members/list_members_spec.rb +++ b/ee/spec/features/groups/members/list_members_spec.rb @@ -16,9 +16,7 @@ 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 @@ -53,36 +51,30 @@ stub_licensed_features(group_saml: true) allow(Gitlab::Session).to receive(:current).and_return(session) - create(:identity, saml_provider: saml_provider, user: user1) + 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) sign_in(user1) + group.add_owner(user1) 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 - 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 + field = find('[data-testid="members-token-select-input"]') + field.native.send_keys :tab + field.click - expect(page).to have_content(user_with_saml.name) - end - - [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 +84,6 @@ 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 diff --git a/spec/frontend/invite_members/components/members_token_select_spec.js b/spec/frontend/invite_members/components/members_token_select_spec.js index 12db7e42464629..196a716d08cc2a 100644 --- a/spec/frontend/invite_members/components/members_token_select_spec.js +++ b/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, + }); + }); + }); }); -- GitLab From 6dd3cf385506111d19ce3a50eb83a9cf10c0689b Mon Sep 17 00:00:00 2001 From: Jackie Fraser Date: Wed, 4 Aug 2021 16:45:38 -0400 Subject: [PATCH 2/4] Apply review feedback: helper, tests --- .../components/invite_members_modal.vue | 7 +++++- app/helpers/invite_members_helper.rb | 9 ++++---- .../groups/_invite_members_modal.html.haml | 2 +- .../projects/_invite_members_modal.html.haml | 2 +- .../groups/members/list_members_spec.rb | 22 ++++++++++--------- 5 files changed, 24 insertions(+), 18 deletions(-) diff --git a/app/assets/javascripts/invite_members/components/invite_members_modal.vue b/app/assets/javascripts/invite_members/components/invite_members_modal.vue index cab70ac11b7a09..ab42e8cdfebeee 100644 --- a/app/assets/javascripts/invite_members/components/invite_members_modal.vue +++ b/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, USERS_FILTER_ALL, 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, diff --git a/app/helpers/invite_members_helper.rb b/app/helpers/invite_members_helper.rb index 2cf105236f30e1..44a2d12d847236 100644 --- a/app/helpers/invite_members_helper.rb +++ b/app/helpers/invite_members_helper.rb @@ -81,10 +81,9 @@ def member_areas_of_focus_options def users_filter_data(group) root_group = group&.root_ancestor - if root_group&.enforced_sso? && root_group&.saml_provider&.id - { users_filter: 'saml_provider_id', filter_id: root_group.saml_provider.id } - else - {} - end + + 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 diff --git a/app/views/groups/_invite_members_modal.html.haml b/app/views/groups/_invite_members_modal.html.haml index a9e1184f956976..760efb16d40f39 100644 --- a/app/views/groups/_invite_members_modal.html.haml +++ b/app/views/groups/_invite_members_modal.html.haml @@ -3,4 +3,4 @@ .js-invite-members-modal{ data: { is_project: 'false', access_levels: GroupMember.access_level_roles.to_json, 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)) } + help_link: help_page_url('user/permissions') }.merge(group_select_data(group)).merge(common_invite_modal_dataset(group)).merge(users_filter_data(group)) } diff --git a/app/views/projects/_invite_members_modal.html.haml b/app/views/projects/_invite_members_modal.html.haml index 3304c9adc33a0a..f2fffeb23c5a87 100644 --- a/app/views/projects/_invite_members_modal.html.haml +++ b/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).merge(users_filter_data(project.group)) } + help_link: help_page_url('user/permissions') }.merge(common_invite_modal_dataset(project)).merge(users_filter_data(project.group)) } diff --git a/ee/spec/features/groups/members/list_members_spec.rb b/ee/spec/features/groups/members/list_members_spec.rb index 9d22885714f669..e39847ca9d959a 100644 --- a/ee/spec/features/groups/members/list_members_spec.rb +++ b/ee/spec/features/groups/members/list_members_spec.rb @@ -4,12 +4,13 @@ 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 @@ -42,20 +43,21 @@ 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) + sign_in(user1) + end + 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) - - sign_in(user1) group.add_owner(user1) end -- GitLab From a857c82680759f541531429fe6c6eb5882765533 Mon Sep 17 00:00:00 2001 From: Jackie Fraser Date: Mon, 9 Aug 2021 13:05:44 -0400 Subject: [PATCH 3/4] Move enforced_sso check to EE helper --- app/helpers/invite_members_helper.rb | 6 +-- ee/app/helpers/ee/invite_members_helper.rb | 13 +++++ .../helpers/ee/invite_members_helper_spec.rb | 49 +++++++++++++++++++ 3 files changed, 64 insertions(+), 4 deletions(-) create mode 100644 ee/app/helpers/ee/invite_members_helper.rb create mode 100644 ee/spec/helpers/ee/invite_members_helper_spec.rb diff --git a/app/helpers/invite_members_helper.rb b/app/helpers/invite_members_helper.rb index 44a2d12d847236..e76328fd9b955a 100644 --- a/app/helpers/invite_members_helper.rb +++ b/app/helpers/invite_members_helper.rb @@ -80,10 +80,8 @@ def member_areas_of_focus_options end def users_filter_data(group) - root_group = group&.root_ancestor + # overridden in EE - 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 diff --git a/ee/app/helpers/ee/invite_members_helper.rb b/ee/app/helpers/ee/invite_members_helper.rb new file mode 100644 index 00000000000000..5de9f9ada9b115 --- /dev/null +++ b/ee/app/helpers/ee/invite_members_helper.rb @@ -0,0 +1,13 @@ +# 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 diff --git a/ee/spec/helpers/ee/invite_members_helper_spec.rb b/ee/spec/helpers/ee/invite_members_helper_spec.rb new file mode 100644 index 00000000000000..fd2724259e1211 --- /dev/null +++ b/ee/spec/helpers/ee/invite_members_helper_spec.rb @@ -0,0 +1,49 @@ +# 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 -- GitLab From 34061fd3a8945ef91d4a89e3f5bc268da682191c Mon Sep 17 00:00:00 2001 From: Tetiana Chupryna Date: Tue, 10 Aug 2021 10:04:45 +0000 Subject: [PATCH 4/4] Make method shorter --- app/helpers/invite_members_helper.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/helpers/invite_members_helper.rb b/app/helpers/invite_members_helper.rb index e76328fd9b955a..e9e5f8b3a0be9c 100644 --- a/app/helpers/invite_members_helper.rb +++ b/app/helpers/invite_members_helper.rb @@ -79,9 +79,8 @@ def member_areas_of_focus_options ] end + # Overridden in EE def users_filter_data(group) - # overridden in EE - {} end end -- GitLab