diff options
author | GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com> | 2023-11-30 20:38:55 +0300 |
---|---|---|
committer | GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com> | 2023-11-30 20:38:55 +0300 |
commit | 45dda602ee61587a5366f0b555375eaa8cd5b94e (patch) | |
tree | f3155bbec7a2fec8c4230286f962e81e16071f69 /app | |
parent | 693d15dcb2f33c01a442784c13933da3d1b8d52e (diff) | |
parent | 21d8cc00343440dc18682ace76094a8958f701fa (diff) |
Merge remote-tracking branch 'dev/16-6-stable' into 16-6-stable
Diffstat (limited to 'app')
-rw-r--r-- | app/controllers/projects/tags_controller.rb | 2 | ||||
-rw-r--r-- | app/finders/packages/composer/packages_finder.rb | 8 | ||||
-rw-r--r-- | app/finders/packages/group_packages_finder.rb | 19 | ||||
-rw-r--r-- | app/graphql/types/permission_types/base_permission_type.rb | 2 | ||||
-rw-r--r-- | app/helpers/groups_helper.rb | 11 | ||||
-rw-r--r-- | app/models/ability.rb | 14 | ||||
-rw-r--r-- | app/models/concerns/protected_branch_access.rb | 2 | ||||
-rw-r--r-- | app/models/integrations/jira.rb | 4 | ||||
-rw-r--r-- | app/models/pages_domain.rb | 5 | ||||
-rw-r--r-- | app/models/user.rb | 6 | ||||
-rw-r--r-- | app/policies/ci/pipeline_schedule_policy.rb | 3 | ||||
-rw-r--r-- | app/policies/issue_policy.rb | 1 | ||||
-rw-r--r-- | app/services/ci/pipeline_schedules/base_save_service.rb | 6 | ||||
-rw-r--r-- | app/services/ci/pipeline_schedules/update_service.rb | 6 | ||||
-rw-r--r-- | app/services/members/creator_service.rb | 24 | ||||
-rw-r--r-- | app/views/groups/_invite_members_modal.html.haml | 2 |
16 files changed, 81 insertions, 34 deletions
diff --git a/app/controllers/projects/tags_controller.rb b/app/controllers/projects/tags_controller.rb index 3c1735c728c..d3e38774aaa 100644 --- a/app/controllers/projects/tags_controller.rb +++ b/app/controllers/projects/tags_controller.rb @@ -29,7 +29,7 @@ class Projects::TagsController < Projects::ApplicationController tag_names = @tags.map(&:name) @tags_pipelines = @project.ci_pipelines.latest_successful_for_refs(tag_names) - @releases = project.releases.where(tag: tag_names) + @releases = ReleasesFinder.new(project, current_user, tag: tag_names).execute @tag_pipeline_statuses = Ci::CommitStatusesFinder.new(@project, @repository, current_user, @tags).execute rescue Gitlab::Git::CommandError => e diff --git a/app/finders/packages/composer/packages_finder.rb b/app/finders/packages/composer/packages_finder.rb index b5a1b19216f..1581c48dd74 100644 --- a/app/finders/packages/composer/packages_finder.rb +++ b/app/finders/packages/composer/packages_finder.rb @@ -2,14 +2,12 @@ module Packages module Composer class PackagesFinder < Packages::GroupPackagesFinder - def initialize(current_user, group, params = {}) - @current_user = current_user - @group = group - @params = params + def initialize(current_user, group, params = { package_type: :composer, with_package_registry_enabled: true }) + super(current_user, group, params) end def execute - packages_for_group_projects(installable_only: true).composer.preload_composer + packages_for_group_projects(installable_only: true).preload_composer end end end diff --git a/app/finders/packages/group_packages_finder.rb b/app/finders/packages/group_packages_finder.rb index 3a068252d5c..3b211882fa0 100644 --- a/app/finders/packages/group_packages_finder.rb +++ b/app/finders/packages/group_packages_finder.rb @@ -40,14 +40,17 @@ module Packages # access to packages is ruled by: # - project is public or the current user has access to it with at least the reporter level # - the repository feature is available to the current_user - if current_user.is_a?(DeployToken) - current_user.accessible_projects - else - ::Project - .in_namespace(groups) - .public_or_visible_to_user(current_user, Gitlab::Access::REPORTER) - .with_feature_available_for_user(:repository, current_user) - end + projects = if current_user.is_a?(DeployToken) + current_user.accessible_projects + else + ::Project + .in_namespace(groups) + .public_or_visible_to_user(current_user, Gitlab::Access::REPORTER) + .with_feature_available_for_user(:repository, current_user) + end + + projects = projects.with_package_registry_enabled if params[:with_package_registry_enabled] + projects end def groups diff --git a/app/graphql/types/permission_types/base_permission_type.rb b/app/graphql/types/permission_types/base_permission_type.rb index 3c0e68bdaf2..3a8519e8196 100644 --- a/app/graphql/types/permission_types/base_permission_type.rb +++ b/app/graphql/types/permission_types/base_permission_type.rb @@ -30,7 +30,7 @@ module Types def self.define_field_resolver_method(ability) unless respond_to?(ability) define_method ability.to_sym do |*args| - Ability.allowed?(context[:current_user], ability, object, args.to_h) + Ability.allowed?(context[:current_user], ability, object, **args.to_h) end end end diff --git a/app/helpers/groups_helper.rb b/app/helpers/groups_helper.rb index f48157cb65a..6cabdf21483 100644 --- a/app/helpers/groups_helper.rb +++ b/app/helpers/groups_helper.rb @@ -207,6 +207,17 @@ module GroupsHelper new_group_custom_emoji_path(group) end + def access_level_roles_user_can_assign(group) + return {} unless current_user + return group.access_level_roles if current_user.can_admin_all_resources? + + max_access_level = group.highest_group_member(current_user)&.access_level + + return {} unless max_access_level + + GroupMember.access_level_roles.select { |_k, v| v <= max_access_level } + end + private def group_title_link(group, hidable: false, show_avatar: false, for_dropdown: false) diff --git a/app/models/ability.rb b/app/models/ability.rb index b8433191d84..9ae96c35d4f 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -70,13 +70,13 @@ class Ability end end - def allowed?(user, ability, subject = :global, opts = {}) + def allowed?(user, ability, subject = :global, **opts) if subject.is_a?(Hash) opts = subject subject = :global end - policy = policy_for(user, subject) + policy = policy_for(user, subject, **opts.slice(:cache)) before_check(policy, ability.to_sym, user, subject, opts) @@ -100,8 +100,14 @@ class Ability # See Support::AbilityCheck and Support::PermissionsCheck. end - def policy_for(user, subject = :global) - DeclarativePolicy.policy_for(user, subject, cache: ::Gitlab::SafeRequestStore.storage) + # We cache in the request store by default. This can lead to unexpected + # results if abilities are re-checked after objects are modified and the + # check depends on the modified attributes. In such cases, you should pass + # `cache: false` for the second check to ensure all rules get re-evaluated. + def policy_for(user, subject = :global, cache: true) + policy_cache = cache ? ::Gitlab::SafeRequestStore.storage : {} + + DeclarativePolicy.policy_for(user, subject, cache: policy_cache) end # This method is something of a band-aid over the problem. The problem is diff --git a/app/models/concerns/protected_branch_access.rb b/app/models/concerns/protected_branch_access.rb index 8156090fd9c..6a7fdce62fb 100644 --- a/app/models/concerns/protected_branch_access.rb +++ b/app/models/concerns/protected_branch_access.rb @@ -10,3 +10,5 @@ module ProtectedBranchAccess delegate :project, to: :protected_branch end end + +ProtectedBranchAccess.prepend_mod_with('ProtectedBranchAccess') diff --git a/app/models/integrations/jira.rb b/app/models/integrations/jira.rb index 22367ee336d..bf49dbca294 100644 --- a/app/models/integrations/jira.rb +++ b/app/models/integrations/jira.rb @@ -401,9 +401,9 @@ module Integrations private def jira_issue_match_regex - return /\b#{jira_issue_prefix}(?<issue>#{Gitlab::Regex.jira_issue_key_regex})/ if jira_issue_regex.blank? + jira_regex = jira_issue_regex.presence || Gitlab::Regex.jira_issue_key_regex.source - Gitlab::UntrustedRegexp.new("\\b#{jira_issue_prefix}(?P<issue>#{jira_issue_regex})") + Gitlab::UntrustedRegexp.new("\\b#{jira_issue_prefix}(?P<issue>#{jira_regex})") end def parse_project_from_issue_key(issue_key) diff --git a/app/models/pages_domain.rb b/app/models/pages_domain.rb index cabd3924fd6..33de5aa21aa 100644 --- a/app/models/pages_domain.rb +++ b/app/models/pages_domain.rb @@ -35,10 +35,11 @@ class PagesDomain < ApplicationRecord validates :verification_code, presence: true, allow_blank: false validate :validate_pages_domain + validate :max_certificate_key_length, if: ->(domain) { domain.key.present? } validate :validate_matching_key, if: ->(domain) { domain.certificate.present? || domain.key.present? } - validate :validate_intermediates, if: ->(domain) { domain.certificate.present? && domain.certificate_changed? } + # validate_intermediates must run after key validations to skip expensive SSL validation when there is a key error + validate :validate_intermediates, if: ->(domain) { domain.certificate.present? && domain.certificate_changed? && errors[:key].blank? } validate :validate_custom_domain_count_per_project, on: :create - validate :max_certificate_key_length, if: ->(domain) { domain.key.present? } attribute :auto_ssl_enabled, default: -> { ::Gitlab::LetsEncrypt.enabled? } attribute :wildcard, default: false diff --git a/app/models/user.rb b/app/models/user.rb index 25f22563136..4a66192e9d8 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1307,9 +1307,11 @@ class User < MainClusterwide::ApplicationRecord several_namespaces? || admin end - def can?(action, subject = :global) - Ability.allowed?(self, action, subject) + # rubocop: disable Style/ArgumentsForwarding + def can?(action, subject = :global, **opts) + Ability.allowed?(self, action, subject, **opts) end + # rubocop: enable Style/ArgumentsForwarding def confirm_deletion_with_password? !password_automatically_set? && allow_password_authentication? diff --git a/app/policies/ci/pipeline_schedule_policy.rb b/app/policies/ci/pipeline_schedule_policy.rb index cbc60c4a30a..9e558cd91c1 100644 --- a/app/policies/ci/pipeline_schedule_policy.rb +++ b/app/policies/ci/pipeline_schedule_policy.rb @@ -25,7 +25,7 @@ module Ci rule { can?(:create_pipeline) }.enable :play_pipeline_schedule - rule { can?(:admin_pipeline) | (can?(:update_build) & owner_of_schedule) }.policy do + rule { can?(:admin_pipeline) | (owner_of_schedule & can?(:update_build)) }.policy do enable :admin_pipeline_schedule enable :read_pipeline_schedule_variables end @@ -45,6 +45,7 @@ module Ci rule { protected_ref }.policy do prevent :play_pipeline_schedule prevent :create_pipeline_schedule + prevent :update_pipeline_schedule end private diff --git a/app/policies/issue_policy.rb b/app/policies/issue_policy.rb index 683c53d8d78..c95cde86e38 100644 --- a/app/policies/issue_policy.rb +++ b/app/policies/issue_policy.rb @@ -60,6 +60,7 @@ class IssuePolicy < IssuablePolicy rule { ~can?(:read_issue) }.policy do prevent :create_note prevent :read_note + prevent :award_emoji end rule { locked }.policy do diff --git a/app/services/ci/pipeline_schedules/base_save_service.rb b/app/services/ci/pipeline_schedules/base_save_service.rb index 45d70e5a65d..e6f633498e9 100644 --- a/app/services/ci/pipeline_schedules/base_save_service.rb +++ b/app/services/ci/pipeline_schedules/base_save_service.rb @@ -23,7 +23,11 @@ module Ci attr_reader :project, :user, :params, :schedule def allowed_to_save? - user.can?(self.class::AUTHORIZE, schedule) + # Disable cache because the same ability may already have been checked + # for the same records with different attributes. For example, we do not + # want an unauthorized user to change an unprotected ref to a protected + # ref. + user.can?(self.class::AUTHORIZE, schedule, cache: false) end def forbidden_to_save diff --git a/app/services/ci/pipeline_schedules/update_service.rb b/app/services/ci/pipeline_schedules/update_service.rb index 2fd1173ecce..76b2121c4e1 100644 --- a/app/services/ci/pipeline_schedules/update_service.rb +++ b/app/services/ci/pipeline_schedules/update_service.rb @@ -12,6 +12,12 @@ module Ci @params = params end + def execute + return forbidden_to_save unless allowed_to_save? + + super + end + private def authorize_message diff --git a/app/services/members/creator_service.rb b/app/services/members/creator_service.rb index 22d8b30db18..d7bf073d8e9 100644 --- a/app/services/members/creator_service.rb +++ b/app/services/members/creator_service.rb @@ -156,12 +156,13 @@ module Members end def commit_member - if can_commit_member? - assign_member_attributes - commit_changes - else - add_commit_error - end + return add_commit_error unless can_commit_member? + + assign_member_attributes + + return add_member_role_error if member_role_too_high? + + commit_changes end def can_commit_member? @@ -175,6 +176,11 @@ module Members end end + # overridden in Members::Groups::CreatorService + def member_role_too_high? + false + end + def can_create_new_member? raise NotImplementedError end @@ -240,6 +246,12 @@ module Members member.errors.add(:base, msg) end + def add_member_role_error + msg = _("the member access level can't be higher than the current user's one") + + member.errors.add(:base, msg) + end + def find_or_build_member @member = builder.new(source, invitee, existing_members).execute end diff --git a/app/views/groups/_invite_members_modal.html.haml b/app/views/groups/_invite_members_modal.html.haml index d53190948fd..c60e7a78b1d 100644 --- a/app/views/groups/_invite_members_modal.html.haml +++ b/app/views/groups/_invite_members_modal.html.haml @@ -1,7 +1,7 @@ - return unless can_admin_group_member?(group) .js-invite-members-modal{ data: { is_project: 'false', - access_levels: group.access_level_roles.to_json, + access_levels: access_level_roles_user_can_assign(group).to_json, reload_page_on_submit: current_path?('group_members#index').to_s, help_link: help_page_url('user/permissions'), is_signup_enabled: signup_enabled?.to_s, |