Commit 35b8f103 authored by Heinrich Lee Yu's avatar Heinrich Lee Yu Committed by Yorick Peterse

Prevent comments by email when issue is locked

This changes the permission check so it uses the policy on Noteable
instead of Project. This prevents bypassing of rules defined in
Noteable for locked discussions and confidential issues.

Also rechecks permissions when reply_to_discussion_id is provided since the
discussion_id may be from a different noteable.
parent 15490396
...@@ -18,6 +18,7 @@ class IssuePolicy < IssuablePolicy ...@@ -18,6 +18,7 @@ class IssuePolicy < IssuablePolicy
prevent :read_issue_iid prevent :read_issue_iid
prevent :update_issue prevent :update_issue
prevent :admin_issue prevent :admin_issue
prevent :create_note
end end
rule { locked }.policy do rule { locked }.policy do
......
...@@ -28,7 +28,10 @@ class PersonalSnippetPolicy < BasePolicy ...@@ -28,7 +28,10 @@ class PersonalSnippetPolicy < BasePolicy
rule { anonymous }.prevent :comment_personal_snippet rule { anonymous }.prevent :comment_personal_snippet
rule { can?(:comment_personal_snippet) }.enable :award_emoji rule { can?(:comment_personal_snippet) }.policy do
enable :create_note
enable :award_emoji
end
rule { full_private_access }.enable :read_personal_snippet rule { full_private_access }.enable :read_personal_snippet
end end
...@@ -43,4 +43,6 @@ class ProjectSnippetPolicy < BasePolicy ...@@ -43,4 +43,6 @@ class ProjectSnippetPolicy < BasePolicy
enable :update_project_snippet enable :update_project_snippet
enable :admin_project_snippet enable :admin_project_snippet
end end
rule { ~can?(:read_project_snippet) }.prevent :create_note
end end
...@@ -9,7 +9,7 @@ module Notes ...@@ -9,7 +9,7 @@ module Notes
if in_reply_to_discussion_id.present? if in_reply_to_discussion_id.present?
discussion = find_discussion(in_reply_to_discussion_id) discussion = find_discussion(in_reply_to_discussion_id)
unless discussion unless discussion && can?(current_user, :create_note, discussion.noteable)
note = Note.new note = Note.new
note.errors.add(:base, 'Discussion to reply to cannot be found') note.errors.add(:base, 'Discussion to reply to cannot be found')
return note return note
...@@ -34,19 +34,8 @@ module Notes ...@@ -34,19 +34,8 @@ module Notes
if project if project
project.notes.find_discussion(discussion_id) project.notes.find_discussion(discussion_id)
else else
discussion = Note.find_discussion(discussion_id) Note.find_discussion(discussion_id)
noteable = discussion.noteable
return nil unless noteable_without_project?(noteable)
discussion
end
end end
def noteable_without_project?(noteable)
return true if noteable.is_a?(PersonalSnippet) && can?(current_user, :comment_personal_snippet, noteable)
false
end end
end end
end end
---
title: Prevent unauthorized replies when discussion is locked or confidential
merge_request:
author:
type: security
...@@ -56,7 +56,7 @@ module Gitlab ...@@ -56,7 +56,7 @@ module Gitlab
raise ProjectNotFound unless author.can?(:read_project, project) raise ProjectNotFound unless author.can?(:read_project, project)
end end
raise UserNotAuthorizedError unless author.can?(permission, project || noteable) raise UserNotAuthorizedError unless author.can?(permission, try(:noteable) || project)
end end
def verify_record!(record:, invalid_exception:, record_name:) def verify_record!(record:, invalid_exception:, record_name:)
......
...@@ -118,6 +118,43 @@ describe Gitlab::Email::Handler::CreateNoteHandler do ...@@ -118,6 +118,43 @@ describe Gitlab::Email::Handler::CreateNoteHandler do
end end
end end
shared_examples "checks permissions on noteable" do
context "when user has access" do
before do
project.add_reporter(user)
end
it "creates a comment" do
expect { receiver.execute }.to change { noteable.notes.count }.by(1)
end
end
context "when user does not have access" do
it "raises UserNotAuthorizedError" do
expect { receiver.execute }.to raise_error(Gitlab::Email::UserNotAuthorizedError)
end
end
end
context "when discussion is locked" do
before do
noteable.update_attribute(:discussion_locked, true)
end
it_behaves_like "checks permissions on noteable"
end
context "when issue is confidential" do
let(:issue) { create(:issue, project: project) }
let(:note) { create(:note, noteable: issue, project: project) }
before do
issue.update_attribute(:confidential, true)
end
it_behaves_like "checks permissions on noteable"
end
context "when everything is fine" do context "when everything is fine" do
before do before do
setup_attachment setup_attachment
......
...@@ -48,7 +48,7 @@ describe SentNotification do ...@@ -48,7 +48,7 @@ describe SentNotification do
let(:note) { create(:diff_note_on_merge_request) } let(:note) { create(:diff_note_on_merge_request) }
it 'creates a new SentNotification' do it 'creates a new SentNotification' do
expect { described_class.record_note(note, user.id) }.to change { described_class.count }.by(1) expect { described_class.record_note(note, note.author.id) }.to change { described_class.count }.by(1)
end end
end end
......
...@@ -14,6 +14,13 @@ describe PersonalSnippetPolicy do ...@@ -14,6 +14,13 @@ describe PersonalSnippetPolicy do
] ]
end end
let(:comment_permissions) do
[
:comment_personal_snippet,
:create_note
]
end
def permissions(user) def permissions(user)
described_class.new(user, snippet) described_class.new(user, snippet)
end end
...@@ -26,7 +33,7 @@ describe PersonalSnippetPolicy do ...@@ -26,7 +33,7 @@ describe PersonalSnippetPolicy do
it do it do
is_expected.to be_allowed(:read_personal_snippet) is_expected.to be_allowed(:read_personal_snippet)
is_expected.to be_disallowed(:comment_personal_snippet) is_expected.to be_disallowed(*comment_permissions)
is_expected.to be_disallowed(:award_emoji) is_expected.to be_disallowed(:award_emoji)
is_expected.to be_disallowed(*author_permissions) is_expected.to be_disallowed(*author_permissions)
end end
...@@ -37,7 +44,7 @@ describe PersonalSnippetPolicy do ...@@ -37,7 +44,7 @@ describe PersonalSnippetPolicy do
it do it do
is_expected.to be_allowed(:read_personal_snippet) is_expected.to be_allowed(:read_personal_snippet)
is_expected.to be_allowed(:comment_personal_snippet) is_expected.to be_allowed(*comment_permissions)
is_expected.to be_allowed(:award_emoji) is_expected.to be_allowed(:award_emoji)
is_expected.to be_disallowed(*author_permissions) is_expected.to be_disallowed(*author_permissions)
end end
...@@ -48,7 +55,7 @@ describe PersonalSnippetPolicy do ...@@ -48,7 +55,7 @@ describe PersonalSnippetPolicy do
it do it do
is_expected.to be_allowed(:read_personal_snippet) is_expected.to be_allowed(:read_personal_snippet)
is_expected.to be_allowed(:comment_personal_snippet) is_expected.to be_allowed(*comment_permissions)
is_expected.to be_allowed(:award_emoji) is_expected.to be_allowed(:award_emoji)
is_expected.to be_allowed(*author_permissions) is_expected.to be_allowed(*author_permissions)
end end
...@@ -63,7 +70,7 @@ describe PersonalSnippetPolicy do ...@@ -63,7 +70,7 @@ describe PersonalSnippetPolicy do
it do it do
is_expected.to be_disallowed(:read_personal_snippet) is_expected.to be_disallowed(:read_personal_snippet)
is_expected.to be_disallowed(:comment_personal_snippet) is_expected.to be_disallowed(*comment_permissions)
is_expected.to be_disallowed(:award_emoji) is_expected.to be_disallowed(:award_emoji)
is_expected.to be_disallowed(*author_permissions) is_expected.to be_disallowed(*author_permissions)
end end
...@@ -74,7 +81,7 @@ describe PersonalSnippetPolicy do ...@@ -74,7 +81,7 @@ describe PersonalSnippetPolicy do
it do it do
is_expected.to be_allowed(:read_personal_snippet) is_expected.to be_allowed(:read_personal_snippet)
is_expected.to be_allowed(:comment_personal_snippet) is_expected.to be_allowed(*comment_permissions)
is_expected.to be_allowed(:award_emoji) is_expected.to be_allowed(:award_emoji)
is_expected.to be_disallowed(*author_permissions) is_expected.to be_disallowed(*author_permissions)
end end
...@@ -85,7 +92,7 @@ describe PersonalSnippetPolicy do ...@@ -85,7 +92,7 @@ describe PersonalSnippetPolicy do
it do it do
is_expected.to be_disallowed(:read_personal_snippet) is_expected.to be_disallowed(:read_personal_snippet)
is_expected.to be_disallowed(:comment_personal_snippet) is_expected.to be_disallowed(*comment_permissions)
is_expected.to be_disallowed(:award_emoji) is_expected.to be_disallowed(:award_emoji)
is_expected.to be_disallowed(*author_permissions) is_expected.to be_disallowed(*author_permissions)
end end
...@@ -96,7 +103,7 @@ describe PersonalSnippetPolicy do ...@@ -96,7 +103,7 @@ describe PersonalSnippetPolicy do
it do it do
is_expected.to be_allowed(:read_personal_snippet) is_expected.to be_allowed(:read_personal_snippet)
is_expected.to be_allowed(:comment_personal_snippet) is_expected.to be_allowed(*comment_permissions)
is_expected.to be_allowed(:award_emoji) is_expected.to be_allowed(:award_emoji)
is_expected.to be_allowed(*author_permissions) is_expected.to be_allowed(*author_permissions)
end end
...@@ -111,7 +118,7 @@ describe PersonalSnippetPolicy do ...@@ -111,7 +118,7 @@ describe PersonalSnippetPolicy do
it do it do
is_expected.to be_disallowed(:read_personal_snippet) is_expected.to be_disallowed(:read_personal_snippet)
is_expected.to be_disallowed(:comment_personal_snippet) is_expected.to be_disallowed(*comment_permissions)
is_expected.to be_disallowed(:award_emoji) is_expected.to be_disallowed(:award_emoji)
is_expected.to be_disallowed(*author_permissions) is_expected.to be_disallowed(*author_permissions)
end end
...@@ -122,7 +129,7 @@ describe PersonalSnippetPolicy do ...@@ -122,7 +129,7 @@ describe PersonalSnippetPolicy do
it do it do
is_expected.to be_disallowed(:read_personal_snippet) is_expected.to be_disallowed(:read_personal_snippet)
is_expected.to be_disallowed(:comment_personal_snippet) is_expected.to be_disallowed(*comment_permissions)
is_expected.to be_disallowed(:award_emoji) is_expected.to be_disallowed(:award_emoji)
is_expected.to be_disallowed(*author_permissions) is_expected.to be_disallowed(*author_permissions)
end end
...@@ -144,7 +151,7 @@ describe PersonalSnippetPolicy do ...@@ -144,7 +151,7 @@ describe PersonalSnippetPolicy do
it do it do
is_expected.to be_disallowed(:read_personal_snippet) is_expected.to be_disallowed(:read_personal_snippet)
is_expected.to be_disallowed(:comment_personal_snippet) is_expected.to be_disallowed(*comment_permissions)
is_expected.to be_disallowed(:award_emoji) is_expected.to be_disallowed(:award_emoji)
is_expected.to be_disallowed(*author_permissions) is_expected.to be_disallowed(*author_permissions)
end end
...@@ -155,7 +162,7 @@ describe PersonalSnippetPolicy do ...@@ -155,7 +162,7 @@ describe PersonalSnippetPolicy do
it do it do
is_expected.to be_allowed(:read_personal_snippet) is_expected.to be_allowed(:read_personal_snippet)
is_expected.to be_allowed(:comment_personal_snippet) is_expected.to be_allowed(*comment_permissions)
is_expected.to be_allowed(:award_emoji) is_expected.to be_allowed(:award_emoji)
is_expected.to be_allowed(*author_permissions) is_expected.to be_allowed(*author_permissions)
end end
......
...@@ -41,7 +41,7 @@ describe ProjectSnippetPolicy do ...@@ -41,7 +41,7 @@ describe ProjectSnippetPolicy do
subject { abilities(regular_user, :public) } subject { abilities(regular_user, :public) }
it do it do
expect_allowed(:read_project_snippet) expect_allowed(:read_project_snippet, :create_note)
expect_disallowed(*author_permissions) expect_disallowed(*author_permissions)
end end
end end
...@@ -50,7 +50,7 @@ describe ProjectSnippetPolicy do ...@@ -50,7 +50,7 @@ describe ProjectSnippetPolicy do
subject { abilities(external_user, :public) } subject { abilities(external_user, :public) }
it do it do
expect_allowed(:read_project_snippet) expect_allowed(:read_project_snippet, :create_note)
expect_disallowed(*author_permissions) expect_disallowed(*author_permissions)
end end
end end
...@@ -70,7 +70,7 @@ describe ProjectSnippetPolicy do ...@@ -70,7 +70,7 @@ describe ProjectSnippetPolicy do
subject { abilities(regular_user, :internal) } subject { abilities(regular_user, :internal) }
it do it do
expect_allowed(:read_project_snippet) expect_allowed(:read_project_snippet, :create_note)
expect_disallowed(*author_permissions) expect_disallowed(*author_permissions)
end end
end end
...@@ -79,7 +79,7 @@ describe ProjectSnippetPolicy do ...@@ -79,7 +79,7 @@ describe ProjectSnippetPolicy do
subject { abilities(external_user, :internal) } subject { abilities(external_user, :internal) }
it do it do
expect_disallowed(:read_project_snippet) expect_disallowed(:read_project_snippet, :create_note)
expect_disallowed(*author_permissions) expect_disallowed(*author_permissions)
end end
end end
...@@ -92,7 +92,7 @@ describe ProjectSnippetPolicy do ...@@ -92,7 +92,7 @@ describe ProjectSnippetPolicy do
end end
it do it do
expect_allowed(:read_project_snippet) expect_allowed(:read_project_snippet, :create_note)
expect_disallowed(*author_permissions) expect_disallowed(*author_permissions)
end end
end end
...@@ -112,7 +112,7 @@ describe ProjectSnippetPolicy do ...@@ -112,7 +112,7 @@ describe ProjectSnippetPolicy do
subject { abilities(regular_user, :private) } subject { abilities(regular_user, :private) }
it do it do
expect_disallowed(:read_project_snippet) expect_disallowed(:read_project_snippet, :create_note)
expect_disallowed(*author_permissions) expect_disallowed(*author_permissions)
end end
end end
...@@ -123,7 +123,7 @@ describe ProjectSnippetPolicy do ...@@ -123,7 +123,7 @@ describe ProjectSnippetPolicy do
subject { described_class.new(regular_user, snippet) } subject { described_class.new(regular_user, snippet) }
it do it do
expect_allowed(:read_project_snippet) expect_allowed(:read_project_snippet, :create_note)
expect_allowed(*author_permissions) expect_allowed(*author_permissions)
end end
end end
...@@ -136,7 +136,7 @@ describe ProjectSnippetPolicy do ...@@ -136,7 +136,7 @@ describe ProjectSnippetPolicy do
end end
it do it do
expect_allowed(:read_project_snippet) expect_allowed(:read_project_snippet, :create_note)
expect_disallowed(*author_permissions) expect_disallowed(*author_permissions)
end end
end end
...@@ -149,7 +149,7 @@ describe ProjectSnippetPolicy do ...@@ -149,7 +149,7 @@ describe ProjectSnippetPolicy do
end end
it do it do
expect_allowed(:read_project_snippet) expect_allowed(:read_project_snippet, :create_note)
expect_disallowed(*author_permissions) expect_disallowed(*author_permissions)
end end
end end
...@@ -158,7 +158,7 @@ describe ProjectSnippetPolicy do ...@@ -158,7 +158,7 @@ describe ProjectSnippetPolicy do
subject { abilities(create(:admin), :private) } subject { abilities(create(:admin), :private) }
it do it do
expect_allowed(:read_project_snippet) expect_allowed(:read_project_snippet, :create_note)
expect_allowed(*author_permissions) expect_allowed(*author_permissions)
end end
end end
......
...@@ -45,6 +45,15 @@ describe Notes::BuildService do ...@@ -45,6 +45,15 @@ describe Notes::BuildService do
end end
end end
context 'when user has no access to discussion' do
it 'sets an error' do
another_user = create(:user)
new_note = described_class.new(project, another_user, note: 'Test', in_reply_to_discussion_id: note.discussion_id).execute
expect(new_note.errors[:base]).to include('Discussion to reply to cannot be found')
end
end
context 'personal snippet note' do context 'personal snippet note' do
def reply(note, user = nil) def reply(note, user = nil)
user ||= create(:user) user ||= create(:user)
......
...@@ -127,6 +127,10 @@ describe Notes::CreateService do ...@@ -127,6 +127,10 @@ describe Notes::CreateService do
create(:diff_note_on_merge_request, noteable: merge_request, project: project_with_repo) create(:diff_note_on_merge_request, noteable: merge_request, project: project_with_repo)
end end
before do
project_with_repo.add_maintainer(user)
end
context 'when eligible to have a note diff file' do context 'when eligible to have a note diff file' do
let(:new_opts) do let(:new_opts) do
opts.merge(in_reply_to_discussion_id: nil, opts.merge(in_reply_to_discussion_id: nil,
......
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