Merge branch 'fix_incorrectly_displayed_participants' into 'master'

Do not show participants invisible to the user

See merge request gitlab-org/gitlab!74906

app/graphql/resolvers/users/participants_resolver.rb

+# frozen_string_literal: true
+
+module Resolvers
+  module Users
+    class ParticipantsResolver < BaseResolver
+      type Types::UserType.connection_type, null: true
+
+      def resolve(**args)
+        object.visible_participants(current_user)
+      end
+    end
+  end
+end

app/graphql/types/issue_type.rb

@@ -80,7 +80,8 @@ module Types
           description: 'Relative position of the issue (used for positioning in epic tree and issue boards).'
 
     field :participants, Types::UserType.connection_type, null: true, complexity: 5,
-          description: 'List of participants in the issue.'
+          description: 'List of participants in the issue.',
+          resolver: Resolvers::Users::ParticipantsResolver
     field :emails_disabled, GraphQL::Types::Boolean, null: false,
           method: :project_emails_disabled?,
           description: 'Indicates if a project has email notifications disabled: `true` if email notifications are disabled.'

app/graphql/types/merge_request_type.rb

@@ -148,7 +148,8 @@ module Types
     field :author, Types::UserType, null: true,
           description: 'User who created this merge request.'
     field :participants, Types::UserType.connection_type, null: true, complexity: 15,
-          description: 'Participants in the merge request. This includes the author, assignees, reviewers, and users mentioned in notes.'
+          description: 'Participants in the merge request. This includes the author, assignees, reviewers, and users mentioned in notes.',
+          resolver: Resolvers::Users::ParticipantsResolver
     field :subscribed, GraphQL::Types::Boolean, method: :subscribed?, null: false, complexity: 5,
           description: 'Indicates if the currently logged in user is subscribed to this merge request.'
     field :labels, Types::LabelType.connection_type, null: true, complexity: 5,

app/models/concerns/participable.rb

@@ -60,6 +60,15 @@ module Participable
     filtered_participants_hash[user]
   end
 
+  # Returns only participants visible for the user
+  #
+  # Returns an Array of User instances.
+  def visible_participants(user)
+    return participants(user) unless Feature.enabled?(:verify_participants_access, project, default_enabled: :yaml)
+
+    filter_by_ability(raw_participants(user, verify_access: true))
+  end
+
   # Checks if the user is a participant in a discussion.
   #
   # This method processes attributes of objects in breadth-first order.
@@ -84,8 +93,7 @@ module Participable
     end
   end
 
-  def raw_participants(current_user = nil)
-    current_user ||= author
+  def raw_participants(current_user = nil, verify_access: false)
     ext = Gitlab::ReferenceExtractor.new(project, current_user)
     participants = Set.new
     process = [self]
@@ -97,6 +105,8 @@ module Participable
       when User
         participants << source
       when Participable
+        next unless !verify_access || source_visible_to_user?(source, current_user)
+
         source.class.participant_attrs.each do |attr|
           if attr.respond_to?(:call)
             source.instance_exec(current_user, ext, &attr)
@@ -116,6 +126,10 @@ module Participable
     participants.merge(ext.users)
   end
 
+  def source_visible_to_user?(source, user)
+    Ability.allowed?(user, "read_#{source.model_name.element}".to_sym, source)
+  end
+
   def filter_by_ability(participants)
     case self
     when PersonalSnippet

config/feature_flags/development/verify_participants_access.yml

+---
+name: verify_participants_access
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/74906
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/347407
+milestone: '14.6'
+type: development
+group: group::source code
+default_enabled: false

ee/app/graphql/types/epic_type.rb

@@ -124,7 +124,8 @@ module Types
 
     field :participants, Types::UserType.connection_type, null: true,
           description: 'List of participants for the epic.',
-          complexity: 5
+          complexity: 5,
+          resolver: Resolvers::Users::ParticipantsResolver
 
     field :subscribed, GraphQL::Types::Boolean,
           method: :subscribed?,

lib/api/issues.rb

@@ -472,7 +472,7 @@ module API
       end
       get ':id/issues/:issue_iid/participants' do
         issue = find_project_issue(params[:issue_iid])
-        participants = ::Kaminari.paginate_array(issue.participants)
+        participants = ::Kaminari.paginate_array(issue.visible_participants(current_user))
 
         present paginate(participants), with: Entities::UserBasic, current_user: current_user, project: user_project
       end

lib/api/merge_requests.rb

@@ -282,7 +282,7 @@ module API
       get ':id/merge_requests/:merge_request_iid/participants', feature_category: :code_review, urgency: :low do
         merge_request = find_merge_request_with_access(params[:merge_request_iid])
 
-        participants = ::Kaminari.paginate_array(merge_request.participants)
+        participants = ::Kaminari.paginate_array(merge_request.visible_participants(current_user))
 
         present paginate(participants), with: Entities::UserBasic
       end

