Commit ec4ade50 authored by John Jarvis's avatar John Jarvis

Merge branch 'security-53543-user-keeps-access-to-mr-issue-when-removed-from-team' into 'master'

[master] Adds validation to check if user can read project

See merge request gitlab/gitlabhq!2645
parents 3fca973e 52feca59
...@@ -11,7 +11,7 @@ class IssuablePolicy < BasePolicy ...@@ -11,7 +11,7 @@ class IssuablePolicy < BasePolicy
@user && @subject.assignee_or_author?(@user) @user && @subject.assignee_or_author?(@user)
end end
rule { assignee_or_author }.policy do rule { can?(:guest_access) & assignee_or_author }.policy do
enable :read_issue enable :read_issue
enable :update_issue enable :update_issue
enable :reopen_issue enable :reopen_issue
......
---
title: Issuable no longer is visible to users when project can't be viewed
merge_request:
author:
type: security
...@@ -243,6 +243,20 @@ describe Event do ...@@ -243,6 +243,20 @@ describe Event do
expect(event.visible_to_user?(admin)).to eq true expect(event.visible_to_user?(admin)).to eq true
end end
end end
context 'private project' do
let(:project) { create(:project, :private) }
let(:target) { note_on_issue }
it do
expect(event.visible_to_user?(non_member)).to eq false
expect(event.visible_to_user?(author)).to eq false
expect(event.visible_to_user?(assignee)).to eq false
expect(event.visible_to_user?(member)).to eq true
expect(event.visible_to_user?(guest)).to eq true
expect(event.visible_to_user?(admin)).to eq true
end
end
end end
context 'merge request diff note event' do context 'merge request diff note event' do
...@@ -265,8 +279,8 @@ describe Event do ...@@ -265,8 +279,8 @@ describe Event do
it do it do
expect(event.visible_to_user?(non_member)).to eq false expect(event.visible_to_user?(non_member)).to eq false
expect(event.visible_to_user?(author)).to eq true expect(event.visible_to_user?(author)).to eq false
expect(event.visible_to_user?(assignee)).to eq true expect(event.visible_to_user?(assignee)).to eq false
expect(event.visible_to_user?(member)).to eq true expect(event.visible_to_user?(member)).to eq true
expect(event.visible_to_user?(guest)).to eq false expect(event.visible_to_user?(guest)).to eq false
expect(event.visible_to_user?(admin)).to eq true expect(event.visible_to_user?(admin)).to eq true
......
...@@ -7,6 +7,33 @@ describe IssuablePolicy, models: true do ...@@ -7,6 +7,33 @@ describe IssuablePolicy, models: true do
let(:policies) { described_class.new(user, issue) } let(:policies) { described_class.new(user, issue) }
describe '#rules' do describe '#rules' do
context 'when user is author of issuable' do
let(:merge_request) { create(:merge_request, source_project: project, author: user) }
let(:policies) { described_class.new(user, merge_request) }
context 'when user is able to read project' do
it 'enables user to read and update issuables' do
expect(policies).to be_allowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request)
end
end
context 'when project is private' do
let(:project) { create(:project, :private) }
context 'when user belongs to the projects team' do
it 'enables user to read and update issuables' do
project.add_maintainer(user)
expect(policies).to be_allowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request)
end
end
it 'disallows user from reading and updating issuables from that project' do
expect(policies).to be_disallowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request)
end
end
end
context 'when discussion is locked for the issuable' do context 'when discussion is locked for the issuable' do
let(:issue) { create(:issue, project: project, discussion_locked: true) } let(:issue) { create(:issue, project: project, discussion_locked: true) }
......
...@@ -28,6 +28,33 @@ describe Issuable::BulkUpdateService do ...@@ -28,6 +28,33 @@ describe Issuable::BulkUpdateService do
expect(project.issues.opened).to be_empty expect(project.issues.opened).to be_empty
expect(project.issues.closed).not_to be_empty expect(project.issues.closed).not_to be_empty
end end
context 'when issue for a different project is created' do
let(:private_project) { create(:project, :private) }
let(:issue) { create(:issue, project: private_project, author: user) }
context 'when user has access to the project' do
it 'closes all issues passed' do
private_project.add_maintainer(user)
bulk_update(issues + [issue], state_event: 'close')
expect(project.issues.opened).to be_empty
expect(project.issues.closed).not_to be_empty
expect(private_project.issues.closed).not_to be_empty
end
end
context 'when user does not have access to project' do
it 'only closes all issues that the user has access to' do
bulk_update(issues + [issue], state_event: 'close')
expect(project.issues.opened).to be_empty
expect(project.issues.closed).not_to be_empty
expect(private_project.issues.closed).to be_empty
end
end
end
end end
describe 'reopen issues' do describe 'reopen issues' do
......
...@@ -19,6 +19,7 @@ describe TodoService do ...@@ -19,6 +19,7 @@ describe TodoService do
before do before do
project.add_guest(guest) project.add_guest(guest)
project.add_developer(author) project.add_developer(author)
project.add_developer(assignee)
project.add_developer(member) project.add_developer(member)
project.add_developer(john_doe) project.add_developer(john_doe)
project.add_developer(skipped) project.add_developer(skipped)
......
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