diff options
| author | GitLab Bot <gitlab-bot@gitlab.com> | 2023-12-21 12:17:08 +0300 |
|---|---|---|
| committer | GitLab Bot <gitlab-bot@gitlab.com> | 2023-12-21 12:17:08 +0300 |
| commit | 4ecd816dcbbf2c3a83087ea1add13f087530e9eb (patch) | |
| tree | faf1d225bf16fa64dea1244217b3f8b6e7dac46d | |
| parent | a293ae1ab5e4253f6003123c79c00bf7b953a7e5 (diff) | |
Add latest changes from gitlab-org/gitlab@master
27 files changed, 202 insertions, 437 deletions
diff --git a/.gitlab/ci/review-apps/qa.gitlab-ci.yml b/.gitlab/ci/review-apps/qa.gitlab-ci.yml index 6030db7264d..8ea51fd89ea 100644 --- a/.gitlab/ci/review-apps/qa.gitlab-ci.yml +++ b/.gitlab/ci/review-apps/qa.gitlab-ci.yml @@ -2,17 +2,6 @@ include: - local: .gitlab/ci/qa-common/main.gitlab-ci.yml - template: Verify/Browser-Performance.gitlab-ci.yml -.test-variables: - variables: - QA_GENERATE_ALLURE_REPORT: "true" - QA_CAN_TEST_PRAEFECT: "false" - GITLAB_USERNAME: "root" - GITLAB_PASSWORD: "${REVIEW_APPS_ROOT_PASSWORD}" - GITLAB_ADMIN_USERNAME: "root" - GITLAB_ADMIN_PASSWORD: "${REVIEW_APPS_ROOT_PASSWORD}" - GITLAB_QA_ADMIN_ACCESS_TOKEN: "${REVIEW_APPS_ROOT_TOKEN}" - GITHUB_ACCESS_TOKEN: "${QA_GITHUB_ACCESS_TOKEN}" - .bundle-base: extends: - .qa-cache @@ -20,30 +9,34 @@ include: before_script: - cd qa && bundle install -.review-qa-base: - image: ${REGISTRY_HOST}/${REGISTRY_GROUP}/gitlab-build-images/debian-${DEBIAN_VERSION}-ruby-${RUBY_VERSION}:bundler-2.3-git-2.36-lfs-2.9-chrome-${CHROME_VERSION}-docker-${DOCKER_VERSION}-gcloud-383-kubectl-1.23 +review-qa-smoke: extends: - .use-docker-in-docker - .bundle-base - - .test-variables + - .default-retry + - .rules:qa-smoke + image: ${REGISTRY_HOST}/${REGISTRY_GROUP}/gitlab-build-images/debian-${DEBIAN_VERSION}-ruby-${RUBY_VERSION}:bundler-2.3-git-2.36-lfs-2.9-chrome-${CHROME_VERSION}-docker-${DOCKER_VERSION}-gcloud-383-kubectl-1.23 stage: qa - needs: - - review-deploy - - download-knapsack-report - - pipeline: $PARENT_PIPELINE_ID - job: retrieve-tests-metadata + needs: [review-deploy] variables: - GIT_LFS_SKIP_SMUDGE: 1 - WD_INSTALL_DIR: /usr/local/bin RSPEC_REPORT_OPTS: --force-color --order random --format documentation --format RspecJunitFormatter --out tmp/rspec-${CI_JOB_ID}.xml + GITLAB_USERNAME: "root" + GITLAB_PASSWORD: "${REVIEW_APPS_ROOT_PASSWORD}" + GITLAB_ADMIN_USERNAME: "root" + GITLAB_ADMIN_PASSWORD: "${REVIEW_APPS_ROOT_PASSWORD}" + GITLAB_QA_ADMIN_ACCESS_TOKEN: "${REVIEW_APPS_ROOT_TOKEN}" + GITHUB_ACCESS_TOKEN: "${QA_GITHUB_ACCESS_TOKEN}" + COLORIZED_LOGS: "true" + QA_GENERATE_ALLURE_REPORT: "true" + QA_CAN_TEST_PRAEFECT: "false" script: - - QA_COMMAND="bundle exec bin/qa ${QA_SCENARIO} ${QA_GITLAB_URL} -- ${QA_TESTS} ${RSPEC_REPORT_OPTS}" + - QA_COMMAND="bundle exec bin/qa Test::Instance::Smoke ${QA_GITLAB_URL} -- ${QA_TESTS} ${RSPEC_REPORT_OPTS}" - echo "Running - '${QA_COMMAND}'" - eval "$QA_COMMAND" after_script: - | echo "Sentry errors for the current review-app test run can be found via following url:" - echo "https://sentry.gitlab.net/gitlab/gitlab-review-apps/releases/$(echo "${CI_COMMIT_SHA}" | cut -c1-11)/all-events/." + echo "https://new-sentry.gitlab.net/organizations/gitlab/releases/$(echo "${CI_COMMIT_SHA}" | cut -c1-11)/?environment=review&issuesType=all&project=19" artifacts: paths: - qa/tmp @@ -52,61 +45,18 @@ include: expire_in: 7 days when: always -# Store knapsack report as artifact so the same report is reused across all jobs -download-knapsack-report: - extends: - - .bundle-base - - .rules:prepare-report - stage: prepare - script: - - bundle exec rake "knapsack:download[qa]" - allow_failure: true - artifacts: - paths: - - qa/knapsack/review-qa-*.json - expire_in: 1 day - -review-qa-smoke: - extends: - - .review-qa-base - - .rules:qa-smoke - variables: - QA_SCENARIO: Test::Instance::Smoke - -review-qa-blocking: - extends: - - .review-qa-base - - .rules:qa-blocking - variables: - QA_SCENARIO: Test::Instance::ReviewBlocking - retry: 1 -review-qa-blocking-parallel: - extends: - - review-qa-blocking - - .rules:qa-blocking-parallel - parallel: 10 - -review-qa-non-blocking: - extends: - - .review-qa-base - - .rules:qa-non-blocking - variables: - QA_SCENARIO: Test::Instance::ReviewNonBlocking - when: manual - allow_failure: true -review-qa-non-blocking-parallel: - extends: - - review-qa-non-blocking - - .rules:qa-non-blocking-parallel - parallel: 5 - browser_performance: extends: - .default-retry - .review:rules:review-performance + image: ${GITLAB_DEPENDENCY_PROXY_ADDRESS}docker:${DOCKER_VERSION}-git + services: + - docker:${DOCKER_VERSION}-dind stage: qa needs: ["review-deploy"] variables: + DOCKER_HOST: tcp://docker:2375 + DOCKER_TLS_CERTDIR: "" URL: environment_url.txt e2e-test-report: @@ -115,26 +65,6 @@ e2e-test-report: variables: ALLURE_RESULTS_GLOB: "qa/tmp/allure-results" -upload-knapsack-report: - extends: - - .generate-knapsack-report-base - - .bundle-base - stage: report - variables: - QA_KNAPSACK_REPORT_FILE_PATTERN: $CI_PROJECT_DIR/qa/tmp/knapsack/*/*.json - -delete-test-resources: - extends: - - .bundle-base - - .rules:prepare-report - stage: report - variables: - GITLAB_QA_ACCESS_TOKEN: $REVIEW_APPS_ROOT_TOKEN - script: - - export GITLAB_ADDRESS="$QA_GITLAB_URL" - - bundle exec rake "test_resources:delete[$CI_PROJECT_DIR/qa/tmp/test-resources-*.json]" - allow_failure: true - notify-slack: extends: - .notify-slack diff --git a/.gitlab/ci/review-apps/rules.gitlab-ci.yml b/.gitlab/ci/review-apps/rules.gitlab-ci.yml index a4b667c6645..3c8169a4722 100644 --- a/.gitlab/ci/review-apps/rules.gitlab-ci.yml +++ b/.gitlab/ci/review-apps/rules.gitlab-ci.yml @@ -1,19 +1,6 @@ # ------------------------------------------ # Conditions # ------------------------------------------ -# Specific specs passed -.specific-specs: &specific-specs - if: $QA_TESTS != "" - -# No specific specs passed -.all-specs: &all-specs - if: $QA_TESTS == "" - -# No specific specs in mr pipeline -.all-specs-mr: &all-specs-mr - if: '($CI_MERGE_REQUEST_EVENT_TYPE == "merged_result" || $CI_MERGE_REQUEST_EVENT_TYPE == "detached") && $QA_TESTS == ""' - when: manual - # Triggered by change pattern .app-changes: &app-changes if: $APP_CHANGE_TRIGGER == "true" @@ -48,29 +35,6 @@ - "{,ee/,jh/}{bin,config}/**/*.rb" # ------------------------------------------ -# Conditions set -# ------------------------------------------ -.qa-manual: &qa-manual - when: manual - allow_failure: true - variables: - QA_TESTS: "" - -.never-when-qa-run-all-tests-or-no-specific-specs: - - <<: *qa-run-all-tests - when: never - - <<: *all-specs - when: never - -.never-when-specific-specs-always-when-qa-run-all-tests: - - *qa-run-all-tests - - <<: *specific-specs - when: manual - allow_failure: true - variables: - QA_TESTS: "" - -# ------------------------------------------ # Prepare # ------------------------------------------ .rules:dont-interrupt: @@ -112,32 +76,11 @@ QA_TESTS: "" # unset QA_TESTS even if specific tests were inferred from stage label - *qa-run-all-tests - if: $QA_SUITES =~ /Test::Instance::Smoke/ - - *qa-manual - -.rules:qa-blocking: - rules: - - <<: *app-changes - when: never - - !reference [.never-when-qa-run-all-tests-or-no-specific-specs] - - if: $QA_SUITES =~ /Test::Instance::ReviewBlocking/ -.rules:qa-blocking-parallel: - rules: - # always trigger blocking suite if review pipeline got triggered by specific changes in application code - - <<: *app-changes + # keep option to trigger tests manually even if no rules match + - when: manual + allow_failure: true variables: - QA_TESTS: "" # unset QA_TESTS even if specific tests were inferred from stage label - - !reference [.never-when-specific-specs-always-when-qa-run-all-tests] - - if: $QA_SUITES =~ /Test::Instance::ReviewBlocking/ - -.rules:qa-non-blocking: - rules: - - !reference [.never-when-qa-run-all-tests-or-no-specific-specs] - - if: $QA_SUITES =~ /Test::Instance::ReviewNonBlocking/ -.rules:qa-non-blocking-parallel: - rules: - - !reference [.never-when-specific-specs-always-when-qa-run-all-tests] - - *all-specs-mr # set full suite to manual when no specific specs passed in mr - - if: $QA_SUITES =~ /Test::Instance::ReviewNonBlocking/ + QA_TESTS: "" .review:rules:review-performance: rules: diff --git a/.gitlab/ci/rules.gitlab-ci.yml b/.gitlab/ci/rules.gitlab-ci.yml index 165c2159bdf..31b4dda4368 100644 --- a/.gitlab/ci/rules.gitlab-ci.yml +++ b/.gitlab/ci/rules.gitlab-ci.yml @@ -2675,7 +2675,6 @@ - <<: *if-dot-com-gitlab-org-schedule allow_failure: true variables: - KNAPSACK_GENERATE_REPORT: "true" QA_SAVE_TEST_METRICS: "true" QA_EXPORT_TEST_METRICS: "false" # on main runs, metrics are exported to separate bucket via rake task for better consistency diff --git a/app/models/group.rb b/app/models/group.rb index ac843f392fd..1bfe68f414a 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -37,8 +37,8 @@ class Group < Namespace has_many :all_group_members, -> { non_request }, dependent: :destroy, as: :source, class_name: 'GroupMember' # rubocop:disable Cop/ActiveRecordDependent has_many :all_owner_members, -> { non_request.all_owners }, as: :source, class_name: 'GroupMember' - has_many :group_members, -> { non_request.where.not(members: { access_level: Gitlab::Access::MINIMAL_ACCESS }) }, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent - has_many :namespace_members, -> { non_request.where.not(members: { access_level: Gitlab::Access::MINIMAL_ACCESS }).unscope(where: %i[source_id source_type]) }, + has_many :group_members, -> { non_request.non_minimal_access }, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent + has_many :namespace_members, -> { non_request.non_minimal_access.unscope(where: %i[source_id source_type]) }, foreign_key: :member_namespace_id, inverse_of: :group, class_name: 'GroupMember' alias_method :members, :group_members @@ -434,7 +434,7 @@ class Group < Namespace end def owned_by?(user) - owners.include?(user) + all_owner_members.exists?(user: user) end def add_members(users, access_level, current_user: nil, expires_at: nil) @@ -593,6 +593,12 @@ class Group < Namespace end end + # Only for direct and not requested members with higher access level than MIMIMAL_ACCESS + # It returns true for non-active users + def has_user?(user) + group_members.exists?(user: user) + end + def direct_members GroupMember.active_without_invites_and_requests .non_minimal_access diff --git a/app/models/project.rb b/app/models/project.rb index 7b996457c0d..738b9b1ef72 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -334,7 +334,7 @@ class Project < ApplicationRecord has_many :authorized_users, -> { allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/422045') }, through: :project_authorizations, source: :user, class_name: 'User' - has_many :project_members, -> { where(requested_at: nil) }, + has_many :project_members, -> { non_request }, as: :source, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent alias_method :members, :project_members has_many :namespace_members, ->(project) { where(requested_at: nil).unscope(where: %i[source_id source_type]) }, @@ -508,6 +508,7 @@ class Project < ApplicationRecord delegate :members, prefix: true delegate :add_member, :add_members, :member? delegate :add_guest, :add_reporter, :add_developer, :add_maintainer, :add_owner, :add_role + delegate :has_user? end with_options to: :namespace do diff --git a/app/models/project_team.rb b/app/models/project_team.rb index 5078642ea3a..e3791379b94 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -172,6 +172,11 @@ class ProjectTeam max_member_access(user.id) >= min_access_level end + # Only for direct and not invited members + def has_user?(user) + project.project_members.exists?(user: user) + end + def human_max_access(user_id) Gitlab::Access.human_access(max_member_access(user_id)) end diff --git a/app/policies/organizations/organization_policy.rb b/app/policies/organizations/organization_policy.rb index afd8c6e144f..d538b786f78 100644 --- a/app/policies/organizations/organization_policy.rb +++ b/app/policies/organizations/organization_policy.rb @@ -13,14 +13,12 @@ module Organizations rule { admin }.policy do enable :admin_organization - enable :create_group enable :read_organization enable :read_organization_user end rule { organization_user }.policy do enable :admin_organization - enable :create_group enable :read_organization enable :read_organization_user end diff --git a/app/services/groups/create_service.rb b/app/services/groups/create_service.rb index bb577b41fa8..21d3c6499a0 100644 --- a/app/services/groups/create_service.rb +++ b/app/services/groups/create_service.rb @@ -92,16 +92,6 @@ module Groups end end - if @group.organization && !can?(current_user, :create_group, @group.organization) - # We are unsetting this here to match behavior of invalid parent_id above and protect against possible - # committing to the database of a value that isn't allowed. - @group.organization = nil - message = s_("CreateGroup|You don't have permission to create a group in the provided organization.") - @group.errors.add(:organization_id, message) - - return false - end - true end diff --git a/app/services/groups/transfer_service.rb b/app/services/groups/transfer_service.rb index 79557dae14a..9fc1a05476e 100644 --- a/app/services/groups/transfer_service.rb +++ b/app/services/groups/transfer_service.rb @@ -236,7 +236,7 @@ module Groups def ensure_ownership return if @new_parent_group - return unless @group.owners.empty? + return unless @group.all_owner_members.empty? add_owner_on_transferred_group end diff --git a/app/views/shared/groups/_group.html.haml b/app/views/shared/groups/_group.html.haml index 38189786c24..cdcbee1bb72 100644 --- a/app/views/shared/groups/_group.html.haml +++ b/app/views/shared/groups/_group.html.haml @@ -22,7 +22,7 @@ %span.gl-ml-5.has-tooltip{ title: _('Users') } = sprite_icon('users', css_class: 'gl-vertical-align-text-bottom') - = number_with_delimiter(group.users.count) + = number_with_delimiter(group.group_members.non_invite.count) %span.gl-ml-5.visibility-icon.has-tooltip{ data: { container: 'body', placement: 'left' }, title: visibility_icon_description(group) } = visibility_level_icon(group.visibility_level) diff --git a/doc/api/groups.md b/doc/api/groups.md index bb208432a49..2dbfb5a4937 100644 --- a/doc/api/groups.md +++ b/doc/api/groups.md @@ -816,31 +816,30 @@ POST /groups Parameters: -| Attribute | Type | Required | Description | -|---------------------------------------------------------| ------- | -------- |-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| `name` | string | yes | The name of the group. | -| `path` | string | yes | The path of the group. | -| `auto_devops_enabled` | boolean | no | Default to Auto DevOps pipeline for all projects within this group. | -| `avatar` | mixed | no | Image file for avatar of the group. [Introduced in GitLab 12.9](https://gitlab.com/gitlab-org/gitlab/-/issues/36681) | -| `default_branch_protection` | integer | no | See [Options for `default_branch_protection`](#options-for-default_branch_protection). Default to the global level default branch protection setting. | -| `description` | string | no | The group's description. | -| `emails_disabled` | boolean | no | _([Deprecated](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/127899) in GitLab 16.5.)_ Disable email notifications. Use `emails_enabled` instead. | -| `emails_enabled` | boolean | no | Enable email notifications. | -| `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. | -| `organization_id` | integer | no | The organization ID for the group. | -| `parent_id` | integer | no | The parent group ID for creating nested group. | +| Attribute | Type | Required | Description | +| ------------------------------------------------------- | ------- | -------- | ----------- | +| `name` | string | yes | The name of the group. | +| `path` | string | yes | The path of the group. | +| `auto_devops_enabled` | boolean | no | Default to Auto DevOps pipeline for all projects within this group. | +| `avatar` | mixed | no | Image file for avatar of the group. [Introduced in GitLab 12.9](https://gitlab.com/gitlab-org/gitlab/-/issues/36681) | +| `default_branch_protection` | integer | no | See [Options for `default_branch_protection`](#options-for-default_branch_protection). Default to the global level default branch protection setting. | +| `description` | string | no | The group's description. | +| `emails_disabled` | boolean | no | _([Deprecated](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/127899) in GitLab 16.5.)_ Disable email notifications. Use `emails_enabled` instead. | +| `emails_enabled` | boolean | no | Enable email notifications. | +| `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). | -| `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. | -| `subgroup_creation_level` | string | no | Allowed to [create subgroups](../user/group/subgroups/index.md#create-a-subgroup). Can be `owner` (Owners), or `maintainer` (users with the Maintainer role). | -| `two_factor_grace_period` | integer | no | Time before Two-factor authentication is enforced (in hours). | -| `visibility` | string | no | The group's visibility. Can be `private`, `internal`, or `public`. | -| `membership_lock` **(PREMIUM ALL)** | boolean | no | Users cannot be added to projects in this group. | -| `extra_shared_runners_minutes_limit` **(PREMIUM SELF)** | integer | no | Can be set by administrators only. Additional compute minutes for this group. | -| `shared_runners_minutes_limit` **(PREMIUM SELF)** | integer | no | Can be set by administrators only. Maximum number of monthly compute minutes for this group. Can be `nil` (default; inherit system default), `0` (unlimited), or `> 0`. | -| `wiki_access_level` **(PREMIUM ALL)** | string | no | The wiki access level. Can be `disabled`, `private`, or `enabled`. | +| `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. | +| `subgroup_creation_level` | string | no | Allowed to [create subgroups](../user/group/subgroups/index.md#create-a-subgroup). Can be `owner` (Owners), or `maintainer` (users with the Maintainer role). | +| `two_factor_grace_period` | integer | no | Time before Two-factor authentication is enforced (in hours). | +| `visibility` | string | no | The group's visibility. Can be `private`, `internal`, or `public`. | +| `membership_lock` **(PREMIUM ALL)** | boolean | no | Users cannot be added to projects in this group. | +| `extra_shared_runners_minutes_limit` **(PREMIUM SELF)** | integer | no | Can be set by administrators only. Additional compute minutes for this group. | +| `shared_runners_minutes_limit` **(PREMIUM SELF)** | integer | no | Can be set by administrators only. Maximum number of monthly compute minutes for this group. Can be `nil` (default; inherit system default), `0` (unlimited), or `> 0`. | +| `wiki_access_level` **(PREMIUM ALL)** | string | no | The wiki access level. Can be `disabled`, `private`, or `enabled`. | ### Options for `default_branch_protection` diff --git a/lib/api/entities/group.rb b/lib/api/entities/group.rb index 14491c2396a..1a1765c2e0a 100644 --- a/lib/api/entities/group.rb +++ b/lib/api/entities/group.rb @@ -23,7 +23,6 @@ module API expose :full_name, :full_path expose :created_at expose :parent_id - expose :organization_id expose :shared_runners_setting expose :custom_attributes, using: 'API::Entities::CustomAttribute', if: :with_custom_attributes diff --git a/lib/api/groups.rb b/lib/api/groups.rb index 7b755a76f29..1ff64cd2ffd 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -213,15 +213,11 @@ module API requires :name, type: String, desc: 'The name of the group' requires :path, type: String, desc: 'The path of the group' optional :parent_id, type: Integer, desc: 'The parent group id for creating nested group' - optional :organization_id, type: Integer, desc: 'The organization id for the group' use :optional_params end post feature_category: :groups_and_projects, urgency: :low do - organization = find_organization!(params[:organization_id]) if params[:organization_id].present? - authorize! :create_group, organization if organization - - parent_group = find_group!(params[:parent_id], organization: organization) if params[:parent_id].present? + parent_group = find_group!(params[:parent_id]) if params[:parent_id].present? if parent_group authorize! :create_subgroup, parent_group else diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index a59734d643d..6cb9d19a2ad 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -211,25 +211,18 @@ module API not_found!('Pipeline') end - def find_organization!(id) - organization = Organizations::Organization.find_by_id(id) - check_organization_access(organization) - end - # rubocop: disable CodeReuse/ActiveRecord - def find_group(id, organization: nil) - collection = organization.present? ? Group.in_organization(organization) : Group.all - + def find_group(id) if id.to_s =~ INTEGER_ID_REGEX - collection.find_by(id: id) + Group.find_by(id: id) else - collection.find_by_full_path(id) + Group.find_by_full_path(id) end end # rubocop: enable CodeReuse/ActiveRecord - def find_group!(id, organization: nil) - group = find_group(id, organization: organization) + def find_group!(id) + group = find_group(id) check_group_access(group) end @@ -842,12 +835,6 @@ module API @sudo_identifier ||= params[SUDO_PARAM] || env[SUDO_HEADER] end - def check_organization_access(organization) - return organization if can?(current_user, :read_organization, organization) - - not_found!('Organization') - end - def secret_token Gitlab::Shell.secret_token end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index e106d98a6ca..f08e4c05730 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -14539,9 +14539,6 @@ msgstr "" msgid "CreateGitTag|Set tag message" msgstr "" -msgid "CreateGroup|You don't have permission to create a group in the provided organization." -msgstr "" - msgid "CreateGroup|You don’t have permission to create a subgroup in this group." msgstr "" diff --git a/spec/lib/api/entities/group_spec.rb b/spec/lib/api/entities/group_spec.rb deleted file mode 100644 index 270ac323c7d..00000000000 --- a/spec/lib/api/entities/group_spec.rb +++ /dev/null @@ -1,24 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe API::Entities::Group, feature_category: :groups_and_projects do - let_it_be(:group) do - base_group = create(:group) { |g| create(:project_statistics, namespace_id: g.id) } - Group.with_statistics.find(base_group.id) - end - - subject(:json) { described_class.new(group, { with_custom_attributes: true, statistics: true }).as_json } - - it 'returns expected data' do - expect(json.keys).to( - include( - :organization_id, :path, :description, :visibility, :share_with_group_lock, :require_two_factor_authentication, - :two_factor_grace_period, :project_creation_level, :auto_devops_enabled, - :subgroup_creation_level, :emails_disabled, :emails_enabled, :lfs_enabled, :default_branch_protection, - :default_branch_protection_defaults, :avatar_url, :request_access_enabled, :full_name, :full_path, :created_at, - :parent_id, :organization_id, :shared_runners_setting, :custom_attributes, :statistics - ) - ) - end -end diff --git a/spec/lib/api/helpers_spec.rb b/spec/lib/api/helpers_spec.rb index 89fcb4b43a6..c76694b60d3 100644 --- a/spec/lib/api/helpers_spec.rb +++ b/spec/lib/api/helpers_spec.rb @@ -406,37 +406,6 @@ RSpec.describe API::Helpers, feature_category: :shared do end end - describe '#find_organization!' do - let_it_be(:organization) { create(:organization) } - let_it_be(:user) { create(:user) } - - before do - allow(helper).to receive(:current_user).and_return(user) - allow(helper).to receive(:initial_current_user).and_return(user) - end - - context 'when user is authenticated' do - it 'returns requested organization' do - expect(helper.find_organization!(organization.id)).to eq(organization) - end - end - - context 'when user is not authenticated' do - let(:user) { nil } - - it 'returns requested organization' do - expect(helper.find_organization!(organization.id)).to eq(organization) - end - end - - context 'when organization does not exist' do - it 'returns nil' do - expect(helper).to receive(:render_api_error!).with('404 Organization Not Found', 404) - expect(helper.find_organization!(non_existing_record_id)).to be_nil - end - end - end - describe '#find_group!' do let_it_be(:group) { create(:group, :public) } let_it_be(:user) { create(:user) } @@ -488,7 +457,7 @@ RSpec.describe API::Helpers, feature_category: :shared do end end - context 'with support for IDs and paths as arguments' do + context 'support for IDs and paths as arguments' do let_it_be(:group) { create(:group) } let(:user) { group.first_owner } @@ -534,34 +503,6 @@ RSpec.describe API::Helpers, feature_category: :shared do it_behaves_like 'group finder' end end - - context 'with support for organization as an argument' do - let_it_be(:group) { create(:group) } - let_it_be(:organization) { create(:organization) } - - before do - allow(helper).to receive(:current_user).and_return(group.first_owner) - allow(helper).to receive(:job_token_authentication?).and_return(false) - allow(helper).to receive(:authenticate_non_public?).and_return(false) - end - - subject { helper.find_group!(group.id, organization: organization) } - - context 'when group exists in the organization' do - before do - group.update!(organization: organization) - end - - it { is_expected.to eq(group) } - end - - context 'when group does not exist in the organization' do - it 'returns nil' do - expect(helper).to receive(:render_api_error!).with('404 Group Not Found', 404) - is_expected.to be_nil - end - end - end end describe '#find_group_by_full_path!' do diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 1fafa64a535..fdbcd84d3df 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -1642,6 +1642,46 @@ RSpec.describe Group, feature_category: :groups_and_projects do it { expect(subject.parent).to be_kind_of(described_class) } end + describe '#has_user?' do + let_it_be(:group) { create(:group) } + let_it_be(:subgroup) { create(:group, parent: group) } + let_it_be(:user) { create(:user) } + let_it_be(:user2) { create(:user) } + + subject { group.has_user?(user) } + + context 'when the user is a member' do + before_all do + group.add_developer(user) + end + + it { is_expected.to be_truthy } + it { expect(group.has_user?(user2)).to be_falsey } + + it 'returns false for subgroup' do + expect(subgroup.has_user?(user)).to be_falsey + end + end + + context 'when the user is a member with minimal access' do + before_all do + group.add_member(user, GroupMember::MINIMAL_ACCESS) + end + + it { is_expected.to be_falsey } + end + + context 'when the user has requested membership' do + before_all do + create(:group_member, :developer, :access_request, user: user, source: group) + end + + it 'returns false' do + expect(subject).to be_falsey + end + end + end + describe '#member?' do let_it_be(:group) { create(:group) } let_it_be(:user) { create(:user) } diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index c256c4f10f8..db1754ef991 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1121,6 +1121,8 @@ RSpec.describe Project, factory_default: :keep, feature_category: :groups_and_pr end it { is_expected.to delegate_method(:members).to(:team).with_prefix(true) } + it { is_expected.to delegate_method(:has_user?).to(:team) } + it { is_expected.to delegate_method(:member?).to(:team) } it { is_expected.to delegate_method(:name).to(:owner).with_prefix(true).allow_nil } it { is_expected.to delegate_method(:root_ancestor).to(:namespace).allow_nil } it { is_expected.to delegate_method(:certificate_based_clusters_enabled?).to(:namespace).allow_nil } diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb index 10a2e967b14..47ab48a6497 100644 --- a/spec/models/project_team_spec.rb +++ b/spec/models/project_team_spec.rb @@ -341,22 +341,52 @@ RSpec.describe ProjectTeam, feature_category: :groups_and_projects do end end + describe '#has_user?' do + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, namespace: group) } + let_it_be(:user) { create(:user) } + let_it_be(:user2) { create(:user) } + + subject { project.team.has_user?(user) } + + context 'when the user is a member' do + before_all do + project.add_developer(user) + end + + it { is_expected.to be_truthy } + it { expect(group.has_user?(user2)).to be_falsey } + end + + context 'when user is a member with minimal access' do + before_all do + project.add_member(user, GroupMember::MINIMAL_ACCESS) + end + + it { is_expected.to be_falsey } + end + + context 'when user is not a direct member of the project' do + before_all do + create(:group_member, :developer, user: user, source: group) + end + + it { is_expected.to be_falsey } + end + end + describe "#human_max_access" do - it 'returns Maintainer role' do - user = create(:user) - group = create(:group) - project = create(:project, namespace: group) + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, namespace: group) } + it 'returns Maintainer role' do group.add_maintainer(user) expect(project.team.human_max_access(user.id)).to eq 'Maintainer' end it 'returns Owner role' do - user = create(:user) - group = create(:group) - project = create(:project, namespace: group) - group.add_owner(user) expect(project.team.human_max_access(user.id)).to eq 'Owner' diff --git a/spec/policies/organizations/organization_policy_spec.rb b/spec/policies/organizations/organization_policy_spec.rb index a1a2f1db305..7eed497d644 100644 --- a/spec/policies/organizations/organization_policy_spec.rb +++ b/spec/policies/organizations/organization_policy_spec.rb @@ -20,7 +20,6 @@ RSpec.describe Organizations::OrganizationPolicy, feature_category: :cell do context 'when admin mode is enabled', :enable_admin_mode do it { is_expected.to be_allowed(:admin_organization) } - it { is_expected.to be_allowed(:create_group) } it { is_expected.to be_allowed(:read_organization) } it { is_expected.to be_allowed(:read_organization_user) } end @@ -37,14 +36,12 @@ RSpec.describe Organizations::OrganizationPolicy, feature_category: :cell do end it { is_expected.to be_allowed(:admin_organization) } - it { is_expected.to be_allowed(:create_group) } it { is_expected.to be_allowed(:read_organization) } it { is_expected.to be_allowed(:read_organization_user) } end context 'when the user is not part of the organization' do it { is_expected.to be_disallowed(:admin_organization) } - it { is_expected.to be_disallowed(:create_group) } it { is_expected.to be_disallowed(:read_organization_user) } # All organizations are currently public, and hence they are allowed to be read # even if the user is not a part of the organization. diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index d1158cba16e..327dfd0a76b 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -1937,59 +1937,6 @@ RSpec.describe API::Groups, feature_category: :groups_and_projects do end end - context 'when group is within a provided organization' do - let_it_be(:organization) { create(:organization) } - - context 'when user is an organization user' do - before_all do - create(:organization_user, user: user3, organization: organization) - end - - it 'creates group within organization' do - post api('/groups', user3), params: attributes_for_group_api(organization_id: organization.id) - - expect(response).to have_gitlab_http_status(:created) - expect(json_response['organization_id']).to eq(organization.id) - end - - context 'when parent_group is not part of the organization' do - it 'does not create the group with not_found' do - post( - api('/groups', user3), - params: attributes_for_group_api(parent_id: group2.id, organization_id: organization.id) - ) - - expect(response).to have_gitlab_http_status(:not_found) - end - end - end - - context 'when organization does not exist' do - it 'does not create the group with not_found' do - post api('/groups', user3), params: attributes_for_group_api(organization_id: non_existing_record_id) - - expect(response).to have_gitlab_http_status(:not_found) - end - end - - context 'when user is not an organization user' do - it 'does not create the group' do - post api('/groups', user3), params: attributes_for_group_api(organization_id: organization.id) - - expect(response).to have_gitlab_http_status(:forbidden) - end - end - - context 'when user is an admin' do - it 'creates group within organization' do - post api('/groups', admin, admin_mode: true), params: attributes_for_group_api(organization_id: organization.id) - - expect(response).to have_gitlab_http_status(:created) - expect(json_response['organization_id']).to eq(organization.id) - end - end - end - context "when authenticated as user with group permissions" do it "creates group", :aggregate_failures do group = attributes_for_group_api request_access_enabled: false diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb index 56b1516096a..b2b27a1a075 100644 --- a/spec/services/groups/create_service_spec.rb +++ b/spec/services/groups/create_service_spec.rb @@ -6,7 +6,7 @@ RSpec.describe Groups::CreateService, '#execute', feature_category: :groups_and_ let!(:user) { create(:user) } let!(:group_params) { { path: "group_path", visibility_level: Gitlab::VisibilityLevel::PUBLIC } } - subject(:execute) { service.execute } + subject { service.execute } shared_examples 'has sync-ed traversal_ids' do specify { expect(subject.reload.traversal_ids).to eq([subject.parent&.traversal_ids, subject.id].flatten.compact) } @@ -119,37 +119,6 @@ RSpec.describe Groups::CreateService, '#execute', feature_category: :groups_and_ end end - describe 'creating a group within a provided organization' do - let_it_be(:organization) { create(:organization) } - - let(:current_user) { user } - let(:params) { group_params.merge(organization_id: organization.id) } - let(:service) { described_class.new(current_user, params) } - - context 'when user can create the group' do - before do - create(:organization_user, user: user, organization: organization) - end - - it { is_expected.to be_persisted } - end - - context 'when user is an admin', :enable_admin_mode do - let(:current_user) { create(:admin) } - - it { is_expected.to be_persisted } - end - - context 'when user can not create the group' do - it 'does not save group and returns an error' do - expect(execute).not_to be_persisted - expect(execute.errors[:organization_id].first) - .to eq(s_("CreateGroup|You don't have permission to create a group in the provided organization.")) - expect(execute.organization_id).to be_nil - end - end - end - describe 'creating subgroup' do let!(:group) { create(:group) } let!(:service) { described_class.new(user, group_params.merge(parent_id: group.id)) } diff --git a/spec/services/members/create_service_spec.rb b/spec/services/members/create_service_spec.rb index b977292bcf4..af151f93dc7 100644 --- a/spec/services/members/create_service_spec.rb +++ b/spec/services/members/create_service_spec.rb @@ -98,7 +98,7 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ it 'adds a user to members' do expect(execute_service[:status]).to eq(:success) - expect(source.users).to include member + expect(source).to have_user(member) expect(Onboarding::Progress.completed?(source, :user_added)).to be(true) end diff --git a/spec/services/resource_access_tokens/revoke_service_spec.rb b/spec/services/resource_access_tokens/revoke_service_spec.rb index 060697cd1df..aab22cb2815 100644 --- a/spec/services/resource_access_tokens/revoke_service_spec.rb +++ b/spec/services/resource_access_tokens/revoke_service_spec.rb @@ -26,7 +26,7 @@ RSpec.describe ResourceAccessTokens::RevokeService, feature_category: :system_ac it 'removes membership of bot user' do subject - expect(resource.reload.users).not_to include(resource_bot) + expect(resource.reload).not_to have_user(resource_bot) end it 'initiates user removal' do @@ -56,7 +56,7 @@ RSpec.describe ResourceAccessTokens::RevokeService, feature_category: :system_ac it 'does not remove bot from member list' do subject - expect(resource.reload.users).to include(resource_bot) + expect(resource.reload).to have_user(resource_bot) end it 'does not transfer issuables of bot user to ghost user' do diff --git a/spec/support/matchers/have_user.rb b/spec/support/matchers/have_user.rb new file mode 100644 index 00000000000..64fc84a75cf --- /dev/null +++ b/spec/support/matchers/have_user.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +RSpec::Matchers.define :have_user do |user| + match do |resource| + raise ArgumentError, 'Unknown resource type' unless resource.is_a?(Group) || resource.is_a?(Project) + + expect(resource.has_user?(user)).to be_truthy + end + + failure_message do |group| + "Expected #{group} to have the user #{user} among its members" + end +end diff --git a/spec/support/shared_examples/models/member_shared_examples.rb b/spec/support/shared_examples/models/member_shared_examples.rb index 731500c4510..6f00a5485a2 100644 --- a/spec/support/shared_examples/models/member_shared_examples.rb +++ b/spec/support/shared_examples/models/member_shared_examples.rb @@ -108,7 +108,7 @@ RSpec.shared_examples_for "member creation" do it 'does not update the member' do member = described_class.add_member(source, project_bot, :maintainer, current_user: user) - expect(source.users.reload).to include(project_bot) + expect(source.reload).to have_user(project_bot) expect(member).to be_persisted expect(member.access_level).to eq(Gitlab::Access::DEVELOPER) expect(member.errors.full_messages).to include(/not authorized to update member/) @@ -119,7 +119,7 @@ RSpec.shared_examples_for "member creation" do it 'adds the member' do member = described_class.add_member(source, project_bot, :maintainer, current_user: user) - expect(source.users.reload).to include(project_bot) + expect(source.reload).to have_user(project_bot) expect(member).to be_persisted end end @@ -130,7 +130,7 @@ RSpec.shared_examples_for "member creation" do member = described_class.add_member(source, user, :maintainer, current_user: admin) expect(member).to be_persisted - expect(source.users.reload).to include(user) + expect(source.reload).to have_user(user) expect(member.created_by).to eq(admin) end end @@ -140,7 +140,7 @@ RSpec.shared_examples_for "member creation" do member = described_class.add_member(source, user, :maintainer, current_user: admin) expect(member).not_to be_persisted - expect(source.users.reload).not_to include(user) + expect(source).not_to have_user(user) expect(member.errors.full_messages).to include(/not authorized to create member/) end end @@ -153,52 +153,52 @@ RSpec.shared_examples_for "member creation" do described_class.access_levels.each do |sym_key, int_access_level| it "accepts the :#{sym_key} symbol as access level", :aggregate_failures do - expect(source.users).not_to include(user) + expect(source).not_to have_user(user) member = described_class.add_member(source, user.id, sym_key) expect(member.access_level).to eq(int_access_level) - expect(source.users.reload).to include(user) + expect(source.reload).to have_user(user) end it "accepts the #{int_access_level} integer as access level", :aggregate_failures do - expect(source.users).not_to include(user) + expect(source).not_to have_user(user) member = described_class.add_member(source, user.id, int_access_level) expect(member.access_level).to eq(int_access_level) - expect(source.users.reload).to include(user) + expect(source.reload).to have_user(user) end end context 'with no current_user' do context 'when called with a known user id' do it 'adds the user as a member' do - expect(source.users).not_to include(user) + expect(source).not_to have_user(user) described_class.add_member(source, user.id, :maintainer) - expect(source.users.reload).to include(user) + expect(source.reload).to have_user(user) end end context 'when called with an unknown user id' do it 'does not add the user as a member' do - expect(source.users).not_to include(user) + expect(source).not_to have_user(user) described_class.add_member(source, non_existing_record_id, :maintainer) - expect(source.users.reload).not_to include(user) + expect(source.reload).not_to have_user(user) end end context 'when called with a user object' do it 'adds the user as a member' do - expect(source.users).not_to include(user) + expect(source).not_to have_user(user) described_class.add_member(source, user, :maintainer) - expect(source.users.reload).to include(user) + expect(source.reload).to have_user(user) end end @@ -208,29 +208,29 @@ RSpec.shared_examples_for "member creation" do end it 'adds the requester as a member', :aggregate_failures do - expect(source.users).not_to include(user) + expect(source.reload).not_to have_user(user) expect(source.requesters.exists?(user_id: user)).to eq(true) described_class.add_member(source, user, :maintainer) - expect(source.users.reload).to include(user) - expect(source.requesters.reload.exists?(user_id: user)).to eq(false) + expect(source.reload).to have_user(user) + expect(source.requesters.exists?(user_id: user)).to eq(false) end end context 'when called with a known user email' do it 'adds the user as a member' do - expect(source.users).not_to include(user) + expect(source).not_to have_user(user) described_class.add_member(source, user.email, :maintainer) - expect(source.users.reload).to include(user) + expect(source.reload).to have_user(user) end end context 'when called with an unknown user email' do it 'creates an invited member' do - expect(source.users).not_to include(user) + expect(source).not_to have_user(user) described_class.add_member(source, 'user@example.com', :maintainer) @@ -245,18 +245,18 @@ RSpec.shared_examples_for "member creation" do described_class.add_member(source, email_starting_with_number, :maintainer) expect(source.members.invite.pluck(:invite_email)).to include(email_starting_with_number) - expect(source.users.reload).not_to include(user) + expect(source.reload).not_to have_user(user) end end end context 'when current_user can update member', :enable_admin_mode do it 'creates the member' do - expect(source.users).not_to include(user) + expect(source).not_to have_user(user) described_class.add_member(source, user, :maintainer, current_user: admin) - expect(source.users.reload).to include(user) + expect(source.reload).to have_user(user) end context 'when called with a requester user object' do @@ -265,12 +265,12 @@ RSpec.shared_examples_for "member creation" do end it 'adds the requester as a member', :aggregate_failures do - expect(source.users).not_to include(user) + expect(source).not_to have_user(user) expect(source.requesters.exists?(user_id: user)).to be_truthy described_class.add_member(source, user, :maintainer, current_user: admin) - expect(source.users.reload).to include(user) + expect(source.reload).to have_user(user) expect(source.requesters.reload.exists?(user_id: user)).to be_falsy end end @@ -278,11 +278,11 @@ RSpec.shared_examples_for "member creation" do context 'when current_user cannot update member' do it 'does not create the member', :aggregate_failures do - expect(source.users).not_to include(user) + expect(source).not_to have_user(user) member = described_class.add_member(source, user, :maintainer, current_user: user) - expect(source.users.reload).not_to include(user) + expect(source.reload).not_to have_user(user) expect(member).not_to be_persisted end @@ -292,12 +292,12 @@ RSpec.shared_examples_for "member creation" do end it 'does not destroy the requester', :aggregate_failures do - expect(source.users).not_to include(user) + expect(source).not_to have_user(user) expect(source.requesters.exists?(user_id: user)).to be_truthy described_class.add_member(source, user, :maintainer, current_user: user) - expect(source.users.reload).not_to include(user) + expect(source.reload).not_to have_user(user) expect(source.requesters.exists?(user_id: user)).to be_truthy end end @@ -311,7 +311,7 @@ RSpec.shared_examples_for "member creation" do context 'with no current_user' do it 'updates the member' do - expect(source.users).to include(user) + expect(source).to have_user(user) described_class.add_member(source, user, :maintainer) @@ -321,7 +321,7 @@ RSpec.shared_examples_for "member creation" do context 'when current_user can update member', :enable_admin_mode do it 'updates the member' do - expect(source.users).to include(user) + expect(source).to have_user(user) described_class.add_member(source, user, :maintainer, current_user: admin) @@ -331,7 +331,7 @@ RSpec.shared_examples_for "member creation" do context 'when current_user cannot update member' do it 'does not update the member' do - expect(source.users).to include(user) + expect(source).to have_user(user) described_class.add_member(source, user, :maintainer, current_user: user) |