kotovalexarian-likes-gitlab/gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Make guests unable to view MRs

Browse source
This commit is contained in:
Valery Sizov 2016-10-04 15:52:08 +03:00
parent a3169d522a
commit b4004488f7
13 changed files with 143 additions and 13 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
CHANGELOG
View file

@ -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

8
app/models/event.rb
View file

@ -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

3
app/policies/project_policy.rb
View file

@ -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!

6
app/services/notification_service.rb
View file

@ -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

6
app/services/todo_service.rb
View file

@ -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

1
doc/user/permissions.md
View file

@ -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 | | | ✓ | ✓ | ✓ |

28
spec/features/projects/guest_navigation_menu_spec.rb Normal file
View file

@ -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

2
spec/features/security/project/private_access_spec.rb
View file

@ -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 }

11
spec/models/event_spec.rb
View file

@ -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

5
spec/policies/project_policy_spec.rb
View file

@ -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

6
spec/services/merge_requests/update_service_spec.rb
View file

@ -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] }

57
spec/services/notification_service_spec.rb
View file

@ -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)

22
spec/services/todo_service_spec.rb
View file

@ -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