Merge branch 'revert-1f365d4b' into 'master'

Revert "Merge branch 'restrict-user-autocompletes' into 'master'"

See merge request gitlab-org/gitlab!30512

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

@@ -4,15 +4,10 @@
 
 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 {
-  AJAX_USERS_SELECT_OPTIONS_MAP,
-  AJAX_USERS_SELECT_PARAMS_MAP,
-} from 'ee_else_ce/users_select/constants';
-import { getAjaxUsersSelectOptions, getAjaxUsersSelectParams } from './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';
 
 // TODO: remove eventHub hack after code splitting refactor
 window.emitSidebarEvent = window.emitSidebarEvent || $.noop;
@@ -563,8 +558,13 @@ function UsersSelect(currentUser, els, options = {}) {
   import(/* webpackChunkName: 'select2' */ 'select2/select2')
     .then(() => {
       $('.ajax-users-select').each((i, select) => {
-        const options = getAjaxUsersSelectOptions($(select), AJAX_USERS_SELECT_OPTIONS_MAP);
+        const options = {};
         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');
@@ -705,7 +705,14 @@ UsersSelect.prototype.users = function(query, options, callback) {
   const params = {
     search: query,
     active: true,
-    ...getAjaxUsersSelectParams(options, AJAX_USERS_SELECT_PARAMS_MAP),
+    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,
   };
 
   if (options.issuableType === 'merge_request') {

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/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, :saml_provider_id
+      attr_reader :skip_ldap, :push_code_to_protected_branches, :push_code
 
       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.limit_to_saml_provider(saml_provider_id)
+        users = super
+
         skip_ldap == 'true' ? users.non_ldap : users
       end
 

ee/app/helpers/ee/selects_helper.rb

@@ -2,8 +2,6 @@
 
 module EE
   module SelectsHelper
-    extend ::Gitlab::Utils::Override
-
     def ldap_server_select_options
       options_from_collection_for_select(
         ::Gitlab::Auth::Ldap::Config.available_servers,
@@ -20,17 +18,5 @@ 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,16 +133,6 @@ 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 }).distinct
-        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: 28115
-author:
-type: fixed

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

@@ -6,13 +6,17 @@ 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,
@@ -44,37 +48,4 @@ 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
-      allow(Gitlab::Session).to receive(:current).and_return(session)
-      group.add_owner(user1)
-      sign_in(user1)
-
-      stub_feature_flags(enforced_sso: true, group_saml: true)
-      stub_licensed_features(group_saml: true)
-    end
-
-    it 'returns only users with SAML in autocomplete', :js do
-      create(:identity, saml_provider: saml_provider, user: user1)
-      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
-  it 'returns only users with that SAML provider when saml_provider_id is given' do
-    current_user = create(:user)
-    user1 = create(:user, username: 'samdoe')
-    user2 = create(:user, username: 'allymolly')
-    group = create(:group)
-    saml_provider = create(:saml_provider, group: group)
-    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
-end

ee/spec/models/user_spec.rb

@@ -527,30 +527,6 @@ 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' }
-    sequence(:extern_uid) { |n| "my-ldap-id-#{n}" }
+    extern_uid { 'my-ldap-id' }
   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,
-    });
-  });
-});