Project members with guest role can't access confidential issues

app/finders/snippets_finder.rb

@@ -51,7 +51,7 @@ class SnippetsFinder
     snippets = project.snippets.fresh
 
     if current_user
-      if project.team.member?(current_user.id) || current_user.admin?
+      if project.team.member?(current_user) || current_user.admin?
         snippets
       else
         snippets.public_and_internal

app/models/ability.rb

@@ -533,7 +533,7 @@ class Ability
     def filter_confidential_issues_abilities(user, issue, rules)
       return rules if user.admin? || !issue.confidential?
 
-      unless issue.author == user || issue.assignee == user || issue.project.team.member?(user.id)
+      unless issue.author == user || issue.assignee == user || issue.project.team.member?(user, Gitlab::Access::REPORTER)
         rules.delete(:admin_issue)
         rules.delete(:read_issue)
         rules.delete(:update_issue)

app/models/issue.rb

@@ -54,7 +54,15 @@ class Issue < ActiveRecord::Base
     return where(confidential: false) if user.blank?
     return all if user.admin?
 
-    where('issues.confidential = false OR (issues.confidential = true AND (issues.author_id = :user_id OR issues.assignee_id = :user_id OR issues.project_id IN(:project_ids)))', user_id: user.id, project_ids: user.authorized_projects.select(:id))
+    where('
+      issues.confidential IS NULL
+      OR issues.confidential IS FALSE
+      OR (issues.confidential = TRUE
+        AND (issues.author_id = :user_id
+          OR issues.assignee_id = :user_id
+          OR issues.project_id IN(:project_ids)))',
+      user_id: user.id,
+      project_ids: user.authorized_projects(Gitlab::Access::REPORTER).select(:id))
   end
 
   def self.reference_prefix

app/models/note.rb

@@ -100,7 +100,7 @@ class Note < ActiveRecord::Base
             OR issues.assignee_id = :user_id
             OR issues.project_id IN(:project_ids)))',
           user_id: as_user.id,
-          project_ids: as_user.authorized_projects.select(:id))
+          project_ids: as_user.authorized_projects(Gitlab::Access::REPORTER).select(:id))
       else
         found_notes.where('issues.confidential IS NULL OR issues.confidential IS FALSE')
       end

app/models/project_team.rb

@@ -131,8 +131,14 @@ class ProjectTeam
     max_member_access(user.id) == Gitlab::Access::MASTER
   end
 
-  def member?(user_id)
-    !!find_member(user_id)
+  def member?(user, min_member_access = nil)
+    member = !!find_member(user.id)
+
+    if min_member_access
+      member && max_member_access(user.id) >= min_member_access
+    else
+      member
+    end
   end
 
   def human_max_access(user_id)

app/models/user.rb

@@ -405,8 +405,8 @@ class User < ActiveRecord::Base
   end
 
   # Returns projects user is authorized to access.
-  def authorized_projects
-    Project.where("projects.id IN (#{projects_union.to_sql})")
+  def authorized_projects(min_access_level = nil)
+    Project.where("projects.id IN (#{projects_union(min_access_level).to_sql})")
   end
 
   def viewable_starred_projects
@@ -824,11 +824,22 @@ class User < ActiveRecord::Base
 
   private
 
-  def projects_union
-    Gitlab::SQL::Union.new([personal_projects.select(:id),
+  def projects_union(min_access_level = nil)
+    relations = if min_access_level
+                  scope = { access_level: Gitlab::Access.values.select { |access| access >= min_access_level } }
+
+                  [personal_projects.select(:id),
+                   groups_projects.where(members: scope).select(:id),
+                   projects.where(members: scope).select(:id),
+                   groups.joins(:shared_projects).where(members: scope).select(:project_id)]
+                else
+                  [personal_projects.select(:id),
                    groups_projects.select(:id),
                    projects.select(:id),
-                            groups.joins(:shared_projects).select(:project_id)])
+                   groups.joins(:shared_projects).select(:project_id)]
+                end
+
+    Gitlab::SQL::Union.new(relations)
   end
 
   def ci_projects_union

