From 4bff1dd00b057807592edde32263bdfb028e3f4f Mon Sep 17 00:00:00 2001 From: Matthew MacRae-Bovell Date: Wed, 17 Sep 2025 12:01:11 -0400 Subject: [PATCH 1/4] Remove usage of can? from base_policy --- app/policies/base_policy.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/policies/base_policy.rb b/app/policies/base_policy.rb index d8803f6712cf3d..966d466ba1a6c3 100644 --- a/app/policies/base_policy.rb +++ b/app/policies/base_policy.rb @@ -111,9 +111,8 @@ class BasePolicy < DeclarativePolicy::Base rule { ~in_current_organization }.prevent_all - rule { external_authorization_enabled & ~can?(:read_all_resources) }.policy do - prevent :read_cross_project - end + rule { external_authorization_enabled }.prevent :read_cross_project + rule { external_authorization_enabled & admin }.enable :read_cross_project rule { admin }.policy do # Only for actual administrator accounts, behavior affected by admin mode application setting -- GitLab From 807d9200f2c982e98a63779f71060c0601ee39b3 Mon Sep 17 00:00:00 2001 From: Matthew MacRae-Bovell Date: Wed, 17 Sep 2025 12:32:38 -0400 Subject: [PATCH 2/4] Update to include auditor --- app/policies/base_policy.rb | 10 +++++++++- ee/app/policies/ee/base_policy.rb | 7 ++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/app/policies/base_policy.rb b/app/policies/base_policy.rb index 966d466ba1a6c3..71fc347a59f84f 100644 --- a/app/policies/base_policy.rb +++ b/app/policies/base_policy.rb @@ -111,8 +111,16 @@ class BasePolicy < DeclarativePolicy::Base rule { ~in_current_organization }.prevent_all + # In CE a "trusted reader" is an admin. + # EE will extend this (e.g., admin | auditor). + desc "Trusted reader allowed to read across projects when external auth is enabled" + with_options scope: :user, score: 0 + condition(:trusted_reader) { admin } + + # When external auth is enabled, default to preventing cross-project read. rule { external_authorization_enabled }.prevent :read_cross_project - rule { external_authorization_enabled & admin }.enable :read_cross_project + # Then allow it back for trusted readers. + rule { external_authorization_enabled & trusted_reader }.enable :read_cross_project rule { admin }.policy do # Only for actual administrator accounts, behavior affected by admin mode application setting diff --git a/ee/app/policies/ee/base_policy.rb b/ee/app/policies/ee/base_policy.rb index 05c2b1a6dbe087..9a2bd0dd9fedcd 100644 --- a/ee/app/policies/ee/base_policy.rb +++ b/ee/app/policies/ee/base_policy.rb @@ -29,6 +29,11 @@ module BasePolicy rule { auditor }.enable :read_all_resources + # EE widens the CE 'trusted_reader' condition to include auditors too. + # This is used by BasePolicy to re-enable cross-project reads under external auth. + with_options scope: :user, score: 0 + condition(:trusted_reader) { admin | auditor } + with_scope :global condition(:allow_to_manage_default_branch_protection) do # When un-licensed: Always allow access. @@ -39,7 +44,7 @@ module BasePolicy end # token_info is set when authenticating user with a token. ai_workflows scope is used only by requests sent by Duo - # Workflow.This is a temporary solution until https://gitlab.com/gitlab-org/gitlab/-/issues/468370 is done. + # Workflow. This is a temporary solution until https://gitlab.com/gitlab-org/gitlab/-/issues/468370 is done. with_scope :user condition(:duo_workflow_token, score: 0) do ::Current.token_info.present? && Array.wrap(Current.token_info[:token_scopes]).include?(:ai_workflows) -- GitLab From cb77d909a954e36275c95d3136e7fc345592d629 Mon Sep 17 00:00:00 2001 From: Matthew MacRae-Bovell Date: Wed, 17 Sep 2025 12:45:35 -0400 Subject: [PATCH 3/4] Update trusted_cross_project_reader naming --- app/policies/base_policy.rb | 6 +++--- ee/app/policies/ee/base_policy.rb | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/policies/base_policy.rb b/app/policies/base_policy.rb index 71fc347a59f84f..3f0f80edda1f3c 100644 --- a/app/policies/base_policy.rb +++ b/app/policies/base_policy.rb @@ -111,11 +111,11 @@ class BasePolicy < DeclarativePolicy::Base rule { ~in_current_organization }.prevent_all - # In CE a "trusted reader" is an admin. + # In CE a "trusted_cross_project_reader" is just an admin. # EE will extend this (e.g., admin | auditor). - desc "Trusted reader allowed to read across projects when external auth is enabled" + desc "Trusted cross-project reader allowed to read across projects when external auth is enabled" with_options scope: :user, score: 0 - condition(:trusted_reader) { admin } + condition(:trusted_cross_project_reader) { admin } # When external auth is enabled, default to preventing cross-project read. rule { external_authorization_enabled }.prevent :read_cross_project diff --git a/ee/app/policies/ee/base_policy.rb b/ee/app/policies/ee/base_policy.rb index 9a2bd0dd9fedcd..a3841313349bf2 100644 --- a/ee/app/policies/ee/base_policy.rb +++ b/ee/app/policies/ee/base_policy.rb @@ -29,10 +29,10 @@ module BasePolicy rule { auditor }.enable :read_all_resources - # EE widens the CE 'trusted_reader' condition to include auditors too. + # EE widens the CE 'trusted_cross_project_reader' condition to include auditors too. # This is used by BasePolicy to re-enable cross-project reads under external auth. with_options scope: :user, score: 0 - condition(:trusted_reader) { admin | auditor } + condition(:trusted_cross_project_reader) { admin | auditor } with_scope :global condition(:allow_to_manage_default_branch_protection) do -- GitLab From 7904746dc9bc561a2c6167f7eb2e75502a889ea6 Mon Sep 17 00:00:00 2001 From: Matthew MacRae-Bovell Date: Thu, 18 Sep 2025 14:54:25 -0400 Subject: [PATCH 4/4] Add updated policys that pass all specs --- app/policies/base_policy.rb | 39 +++++++++++++++++-------------- ee/app/policies/ee/base_policy.rb | 23 +++++++++++------- 2 files changed, 35 insertions(+), 27 deletions(-) diff --git a/app/policies/base_policy.rb b/app/policies/base_policy.rb index 3f0f80edda1f3c..a45b5100638d01 100644 --- a/app/policies/base_policy.rb +++ b/app/policies/base_policy.rb @@ -25,14 +25,7 @@ class BasePolicy < DeclarativePolicy::Base desc "User is an instance admin" with_options scope: :user, score: 0 condition(:admin) do - next false if @user&.from_ci_job_token? - next true if user_is_user? && @user.admin_bot? - - if Gitlab::CurrentSettings.admin_mode - @user&.admin? && Gitlab::Auth::CurrentUserMode.new(@user).admin_mode? - else - @user&.admin? - end + user_is_admin? end desc "The current instance is a GitLab Dedicated instance" @@ -109,18 +102,13 @@ class BasePolicy < DeclarativePolicy::Base ::Gitlab::ExternalAuthorization.perform_check? end - rule { ~in_current_organization }.prevent_all + condition(:user_can_read_cross_project, scope: :user, score: 0) do + user_can_read_cross_project? + end - # In CE a "trusted_cross_project_reader" is just an admin. - # EE will extend this (e.g., admin | auditor). - desc "Trusted cross-project reader allowed to read across projects when external auth is enabled" - with_options scope: :user, score: 0 - condition(:trusted_cross_project_reader) { admin } + rule { ~in_current_organization }.prevent_all - # When external auth is enabled, default to preventing cross-project read. - rule { external_authorization_enabled }.prevent :read_cross_project - # Then allow it back for trusted readers. - rule { external_authorization_enabled & trusted_reader }.enable :read_cross_project + rule { external_authorization_enabled & ~user_can_read_cross_project }.prevent :read_cross_project rule { admin }.policy do # Only for actual administrator accounts, behavior affected by admin mode application setting @@ -147,6 +135,17 @@ def user_is_user? user.is_a?(User) end + def user_is_admin? + return false if @user&.from_ci_job_token? + return true if user_is_user? && @user.admin_bot? + + if Gitlab::CurrentSettings.admin_mode + @user&.admin? && Gitlab::Auth::CurrentUserMode.new(@user).admin_mode? + else + @user&.admin? + end + end + def owns_organization?(org) return false unless org.present? return false unless user_is_user? @@ -165,6 +164,10 @@ def admin_mode_required? !Gitlab::Auth::CurrentUserMode.new(@user).admin_mode? end + + def user_can_read_cross_project? + user_is_admin? + end end BasePolicy.prepend_mod_with('BasePolicy') diff --git a/ee/app/policies/ee/base_policy.rb b/ee/app/policies/ee/base_policy.rb index a3841313349bf2..9a4412b05a6926 100644 --- a/ee/app/policies/ee/base_policy.rb +++ b/ee/app/policies/ee/base_policy.rb @@ -7,10 +7,7 @@ module BasePolicy prepended do with_scope :user condition(:auditor, score: 0) do - # We pass in Gitlab::Auth::GroupSaml::TokenActor to policies via - # Groups::SsoController#check_user_can_sign_in_with_provider. - # However, only User can be an auditor. - @user.respond_to?(:auditor?) && @user.auditor? + user_is_auditor? end with_scope :user @@ -29,11 +26,6 @@ module BasePolicy rule { auditor }.enable :read_all_resources - # EE widens the CE 'trusted_cross_project_reader' condition to include auditors too. - # This is used by BasePolicy to re-enable cross-project reads under external auth. - with_options scope: :user, score: 0 - condition(:trusted_cross_project_reader) { admin | auditor } - with_scope :global condition(:allow_to_manage_default_branch_protection) do # When un-licensed: Always allow access. @@ -49,6 +41,19 @@ module BasePolicy condition(:duo_workflow_token, score: 0) do ::Current.token_info.present? && Array.wrap(Current.token_info[:token_scopes]).include?(:ai_workflows) end + + private + + def user_is_auditor? + # We pass in Gitlab::Auth::GroupSaml::TokenActor to policies via + # Groups::SsoController#check_user_can_sign_in_with_provider. + # However, only User can be an auditor. + @user.respond_to?(:auditor?) && @user.auditor? + end + + def user_can_read_cross_project? + user_is_auditor? | user_is_admin? + end end end end -- GitLab