Merge remote-tracking branch 'dev/16-6-stable' into 16-6-stable

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,