diff options
Diffstat (limited to 'app/services/issuable/discussions_list_service.rb')
| -rw-r--r-- | app/services/issuable/discussions_list_service.rb | 16 |
1 files changed, 7 insertions, 9 deletions
diff --git a/app/services/issuable/discussions_list_service.rb b/app/services/issuable/discussions_list_service.rb index 7aa0363af01..1e5e37e4e1b 100644 --- a/app/services/issuable/discussions_list_service.rb +++ b/app/services/issuable/discussions_list_service.rb @@ -16,7 +16,7 @@ module Issuable end def execute - return Note.none unless can_read_issuable? + return Note.none unless can_read_issuable_notes? notes = NotesFinder.new(current_user, params.merge({ target: issuable, project: issuable.project })) .execute.with_web_entity_associations.inc_relations_for_view.fresh @@ -39,12 +39,9 @@ module Issuable notes = prepare_notes_for_rendering(notes) - # TODO: optimize this permission check. - # Given this loads notes on a single issuable and current permission system, we should not have to check - # permission on every single note. We should be able to check permission on the given issuable or its container, - # which should result in just one permission check. Perhaps that should also either be passed to NotesFinder or - # should be done in NotesFinder, which would decide right away if it would need to return no notes - # or if it should just filter out internal notes. + # we need to check the permission on every note, because some system notes for instance can have references to + # resources that some user do not have read access, so those notes are filtered out from the list of notes. + # see Note#all_referenced_mentionables_allowed? notes = notes.select { |n| n.readable_by?(current_user) } Discussion.build_collection(notes, issuable) @@ -61,10 +58,11 @@ module Issuable end end - def can_read_issuable? + def can_read_issuable_notes? return Ability.allowed?(current_user, :read_security_resource, issuable) if issuable.is_a?(Vulnerability) - Ability.allowed?(current_user, :"read_#{issuable.to_ability_name}", issuable) + Ability.allowed?(current_user, :"read_#{issuable.to_ability_name}", issuable) && + Ability.allowed?(current_user, :read_note, issuable) end end end |