app/views/shared/issuable/_form.html.haml

@@ -41,7 +41,7 @@
       .checkbox
         = f.label :confidential do
           = f.check_box :confidential
-          This issue is confidential and should only be visible to team members
+          This issue is confidential and should only be visible to team members with at least Reporter access.
 
 - if can?(current_user, :"admin_#{issuable.to_ability_name}", issuable.project)
   - has_due_date = issuable.has_attribute?(:due_date)

spec/controllers/projects/issues_controller_spec.rb

@@ -105,6 +105,15 @@ describe Projects::IssuesController do
         expect(assigns(:issues)).to eq [issue]
       end
 
+      it 'should not list confidential issues for project members with guest role' do
+        sign_in(member)
+        project.team << [member, :guest]
+
+        get_issues
+
+        expect(assigns(:issues)).to eq [issue]
+      end
+
       it 'should list confidential issues for author' do
         sign_in(author)
         get_issues
@@ -148,7 +157,7 @@ describe Projects::IssuesController do
 
     shared_examples_for 'restricted action' do |http_status|
       it 'returns 404 for guests' do
-        sign_out :user
+        sign_out(:user)
         go(id: unescaped_parameter_value.to_param)
 
         expect(response).to have_http_status :not_found
@@ -161,6 +170,14 @@ describe Projects::IssuesController do
         expect(response).to have_http_status :not_found
       end
 
+      it 'returns 404 for project members with guest role' do
+        sign_in(member)
+        project.team << [member, :guest]
+        go(id: unescaped_parameter_value.to_param)
+
+        expect(response).to have_http_status :not_found
+      end
+
       it "returns #{http_status[:success]} for author" do
         sign_in(author)
         go(id: unescaped_parameter_value.to_param)

spec/lib/banzai/filter/redactor_filter_spec.rb

@@ -69,6 +69,18 @@ describe Banzai::Filter::RedactorFilter, lib: true do
         expect(doc.css('a').length).to eq 0
       end
 
+      it 'removes references for project members with guest role' do
+        member = create(:user)
+        project = create(:empty_project, :public)
+        project.team << [member, :guest]
+        issue = create(:issue, :confidential, project: project)
+
+        link = reference_link(project: project.id, issue: issue.id, reference_type: 'issue')
+        doc = filter(link, current_user: member)
+
+        expect(doc.css('a').length).to eq 0
+      end
+
       it 'allows references for author' do
         author = create(:user)
         project = create(:empty_project, :public)

spec/lib/gitlab/project_search_results_spec.rb

@@ -43,6 +43,18 @@ describe Gitlab::ProjectSearchResults, lib: true do
       expect(results.issues_count).to eq 1
     end
 
+    it 'should not list project confidential issues for project members with guest role' do
+      project.team << [member, :guest]
+
+      results = described_class.new(member, project, query)
+      issues = results.objects('issues')
+
+      expect(issues).to include issue
+      expect(issues).not_to include security_issue_1
+      expect(issues).not_to include security_issue_2
+      expect(results.issues_count).to eq 1
+    end
+
     it 'should list project confidential issues for author' do
       results = described_class.new(author, project, query)
       issues = results.objects('issues')

spec/lib/gitlab/search_results_spec.rb

@@ -86,6 +86,22 @@ describe Gitlab::SearchResults do
       expect(results.issues_count).to eq 1
     end
 
+    it 'should not list confidential issues for project members with guest role' do
+      project_1.team << [member, :guest]
+      project_2.team << [member, :guest]
+
+      results = described_class.new(member, limit_projects, query)
+      issues = results.objects('issues')
+
+      expect(issues).to include issue
+      expect(issues).not_to include security_issue_1
+      expect(issues).not_to include security_issue_2
+      expect(issues).not_to include security_issue_3
+      expect(issues).not_to include security_issue_4
+      expect(issues).not_to include security_issue_5
+      expect(results.issues_count).to eq 1
+    end
+
     it 'should list confidential issues for author' do
       results = described_class.new(author, limit_projects, query)
       issues = results.objects('issues')

spec/models/concerns/milestoneish_spec.rb

