Add latest changes from gitlab-org/security/gitlab@16-6-stable-ee

9 files changed, 58 insertions, 16 deletions

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/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/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,