Merge branch 'policy_rule_boolean' into 'master'

Add cop banning boolean operators in rule blocks for Policy classes

See merge request gitlab-org/gitlab!49771

.rubocop.yml

@@ -276,6 +276,12 @@ GitlabSecurity/PublicSend:
 Gitlab/DuplicateSpecLocation:
   Enabled: true
 
+Gitlab/PolicyRuleBoolean:
+  Enabled: true
+  Include:
+    - 'app/policies/**/*'
+    - 'ee/app/policies/**/*'
+
 Cop/InjectEnterpriseEditionModule:
   Enabled: true
   Exclude:

.rubocop_manual_todo.yml

@@ -41,6 +41,10 @@ Graphql/ResolverType:
     - 'app/graphql/resolvers/users/group_count_resolver.rb'
     - 'ee/app/graphql/resolvers/vulnerabilities_base_resolver.rb'
 
+Gitlab/PolicyRuleBoolean:
+  Exclude:
+    - 'ee/app/policies/ee/identity_provider_policy.rb'
+
 Rails/SaveBang:
   Exclude:
     - 'ee/spec/controllers/projects/merge_requests_controller_spec.rb'

doc/development/policies.md

@@ -63,10 +63,20 @@ end
 Within the rule DSL, you can use:
 
 - A regular word mentions a condition by name - a rule that is in effect when that condition is truthy.
-- `~` indicates negation.
+- `~` indicates negation, also available as `negate`.
 - `&` and `|` are logical combinations, also available as `all?(...)` and `any?(...)`.
 - `can?(:other_ability)` delegates to the rules that apply to `:other_ability`. Note that this is distinct from the instance method `can?`, which can check dynamically - this only configures a delegation to another ability.
 
+`~`, `&` and `|` operators are overridden methods in
+[`DeclarativePolicy::Rule::Base`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/declarative_policy/rule.rb).
+
+Do not use boolean operators such as `&&` and `||` within the rule DSL,
+as conditions within rule blocks are objects, not booleans. The same
+applies for ternary operators (`condition ? ... : ...`), and `if`
+blocks. These operators cannot be overridden, and are hence banned via a
+[custom
+cop](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/49771).
+
 ## Scores, Order, Performance
 
 To see how the rules get evaluated into a judgment, it is useful in a console to use `policy.debug(:some_ability)`. This prints the rules in the order they are evaluated.

rubocop/cop/gitlab/policy_rule_boolean.rb

+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Gitlab
+      # This cop checks for usage of boolean operators in rule blocks, which
+      # does not work because conditions are objects, not booleans.
+      #
+      # @example
+      #
+      # # bad, `conducts_electricity` returns a Rule object, not a boolean!
+      # rule { conducts_electricity && batteries }.enable :light_bulb
+      #
+      # # good
+      # rule { conducts_electricity & batteries }.enable :light_bulb
+      #
+      # @example
+      #
+      # # bad, `conducts_electricity` returns a Rule object, so the ternary is always going to be true
+      # rule { conducts_electricity ? can?(:magnetize) : batteries }.enable :motor
+      #
+      # # good
+      # rule { conducts_electricity & can?(:magnetize) }.enable :motor
+      # rule { ~conducts_electricity & batteries }.enable :motor
+      class PolicyRuleBoolean < RuboCop::Cop::Cop
+        def_node_search :has_and_operator?, <<~PATTERN
+          (and ...)
+        PATTERN
+
+        def_node_search :has_or_operator?, <<~PATTERN
+          (or ...)
+        PATTERN
+
+        def_node_search :has_if?, <<~PATTERN
+          (if ...)
+        PATTERN
+
+        def on_block(node)
+          return unless node.method_name == :rule
+
+          if has_and_operator?(node)
+            add_offense(node, message: '&& is not allowed within a rule block. Did you mean to use `&`?')
+          end
+
+          if has_or_operator?(node)
+            add_offense(node, message: '|| is not allowed within a rule block. Did you mean to use `|`?')
+          end
+
+          if has_if?(node)
+            add_offense(node, message: 'if and ternary operators are not allowed within a rule block.')
+          end
+        end
+      end
+    end
+  end
+end

spec/rubocop/cop/gitlab/policy_rule_boolean_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'rubocop'
+require 'rubocop/rspec/support'
+require_relative '../../../../rubocop/cop/gitlab/policy_rule_boolean'
+
+RSpec.describe RuboCop::Cop::Gitlab::PolicyRuleBoolean, type: :rubocop do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  it 'registers offense for &&' do
+    expect_offense(<<~SOURCE)
+      rule { conducts_electricity && batteries }.enable :light_bulb
+      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ && is not allowed within a rule block. Did you mean to use `&`?
+    SOURCE
+  end
+
+  it 'registers offense for ||' do
+    expect_offense(<<~SOURCE)
+      rule { conducts_electricity || batteries }.enable :light_bulb
+      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ || is not allowed within a rule block. Did you mean to use `|`?
+    SOURCE
+  end
+
+  it 'registers offense for if' do
+    expect_offense(<<~SOURCE)
+      rule { if conducts_electricity then can?(:magnetize) else batteries end }.enable :motor
+      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ if and ternary operators are not allowed within a rule block.
+    SOURCE
+  end
+
+  it 'registers offense for ternary operator' do
+    expect_offense(<<~SOURCE)
+      rule { conducts_electricity ? can?(:magnetize) : batteries }.enable :motor
+      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ if and ternary operators are not allowed within a rule block.
+    SOURCE
+  end
+
+  it 'registers no offense for &' do
+    expect_no_offenses(<<~SOURCE)
+      rule { conducts_electricity & batteries }.enable :light_bulb
+    SOURCE
+  end
+
+  it 'registers no offense for |' do
+    expect_no_offenses(<<~SOURCE)
+      rule { conducts_electricity | batteries }.enable :light_bulb
+    SOURCE
+  end
+end