@@ -5,6 +5,7 @@ describe Milestone, 'Milestoneish' do
   let(:assignee) { create(:user) }
   let(:non_member) { create(:user) }
   let(:member) { create(:user) }
+  let(:guest) { create(:user) }
   let(:admin) { create(:admin) }
   let(:project) { create(:project, :public) }
   let(:milestone) { create(:milestone, project: project) }
@@ -21,6 +22,7 @@ describe Milestone, 'Milestoneish' do
 
   before do
     project.team << [member, :developer]
+    project.team << [guest, :guest]
   end
 
   describe '#closed_items_count' do
@@ -28,6 +30,10 @@ describe Milestone, 'Milestoneish' do
       expect(milestone.closed_items_count(non_member)).to eq 2
     end
 
+    it 'should not count confidential issues for project members with guest role' do
+      expect(milestone.closed_items_count(guest)).to eq 2
+    end
+
     it 'should count confidential issues for author' do
       expect(milestone.closed_items_count(author)).to eq 4
     end
@@ -50,6 +56,10 @@ describe Milestone, 'Milestoneish' do
       expect(milestone.total_items_count(non_member)).to eq 4
     end
 
+    it 'should not count confidential issues for project members with guest role' do
+      expect(milestone.total_items_count(guest)).to eq 4
+    end
+
     it 'should count confidential issues for author' do
       expect(milestone.total_items_count(author)).to eq 7
     end
@@ -85,6 +95,10 @@ describe Milestone, 'Milestoneish' do
       expect(milestone.percent_complete(non_member)).to eq 50
     end
 
+    it 'should not count confidential issues for project members with guest role' do
+      expect(milestone.percent_complete(guest)).to eq 50
+    end
+
     it 'should count confidential issues for author' do
       expect(milestone.percent_complete(author)).to eq 57
     end

spec/models/event_spec.rb

@@ -50,6 +50,7 @@ describe Event, models: true do
     let(:project) { create(:empty_project, :public) }
     let(:non_member) { create(:user) }
     let(:member)  { create(:user) }
+    let(:guest)  { create(:user) }
     let(:author) { create(:author) }
     let(:assignee) { create(:user) }
     let(:admin) { create(:admin) }
@@ -61,6 +62,7 @@ describe Event, models: true do
 
     before do
       project.team << [member, :developer]
+      project.team << [guest, :guest]
     end
 
     context 'issue event' do
@@ -71,6 +73,7 @@ describe Event, models: true do
         it { expect(event.visible_to_user?(author)).to eq true }
         it { expect(event.visible_to_user?(assignee)).to eq true }
         it { expect(event.visible_to_user?(member)).to eq true }
+        it { expect(event.visible_to_user?(guest)).to eq true }
         it { expect(event.visible_to_user?(admin)).to eq true }
       end
 
@@ -81,6 +84,7 @@ describe Event, models: true do
         it { expect(event.visible_to_user?(author)).to eq true }
         it { expect(event.visible_to_user?(assignee)).to eq true }
         it { expect(event.visible_to_user?(member)).to eq true }
+        it { expect(event.visible_to_user?(guest)).to eq false }
         it { expect(event.visible_to_user?(admin)).to eq true }
       end
     end
@@ -93,6 +97,7 @@ describe Event, models: true do
         it { expect(event.visible_to_user?(author)).to eq true }
         it { expect(event.visible_to_user?(assignee)).to eq true }
         it { expect(event.visible_to_user?(member)).to eq true }
+        it { expect(event.visible_to_user?(guest)).to eq true }
         it { expect(event.visible_to_user?(admin)).to eq true }
       end
 
@@ -103,6 +108,7 @@ describe Event, models: true do
         it { expect(event.visible_to_user?(author)).to eq true }
         it { expect(event.visible_to_user?(assignee)).to eq true }
         it { expect(event.visible_to_user?(member)).to eq true }
+        it { expect(event.visible_to_user?(guest)).to eq false }
         it { expect(event.visible_to_user?(admin)).to eq true }
       end
     end

spec/models/note_spec.rb

