diff --git a/doc/api/users.md b/doc/api/users.md index e5331fd45c2c231d01394a5f44813cd46cab3dc0..cc5f6e1383f8e95787c191a727bfb18c87dfda42 100644 --- a/doc/api/users.md +++ b/doc/api/users.md @@ -26,6 +26,7 @@ Takes [pagination parameters](rest/_index.md#offset-based-pagination) `page` and {{< history >}} - Keyset pagination [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/419556) in GitLab 16.5. +- `saml_provider_id` attribute [removed](https://gitlab.com/gitlab-org/gitlab/-/issues/424505) in GitLab 18.2. {{< /history >}} @@ -50,6 +51,7 @@ Supported attributes: | `exclude_humans` | boolean | no | Filters only bot or internal users. Default is `false`. | | `exclude_internal` | boolean | no | Filters only non internal users. Default is `false`. | | `without_project_bots` | boolean | no | Filters user without project bots. Default is `false`. | +| `saml_provider_id` | number | no | Removed in GitLab 18.2. Use [`GET /groups/:id/saml_users`](groups.md#list-all-saml-users) instead. | Example response: @@ -186,7 +188,6 @@ Supported attributes: | `without_projects` | boolean | no | Filter users without projects. Default is `false`, which means that all users are returned, with and without projects. | | `admins` | boolean | no | Return only administrators. Default is `false` | | `auditors` | boolean | no | Return only auditor users. Default is `false`. If not included, it returns all users. Premium and Ultimate only. | -| `saml_provider_id` | number | no | Return only users created by the specified SAML provider ID. If not included, it returns all users. Premium and Ultimate only. | | `skip_ldap` | boolean | no | Skip LDAP users. Premium and Ultimate only. | Example response: diff --git a/ee/app/finders/ee/users_finder.rb b/ee/app/finders/ee/users_finder.rb index 1537a49ff5e427616117196f87c7e7e9d22ceb15..bef8fc8e08a2d164360ae1508ef0840eb82aa363 100644 --- a/ee/app/finders/ee/users_finder.rb +++ b/ee/app/finders/ee/users_finder.rb @@ -7,7 +7,6 @@ module UsersFinder override :execute def execute users = by_non_ldap(super) - users = by_saml_provider_id(users) users = by_auditors(users) if ::License.feature_available?(:auditor_user) order(users) end @@ -18,13 +17,6 @@ def by_non_ldap(users) users.non_ldap end - def by_saml_provider_id(users) - saml_provider_id = params[:saml_provider_id] - return users unless saml_provider_id - - users.limit_to_saml_provider(saml_provider_id) - end - override :by_external_identity def by_external_identity(users) return users unless params[:extern_uid] && params[:provider] diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index 5e38e942fe7b08e422eedf58417bf84eb9d89ce6..5e940d1b62fba8b0cd1b9b1b665a83f6a575a7a4 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -252,22 +252,6 @@ def find_by_smartcard_identity(certificate_subject, certificate_issuer) .find_by(smartcard_identities: { subject: certificate_subject, issuer: certificate_issuer }) 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 - group_id = SamlProvider.select(:group_id).find(saml_provider_id).group_id - saml_users = joins(:identities).where(identities: { saml_provider_id: saml_provider_id }) - service_accounts = - service_account.joins(:user_detail).where(user_detail: { provisioned_by_group_id: group_id }) - - union = ::Gitlab::SQL::Union.new([saml_users, service_accounts]).to_sql - ::User.from("(#{union}) #{::User.table_name}") - else - all - end - end - def billable scope = active.without_bots diff --git a/ee/lib/ee/api/helpers/users_helpers.rb b/ee/lib/ee/api/helpers/users_helpers.rb index 5674b4164274efc07503bc10538a287c712c8c25..0d160048719e0154202601e2fd2e5d5e2dce3dd9 100644 --- a/ee/lib/ee/api/helpers/users_helpers.rb +++ b/ee/lib/ee/api/helpers/users_helpers.rb @@ -5,7 +5,6 @@ module API module Helpers module UsersHelpers extend ActiveSupport::Concern - extend ::Gitlab::Utils::Override prepended do params :optional_params_ee do @@ -17,9 +16,18 @@ module UsersHelpers params :optional_index_params_ee do optional :skip_ldap, type: Grape::API::Boolean, default: false, desc: 'Skip LDAP users' - optional :saml_provider_id, type: Integer, desc: 'Return only users from the specified SAML provider Id' optional :auditors, type: Grape::API::Boolean, default: false, desc: 'Filters only auditor users' end + + def error_for_saml_provider_id_param_ee + return unless params[:saml_provider_id].present? + + forbidden!( + "saml_provider_id attribute was removed for security reasons. " \ + "Consider using 'GET /groups/:id/saml_users' API endpoint instead, " \ + "see #{Rails.application.routes.url_helpers.help_page_url('api/groups.md', anchor: 'list-all-saml-users')}" + ) + end end end end diff --git a/ee/spec/finders/users_finder_spec.rb b/ee/spec/finders/users_finder_spec.rb index 9deeb8c1a83d0a22e981a3e10802c8189716b9ab..91531746611138be719fff93d55240173ed758d4 100644 --- a/ee/spec/finders/users_finder_spec.rb +++ b/ee/spec/finders/users_finder_spec.rb @@ -39,7 +39,7 @@ create(:identity, provider: 'group_saml1', saml_provider_id: saml_provider.id, user: saml_user) end - it 'returns all users' do + it 'returns all users by default' do users = described_class.new(user).execute expect(users).to contain_exactly( @@ -52,12 +52,6 @@ *users_visible_to_admin ) end - - it 'returns saml users and service accounts for the SAML provider and associated group' do - users = described_class.new(user, saml_provider_id: saml_provider.id).execute - - expect(users).to contain_exactly(saml_user, service_account) - end end context 'with auditor users' do diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index 0592cf5fb8c3adbe3f9b05c190c00ebbb1d5de35..edb99573194fd5d2d1e3d8480b09d362320c32b9 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -1338,43 +1338,6 @@ end end - describe '.limit_to_saml_provider' do - let_it_be(:user1) { create(:user) } - let_it_be(:user2) { create(:user) } - let_it_be(:service_account) { create(:service_account) } - let_it_be(:group) { create(:group) } - let_it_be(:saml_provider) { create(:saml_provider, group: group) } - - it 'returns all users when SAML provider is nil' do - rel = described_class.limit_to_saml_provider(nil) - - expect(rel).to include(user1, user2, service_account) - end - - context 'when users have an identity belonging to the given SAML provider' do - before_all do - create(:user) - create(:identity, saml_provider: saml_provider, user: user1) - create(:identity, saml_provider: saml_provider, user: user2) - create(:identity, user: create(:user)) - end - - it 'does not return service accounts not provisioned by the group' do - rel = described_class.limit_to_saml_provider(saml_provider.id) - - expect(rel).to contain_exactly(user1, user2) - end - - it 'returns only the users who have an identity that belongs to the given SAML provider and service accounts' do - service_account.update!(provisioned_by_group: group) - - rel = described_class.limit_to_saml_provider(saml_provider.id) - - expect(rel).to contain_exactly(user1, user2, service_account) - end - end - end - describe '.billable' do let_it_be(:bot_user) { create(:user, :bot) } let_it_be(:service_account) { create(:user, :service_account) } diff --git a/ee/spec/requests/api/users_spec.rb b/ee/spec/requests/api/users_spec.rb index 2145e58473669d41faec02a485c86561a39f234c..5b80287147a8da893c3b7d74119eca376bc6a922 100644 --- a/ee/spec/requests/api/users_spec.rb +++ b/ee/spec/requests/api/users_spec.rb @@ -311,20 +311,16 @@ describe 'GET /api/users?saml_provider_id' do context 'querying users by saml provider id' do - let(:group) { create(:group) } - let(:saml_provider) { create(:saml_provider, group: group, enabled: true, enforced_sso: true) } - - it 'returns only users for the saml_provider_id' do - saml_user = create(:user) - create(:identity, provider: 'group_saml1', saml_provider_id: saml_provider.id, user: saml_user) - non_saml_user = create(:user) - - get api("/users", user), params: { saml_provider_id: saml_provider.id } - - expect(response).to match_response_schema('public_api/v4/user/basics') - expect(response).to include_pagination_headers - expect(json_response.map { |u| u['id'] }).to include(saml_user.id) - expect(json_response.map { |u| u['id'] }).not_to include(non_saml_user.id) + # See https://gitlab.com/gitlab-org/gitlab/-/issues/424505 + it 'returns forbidden status with message' do + get api("/users", user), params: { saml_provider_id: 42 } + + expect(response).to have_gitlab_http_status(:forbidden) + expect(json_response['message']).to eq( + "403 Forbidden - saml_provider_id attribute was removed for security reasons. " \ + "Consider using 'GET /groups/:id/saml_users' API endpoint instead, " \ + "see #{Rails.application.routes.url_helpers.help_page_url('api/groups.md', anchor: 'list-all-saml-users')}" + ) end end end diff --git a/lib/api/helpers/users_helpers.rb b/lib/api/helpers/users_helpers.rb index f97071d9a9767145201ef8d01a6f0df4af33a9a6..6fcdb6e78fbdf9e04586fd4e6ec9467080c1d8a0 100644 --- a/lib/api/helpers/users_helpers.rb +++ b/lib/api/helpers/users_helpers.rb @@ -12,6 +12,8 @@ module UsersHelpers params :optional_index_params_ee do end + def error_for_saml_provider_id_param_ee; end + def model_errors(model) super.tap do |errors| # Remapping errors from nested associations. diff --git a/lib/api/users.rb b/lib/api/users.rb index 200524bcc6367b4715650d0bf8f9bb9f59ab7586..2c5b1f62ed92bc985c47fc27106871fc2587fce3 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -192,6 +192,10 @@ def reorder_users(users) end # rubocop: disable CodeReuse/ActiveRecord get feature_category: :user_profile, urgency: :low do + # This error can be removed in/after 19.0 release. + # https://gitlab.com/gitlab-org/gitlab/-/issues/549951 + error_for_saml_provider_id_param_ee + index_params = declared_params(include_missing: false) authenticated_as_admin! if index_params[:extern_uid].present? && index_params[:provider].present?