From 27b671a91c1ed1acd50d8bc66feb6a7963e06cf6 Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Tue, 29 Apr 2025 17:04:33 +0300 Subject: [PATCH 1/6] Reverts MR https://gitlab.com/gitlab-org/gitlab/-/merge_requests/141040 --- ee/app/policies/ee/group_policy.rb | 11 ----- ee/lib/ee/api/groups.rb | 2 +- ee/spec/policies/group_policy_spec.rb | 60 --------------------------- ee/spec/requests/api/groups_spec.rb | 18 ++------ 4 files changed, 5 insertions(+), 86 deletions(-) diff --git a/ee/app/policies/ee/group_policy.rb b/ee/app/policies/ee/group_policy.rb index ca964701d3871c..9e053f22a50f40 100644 --- a/ee/app/policies/ee/group_policy.rb +++ b/ee/app/policies/ee/group_policy.rb @@ -105,13 +105,6 @@ module GroupPolicy ::Gitlab::Auth::GroupSaml::SessionEnforcer.new(@user, @subject).access_restricted? end - condition(:sso_enforced, scope: :subject) do - saml_provider = @subject.root_ancestor.saml_provider - next false unless saml_provider - - saml_provider.enabled? && saml_provider.enforced_sso? - end - condition(:ip_enforcement_prevents_access, scope: :subject) do !::Gitlab::IpRestriction::Enforcer.new(subject).allows_current_ip? end @@ -965,10 +958,6 @@ module GroupPolicy rule { can?(:read_group) & duo_features_enabled }.enable :access_duo_features - rule { can?(:admin_group_member) & sso_enforced }.policy do - enable :read_saml_user - end - rule { supports_saved_replies & guest }.enable :read_saved_replies rule { supports_saved_replies & developer }.policy do diff --git a/ee/lib/ee/api/groups.rb b/ee/lib/ee/api/groups.rb index 161446da2391fb..9f97124bab3a4c 100644 --- a/ee/lib/ee/api/groups.rb +++ b/ee/lib/ee/api/groups.rb @@ -293,7 +293,7 @@ def check_ssh_certificate_available_to_group(group) end # rubocop: disable CodeReuse/ActiveRecord get ':id/users', feature_category: :user_management do - authorize! :read_saml_user, user_group + authorize! :read_group_member, user_group params = declared_params(include_missing: false).except!(:id) bad_request!(users_params_error) unless any_allowed_filters_present?(params) diff --git a/ee/spec/policies/group_policy_spec.rb b/ee/spec/policies/group_policy_spec.rb index 9aae8da01d6c4f..97f0897337f01b 100644 --- a/ee/spec/policies/group_policy_spec.rb +++ b/ee/spec/policies/group_policy_spec.rb @@ -4008,66 +4008,6 @@ def expect_private_group_permissions_as_if_non_member end end - describe ':read_saml_user' do - let_it_be(:user) { non_group_member } - let_it_be(:subgroup) { create(:group, :private, parent: group) } - - subject(:policy) { described_class.new(user, the_group) } - - context 'when a SAML provider does not exist' do - let_it_be(:the_group) { subgroup } - - before do - stub_licensed_features(group_saml: true) - the_group.add_member(user, Gitlab::Access::OWNER) - end - - it { is_expected.to be_disallowed(:read_saml_user) } - end - - context 'when a SAML provider exists' do - before_all do - create(:saml_provider, group: group) - end - where(:the_group, :licensed, :saml_enabled, :sso_enforced, :role, :allowed) do - ref(:group) | false | false | false | Gitlab::Access::OWNER | false - ref(:group) | false | false | false | Gitlab::Access::MAINTAINER | false - - ref(:group) | true | false | false | Gitlab::Access::OWNER | false - ref(:group) | true | false | false | Gitlab::Access::MAINTAINER | false - - ref(:group) | true | true | false | Gitlab::Access::OWNER | false - ref(:group) | true | true | false | Gitlab::Access::MAINTAINER | false - - ref(:group) | true | true | true | Gitlab::Access::OWNER | true - ref(:group) | true | true | true | Gitlab::Access::MAINTAINER | false - - ref(:subgroup) | false | false | false | Gitlab::Access::OWNER | false - ref(:subgroup) | false | false | false | Gitlab::Access::MAINTAINER | false - - ref(:subgroup) | true | false | false | Gitlab::Access::OWNER | false - ref(:subgroup) | true | false | false | Gitlab::Access::MAINTAINER | false - - ref(:subgroup) | true | true | false | Gitlab::Access::OWNER | false - ref(:subgroup) | true | true | false | Gitlab::Access::MAINTAINER | false - - ref(:subgroup) | true | true | true | Gitlab::Access::OWNER | true - ref(:subgroup) | true | true | true | Gitlab::Access::MAINTAINER | false - end - - with_them do - before do - stub_licensed_features(group_saml: licensed) - the_group.add_member(user, role) - the_group.root_ancestor.saml_provider.update!(enabled: saml_enabled) - the_group.root_ancestor.saml_provider.update!(enforced_sso: sso_enforced) - end - - it { expect(policy.allowed?(:read_saml_user)).to eq(allowed) } - end - end - end - context 'custom role' do let_it_be(:guest) { create(:user) } let_it_be(:parent_group) { create(:group) } diff --git a/ee/spec/requests/api/groups_spec.rb b/ee/spec/requests/api/groups_spec.rb index ab61d50309a484..7560457fbb0cc1 100644 --- a/ee/spec/requests/api/groups_spec.rb +++ b/ee/spec/requests/api/groups_spec.rb @@ -1917,8 +1917,8 @@ describe 'GET /groups/:id/users' do let_it_be(:group) { create(:group, :private) } let_it_be(:regular_user) { create(:user) } - let_it_be_with_refind(:saml_provider) { create(:saml_provider, group: group, enforced_sso: true) } - let_it_be(:group_member_user) { create(:user) { |u| group.add_owner(u) } } + let_it_be(:saml_provider) { create(:saml_provider, group: group) } + let_it_be(:group_member_user) { create(:user) { |u| group.add_developer(u) } } let_it_be(:saml_user) { create(:user, :public_email) } let_it_be(:non_saml_user) { create(:user) { |u| group.add_maintainer(u) } } @@ -1927,7 +1927,7 @@ subject(:get_users) { get api("/groups/#{group.to_param}/users", current_user), params: params } before do - stub_licensed_features(group_saml: true) + create(:saml_provider, group: group) create(:group_saml_identity, user: group_member_user, saml_provider: saml_provider) create(:group_saml_identity, user: saml_user, saml_provider: saml_provider) end @@ -1958,7 +1958,7 @@ context 'when no include params are true' do let(:params) { { include_saml_users: false } } - it 'returns 400' do + it 'returns 404' do get_users expect(response).to have_gitlab_http_status(:bad_request) @@ -1973,16 +1973,6 @@ expect(json_response.pluck('id')).to match_array([group_member_user.id, saml_user.id, service_account.id]) end - - context 'when no SAML provider exists' do - it 'returns 403' do - saml_provider.destroy! - - get_users - - expect(response).to have_gitlab_http_status(:forbidden) - end - end end context 'when only include_saml_users is true' do -- GitLab From e7c3189fc73c1e6a4e87b19d50ebfd239b14e0f8 Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Wed, 30 Apr 2025 13:45:44 +0300 Subject: [PATCH 2/6] Reverts MR https://gitlab.com/gitlab-org/gitlab/-/merge_requests/140241 --- ee/app/finders/groups/users_finder.rb | 3 +-- ee/lib/ee/api/groups.rb | 5 ++--- ee/spec/finders/groups/users_finder_spec.rb | 16 ---------------- 3 files changed, 3 insertions(+), 21 deletions(-) diff --git a/ee/app/finders/groups/users_finder.rb b/ee/app/finders/groups/users_finder.rb index 6510f44e34c2f0..67285f2310cbdc 100644 --- a/ee/app/finders/groups/users_finder.rb +++ b/ee/app/finders/groups/users_finder.rb @@ -25,8 +25,7 @@ def base_scope return relations.first if relations.size == 1 - union = ::User.from_union(relations) - union.order_id_desc + ::User.from_union(relations) # rubocop:disable CodeReuse/ActiveRecord -- Simple union of existing relations/scopes end def relations_to_join(users) diff --git a/ee/lib/ee/api/groups.rb b/ee/lib/ee/api/groups.rb index 9f97124bab3a4c..3fff547ea65ec1 100644 --- a/ee/lib/ee/api/groups.rb +++ b/ee/lib/ee/api/groups.rb @@ -278,9 +278,6 @@ def check_ssh_certificate_available_to_group(group) ] end params do - optional :search, type: String, desc: 'Search users by name, email or username' - optional :active, type: ::Grape::API::Boolean, default: false, desc: 'Filters only active users' - optional :include_saml_users, type: Grape::API::Boolean, desc: 'Return users with a SAML identity in this group' @@ -289,6 +286,8 @@ def check_ssh_certificate_available_to_group(group) desc: 'Return service accounts owned by this group' at_least_one_of :include_saml_users, :include_service_accounts + optional :search, type: String, desc: 'Search users by name, email or username' + use :pagination end # rubocop: disable CodeReuse/ActiveRecord diff --git a/ee/spec/finders/groups/users_finder_spec.rb b/ee/spec/finders/groups/users_finder_spec.rb index 26fc0bdc16a7db..a3543ff5efe5a9 100644 --- a/ee/spec/finders/groups/users_finder_spec.rb +++ b/ee/spec/finders/groups/users_finder_spec.rb @@ -38,10 +38,6 @@ it 'finds both saml users and service accounts' do expect(finder.execute).to match_array([saml_user1, saml_user2, service_account1, service_account2]) end - - it 'orders by id descending' do - expect(finder.execute).to eq([service_account2, service_account1, saml_user2, saml_user1]) - end end context 'when params includes only saml users' do @@ -50,18 +46,6 @@ it 'finds only saml users' do expect(finder.execute).to match_array([saml_user1, saml_user2]) end - - context 'when active param is true' do - let(:params) { { active: true, include_saml_users: true } } - - before do - saml_user2.block! - end - - it 'includes only active users' do - expect(finder.execute).to match_array([saml_user1]) - end - end end context 'when params includes only service accounts' do -- GitLab From 5c87b68242043316bc2ca2ca7baf751a4f025b5e Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Wed, 30 Apr 2025 13:58:36 +0300 Subject: [PATCH 3/6] Reverts MR https://gitlab.com/gitlab-org/gitlab/-/merge_requests/134570 --- doc/api/groups.md | 101 ------------------ ee/app/finders/groups/users_finder.rb | 61 ----------- ee/app/models/ee/user.rb | 6 -- ee/lib/ee/api/groups.rb | 44 -------- ee/spec/finders/groups/users_finder_spec.rb | 96 ------------------ ee/spec/models/ee/user_spec.rb | 53 ---------- ee/spec/requests/api/groups_spec.rb | 107 -------------------- locale/gitlab.pot | 3 - 8 files changed, 471 deletions(-) delete mode 100644 ee/app/finders/groups/users_finder.rb delete mode 100644 ee/spec/finders/groups/users_finder_spec.rb diff --git a/doc/api/groups.md b/doc/api/groups.md index 0d8ec37087124f..0465273efa3d2e 100644 --- a/doc/api/groups.md +++ b/doc/api/groups.md @@ -908,107 +908,6 @@ Example response: ] ``` -### List users - -{{< details >}} - -- Tier: Premium, Ultimate -- Offering: GitLab.com, GitLab Self-Managed, GitLab Dedicated -- Status: Experiment - -{{< /details >}} - -{{< history >}} - -- [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/424505) in GitLab 16.6. This feature is an [experiment](../policy/development_stages_support.md). - -{{< /history >}} - -{{< alert type="warning" >}} - -This endpoint is scheduled for removal in GitLab 18.3 (August 11th, 2025). -Use [`GET /groups/:id/saml_users`](#list-all-saml-users) and [`GET /groups/:id/service_accounts`](service_accounts.md#list-all-group-service-accounts) instead. - -{{< /alert >}} - -Get a list of users for a group. This endpoint returns users that are related to a top-level group regardless -of their current membership. For example, users that have a SAML identity connected to the group, or service accounts created -by the group or subgroups. - -This endpoint is an [experiment](../policy/development_stages_support.md) and might be changed or removed without notice. - -Prerequisites: - -- You must have the Owner role for the group. - -```plaintext -GET /groups/:id/users -``` - -```shell -curl --header "PRIVATE-TOKEN: " "https://gitlab.example.com/api/v4/groups/345/users?include_saml_users=true&include_service_accounts=true" -``` - -Parameters: - -| Attribute | Type | Required | Description | -|:---------------------------|:---------------|:----------------------|:------------| -| `id` | integer/string | yes | ID or [URL-encoded path of the group](rest/_index.md#namespaced-paths). | -| `include_saml_users` | boolean | yes (see description) | Include users with a SAML identity. Either this value or `include_service_accounts` must be `true`. | -| `include_service_accounts` | boolean | yes (see description) | Include service account users. Either this value or `include_saml_users` must be `true`. | -| `search` | string | no | Search users by name, email, username. | - -If successful, returns [`200 OK`](rest/troubleshooting.md#status-codes) and the -following response attributes: - -Example response: - -```json -[ - { - "id": 66, - "username": "user22", - "name": "John Doe22", - "state": "active", - "avatar_url": "https://www.gravatar.com/avatar/xxx?s=80&d=identicon", - "web_url": "http://my.gitlab.com/user22", - "created_at": "2021-09-10T12:48:22.381Z", - "bio": "", - "location": null, - "public_email": "", - "linkedin": "", - "twitter": "", - "website_url": "", - "organization": null, - "job_title": "", - "pronouns": null, - "bot": false, - "work_information": null, - "followers": 0, - "following": 0, - "local_time": null, - "last_sign_in_at": null, - "confirmed_at": "2021-09-10T12:48:22.330Z", - "last_activity_on": null, - "email": "user22@example.org", - "theme_id": 1, - "color_scheme_id": 1, - "projects_limit": 100000, - "current_sign_in_at": null, - "identities": [ ], - "can_create_group": true, - "can_create_project": true, - "two_factor_enabled": false, - "external": false, - "private_profile": false, - "commit_email": "user22@example.org", - "shared_runners_minutes_limit": null, - "extra_shared_runners_minutes_limit": null - }, - ... -] -``` - ### List subgroups Get a list of visible direct subgroups in this group. diff --git a/ee/app/finders/groups/users_finder.rb b/ee/app/finders/groups/users_finder.rb deleted file mode 100644 index 67285f2310cbdc..00000000000000 --- a/ee/app/finders/groups/users_finder.rb +++ /dev/null @@ -1,61 +0,0 @@ -# frozen_string_literal: true - -module Groups - class UsersFinder < UsersFinder - extend ::Gitlab::Utils::Override - - attr_accessor :group - - ALLOWED_FILTERS = [:include_saml_users, :include_service_accounts].freeze - - def initialize(current_user, group, params) - @current_user = current_user - @group = group.root_ancestor - @params = params || {} - - raise_params_error unless at_least_one_param_true? - end - - private - - override :base_scope - def base_scope - users = super - relations = relations_to_join(users) - - return relations.first if relations.size == 1 - - ::User.from_union(relations) # rubocop:disable CodeReuse/ActiveRecord -- Simple union of existing relations/scopes - end - - def relations_to_join(users) - relations = [] - - relations << saml_users(users) - relations << service_accounts(users) - - relations.compact - end - - def saml_users(users) - saml_provider_id = group.saml_provider - return unless params[:include_saml_users] && saml_provider_id - - users.with_saml_provider(saml_provider_id) - end - - def service_accounts(users) - return unless params[:include_service_accounts] - - users.service_account.with_provisioning_group(group) - end - - def at_least_one_param_true? - ALLOWED_FILTERS.any? { |param| params[param] } - end - - def raise_params_error - raise(ArgumentError, format(_("At least one of %{params} must be true"), params: ALLOWED_FILTERS.join(', '))) - end - end -end diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index e7243a29cc498a..cbdd5294d18913 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -200,12 +200,6 @@ module User scope :with_provider, ->(provider) do joins(:identities).where(identities: { provider: provider }) end - scope :with_saml_provider, ->(saml_provider) do - joins(:identities).where(identities: { saml_provider: saml_provider }) - end - scope :with_provisioning_group, ->(group) do - joins(:user_detail).where(user_detail: { provisioned_by_group: group }) - end scope :with_invalid_expires_at_tokens, ->(expiration_date) do where(id: ::PersonalAccessToken.with_invalid_expires_at(expiration_date).select(:user_id)) diff --git a/ee/lib/ee/api/groups.rb b/ee/lib/ee/api/groups.rb index 3fff547ea65ec1..2eb02e99d9f5ad 100644 --- a/ee/lib/ee/api/groups.rb +++ b/ee/lib/ee/api/groups.rb @@ -105,16 +105,6 @@ def filter_by_author(params) can?(current_user, :admin_group, user_group) ? params : params.merge(author_id: current_user.id) end - def users_params_error - format( - _("At least one of %{params} must be true"), params: ::Groups::UsersFinder::ALLOWED_FILTERS.join(', ') - ) - end - - def any_allowed_filters_present?(params) - ::Groups::UsersFinder::ALLOWED_FILTERS.any? { |param| params[param].presence } - end - def check_ssh_certificate_available_to_group(group) not_found!('Group') unless group not_found! unless group.licensed_feature_available?(:ssh_certificates) @@ -269,40 +259,6 @@ def check_ssh_certificate_available_to_group(group) end # rubocop: enable CodeReuse/ActiveRecord - desc 'Get a list of users for the group' do - success code: 200, model: ::API::Entities::UserPublic - failure [ - { code: 400, message: 'Bad request' }, - { code: 403, message: 'Forbidden' }, - { code: 404, message: '404 Not Found' } - ] - end - params do - optional :include_saml_users, - type: Grape::API::Boolean, - desc: 'Return users with a SAML identity in this group' - optional :include_service_accounts, - type: Grape::API::Boolean, - desc: 'Return service accounts owned by this group' - at_least_one_of :include_saml_users, :include_service_accounts - - optional :search, type: String, desc: 'Search users by name, email or username' - - use :pagination - end - # rubocop: disable CodeReuse/ActiveRecord - get ':id/users', feature_category: :user_management do - authorize! :read_group_member, user_group - params = declared_params(include_missing: false).except!(:id) - bad_request!(users_params_error) unless any_allowed_filters_present?(params) - - finder = ::Groups::UsersFinder.new(current_user, user_group, params) - users = finder.execute.preload(:identities, :group_scim_identities, :instance_scim_identities) - - present paginate(users), with: ::API::Entities::UserPublic - end - # rubocop: enable CodeReuse/ActiveRecord - desc 'Get a list of ssh certificates created for a group.' do summary 'Get a list of Groups::SshCertificate for a Group.' success code: 200, model: EE::API::Entities::SshCertificate diff --git a/ee/spec/finders/groups/users_finder_spec.rb b/ee/spec/finders/groups/users_finder_spec.rb deleted file mode 100644 index a3543ff5efe5a9..00000000000000 --- a/ee/spec/finders/groups/users_finder_spec.rb +++ /dev/null @@ -1,96 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Groups::UsersFinder, feature_category: :user_management do - describe '#execute' do - let_it_be(:group) { create(:group) } - let_it_be(:other_group) { create(:group) } - let_it_be(:saml_provider) { create(:saml_provider, group: group) } - let_it_be(:other_saml_provider) { create(:saml_provider, group: other_group) } - - let_it_be(:saml_user1) { create(:user) } - let_it_be(:saml_user2) { create(:user) } - let_it_be(:service_account1) { create(:service_account, provisioned_by_group: group) } - let_it_be(:service_account2) { create(:service_account, provisioned_by_group: group) } - - let_it_be(:saml_identity1) do - create(:group_saml_identity, saml_provider: saml_provider, user: saml_user1) - end - - let_it_be(:saml_identity2) do - create(:group_saml_identity, saml_provider: saml_provider, user: saml_user2) - end - - let_it_be(:other_saml_user) { create(:user) } - - let_it_be(:other_saml_identity) do - create(:group_saml_identity, saml_provider: other_saml_provider, user: other_saml_user) - end - - let_it_be(:other_service_account) { create(:service_account, provisioned_by_group: other_group) } - - subject(:finder) { described_class.new(saml_user1, group, params) } - - context 'when params includes both saml users and service accounts' do - let(:params) { { include_saml_users: true, include_service_accounts: true } } - - it 'finds both saml users and service accounts' do - expect(finder.execute).to match_array([saml_user1, saml_user2, service_account1, service_account2]) - end - end - - context 'when params includes only saml users' do - let(:params) { { include_saml_users: true } } - - it 'finds only saml users' do - expect(finder.execute).to match_array([saml_user1, saml_user2]) - end - end - - context 'when params includes only service accounts' do - let(:params) { { include_service_accounts: true } } - - it 'finds only service accounts' do - expect(finder.execute).to match_array([service_account1, service_account2]) - end - end - - context 'when params do not include either saml users or service accounts' do - let(:params) { nil } - - it 'raises an argument error' do - expect { finder.execute } - .to raise_error(ArgumentError, - format( - _("At least one of %{params} must be true"), - params: described_class::ALLOWED_FILTERS.join(', ') - ) - ) - end - end - - context 'when params are all false' do - let(:params) { { include_saml_users: false, include_service_accounts: false } } - - it 'raises an argument error' do - expect { finder.execute } - .to raise_error(ArgumentError, - format( - _("At least one of %{params} must be true"), - params: described_class::ALLOWED_FILTERS.join(', ') - ) - ) - end - end - - context 'when a group has no SAML providers, SAML users, or Service Accounts' do - it 'returns no users' do - group = create(:group) - finder = described_class.new(saml_user1, group, { include_saml_users: true, include_service_accounts: true }) - - expect(finder.execute).to be_empty - end - end - end -end diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index f63ca956854729..9b66dceb9ddf3c 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -401,31 +401,6 @@ end end - describe '.with_saml_provider' do - let_it_be(:user) { create(:user) } - let_it_be(:saml_provider) { create(:saml_provider) } - - subject(:with_saml_provider) { described_class.with_saml_provider(saml_provider) } - - it 'does not find users without a SAML identity' do - expect(with_saml_provider).to be_empty - end - - context 'when users have a SAML identity tied to the provider' do - let_it_be(:saml_identity) { create(:group_saml_identity, user: user, saml_provider: saml_provider) } - - it 'finds the matching users' do - expect(with_saml_provider).to match_array([user]) - end - - it 'does not find users with a different SAML provider' do - provider = create(:saml_provider) - - expect(described_class.with_saml_provider(provider)).to be_empty - end - end - end - describe '.expired_sso_session_saml_providers_with_access_restricted' do let_it_be(:user) { create(:user) } let_it_be(:saml_provider) { create(:saml_provider) } @@ -517,34 +492,6 @@ end end - describe '.with_provisioning_group' do - let_it_be(:user) { create(:user) } - let_it_be(:group) { create(:group) } - - subject(:with_provisioning_group) { described_class.with_provisioning_group(group) } - - it 'does not find users without a provisioning group' do - expect(with_provisioning_group).to be_empty - end - - context 'when users have a provisioning group' do - before do - user.provisioned_by_group = group - user.save! - end - - it 'finds the matching users' do - expect(with_provisioning_group).to match_array([user]) - end - - it 'does not find users with a different provisioning group' do - group = create(:group) - - expect(described_class.with_provisioning_group(group)).to be_empty - end - end - end - describe '.security_policy_bots_for_projects' do let_it_be(:project_1) { create(:project) } let_it_be(:project_2) { create(:project) } diff --git a/ee/spec/requests/api/groups_spec.rb b/ee/spec/requests/api/groups_spec.rb index 7560457fbb0cc1..a7aa78104d8924 100644 --- a/ee/spec/requests/api/groups_spec.rb +++ b/ee/spec/requests/api/groups_spec.rb @@ -1914,113 +1914,6 @@ end end - describe 'GET /groups/:id/users' do - let_it_be(:group) { create(:group, :private) } - let_it_be(:regular_user) { create(:user) } - let_it_be(:saml_provider) { create(:saml_provider, group: group) } - let_it_be(:group_member_user) { create(:user) { |u| group.add_developer(u) } } - - let_it_be(:saml_user) { create(:user, :public_email) } - let_it_be(:non_saml_user) { create(:user) { |u| group.add_maintainer(u) } } - let_it_be(:service_account) { create(:service_account, provisioned_by_group: group) } - - subject(:get_users) { get api("/groups/#{group.to_param}/users", current_user), params: params } - - before do - create(:saml_provider, group: group) - create(:group_saml_identity, user: group_member_user, saml_provider: saml_provider) - create(:group_saml_identity, user: saml_user, saml_provider: saml_provider) - end - - context 'when current_user is not a group member' do - let(:params) { { include_saml_users: true, include_service_accounts: true } } - - let_it_be(:current_user) { regular_user } - - it 'returns 404' do - get_users - - expect(response).to have_gitlab_http_status(:not_found) - end - end - - shared_examples 'authorized current_user responses' do - context 'when no include params are present' do - let(:params) { {} } - - it 'returns 400' do - get_users - - expect(response).to have_gitlab_http_status(:bad_request) - end - end - - context 'when no include params are true' do - let(:params) { { include_saml_users: false } } - - it 'returns 404' do - get_users - - expect(response).to have_gitlab_http_status(:bad_request) - end - end - - context 'when all include params are present' do - let(:params) { { include_saml_users: true, include_service_accounts: true } } - - it 'returns a list of matching users' do - get_users - - expect(json_response.pluck('id')).to match_array([group_member_user.id, saml_user.id, service_account.id]) - end - end - - context 'when only include_saml_users is true' do - let(:params) { { include_saml_users: true } } - - it 'returns only SAML users' do - get_users - - expect(json_response.pluck('id')).to match_array([group_member_user.id, saml_user.id]) - end - end - - context 'when only include_service_accounts is true' do - let(:params) { { include_service_accounts: true } } - - it 'returns only service accounts' do - get_users - - expect(json_response.pluck('id')).to match_array([service_account.id]) - end - end - - context 'optional params' do - context 'search param' do - let(:params) { { include_saml_users: true, include_service_accounts: true, search: saml_user.email } } - - it 'filters by search' do - get_users - - expect(json_response.pluck('id')).to match_array([saml_user.id]) - end - end - end - end - - context 'when current_user is an admin', :enable_admin_mode do - let_it_be(:current_user) { create(:admin) } - - it_behaves_like 'authorized current_user responses' - end - - context 'when current_user is a group member' do - let_it_be(:current_user) { group_member_user } - - it_behaves_like 'authorized current_user responses' - end - end - shared_examples_for 'when unauthenticated' do it_behaves_like '401 response' do let(:message) { '401 Unauthorized' } diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 408f6c07beecae..cd21c420e879b6 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -9152,9 +9152,6 @@ msgstr "" msgid "At least one field of %{one_of_required_fields} must be present" msgstr "" -msgid "At least one of %{params} must be true" -msgstr "" - msgid "At least one of group_id or project_id must be specified" msgstr "" -- GitLab From 6912b75b5f668b8418d7433517e9c73787c527f7 Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Wed, 30 Apr 2025 14:13:07 +0300 Subject: [PATCH 4/6] Remove ee/app/finders/groups/users_finder.rb from bounded_contexts.yml --- .rubocop_todo/gitlab/bounded_contexts.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.rubocop_todo/gitlab/bounded_contexts.yml b/.rubocop_todo/gitlab/bounded_contexts.yml index 3e1f50db0b636c..3f1965be44a945 100644 --- a/.rubocop_todo/gitlab/bounded_contexts.yml +++ b/.rubocop_todo/gitlab/bounded_contexts.yml @@ -2126,7 +2126,6 @@ Gitlab/BoundedContexts: - 'ee/app/finders/gpg_keys_finder.rb' - 'ee/app/finders/group_saml_identity_finder.rb' - 'ee/app/finders/groups/saved_replies_finder.rb' - - 'ee/app/finders/groups/users_finder.rb' - 'ee/app/finders/groups_with_templates_finder.rb' - 'ee/app/finders/incident_management/escalation_policies_finder.rb' - 'ee/app/finders/incident_management/escalation_rules_finder.rb' -- GitLab From d63f60433e96c8a15a33c8f61db9d31bc4f379f5 Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Wed, 7 May 2025 13:45:21 +0300 Subject: [PATCH 5/6] Retain User.with_saml_provider and User.with_provisioning_group scopes We need them in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/190070 --- ee/app/models/ee/user.rb | 6 ++++ ee/spec/models/ee/user_spec.rb | 53 ++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index cbdd5294d18913..e7243a29cc498a 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -200,6 +200,12 @@ module User scope :with_provider, ->(provider) do joins(:identities).where(identities: { provider: provider }) end + scope :with_saml_provider, ->(saml_provider) do + joins(:identities).where(identities: { saml_provider: saml_provider }) + end + scope :with_provisioning_group, ->(group) do + joins(:user_detail).where(user_detail: { provisioned_by_group: group }) + end scope :with_invalid_expires_at_tokens, ->(expiration_date) do where(id: ::PersonalAccessToken.with_invalid_expires_at(expiration_date).select(:user_id)) diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index 9b66dceb9ddf3c..f63ca956854729 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -401,6 +401,31 @@ end end + describe '.with_saml_provider' do + let_it_be(:user) { create(:user) } + let_it_be(:saml_provider) { create(:saml_provider) } + + subject(:with_saml_provider) { described_class.with_saml_provider(saml_provider) } + + it 'does not find users without a SAML identity' do + expect(with_saml_provider).to be_empty + end + + context 'when users have a SAML identity tied to the provider' do + let_it_be(:saml_identity) { create(:group_saml_identity, user: user, saml_provider: saml_provider) } + + it 'finds the matching users' do + expect(with_saml_provider).to match_array([user]) + end + + it 'does not find users with a different SAML provider' do + provider = create(:saml_provider) + + expect(described_class.with_saml_provider(provider)).to be_empty + end + end + end + describe '.expired_sso_session_saml_providers_with_access_restricted' do let_it_be(:user) { create(:user) } let_it_be(:saml_provider) { create(:saml_provider) } @@ -492,6 +517,34 @@ end end + describe '.with_provisioning_group' do + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group) } + + subject(:with_provisioning_group) { described_class.with_provisioning_group(group) } + + it 'does not find users without a provisioning group' do + expect(with_provisioning_group).to be_empty + end + + context 'when users have a provisioning group' do + before do + user.provisioned_by_group = group + user.save! + end + + it 'finds the matching users' do + expect(with_provisioning_group).to match_array([user]) + end + + it 'does not find users with a different provisioning group' do + group = create(:group) + + expect(described_class.with_provisioning_group(group)).to be_empty + end + end + end + describe '.security_policy_bots_for_projects' do let_it_be(:project_1) { create(:project) } let_it_be(:project_2) { create(:project) } -- GitLab From f8ef27440780652bde2a4a160716b65541b0a7d3 Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Fri, 18 Jul 2025 19:04:29 +0300 Subject: [PATCH 6/6] Run `bin/rake gitlab:openapi:generate` --- doc/api/openapi/openapi_v2.yaml | 62 --------------------------------- 1 file changed, 62 deletions(-) diff --git a/doc/api/openapi/openapi_v2.yaml b/doc/api/openapi/openapi_v2.yaml index 3d4d0e0472b17d..f90e894b155731 100644 --- a/doc/api/openapi/openapi_v2.yaml +++ b/doc/api/openapi/openapi_v2.yaml @@ -2418,68 +2418,6 @@ paths: tags: - groups operationId: getApiV4GroupsIdProvisionedUsers - "/api/v4/groups/{id}/users": - get: - description: Get a list of users for the group - produces: - - application/json - parameters: - - in: query - name: search - description: Search users by name, email or username - type: string - required: false - - in: query - name: active - description: Filters only active users - type: boolean - default: false - required: false - - in: query - name: include_saml_users - description: Return users with a SAML identity in this group - type: boolean - required: false - - in: query - name: include_service_accounts - description: Return service accounts owned by this group - type: boolean - required: false - - in: query - name: page - description: Current page number - type: integer - format: int32 - default: 1 - required: false - example: 1 - - in: query - name: per_page - description: Number of items per page - type: integer - format: int32 - default: 20 - required: false - example: 20 - - in: path - name: id - type: integer - format: int32 - required: true - responses: - '200': - description: Get a list of users for the group - schema: - "$ref": "#/definitions/API_Entities_UserPublic" - '400': - description: Bad request - '403': - description: Forbidden - '404': - description: 404 Not Found - tags: - - groups - operationId: getApiV4GroupsIdUsers "/api/v4/groups/{id}/ssh_certificates": get: summary: Get a list of Groups::SshCertificate for a Group. -- GitLab