@@ -162,16 +162,23 @@ describe Note, models: true do
     end
 
     context "confidential issues" do
-      let(:user) { create :user }
-      let(:confidential_issue) { create(:issue, :confidential, author: user) }
-      let(:confidential_note) { create :note, note: "Random", noteable: confidential_issue, project: confidential_issue.project }
+      let(:user) { create(:user) }
+      let(:project) { create(:project) }
+      let(:confidential_issue) { create(:issue, :confidential, project: project, author: user) }
+      let(:confidential_note) { create(:note, note: "Random", noteable: confidential_issue, project: confidential_issue.project) }
 
       it "returns notes with matching content if user can see the issue" do
         expect(described_class.search(confidential_note.note, as_user: user)).to eq([confidential_note])
       end
 
       it "does not return notes with matching content if user can not see the issue" do
-        user = create :user
+        user = create(:user)
+        expect(described_class.search(confidential_note.note, as_user: user)).to be_empty
+      end
+
+      it "does not return notes with matching content for project members with guest role" do
+        user = create(:user)
+        project.team << [user, :guest]
         expect(described_class.search(confidential_note.note, as_user: user)).to be_empty
       end
 

spec/models/project_team_spec.rb

@@ -29,6 +29,9 @@ describe ProjectTeam, models: true do
       it { expect(project.team.master?(nonmember)).to be_falsey }
       it { expect(project.team.member?(nonmember)).to be_falsey }
       it { expect(project.team.member?(guest)).to be_truthy }
+      it { expect(project.team.member?(reporter, Gitlab::Access::REPORTER)).to be_truthy }
+      it { expect(project.team.member?(guest, Gitlab::Access::REPORTER)).to be_falsey }
+      it { expect(project.team.member?(nonmember, Gitlab::Access::GUEST)).to be_falsey }
     end
   end
 
@@ -64,6 +67,9 @@ describe ProjectTeam, models: true do
       it { expect(project.team.master?(nonmember)).to be_falsey }
       it { expect(project.team.member?(nonmember)).to be_falsey }
       it { expect(project.team.member?(guest)).to be_truthy }
+      it { expect(project.team.member?(guest, Gitlab::Access::MASTER)).to be_truthy }
+      it { expect(project.team.member?(reporter, Gitlab::Access::MASTER)).to be_falsey }
+      it { expect(project.team.member?(nonmember, Gitlab::Access::GUEST)).to be_falsey }
     end
   end
 

spec/requests/api/issues_spec.rb

@@ -5,6 +5,7 @@ describe API::API, api: true  do
   let(:user)        { create(:user) }
   let(:user2)       { create(:user) }
   let(:non_member)  { create(:user) }
+  let(:guest)       { create(:user) }
   let(:author)      { create(:author) }
   let(:assignee)    { create(:assignee) }
   let(:admin)       { create(:user, :admin) }
@@ -41,7 +42,10 @@ describe API::API, api: true  do
   end
   let!(:note) { create(:note_on_issue, author: user, project: project, noteable: issue) }
 
-  before { project.team << [user, :reporter] }
+  before do
+    project.team << [user, :reporter]
+    project.team << [guest, :guest]
+  end
 
   describe "GET /issues" do
     context "when unauthenticated" do
@@ -144,6 +148,14 @@ describe API::API, api: true  do
       expect(json_response.first['title']).to eq(issue.title)
     end
 
+    it 'should return project issues without confidential issues for project members with guest role' do
+      get api("#{base_url}/issues", guest)
+      expect(response.status).to eq(200)
+      expect(json_response).to be_an Array
+      expect(json_response.length).to eq(2)
+      expect(json_response.first['title']).to eq(issue.title)
+    end
+
     it 'should return project confidential issues for author' do
       get api("#{base_url}/issues", author)
       expect(response.status).to eq(200)
@@ -278,6 +290,11 @@ describe API::API, api: true  do
         expect(response.status).to eq(404)
       end
 
+      it "should return 404 for project members with guest role" do
+        get api("/projects/#{project.id}/issues/#{confidential_issue.id}", guest)
+        expect(response.status).to eq(404)
+      end
+
       it "should return confidential issue for project members" do
         get api("/projects/#{project.id}/issues/#{confidential_issue.id}", user)
         expect(response.status).to eq(200)
