diff options
| -rw-r--r-- | app/policies/issuable_policy.rb | 2 | ||||
| -rw-r--r-- | changelogs/unreleased/security-53543-user-keeps-access-to-mr-issue-when-removed-from-team.yml | 5 | ||||
| -rw-r--r-- | spec/models/event_spec.rb | 18 | ||||
| -rw-r--r-- | spec/policies/issuable_policy_spec.rb | 27 | ||||
| -rw-r--r-- | spec/services/issuable/bulk_update_service_spec.rb | 27 | ||||
| -rw-r--r-- | spec/services/todo_service_spec.rb | 1 |
6 files changed, 77 insertions, 3 deletions
diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb index 6d8b575102e..ecb2797d1d9 100644 --- a/app/policies/issuable_policy.rb +++ b/app/policies/issuable_policy.rb @@ -11,7 +11,7 @@ class IssuablePolicy < BasePolicy @user && @subject.assignee_or_author?(@user) end - rule { assignee_or_author }.policy do + rule { can?(:guest_access) & assignee_or_author }.policy do enable :read_issue enable :update_issue enable :reopen_issue diff --git a/changelogs/unreleased/security-53543-user-keeps-access-to-mr-issue-when-removed-from-team.yml b/changelogs/unreleased/security-53543-user-keeps-access-to-mr-issue-when-removed-from-team.yml new file mode 100644 index 00000000000..ab12ba539c1 --- /dev/null +++ b/changelogs/unreleased/security-53543-user-keeps-access-to-mr-issue-when-removed-from-team.yml @@ -0,0 +1,5 @@ +--- +title: Issuable no longer is visible to users when project can't be viewed +merge_request: +author: +type: security diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb index 81748681528..a64720f1876 100644 --- a/spec/models/event_spec.rb +++ b/spec/models/event_spec.rb @@ -243,6 +243,20 @@ describe Event do expect(event.visible_to_user?(admin)).to eq true 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 context 'merge request diff note event' do @@ -265,8 +279,8 @@ describe Event do it do expect(event.visible_to_user?(non_member)).to eq false - expect(event.visible_to_user?(author)).to eq true - expect(event.visible_to_user?(assignee)).to eq true + 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 false expect(event.visible_to_user?(admin)).to eq true diff --git a/spec/policies/issuable_policy_spec.rb b/spec/policies/issuable_policy_spec.rb index d1bf98995e7..db3df760472 100644 --- a/spec/policies/issuable_policy_spec.rb +++ b/spec/policies/issuable_policy_spec.rb @@ -7,6 +7,33 @@ describe IssuablePolicy, models: true do let(:policies) { described_class.new(user, issue) } 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 let(:issue) { create(:issue, project: project, discussion_locked: true) } diff --git a/spec/services/issuable/bulk_update_service_spec.rb b/spec/services/issuable/bulk_update_service_spec.rb index f0b0f7956ce..ca366cdf1df 100644 --- a/spec/services/issuable/bulk_update_service_spec.rb +++ b/spec/services/issuable/bulk_update_service_spec.rb @@ -28,6 +28,33 @@ describe Issuable::BulkUpdateService do expect(project.issues.opened).to be_empty expect(project.issues.closed).not_to be_empty 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 describe 'reopen issues' do diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index c52515aefd8..253f2e44d10 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -19,6 +19,7 @@ describe TodoService do before do project.add_guest(guest) project.add_developer(author) + project.add_developer(assignee) project.add_developer(member) project.add_developer(john_doe) project.add_developer(skipped) |