diff --git a/.rubocop.yml b/.rubocop.yml index 8647c7ffab4de5e2d01a68d5b3b2b7be58f8d8b1..c3c6cd887b6c8508d4f8dbfd2acbf71d56a6401c 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/doc/development/permissions/conventions.md b/doc/development/permissions/conventions.md index f3e186877d7438e9fd9b345707da751b562c35a0..9d9b03e36060e065d9a1bfa9cd213f143745a502 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 +``` 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 0000000000000000000000000000000000000000..fb8e23764754b544af5bedca7938815f0e5ae061 --- /dev/null +++ b/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +require 'rubocop' + +module RuboCop + module Cop + module Gitlab + module DeclarativePolicy + # Forbid calling `can?` anywhere in policy files. + # + # Why: Using `can?` inside policies creates hidden coupling and + # circular/indirect authorization paths. Encode logic as rules/conditions + # over state/feature predicates instead. + # + # This cop only runs for files that look like policy files: + # - app/policies/**/*_policy.rb + # - ee/app/policies/**/*_policy.rb + # + # @example + # # bad - using can? directly in a rule + # rule { developer_access & can?(:download_code) }.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 = 'Do not call `can?` inside policy files. ' \ + 'Use rule/condition predicates based on state/features instead.' + RESTRICT_ON_SEND = %i[can?].freeze + + def on_send(node) + return unless policy_file?(processed_source.file_path) + + add_offense(node.loc.selector) + end + + def on_csend(node) + on_send(node) + end + + private + + def policy_file?(filename) + return false unless filename + + filename.match?(%r{/(ee/)?app/policies/.+_policy\.rb\z}) || + File.basename(filename).end_with?('_policy.rb') + 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 0000000000000000000000000000000000000000..dfb7e5e1980be70d644f2f5da9b2c50e58cd2d0a --- /dev/null +++ b/spec/rubocop/cop/gitlab/declarative_policy/no_permission_based_enables_spec.rb @@ -0,0 +1,80 @@ +# 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, + :config, feature_category: :tooling do + include RuboCop::RSpec::ExpectOffense + + 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, policy_filename) + rule { can?(:update_issue) }.enable :admin_issue + ^^^^ Do not call `can?` inside policy files. Use rule/condition predicates based on state/features instead. + RUBY + end + + 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 '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 '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 + + 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 + + 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