Merge branch 'fixed-restrict-user-autocompletes' into 'master'

Restrict user autocompletes

See merge request gitlab-org/gitlab!30607

app/assets/javascripts/users_select/constants.js

+export const AJAX_USERS_SELECT_OPTIONS_MAP = {
+  projectId: 'projectId',
+  groupId: 'groupId',
+  showCurrentUser: 'currentUser',
+  authorId: 'authorId',
+  skipUsers: 'skipUsers',
+};
+
+export const AJAX_USERS_SELECT_PARAMS_MAP = {
+  project_id: 'projectId',
+  group_id: 'groupId',
+  skip_ldap: 'skipLdap',
+  todo_filter: 'todoFilter',
+  todo_state_filter: 'todoStateFilter',
+  current_user: 'showCurrentUser',
+  author_id: 'authorId',
+  skip_users: 'skipUsers',
+};

app/assets/javascripts/users_select.js → app/assets/javascripts/users_select/index.js

@@ -4,10 +4,15 @@
 
 import $ from 'jquery';
 import { escape, template, uniqBy } from 'lodash';
-import axios from './lib/utils/axios_utils';
-import { s__, __, sprintf } from './locale';
-import ModalStore from './boards/stores/modal_store';
-import { parseBoolean } from './lib/utils/common_utils';
+import axios from '../lib/utils/axios_utils';
+import { s__, __, sprintf } from '../locale';
+import ModalStore from '../boards/stores/modal_store';
+import { parseBoolean } from '../lib/utils/common_utils';
+import {
+  AJAX_USERS_SELECT_OPTIONS_MAP,
+  AJAX_USERS_SELECT_PARAMS_MAP,
+} from 'ee_else_ce/users_select/constants';
+import { getAjaxUsersSelectOptions, getAjaxUsersSelectParams } from './utils';
 
 // TODO: remove eventHub hack after code splitting refactor
 window.emitSidebarEvent = window.emitSidebarEvent || $.noop;