@@ -413,6 +430,12 @@ describe API::API, api: true  do
         expect(response.status).to eq(403)
       end
 
+      it "should return 403 for project members with guest role" do
+        put api("/projects/#{project.id}/issues/#{confidential_issue.id}", guest),
+          title: 'updated title'
+        expect(response.status).to eq(403)
+      end
+
       it "should update a confidential issue for project members" do
         put api("/projects/#{project.id}/issues/#{confidential_issue.id}", user),
           title: 'updated title'

spec/requests/api/milestones_spec.rb

@@ -146,6 +146,7 @@ describe API::API, api: true  do
       let(:milestone) { create(:milestone, project: public_project) }
       let(:issue) { create(:issue, project: public_project) }
       let(:confidential_issue) { create(:issue, confidential: true, project: public_project) }
+
       before do
         public_project.team << [user, :developer]
         milestone.issues << issue << confidential_issue
@@ -160,6 +161,18 @@ describe API::API, api: true  do
         expect(json_response.map { |issue| issue['id'] }).to include(issue.id, confidential_issue.id)
       end
 
+      it 'does not return confidential issues to team members with guest role' do
+        member = create(:user)
+        project.team << [member, :guest]
+
+        get api("/projects/#{public_project.id}/milestones/#{milestone.id}/issues", member)
+
+        expect(response.status).to eq(200)
+        expect(json_response).to be_an Array
+        expect(json_response.size).to eq(1)
+        expect(json_response.map { |issue| issue['id'] }).to include(issue.id)
+      end
+
       it 'does not return confidential issues to regular users' do
         get api("/projects/#{public_project.id}/milestones/#{milestone.id}/issues", create(:user))
 

spec/services/notification_service_spec.rb

@@ -132,12 +132,14 @@ describe NotificationService, services: true do
       let(:assignee) { create(:user) }
       let(:non_member) { create(:user) }
       let(:member) { create(:user) }
+      let(:guest) { create(:user) }
       let(:admin) { create(:admin) }
       let(:confidential_issue) { create(:issue, :confidential, project: project, author: author, assignee: assignee) }
       let(:note) { create(:note_on_issue, noteable: confidential_issue, project: project, note: "#{author.to_reference} #{assignee.to_reference} #{non_member.to_reference} #{member.to_reference} #{admin.to_reference}") }
 
       it 'filters out users that can not read the issue' do
         project.team << [member, :developer]
+        project.team << [guest, :guest]
 
         expect(SentNotification).to receive(:record).with(confidential_issue, any_args).exactly(4).times
 
@@ -146,6 +148,7 @@ describe NotificationService, services: true do
         notification.new_note(note)
 
         should_not_email(non_member)
+        should_not_email(guest)
         should_email(author)
         should_email(assignee)
         should_email(member)
@@ -322,17 +325,20 @@ describe NotificationService, services: true do
         let(:assignee) { create(:user) }
         let(:non_member) { create(:user) }
         let(:member) { create(:user) }
+        let(:guest) { create(:user) }
         let(:admin) { create(:admin) }
         let(:confidential_issue) { create(:issue, :confidential, project: project, title: 'Confidential issue', author: author, assignee: assignee) }
 
         it "emails subscribers of the issue's labels that can read the issue" do
           project.team << [member, :developer]
+          project.team << [guest, :guest]
 
           label = create(:label, issues: [confidential_issue])
           label.toggle_subscription(non_member)
           label.toggle_subscription(author)
           label.toggle_subscription(assignee)
           label.toggle_subscription(member)
+          label.toggle_subscription(guest)
           label.toggle_subscription(admin)
 
           ActionMailer::Base.deliveries.clear
@@ -341,6 +347,7 @@ describe NotificationService, services: true do
 
           should_not_email(non_member)
           should_not_email(author)
+          should_not_email(guest)
           should_email(assignee)
           should_email(member)
           should_email(admin)
