Iterate runner values when invalidating cached policies

Michael Angelo Riverarequested to merge
michaelangeloio/fix-invalidate-cached-policies into main

Description

DeclarativePolicy.invalidate(cache, keys) raises NoMethodError whenever the cache holds a policy object. This MR fixes the loop that walks the runners.

How invalidation works

  1. DeclarativePolicy.policy_for stores each policy in the cache under a key like /dp/policy/User:42/Project:7.
  2. Each policy keeps one runner per ability in Base#runners. That is a Hash of ability to runner.
  3. A runner remembers the cache keys of the conditions it read. Runner#dependencies returns them.
  4. invalidate finds the cached policies. It uncaches every runner that read one of the keys you pass, then deletes those keys.

The bug

Step 4 iterated the Hash with one block argument:

policy.runners.each do |runner|
  runner.uncache! if keys.intersect?(runner.dependencies)
end
  • Each item of a Hash is an [ability, runner] pair, so runner is an Array.
  • Array#dependencies does not exist, so the call raises.

The fix iterates the values:

policy.runners.each_value do |runner|
  runner.uncache! if keys.intersect?(runner.dependencies)
end

Why the specs missed it

The existing .invalidate specs build each policy with ReadmePolicy.new. So no policy is ever in the cache, and the loop body never runs. The new spec stores the policies under their cache key, the same way policy_for does. Without the fix it fails with the same NoMethodError.

Proof on a GitLab dev VM

GitLab EE with an Ultimate license, in a request store:

  1. Call Ability.allowed?(user, :read_code, project). This caches 2 policy objects.
  2. Call DeclarativePolicy.invalidate with one of the condition keys.
Gem Result
2.1.0 NoMethodError: undefined method 'dependencies' for an instance of Array
This MR returns nil and uncaches the runner

In the gem:

Check Without the fix With the fix
New spec 1 failure, NoMethodError passes
Full suite not run 244 examples, 0 failures
Rubocop not run no offenses

GitLab does not call invalidate today, so this has no effect on GitLab.com. It matters for any application that uses the invalidation API added in 1.1.0.

The first commit is the minitest bump from !85 (merged), so the JRuby job can install. It is the same change, so it merges cleanly with !85 (merged).

Related Issues

Found while reviewing !85 (merged) and !86 (merged).

Suggested version bump

  • Major (backwards incompatible changes)
  • Minor (backwards compatible changes)
  • Patch (API compatible changes)

Checklist

  • Tests have been added or updated to cover any changes in behavior
  • This does not change the API to consume this library, or a suggested version bump has been provided
  • Add git trailer Changelog: <category>
  • No new runtime dependencies have been introduced

References

Merge request reports