Deal with invalid caches

We need to manage some subtle caching edge cases. These are dealt with
by:

- Allowing clients to request a non-forgetful store
- Abstracting over throttling of actions per-period. A new method is
  introduced: `ExclusiveLease.throttle` that takes care of such needs.
- Allow specific keys to be forgotten after running policies
- Invalidating the `CurrentUserMode` cache for `admin_mode` if the
  session-bypass is active (which may change the value).

This deals with a very subtle cache collision problem. When this block
is run, it calls the users update-service, which computes, during
permission checks, the `admin` condition for this user. This is normally
fine, but this callback is executed _before_ the bypass-session admin ID
is set, which means that the cached value is stale during the execution
of the main action.

To avoid this, we deliberately discard the cached value of the `admin`
condition, leaving all other cache values intact.

Thankfully, `admin` is a cheap condition to recompute, and this does not
cause any additional I/O to be run.

Separately, the `Gitlab::Auth::CurrentUserMode` caches the value of
`admin_mode`, which may become invalidated due to the session-bypass
used for sessionless requests. We invalidate the cache when the
session-bypass setting is changed.
This invalidates the cached value for admin_mode, rather than requiring
the caller to know when to force recomputation.

A necessary change is made to prevent duplicate user-detail records.
See: https://gitlab.com/gitlab-org/gitlab/-/issues/333245

Changelog: fixed

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,10 +84,11 @@ 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
-
-    Users::UpdateService.new(self, user: self).execute(validate: false)
+    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
 

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,9 +5,9 @@ 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
+          ::Gitlab::CurrentSettings.current_application_settings.updating_name_disabled_for_users
       end
 
       condition(:can_remove_self, scope: :subject) do
@@ -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?
-            begin
-              yield
-            ensure
-              reset_bypass_session!
-            end
+          return unless block_given?
+
+          begin
+            yield
+          ensure
+            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