@@ -490,6 +497,7 @@ describe NotificationService, services: true do
         let(:assignee) { create(:user) }
         let(:non_member) { create(:user) }
         let(:member) { create(:user) }
+        let(:guest) { create(:user) }
         let(:admin) { create(:admin) }
         let(:confidential_issue) { create(:issue, :confidential, project: project, title: 'Confidential issue', author: author, assignee: assignee) }
         let!(:label_1) { create(:label, issues: [confidential_issue]) }
@@ -497,11 +505,13 @@ describe NotificationService, services: true do
 
         it "emails subscribers of the issue's labels that can read the issue" do
           project.team << [member, :developer]
+          project.team << [guest, :guest]
 
           label_2.toggle_subscription(non_member)
           label_2.toggle_subscription(author)
           label_2.toggle_subscription(assignee)
           label_2.toggle_subscription(member)
+          label_2.toggle_subscription(guest)
           label_2.toggle_subscription(admin)
 
           ActionMailer::Base.deliveries.clear
@@ -509,6 +519,7 @@ describe NotificationService, services: true do
           notification.relabeled_issue(confidential_issue, [label_2], @u_disabled)
 
           should_not_email(non_member)
+          should_not_email(guest)
           should_email(author)
           should_email(assignee)
           should_email(member)

spec/services/projects/autocomplete_service_spec.rb

@@ -33,6 +33,18 @@ describe Projects::AutocompleteService, services: true do
         expect(issues.count).to eq 1
       end
 
+      it 'should not list project confidential issues for project members with guest role' do
+        project.team << [member, :guest]
+
+        autocomplete = described_class.new(project, non_member)
+        issues = autocomplete.issues.map(&:iid)
+
+        expect(issues).to include issue.iid
+        expect(issues).not_to include security_issue_1.iid
+        expect(issues).not_to include security_issue_2.iid
+        expect(issues.count).to eq 1
+      end
+
       it 'should list project confidential issues for author' do
         autocomplete = described_class.new(project, author)
         issues = autocomplete.issues.map(&:iid)

spec/services/todo_service_spec.rb

@@ -5,13 +5,15 @@ describe TodoService, services: true do
   let(:assignee) { create(:user) }
   let(:non_member) { create(:user) }
   let(:member) { create(:user) }
+  let(:guest) { create(:user) }
   let(:admin) { create(:admin) }
   let(:john_doe) { create(:user) }
   let(:project) { create(:project) }
-  let(:mentions) { [author, assignee, john_doe, member, non_member, admin].map(&:to_reference).join(' ') }
+  let(:mentions) { [author, assignee, john_doe, member, guest, non_member, admin].map(&:to_reference).join(' ') }
   let(:service) { described_class.new }
 
   before do
+    project.team << [guest, :guest]
     project.team << [author, :developer]
     project.team << [member, :developer]
     project.team << [john_doe, :developer]
@@ -41,18 +43,20 @@ describe TodoService, services: true do
         service.new_issue(issue, author)
 
         should_create_todo(user: member, target: issue, action: Todo::MENTIONED)
+        should_create_todo(user: guest, target: issue, action: Todo::MENTIONED)
         should_not_create_todo(user: author, target: issue, action: Todo::MENTIONED)
         should_not_create_todo(user: john_doe, target: issue, action: Todo::MENTIONED)
         should_not_create_todo(user: non_member, target: issue, action: Todo::MENTIONED)
       end
 
-      it 'does not create todo for non project members when issue is confidential' do
+      it 'does not create todo if user can not see the issue when issue is confidential' do
         service.new_issue(confidential_issue, john_doe)
 
         should_create_todo(user: assignee, target: confidential_issue, author: john_doe, action: Todo::ASSIGNED)
         should_create_todo(user: author, target: confidential_issue, author: john_doe, action: Todo::MENTIONED)
         should_create_todo(user: member, target: confidential_issue, author: john_doe, action: Todo::MENTIONED)
         should_create_todo(user: admin, target: confidential_issue, author: john_doe, action: Todo::MENTIONED)
