Merge branch 'fix/private-references' into 'master'

CHANGELOG

@@ -42,6 +42,7 @@ v 8.4.0 (unreleased)
   - Ajax filter by message for commits page
   - API: Add support for deleting a tag via the API (Robert Schilling)
   - Allow subsequent validations in CI Linter
+  - Show referenced MRs & Issues only when the current viewer can access them
   - Fix Encoding::CompatibilityError bug when markdown content has some complex URL (Jason Lee)
 
 v 8.3.4

app/controllers/projects/issues_controller.rb

@@ -61,7 +61,7 @@ class Projects::IssuesController < Projects::ApplicationController
     @note = @project.notes.new(noteable: @issue)
     @notes = @issue.notes.nonawards.with_associations.fresh
     @noteable = @issue
-    @merge_requests = @issue.referenced_merge_requests
+    @merge_requests = @issue.referenced_merge_requests(current_user)
 
     respond_with(@issue)
   end

app/models/issue.rb

@@ -85,10 +85,10 @@ class Issue < ActiveRecord::Base
     reference
   end
 
-  def referenced_merge_requests
+  def referenced_merge_requests(current_user = nil)
     Gitlab::ReferenceExtractor.lazily do
       [self, *notes].flat_map do |note|
-        note.all_references.merge_requests
+        note.all_references(current_user).merge_requests
       end
     end.sort_by(&:iid)
   end

app/models/note.rb

@@ -358,6 +358,10 @@ class Note < ActiveRecord::Base
     !system? && !is_award
   end
 
+  def cross_reference_not_visible_for?(user)
+    cross_reference? && referenced_mentionables(user).empty?
+  end
+
   # Checks if note is an award added as a comment
   #
   # If note is an award, this method sets is_award to true

app/views/projects/notes/_notes.html.haml

@@ -2,10 +2,14 @@
   - @discussions.each do |discussion_notes|
     - note = discussion_notes.first
     - if note_for_main_target?(note)
+      - next if note.cross_reference_not_visible_for?(current_user)
+
       = render discussion_notes
     - else
       = render 'projects/notes/discussion', discussion_notes: discussion_notes
 - else
   - @notes.each do |note|
     - next unless note.author
+    - next if note.cross_reference_not_visible_for?(current_user)
+
     = render note

features/project/issues/references.feature

+@project_issues
+Feature: Project Issues References
+  Background:
+    Given I sign in as "John Doe"
+    And public project "Community"
+    And "John Doe" owns public project "Community"
+    And project "Community" has "Community issue" open issue
+    And I logout
+    And I sign in as "Mary Jane"
+    And private project "Enterprise"
+    And "Mary Jane" owns private project "Enterprise"
+    And project "Enterprise" has "Enterprise issue" open issue
+    And project "Enterprise" has "Enterprise fix" open merge request
+    And I visit issue page "Enterprise issue"
+    And I leave a comment referencing issue "Community issue"
+    And I visit merge request page "Enterprise fix"
+    And I leave a comment referencing issue "Community issue"
+    And I logout
+
+  @javascript
+  Scenario: Viewing the public issue as a "John Doe"
+    Given I sign in as "John Doe"
+    When I visit issue page "Community issue"
+    Then I should not see any related merge requests
+    And I should see no notes at all
+
+  @javascript
+  Scenario: Viewing the public issue as "Mary Jane"
+    Given I sign in as "Mary Jane"
+    When I visit issue page "Community issue"
+    Then I should see the "Enterprise fix" related merge request
+    And I should see a note linking to "Enterprise fix" merge request
+    And I should see a note linking to "Enterprise issue" issue

features/project/merge_requests/references.feature

+@project_merge_requests
+Feature: Project Merge Requests References
+  Background:
+    Given I sign in as "John Doe"
+    And public project "Community"
+    And "John Doe" owns public project "Community"
+    And project "Community" has "Community fix" open merge request
+    And I logout
+    And I sign in as "Mary Jane"
+    And private project "Enterprise"
+    And "Mary Jane" owns private project "Enterprise"
+    And project "Enterprise" has "Enterprise issue" open issue
+    And project "Enterprise" has "Enterprise fix" open merge request
+    And I visit issue page "Enterprise issue"
+    And I leave a comment referencing issue "Community fix"
+    And I visit merge request page "Enterprise fix"
+    And I leave a comment referencing issue "Community fix"
+    And I logout
+
+  @javascript
+  Scenario: Viewing the public issue as a "John Doe"
+    Given I sign in as "John Doe"
+    When I visit issue page "Community fix"
+    Then I should see no notes at all
+
+  @javascript
+  Scenario: Viewing the public issue as "Mary Jane"
+    Given I sign in as "Mary Jane"
+    When I visit issue page "Community fix"
+    And I should see a note linking to "Enterprise fix" merge request
+    And I should see a note linking to "Enterprise issue" issue

features/steps/project/issues/references.rb

+class Spinach::Features::ProjectIssuesReferences < Spinach::FeatureSteps
+  include SharedAuthentication
+  include SharedIssuable
+  include SharedNote
+  include SharedProject
+  include SharedUser
+end

features/steps/project/merge_requests/references.rb

+class Spinach::Features::ProjectMergeRequestsReferences < Spinach::FeatureSteps
+  include SharedAuthentication
+  include SharedIssuable
+  include SharedNote
+  include SharedProject
+  include SharedUser
+end

features/steps/shared/issuable.rb

@@ -5,6 +5,99 @@ module SharedIssuable
     find(:css, '.issuable-edit').click
   end
 
+  step 'project "Community" has "Community issue" open issue' do
+    create_issuable_for_project(
+      project_name: 'Community',
+      title: 'Community issue'
+    )
+  end
+
+  step 'project "Community" has "Community fix" open merge request' do
+    create_issuable_for_project(
+      project_name: 'Community',
+      type: :merge_request,
+      title: 'Community fix'
+    )
+  end
+
+  step 'project "Enterprise" has "Enterprise issue" open issue' do
+    create_issuable_for_project(
+      project_name: 'Enterprise',
+      title: 'Enterprise issue'
+    )
+  end
+
+  step 'project "Enterprise" has "Enterprise fix" open merge request' do
+    create_issuable_for_project(
+      project_name: 'Enterprise',
+      type: :merge_request,
+      title: 'Enterprise fix'
+    )
+  end
+
+  step 'I leave a comment referencing issue "Community issue"' do
+    leave_reference_comment(
+      issuable: Issue.find_by(title: 'Community issue'),
+      from_project_name: 'Enterprise'
+    )
+  end
+
+  step 'I leave a comment referencing issue "Community fix"' do
+    leave_reference_comment(
+      issuable: MergeRequest.find_by(title: 'Community fix'),
+      from_project_name: 'Enterprise'
+    )
+  end
+
+  step 'I visit issue page "Enterprise issue"' do
+    issue = Issue.find_by(title: 'Enterprise issue')
+    visit namespace_project_issue_path(issue.project.namespace, issue.project, issue)
+  end
+
+  step 'I visit merge request page "Enterprise fix"' do
+    mr = MergeRequest.find_by(title: 'Enterprise fix')
+    visit namespace_project_merge_request_path(mr.target_project.namespace, mr.target_project, mr)
+  end
+
+  step 'I visit issue page "Community issue"' do
+    issue = Issue.find_by(title: 'Community issue')
+    visit namespace_project_issue_path(issue.project.namespace, issue.project, issue)
+  end
+
+  step 'I visit issue page "Community fix"' do
+    mr = MergeRequest.find_by(title: 'Community fix')
+    visit namespace_project_merge_request_path(mr.target_project.namespace, mr.target_project, mr)
+  end
+
+  step 'I should not see any related merge requests' do
+    page.within '.issue-details' do
+      expect(page).not_to have_content('.merge-requests')
+    end
+  end
+
+  step 'I should see the "Enterprise fix" related merge request' do
+    page.within '.merge-requests' do
+      expect(page).to have_content('1 Related Merge Request')
+      expect(page).to have_content('Enterprise fix')
+    end
+  end
+
+  step 'I should see a note linking to "Enterprise fix" merge request' do
+    visible_note(
+      issuable: MergeRequest.find_by(title: 'Enterprise fix'),
+      from_project_name: 'Community',
+      user_name: 'Mary Jane'
+    )
+  end
+
+  step 'I should see a note linking to "Enterprise issue" issue' do
+    visible_note(
+      issuable: Issue.find_by(title: 'Enterprise issue'),
+      from_project_name: 'Community',
+      user_name: 'Mary Jane'
+    )
+  end
+
   step 'I click link "Edit" for the merge request' do
     edit_issuable
   end
@@ -12,4 +105,45 @@ module SharedIssuable
   step 'I click link "Edit" for the issue' do
     edit_issuable
   end
+
+  def create_issuable_for_project(project_name:, title:, type: :issue)
+    project = Project.find_by(name: project_name)
+
+    attrs = {
+      title: title,
+      author: project.users.first,
+      description: '# Description header'
+    }
+
+    case type
+    when :issue
+      attrs.merge!(project: project)
+    when :merge_request
+      attrs.merge!(
+        source_project: project,
+        target_project: project,
+        source_branch: 'fix',
+        target_branch: 'master'
+      )
+    end
+
+    create(type, attrs)
+  end
+
+  def leave_reference_comment(issuable:, from_project_name:)
+    project = Project.find_by(name: from_project_name)
+
+    page.within('.js-main-target-form') do
+      fill_in 'note[note]', with: "##{issuable.to_reference(project)}"
+      click_button 'Add Comment'
+    end
+  end
+
+  def visible_note(issuable:, from_project_name:, user_name:)
+    project = Project.find_by(name: from_project_name)
+
+    expect(page).to have_content(user_name)
+    expect(page).to have_content("mentioned in #{issuable.class.to_s.titleize.downcase} #{issuable.to_reference(project)}")
+  end
+
 end

features/steps/shared/note.rb

@@ -106,6 +106,10 @@ module SharedNote
     end
   end
 
+  step 'I should see no notes at all' do
+    expect(page).to_not have_css('.note')
+  end
+
   # Markdown
 
   step 'I leave a comment with a header containing "Comment with a header"' do

features/steps/shared/project.rb

@@ -161,24 +161,33 @@ module SharedProject
   end
 
   step '"John Doe" owns private project "Enterprise"' do
-    user = user_exists("John Doe", username: "john_doe")
-    project = Project.find_by(name: "Enterprise")
-    project ||= create(:empty_project, name: "Enterprise", namespace: user.namespace)
-    project.team << [user, :master]
+    user_owns_project(
+      user_name: 'John Doe',
+      project_name: 'Enterprise'
+    )
+  end
+
+  step '"Mary Jane" owns private project "Enterprise"' do
+    user_owns_project(
+      user_name: 'Mary Jane',
+      project_name: 'Enterprise'
+    )
   end
 
   step '"John Doe" owns internal project "Internal"' do
-    user = user_exists("John Doe", username: "john_doe")
-    project = Project.find_by(name: "Internal")
-    project ||= create :empty_project, :internal, name: 'Internal', namespace: user.namespace
-    project.team << [user, :master]
+    user_owns_project(
+      user_name: 'John Doe',
+      project_name: 'Internal',
+      visibility: :internal
+    )
   end
 
   step '"John Doe" owns public project "Community"' do
-    user = user_exists("John Doe", username: "john_doe")
-    project = Project.find_by(name: "Community")
-    project ||= create :empty_project, :public, name: 'Community', namespace: user.namespace
-    project.team << [user, :master]
+    user_owns_project(
+      user_name: 'John Doe',
+      project_name: 'Community',
+      visibility: :public
+    )
   end
 
   step 'public empty project "Empty Public Project"' do
@@ -213,4 +222,12 @@ module SharedProject
       expect(page).to have_content("skipped")
     end
   end
+
+  def user_owns_project(user_name:, project_name:, visibility: :private)
+    user = user_exists(user_name, username: user_name.gsub(/\s/, '').underscore)
+    project = Project.find_by(name: project_name)
+    project ||= create(:empty_project, visibility, name: project_name, namespace: user.namespace)
+    project.team << [user, :master]
+  end
+
 end

lib/api/notes.rb

@@ -20,7 +20,19 @@ module API
         #   GET /projects/:id/snippets/:noteable_id/notes
         get ":id/#{noteables_str}/:#{noteable_id_str}/notes" do
           @noteable = user_project.send(:"#{noteables_str}").find(params[:"#{noteable_id_str}"])
-          present paginate(@noteable.notes), with: Entities::Note
+
+          # We exclude notes that are cross-references and that cannot be viewed
+          # by the current user. By doing this exclusion at this level and not
+          # at the DB query level (which we cannot in that case), the current
+          # page can have less elements than :per_page even if
+          # there's more than one page.
+          notes =
+            # paginate() only works with a relation. This could lead to a
+            # mismatch between the pagination headers info and the actual notes
+            # array returned, but this is really a edge-case.
+            paginate(@noteable.notes).
+            reject { |n| n.cross_reference_not_visible_for?(current_user) }
+          present notes, with: Entities::Note
         end
 
         # Get a single +noteable+ note
@@ -35,8 +47,13 @@ module API
         get ":id/#{noteables_str}/:#{noteable_id_str}/notes/:note_id" do
           @noteable = user_project.send(:"#{noteables_str}").find(params[:"#{noteable_id_str}"])
           @note = @noteable.notes.find(params[:note_id])
+
+          if @note.cross_reference_not_visible_for?(current_user)
+            not_found!("Note")
+          else
             present @note, with: Entities::Note
           end
+        end
 
         # Create a new +noteable+ note
         #

spec/models/note_spec.rb

@@ -178,6 +178,30 @@ describe Note, models: true do
     end
   end
 
+  describe "cross_reference_not_visible_for?" do
+    let(:private_user)    { create(:user) }
+    let(:private_project) { create(:project, namespace: private_user.namespace).tap { |p| p.team << [private_user, :master] } }
+    let(:private_issue)   { create(:issue, project: private_project) }
+
+    let(:ext_proj)  { create(:project, :public) }
+    let(:ext_issue) { create(:issue, project: ext_proj) }
+
+    let(:note) do
+      create :note,
+        noteable: ext_issue, project: ext_proj,
+        note: "mentioned in issue #{private_issue.to_reference(ext_proj)}",
+        system: true
+    end
+
+    it "returns true" do
+      expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_truthy
+    end
+
+    it "returns false" do
+      expect(note.cross_reference_not_visible_for?(private_user)).to be_falsy
+    end
+  end
+
   describe "set_award!" do
     let(:issue) { create :issue }
 

spec/requests/api/notes_spec.rb

@@ -10,6 +10,25 @@ describe API::API, api: true  do
   let!(:issue_note) { create(:note, noteable: issue, project: project, author: user) }
   let!(:merge_request_note) { create(:note, noteable: merge_request, project: project, author: user) }
   let!(:snippet_note) { create(:note, noteable: snippet, project: project, author: user) }
+
+  # For testing the cross-reference of a private issue in a public issue
+  let(:private_user)    { create(:user) }
+  let(:private_project) do
+    create(:project, namespace: private_user.namespace).
+    tap { |p| p.team << [private_user, :master] }
+  end
+  let(:private_issue)    { create(:issue, project: private_project) }
+
+  let(:ext_proj)  { create(:project, :public) }
+  let(:ext_issue) { create(:issue, project: ext_proj) }
+
+  let!(:cross_reference_note) do
+    create :note,
+    noteable: ext_issue, project: ext_proj,
+    note: "mentioned in issue #{private_issue.to_reference(ext_proj)}",
+    system: true
+  end
+
   before { project.team << [user, :reporter] }
 
   describe "GET /projects/:id/noteable/:noteable_id/notes" do
@@ -25,6 +44,24 @@ describe API::API, api: true  do
         get api("/projects/#{project.id}/issues/123/notes", user)
         expect(response.status).to eq(404)
       end
+
+      context "that references a private issue" do
+        it "should return an empty array" do
+          get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes", user)
+          expect(response.status).to eq(200)
+          expect(json_response).to be_an Array
+          expect(json_response).to be_empty
+        end
+
+        context "and current user can view the note" do
+          it "should return an empty array" do
+            get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes", private_user)
+            expect(response.status).to eq(200)
+            expect(json_response).to be_an Array
+            expect(json_response.first['body']).to eq(cross_reference_note.note)
+          end
+        end
+      end
     end
 
     context "when noteable is a Snippet" do
@@ -68,6 +105,21 @@ describe API::API, api: true  do
         get api("/projects/#{project.id}/issues/#{issue.id}/notes/123", user)
         expect(response.status).to eq(404)
       end
+
+      context "that references a private issue" do
+        it "should return a 404 error" do
+          get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes/#{cross_reference_note.id}", user)
+          expect(response.status).to eq(404)
+        end
+
+        context "and current user can view the note" do
+          it "should return an issue note by id" do
+            get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes/#{cross_reference_note.id}", private_user)
+            expect(response.status).to eq(200)
+            expect(json_response['body']).to eq(cross_reference_note.note)
+          end
+        end
+      end
     end
 
     context "when noteable is a Snippet" do