Merge branch 'ajk-declarative-policy-integration-no-spec-changes' into 'master'

Fix invalid cached values for `BasePolicy#admin?` [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!63395

app/models/ability.rb

@@ -54,7 +54,7 @@ class Ability
       end
     end
 
-    def allowed?(user, action, subject = :global, opts = {})
+    def allowed?(user, ability, subject = :global, opts = {})
       if subject.is_a?(Hash)
         opts = subject
         subject = :global
@@ -64,21 +64,76 @@ class Ability
 
       case opts[:scope]
       when :user
-        DeclarativePolicy.user_scope { policy.can?(action) }
+        DeclarativePolicy.user_scope { policy.allowed?(ability) }
       when :subject
-        DeclarativePolicy.subject_scope { policy.can?(action) }
+        DeclarativePolicy.subject_scope { policy.allowed?(ability) }
       else
-        policy.can?(action)
+        policy.allowed?(ability)
       end
+    ensure
+      # TODO: replace with runner invalidation:
+      # See: https://gitlab.com/gitlab-org/declarative-policy/-/merge_requests/24
+      # See: https://gitlab.com/gitlab-org/declarative-policy/-/merge_requests/25
+      forget_runner_result(policy.runner(ability)) if policy && ability_forgetting?
     end
 
     def policy_for(user, subject = :global)
-      cache = Gitlab::SafeRequestStore.active? ? Gitlab::SafeRequestStore : {}
-      DeclarativePolicy.policy_for(user, subject, cache: cache)
+      DeclarativePolicy.policy_for(user, subject, cache: ::Gitlab::SafeRequestStore.storage)
+    end
+
+    # This method is something of a band-aid over the problem. The problem is
+    # that some conditions may not be re-entrant, if facts change.
+    # (`BasePolicy#admin?` is a known offender, due to the effects of
+    # `admin_mode`)
+    #
+    # To deal with this we need to clear two elements of state: the offending
+    # conditions (selected by 'pattern') and the cached ability checks (cached
+    # on the `policy#runner(ability)`).
+    #
+    # Clearing the conditions (see `forget_all_but`) is fairly robust, provided
+    # the pattern is not _under_-selective. Clearing the runners is harder,
+    # since there is not good way to know which abilities any given condition
+    # may affect. The approach taken here (see `forget_runner_result`) is to
+    # discard all runner results generated during a `forgetting` block. This may
+    # be _under_-selective if a runner prior to this block cached a state value
+    # that might now be invalid.
+    #
+    # TODO: add some kind of reverse-dependency mapping in DeclarativePolicy
+    # See: https://gitlab.com/gitlab-org/declarative-policy/-/issues/14
+    def forgetting(pattern, &block)
+      was_forgetting = ability_forgetting?
+      ::Gitlab::SafeRequestStore[:ability_forgetting] = true
+      keys_before = ::Gitlab::SafeRequestStore.storage.keys
+
+      yield
+    ensure
+      ::Gitlab::SafeRequestStore[:ability_forgetting] = was_forgetting
+      forget_all_but(keys_before, matching: pattern)
     end
 
     private
 
+    def ability_forgetting?
+      ::Gitlab::SafeRequestStore[:ability_forgetting]
+    end
+
+    def forget_all_but(keys_before, matching:)
+      keys_after = ::Gitlab::SafeRequestStore.storage.keys
+
+      added_keys = keys_after - keys_before
+      added_keys.each do |key|
+        if key.is_a?(String) && key.start_with?('/dp') && key =~ matching
+          ::Gitlab::SafeRequestStore.delete(key)
+        end
+      end
+    end
+
+    def forget_runner_result(runner)
+      # TODO: add support in DP for this
+      # See: https://gitlab.com/gitlab-org/declarative-policy/-/issues/15
+      runner.instance_variable_set(:@state, nil)
+    end
+
     def apply_filters_if_needed(elements, user, filters)
       filters.each do |ability, filter|
         elements = filter.call(elements) unless allowed?(user, ability)

app/models/user.rb

@@ -84,11 +84,12 @@ class User < ApplicationRecord
 
     update_tracked_fields(request)
 
-    lease = Gitlab::ExclusiveLease.new("user_update_tracked_fields:#{id}", timeout: 1.hour.to_i)
-    return unless lease.try_obtain
-
+    Gitlab::ExclusiveLease.throttle(id) do
+      ::Ability.forgetting(/admin/) do
         Users::UpdateService.new(self, user: self).execute(validate: false)
       end
+    end
+  end
   # rubocop: enable CodeReuse/ServiceClass
 
   attr_accessor :force_random_password

app/policies/base_policy.rb

@@ -67,7 +67,7 @@ class BasePolicy < DeclarativePolicy::Base
 
   rule { default }.enable :read_cross_project
 
-  condition(:is_gitlab_com) { ::Gitlab.dev_env_or_com? }
+  condition(:is_gitlab_com, score: 0, scope: :global) { ::Gitlab.dev_env_or_com? }
 end
 
 BasePolicy.prepend_mod_with('BasePolicy')

app/services/users/update_service.rb

@@ -17,6 +17,7 @@ module Users
       yield(@user) if block_given?
 
       user_exists = @user.persisted?
+      @user.user_detail # prevent assignment
 
       discard_read_only_attributes
       assign_attributes

config/initializers/declarative_policy.rb

 # frozen_string_literal: true
 
+require 'declarative_policy'
+
 DeclarativePolicy.configure do
   named_policy :global, ::GlobalPolicy
 end

ee/app/policies/ee/user_policy.rb

@@ -5,7 +5,7 @@ module EE
     extend ActiveSupport::Concern
 
     prepended do
-      condition(:updating_name_disabled_for_users) do
+      condition(:updating_name_disabled_for_users, scope: :global) do
         ::License.feature_available?(:disable_name_update_for_users) &&
           ::Gitlab::CurrentSettings.current_application_settings.updating_name_disabled_for_users
       end
@@ -16,10 +16,7 @@ module EE
 
       rule { can?(:update_user) }.enable :update_name
 
-      rule do
-        updating_name_disabled_for_users &
-        ~admin
-      end.prevent :update_name
+      rule { updating_name_disabled_for_users & ~admin }.prevent :update_name
 
       rule { user_is_self & ~can_remove_self }.prevent :destroy_user
     end

ee/spec/policies/user_policy_spec.rb

@@ -84,6 +84,18 @@ RSpec.describe UserPolicy do
           context 'when admin mode disabled' do
             it { is_expected.not_to be_allowed(:update_name) }
           end
+
+          context 'when admin mode is disabled, and then enabled following sessionless login' do
+            def policy
+              # method, because we want a fresh cache each time.
+              described_class.new(current_user, user)
+            end
+
+            it 'changes from prevented to allowed', :request_store do
+              expect { Gitlab::Auth::CurrentUserMode.bypass_session!(current_user.id) }
+                .to change { policy.allowed?(:update_name) }.from(false).to(true)
+            end
+          end
         end
       end
     end

ee/spec/requests/admin/credentials_controller_spec.rb

@@ -182,6 +182,7 @@ RSpec.describe Admin::CredentialsController, type: :request do
             before do
               allow(Ability).to receive(:allowed?).with(admin, :log_in, :global) { true }
               allow(Ability).to receive(:allowed?).with(admin, :revoke_token, personal_access_token) { false }
+              allow(Ability).to receive(:allowed?).with(admin, :update_name, admin).and_call_original
             end
 
             let(:token_id) { personal_access_token.id }

lib/gitlab/auth/current_user_mode.rb

@@ -27,22 +27,27 @@ module Gitlab
         # will bypass the session check for a user that was already in admin mode
         #
         # If passed a block, it will surround the block execution and reset the session
-        # bypass at the end; otherwise use manually '.reset_bypass_session!'
+        # bypass at the end; otherwise you must remember to call '.reset_bypass_session!'
         def bypass_session!(admin_id)
           Gitlab::SafeRequestStore[CURRENT_REQUEST_BYPASS_SESSION_ADMIN_ID_RS_KEY] = admin_id
+          # Bypassing the session invalidates the cached value of admin_mode?
+          # Any new calls need to be re-computed.
+          uncache_admin_mode_state(admin_id)
 
           Gitlab::AppLogger.debug("Bypassing session in admin mode for: #{admin_id}")
 
-          if block_given?
+          return unless block_given?
+
           begin
             yield
           ensure
-              reset_bypass_session!
-            end
+            reset_bypass_session!(admin_id)
           end
         end
 
-        def reset_bypass_session!
+        def reset_bypass_session!(admin_id = nil)
+          # Restoring the session bypass invalidates the cached value of admin_mode?
+          uncache_admin_mode_state(admin_id)
           Gitlab::SafeRequestStore.delete(CURRENT_REQUEST_BYPASS_SESSION_ADMIN_ID_RS_KEY)
         end
 
@@ -50,10 +55,21 @@ module Gitlab
           Gitlab::SafeRequestStore[CURRENT_REQUEST_BYPASS_SESSION_ADMIN_ID_RS_KEY]
         end
 
+        def uncache_admin_mode_state(admin_id = nil)
+          if admin_id
+            key = { res: :current_user_mode, user: admin_id, method: :admin_mode? }
+            Gitlab::SafeRequestStore.delete(key)
+          else
+            Gitlab::SafeRequestStore.delete_if do |key|
+              key.is_a?(Hash) && key[:res] == :current_user_mode && key[:method] == :admin_mode?
+            end
+          end
+        end
+
         # Store in the current request the provided user model (only if in admin mode)
         # and yield
         def with_current_admin(admin)
-          return yield unless self.new(admin).admin_mode?
+          return yield unless new(admin).admin_mode?
 
           Gitlab::SafeRequestStore[CURRENT_REQUEST_ADMIN_MODE_USER_RS_KEY] = admin
 

lib/gitlab/exclusive_lease.rb

@@ -36,6 +36,28 @@ module Gitlab
       end
     end
 
+    # yield to the {block} at most {count} times per {period}
+    #
+    # Defaults to once per hour.
+    #
+    # For example:
+    #
+    #   # toot the train horn at most every 20min:
+    #   throttle(locomotive.id, count: 3, period: 1.hour) { toot_train_horn }
+    #   # Brake suddenly at most once every minute:
+    #   throttle(locomotive.id, period: 1.minute) { brake_suddenly }
+    #   # Specify a uniqueness group:
+    #   throttle(locomotive.id, group: :locomotive_brake) { brake_suddenly }
+    #
+    # If a group is not specified, each block will get a separate group to itself.
+    def self.throttle(key, group: nil, period: 1.hour, count: 1, &block)
+      group ||= block.source_location.join(':')
+
+      return if new("el:throttle:#{group}:#{key}", timeout: period.to_i / count).waiting?
+
+      yield
+    end
+
     def self.cancel(key, uuid)
       return unless key.present?
 
@@ -79,6 +101,11 @@ module Gitlab
       end
     end
 
+    # This lease is waiting to obtain
+    def waiting?
+      !try_obtain
+    end
+
     # Try to renew an existing lease. Return lease UUID on success,
     # false if the lease is taken by a different UUID or inexistent.
     def renew

lib/gitlab/safe_request_store.rb

@@ -20,6 +20,15 @@ module Gitlab
       end
     end
 
+    # Access to the backing storage of the request store. This returns an object
+    # with `[]` and `[]=` methods that does not discard values.
+    #
+    # This can be useful if storage is needed for a delimited purpose, and the
+    # forgetful nature of the null store is undesirable.
+    def self.storage
+      store.store
+    end
+
     # This method accept an options hash to be compatible with
     # ActiveSupport::Cache::Store#write method. The options are
     # not passed to the underlying cache implementation because
@@ -27,5 +36,11 @@ module Gitlab
     def self.write(key, value, options = nil)
       store.write(key, value)
     end
+
+    def self.delete_if(&block)
+      return unless RequestStore.active?
+
+      storage.delete_if { |k, v| block.call(k) }
+    end
   end
 end

spec/controllers/projects/protected_branches_controller_spec.rb

@@ -42,7 +42,7 @@ RSpec.describe Projects::ProtectedBranchesController do
 
     context 'when a policy restricts rule deletion' do
       before do
-        policy = instance_double(ProtectedBranchPolicy, can?: false)
+        policy = instance_double(ProtectedBranchPolicy, allowed?: false)
         allow(ProtectedBranchPolicy).to receive(:new).and_return(policy)
       end
 
@@ -70,7 +70,7 @@ RSpec.describe Projects::ProtectedBranchesController do
 
     context 'when a policy restricts rule deletion' do
       before do
-        policy = instance_double(ProtectedBranchPolicy, can?: false)
+        policy = instance_double(ProtectedBranchPolicy, allowed?: false)
         allow(ProtectedBranchPolicy).to receive(:new).and_return(policy)
       end
 
@@ -97,7 +97,7 @@ RSpec.describe Projects::ProtectedBranchesController do
 
     context 'when a policy restricts rule deletion' do
       before do
-        policy = instance_double(ProtectedBranchPolicy, can?: false)
+        policy = instance_double(ProtectedBranchPolicy, allowed?: false)
         allow(ProtectedBranchPolicy).to receive(:new).and_return(policy)
       end
 

spec/frontend/fixtures/releases.rb

@@ -146,6 +146,7 @@ RSpec.describe 'Releases (JavaScript fixtures)' do
       post_graphql(query, current_user: admin, variables: { fullPath: project.full_path })
 
       expect_graphql_errors_to_be_empty
+      expect(graphql_data_at(:project, :releases)).to be_present
     end
 
     it "graphql/#{one_release_query_path}.json" do
@@ -154,6 +155,7 @@ RSpec.describe 'Releases (JavaScript fixtures)' do
       post_graphql(query, current_user: admin, variables: { fullPath: project.full_path, tagName: release.tag })
 
       expect_graphql_errors_to_be_empty
+      expect(graphql_data_at(:project, :release)).to be_present
     end
 
     it "graphql/#{one_release_for_editing_query_path}.json" do
@@ -162,6 +164,7 @@ RSpec.describe 'Releases (JavaScript fixtures)' do
       post_graphql(query, current_user: admin, variables: { fullPath: project.full_path, tagName: release.tag })
 
       expect_graphql_errors_to_be_empty
+      expect(graphql_data_at(:project, :release)).to be_present
     end
   end
 end

spec/lib/gitlab/exclusive_lease_spec.rb

@@ -166,4 +166,82 @@ RSpec.describe Gitlab::ExclusiveLease, :clean_gitlab_redis_shared_state do
       expect(described_class.get_uuid(unique_key)).to be_falsey
     end
   end
+
+  describe '.throttle' do
+    it 'prevents repeated execution of the block' do
+      number = 0
+
+      action = -> { described_class.throttle(1) { number += 1 } }
+
+      action.call
+      action.call
+
+      expect(number).to eq 1
+    end
+
+    it 'is distinct by block' do
+      number = 0
+
+      described_class.throttle(1) { number += 1 }
+      described_class.throttle(1) { number += 1 }
+
+      expect(number).to eq 2
+    end
+
+    it 'is distinct by key' do
+      number = 0
+
+      action = ->(k) { described_class.throttle(k) { number += 1 } }
+
+      action.call(:a)
+      action.call(:b)
+      action.call(:a)
+
+      expect(number).to eq 2
+    end
+
+    it 'allows a group to be passed' do
+      number = 0
+
+      described_class.throttle(1, group: :a) { number += 1 }
+      described_class.throttle(1, group: :b) { number += 1 }
+      described_class.throttle(1, group: :a) { number += 1 }
+      described_class.throttle(1, group: :b) { number += 1 }
+
+      expect(number).to eq 2
+    end
+
+    it 'defaults to a 60min timeout' do
+      expect(described_class).to receive(:new).with(anything, hash_including(timeout: 1.hour.to_i)).and_call_original
+
+      described_class.throttle(1) {}
+    end
+
+    it 'allows count to be specified' do
+      expect(described_class)
+        .to receive(:new)
+        .with(anything, hash_including(timeout: 15.minutes.to_i))
+        .and_call_original
+
+      described_class.throttle(1, count: 4) {}
+    end
+
+    it 'allows period to be specified' do
+      expect(described_class)
+        .to receive(:new)
+        .with(anything, hash_including(timeout: 1.day.to_i))
+        .and_call_original
+
+      described_class.throttle(1, period: 1.day) {}
+    end
+
+    it 'allows period and count to be specified' do
+      expect(described_class)
+        .to receive(:new)
+        .with(anything, hash_including(timeout: 30.minutes.to_i))
+        .and_call_original
+
+      described_class.throttle(1, count: 48, period: 1.day) {}
+    end
+  end
 end

spec/models/ability_spec.rb

@@ -342,4 +342,45 @@ RSpec.describe Ability do
       end
     end
   end
+
+  describe 'forgetting', :request_store do
+    it 'allows us to discard specific values from the DeclarativePolicy cache' do
+      user_a = build_stubbed(:user)
+      user_b = build_stubbed(:user)
+
+      # expect these keys to remain
+      Gitlab::SafeRequestStore[:administrator] = :wibble
+      Gitlab::SafeRequestStore['admin'] = :wobble
+      described_class.allowed?(user_b, :read_all_resources)
+      # expect the DeclarativePolicy cache keys added by this action not to remain
+      described_class.forgetting(/admin/) do
+        described_class.allowed?(user_a, :read_all_resources)
+      end
+
+      keys = Gitlab::SafeRequestStore.storage.keys
+
+      expect(keys).to include(
+        :administrator,
+        'admin',
+        "/dp/condition/BasePolicy/admin/#{user_b.id}"
+      )
+      expect(keys).not_to include("/dp/condition/BasePolicy/admin/#{user_a.id}")
+    end
+
+    # regression spec for re-entrant admin condition checks
+    # See: https://gitlab.com/gitlab-org/gitlab/-/issues/332983
+    context 'when bypassing the session' do
+      let(:user) { build_stubbed(:admin) }
+      let(:ability) { :admin_all_resources } # any admin-only ability is fine here.
+
+      def check_ability
+        described_class.forgetting(/admin/) { described_class.allowed?(user, ability) }
+      end
+
+      it 'allows us to have re-entrant evaluation of admin-only permissions' do
+        expect { Gitlab::Auth::CurrentUserMode.bypass_session!(user.id) }
+          .to change { check_ability }.from(false).to(true)
+      end
+    end
+  end
 end

spec/policies/base_policy_spec.rb

@@ -22,31 +22,45 @@ RSpec.describe BasePolicy do
     end
   end
 
-  shared_examples 'admin only access' do |policy|
+  shared_examples 'admin only access' do |ability|
+    def policy
+      # method, because we want a fresh cache each time.
+      described_class.new(current_user, nil)
+    end
+
     let(:current_user) { build_stubbed(:user) }
 
-    subject { described_class.new(current_user, nil) }
+    subject { policy }
 
-    it { is_expected.not_to be_allowed(policy) }
+    it { is_expected.not_to be_allowed(ability) }
 
-    context 'for admins' do
+    context 'with an admin' do
       let(:current_user) { build_stubbed(:admin) }
 
       it 'allowed when in admin mode' do
         enable_admin_mode!(current_user)
 
-        is_expected.to be_allowed(policy)
+        is_expected.to be_allowed(ability)
       end
 
       it 'prevented when not in admin mode' do
-        is_expected.not_to be_allowed(policy)
+        is_expected.not_to be_allowed(ability)
       end
     end
 
-    context 'for anonymous' do
+    context 'with anonymous' do
       let(:current_user) { nil }
 
-      it { is_expected.not_to be_allowed(policy) }
+      it { is_expected.not_to be_allowed(ability) }
+    end
+
+    describe 'bypassing the session for sessionless login', :request_store do
+      let(:current_user) { build_stubbed(:admin) }
+
+      it 'changes from prevented to allowed' do
+        expect { Gitlab::Auth::CurrentUserMode.bypass_session!(current_user.id) }
+          .to change { policy.allowed?(ability) }.from(false).to(true)
+      end
     end
   end
 

spec/requests/api/graphql/group_query_spec.rb

@@ -96,7 +96,7 @@ RSpec.describe 'getting group information' do
         expect(graphql_data['group']).to be_nil
       end
 
-      it 'avoids N+1 queries' do
+      it 'avoids N+1 queries', :assume_throttled do
         pending('See: https://gitlab.com/gitlab-org/gitlab/-/issues/245272')
 
         queries = [{ query: group_query(group1) },

spec/requests/api/graphql/mutations/merge_requests/set_assignees_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe 'Setting assignees of a merge request' do
+RSpec.describe 'Setting assignees of a merge request', :assume_throttled do
   include GraphqlHelpers
 
   let_it_be(:project) { create(:project, :repository) }

spec/requests/api/protected_branches_spec.rb

@@ -228,7 +228,7 @@ RSpec.describe API::ProtectedBranches do
 
       context 'when a policy restricts rule deletion' do
         before do
-          policy = instance_double(ProtectedBranchPolicy, can?: false)
+          policy = instance_double(ProtectedBranchPolicy, allowed?: false)
           expect(ProtectedBranchPolicy).to receive(:new).and_return(policy)
         end
 
@@ -278,7 +278,7 @@ RSpec.describe API::ProtectedBranches do
 
     context 'when a policy restricts rule deletion' do
       before do
-        policy = instance_double(ProtectedBranchPolicy, can?: false)
+        policy = instance_double(ProtectedBranchPolicy, allowed?: false)
         expect(ProtectedBranchPolicy).to receive(:new).and_return(policy)
       end
 

spec/services/protected_branches/create_service_spec.rb

@@ -40,7 +40,7 @@ RSpec.describe ProtectedBranches::CreateService do
 
     context 'when a policy restricts rule creation' do
       before do
-        policy = instance_double(ProtectedBranchPolicy, can?: false)
+        policy = instance_double(ProtectedBranchPolicy, allowed?: false)
         expect(ProtectedBranchPolicy).to receive(:new).and_return(policy)
       end
 

spec/services/protected_branches/destroy_service_spec.rb

@@ -18,7 +18,7 @@ RSpec.describe ProtectedBranches::DestroyService do
 
     context 'when a policy restricts rule deletion' do
       before do
-        policy = instance_double(ProtectedBranchPolicy, can?: false)
+        policy = instance_double(ProtectedBranchPolicy, allowed?: false)
         expect(ProtectedBranchPolicy).to receive(:new).and_return(policy)
       end
 

spec/services/protected_branches/update_service_spec.rb

@@ -27,7 +27,7 @@ RSpec.describe ProtectedBranches::UpdateService do
 
     context 'when a policy restricts rule creation' do
       before do
-        policy = instance_double(ProtectedBranchPolicy, can?: false)
+        policy = instance_double(ProtectedBranchPolicy, allowed?: false)
         expect(ProtectedBranchPolicy).to receive(:new).and_return(policy)
       end
 

spec/spec_helper.rb

@@ -346,6 +346,15 @@ RSpec.configure do |config|
     Gitlab::WithRequestStore.with_request_store { example.run }
   end
 
+  # previous test runs may have left some resources throttled
+  config.before do
+    ::Gitlab::ExclusiveLease.reset_all!("el:throttle:*")
+  end
+
+  config.before(:example, :assume_throttled) do |example|
+    allow(::Gitlab::ExclusiveLease).to receive(:throttle).and_return(nil)
+  end
+
   config.before(:example, :request_store) do
     # Clear request store before actually starting the spec (the
     # `around` above will have the request store enabled for all