+        should_not_create_todo(user: guest, target: confidential_issue, author: john_doe, action: Todo::MENTIONED)
         should_not_create_todo(user: john_doe, target: confidential_issue, author: john_doe, action: Todo::MENTIONED)
       end
 
@@ -81,6 +85,7 @@ describe TodoService, services: true do
         service.update_issue(issue, author)
 
         should_create_todo(user: member, target: issue, action: Todo::MENTIONED)
+        should_create_todo(user: guest, target: issue, action: Todo::MENTIONED)
         should_create_todo(user: john_doe, target: issue, action: Todo::MENTIONED)
         should_not_create_todo(user: author, target: issue, action: Todo::MENTIONED)
         should_not_create_todo(user: non_member, target: issue, action: Todo::MENTIONED)
@@ -92,13 +97,14 @@ describe TodoService, services: true do
         expect { service.update_issue(issue, author) }.not_to change(member.todos, :count)
       end
 
-      it 'does not create todo for non project members when issue is confidential' do
+      it 'does not create todo if user can not see the issue when issue is confidential' do
         service.update_issue(confidential_issue, john_doe)
 
         should_create_todo(user: author, target: confidential_issue, author: john_doe, action: Todo::MENTIONED)
         should_create_todo(user: assignee, target: confidential_issue, author: john_doe, action: Todo::MENTIONED)
         should_create_todo(user: member, target: confidential_issue, author: john_doe, action: Todo::MENTIONED)
         should_create_todo(user: admin, target: confidential_issue, author: john_doe, action: Todo::MENTIONED)
+        should_not_create_todo(user: guest, target: confidential_issue, author: john_doe, action: Todo::MENTIONED)
         should_not_create_todo(user: john_doe, target: confidential_issue, author: john_doe, action: Todo::MENTIONED)
       end
 
@@ -192,18 +198,20 @@ describe TodoService, services: true do
         service.new_note(note, john_doe)
 
         should_create_todo(user: member, target: issue, author: john_doe, action: Todo::MENTIONED, note: note)
+        should_create_todo(user: guest, target: issue, author: john_doe, action: Todo::MENTIONED, note: note)
         should_create_todo(user: author, target: issue, author: john_doe, action: Todo::MENTIONED, note: note)
         should_not_create_todo(user: john_doe, target: issue, author: john_doe, action: Todo::MENTIONED, note: note)
         should_not_create_todo(user: non_member, target: issue, author: john_doe, action: Todo::MENTIONED, note: note)
       end
 
-      it 'does not create todo for non project members when leaving a note on a confidential issue' do
+      it 'does not create todo if user can not see the issue when leaving a note on a confidential issue' do
         service.new_note(note_on_confidential_issue, john_doe)
 
         should_create_todo(user: author, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue)
         should_create_todo(user: assignee, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue)
         should_create_todo(user: member, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue)
         should_create_todo(user: admin, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue)
+        should_not_create_todo(user: guest, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue)
         should_not_create_todo(user: john_doe, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue)
       end
 
@@ -245,6 +253,7 @@ describe TodoService, services: true do
         service.new_merge_request(mr_assigned, author)
 
         should_create_todo(user: member, target: mr_assigned, action: Todo::MENTIONED)
+        should_create_todo(user: guest, target: mr_assigned, action: Todo::MENTIONED)
         should_not_create_todo(user: author, target: mr_assigned, action: Todo::MENTIONED)
         should_not_create_todo(user: john_doe, target: mr_assigned, action: Todo::MENTIONED)
         should_not_create_todo(user: non_member, target: mr_assigned, action: Todo::MENTIONED)
@@ -256,6 +265,7 @@ describe TodoService, services: true do
         service.update_merge_request(mr_assigned, author)
 
         should_create_todo(user: member, target: mr_assigned, action: Todo::MENTIONED)
+        should_create_todo(user: guest, target: mr_assigned, action: Todo::MENTIONED)
         should_create_todo(user: john_doe, target: mr_assigned, action: Todo::MENTIONED)
         should_not_create_todo(user: author, target: mr_assigned, action: Todo::MENTIONED)
         should_not_create_todo(user: non_member, target: mr_assigned, action: Todo::MENTIONED)