Read about our upcoming Code of Conduct on this issue

This instance will be upgraded to Heptapod 0.28.1 on 2022-01-26 at 16:00 UTC+1 (a few minutes of down time)

Commit 0c0ef7df authored by Douwe Maan's avatar Douwe Maan
Browse files

Merge branch 'confidential-issues-in-private-projects' into 'master'

Allow users to create confidential issues in private projects

Closes #14787

## What does this MR do?

Allow users to create confidential issues in private projects, and exclude access to them to project members with `Guest` role.

## Are there points in the code the reviewer needs to double check?

The query generated by the `User#authorized_projects` method.

## Why was this MR needed?

Community have been requesting this feature.

## What are the relevant issue numbers?

https://gitlab.com/gitlab-org/gitlab-ce/issues/14787

https://gitlab.com/gitlab-org/gitlab-ce/issues/3678

## Screenshots (if relevant)

Not relevant.

## Todo

- [x] Allow users to create confidential issues in private projects
- [x] Project members with `Guest` role should not have access to confidential issues
- [ ] ~~Apply changes in EE + Elasticsearch~~ Will be done in another MR, when this got merged

See merge request !3471
......@@ -76,6 +76,7 @@ v 8.9.0 (unreleased)
- Put project Labels and Milestones pages links under Issues and Merge Requests tabs as subnav
- All classes in the Banzai::ReferenceParser namespace are now instrumented
- Remove deprecated issues_tracker and issues_tracker_id from project model
- Allow users to create confidential issues in private projects
v 8.8.5 (unreleased)
- Ensure branch cleanup regardless of whether the GitHub import process succeeds
......
......@@ -51,7 +51,7 @@ def by_project(current_user, project)
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
......
......@@ -533,7 +533,7 @@ def named_abilities(name)
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)
......
......@@ -51,10 +51,18 @@ def hook_attrs
end
def self.visible_to_user(user)
return where(confidential: false) if user.blank?
return where('issues.confidential IS NULL OR issues.confidential IS 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
......
......@@ -88,22 +88,9 @@ def search(query, as_user: nil)
table = arel_table
pattern = "%#{query}%"
found_notes = joins('LEFT JOIN issues ON issues.id = noteable_id').
where(table[:note].matches(pattern))
if as_user
found_notes.where('
issues.confidential IS NULL
OR issues.confidential IS FALSE
OR (issues.confidential IS TRUE
AND (issues.author_id = :user_id
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))
else
found_notes.where('issues.confidential IS NULL OR issues.confidential IS FALSE')
end
Note.joins('LEFT JOIN issues ON issues.id = noteable_id').
where(table[:note].matches(pattern)).
merge(Issue.visible_to_user(as_user))
end
end
......
......@@ -131,8 +131,14 @@ def master?(user)
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)
......
......@@ -405,8 +405,8 @@ def authorized_groups
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,19 @@ def update_cache_counts
private
def projects_union
Gitlab::SQL::Union.new([personal_projects.select(:id),
groups_projects.select(:id),
projects.select(:id),
groups.joins(:shared_projects).select(:project_id)])
def projects_union(min_access_level = nil)
relations = [personal_projects.select(:id),
groups_projects.select(:id),
projects.select(:id),
groups.joins(:shared_projects).select(:project_id)]
if min_access_level
scope = { access_level: Gitlab::Access.values.select { |access| access >= min_access_level } }
relations = [relations.shift] + relations.map { |relation| relation.where(members: scope) }
end
Gitlab::SQL::Union.new(relations)
end
def ci_projects_union
......
......@@ -35,13 +35,13 @@
.clearfix
.error-alert
- if issuable.is_a?(Issue) && !issuable.project.private?
- if issuable.is_a?(Issue)
.form-group
.col-sm-offset-2.col-sm-10
.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)
......
......@@ -105,6 +105,15 @@ def move_issue
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 @@ def get_issues
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 @@ def get_issues
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)
......
......@@ -69,6 +69,18 @@ def reference_link(data)
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)
......
......@@ -43,6 +43,18 @@
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')
......
......@@ -86,6 +86,22 @@
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')
......
......@@ -5,6 +5,7 @@
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 @@
before do
project.team << [member, :developer]
project.team << [guest, :guest]
end
describe '#closed_items_count' do
......@@ -28,6 +30,10 @@
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 @@
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 @@
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
......
......@@ -50,6 +50,7 @@
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 @@
before do
project.team << [member, :developer]
project.team << [guest, :guest]
end
context 'issue event' do
......@@ -71,6 +73,7 @@
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 @@
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 @@
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 @@
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
......
......@@ -162,16 +162,23 @@
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
......
......@@ -29,6 +29,9 @@
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 @@
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
......
......@@ -5,6 +5,7 @@
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 @@
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 @@
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 @@
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 @@
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'
......
......@@ -146,6 +146,7 @@
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 @@
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))
......
......@@ -132,12 +132,14 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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)
......
......@@ -33,6 +33,18 @@
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)
......
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