Merge branch 'ajk-declarative-policy-overrides' into 'master'

[DeclarativePolicy] Allow overrides

Closes #207482

See merge request gitlab-org/gitlab!33990

changelogs/unreleased/ajk-declarative-policy-overrides.yml

+---
+title: Allow policies to override parent rules
+merge_request: 33990
+author:
+type: added

doc/development/policies.md

@@ -158,6 +158,89 @@ end
 
 will include all rules from `ProjectPolicy`. The delegated conditions will be evaluated with the correct delegated subject, and will be sorted along with the regular rules in the policy. Note that only the relevant rules for a particular ability will actually be considered.
 
+### Overrides
+
+We allow policies to opt-out of delegated abilities.
+
+Delegated policies may define some abilities in a way that is incorrect for the
+delegating policy. Take for example a child/parent relationship, where some
+abilities can be inferred, and some cannot:
+
+```ruby
+class ParentPolicy < BasePolicy
+  condition(:speaks_spanish) { @subject.spoken_languages.include?(:es) }
+  condition(:has_license) { @subject.driving_license.present? }
+  condition(:enjoys_broccoli) { @subject.enjoyment_of(:broccoli) > 0 }
+  
+  rule { speaks_spanish }.enable :read_spanish
+  rule { has_license }.enable :drive_car
+  rule { enjoys_broccoli }.enable :eat_broccoli
+  rule { ~enjoys_broccoli }.prevent :eat_broccoli
+end
+```
+
+Here, if we delegated the child policy to the parent policy, some values would be
+incorrect - we might correctly infer that the child can speak their parent's
+language, but it would be incorrect to infer that the child can drive or would
+eat broccoli just because the parent can and does.
+
+Some of these things we can deal with - we can forbid driving universally in the
+child policy, for example:
+
+```ruby
+class ChildPolicy < BasePolicy
+  delegate { @subject.parent }
+  
+  rule { default }.prevent :drive_car
+end
+```
+
+But the food preferences one is harder - because of the `prevent` call in the
+parent policy, if the parent dislikes it, even calling `enable` in the child
+will not enable `:eat_broccoli`.
+
+We could remove the `prevent` call in the parent policy, but that still doesn't
+help us, since the rules are different: parents get to eat what they like, and
+children eat what they are given, provided they are well behaved. Allowing
+delegation would end up with only children whose parents enjoy green vegetables
+eating it. But a parent may well give their child broccoli, even if they dislike
+it themselves, because it is good for their child.
+
+The solution it to override the `:eat_broccoli` ability in the child policy:
+
+```ruby
+class ChildPolicy < BasePolicy
+  delegate { @subject.parent }
+  
+  overrides :eat_broccoli
+  
+  condition(:good_kid) { @subject.behavior_level >= Child::GOOD }
+  
+  rule { good_kid }.enable :eat_broccoli
+end
+```
+
+With this definition, the `ChildPolicy` will _never_ look in the `ParentPolicy` to
+satisfy `:eat_broccoli`, but it _will_ use it for any other abilities. The child
+policy can then define `:eat_broccoli` in a way that makes sense for `Child` and not
+`Parent`.
+
+### Alternatives to using `overrides`
+
+Overriding policy delegation is complex, for the same reason delegation is
+complex - it involves reasoning about logical inference, and being clear about
+semantics. Misuse of `override` has the potential to duplicate code, and
+potentially introduce security bugs, allowing things that should be prevented.
+For this reason, it should be used only when other approaches are not feasible.
+
+Other approaches can include for example using different ability names. Choosing
+to eat a food and eating foods you are given are semantically distinct, and they
+could be named differently (perhaps `chooses_to_eat_broccoli` and
+`eats_what_is_given` in this case). It can depend on how polymorphic the call
+site is. If you know that we will always check the policy with a `Parent` or a
+`Child`, then we can choose the appropriate ability name. If the call site is
+polymorphic, then we cannot do that.
+
 ## Specifying Policy Class
 
 You can also override the Policy used for a given subject:

lib/declarative_policy/base.rb

