From b4004488f76d7360acd2f38277d617447c76b888 Mon Sep 17 00:00:00 2001
From: Valery Sizov <valery@gitlab.com>
Date: Tue, 4 Oct 2016 15:52:08 +0300
Subject: [PATCH] Make guests unable to view MRs
---
CHANGELOG | 1 +
app/models/event.rb | 8 ++-
app/policies/project_policy.rb | 3 +-
app/services/notification_service.rb | 6 +-
app/services/todo_service.rb | 6 +-
doc/user/permissions.md | 1 +
.../projects/guest_navigation_menu_spec.rb | 28 +++++++++
.../security/project/private_access_spec.rb | 2 +-
spec/models/event_spec.rb | 11 ++++
spec/policies/project_policy_spec.rb | 5 +-
.../merge_requests/update_service_spec.rb | 6 ++
spec/services/notification_service_spec.rb | 57 ++++++++++++++++++-
spec/services/todo_service_spec.rb | 22 ++++++-
13 files changed, 143 insertions(+), 13 deletions(-)
create mode 100644 spec/features/projects/guest_navigation_menu_spec.rb
diff --git a/CHANGELOG b/CHANGELOG
index 2095e4fc0fd..9653c4da17f 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -93,6 +93,7 @@ v 8.13.0 (unreleased)
- Cleanup Ci::ApplicationController. !6757 (Takuya Noguchi)
- Fix a typo in doc/api/labels.md
- API: all unknown routing will be handled with 404 Not Found
+ - Make guests unable to view MRs on private projects
v 8.12.5 (unreleased)
- Update the mail_room gem to 0.8.1 to fix a race condition with the mailbox watching thread
diff --git a/app/models/event.rb b/app/models/event.rb
index 314d5ba438f..0764cb8cabd 100644
--- a/app/models/event.rb
+++ b/app/models/event.rb
@@ -68,8 +68,10 @@ class Event < ActiveRecord::Base
true
elsif issue? || issue_note?
Ability.allowed?(user, :read_issue, note? ? note_target : target)
+ elsif merge_request? || merge_request_note?
+ Ability.allowed?(user, :read_merge_request, note? ? note_target : target)
else
- ((merge_request? || note?) && target.present?) || milestone?
+ milestone?
end
end
@@ -280,6 +282,10 @@ class Event < ActiveRecord::Base
note? && target && target.for_issue?
end
+ def merge_request_note?
+ note? && target && target.for_merge_request?
+ end
+
def project_snippet_note?
target.for_snippet?
end
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index a806cf83782..be4721d7a51 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -40,7 +40,6 @@ class ProjectPolicy < BasePolicy
can! :read_milestone
can! :read_project_snippet
can! :read_project_member
- can! :read_merge_request
can! :read_note
can! :create_project
can! :create_issue
@@ -63,6 +62,7 @@ class ProjectPolicy < BasePolicy
can! :read_pipeline
can! :read_environment
can! :read_deployment
+ can! :read_merge_request
end
# Permissions given when an user is team member of a project
@@ -117,6 +117,7 @@ class ProjectPolicy < BasePolicy
can! :read_container_image
can! :build_download_code
can! :build_read_container_image
+ can! :read_merge_request
end
def owner_access!
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index de8049b8e2e..72712afc07e 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -475,10 +475,12 @@ class NotificationService
end
def reject_users_without_access(recipients, target)
- return recipients unless target.is_a?(Issue)
+ return recipients unless target.is_a?(Issuable)
+
+ ability = :"read_#{target.to_ability_name}"
recipients.select do |user|
- user.can?(:read_issue, target)
+ user.can?(ability, target)
end
end
diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb
index 776530ac0a5..f8e6b2ef094 100644
--- a/app/services/todo_service.rb
+++ b/app/services/todo_service.rb
@@ -273,12 +273,12 @@ class TodoService
end
def reject_users_without_access(users, project, target)
- if target.is_a?(Note) && target.for_issue?
+ if target.is_a?(Note) && (target.for_issue? || target.for_merge_request?)
target = target.noteable
end
- if target.is_a?(Issue)
- select_users(users, :read_issue, target)
+ if target.is_a?(Issuable)
+ select_users(users, :"read_#{target.to_ability_name}", target)
else
select_users(users, :read_project, project)
end
diff --git a/doc/user/permissions.md b/doc/user/permissions.md
index c0dc80325b6..d6216a8dd50 100644
--- a/doc/user/permissions.md
+++ b/doc/user/permissions.md
@@ -32,6 +32,7 @@ The following table depicts the various user permission levels in a project.
| See a commit status | | ✓ | ✓ | ✓ | ✓ |
| See a container registry | | ✓ | ✓ | ✓ | ✓ |
| See environments | | ✓ | ✓ | ✓ | ✓ |
+| See a list of merge requests | | ✓ | ✓ | ✓ | ✓ |
| Manage/Accept merge requests | | | ✓ | ✓ | ✓ |
| Create new merge request | | | ✓ | ✓ | ✓ |
| Create new branches | | | ✓ | ✓ | ✓ |
diff --git a/spec/features/projects/guest_navigation_menu_spec.rb b/spec/features/projects/guest_navigation_menu_spec.rb
new file mode 100644
index 00000000000..c22441f8929
--- /dev/null
+++ b/spec/features/projects/guest_navigation_menu_spec.rb
@@ -0,0 +1,28 @@
+require 'spec_helper'
+
+describe "Guest navigation menu" do
+ let(:project) { create :empty_project, :private }
+ let(:guest) { create :user }
+
+ before do
+ project.team << [guest, :guest]
+
+ login_as(guest)
+ end
+
+ it "shows allowed tabs only" do
+ visit namespace_project_path(project.namespace, project)
+
+ within(".nav-links") do
+ expect(page).to have_content 'Project'
+ expect(page).to have_content 'Activity'
+ expect(page).to have_content 'Issues'
+ expect(page).to have_content 'Wiki'
+
+ expect(page).not_to have_content 'Repository'
+ expect(page).not_to have_content 'Pipelines'
+ expect(page).not_to have_content 'Graphs'
+ expect(page).not_to have_content 'Merge Requests'
+ end
+ end
+end
diff --git a/spec/features/security/project/private_access_spec.rb b/spec/features/security/project/private_access_spec.rb
index ccb5c06dab0..79417c769a8 100644
--- a/spec/features/security/project/private_access_spec.rb
+++ b/spec/features/security/project/private_access_spec.rb
@@ -203,7 +203,7 @@ describe "Private Project Access", feature: true do
it { is_expected.to be_allowed_for master }
it { is_expected.to be_allowed_for developer }
it { is_expected.to be_allowed_for reporter }
- it { is_expected.to be_allowed_for guest }
+ it { is_expected.to be_denied_for guest }
it { is_expected.to be_denied_for :user }
it { is_expected.to be_denied_for :external }
it { is_expected.to be_denied_for :visitor }
diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb
index af5002487cc..06cac929bbf 100644
--- a/spec/models/event_spec.rb
+++ b/spec/models/event_spec.rb
@@ -135,6 +135,17 @@ describe Event, models: true do
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 }
+
+ context 'private project' do
+ let(:project) { create(:project, :private) }
+
+ it { expect(event.visible_to_user?(non_member)).to eq false }
+ 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
end
diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb
index 43c8d884a47..658e3c13a73 100644
--- a/spec/policies/project_policy_spec.rb
+++ b/spec/policies/project_policy_spec.rb
@@ -12,7 +12,7 @@ describe ProjectPolicy, models: true do
[
:read_project, :read_board, :read_list, :read_wiki, :read_issue, :read_label,
:read_milestone, :read_project_snippet, :read_project_member,
- :read_merge_request, :read_note, :create_project, :create_issue, :create_note,
+ :read_note, :create_project, :create_issue, :create_note,
:upload_file
]
end
@@ -21,7 +21,8 @@ describe ProjectPolicy, models: true do
[
:download_code, :fork_project, :create_project_snippet, :update_issue,
:admin_issue, :admin_label, :admin_list, :read_commit_status, :read_build,
- :read_container_image, :read_pipeline, :read_environment, :read_deployment
+ :read_container_image, :read_pipeline, :read_environment, :read_deployment,
+ :read_merge_request
]
end
diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb
index 33db34c0f62..fd5f94047c2 100644
--- a/spec/services/merge_requests/update_service_spec.rb
+++ b/spec/services/merge_requests/update_service_spec.rb
@@ -17,6 +17,7 @@ describe MergeRequests::UpdateService, services: true do
before do
project.team << [user, :master]
project.team << [user2, :developer]
+ project.team << [user3, :developer]
end
describe 'execute' do
@@ -188,6 +189,11 @@ describe MergeRequests::UpdateService, services: true do
let!(:non_subscriber) { create(:user) }
let!(:subscriber) { create(:user).tap { |u| label.toggle_subscription(u) } }
+ before do
+ project.team << [non_subscriber, :developer]
+ project.team << [subscriber, :developer]
+ end
+
it 'sends notifications for subscribers of newly added labels' do
opts = { label_ids: [label.id] }
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index d820646ebdf..699b9925b4e 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -331,7 +331,7 @@ describe NotificationService, services: true do
describe '#new_note' do
it "records sent notifications" do
# Ensure create SentNotification by noteable = merge_request 6 times, not noteable = note
- expect(SentNotification).to receive(:record_note).with(note, any_args).exactly(4).times.and_call_original
+ expect(SentNotification).to receive(:record_note).with(note, any_args).exactly(3).times.and_call_original
notification.new_note(note)
@@ -1169,6 +1169,61 @@ describe NotificationService, services: true do
end
end
+ context 'guest user in private project' do
+ let(:private_project) { create(:empty_project, :private) }
+ let(:guest) { create(:user) }
+ let(:developer) { create(:user) }
+ let(:assignee) { create(:user) }
+ let(:merge_request) { create(:merge_request, source_project: private_project, assignee: assignee) }
+ let(:merge_request1) { create(:merge_request, source_project: private_project, assignee: assignee, description: "cc @#{guest.username}") }
+ let(:note) { create(:note, noteable: merge_request, project: private_project) }
+
+ before do
+ private_project.team << [assignee, :developer]
+ private_project.team << [developer, :developer]
+ private_project.team << [guest, :guest]
+
+ ActionMailer::Base.deliveries.clear
+ end
+
+ it 'filters out guests when new note is created' do
+ expect(SentNotification).to receive(:record).with(merge_request, any_args).exactly(1).times
+
+ notification.new_note(note)
+
+ should_not_email(guest)
+ should_email(assignee)
+ end
+
+ it 'filters out guests when new merge request is created' do
+ notification.new_merge_request(merge_request1, @u_disabled)
+
+ should_not_email(guest)
+ should_email(assignee)
+ end
+
+ it 'filters out guests when merge request is closed' do
+ notification.close_mr(merge_request, developer)
+
+ should_not_email(guest)
+ should_email(assignee)
+ end
+
+ it 'filters out guests when merge request is reopened' do
+ notification.reopen_mr(merge_request, developer)
+
+ should_not_email(guest)
+ should_email(assignee)
+ end
+
+ it 'filters out guests when merge request is merged' do
+ notification.merge_mr(merge_request, developer)
+
+ should_not_email(guest)
+ should_email(assignee)
+ end
+ end
+
def build_team(project)
@u_watcher = create_global_setting_for(create(:user), :watch)
@u_participating = create_global_setting_for(create(:user), :participating)
diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb
index b41f6f14fbd..ed55791d24e 100644
--- a/spec/services/todo_service_spec.rb
+++ b/spec/services/todo_service_spec.rb
@@ -345,7 +345,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: guest, target: mr_assigned, action: Todo::MENTIONED)
should_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)
@@ -357,7 +357,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_not_create_todo(user: guest, target: mr_assigned, action: Todo::MENTIONED)
should_create_todo(user: john_doe, target: mr_assigned, action: Todo::MENTIONED)
should_create_todo(user: author, target: mr_assigned, action: Todo::MENTIONED)
should_not_create_todo(user: non_member, target: mr_assigned, action: Todo::MENTIONED)
@@ -381,6 +381,7 @@ describe TodoService, services: true do
should_not_create_todo(user: john_doe, target: mr_assigned, action: Todo::MENTIONED)
should_not_create_todo(user: member, target: mr_assigned, action: Todo::MENTIONED)
should_not_create_todo(user: non_member, target: mr_assigned, action: Todo::MENTIONED)
+ should_not_create_todo(user: guest, target: mr_assigned, action: Todo::MENTIONED)
end
it 'does not raise an error when description not change' do
@@ -430,6 +431,11 @@ describe TodoService, services: true do
should_create_todo(user: john_doe, target: mr_assigned, author: john_doe, action: Todo::ASSIGNED)
end
+
+ it 'does not create a todo for guests' do
+ service.reassigned_merge_request(mr_assigned, author)
+ should_not_create_todo(user: guest, target: mr_assigned, action: Todo::MENTIONED)
+ end
end
describe '#merge_merge_request' do
@@ -441,6 +447,11 @@ describe TodoService, services: true do
expect(first_todo.reload).to be_done
expect(second_todo.reload).to be_done
end
+
+ it 'does not create todo for guests' do
+ service.merge_merge_request(mr_assigned, john_doe)
+ should_not_create_todo(user: guest, target: mr_assigned, action: Todo::MENTIONED)
+ end
end
describe '#new_award_emoji' do
@@ -495,6 +506,13 @@ describe TodoService, services: true do
should_create_todo(user: john_doe, target: mr_unassigned, author: author, action: Todo::MENTIONED, note: legacy_diff_note_on_merge_request)
end
+
+ it 'does not create todo for guests' do
+ note_on_merge_request = create :note_on_merge_request, project: project, noteable: mr_assigned, note: mentions
+ service.new_note(note_on_merge_request, author)
+
+ should_not_create_todo(user: guest, target: mr_assigned, action: Todo::MENTIONED)
+ end
end
end
--
GitLab