Skip to content

How to evaluate a safe authorization check change / permissions refactor

Summary

Sometimes, we will want to change a permissions check in the GitLab codebase without any behavioral changes. What is the best way to ensure that this is the case if we are not 100% confident in the test coverage of a permission? This issue will seek to resolve that.

Details

This issue is going to come up regularly as we seek to consolidate permissions. Having a methodical approach will ensure that we do not introduce regressions in the course of our refactoring.

Our permissions engine, DeclarativePolicy, evaluates conditions based on the object, user, and fact being evaluated. Example:

user = User.last
project = Project.last

policy = DeclarativePolicy.policy_for(user, project)
policy.debug(:read_pipeline)
+ [0] enable when can?(:reporter_access) ((@user_1 : Project/29))
- [0] prevent when all?(builds_disabled, ~internal_builds_disabled) ((@user_1 : Project/29))
- [0] prevent when repository_disabled ((@user_1 : Project/29))
- [0] prevent when repository_disabled ((@user_1 : Project/29))
- [0] prevent when all?(anonymous, ~public_project) ((@user_1 : Project/29))
- [0] prevent when all?(~public_project, ~internal_access, ~project_allowed_for_job_token) ((@user_1 : Project/29))
=> #<DeclarativePolicy::Runner::State:0x0000000113202df8
 @called_conditions=
  #<Set: {"/dp/condition/BasePolicy/admin/User:79",
   "/dp/condition/DeclarativePolicy::Base/anonymous/User:79",
   "/dp/condition/ProjectPolicy/public_project/Project:29",
   "/dp/condition/ProjectPolicy/internal_access/User:79,Project:29",
   "/dp/condition/ProjectPolicy/project_allowed_for_job_token/User:79,Project:29",
   "/dp/condition/BasePolicy/auditor/User:79",
   "/dp/condition/ProjectPolicy/needs_new_sso_session/Project:29",
   "/dp/condition/ProjectPolicy/reporter/User:79,Project:29",
   "/dp/condition/BasePolicy/visual_review_bot/User:79",
   "/dp/condition/BasePolicy/security_bot/User:79",
   "/dp/condition/BasePolicy/alert_bot/User:79",
   "/dp/condition/BasePolicy/support_bot/User:79",
   "/dp/condition/BasePolicy/external_authorization_enabled",
   "/dp/condition/DeclarativePolicy::Base/default",
   "/dp/condition/ProjectPolicy/builds_disabled/User:79,Project:29",
   "/dp/condition/ProjectPolicy/repository_disabled/User:79,Project:29"}>,
 @enabled=true,
 @prevented=false>

This tells us that this user read_pipeline will evaluate to true for this user + project.

But what if, as a refactor, want to change a read_pipeline call in our controller to read_build because the latter makes more sense for some reason?

> policy.debug(:read_build)
+ [0] enable when can?(:reporter_access) ((@user_1 : Project/29))
- [0] prevent when builds_disabled ((@user_1 : Project/29))
- [0] prevent when repository_disabled ((@user_1 : Project/29))
- [0] prevent when all?(anonymous, ~public_project) ((@user_1 : Project/29))
- [0] prevent when all?(~public_project, ~internal_access, ~project_allowed_for_job_token) ((@user_1 : Project/29))
=> #<DeclarativePolicy::Runner::State:0x0000000130385208
 @called_conditions=
  #<Set: {"/dp/condition/BasePolicy/admin/User:79",
   "/dp/condition/DeclarativePolicy::Base/anonymous/User:79",
   "/dp/condition/ProjectPolicy/public_project/Project:29",
   "/dp/condition/ProjectPolicy/internal_access/User:79,Project:29",
   "/dp/condition/ProjectPolicy/project_allowed_for_job_token/User:79,Project:29",
   "/dp/condition/BasePolicy/auditor/User:79",
   "/dp/condition/ProjectPolicy/needs_new_sso_session/Project:29",
   "/dp/condition/ProjectPolicy/reporter/User:79,Project:29",
   "/dp/condition/BasePolicy/visual_review_bot/User:79",
   "/dp/condition/BasePolicy/security_bot/User:79",
   "/dp/condition/BasePolicy/alert_bot/User:79",
   "/dp/condition/BasePolicy/support_bot/User:79",
   "/dp/condition/BasePolicy/external_authorization_enabled",
   "/dp/condition/DeclarativePolicy::Base/default",
   "/dp/condition/ProjectPolicy/builds_disabled/User:79,Project:29",
   "/dp/condition/ProjectPolicy/repository_disabled/User:79,Project:29"}>,
 @enabled=true,
 @prevented=false>

The good news is: same output. For this user + project anyway. How can we be confident that we aren't changing behavior for any other user + project types by making this refactor?

In RSpec, we already have a method for creating a matrix of project visibility types + member access levels and evaluating whether a permissions check will be true or false. Example here.

We can use this method to compare that 2 separate permissions have the same values in each matrix scenario:

    describe 'permissions_refactor', feature_category: :metrics do
      using RSpec::Parameterized::TableSyntax

      let(:old_policy) { :read_build }
      let(:new_policy) { :read_pipeline }

      where(:project_visibility, :role, :allowed) do
        :public   | :anonymous | true 
        :public   | :guest     | true 
        :public   | :reporter  | true 
        :internal | :anonymous | false 
        :internal | :guest     | true  
        :internal | :reporter  | true 
        :private  | :anonymous | false 
        :private  | :guest     | true 
        :private  | :reporter  | true
      end

      with_them do
        let(:current_user) { public_send(role) }
        let(:project) { public_send("#{project_visibility}_project") }

        if params[:allowed]
          it { is_expected.to be_allowed(old_policy) }
          it { is_expected.to be_allowed(new_policy) }
        else
          it { is_expected.not_to be_allowed(old_policy) }
          it { is_expected.not_to be_allowed(new_policy) }
        end
      end
    end

In this example, we are asserting that a guest on a public project will be able to read_build but an anonymous user (unauthenticated) on an internal project will not be able to read_build etc.

Question

  1. If we wrote a script or had an RSpec shared example to make it easy to write a test like the one shown above, would that be somewehat sufficient for demonstrating that 2 policy checks are "equal"?
  2. Are there other conditions we would want to test for? Some elements that might make this testing strategy more complex:
    • Usage of feature flags in policy definitions is not taken into account in the above matrix
    • Other user types (such as DeployToken) are not taken into consideration in the above matrix
    • Other one-off attribute checks, such as project.has_external_wiki? are often taken into consideration in Policy classes.
  3. If we are concerned about missing edge cases, should we instead use something like https://github.com/github/scientist? This would be the safest option, but potentially slower as we would need a multi-step approach (first deploy experiment, then evaluate experiment, then implement switch if deemed successful)
Edited by Jessie Young