spec/graphql/resolvers/users/participants_resolver_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Resolvers::Users::ParticipantsResolver do
+  include GraphqlHelpers
+
+  describe '#resolve' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:guest) { create(:user) }
+    let_it_be(:project) { create(:project, :public) }
+    let_it_be(:issue) { create(:issue, project: project) }
+    let_it_be(:note) do
+      create(
+        :note,
+        :confidential,
+        project: project,
+        noteable: issue,
+        author: create(:user)
+      )
+    end
+
+    subject(:resolved_items) { resolve(described_class, args: {}, ctx: { current_user: current_user }, obj: issue)&.items }
+
+    before do
+      project.add_guest(guest)
+      project.add_developer(user)
+    end
+
+    context 'when current user is not set' do
+      let(:current_user) { nil }
+
+      it 'returns only publicly visible participants for this user' do
+        is_expected.to match_array([issue.author])
+      end
+    end
+
+    context 'when current user does not have enough permissions' do
+      let(:current_user) { guest }
+
+      it 'returns only publicly visible participants for this user' do
+        is_expected.to match_array([issue.author])
+      end
+    end
+
+    context 'when current user has access to confidential notes' do
+      let(:current_user) { user }
+
+      it 'returns all participants for this user' do
+        is_expected.to match_array([issue.author, note.author])
+      end
+    end
+  end
+end

spec/models/concerns/participable_spec.rb

@@ -51,7 +51,9 @@ RSpec.describe Participable do
     end
 
     it 'supports attributes returning another Participable' do
-      other_model = Class.new { include Participable }
+      other_model = Class.new do
+        include Participable
+      end
 
       other_model.participant(:bar)
       model.participant(:foo)
@@ -115,6 +117,76 @@ RSpec.describe Participable do
     end
   end
 
+  describe '#visible_participants' do
+    before do
+      allow(Ability).to receive(:allowed?).and_call_original
+      allow(Ability).to receive(:allowed?).with(anything, :read_class, anything) { readable }
+    end
+
+    let(:readable) { true }
+
+    it 'returns the list of participants' do
+      model.participant(:foo)
+      model.participant(:bar)
+
+      user1 = build(:user)
+      user2 = build(:user)
+      user3 = build(:user)
+      project = build(:project, :public)
+      instance = model.new
+
+      allow(instance).to receive_message_chain(:model_name, :element) { 'class' }
+      expect(instance).to receive(:foo).and_return(user2)
+      expect(instance).to receive(:bar).and_return(user3)
+      expect(instance).to receive(:project).thrice.and_return(project)
+
+      participants = instance.visible_participants(user1)
+
+      expect(participants).to include(user2)
+      expect(participants).to include(user3)
+    end
+
+    context 'when Participable is not readable by the user' do
+      let(:readable) { false }
+
+      it 'does not return unavailable participants' do
+        model.participant(:bar)
+
+        instance = model.new
+        user1 = build(:user)
+        user2 = build(:user)
+        project = build(:project, :public)
+
+        allow(instance).to receive_message_chain(:model_name, :element) { 'class' }
+        allow(instance).to receive(:bar).and_return(user2)
+        expect(instance).to receive(:project).thrice.and_return(project)
+
+        expect(instance.visible_participants(user1)).to be_empty
+      end
+
+      context 'when feature flag is disabled' do
+        before do
+          stub_feature_flags(verify_participants_access: false)
+        end
+
+        it 'returns unavailable participants' do
+          model.participant(:bar)
+
+          instance = model.new
+          user1 = build(:user)
+          user2 = build(:user)
+          project = build(:project, :public)
+
+          allow(instance).to receive_message_chain(:model_name, :element) { 'class' }
+          allow(instance).to receive(:bar).and_return(user2)
+          expect(instance).to receive(:project).thrice.and_return(project)
+
+          expect(instance.visible_participants(user1)).to match_array([user2])
+        end
+      end
+    end
+  end
+
   describe '#participant?' do
     let(:instance) { model.new }
 

spec/requests/api/issues/get_project_issues_spec.rb

@@ -873,7 +873,7 @@ RSpec.describe API::Issues do
     end
 
     it 'returns 404 if the issue is confidential' do
-      post api("/projects/#{project.id}/issues/#{confidential_issue.iid}/participants", non_member)
+      get api("/projects/#{project.id}/issues/#{confidential_issue.iid}/participants", non_member)
 
       expect(response).to have_gitlab_http_status(:not_found)
     end

spec/support/shared_examples/requests/api/issuable_participants_examples.rb

@@ -28,4 +28,34 @@ RSpec.shared_examples 'issuable participants endpoint' do
 
     expect(response).to have_gitlab_http_status(:not_found)
   end
+
+  context 'with a confidential note' do
+    let!(:note) do
+      create(
+        :note,
+        :confidential,
+        project: project,
+        noteable: entity,
+        author: create(:user)
+      )
+    end
+
+    it 'returns a full list of participants' do
+      get api("/projects/#{project.id}/#{area}/#{entity.iid}/participants", user)
+
+      expect(response).to have_gitlab_http_status(:ok)
+      participant_ids = json_response.map { |el| el['id'] }
+      expect(participant_ids).to match_array([entity.author_id, note.author_id])
+    end
+
+    context 'when user cannot see a confidential note' do
+      it 'returns a limited list of participants' do
+        get api("/projects/#{project.id}/#{area}/#{entity.iid}/participants", create(:user))
+
+        expect(response).to have_gitlab_http_status(:ok)
+        participant_ids = json_response.map { |el| el['id'] }
+        expect(participant_ids).to match_array([entity.author_id])
+      end
+    end
+  end
 end