From a530647f270418b2bf049f214da7294a6ad46e9b Mon Sep 17 00:00:00 2001 From: Matthew MacRae-Bovell Date: Thu, 4 Sep 2025 16:17:50 -0400 Subject: [PATCH 1/5] Add custom cop to prevent permissions enabling permissions --- .rubocop.yml | 6 ++ .../no_permission_based_enables.rb | 83 +++++++++++++++++++ .../no_permission_based_enables_spec.rb | 54 ++++++++++++ 3 files changed, 143 insertions(+) create mode 100644 rubocop/cop/gitlab/declarative_policy/no_permission_based_enables.rb create mode 100644 spec/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index 8647c7ffab4de5..c3c6cd887b6c85 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1434,3 +1434,9 @@ Gitlab/NoHelpersInPresenters: Enabled: true Include: - '{,ee/,jh/}app/presenters/**/*.rb' + +Gitlab/DeclarativePolicy/NoPermissionBasedEnables: + Enabled: true + Include: + - 'app/policies/**/*.rb' + - 'ee/app/policies/**/*.rb' diff --git a/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables.rb b/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables.rb new file mode 100644 index 00000000000000..f97d751321e8eb --- /dev/null +++ b/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require 'rubocop' # rubocop-rspec is only needed in specs + +module RuboCop + module Cop + module Gitlab + module DeclarativePolicy + # Prevent enabling a permission based on another permission. + # + # Granting permission A because "user can? :B" creates hidden coupling and + # can introduce circular or non-obvious authorization paths. Rules should + # enable permissions based on state/feature predicates, not other permissions. + # + # @example + # # bad + # rule { dependency_scanning_enabled & can?(:download_code) }.enable :read_dependency + # + # @example + # # good + # rule { dependency_scanning_enabled }.enable :read_dependency + # + # @example + # # good + # rule { dependency_scanning_enabled & project.public? }.enable :read_dependency + class NoPermissionBasedEnables < ::RuboCop::Cop::Base + extend AutoCorrector + + MSG = "Avoid enabling permissions based on other permissions. " \ + "Do not call `can?(:...)` inside a `rule { ... }` that `.enable`s a permission." + + # Match: (send (block (send nil? :rule) (args) ) :enable ) + def on_send(node) + return unless node.method?(:enable) + + receiver = node.receiver + return unless receiver&.block_type? + + send_inside_rule = receiver.send_node + return unless send_inside_rule&.method?(:rule) + + condition = receiver.body + return unless condition + + # Find the first `can?` call within the condition + offending = find_permission_query(condition) + return unless offending + + # Highlight the `can?` selector itself to keep caret placement stable + add_offense(offending.loc.selector) + end + + # Support safe navigation (&.) as well (required by InternalAffairs/OnSendWithoutOnCSend) + def on_csend(node) + on_send(node) + end + + private + + # Depth-first search for a `can?` send node + def find_permission_query(node) + return node if can_query_call?(node) + + node.children.each do |child| + next unless child.is_a?(Parser::AST::Node) + + found = find_permission_query(child) + return found if found + end + nil + end + + def can_query_call?(node) + node.send_type? && + node.receiver.nil? && + node.method?(:can?) && + node.arguments.any? + end + end + end + end + end +end diff --git a/spec/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables_spec.rb b/spec/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables_spec.rb new file mode 100644 index 00000000000000..d8b46417242ef5 --- /dev/null +++ b/spec/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables_spec.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +require 'rubocop' +require 'rubocop/rspec/support' +require 'rubocop/rspec/expect_offense' +require_relative '../../../../../rubocop/cop/gitlab/declarative_policy/no_permission_based_enables' + +RSpec.describe RuboCop::Cop::Gitlab::DeclarativePolicy::NoPermissionBasedEnables, + :rubocop, :config, + feature_category: :tooling do + include RuboCop::RSpec::ExpectOffense + + it 'registers an offense when a rule enabling a permission depends on can?(:download_code)' do + expect_offense(<<~RUBY) + rule { dependency_scanning_enabled & can?(:download_code) }.enable :read_dependency + ^^^^ Avoid enabling permissions based on other permissions. Do not call `can?(:...)` inside a `rule { ... }` that `.enable`s a permission. + RUBY + end + + it 'registers an offense for a simple can? condition' do + expect_offense(<<~RUBY) + rule { can?(:update_issue) }.enable :admin_issue + ^^^^ Avoid enabling permissions based on other permissions. Do not call `can?(:...)` inside a `rule { ... }` that `.enable`s a permission. + RUBY + end + + it 'does not register an offense for non-permission predicates' do + expect_no_offenses(<<~RUBY) + rule { dependency_scanning_enabled & project.public? }.enable :read_dependency + RUBY + end + + it 'does not register an offense for rules that do not call enable' do + expect_no_offenses(<<~RUBY) + rule { can?(:download_code) } + RUBY + end + + it 'handles nested boolean expressions' do + source = <<~RUBY + rule { feature_enabled?(:x) & (project.public? | can?(:read_issue)) }.enable :read_something + RUBY + + offenses = inspect_source(source) + expect(offenses.size).to eq(1) + + offense = offenses.first + expect(offense.message).to eq( + "Avoid enabling permissions based on other permissions. " \ + "Do not call `can?(:...)` inside a `rule { ... }` that `.enable`s a permission." + ) + expect(offense.location.source).to eq('can?') + end +end -- GitLab From 9c48ff741186c35a20156ce7c821abea4bccea9f Mon Sep 17 00:00:00 2001 From: Matthew MacRae-Bovell Date: Thu, 4 Sep 2025 16:26:24 -0400 Subject: [PATCH 2/5] Remove dead comment --- .../gitlab/declarative_policy/no_permission_based_enables.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables.rb b/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables.rb index f97d751321e8eb..c7becd7303e46c 100644 --- a/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables.rb +++ b/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -require 'rubocop' # rubocop-rspec is only needed in specs +require 'rubocop' module RuboCop module Cop -- GitLab From f3678be4f6faa3fe2b1608631e0ab4a86d4033a0 Mon Sep 17 00:00:00 2001 From: Matthew MacRae-Bovell Date: Tue, 9 Sep 2025 09:44:44 -0400 Subject: [PATCH 3/5] Add updated cop that identifies "can?" --- .../no_permission_based_enables.rb | 83 ++++++++----------- .../no_permission_based_enables_spec.rb | 76 +++++++++++------ 2 files changed, 85 insertions(+), 74 deletions(-) diff --git a/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables.rb b/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables.rb index c7becd7303e46c..fb8e23764754b5 100644 --- a/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables.rb +++ b/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables.rb @@ -6,75 +6,60 @@ module RuboCop module Cop module Gitlab module DeclarativePolicy - # Prevent enabling a permission based on another permission. + # Forbid calling `can?` anywhere in policy files. # - # Granting permission A because "user can? :B" creates hidden coupling and - # can introduce circular or non-obvious authorization paths. Rules should - # enable permissions based on state/feature predicates, not other permissions. + # Why: Using `can?` inside policies creates hidden coupling and + # circular/indirect authorization paths. Encode logic as rules/conditions + # over state/feature predicates instead. # - # @example - # # bad - # rule { dependency_scanning_enabled & can?(:download_code) }.enable :read_dependency + # This cop only runs for files that look like policy files: + # - app/policies/**/*_policy.rb + # - ee/app/policies/**/*_policy.rb # # @example - # # good - # rule { dependency_scanning_enabled }.enable :read_dependency + # # bad - using can? directly in a rule + # rule { developer_access & can?(:download_code) }.enable :read_dependency # - # @example - # # good - # rule { dependency_scanning_enabled & project.public? }.enable :read_dependency + # # bad - hiding can? in a condition or helper + # condition(:can_do_thing) { can?(:do_thing) } + # + # def can_do_thing? + # can?(:do_thing) + # end + # + # # good - base enables on state/feature predicates only + # rule { developer_access & dependency_scanning_enabled }.enable :read_dependency + # + # # good - split positive enable and negative prevent without can? + # rule { developer_access }.policy do + # enable :do_thing + # enable :do_other_thing + # end + # rule { prevent_condition }.prevent :do_other_thing class NoPermissionBasedEnables < ::RuboCop::Cop::Base extend AutoCorrector - MSG = "Avoid enabling permissions based on other permissions. " \ - "Do not call `can?(:...)` inside a `rule { ... }` that `.enable`s a permission." + MSG = 'Do not call `can?` inside policy files. ' \ + 'Use rule/condition predicates based on state/features instead.' + RESTRICT_ON_SEND = %i[can?].freeze - # Match: (send (block (send nil? :rule) (args) ) :enable ) def on_send(node) - return unless node.method?(:enable) - - receiver = node.receiver - return unless receiver&.block_type? + return unless policy_file?(processed_source.file_path) - send_inside_rule = receiver.send_node - return unless send_inside_rule&.method?(:rule) - - condition = receiver.body - return unless condition - - # Find the first `can?` call within the condition - offending = find_permission_query(condition) - return unless offending - - # Highlight the `can?` selector itself to keep caret placement stable - add_offense(offending.loc.selector) + add_offense(node.loc.selector) end - # Support safe navigation (&.) as well (required by InternalAffairs/OnSendWithoutOnCSend) def on_csend(node) on_send(node) end private - # Depth-first search for a `can?` send node - def find_permission_query(node) - return node if can_query_call?(node) - - node.children.each do |child| - next unless child.is_a?(Parser::AST::Node) - - found = find_permission_query(child) - return found if found - end - nil - end + def policy_file?(filename) + return false unless filename - def can_query_call?(node) - node.send_type? && - node.receiver.nil? && - node.method?(:can?) && - node.arguments.any? + filename.match?(%r{/(ee/)?app/policies/.+_policy\.rb\z}) || + File.basename(filename).end_with?('_policy.rb') end end end diff --git a/spec/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables_spec.rb b/spec/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables_spec.rb index d8b46417242ef5..a8fb70e5eadfe2 100644 --- a/spec/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables_spec.rb +++ b/spec/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables_spec.rb @@ -6,49 +6,75 @@ require_relative '../../../../../rubocop/cop/gitlab/declarative_policy/no_permission_based_enables' RSpec.describe RuboCop::Cop::Gitlab::DeclarativePolicy::NoPermissionBasedEnables, - :rubocop, :config, - feature_category: :tooling do + :rubocop, :config, feature_category: :tooling do include RuboCop::RSpec::ExpectOffense - it 'registers an offense when a rule enabling a permission depends on can?(:download_code)' do - expect_offense(<<~RUBY) - rule { dependency_scanning_enabled & can?(:download_code) }.enable :read_dependency - ^^^^ Avoid enabling permissions based on other permissions. Do not call `can?(:...)` inside a `rule { ... }` that `.enable`s a permission. + let(:policy_filename) { 'ee/app/policies/projects/some_policy.rb' } + let(:non_policy_filename) { 'app/models/user.rb' } + + it 'registers an offense for can? inside a rule/enable' do + expect_offense(<<~RUBY, policy_filename) + rule { dependency_scanning_enabled & + can?(:download_code) }.enable :read_dependency + ^^^^ Do not call `can?` inside policy files. Use rule/condition predicates based on state/features instead. RUBY end it 'registers an offense for a simple can? condition' do - expect_offense(<<~RUBY) + expect_offense(<<~RUBY, policy_filename) rule { can?(:update_issue) }.enable :admin_issue - ^^^^ Avoid enabling permissions based on other permissions. Do not call `can?(:...)` inside a `rule { ... }` that `.enable`s a permission. + ^^^^ Do not call `can?` inside policy files. Use rule/condition predicates based on state/features instead. RUBY end - it 'does not register an offense for non-permission predicates' do - expect_no_offenses(<<~RUBY) - rule { dependency_scanning_enabled & project.public? }.enable :read_dependency + it 'registers an offense when used inside a policy block' do + expect_offense(<<~RUBY, policy_filename) + rule { developer_access & + can?(:do_thing) }.policy do + ^^^^ Do not call `can?` inside policy files. Use rule/condition predicates based on state/features instead. + enable :thing + end + RUBY + end + + it 'registers an offense inside a condition block' do + expect_offense(<<~RUBY, policy_filename) + condition(:can_do_thing) { can?(:do_thing) } + ^^^^ Do not call `can?` inside policy files. Use rule/condition predicates based on state/features instead. RUBY end - it 'does not register an offense for rules that do not call enable' do - expect_no_offenses(<<~RUBY) - rule { can?(:download_code) } + it 'registers an offense inside a helper method' do + expect_offense(<<~RUBY, policy_filename) + def can_do_thing? + can?(:do_thing) + ^^^^ Do not call `can?` inside policy files. Use rule/condition predicates based on state/features instead. + end RUBY end - it 'handles nested boolean expressions' do - source = <<~RUBY - rule { feature_enabled?(:x) & (project.public? | can?(:read_issue)) }.enable :read_something + it 'registers an offense with safe navigation' do + expect_offense(<<~RUBY, policy_filename) + def can_do_thing? + self&.can?(:do_thing) + ^^^^ Do not call `can?` inside policy files. Use rule/condition predicates based on state/features instead. + end RUBY + end - offenses = inspect_source(source) - expect(offenses.size).to eq(1) + it 'does not register an offense outside policy files' do + expect_no_offenses(<<~RUBY, non_policy_filename) + class User + def check + can?(:foo) # allowed outside of policy files + end + end + RUBY + end - offense = offenses.first - expect(offense.message).to eq( - "Avoid enabling permissions based on other permissions. " \ - "Do not call `can?(:...)` inside a `rule { ... }` that `.enable`s a permission." - ) - expect(offense.location.source).to eq('can?') + it 'does not register an offense for non-permission predicates' do + expect_no_offenses(<<~RUBY, policy_filename) + rule { dependency_scanning_enabled & project.public? }.enable :read_dependency + RUBY end end -- GitLab From c89368a703555bd118118193a50400e8d8006b8f Mon Sep 17 00:00:00 2001 From: Matthew MacRae-Bovell Date: Tue, 9 Sep 2025 09:59:05 -0400 Subject: [PATCH 4/5] Update conventions for dp --- doc/development/permissions/conventions.md | 30 ++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/doc/development/permissions/conventions.md b/doc/development/permissions/conventions.md index f3e186877d7438..9d9b03e36060e0 100644 --- a/doc/development/permissions/conventions.md +++ b/doc/development/permissions/conventions.md @@ -73,3 +73,33 @@ rule { is_author }.policy do enable :delete_note end ``` + +### What to Avoid in Policy Classes + +#### Avoid Using `can?` in Policy Classes + +As a convention, **do not use `can?` inside policy classes**. Calling `can?` from within a policy creates hidden coupling and can introduce circular or non-obvious authorization paths. Instead, policies should be expressed in terms of roles, feature flags, or other explicit state/feature predicates. + +Examples: + +```ruby +rule { developer_access }.policy do + enable :do_thing +end + + +rule { ~prevent_condition & can?(:do_thing) }.policy do + enable :do_other_thing +end +``` + +Can be refactored to avoid `can?` entirely: + +```ruby +rule { developer_access }.policy do + enable :do_thing + enable :do_other_thing +end + +rule { prevent_condition }.prevent :do_other_thing +``` -- GitLab From 71b9a6bf4c0e397c4779a4b4d63dc18ce84a7e35 Mon Sep 17 00:00:00 2001 From: Matthew MacRae-Bovell Date: Tue, 9 Sep 2025 11:29:49 -0400 Subject: [PATCH 5/5] Remove :rubocop from spec --- .../no_permission_based_enables_spec.rb | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/spec/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables_spec.rb b/spec/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables_spec.rb index a8fb70e5eadfe2..dfb7e5e1980be7 100644 --- a/spec/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables_spec.rb +++ b/spec/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables_spec.rb @@ -6,7 +6,7 @@ require_relative '../../../../../rubocop/cop/gitlab/declarative_policy/no_permission_based_enables' RSpec.describe RuboCop::Cop::Gitlab::DeclarativePolicy::NoPermissionBasedEnables, - :rubocop, :config, feature_category: :tooling do + :config, feature_category: :tooling do include RuboCop::RSpec::ExpectOffense let(:policy_filename) { 'ee/app/policies/projects/some_policy.rb' } @@ -14,9 +14,9 @@ it 'registers an offense for can? inside a rule/enable' do expect_offense(<<~RUBY, policy_filename) - rule { dependency_scanning_enabled & - can?(:download_code) }.enable :read_dependency - ^^^^ Do not call `can?` inside policy files. Use rule/condition predicates based on state/features instead. + rule { dependency_scanning_enabled & + can?(:download_code) }.enable :read_dependency + ^^^^ Do not call `can?` inside policy files. Use rule/condition predicates based on state/features instead. RUBY end @@ -29,11 +29,11 @@ it 'registers an offense when used inside a policy block' do expect_offense(<<~RUBY, policy_filename) - rule { developer_access & - can?(:do_thing) }.policy do - ^^^^ Do not call `can?` inside policy files. Use rule/condition predicates based on state/features instead. - enable :thing - end + rule { developer_access & + can?(:do_thing) }.policy do + ^^^^ Do not call `can?` inside policy files. Use rule/condition predicates based on state/features instead. + enable :thing + end RUBY end -- GitLab