Skip to content

Reduce perceived complexity of capability computation

Alex Kalderimis requested to merge ajk-capability-complexity into master

What does this MR do?

Fixes #216879 (closed)

This merge request This changes the capability computation from code (a very large if statement) to a transformation of static data.

The data we transform is a map from ability to requirements:

{
  ability_needed: [:test_one?, :test_two?],
  other_ability: [:test_three?]
}

If the object passes any of the requirement tests, then it must pass the policy ability check for all of the matched abilities. If the tests are inductive then this becomes a one-to-one reverse mapping, but it need not be, so this formulation can actually express stricter policy tests than the current if statement can (i.e. we can assert that some event needs multiple abilities).

This change expresses (like a DSL) the rules as symbols that must be implemented as methods on the receiver, so we need to send the requirements to the receiving event.

This transformation is much simpler code, with a nearly branchless control-flow (we do have a branch that tests if the capability list is empty, since all on an empty list is always true).

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

This is a refactoring MR, and very fortunately we have extensive tests for the functionality that this code supports (a visibility check on events).

Security

This MR is a refactoring of authorization methods. It is not, however, intended to change the result of that authorization in any way. Therefore is should have a security review.

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Alex Kalderimis

Merge request reports