Merge branch 'skip-individual-subscribed-access-filter' into 'master'

Check access for only the requesting user when determining subscribed status

See merge request gitlab-org/gitlab!57201

app/models/concerns/issuable.rb

@@ -437,7 +437,7 @@ module Issuable
   end
 
   def subscribed_without_subscriptions?(user, project)
-    participants(user).include?(user)
+    participant?(user)
   end
 
   def can_assign_epic?(user)

app/models/concerns/participable.rb

@@ -56,18 +56,34 @@ module Participable
   # This method processes attributes of objects in breadth-first order.
   #
   # Returns an Array of User instances.
-  def participants(current_user = nil)
-    all_participants[current_user]
+  def participants(user = nil)
+    filtered_participants_hash[user]
+  end
+
+  # Checks if the user is a participant in a discussion.
+  #
+  # This method processes attributes of objects in breadth-first order.
+  #
+  # Returns a Boolean.
+  def participant?(user)
+    can_read_participable?(user) &&
+      all_participants_hash[user].include?(user)
   end
 
   private
 
-  def all_participants
-    @all_participants ||= Hash.new do |hash, user|
+  def all_participants_hash
+    @all_participants_hash ||= Hash.new do |hash, user|
       hash[user] = raw_participants(user)
     end
   end
 
+  def filtered_participants_hash
+    @filtered_participants_hash ||= Hash.new do |hash, user|
+      hash[user] = filter_by_ability(all_participants_hash[user])
+    end
+  end
+
   def raw_participants(current_user = nil)
     current_user ||= author
     ext = Gitlab::ReferenceExtractor.new(project, current_user)
@@ -98,8 +114,6 @@ module Participable
     end
 
     participants.merge(ext.users)
-
-    filter_by_ability(participants)
   end
 
   def filter_by_ability(participants)
@@ -110,6 +124,15 @@ module Participable
       Ability.users_that_can_read_project(participants.to_a, project)
     end
   end
+
+  def can_read_participable?(participant)
+    case self
+    when PersonalSnippet
+      participant.can?(:read_snippet, self)
+    else
+      participant.can?(:read_project, project)
+    end
+  end
 end
 
 Participable.prepend_if_ee('EE::Participable')

changelogs/unreleased/skip-individual-subscribed-access-filter.yml

+---
+title: Check access only for requesting user when checking if subscribed
+merge_request: 57201
+author:
+type: performance

ee/app/models/concerns/ee/participable.rb

@@ -10,5 +10,12 @@ module EE
 
       Ability.users_that_can_read_group(participants.to_a, self.group)
     end
+
+    override :can_read_participable?
+    def can_read_participable?(participant)
+      return super unless self.is_a?(Epic)
+
+      participant.can?(:read_group, group)
+    end
   end
 end

ee/app/models/ee/issue.rb

@@ -115,25 +115,6 @@ module EE
       issues.preload(project: [:route, { namespace: [:route] }])
     end
 
-    # override
-    def subscribed_without_subscriptions?(user, *)
-      # TODO: this really shouldn't be necessary, because the support
-      # bot should be a participant (which is what the superclass
-      # method checks for). However, the support bot gets filtered out
-      # at the end of Participable#raw_participants as not being able
-      # to read the project. Overriding *that* behavior is problematic
-      # because it doesn't use the Policy framework, and instead uses a
-      # custom-coded Ability.users_that_can_read_project, which is...
-      # a pain to override in EE. So... here we say, the support bot
-      # is subscribed by default, until an unsubscribed record appears,
-      # even though it's not *technically* a participant in this issue.
-
-      # Making the support bot subscribed to every issue is not as bad as it
-      # seems, though, since it isn't permitted to :receive_notifications,
-      # and doesn't actually show up in the participants list.
-      user.bot? || super
-    end
-
     # override
     def weight
       super if weight_available?

