From bc77a6adefcd02a058e3d5a10718d74a5dad954e Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 29 Nov 2023 16:29:06 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@16-6-stable-ee --- .../types/permission_types/base_permission_type.rb | 2 +- app/helpers/groups_helper.rb | 11 ++++++++++ app/models/ability.rb | 14 +++++++++---- app/models/user.rb | 6 ++++-- app/policies/ci/pipeline_schedule_policy.rb | 3 ++- .../ci/pipeline_schedules/base_save_service.rb | 6 +++++- .../ci/pipeline_schedules/update_service.rb | 6 ++++++ app/services/members/creator_service.rb | 24 ++++++++++++++++------ app/views/groups/_invite_members_modal.html.haml | 2 +- 9 files changed, 58 insertions(+), 16 deletions(-) (limited to 'app') 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, -- cgit v1.2.3