@@ -555,13 +560,8 @@ function UsersSelect(currentUser, els, options = {}) {
   import(/* webpackChunkName: 'select2' */ 'select2/select2')
     .then(() => {
       $('.ajax-users-select').each((i, select) => {
-        const options = {};
+        const options = getAjaxUsersSelectOptions($(select), AJAX_USERS_SELECT_OPTIONS_MAP);
         options.skipLdap = $(select).hasClass('skip_ldap');
-        options.projectId = $(select).data('projectId');
-        options.groupId = $(select).data('groupId');
-        options.showCurrentUser = $(select).data('currentUser');
-        options.authorId = $(select).data('authorId');
-        options.skipUsers = $(select).data('skipUsers');
         const showNullUser = $(select).data('nullUser');
         const showAnyUser = $(select).data('anyUser');
         const showEmailUser = $(select).data('emailUser');
@@ -702,14 +702,7 @@ UsersSelect.prototype.users = function(query, options, callback) {
   const params = {
     search: query,
     active: true,
-    project_id: options.projectId || null,
-    group_id: options.groupId || null,
-    skip_ldap: options.skipLdap || null,
-    todo_filter: options.todoFilter || null,
-    todo_state_filter: options.todoStateFilter || null,
-    current_user: options.showCurrentUser || null,
-    author_id: options.authorId || null,
-    skip_users: options.skipUsers || null,
+    ...getAjaxUsersSelectParams(options, AJAX_USERS_SELECT_PARAMS_MAP),
   };
 
   if (options.issuableType === 'merge_request') {

app/assets/javascripts/users_select/utils.js

+/**
+ * Get options from data attributes on passed `$select`.
+ * @param {jQuery} $select
+ * @param {Object} optionsMap e.g. { optionKeyName: 'dataAttributeName' }
+ */
+export const getAjaxUsersSelectOptions = ($select, optionsMap) => {
+  return Object.keys(optionsMap).reduce((accumulator, optionKey) => {
+    const dataKey = optionsMap[optionKey];
+    accumulator[optionKey] = $select.data(dataKey);
+
+    return accumulator;
+  }, {});
+};
+
+/**
+ * Get query parameters used for users request from passed `options` parameter
+ * @param {Object} options e.g. { currentUserId: 1, fooBar: 'baz' }
+ * @param {Object} paramsMap e.g. { user_id: 'currentUserId', foo_bar: 'fooBar' }
+ */
+export const getAjaxUsersSelectParams = (options, paramsMap) => {
+  return Object.keys(paramsMap).reduce((accumulator, paramKey) => {
+    const optionKey = paramsMap[paramKey];
+    accumulator[paramKey] = options[optionKey] || null;
+
+    return accumulator;
+  }, {});
+};

ee/app/assets/javascripts/users_select/constants.js

+import {
+  AJAX_USERS_SELECT_OPTIONS_MAP as CE_AJAX_USERS_SELECT_OPTIONS_MAP,
+  AJAX_USERS_SELECT_PARAMS_MAP as CE_AJAX_USERS_SELECT_PARAMS_MAP,
+} from '~/users_select/constants';
+
+export const AJAX_USERS_SELECT_OPTIONS_MAP = {
+  ...CE_AJAX_USERS_SELECT_OPTIONS_MAP,
+  samlProviderId: 'samlProviderId',
+};
+
+export const AJAX_USERS_SELECT_PARAMS_MAP = {
+  ...CE_AJAX_USERS_SELECT_PARAMS_MAP,
+  saml_provider_id: 'samlProviderId',
+};

ee/app/finders/ee/autocomplete/users_finder.rb

@@ -5,7 +5,7 @@ module EE
     module UsersFinder
       extend ::Gitlab::Utils::Override
 
-      attr_reader :skip_ldap, :push_code_to_protected_branches, :push_code
+      attr_reader :skip_ldap, :push_code_to_protected_branches, :push_code, :saml_provider_id
 
       override :initialize
       def initialize(params:, current_user:, project:, group:)
@@ -14,12 +14,12 @@ module EE
         @skip_ldap = params[:skip_ldap]
         @push_code_to_protected_branches = params[:push_code_to_protected_branches]
         @push_code = params[:push_code]
+        @saml_provider_id = params[:saml_provider_id]
       end
 
       override :find_users
       def find_users
-        users = super
-
+        users = super.limit_to_saml_provider(saml_provider_id)
         skip_ldap == 'true' ? users.non_ldap : users
       end
 

ee/app/helpers/ee/selects_helper.rb

@@ -2,6 +2,8 @@
 
 module EE
   module SelectsHelper
+    extend ::Gitlab::Utils::Override
+
     def ldap_server_select_options
       options_from_collection_for_select(
         ::Gitlab::Auth::Ldap::Config.available_servers,
@@ -18,5 +20,17 @@ module EE
 
       hidden_field_tag(id, value, class: css_class.join(' '))
     end
+
+    override :users_select_data_attributes
+    def users_select_data_attributes(opts)
+      super.merge(saml_provider_id: opts[:saml_provider_id] || nil)
+    end
+
+    override :users_select_tag
+    def users_select_tag(id, opts = {})
+      root_group = @group&.root_ancestor || @project&.group&.root_ancestor
+      opts[:saml_provider_id] = root_group&.enforced_sso? && root_group.saml_provider&.id
+      super(id, opts)
+    end
   end
 end

ee/app/models/ee/user.rb

@@ -133,6 +133,16 @@ module EE
 
         ''
       end
+
+      # Limits the users to those who have an identity that belongs to
+      # the given SAML Provider
+      def limit_to_saml_provider(saml_provider_id)
+        if saml_provider_id
+          joins(:identities).where(identities: { saml_provider_id: saml_provider_id })
+        else
+          all
+        end
+      end
     end
 
     def cannot_be_admin_and_auditor

ee/changelogs/unreleased/restrict-user-autocompletes.yml

+---
+title: If a parent group enforces SAML SSO, when adding a member to that group, its
+  subgroup or its project the autocomplete shows only users who have identity with
+  the SAML provider of the parent group
+merge_request: 30607
+author:
+type: fixed

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

@@ -6,17 +6,13 @@ describe 'Groups > Members > List members' do
   let(:user2) { create(:user, name: 'Mary Jane') }
   let(:group) { create(:group) }
 
-  before do
-    group.add_developer(user1)
-
-    sign_in(user1)
-  end
-
   context 'with Group SAML identity linked for a user' do
     let(: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,
@@ -48,4 +44,38 @@ describe 'Groups > Members > List members' do
       expect(page).to have_selector("#group_member_#{member.id} .badge-info", text: 'Managed Account')
     end
   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 } } }
+
+    before do
+      stub_feature_flags(enforced_sso: true, group_saml: true)
+      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)
+      create(:identity, user: user3)
+
+      visit group_group_members_path(group)
+
+      wait_for_requests
+
+      find('.select2-container').click
+
+      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
 end

ee/spec/finders/ee/autocomplete/users_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Autocomplete::UsersFinder do
+  let(:current_user) { create(:user) }
+  let(:group) { create(:group) }
+  let(:saml_provider) { create(:saml_provider, group: group) }
+
+  it 'returns only users with that SAML provider when saml_provider_id is given' do
+    user1 = create(:user, username: 'samdoe')
+    user2 = create(:user, username: 'allymolly')
+    create(:identity, saml_provider: saml_provider, user: user1)
+    params = { saml_provider_id: saml_provider.id }
+    group.add_users([user1, user2], GroupMember::DEVELOPER)
+
+    users = described_class.new(params: params, current_user: current_user, project: nil, group: nil).execute.to_a
+
+    expect(users).to match_array([user1])
+  end
+
+  it 'returns the user that name matches the search' do
+    user1 = create(:user, username: 'samdoe')
+    user2 = create(:user, username: 'allymolly')
+    user3 = create(:user, username: 'Samamntha')
+    create(:identity, saml_provider: saml_provider, user: user1)
+    create(:identity, saml_provider: saml_provider, user: user2)
+    create(:identity, saml_provider: saml_provider, user: user3)
+    params = { saml_provider_id: saml_provider.id, search: 'sam' }
+    group.add_users([user1, user2, user3], GroupMember::DEVELOPER)
+
+    users = described_class.new(params: params, current_user: current_user, project: nil, group: nil).execute.to_a
+    expect(users).to match_array([user1, user3])
+  end
+end

ee/spec/models/user_spec.rb

@@ -527,6 +527,30 @@ describe User do
     end
   end
 
+  describe '.limit_to_saml_provider' do
+    let_it_be(:user1) { create(:user) }
+    let_it_be(:user2) { create(:user) }
+
+    it 'returns all users when SAML provider is nil' do
+      rel = described_class.limit_to_saml_provider(nil)
+
+      expect(rel).to include(user1, user2)
+    end
+
+    it 'returns only the users who have an identity that belongs to the given SAML provider' do
+      create(:user)
+      group = create(:group)
+      saml_provider = create(:saml_provider, group: group)
+      create(:identity, saml_provider: saml_provider, user: user1)
+      create(:identity, saml_provider: saml_provider, user: user2)
+      create(:identity, user: create(:user))
+
+      rel = described_class.limit_to_saml_provider(saml_provider.id)
+
+      expect(rel).to contain_exactly(user1, user2)
+    end
+  end
+
   describe '#group_managed_account?' do
     subject { user.group_managed_account? }
 

spec/factories/identities.rb

@@ -3,6 +3,6 @@
 FactoryBot.define do
   factory :identity do
     provider { 'ldapmain' }
-    extern_uid { 'my-ldap-id' }
+    sequence(:extern_uid) { |n| "my-ldap-id-#{n}" }
   end
 end

spec/frontend/users_select/utils_spec.js

+import $ from 'jquery';
+import { getAjaxUsersSelectOptions, getAjaxUsersSelectParams } from '~/users_select/utils';
+
+const options = {
+  fooBar: 'baz',
+  activeUserId: 1,
+};
+
+describe('getAjaxUsersSelectOptions', () => {
+  it('returns options built from select data attributes', () => {
+    const $select = $('<select />', { 'data-foo-bar': 'baz', 'data-user-id': 1 });
+
+    expect(
+      getAjaxUsersSelectOptions($select, { fooBar: 'fooBar', activeUserId: 'user-id' }),
+    ).toEqual(options);
+  });
+});
+
+describe('getAjaxUsersSelectParams', () => {
+  it('returns query parameters built from provided options', () => {
+    expect(
+      getAjaxUsersSelectParams(options, {
+        foo_bar: 'fooBar',
+        active_user_id: 'activeUserId',
+        non_existent_key: 'nonExistentKey',
+      }),
+    ).toEqual({
+      foo_bar: 'baz',
+      active_user_id: 1,
+      non_existent_key: null,
+    });
+  });
+});