ee/spec/models/concerns/ee/participable_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe EE::Participable do
+  context 'participable is an epic' do
+    let(:model) { Epic }
+    let(:instance) { model.new }
+
+    let(:user1) { build(:user) }
+    let(:user2) { build(:user) }
+    let(:user3) { build(:user) }
+    let(:group) { build(:group, :public) }
+
+    before do
+      allow(model).to receive(:participant_attrs).and_return([:foo, :bar])
+    end
+
+    describe '#participants' do
+      it 'returns the list of participants' do
+        expect(instance).to receive(:foo).and_return(user2)
+        expect(instance).to receive(:bar).and_return(user3)
+        expect(instance).to receive(:group).and_return(group)
+
+        participants = instance.participants(user1)
+        expect(participants).to contain_exactly(user2, user3)
+      end
+    end
+
+    describe '#participant?' do
+      it 'returns whether the user is a participant' do
+        allow(instance).to receive(:foo).and_return(user2)
+        allow(instance).to receive(:bar).and_return(user3)
+        allow(instance).to receive(:group).and_return(group)
+
+        expect(instance.participant?(user1)).to be false
+        expect(instance.participant?(user2)).to be true
+        expect(instance.participant?(user3)).to be true
+      end
+    end
+  end
+end

spec/models/concerns/issuable_spec.rb

@@ -380,7 +380,7 @@ RSpec.describe Issuable do
 
     context 'user is a participant in the issue' do
       before do
-        allow(issue).to receive(:participants).with(user).and_return([user])
+        allow(issue).to receive(:participant?).with(user).and_return(true)
       end
 
       it 'returns false when no subcription exists' do

spec/models/concerns/participable_spec.rb

@@ -39,11 +39,12 @@ RSpec.describe Participable do
       expect(participants).to include(user3)
     end
 
-    it 'caches the raw list of participants' do
+    it 'caches the list of filtered participants' do
       instance = model.new
       user1 = build(:user)
 
-      expect(instance).to receive(:raw_participants).once
+      expect(instance).to receive(:all_participants_hash).once.and_return({})
+      expect(instance).to receive(:filter_by_ability).once
 
       instance.participants(user1)
       instance.participants(user1)
@@ -91,5 +92,71 @@ RSpec.describe Participable do
         expect(ext_arg).to be_an_instance_of(Gitlab::ReferenceExtractor)
       end
     end
+
+    context 'participable is a personal snippet' do
+      let(:model) { PersonalSnippet }
+      let(:instance) { model.new(author: user1) }
+
+      let(:user1) { build(:user) }
+      let(:user2) { build(:user) }
+      let(:user3) { build(:user) }
+
+      before do
+        allow(model).to receive(:participant_attrs).and_return([:foo, :bar])
+      end
+
+      it 'returns the list of participants' do
+        expect(instance).to receive(:foo).and_return(user1)
+        expect(instance).to receive(:bar).and_return(user2)
+
+        participants = instance.participants(user1)
+        expect(participants).to contain_exactly(user1)
+      end
+    end
+  end
+
+  describe '#participant?' do
+    let(:instance) { model.new }
+
+    let(:user1) { build(:user) }
+    let(:user2) { build(:user) }
+    let(:user3) { build(:user) }
+    let(:project) { build(:project, :public) }
+
+    before do
+      allow(model).to receive(:participant_attrs).and_return([:foo, :bar])
+    end
+
+    it 'returns whether the user is a participant' do
+      allow(instance).to receive(:foo).and_return(user2)
+      allow(instance).to receive(:bar).and_return(user3)
+      allow(instance).to receive(:project).and_return(project)
+
+      expect(instance.participant?(user1)).to be false
+      expect(instance.participant?(user2)).to be true
+      expect(instance.participant?(user3)).to be true
+    end
+
+    it 'caches the list of raw participants' do
+      expect(instance).to receive(:raw_participants).once.and_return([])
+      expect(instance).to receive(:project).twice.and_return(project)
+
+      instance.participant?(user1)
+      instance.participant?(user1)
+    end
+
+    context 'participable is a personal snippet' do
+      let(:model) { PersonalSnippet }
+      let(:instance) { model.new(author: user1) }
+
+      it 'returns whether the user is a participant' do
+        allow(instance).to receive(:foo).and_return(user1)
+        allow(instance).to receive(:bar).and_return(user2)
+
+        expect(instance.participant?(user1)).to be true
+        expect(instance.participant?(user2)).to be false
+        expect(instance.participant?(user3)).to be false
+      end
+    end
   end
 end