diff --git a/.rubocop_todo/gitlab/bounded_contexts.yml b/.rubocop_todo/gitlab/bounded_contexts.yml index 3e1f50db0b636cb5e4ce224942d8e91b64468f3b..3f1965be44a9456a2262c13dc7b8c47ae9ff17cb 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' diff --git a/doc/api/groups.md b/doc/api/groups.md index 0d8ec37087124fdd0c57f94a7cacda2ff7dab1f1..0465273efa3d2e6b0f409060b62f902cb3494ab4 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/doc/api/openapi/openapi_v2.yaml b/doc/api/openapi/openapi_v2.yaml index 3d4d0e0472b17dc8189eff86da589647c7a710dc..f90e894b15573134cc56dcb03b112da052b9d1c1 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. diff --git a/ee/app/finders/groups/users_finder.rb b/ee/app/finders/groups/users_finder.rb deleted file mode 100644 index 6510f44e34c2f0d2617b8ca9dd025343e09d3eb8..0000000000000000000000000000000000000000 --- a/ee/app/finders/groups/users_finder.rb +++ /dev/null @@ -1,62 +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 - - union = ::User.from_union(relations) - union.order_id_desc - 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/policies/ee/group_policy.rb b/ee/app/policies/ee/group_policy.rb index ca964701d3871cb8c09504e9bc57f5ee7093d201..9e053f22a50f40a9fddd409eff6ef06d16eb936e 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 161446da2391fb8c8ccc3915463f0fd1c18fac5b..2eb02e99d9f5ad134661d51d50f1c51f53bb396d 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,41 +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 :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' - 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 - - use :pagination - end - # rubocop: disable CodeReuse/ActiveRecord - get ':id/users', feature_category: :user_management do - authorize! :read_saml_user, 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 26fc0bdc16a7db6dde3ec82e42ec42369315222a..0000000000000000000000000000000000000000 --- a/ee/spec/finders/groups/users_finder_spec.rb +++ /dev/null @@ -1,112 +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 - - 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 - let(:params) { { include_saml_users: true } } - - 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 - 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/policies/group_policy_spec.rb b/ee/spec/policies/group_policy_spec.rb index 9aae8da01d6c4f890e2d3649248540b7e48a1908..97f0897337f01b8afc29ee0b610ad73d821c31b2 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 ab61d50309a4840ed3b0421630eecb07daa8644d..a7aa78104d892433555be7b2c24f5ebb810b7b3d 100644 --- a/ee/spec/requests/api/groups_spec.rb +++ b/ee/spec/requests/api/groups_spec.rb @@ -1914,123 +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_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_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 - stub_licensed_features(group_saml: true) - 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 400' 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 - - 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 - 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 408f6c07beecaef1443955b92a9a8405e336507b..cd21c420e879b697f10f21992b5bb57dd5cb1251 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 ""