Commit f10f5d03 authored by Vasilii Iakliushin's avatar Vasilii Iakliushin

Fix participants list references

Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/277376

**Problem**

We expose participants that current user cannot see, because we don't
provide the current user as an argument to participants method in
GraphQL. When user is missing, then we use author of the issuable
permissions to fetch participants.

**Solution**

* Add GraphQL resolver to participants field
* Return only visible participants for issues/MRs API
* Verify that participable object is readable by user
* Remove fallback to author when current user is not set
parent a4bebd81
# 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
......@@ -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.'
......
......@@ -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,
......
......@@ -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
......
---
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
......@@ -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?,
......
......@@ -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
......
......@@ -282,7 +282,7 @@ module API
get ':id/merge_requests/:merge_request_iid/participants', feature_category: :code_review 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
......
# 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
......@@ -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 }
......
......@@ -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
......
......@@ -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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment