Commit 9f8c38bd authored by Douwe Maan's avatar Douwe Maan

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

Show referenced MRs & Issues only when the current viewer can access them

This addresses both issues identified in #6066.

## The private MR by user `remy2` with a note referencing to a public issue

![Screen_Shot_2016-01-12_at_16.45.02](/uploads/c245ec2c1fdea1f9ba05183c24e142d9/Screen_Shot_2016-01-12_at_16.45.02.png)

---

## The public issue viewed by user `remy` **who doesn't have access to `remy2/private-project`** before the fix

![Screen_Shot_2016-01-12_at_18.14.50](/uploads/8db5580e803f5bddd6cb935233c579a0/Screen_Shot_2016-01-12_at_18.14.50.png)

---

## The public issue viewed by user `remy` **who doesn't have access to `remy2/private-project`** with the fix

![Screen_Shot_2016-01-13_at_12.02.32](/uploads/cb199f7b78191fba486a11412412e307/Screen_Shot_2016-01-13_at_12.02.32.png)

---

## The public issue viewed by user `remy2` with the fix (no change)

![Screen_Shot_2016-01-13_at_11.54.06](/uploads/ddece590d69f597a95559beddcd36660/Screen_Shot_2016-01-13_at_11.54.06.png)


See merge request !2405
parents 54734fa6 e918493f
...@@ -42,6 +42,7 @@ v 8.4.0 (unreleased) ...@@ -42,6 +42,7 @@ v 8.4.0 (unreleased)
- Ajax filter by message for commits page - Ajax filter by message for commits page
- API: Add support for deleting a tag via the API (Robert Schilling) - API: Add support for deleting a tag via the API (Robert Schilling)
- Allow subsequent validations in CI Linter - 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) - Fix Encoding::CompatibilityError bug when markdown content has some complex URL (Jason Lee)
- Allow broadcast messages to be edited - Allow broadcast messages to be edited
......
...@@ -61,7 +61,7 @@ class Projects::IssuesController < Projects::ApplicationController ...@@ -61,7 +61,7 @@ class Projects::IssuesController < Projects::ApplicationController
@note = @project.notes.new(noteable: @issue) @note = @project.notes.new(noteable: @issue)
@notes = @issue.notes.nonawards.with_associations.fresh @notes = @issue.notes.nonawards.with_associations.fresh
@noteable = @issue @noteable = @issue
@merge_requests = @issue.referenced_merge_requests @merge_requests = @issue.referenced_merge_requests(current_user)
respond_with(@issue) respond_with(@issue)
end end
......
...@@ -85,10 +85,10 @@ class Issue < ActiveRecord::Base ...@@ -85,10 +85,10 @@ class Issue < ActiveRecord::Base
reference reference
end end
def referenced_merge_requests def referenced_merge_requests(current_user = nil)
Gitlab::ReferenceExtractor.lazily do Gitlab::ReferenceExtractor.lazily do
[self, *notes].flat_map do |note| [self, *notes].flat_map do |note|
note.all_references.merge_requests note.all_references(current_user).merge_requests
end end
end.sort_by(&:iid) end.sort_by(&:iid)
end end
......
...@@ -358,6 +358,10 @@ class Note < ActiveRecord::Base ...@@ -358,6 +358,10 @@ class Note < ActiveRecord::Base
!system? && !is_award !system? && !is_award
end 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 # Checks if note is an award added as a comment
# #
# If note is an award, this method sets is_award to true # If note is an award, this method sets is_award to true
......
...@@ -2,10 +2,14 @@ ...@@ -2,10 +2,14 @@
- @discussions.each do |discussion_notes| - @discussions.each do |discussion_notes|
- note = discussion_notes.first - note = discussion_notes.first
- if note_for_main_target?(note) - if note_for_main_target?(note)
- next if note.cross_reference_not_visible_for?(current_user)
= render discussion_notes = render discussion_notes
- else - else
= render 'projects/notes/discussion', discussion_notes: discussion_notes = render 'projects/notes/discussion', discussion_notes: discussion_notes
- else - else
- @notes.each do |note| - @notes.each do |note|
- next unless note.author - next unless note.author
- next if note.cross_reference_not_visible_for?(current_user)
= render note = render note
@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
@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
class Spinach::Features::ProjectIssuesReferences < Spinach::FeatureSteps
include SharedAuthentication
include SharedIssuable
include SharedNote
include SharedProject
include SharedUser
end
class Spinach::Features::ProjectMergeRequestsReferences < Spinach::FeatureSteps
include SharedAuthentication
include SharedIssuable
include SharedNote
include SharedProject
include SharedUser
end
...@@ -5,6 +5,99 @@ module SharedIssuable ...@@ -5,6 +5,99 @@ module SharedIssuable
find(:css, '.issuable-edit').click find(:css, '.issuable-edit').click
end 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 step 'I click link "Edit" for the merge request' do
edit_issuable edit_issuable
end end
...@@ -12,4 +105,45 @@ module SharedIssuable ...@@ -12,4 +105,45 @@ module SharedIssuable
step 'I click link "Edit" for the issue' do step 'I click link "Edit" for the issue' do
edit_issuable edit_issuable
end 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 end
...@@ -106,6 +106,10 @@ module SharedNote ...@@ -106,6 +106,10 @@ module SharedNote
end end
end end
step 'I should see no notes at all' do
expect(page).to_not have_css('.note')
end
# Markdown # Markdown
step 'I leave a comment with a header containing "Comment with a header"' do step 'I leave a comment with a header containing "Comment with a header"' do
......
...@@ -161,24 +161,33 @@ module SharedProject ...@@ -161,24 +161,33 @@ module SharedProject
end end
step '"John Doe" owns private project "Enterprise"' do step '"John Doe" owns private project "Enterprise"' do
user = user_exists("John Doe", username: "john_doe") user_owns_project(
project = Project.find_by(name: "Enterprise") user_name: 'John Doe',
project ||= create(:empty_project, name: "Enterprise", namespace: user.namespace) project_name: 'Enterprise'
project.team << [user, :master] )
end
step '"Mary Jane" owns private project "Enterprise"' do
user_owns_project(
user_name: 'Mary Jane',
project_name: 'Enterprise'
)
end end
step '"John Doe" owns internal project "Internal"' do step '"John Doe" owns internal project "Internal"' do
user = user_exists("John Doe", username: "john_doe") user_owns_project(
project = Project.find_by(name: "Internal") user_name: 'John Doe',
project ||= create :empty_project, :internal, name: 'Internal', namespace: user.namespace project_name: 'Internal',
project.team << [user, :master] visibility: :internal
)
end end
step '"John Doe" owns public project "Community"' do step '"John Doe" owns public project "Community"' do
user = user_exists("John Doe", username: "john_doe") user_owns_project(
project = Project.find_by(name: "Community") user_name: 'John Doe',
project ||= create :empty_project, :public, name: 'Community', namespace: user.namespace project_name: 'Community',
project.team << [user, :master] visibility: :public
)
end end
step 'public empty project "Empty Public Project"' do step 'public empty project "Empty Public Project"' do
...@@ -213,4 +222,12 @@ module SharedProject ...@@ -213,4 +222,12 @@ module SharedProject
expect(page).to have_content("skipped") expect(page).to have_content("skipped")
end end
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 end
...@@ -20,7 +20,19 @@ module API ...@@ -20,7 +20,19 @@ module API
# GET /projects/:id/snippets/:noteable_id/notes # GET /projects/:id/snippets/:noteable_id/notes
get ":id/#{noteables_str}/:#{noteable_id_str}/notes" do get ":id/#{noteables_str}/:#{noteable_id_str}/notes" do
@noteable = user_project.send(:"#{noteables_str}").find(params[:"#{noteable_id_str}"]) @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 end
# Get a single +noteable+ note # Get a single +noteable+ note
...@@ -35,7 +47,12 @@ module API ...@@ -35,7 +47,12 @@ module API
get ":id/#{noteables_str}/:#{noteable_id_str}/notes/:note_id" do get ":id/#{noteables_str}/:#{noteable_id_str}/notes/:note_id" do
@noteable = user_project.send(:"#{noteables_str}").find(params[:"#{noteable_id_str}"]) @noteable = user_project.send(:"#{noteables_str}").find(params[:"#{noteable_id_str}"])
@note = @noteable.notes.find(params[:note_id]) @note = @noteable.notes.find(params[:note_id])
present @note, with: Entities::Note
if @note.cross_reference_not_visible_for?(current_user)
not_found!("Note")
else
present @note, with: Entities::Note
end
end end
# Create a new +noteable+ note # Create a new +noteable+ note
......
...@@ -178,6 +178,30 @@ describe Note, models: true do ...@@ -178,6 +178,30 @@ describe Note, models: true do
end end
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 describe "set_award!" do
let(:issue) { create :issue } let(:issue) { create :issue }
......
...@@ -10,6 +10,25 @@ describe API::API, api: true do ...@@ -10,6 +10,25 @@ describe API::API, api: true do
let!(:issue_note) { create(:note, noteable: issue, project: project, author: user) } 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!(:merge_request_note) { create(:note, noteable: merge_request, project: project, author: user) }
let!(:snippet_note) { create(:note, noteable: snippet, 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] } before { project.team << [user, :reporter] }
describe "GET /projects/:id/noteable/:noteable_id/notes" do describe "GET /projects/:id/noteable/:noteable_id/notes" do
...@@ -25,6 +44,24 @@ describe API::API, api: true do ...@@ -25,6 +44,24 @@ describe API::API, api: true do
get api("/projects/#{project.id}/issues/123/notes", user) get api("/projects/#{project.id}/issues/123/notes", user)
expect(response.status).to eq(404) expect(response.status).to eq(404)
end 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 end
context "when noteable is a Snippet" do context "when noteable is a Snippet" do
...@@ -68,6 +105,21 @@ describe API::API, api: true do ...@@ -68,6 +105,21 @@ describe API::API, api: true do
get api("/projects/#{project.id}/issues/#{issue.id}/notes/123", user) get api("/projects/#{project.id}/issues/#{issue.id}/notes/123", user)
expect(response.status).to eq(404) expect(response.status).to eq(404)
end 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 end
context "when noteable is a Snippet" do context "when noteable is a Snippet" do
......
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