@@ -117,6 +117,23 @@ module DeclarativePolicy
         own_delegations[name] = delegation_block
       end
 
+      # Declare that the given abilities should not be read from delegates.
+      #
+      # This is useful if you have an ability that you want to define
+      # differently in a policy than in a delegated policy, but still want to
+      # delegate all other abilities.
+      #
+      # example:
+      #
+      #   delegate { @subect.parent }
+      #
+      #   overrides :drive_car, :watch_tv
+      #
+      def overrides(*names)
+        @overrides ||= [].to_set
+        @overrides.merge(names)
+      end
+
       # Declares a rule, constructed using RuleDsl, and returns
       # a PolicyDsl which is used for registering the rule with
       # this class. PolicyDsl will call back into Base.enable_when,
@@ -265,9 +282,13 @@ module DeclarativePolicy
       @runners ||= {}
       @runners[ability] ||=
         begin
-          delegated_runners = delegated_policies.values.compact.map { |p| p.runner(ability) }
           own_runner = Runner.new(own_steps(ability))
-          delegated_runners.inject(own_runner, &:merge_runner)
+          if self.class.overrides.include?(ability)
+            own_runner
+          else
+            delegated_runners = delegated_policies.values.compact.map { |p| p.runner(ability) }
+            delegated_runners.inject(own_runner, &:merge_runner)
+          end
         end
     end
 

spec/lib/declarative_policy/overrides_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require_dependency 'rspec-parameterized'
+
+RSpec.describe 'DeclarativePolicy overrides' do
+  let(:foo_policy) do
+    Class.new(DeclarativePolicy::Base) do
+      condition(:foo_prop_cond) { @subject.foo_prop }
+
+      rule { foo_prop_cond }.policy do
+        enable :common_ability
+        enable :foo_prop_ability
+      end
+    end
+  end
+
+  let(:bar_policy) do
+    Class.new(DeclarativePolicy::Base) do
+      delegate { @subject.foo }
+
+      overrides :common_ability
+
+      condition(:bar_prop_cond) { @subject.bar_prop }
+
+      rule { bar_prop_cond }.policy do
+        enable :common_ability
+        enable :bar_prop_ability
+      end
+
+      rule { bar_prop_cond & can?(:foo_prop_ability) }.policy do
+        enable :combined_ability
+      end
+    end
+  end
+
+  before do
+    stub_const('Foo', Struct.new(:foo_prop))
+    stub_const('FooPolicy', foo_policy)
+    stub_const('Bar', Struct.new(:foo, :bar_prop))
+    stub_const('BarPolicy', bar_policy)
+  end
+
+  where(:foo_prop, :bar_prop) do
+    [
+      [true, true],
+      [true, false],
+      [false, true],
+      [false, false]
+    ]
+  end
+
+  with_them do
+    let(:foo) { Foo.new(foo_prop) }
+    let(:bar) { Bar.new(foo, bar_prop) }
+
+    it 'determines the correct bar_prop_ability (non-delegated) permissions for bar' do
+      policy = DeclarativePolicy.policy_for(nil, bar)
+      expect(policy.allowed?(:bar_prop_ability)).to eq(bar_prop)
+    end
+
+    it 'determines the correct foo_prop (non-overridden) permissions for bar' do
+      policy = DeclarativePolicy.policy_for(nil, bar)
+      expect(policy.allowed?(:foo_prop_ability)).to eq(foo_prop)
+    end
+
+    it 'determines the correct common_ability (overridden) permissions for bar' do
+      policy = DeclarativePolicy.policy_for(nil, bar)
+      expect(policy.allowed?(:common_ability)).to eq(bar_prop)
+    end
+
+    it 'determines the correct common_ability permissions for foo' do
+      policy = DeclarativePolicy.policy_for(nil, foo)
+      expect(policy.allowed?(:common_ability)).to eq(foo_prop)
+    end
+
+    it 'allows combinations of overridden and inherited values' do
+      policy = DeclarativePolicy.policy_for(nil, bar)
+      expect(policy.allowed?(:combined_ability)).to eq(foo_prop && bar_prop)
+    end
+  end
+end