From c9da4a01b38efa02a1f81ba170ae6c3e6762d543 Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Wed, 7 May 2025 14:55:19 +0300 Subject: [PATCH 1/2] Remove `saml_provider_id` filter for `GET /users` API endpoint Resolves https://gitlab.com/gitlab-org/gitlab/-/issues/424505 Changelog: security EE: true --- doc/api/users.md | 3 ++- ee/app/finders/ee/users_finder.rb | 8 ------ ee/app/models/ee/user.rb | 16 ----------- ee/lib/ee/api/helpers/users_helpers.rb | 12 +++++++-- ee/spec/finders/users_finder_spec.rb | 8 +----- ee/spec/models/ee/user_spec.rb | 37 -------------------------- ee/spec/requests/api/users_spec.rb | 24 +++++++---------- lib/api/helpers/users_helpers.rb | 2 ++ lib/api/users.rb | 3 +++ 9 files changed, 28 insertions(+), 85 deletions(-) diff --git a/doc/api/users.md b/doc/api/users.md index e5331fd45c2c23..cc5f6e1383f8e9 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 1537a49ff5e427..bef8fc8e08a2d1 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 5e38e942fe7b08..5e940d1b62fba8 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 5674b4164274ef..0d160048719e01 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 9deeb8c1a83d0a..91531746611138 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 0592cf5fb8c3ad..edb99573194fd5 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 2145e58473669d..5b80287147a8da 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 f97071d9a97671..6fcdb6e78fbdf9 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 200524bcc6367b..7aaa5c9d36475c 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -192,6 +192,9 @@ 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. + 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? -- GitLab From 14ba9b061c2f9d4944498e909c840754d6e1fbda Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Tue, 17 Jun 2025 12:18:38 +0300 Subject: [PATCH 2/2] Link to the issue about `error_for_saml_provider_id_param_ee` removal --- lib/api/users.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/api/users.rb b/lib/api/users.rb index 7aaa5c9d36475c..2c5b1f62ed92bc 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -193,6 +193,7 @@ def reorder_users(users) # 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) -- GitLab