diff options
author | Yorick Peterse <yorickpeterse@gmail.com> | 2019-01-24 12:48:32 +0000 |
---|---|---|
committer | Yorick Peterse <yorickpeterse@gmail.com> | 2019-01-24 12:48:35 +0000 |
commit | 9816b5cf75ac2b0ff3d35a78aa591b737024db7f (patch) | |
tree | 792a9bdbf8c2a36672f0a37cf90416860352de12 | |
parent | f25ba7820ed2c3f2461c09191369b80d18e9b934 (diff) | |
download | gitlab-ce-9816b5cf75ac2b0ff3d35a78aa591b737024db7f.tar.gz |
Merge branch 'security-2779-fix-email-comment-permissions-check-11-6' into 'security-11-6'
[11.6] Fix discussion replies permissions check
See merge request gitlab/gitlabhq!2825
(cherry picked from commit 367767766d9727101908a1f195120732d72201b1)
313a9f2e Prevent comments by email when issue is locked
-rw-r--r-- | app/policies/issue_policy.rb | 1 | ||||
-rw-r--r-- | app/policies/personal_snippet_policy.rb | 5 | ||||
-rw-r--r-- | app/policies/project_snippet_policy.rb | 2 | ||||
-rw-r--r-- | app/services/notes/build_service.rb | 15 | ||||
-rw-r--r-- | changelogs/unreleased/security-2779-fix-email-comment-permissions-check.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/email/handler/reply_processing.rb | 2 | ||||
-rw-r--r-- | spec/lib/gitlab/email/handler/create_note_handler_spec.rb | 37 | ||||
-rw-r--r-- | spec/models/sent_notification_spec.rb | 2 | ||||
-rw-r--r-- | spec/policies/personal_snippet_policy_spec.rb | 29 | ||||
-rw-r--r-- | spec/policies/project_snippet_policy_spec.rb | 20 | ||||
-rw-r--r-- | spec/services/notes/build_service_spec.rb | 9 | ||||
-rw-r--r-- | spec/services/notes/create_service_spec.rb | 4 |
12 files changed, 94 insertions, 37 deletions
diff --git a/app/policies/issue_policy.rb b/app/policies/issue_policy.rb index a0706eaa46c..dd8c5d49cf4 100644 --- a/app/policies/issue_policy.rb +++ b/app/policies/issue_policy.rb @@ -18,6 +18,7 @@ class IssuePolicy < IssuablePolicy prevent :read_issue_iid prevent :update_issue prevent :admin_issue + prevent :create_note end rule { locked }.policy do diff --git a/app/policies/personal_snippet_policy.rb b/app/policies/personal_snippet_policy.rb index 777f933cdcd..16e67a06d56 100644 --- a/app/policies/personal_snippet_policy.rb +++ b/app/policies/personal_snippet_policy.rb @@ -28,5 +28,8 @@ class PersonalSnippetPolicy < BasePolicy 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 end diff --git a/app/policies/project_snippet_policy.rb b/app/policies/project_snippet_policy.rb index 288bf070cfc..db39de5e0d8 100644 --- a/app/policies/project_snippet_policy.rb +++ b/app/policies/project_snippet_policy.rb @@ -44,4 +44,6 @@ class ProjectSnippetPolicy < BasePolicy enable :update_project_snippet enable :admin_project_snippet end + + rule { ~can?(:read_project_snippet) }.prevent :create_note end diff --git a/app/services/notes/build_service.rb b/app/services/notes/build_service.rb index 7b92fe6fe14..bae98ede561 100644 --- a/app/services/notes/build_service.rb +++ b/app/services/notes/build_service.rb @@ -9,7 +9,7 @@ module Notes if in_reply_to_discussion_id.present? discussion = find_discussion(in_reply_to_discussion_id) - unless discussion + unless discussion && can?(current_user, :create_note, discussion.noteable) note = Note.new note.errors.add(:base, 'Discussion to reply to cannot be found') return note @@ -34,19 +34,8 @@ module Notes if project project.notes.find_discussion(discussion_id) else - discussion = Note.find_discussion(discussion_id) - noteable = discussion.noteable - - return nil unless noteable_without_project?(noteable) - - discussion + Note.find_discussion(discussion_id) 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 diff --git a/changelogs/unreleased/security-2779-fix-email-comment-permissions-check.yml b/changelogs/unreleased/security-2779-fix-email-comment-permissions-check.yml new file mode 100644 index 00000000000..2f76064d8a4 --- /dev/null +++ b/changelogs/unreleased/security-2779-fix-email-comment-permissions-check.yml @@ -0,0 +1,5 @@ +--- +title: Prevent unauthorized replies when discussion is locked or confidential +merge_request: +author: +type: security diff --git a/lib/gitlab/email/handler/reply_processing.rb b/lib/gitlab/email/handler/reply_processing.rb index ff6b2c729b2..4013d2736ce 100644 --- a/lib/gitlab/email/handler/reply_processing.rb +++ b/lib/gitlab/email/handler/reply_processing.rb @@ -43,7 +43,7 @@ module Gitlab raise ProjectNotFound unless author.can?(:read_project, project) end - raise UserNotAuthorizedError unless author.can?(permission, project || noteable) + raise UserNotAuthorizedError unless author.can?(permission, try(:noteable) || project) end def verify_record!(record:, invalid_exception:, record_name:) diff --git a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb index b1f48c15c21..e5420ea6bea 100644 --- a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb +++ b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb @@ -118,6 +118,43 @@ describe Gitlab::Email::Handler::CreateNoteHandler do 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 before do setup_attachment diff --git a/spec/models/sent_notification_spec.rb b/spec/models/sent_notification_spec.rb index 5ec04b99957..677613b7980 100644 --- a/spec/models/sent_notification_spec.rb +++ b/spec/models/sent_notification_spec.rb @@ -48,7 +48,7 @@ describe SentNotification do let(:note) { create(:diff_note_on_merge_request) } 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 diff --git a/spec/policies/personal_snippet_policy_spec.rb b/spec/policies/personal_snippet_policy_spec.rb index 3809692b373..d7036cc4171 100644 --- a/spec/policies/personal_snippet_policy_spec.rb +++ b/spec/policies/personal_snippet_policy_spec.rb @@ -14,6 +14,13 @@ describe PersonalSnippetPolicy do ] end + let(:comment_permissions) do + [ + :comment_personal_snippet, + :create_note + ] + end + def permissions(user) described_class.new(user, snippet) end @@ -26,7 +33,7 @@ describe PersonalSnippetPolicy do it do 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(*author_permissions) end @@ -37,7 +44,7 @@ describe PersonalSnippetPolicy do it do 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_disallowed(*author_permissions) end @@ -48,7 +55,7 @@ describe PersonalSnippetPolicy do it do 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(*author_permissions) end @@ -63,7 +70,7 @@ describe PersonalSnippetPolicy do it do 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(*author_permissions) end @@ -74,7 +81,7 @@ describe PersonalSnippetPolicy do it do 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_disallowed(*author_permissions) end @@ -85,7 +92,7 @@ describe PersonalSnippetPolicy do it do 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(*author_permissions) end @@ -96,7 +103,7 @@ describe PersonalSnippetPolicy do it do 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(*author_permissions) end @@ -111,7 +118,7 @@ describe PersonalSnippetPolicy do it do 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(*author_permissions) end @@ -122,7 +129,7 @@ describe PersonalSnippetPolicy do it do 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(*author_permissions) end @@ -133,7 +140,7 @@ describe PersonalSnippetPolicy do it do 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(*author_permissions) end @@ -144,7 +151,7 @@ describe PersonalSnippetPolicy do it do 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(*author_permissions) end diff --git a/spec/policies/project_snippet_policy_spec.rb b/spec/policies/project_snippet_policy_spec.rb index 4d32e06b553..d6329e84579 100644 --- a/spec/policies/project_snippet_policy_spec.rb +++ b/spec/policies/project_snippet_policy_spec.rb @@ -41,7 +41,7 @@ describe ProjectSnippetPolicy do subject { abilities(regular_user, :public) } it do - expect_allowed(:read_project_snippet) + expect_allowed(:read_project_snippet, :create_note) expect_disallowed(*author_permissions) end end @@ -50,7 +50,7 @@ describe ProjectSnippetPolicy do subject { abilities(external_user, :public) } it do - expect_allowed(:read_project_snippet) + expect_allowed(:read_project_snippet, :create_note) expect_disallowed(*author_permissions) end end @@ -70,7 +70,7 @@ describe ProjectSnippetPolicy do subject { abilities(regular_user, :internal) } it do - expect_allowed(:read_project_snippet) + expect_allowed(:read_project_snippet, :create_note) expect_disallowed(*author_permissions) end end @@ -79,7 +79,7 @@ describe ProjectSnippetPolicy do subject { abilities(external_user, :internal) } it do - expect_disallowed(:read_project_snippet) + expect_disallowed(:read_project_snippet, :create_note) expect_disallowed(*author_permissions) end end @@ -92,7 +92,7 @@ describe ProjectSnippetPolicy do end it do - expect_allowed(:read_project_snippet) + expect_allowed(:read_project_snippet, :create_note) expect_disallowed(*author_permissions) end end @@ -112,7 +112,7 @@ describe ProjectSnippetPolicy do subject { abilities(regular_user, :private) } it do - expect_disallowed(:read_project_snippet) + expect_disallowed(:read_project_snippet, :create_note) expect_disallowed(*author_permissions) end end @@ -123,7 +123,7 @@ describe ProjectSnippetPolicy do subject { described_class.new(regular_user, snippet) } it do - expect_allowed(:read_project_snippet) + expect_allowed(:read_project_snippet, :create_note) expect_allowed(*author_permissions) end end @@ -136,7 +136,7 @@ describe ProjectSnippetPolicy do end it do - expect_allowed(:read_project_snippet) + expect_allowed(:read_project_snippet, :create_note) expect_disallowed(*author_permissions) end end @@ -149,7 +149,7 @@ describe ProjectSnippetPolicy do end it do - expect_allowed(:read_project_snippet) + expect_allowed(:read_project_snippet, :create_note) expect_disallowed(*author_permissions) end end @@ -158,7 +158,7 @@ describe ProjectSnippetPolicy do subject { abilities(create(:admin), :private) } it do - expect_allowed(:read_project_snippet) + expect_allowed(:read_project_snippet, :create_note) expect_allowed(*author_permissions) end end diff --git a/spec/services/notes/build_service_spec.rb b/spec/services/notes/build_service_spec.rb index ff85c261cd4..9aaccb4bffe 100644 --- a/spec/services/notes/build_service_spec.rb +++ b/spec/services/notes/build_service_spec.rb @@ -45,6 +45,15 @@ describe Notes::BuildService do 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 def reply(note, user = nil) user ||= create(:user) diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index 80b015d4cd0..1b9ba42cfd6 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -127,6 +127,10 @@ describe Notes::CreateService do create(:diff_note_on_merge_request, noteable: merge_request, project: project_with_repo) end + before do + project_with_repo.add_maintainer(user) + end + context 'when eligible to have a note diff file' do let(:new_opts) do opts.merge(in_reply_to_discussion_id: nil, |