2 files changed, 63 insertions, 41 deletions

diff --git a/lib/gitlab/performance_bar.rb b/lib/gitlab/performance_bar.rb
index 60c8ba5063e..85f4371ec23 100644
--- a/lib/gitlab/performance_bar.rb
+++ b/lib/gitlab/performance_bar.rb
@@ -1,35 +1,53 @@
 module Gitlab
   module PerformanceBar
+    ALLOWED_USER_IDS_KEY = 'performance_bar_allowed_user_ids'.freeze
+    # The time (in seconds) after which a set of allowed user IDs is expired
+    # automatically.
+    ALLOWED_USER_IDS_TIME_TO_LIVE = 10.minutes.to_i
+
     def self.enabled?(current_user = nil)
       Feature.enabled?(:gitlab_performance_bar, current_user)
     end
 
     def self.allowed_user?(user)
-      return false unless allowed_group
+      return false unless allowed_group_name
 
-      if RequestStore.active?
-        RequestStore.fetch('performance_bar:user_member_of_allowed_group') do
-          user_member_of_allowed_group?(user)
-        end
-      else
-        user_member_of_allowed_group?(user)
-      end
+      allowed_user_ids.include?(user.id)
     end
 
-    def self.allowed_group
-      return nil unless Gitlab.config.performance_bar.allowed_group
+    def self.allowed_group_name
+      Gitlab.config.performance_bar.allowed_group
+    end
+
+    def self.allowed_user_ids
+      Gitlab::Redis.with do |redis|
+        if redis.exists(cache_key)
+          redis.smembers(cache_key).map(&:to_i)
+        else
+          group = Group.find_by_full_path(allowed_group_name)
+          # Redis#sadd doesn't accept an empty array, but we still want to use
+          # Redis to let us know that no users are allowed, so we set the
+          # array to [-1] in this case.
+          user_ids =
+            if group
+              GroupMembersFinder.new(group).execute
+                .pluck(:user_id).presence || [-1]
+            else
+              [-1]
+            end
+
+          redis.multi do
+            redis.sadd(cache_key, user_ids)
+            redis.expire(cache_key, ALLOWED_USER_IDS_TIME_TO_LIVE)
+          end
 
-      if RequestStore.active?
-        RequestStore.fetch('performance_bar:allowed_group') do
-          Group.find_by_full_path(Gitlab.config.performance_bar.allowed_group)
+          user_ids
         end
-      else
-        Group.find_by_full_path(Gitlab.config.performance_bar.allowed_group)
       end
     end
 
-    def self.user_member_of_allowed_group?(user)
-      GroupMembersFinder.new(allowed_group).execute.exists?(user_id: user.id)
+    def self.cache_key
+      "#{ALLOWED_USER_IDS_KEY}:#{allowed_group_name}"
     end
   end
 end
diff --git a/spec/lib/gitlab/performance_bar_spec.rb b/spec/lib/gitlab/performance_bar_spec.rb
index cab267cde30..ecdecc03304 100644
--- a/spec/lib/gitlab/performance_bar_spec.rb
+++ b/spec/lib/gitlab/performance_bar_spec.rb
@@ -25,6 +25,27 @@ describe Gitlab::PerformanceBar do
     end
   end
 
+  shared_examples 'allowed user IDs are cached in Redis for 10 minutes' do
+    before do
+      # Warm the Redis cache
+      described_class.allowed_user?(user)
+    end
+
+    it 'caches the allowed user IDs in Redis', :redis do
+      expect do
+        expect(described_class.allowed_user?(user)).to be_truthy
+      end.not_to exceed_query_limit(0)
+    end
+
+    it 'caches the allowed user IDs for 10 minutes', :redis do
+      ttl_cached_user_ids = Gitlab::Redis.with do |redis|
+        redis.ttl(described_class.cache_key)
+      end
+
+      expect(ttl_cached_user_ids).to be <= 10.minutes
+    end
+  end
+
   describe '.allowed_user?' do
     let(:user) { create(:user) }
 
@@ -45,6 +66,8 @@ describe Gitlab::PerformanceBar do
         it 'returns false' do
           expect(described_class.allowed_user?(user)).to be_falsy
         end
+
+        it_behaves_like 'allowed user IDs are cached in Redis for 10 minutes'
       end
 
       context 'when user is a member of the allowed group' do
@@ -55,28 +78,8 @@ describe Gitlab::PerformanceBar do
         it 'returns true' do
           expect(described_class.allowed_user?(user)).to be_truthy
         end
-      end
-    end
-  end
 
-  describe '.allowed_group' do
-    before do
-      stub_performance_bar_setting(allowed_group: 'my-group')
-    end
-
-    context 'when allowed group does not exist' do
-      it 'returns false' do
-        expect(described_class.allowed_group).to be_falsy
-      end
-    end
-
-    context 'when allowed group exists' do
-      let!(:my_group) { create(:group, path: 'my-group') }
-
-      context 'when user is not a member of the allowed group' do
-        it 'returns the group' do
-          expect(described_class.allowed_group).to eq(my_group)
-        end
+        it_behaves_like 'allowed user IDs are cached in Redis for 10 minutes'
       end
     end
 
@@ -85,21 +88,22 @@ describe Gitlab::PerformanceBar do
 
       before do
         create(:group, path: 'my-group')
+        nested_my_group.add_developer(user)
         stub_performance_bar_setting(allowed_group: 'my-org/my-group')
       end
 
       it 'returns the nested group' do
-        expect(described_class.allowed_group).to eq(nested_my_group)
+        expect(described_class.allowed_user?(user)).to be_truthy
       end
     end
 
     context 'when a nested group has the same path', :nested_groups do
       before do
-        create(:group, :nested, path: 'my-group')
+        create(:group, :nested, path: 'my-group').add_developer(user)
       end
 
       it 'returns false' do
-        expect(described_class.allowed_group).to be_falsy
+        expect(described_class.allowed_user?(user)).to be_falsy
       end
     end
   end