From 5f79f6c0bd1c10050a7dcd1fb37581f70a9d5f53 Mon Sep 17 00:00:00 2001 From: Carla Drago Date: Wed, 31 Aug 2022 12:51:38 +0200 Subject: [PATCH 01/10] Update graphql types to include project import level This change updates GraphQL types to include the project_import_level group setting. It is part of changes required to make project_import_level configurable, see https://gitlab.com/gitlab-org/gitlab/-/issues/358750 Changelog: added --- app/graphql/types/group_type.rb | 6 +++++ app/graphql/types/permission_types/group.rb | 2 +- .../types/permission_types/group_enum.rb | 3 +++ .../concerns/group_api_compatibility.rb | 4 +++ app/models/group.rb | 1 + app/models/namespace.rb | 1 + app/models/namespace_setting.rb | 1 + doc/api/graphql/reference/index.md | 3 +++ .../groups/graphql/get_group_query.rb | 1 + lib/gitlab/access.rb | 26 +++++++++++++++++++ spec/graphql/types/group_type_spec.rb | 2 +- 11 files changed, 48 insertions(+), 2 deletions(-) diff --git a/app/graphql/types/group_type.rb b/app/graphql/types/group_type.rb index 45357de55026f6..34ae292fc39b27 100644 --- a/app/graphql/types/group_type.rb +++ b/app/graphql/types/group_type.rb @@ -35,6 +35,12 @@ class GroupType < NamespaceType method: :project_creation_level_str, description: 'Permission level required to create projects in the group.' + field :project_import_level, + type: GraphQL::Types::String, + null: true, + method: :project_import_level_str, + description: 'Permission level required to import projects to the group.' + field :subgroup_creation_level, type: GraphQL::Types::String, null: true, diff --git a/app/graphql/types/permission_types/group.rb b/app/graphql/types/permission_types/group.rb index 6a1031e253223c..969f53b1cc96a3 100644 --- a/app/graphql/types/permission_types/group.rb +++ b/app/graphql/types/permission_types/group.rb @@ -5,7 +5,7 @@ module PermissionTypes class Group < BasePermissionType graphql_name 'GroupPermissions' - abilities :read_group, :create_projects + abilities :read_group, :create_projects, :import_projects end end end diff --git a/app/graphql/types/permission_types/group_enum.rb b/app/graphql/types/permission_types/group_enum.rb index f636d43790fb4f..e3e57bb9d07133 100644 --- a/app/graphql/types/permission_types/group_enum.rb +++ b/app/graphql/types/permission_types/group_enum.rb @@ -7,6 +7,9 @@ class GroupEnum < BaseEnum description 'User permission on groups' value 'CREATE_PROJECTS', value: :create_projects, description: 'Groups where the user can create projects.' + value 'IMPORT_PROJECTS', + value: :import_projects, + description: 'Groups where the user can import projects.' value 'TRANSFER_PROJECTS', value: :transfer_projects, description: 'Groups where the user can transfer projects to.' diff --git a/app/models/concerns/group_api_compatibility.rb b/app/models/concerns/group_api_compatibility.rb index f02aa2035e57f4..24db9f40a8e69d 100644 --- a/app/models/concerns/group_api_compatibility.rb +++ b/app/models/concerns/group_api_compatibility.rb @@ -12,6 +12,10 @@ def project_creation_level_str=(value) write_attribute(:project_creation_level, ::Gitlab::Access.project_creation_string_options.fetch(value)) end + def project_import_level_str + ::Gitlab::Access.project_import_string_options.key(project_import_level) + end + def subgroup_creation_level_str ::Gitlab::Access.subgroup_creation_string_options.key(subgroup_creation_level) end diff --git a/app/models/group.rb b/app/models/group.rb index 38623d91705ef9..d4506c6f5188c7 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -125,6 +125,7 @@ def of_ancestors_and_self delegate :runner_token_expiration_interval, :runner_token_expiration_interval=, :runner_token_expiration_interval_human_readable, :runner_token_expiration_interval_human_readable=, to: :namespace_settings, allow_nil: true delegate :subgroup_runner_token_expiration_interval, :subgroup_runner_token_expiration_interval=, :subgroup_runner_token_expiration_interval_human_readable, :subgroup_runner_token_expiration_interval_human_readable=, to: :namespace_settings, allow_nil: true delegate :project_runner_token_expiration_interval, :project_runner_token_expiration_interval=, :project_runner_token_expiration_interval_human_readable, :project_runner_token_expiration_interval_human_readable=, to: :namespace_settings, allow_nil: true + delegate :project_import_level, :project_import_level=, to: :namespace_settings, allow_nil: true has_one :crm_settings, class_name: 'Group::CrmSettings', inverse_of: :group diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 42f362876bb950..410257e4a827a8 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -134,6 +134,7 @@ class Namespace < ApplicationRecord :pypi_package_requests_forwarding, :npm_package_requests_forwarding, to: :package_settings + delegate :project_import_level, :project_import_level=, to: :namespace_settings, allow_nil: true after_save :reload_namespace_details diff --git a/app/models/namespace_setting.rb b/app/models/namespace_setting.rb index 6a87fba57acfdd..7e0d560fb0d83a 100644 --- a/app/models/namespace_setting.rb +++ b/app/models/namespace_setting.rb @@ -42,6 +42,7 @@ class NamespaceSetting < ApplicationRecord enabled_git_access_protocol subgroup_runner_token_expiration_interval project_runner_token_expiration_interval + project_import_level ].freeze self.primary_key = :namespace_id diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 67dea28cbd5836..ca5d3ba159d768 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -12690,6 +12690,7 @@ four standard [pagination arguments](#connection-pagination-arguments): | `parent` | [`Group`](#group) | Parent group. | | `path` | [`String!`](#string) | Path of the namespace. | | `projectCreationLevel` | [`String`](#string) | Permission level required to create projects in the group. | +| `projectImportLevel` | [`String`](#string) | Permission level required to import projects to the group. | | `recentIssueBoards` | [`BoardConnection`](#boardconnection) | List of recently visited boards of the group. Maximum size is 4. (see [Connections](#connections)) | | `repositorySizeExcessProjectCount` | [`Int!`](#int) | Number of projects in the root namespace where the repository size exceeds the limit. | | `requestAccessEnabled` | [`Boolean`](#boolean) | Indicates if users can request access to namespace. | @@ -13444,6 +13445,7 @@ Returns [`UserMergeRequestInteraction`](#usermergerequestinteraction). | Name | Type | Description | | ---- | ---- | ----------- | | `createProjects` | [`Boolean!`](#boolean) | Indicates the user can perform `create_projects` on this resource. | +| `importProjects` | [`Boolean!`](#boolean) | Indicates the user can perform `import_projects` on this resource. | | `readGroup` | [`Boolean!`](#boolean) | Indicates the user can perform `read_group` on this resource. | ### `GroupReleaseStats` @@ -20512,6 +20514,7 @@ User permission on groups. | Value | Description | | ----- | ----------- | | `CREATE_PROJECTS` | Groups where the user can create projects. | +| `IMPORT_PROJECTS` | Groups where the user can import projects. | | `TRANSFER_PROJECTS` | Groups where the user can transfer projects to. | ### `HealthStatus` diff --git a/lib/bulk_imports/groups/graphql/get_group_query.rb b/lib/bulk_imports/groups/graphql/get_group_query.rb index 0e73a7fb4b9a5c..fcd7d8630daa54 100644 --- a/lib/bulk_imports/groups/graphql/get_group_query.rb +++ b/lib/bulk_imports/groups/graphql/get_group_query.rb @@ -23,6 +23,7 @@ def to_s lfs_enabled: lfsEnabled mentions_disabled: mentionsDisabled project_creation_level: projectCreationLevel + project_import_level: projectImportLevel request_access_enabled: requestAccessEnabled require_two_factor_authentication: requireTwoFactorAuthentication share_with_group_lock: shareWithGroupLock diff --git a/lib/gitlab/access.rb b/lib/gitlab/access.rb index fa025a2658fb73..de102b8ee18d46 100644 --- a/lib/gitlab/access.rb +++ b/lib/gitlab/access.rb @@ -138,6 +138,32 @@ def project_creation_level_name(name) project_creation_options.key(name) end + def project_import_string_values + project_import_string_options.keys + end + + def project_import_level_name(name) + project_import_options.keys + end + + def project_import_options + { + s_('ProjectImportLevel|No one') => NO_ACCESS, + s_('ProjectImportLevel|Owners') => OWNER, + s_('ProjectImportLevel|Maintainers and above') => MAINTAINER, + s_('ProjectImportLevel|Developers and above') => DEVELOPER + } + end + + def project_import_string_options + { + 'no one' => NO_ACCESS, + 'owner' => OWNER, + 'maintainer' => MAINTAINER, + 'developer' => DEVELOPER + } + end + def subgroup_creation_options { s_('SubgroupCreationlevel|Owners') => OWNER_SUBGROUP_ACCESS, diff --git a/spec/graphql/types/group_type_spec.rb b/spec/graphql/types/group_type_spec.rb index 0b65778ce90541..9357df4ccf83a7 100644 --- a/spec/graphql/types/group_type_spec.rb +++ b/spec/graphql/types/group_type_spec.rb @@ -26,7 +26,7 @@ dependency_proxy_image_prefix dependency_proxy_image_ttl_policy shared_runners_setting timelogs organization_state_counts organizations contact_state_counts contacts work_item_types - recent_issue_boards ci_variables + recent_issue_boards ci_variables project_import_level ] expect(described_class).to include_graphql_fields(*expected_fields) -- GitLab From b51517c1a0192271cf2b60b493319593dd38cc68 Mon Sep 17 00:00:00 2001 From: Carla Drago Date: Wed, 31 Aug 2022 14:34:53 +0200 Subject: [PATCH 02/10] Update locales with rake task --- lib/api/helpers/groups_helpers.rb | 1 + locale/gitlab.pot | 12 ++++++++++++ 2 files changed, 13 insertions(+) diff --git a/lib/api/helpers/groups_helpers.rb b/lib/api/helpers/groups_helpers.rb index e9af50b80beea7..ce5a1013aac653 100644 --- a/lib/api/helpers/groups_helpers.rb +++ b/lib/api/helpers/groups_helpers.rb @@ -24,6 +24,7 @@ module GroupsHelpers optional :request_access_enabled, type: Boolean, desc: 'Allow users to request member access' optional :default_branch_protection, type: Integer, values: ::Gitlab::Access.protection_values, desc: 'Determine if developers can push to default branch' optional :shared_runners_setting, type: String, values: ::Namespace::SHARED_RUNNERS_SETTINGS, desc: 'Enable/disable shared runners for the group and its subgroups and projects' + optional :project_import_level, type: String, values: ::Gitlab::Access.project_import_string_values, desc: 'Determine if developers can import projects to the group', as: :project_import_level_str end params :optional_params_ee do diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 4701cabba1c799..3fb0e1f73e9484 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -31299,6 +31299,18 @@ msgstr "" msgid "ProjectFileTree|Show more" msgstr "" +msgid "ProjectImportLevel|Developers and above" +msgstr "" + +msgid "ProjectImportLevel|Maintainers and above" +msgstr "" + +msgid "ProjectImportLevel|No one" +msgstr "" + +msgid "ProjectImportLevel|Owners" +msgstr "" + msgid "ProjectLastActivity|Never" msgstr "" -- GitLab From 5478470c75e0410b30452b034e9a8a4bc2901248 Mon Sep 17 00:00:00 2001 From: Carla Drago Date: Wed, 31 Aug 2022 16:28:20 +0200 Subject: [PATCH 03/10] Update spec helper for group api --- spec/support/helpers/group_api_helpers.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/spec/support/helpers/group_api_helpers.rb b/spec/support/helpers/group_api_helpers.rb index 56c4cc121a7c54..ec3cc72011ed83 100644 --- a/spec/support/helpers/group_api_helpers.rb +++ b/spec/support/helpers/group_api_helpers.rb @@ -4,8 +4,11 @@ module GroupAPIHelpers extend self def attributes_for_group_api(params = {}) - # project_creation_level and subgroup_creation_level are Integers in the model - # but are strings in the API - attributes_for(:group, params).except(:project_creation_level, :subgroup_creation_level) + # project_creation_level, project_import_level, and subgroup_creation_level + # are Integers in the model but are strings in the API + attributes_for(:group, params).except( + :project_creation_level, + :project_import_level, + :subgroup_creation_level) end end -- GitLab From c89e5dcd2d8eb78b2a341715ea9004001dbe97e0 Mon Sep 17 00:00:00 2001 From: Carla Drago Date: Thu, 1 Sep 2022 10:34:10 +0200 Subject: [PATCH 04/10] Address undercoverage failure in pipeline --- .../graphql/current_user/groups_query_spec.rb | 36 +++++++++++++++++++ lib/gitlab/access.rb | 13 ------- 2 files changed, 36 insertions(+), 13 deletions(-) diff --git a/ee/spec/requests/api/graphql/current_user/groups_query_spec.rb b/ee/spec/requests/api/graphql/current_user/groups_query_spec.rb index 9c821a6cf4db4e..5fea5b6e2f32b7 100644 --- a/ee/spec/requests/api/graphql/current_user/groups_query_spec.rb +++ b/ee/spec/requests/api/graphql/current_user/groups_query_spec.rb @@ -77,4 +77,40 @@ end end end + + context 'when permission_scope is IMPORT_PROJECTS' do + let(:group_arguments) { { permission_scope: :IMPORT_PROJECTS } } + + Feature.enable(:project_import_level_configuration) + + before do + post_graphql(query, current_user: current_user) + end + + it_behaves_like 'a working graphql query' + + context 'when ip_restrictions feature is enabled' do + before do + stub_licensed_features(group_ip_restriction: true) + end + + context 'when check_namespace_plan setting is enabled' do + before do + stub_application_setting(check_namespace_plan: true) + end + + it_behaves_like 'no N + 1 DB queries' + end + + context 'when check_namespace_plan setting is disabled' do + before do + stub_application_setting(check_namespace_plan: false) + end + + it_behaves_like 'no N + 1 DB queries' + end + end + + Feature.disable(:project_import_level_configuration) + end end diff --git a/lib/gitlab/access.rb b/lib/gitlab/access.rb index de102b8ee18d46..6dc13558cdd600 100644 --- a/lib/gitlab/access.rb +++ b/lib/gitlab/access.rb @@ -142,19 +142,6 @@ def project_import_string_values project_import_string_options.keys end - def project_import_level_name(name) - project_import_options.keys - end - - def project_import_options - { - s_('ProjectImportLevel|No one') => NO_ACCESS, - s_('ProjectImportLevel|Owners') => OWNER, - s_('ProjectImportLevel|Maintainers and above') => MAINTAINER, - s_('ProjectImportLevel|Developers and above') => DEVELOPER - } - end - def project_import_string_options { 'no one' => NO_ACCESS, -- GitLab From f949737674ad7d1802f263dfb34750ec79485d7a Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Thu, 1 Sep 2022 11:15:31 +0000 Subject: [PATCH 05/10] Apply 1 suggestion(s) to 1 file(s) --- .../requests/api/graphql/current_user/groups_query_spec.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ee/spec/requests/api/graphql/current_user/groups_query_spec.rb b/ee/spec/requests/api/graphql/current_user/groups_query_spec.rb index 5fea5b6e2f32b7..50cab4f30d2aed 100644 --- a/ee/spec/requests/api/graphql/current_user/groups_query_spec.rb +++ b/ee/spec/requests/api/graphql/current_user/groups_query_spec.rb @@ -81,7 +81,9 @@ context 'when permission_scope is IMPORT_PROJECTS' do let(:group_arguments) { { permission_scope: :IMPORT_PROJECTS } } - Feature.enable(:project_import_level_configuration) + before do + stub_feature_flags(project_import_level_configuration: true) + end before do post_graphql(query, current_user: current_user) -- GitLab From a2ef84b350f2eafc374212fa44ced289283a41be Mon Sep 17 00:00:00 2001 From: Carla Drago Date: Thu, 1 Sep 2022 13:27:07 +0200 Subject: [PATCH 06/10] Address merge request comments This commit includes the following changes: Trigger pipeline to run-as-if-foss Add tests for group model Add feature flag config Enable correct graphql response Fix shared import service examples Replace AR association with custom method Add tests for :project_import_level_str handlling Preload namespace settings for group get request --- app/controllers/admin/groups_controller.rb | 1 + app/controllers/groups_controller.rb | 1 + app/finders/groups/user_groups_finder.rb | 6 + app/models/user.rb | 24 +++ app/policies/group_policy.rb | 10 ++ app/services/groups/base_service.rb | 9 ++ .../project_import_level_configuration.yml | 8 + doc/api/groups.md | 11 +- .../graphql/current_user/groups_query_spec.rb | 27 ---- lib/api/entities/group.rb | 1 + lib/api/groups.rb | 3 +- lib/gitlab/access.rb | 8 +- locale/gitlab.pot | 12 -- .../admin/groups_controller_spec.rb | 66 ++++++++ spec/controllers/groups_controller_spec.rb | 7 + spec/models/group_spec.rb | 15 ++ spec/models/user_spec.rb | 31 ++++ spec/policies/group_policy_spec.rb | 148 ++++++++++++++++++ .../graphql/current_user/groups_query_spec.rb | 38 ++++- spec/requests/api/groups_spec.rb | 9 +- spec/services/groups/update_service_spec.rb | 28 ++++ 21 files changed, 414 insertions(+), 49 deletions(-) create mode 100644 config/feature_flags/development/project_import_level_configuration.yml diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index f3c4244269d408..02e9abb2ced40a 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/app/controllers/admin/groups_controller.rb @@ -99,6 +99,7 @@ def allowed_group_params :two_factor_grace_period, :enabled_git_access_protocol, :project_creation_level, + :project_import_level, :subgroup_creation_level, admin_note_attributes: [ :note diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 8963b84e6d8f01..1cafa885752348 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -292,6 +292,7 @@ def group_params_attributes :two_factor_grace_period, :enabled_git_access_protocol, :project_creation_level, + :project_import_level, :subgroup_creation_level, :default_branch_protection, :default_branch_name, diff --git a/app/finders/groups/user_groups_finder.rb b/app/finders/groups/user_groups_finder.rb index b58c1323b1fcae..86ba7f0c1c1a4d 100644 --- a/app/finders/groups/user_groups_finder.rb +++ b/app/finders/groups/user_groups_finder.rb @@ -39,6 +39,8 @@ def by_permission_scope target_user.manageable_groups(include_groups_with_developer_maintainer_access: true) elsif permission_scope_transfer_projects? Groups::AcceptingProjectTransfersFinder.new(target_user).execute # rubocop: disable CodeReuse/Finder + elsif permission_scope_import_projects? + target_user.import_groups else target_user.groups end @@ -51,5 +53,9 @@ def permission_scope_create_projects? def permission_scope_transfer_projects? params[:permission_scope] == :transfer_projects end + + def permission_scope_import_projects? + params[:permission_scope] == :import_projects + end end end diff --git a/app/models/user.rb b/app/models/user.rb index 338cebad6a1956..6ed85fd0f4e38c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1112,6 +1112,30 @@ def membership_groups groups.self_and_descendants end + # Returns a subset of membership_groups where the user has project_import_level permission. + # Since membership can exist automatically through membership to a parent group, the + # user's membership access_level may only exist in the parent group settings, therefore + # a recursive method is required. + def import_groups + collection = [] + membership_groups.each do |group| + member_access_level = determine_member_access_level(self, group) + project_import_level = group.project_import_level + unless project_import_level == 0 || member_access_level < project_import_level + collection << group + end + end + Group.where(id: collection.map(&:id)) + end + + def determine_member_access_level(user, group) + if group.members.find_by(user_id: user.id) + group.members.find_by(user_id: user.id).access_level + else + determine_member_access_level(user, group.parent) + end + end + # Returns a relation of groups the user has access to, including their parent # and child groups (recursively). def all_expanded_groups diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index a2dfce6748fd01..e7460ebeac2a41 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -24,6 +24,7 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy condition(:can_change_parent_share_with_group_lock) { can?(:change_share_with_group_lock, @subject.parent) } condition(:migration_bot, scope: :user) { @user.migration_bot? } condition(:can_read_group_member) { can_read_group_member? } + condition(:can_import_projects) { can_import_projects? } desc "User is a project bot" condition(:project_bot) { user.project_bot? && access_level >= GroupMember::GUEST } @@ -232,6 +233,10 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy rule { owner }.enable :create_subgroup rule { maintainer & maintainer_can_create_group }.enable :create_subgroup + rule { can_import_projects }.policy do + enable :import_projects + end + rule { public_group | logged_in_viewable }.enable :view_globally rule { default }.enable(:request_access) @@ -339,6 +344,11 @@ def can_read_group_member? !(@subject.private? && access_level == GroupMember::NO_ACCESS) end + def can_import_projects? + import_level = @subject.project_import_level + access_level >= import_level && import_level != 0 + end + def resource_access_token_creation_allowed? resource_access_token_feature_available? && group.root_ancestor.namespace_settings.resource_access_token_creation_allowed? end diff --git a/app/services/groups/base_service.rb b/app/services/groups/base_service.rb index 9705f3a560d922..435adb6c952b8a 100644 --- a/app/services/groups/base_service.rb +++ b/app/services/groups/base_service.rb @@ -13,6 +13,7 @@ def initialize(group, user, params = {}) private def handle_namespace_settings + handle_project_import_level settings_params = params.slice(*::NamespaceSetting.allowed_namespace_settings_params) return if settings_params.empty? @@ -27,5 +28,13 @@ def handle_namespace_settings def remove_unallowed_params # overridden in EE end + + def handle_project_import_level + param = params.delete(:project_import_level_str) + return unless param && Feature.enabled?(:project_import_level_configuration) + + import_level = ::Gitlab::Access.project_import_string_options.fetch(param) + params[:project_import_level] = import_level + end end end diff --git a/config/feature_flags/development/project_import_level_configuration.yml b/config/feature_flags/development/project_import_level_configuration.yml new file mode 100644 index 00000000000000..c594cbf2a59afe --- /dev/null +++ b/config/feature_flags/development/project_import_level_configuration.yml @@ -0,0 +1,8 @@ +--- +name: project_import_level_configuration +introduced_by_url: 'https://gitlab.com/gitlab-org/gitlab/-/merge_requests/96666' +rollout_issue_url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/371643' +milestone: '15.4' +type: development +group: group::import +default_enabled: false diff --git a/doc/api/groups.md b/doc/api/groups.md index c1effc78a4d1b3..cc7f2618f46b5b 100644 --- a/doc/api/groups.md +++ b/doc/api/groups.md @@ -49,6 +49,7 @@ GET /groups "require_two_factor_authentication": false, "two_factor_grace_period": 48, "project_creation_level": "developer", + "project_import_level": "owner", "auto_devops_enabled": null, "subgroup_creation_level": "owner", "emails_disabled": null, @@ -86,6 +87,7 @@ GET /groups?statistics=true "require_two_factor_authentication": false, "two_factor_grace_period": 48, "project_creation_level": "developer", + "project_import_level": "owner", "auto_devops_enabled": null, "subgroup_creation_level": "owner", "emails_disabled": null, @@ -166,6 +168,7 @@ GET /groups/:id/subgroups "require_two_factor_authentication": false, "two_factor_grace_period": 48, "project_creation_level": "developer", + "project_import_level": "owner", "auto_devops_enabled": null, "subgroup_creation_level": "owner", "emails_disabled": null, @@ -224,6 +227,7 @@ GET /groups/:id/descendant_groups "require_two_factor_authentication": false, "two_factor_grace_period": 48, "project_creation_level": "developer", + "project_import_level": "owner", "auto_devops_enabled": null, "subgroup_creation_level": "owner", "emails_disabled": null, @@ -249,6 +253,7 @@ GET /groups/:id/descendant_groups "require_two_factor_authentication": false, "two_factor_grace_period": 48, "project_creation_level": "developer", + "project_import_level": "owner", "auto_devops_enabled": null, "subgroup_creation_level": "owner", "emails_disabled": null, @@ -822,7 +827,8 @@ Parameters: | `lfs_enabled` | boolean | no | Enable/disable Large File Storage (LFS) for the projects in this group. | | `mentions_disabled` | boolean | no | Disable the capability of a group from getting mentioned. | | `parent_id` | integer | no | The parent group ID for creating nested group. | -| `project_creation_level` | string | no | Determine if developers can create projects in the group. Can be `noone` (No one), `maintainer` (users with the Maintainer role), or `developer` (users with the Developer or Maintainer role). | +| `project_creation_level` | string | no | Determine if developers can create projects in the group. Can be `noone` (No one), `maintainer` (users with the Maintainer role), or `developer` (users with the Developer, or Maintainer role). | +| `project_import_level` | string | no | Determine if developers can import projects to the group. Can be `noone` (No one), `owner` (users with Owner role), `maintainer` (users with the Maintainer role), or `developer` (users with the Owner, Developer or Maintainer role). | | `request_access_enabled` | boolean | no | Allow users to request member access. | | `require_two_factor_authentication` | boolean | no | Require all users in this group to setup Two-factor authentication. | | `share_with_group_lock` | boolean | no | Prevent sharing a project with another group within this group. | @@ -977,7 +983,8 @@ PUT /groups/:id | `lfs_enabled` | boolean | no | Enable/disable Large File Storage (LFS) for the projects in this group. | | `mentions_disabled` | boolean | no | Disable the capability of a group from getting mentioned. | | `prevent_sharing_groups_outside_hierarchy` | boolean | no | See [Prevent group sharing outside the group hierarchy](../user/group/access_and_permissions.md#prevent-group-sharing-outside-the-group-hierarchy). This attribute is only available on top-level groups. [Introduced in GitLab 14.1](https://gitlab.com/gitlab-org/gitlab/-/issues/333721) | -| `project_creation_level` | string | no | Determine if developers can create projects in the group. Can be `noone` (No one), `maintainer` (users with the Maintainer role), or `developer` (users with the Developer or Maintainer role). | +| `project_creation_level` | string | no | Determine if developers can create projects in the group. Can be `noone` (No one), `maintainer` (users with the Maintainer role), or `developer` (users with the Developer, or Maintainer role). | +| `project_import_level` | string | no | Determine if developers can import projects to the group. Can be `noone` (No one), `owner` (users with Owner role), `maintainer` (users with the Maintainer role), or `developer` (users with the Owner, Developer, or Maintainer role). | | `request_access_enabled` | boolean | no | Allow users to request member access. | | `require_two_factor_authentication` | boolean | no | Require all users in this group to setup Two-factor authentication. | | `shared_runners_setting` | string | no | See [Options for `shared_runners_setting`](#options-for-shared_runners_setting). Enable or disable shared runners for a group's subgroups and projects. | diff --git a/ee/spec/requests/api/graphql/current_user/groups_query_spec.rb b/ee/spec/requests/api/graphql/current_user/groups_query_spec.rb index 50cab4f30d2aed..4734d4bd758b9e 100644 --- a/ee/spec/requests/api/graphql/current_user/groups_query_spec.rb +++ b/ee/spec/requests/api/graphql/current_user/groups_query_spec.rb @@ -83,36 +83,9 @@ before do stub_feature_flags(project_import_level_configuration: true) - end - - before do post_graphql(query, current_user: current_user) end it_behaves_like 'a working graphql query' - - context 'when ip_restrictions feature is enabled' do - before do - stub_licensed_features(group_ip_restriction: true) - end - - context 'when check_namespace_plan setting is enabled' do - before do - stub_application_setting(check_namespace_plan: true) - end - - it_behaves_like 'no N + 1 DB queries' - end - - context 'when check_namespace_plan setting is disabled' do - before do - stub_application_setting(check_namespace_plan: false) - end - - it_behaves_like 'no N + 1 DB queries' - end - end - - Feature.disable(:project_import_level_configuration) end end diff --git a/lib/api/entities/group.rb b/lib/api/entities/group.rb index 246fb819890694..d2d88fb1da74a7 100644 --- a/lib/api/entities/group.rb +++ b/lib/api/entities/group.rb @@ -8,6 +8,7 @@ class Group < BasicGroupDetails expose :require_two_factor_authentication expose :two_factor_grace_period expose :project_creation_level_str, as: :project_creation_level + expose :project_import_level_str, as: :project_import_level expose :auto_devops_enabled expose :subgroup_creation_level_str, as: :subgroup_creation_level expose :emails_disabled diff --git a/lib/api/groups.rb b/lib/api/groups.rb index ca99e30fbf78ea..9d6a8e7aa5fd77 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -201,10 +201,9 @@ def check_subscription!(group) use :with_custom_attributes end get feature_category: :subgroups do - groups = find_groups(declared_params(include_missing: false), params[:id]) + groups = find_groups(declared_params(include_missing: false), params[:id]).preload(:namespace_settings) # rubocop: disable CodeReuse/ActiveRecord present_groups_with_pagination_strategies params, groups end - desc 'Create a group. Available only for users who can create groups.' do success Entities::Group end diff --git a/lib/gitlab/access.rb b/lib/gitlab/access.rb index 6dc13558cdd600..c0422c9c6deb77 100644 --- a/lib/gitlab/access.rb +++ b/lib/gitlab/access.rb @@ -138,10 +138,6 @@ def project_creation_level_name(name) project_creation_options.key(name) end - def project_import_string_values - project_import_string_options.keys - end - def project_import_string_options { 'no one' => NO_ACCESS, @@ -151,6 +147,10 @@ def project_import_string_options } end + def project_import_string_values + project_import_string_options.keys + end + def subgroup_creation_options { s_('SubgroupCreationlevel|Owners') => OWNER_SUBGROUP_ACCESS, diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 3fb0e1f73e9484..4701cabba1c799 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -31299,18 +31299,6 @@ msgstr "" msgid "ProjectFileTree|Show more" msgstr "" -msgid "ProjectImportLevel|Developers and above" -msgstr "" - -msgid "ProjectImportLevel|Maintainers and above" -msgstr "" - -msgid "ProjectImportLevel|No one" -msgstr "" - -msgid "ProjectImportLevel|Owners" -msgstr "" - msgid "ProjectLastActivity|Never" msgstr "" diff --git a/spec/controllers/admin/groups_controller_spec.rb b/spec/controllers/admin/groups_controller_spec.rb index 37cb0a1f2894f6..5ecdbd35bfaedc 100644 --- a/spec/controllers/admin/groups_controller_spec.rb +++ b/spec/controllers/admin/groups_controller_spec.rb @@ -44,4 +44,70 @@ end.to change { Namespace::AdminNote.count }.by(1) end end + + describe 'PUT #members_update' do + let_it_be(:group_user) { create(:user) } + + it 'adds user to members', :aggregate_failures, :snowplow do + put :members_update, params: { + id: group, + user_id: group_user.id, + access_level: Gitlab::Access::GUEST + } + + expect(controller).to set_flash.to 'Users were successfully added.' + expect(response).to redirect_to(admin_group_path(group)) + expect(group.users).to include group_user + expect_snowplow_event( + category: 'Members::CreateService', + action: 'create_member', + label: 'admin-group-page', + property: 'existing_user', + user: admin + ) + end + + it 'can add unlimited members', :aggregate_failures do + put :members_update, params: { + id: group, + user_id: 1.upto(1000).to_a.join(','), + access_level: Gitlab::Access::GUEST + } + + expect(controller).to set_flash.to 'Users were successfully added.' + expect(response).to redirect_to(admin_group_path(group)) + end + + it 'adds no user to members', :aggregate_failures do + put :members_update, params: { + id: group, + user_id: '', + access_level: Gitlab::Access::GUEST + } + + expect(controller).to set_flash.to 'No users specified.' + expect(response).to redirect_to(admin_group_path(group)) + expect(group.users).not_to include group_user + end + + it 'updates the project_creation_level successfully' do + expect do + post :update, params: { id: group.to_param, group: { project_creation_level: ::Gitlab::Access::NO_ONE_PROJECT_ACCESS } } + end.to change { group.reload.project_creation_level }.to(::Gitlab::Access::NO_ONE_PROJECT_ACCESS) + end + + it 'updates the project_import_level successfully' do + expect do + post :update, params: { id: group.to_param, group: { project_import_level: ::Gitlab::Access::DEVELOPER } } + end.to change { group.reload.project_import_level }.to(::Gitlab::Access::DEVELOPER) + end + + it 'updates the subgroup_creation_level successfully' do + expect do + post :update, + params: { id: group.to_param, + group: { subgroup_creation_level: ::Gitlab::Access::OWNER_SUBGROUP_ACCESS } } + end.to change { group.reload.subgroup_creation_level }.to(::Gitlab::Access::OWNER_SUBGROUP_ACCESS) + end + end end diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index c4e4eeec953060..ebb715f3b2a162 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -595,6 +595,13 @@ expect(group.reload.project_creation_level).to eq(::Gitlab::Access::MAINTAINER_PROJECT_ACCESS) end + it 'updates the project_import_level successfully' do + post :update, params: { id: group.to_param, group: { project_import_level: ::Gitlab::Access::DEVELOPER } } + + expect(response).to have_gitlab_http_status(:found) + expect(group.reload.project_import_level).to eq(::Gitlab::Access::DEVELOPER) + end + context 'updating default_branch_protection' do subject do put :update, params: { id: group.to_param, group: { default_branch_protection: ::Gitlab::Access::PROTECTION_DEV_CAN_MERGE } } diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 68c2d1d3995b52..f66d71b664e60d 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -2368,6 +2368,21 @@ def define_cache_expectations(cache_key) end end + describe 'project_import_level' do + it 'defaults to owners' do + expect(group.project_import_level) + .to eq(Gitlab::Access::OWNER) + end + + it 'updates access level on namespace_settings table' do + group.project_import_level = Gitlab::Access::MAINTAINER + group.save! + + expect(NamespaceSetting.find_by(namespace_id: group.id).project_import_level) + .to eq(Gitlab::Access::MAINTAINER) + end + end + describe 'subgroup_creation_level' do it 'defaults to maintainers' do expect(group.subgroup_creation_level) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index a163441590f503..33ca3a5b9bc7bd 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -4154,6 +4154,37 @@ def login_method(login) specify { is_expected.to contain_exactly(parent_group, child_group) } end + describe '#import_groups' do + context 'where project import permissions are not met' do + let!(:user) { create(:user) } + + let!(:parent_group) { create(:group) } + let!(:child_group) { create(:group, parent: parent_group) } + let!(:other_group) { create(:group) } + + it 'inherits access level from parent does not contain any import groups' do + parent_group.add_maintainer(user) + + expect(user.import_groups).to be_empty + end + end + + context 'where project import permissions are met' do + let!(:user) { create(:user) } + let!(:parent_group) { create(:group) } + let!(:child_group) { create(:group, parent: parent_group) } + let!(:other_group) { create(:group) } + + subject { user.import_groups } + + it 'inherits access level from parent and contains correct import groups' do + parent_group.add_owner(user) + + expect(user.import_groups).to contain_exactly(parent_group, child_group) + end + end + end + describe '#authorizations_for_projects' do let!(:user) { create(:user) } diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb index 5f8e582b1aaf7e..c81a01c2226f43 100644 --- a/spec/policies/group_policy_spec.rb +++ b/spec/policies/group_policy_spec.rb @@ -718,6 +718,154 @@ end end + context 'import_project' do + context 'when group has project import level set to owner' do + before_all do + group.update!(project_import_level: ::Gitlab::Access::OWNER) + end + + context 'guest' do + let(:current_user) { guest } + + it { is_expected.to be_disallowed(:import_projects) } + end + + context 'reporter' do + let(:current_user) { reporter } + + it { is_expected.to be_disallowed(:import_projects) } + end + + context 'developer' do + let(:current_user) { developer } + + it { is_expected.to be_disallowed(:import_projects) } + end + + context 'maintainer' do + let(:current_user) { maintainer } + + it { is_expected.to be_disallowed(:import_projects) } + end + + context 'owner' do + let(:current_user) { owner } + + it { is_expected.to be_allowed(:import_projects) } + end + end + + context 'when group has project import level set to maintainer' do + before_all do + group.update!(project_import_level: ::Gitlab::Access::MAINTAINER) + end + + context 'guest' do + let(:current_user) { guest } + + it { is_expected.to be_disallowed(:import_projects) } + end + + context 'reporter' do + let(:current_user) { reporter } + + it { is_expected.to be_disallowed(:import_projects) } + end + + context 'developer' do + let(:current_user) { developer } + + it { is_expected.to be_disallowed(:import_project) } + end + + context 'maintainer' do + let(:current_user) { maintainer } + + it { is_expected.to be_allowed(:import_projects) } + end + + context 'owner' do + let(:current_user) { owner } + + it { is_expected.to be_allowed(:import_projects) } + end + end + + context 'when group has project import level set to developer' do + before_all do + group.update!(project_import_level: ::Gitlab::Access::DEVELOPER) + end + + context 'guest' do + let(:current_user) { guest } + + it { is_expected.to be_disallowed(:import_projects) } + end + + context 'reporter' do + let(:current_user) { reporter } + + it { is_expected.to be_disallowed(:import_projects) } + end + + context 'developer' do + let(:current_user) { developer } + + it { is_expected.to be_allowed(:import_projects) } + end + + context 'maintainer' do + let(:current_user) { maintainer } + + it { is_expected.to be_allowed(:import_projects) } + end + + context 'owner' do + let(:current_user) { owner } + + it { is_expected.to be_allowed(:import_projects) } + end + end + end + + context 'when group has project import level set to no one' do + before_all do + group.update!(project_import_level: ::Gitlab::Access::NO_ACCESS) + end + + context 'guest' do + let(:current_user) { guest } + + it do + is_expected.to be_disallowed(:import_projects) + end + end + + context 'reporter' do + let(:current_user) { reporter } + + it { is_expected.to be_disallowed(:import_projects) } + end + + context 'developer' do + let(:current_user) { developer } + + it { is_expected.to be_disallowed(:import_projects) } + end + + context 'maintainer' do + let(:current_user) { maintainer } + + it { is_expected.to be_disallowed(:import_projects) } + end + + context 'owner' do + let(:current_user) { owner } + + it { is_expected.to be_disallowed(:import_projects) } + end + end + it_behaves_like 'clusterable policies' do let(:clusterable) { create(:group, :crm_enabled) } let(:cluster) do diff --git a/spec/requests/api/graphql/current_user/groups_query_spec.rb b/spec/requests/api/graphql/current_user/groups_query_spec.rb index 6e36beb2afc754..74d16885beab18 100644 --- a/spec/requests/api/graphql/current_user/groups_query_spec.rb +++ b/spec/requests/api/graphql/current_user/groups_query_spec.rb @@ -10,6 +10,7 @@ let_it_be(:guest_group) { create(:group, name: 'public guest', path: 'public-guest') } let_it_be(:private_maintainer_group) { create(:group, :private, name: 'b private maintainer', path: 'b-private-maintainer', parent: root_group) } let_it_be(:public_developer_group) { create(:group, project_creation_level: nil, name: 'c public developer', path: 'c-public-developer') } + let_it_be(:public_developer_group_with_import_permissions) { create(:group, project_creation_level: nil, name: 'c public developer with import', path: 'c-public-developer-import') } let_it_be(:public_maintainer_group) { create(:group, name: 'a public maintainer', path: 'a-public-maintainer', parent: root_group) } let_it_be(:public_owner_group) { create(:group, name: 'a public owner', path: 'a-public-owner') } @@ -30,8 +31,12 @@ guest_group.add_guest(user) private_maintainer_group.add_maintainer(user) public_developer_group.add_developer(user) + public_developer_group_with_import_permissions.add_developer(user) public_maintainer_group.add_maintainer(user) public_owner_group.add_owner(user) + + public_developer_group_with_import_permissions.project_import_level = ::Gitlab::Access::DEVELOPER + public_developer_group_with_import_permissions.namespace_settings.save! end subject { graphql_data.dig('currentUser', 'groups', 'nodes') } @@ -58,6 +63,7 @@ public_owner_group, private_maintainer_group, public_developer_group, + public_developer_group_with_import_permissions, guest_group ) ) @@ -72,7 +78,8 @@ public_maintainer_group, public_owner_group, private_maintainer_group, - public_developer_group + public_developer_group, + public_developer_group_with_import_permissions ) ) end @@ -91,6 +98,35 @@ end end + context 'when permission_scope is IMPORT_PROJECTS' do + let(:group_arguments) { { permission_scope: :IMPORT_PROJECTS } } + + before do + stub_feature_flags(project_import_level_configuration: true) + end + + specify do + is_expected.to match( + expected_group_hash( + public_owner_group, + public_developer_group_with_import_permissions + ) + ) + end + + context 'when search is provided' do + let(:group_arguments) { { permission_scope: :IMPORT_PROJECTS, search: 'owner' } } + + specify do + is_expected.to match( + expected_group_hash( + public_owner_group + ) + ) + end + end + end + context 'when permission_scope is TRANSFER_PROJECTS' do let(:group_arguments) { { permission_scope: :TRANSFER_PROJECTS } } diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index 02d29601cebde6..6a6018c62390d4 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -103,7 +103,7 @@ expect do get api("/groups", admin) - end.not_to exceed_query_limit(control) + end.not_to exceed_query_limit(control.count) end context 'when statistics are requested' do @@ -662,6 +662,7 @@ def response_project_ids(json_response, key) expect(json_response['shared_projects']).to be_an Array expect(json_response['shared_projects'].length).to eq(1) expect(json_response['shared_projects'][0]['id']).to eq(project.id) + expect(json_response['project_import_level']).to eq('owner') end it "returns one of user1's groups without projects when with_projects option is set to false" do @@ -905,6 +906,10 @@ def make_upload_request end context 'when authenticated as the group owner' do + before do + stub_feature_flags(project_import_level_configuration: true) + end + it 'updates the group' do workhorse_form_with_file( api("/groups/#{group1.id}", user1), @@ -914,6 +919,7 @@ def make_upload_request name: new_group_name, request_access_enabled: true, project_creation_level: "noone", + project_import_level: "maintainer", subgroup_creation_level: "maintainer", default_branch_protection: ::Gitlab::Access::MAINTAINER_PROJECT_ACCESS, prevent_sharing_groups_outside_hierarchy: true, @@ -943,6 +949,7 @@ def make_upload_request expect(json_response['default_branch_protection']).to eq(::Gitlab::Access::MAINTAINER_PROJECT_ACCESS) expect(json_response['avatar_url']).to end_with('dk.png') expect(json_response['prevent_sharing_groups_outside_hierarchy']).to eq(true) + expect(json_response['project_import_level']).to eq("maintainer") end it 'removes the group avatar' do diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 5c87b9ac8bb1bd..ab1bf19c01b1a8 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -53,6 +53,34 @@ end end + context 'updating project_import_level setting' do + let!(:group) { create(:group) } + + context 'with project_import_level_configuration enabled' do + before do + stub_feature_flags(project_import_level_configuration: true) + end + + it 'updates the project_import_level' do + expect(update_group(group, user, project_import_level_str: 'developer')).to be true + expect(group.errors).to be_empty + expect(group.project_import_level).to eq(30) + end + end + + context 'with project_import_level_configuration disabled' do + before do + stub_feature_flags(project_import_level_configuration: false) + end + + it 'ignores the project_import_level_str param' do + expect { update_group(group, user, project_import_level_str: 'developer') } + .to not_change { group.project_import_level } + expect(group.project_import_level).to eq(50) + end + end + end + context "project visibility_level validation" do context "public group with public projects" do let!(:service) { described_class.new(public_group, user, visibility_level: Gitlab::VisibilityLevel::INTERNAL) } -- GitLab From 3d81934c3fe02e6196b3c1a4df39731091fa3c25 Mon Sep 17 00:00:00 2001 From: Carla Drago Date: Wed, 14 Sep 2022 16:30:33 +0200 Subject: [PATCH 07/10] Improve import_groups method --- app/models/user.rb | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 6ed85fd0f4e38c..4cbd4b20ebef68 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -178,6 +178,13 @@ def update_tracked_fields!(request) source: :group has_many :minimal_access_group_members, -> { where(access_level: [Gitlab::Access::MINIMAL_ACCESS]) }, class_name: 'GroupMember' has_many :minimal_access_groups, through: :minimal_access_group_members, source: :group + has_many :root_ancestor_import_groups, + -> { + joins(:namespace_settings) + .where("namespace_settings.project_import_level<>0") + .where("members.access_level>=namespace_settings.project_import_level") + }, + through: :group_members, source: :group # Projects has_many :groups_projects, through: :groups, source: :projects @@ -1112,28 +1119,20 @@ def membership_groups groups.self_and_descendants end - # Returns a subset of membership_groups where the user has project_import_level permission. - # Since membership can exist automatically through membership to a parent group, the - # user's membership access_level may only exist in the parent group settings, therefore - # a recursive method is required. + # Returns all groups where the user has project_import_level permission, + # i.e. where member.access_level is >= group.project_import_level. + # Since membership is inherited automatically on descendant sub-groups but not + # stored in the database, the user's membership access_level is determined + # thought the root_ancestor group members. def import_groups collection = [] - membership_groups.each do |group| - member_access_level = determine_member_access_level(self, group) + root_ancestor_import_groups.self_and_descendants.each do |group| project_import_level = group.project_import_level - unless project_import_level == 0 || member_access_level < project_import_level + member_access_level = group.root_ancestor.members.find_by(user_id: self.id).access_level + unless group.project_import_level == 0 || member_access_level < project_import_level collection << group end end - Group.where(id: collection.map(&:id)) - end - - def determine_member_access_level(user, group) - if group.members.find_by(user_id: user.id) - group.members.find_by(user_id: user.id).access_level - else - determine_member_access_level(user, group.parent) - end end # Returns a relation of groups the user has access to, including their parent -- GitLab From 17c16722ee2d09d8aaf9d1ef7975d8b682273244 Mon Sep 17 00:00:00 2001 From: Carla Drago Date: Wed, 14 Sep 2022 18:29:02 +0200 Subject: [PATCH 08/10] Return an active record relation --- app/models/user.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/user.rb b/app/models/user.rb index 4cbd4b20ebef68..3c6a1fa8514897 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1133,6 +1133,7 @@ def import_groups collection << group end end + Group.where(id: collection.map(&:id)) end # Returns a relation of groups the user has access to, including their parent -- GitLab From a97eae2ca36f739ee92a0ea1874aa573c378054c Mon Sep 17 00:00:00 2001 From: Carla Drago Date: Tue, 27 Sep 2022 17:38:14 +0200 Subject: [PATCH 09/10] Remove replicated model delegation --- app/models/group.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/models/group.rb b/app/models/group.rb index d4506c6f5188c7..38623d91705ef9 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -125,7 +125,6 @@ def of_ancestors_and_self delegate :runner_token_expiration_interval, :runner_token_expiration_interval=, :runner_token_expiration_interval_human_readable, :runner_token_expiration_interval_human_readable=, to: :namespace_settings, allow_nil: true delegate :subgroup_runner_token_expiration_interval, :subgroup_runner_token_expiration_interval=, :subgroup_runner_token_expiration_interval_human_readable, :subgroup_runner_token_expiration_interval_human_readable=, to: :namespace_settings, allow_nil: true delegate :project_runner_token_expiration_interval, :project_runner_token_expiration_interval=, :project_runner_token_expiration_interval_human_readable, :project_runner_token_expiration_interval_human_readable=, to: :namespace_settings, allow_nil: true - delegate :project_import_level, :project_import_level=, to: :namespace_settings, allow_nil: true has_one :crm_settings, class_name: 'Group::CrmSettings', inverse_of: :group -- GitLab From 4e080fd591bf50e16193051d6fb90e8c1742f50d Mon Sep 17 00:00:00 2001 From: Carla Drago Date: Tue, 11 Oct 2022 20:08:28 +0200 Subject: [PATCH 10/10] Use as_inherited_level scope for import_groups --- app/models/member.rb | 25 ++++++ app/models/user.rb | 24 +++--- spec/models/member_spec.rb | 154 +++++++++++++++++++++++++++++++++++++ 3 files changed, 189 insertions(+), 14 deletions(-) diff --git a/app/models/member.rb b/app/models/member.rb index f23705816b6c57..929a0d784dc0ef 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -14,6 +14,7 @@ class Member < ApplicationRecord include UpdateHighestRole include RestrictedSignup include Gitlab::Experiment::Dsl + include AsCte AVATAR_SIZE = 40 ACCESS_REQUEST_APPROVERS_TO_BE_NOTIFIED_LIMIT = 10 @@ -184,6 +185,30 @@ class Member < ApplicationRecord unscoped.from(distinct_members, :members) end + # TODO This scope will ignore any previous `select` projection settings. + scope :as_inherited_levels, -> { + members_cte = all.as_cte(:members) + + skope = unscoped + .with(members_cte.to_arel) + .from(members_cte.table) + .joins('RIGHT OUTER JOIN "namespaces" ON namespaces.traversal_ids::bigint[] @> ARRAY[member_namespace_id]') + .where.not(access_level: nil) + + user_id_col = Member.arel_table['user_id'] + mn_id_col = 'namespaces.id AS member_namespace_id' + access_level_col = 'MAX(access_level) AS access_level' + + skope = skope.reselect(user_id_col, mn_id_col, access_level_col) + + skope = skope.group( + Member.arel_table['user_id'], + Namespace.arel_table['id'] + ) + + skope + } + scope :order_name_asc, -> do build_keyset_order_on_joined_column( scope: left_join_users, diff --git a/app/models/user.rb b/app/models/user.rb index 3c6a1fa8514897..a92db5d04d3727 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -178,13 +178,6 @@ def update_tracked_fields!(request) source: :group has_many :minimal_access_group_members, -> { where(access_level: [Gitlab::Access::MINIMAL_ACCESS]) }, class_name: 'GroupMember' has_many :minimal_access_groups, through: :minimal_access_group_members, source: :group - has_many :root_ancestor_import_groups, - -> { - joins(:namespace_settings) - .where("namespace_settings.project_import_level<>0") - .where("members.access_level>=namespace_settings.project_import_level") - }, - through: :group_members, source: :group # Projects has_many :groups_projects, through: :groups, source: :projects @@ -1123,17 +1116,20 @@ def membership_groups # i.e. where member.access_level is >= group.project_import_level. # Since membership is inherited automatically on descendant sub-groups but not # stored in the database, the user's membership access_level is determined - # thought the root_ancestor group members. + # through a Member scope :as_inherited_levels, which returns a collection of + # Member records that include `access_level`, `user_id`, and `member_namespace_id` + # attributes. def import_groups collection = [] - root_ancestor_import_groups.self_and_descendants.each do |group| - project_import_level = group.project_import_level - member_access_level = group.root_ancestor.members.find_by(user_id: self.id).access_level - unless group.project_import_level == 0 || member_access_level < project_import_level - collection << group + Member.as_inherited_levels.where(user_id: self.id).each do |m| # rubocop: disable Rails/FindEach + namespace = Namespace.find(m.member_namespace_id) + member_access_level = m.access_level + project_import_level = namespace.project_import_level + unless project_import_level.nil? || project_import_level == 0 || member_access_level < project_import_level + collection << namespace end end - Group.where(id: collection.map(&:id)) + collection end # Returns a relation of groups the user has access to, including their parent diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb index 414a201aa34528..98954a81ce877b 100644 --- a/spec/models/member_spec.rb +++ b/spec/models/member_spec.rb @@ -658,6 +658,160 @@ end end + shared_context 'with inherited levels' do + let!(:user) { create(:user) } + let!(:root) { create :group } + let!(:subgroup1) { create :group, parent: root } + let!(:subgroup2) { create :group, parent: root } + let!(:subsubgroup) { create :group, parent: subgroup1 } + end + + describe '.as_inherited_levels' do + include_context 'with inherited levels' + + subject { Member.where(user: user).as_inherited_levels } + + context 'when member of hierarchy' do + before do + root.add_developer user + end + + it 'assigns access level to inherited namespaces' do + expect(subject).to include( + an_object_having_attributes( + 'user_id' => user.id, + 'member_namespace_id' => root.id, + 'access_level' => Gitlab::Access::DEVELOPER + ), + an_object_having_attributes( + 'user_id' => user.id, + 'member_namespace_id' => subgroup1.id, + 'access_level' => Gitlab::Access::DEVELOPER + ), + an_object_having_attributes( + 'user_id' => user.id, + 'member_namespace_id' => subgroup2.id, + 'access_level' => Gitlab::Access::DEVELOPER + ), + an_object_having_attributes( + 'user_id' => user.id, + 'member_namespace_id' => subsubgroup.id, + 'access_level' => Gitlab::Access::DEVELOPER + ) + ) + end + end + + context 'with nested memberships in hierarchy' do + before do + root.add_developer user + subgroup1.add_maintainer user + end + + it 'assigns access level to nested inherited namespaces' do + expect(subject).to include( + an_object_having_attributes( + 'user_id' => user.id, + 'member_namespace_id' => root.id, + 'access_level' => Gitlab::Access::DEVELOPER + ), + an_object_having_attributes( + 'user_id' => user.id, + 'member_namespace_id' => subgroup1.id, + 'access_level' => Gitlab::Access::MAINTAINER + ), + an_object_having_attributes( + 'user_id' => user.id, + 'member_namespace_id' => subgroup2.id, + 'access_level' => Gitlab::Access::DEVELOPER + ), + an_object_having_attributes( + 'user_id' => user.id, + 'member_namespace_id' => subsubgroup.id, + 'access_level' => Gitlab::Access::MAINTAINER + ) + ) + end + end + end + + describe '.with_inherited' do + include_context 'with inherited levels' + + subject { Member.where(user: user).with_inherited } + + context 'when member of hierarchy' do + before do + root.add_developer user + end + + it 'assigns access level to inherited namespaces' do + expect(subject).to include( + an_object_having_attributes( + 'id' => an_instance_of(Integer), + 'user_id' => user.id, + 'member_namespace_id' => root.id, + 'access_level' => Gitlab::Access::DEVELOPER + ), + an_object_having_attributes( + 'id' => nil, + 'user_id' => user.id, + 'member_namespace_id' => subgroup1.id, + 'access_level' => Gitlab::Access::DEVELOPER + ), + an_object_having_attributes( + 'id' => nil, + 'user_id' => user.id, + 'member_namespace_id' => subgroup2.id, + 'access_level' => Gitlab::Access::DEVELOPER + ), + an_object_having_attributes( + 'id' => nil, + 'user_id' => user.id, + 'member_namespace_id' => subsubgroup.id, + 'access_level' => Gitlab::Access::DEVELOPER + ) + ) + end + end + + context 'with nested memberships in hierarchy' do + before do + root.add_developer user + subgroup1.add_maintainer user + end + + it 'assigns access level to nested inherited namespaces' do + expect(subject).to include( + an_object_having_attributes( + 'id' => an_instance_of(Integer), + 'user_id' => user.id, + 'member_namespace_id' => root.id, + 'access_level' => Gitlab::Access::DEVELOPER + ), + an_object_having_attributes( + 'id' => an_instance_of(Integer), + 'user_id' => user.id, + 'member_namespace_id' => subgroup1.id, + 'access_level' => Gitlab::Access::MAINTAINER + ), + an_object_having_attributes( + 'id' => nil, + 'user_id' => user.id, + 'member_namespace_id' => subgroup2.id, + 'access_level' => Gitlab::Access::DEVELOPER + ), + an_object_having_attributes( + 'id' => nil, + 'user_id' => user.id, + 'member_namespace_id' => subsubgroup.id, + 'access_level' => Gitlab::Access::MAINTAINER + ) + ) + end + end + end + describe '.with_invited_user_state' do subject(:with_invited_user_state) { described_class.with_invited_user_state } -- GitLab