Commit b0e37232 authored by Cindy Pallares's avatar Cindy Pallares

Merge branch 'security-guest-comments' into 'master'

[master]Fixed ability to comment on and edit/delete comments on locked or confidential issues

See merge request gitlab/gitlabhq!2612
parents 163469e9 387a9c4e
...@@ -6,6 +6,7 @@ module NotesActions ...@@ -6,6 +6,7 @@ module NotesActions
extend ActiveSupport::Concern extend ActiveSupport::Concern
included do included do
prepend_before_action :normalize_create_params, only: [:create]
before_action :set_polling_interval_header, only: [:index] before_action :set_polling_interval_header, only: [:index]
before_action :require_noteable!, only: [:index, :create] before_action :require_noteable!, only: [:index, :create]
before_action :authorize_admin_note!, only: [:update, :destroy] before_action :authorize_admin_note!, only: [:update, :destroy]
...@@ -247,6 +248,15 @@ module NotesActions ...@@ -247,6 +248,15 @@ module NotesActions
DiscussionSerializer.new(project: project, noteable: noteable, current_user: current_user, note_entity: ProjectNoteEntity) DiscussionSerializer.new(project: project, noteable: noteable, current_user: current_user, note_entity: ProjectNoteEntity)
end end
# Avoids checking permissions in the wrong object - this ensures that the object we checked permissions for
# is the object we're actually creating a note in.
def normalize_create_params
params[:note].try do |note|
note[:noteable_id] = params[:target_id]
note[:noteable_type] = params[:target_type].classify
end
end
def note_project def note_project
strong_memoize(:note_project) do strong_memoize(:note_project) do
next nil unless project next nil unless project
......
...@@ -26,7 +26,7 @@ module Emails ...@@ -26,7 +26,7 @@ module Emails
mail_answer_note_thread(@merge_request, @note, note_thread_options(recipient_id)) mail_answer_note_thread(@merge_request, @note, note_thread_options(recipient_id))
end end
def note_snippet_email(recipient_id, note_id) def note_project_snippet_email(recipient_id, note_id)
setup_note_mail(note_id, recipient_id) setup_note_mail(note_id, recipient_id)
@snippet = @note.noteable @snippet = @note.noteable
......
...@@ -324,7 +324,7 @@ class Note < ActiveRecord::Base ...@@ -324,7 +324,7 @@ class Note < ActiveRecord::Base
end end
def to_ability_name def to_ability_name
for_personal_snippet? ? 'personal_snippet' : noteable_type.underscore for_snippet? ? noteable.class.name.underscore : noteable_type.underscore
end end
def can_be_discussion_note? def can_be_discussion_note?
......
...@@ -2,4 +2,6 @@ ...@@ -2,4 +2,6 @@
class CommitPolicy < BasePolicy class CommitPolicy < BasePolicy
delegate { @subject.project } delegate { @subject.project }
rule { can?(:download_code) }.enable :read_commit
end end
...@@ -9,8 +9,17 @@ class NotePolicy < BasePolicy ...@@ -9,8 +9,17 @@ class NotePolicy < BasePolicy
condition(:editable, scope: :subject) { @subject.editable? } condition(:editable, scope: :subject) { @subject.editable? }
condition(:can_read_noteable) { can?(:"read_#{@subject.to_ability_name}") }
rule { ~editable }.prevent :admin_note rule { ~editable }.prevent :admin_note
# If user can't read the issue/MR/etc then they should not be allowed to do anything to their own notes
rule { ~can_read_noteable }.policy do
prevent :read_note
prevent :admin_note
prevent :resolve_note
end
rule { is_author }.policy do rule { is_author }.policy do
enable :read_note enable :read_note
enable :admin_note enable :admin_note
......
---
title: Fixed ability to comment on locked/confidential issues.
merge_request:
author:
type: security
---
title: Fixed ability of guest users to edit/delete comments on locked or confidential issues.
merge_request:
author:
type: security
...@@ -283,14 +283,14 @@ describe Projects::NotesController do ...@@ -283,14 +283,14 @@ describe Projects::NotesController do
def post_create(extra_params = {}) def post_create(extra_params = {})
post :create, { post :create, {
note: { note: 'some other note' }, note: { note: 'some other note', noteable_id: merge_request.id },
namespace_id: project.namespace, namespace_id: project.namespace,
project_id: project, project_id: project,
target_type: 'merge_request', target_type: 'merge_request',
target_id: merge_request.id, target_id: merge_request.id,
note_project_id: forked_project.id, note_project_id: forked_project.id,
in_reply_to_discussion_id: existing_comment.discussion_id in_reply_to_discussion_id: existing_comment.discussion_id
}.merge(extra_params) }.merge(extra_params)
end end
context 'when the note_project_id is not correct' do context 'when the note_project_id is not correct' do
...@@ -324,6 +324,30 @@ describe Projects::NotesController do ...@@ -324,6 +324,30 @@ describe Projects::NotesController do
end end
end end
context 'when target_id and noteable_id do not match' do
let(:locked_issue) { create(:issue, :locked, project: project) }
let(:issue) {create(:issue, project: project)}
before do
project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC)
project.project_member(user).destroy
end
it 'uses target_id and ignores noteable_id' do
request_params = {
note: { note: 'some note', noteable_type: 'Issue', noteable_id: locked_issue.id },
target_type: 'issue',
target_id: issue.id,
project_id: project,
namespace_id: project.namespace
}
expect { post :create, request_params }.to change { issue.notes.count }.by(1)
.and change { locked_issue.notes.count }.by(0)
expect(response).to have_gitlab_http_status(302)
end
end
context 'when the merge request discussion is locked' do context 'when the merge request discussion is locked' do
before do before do
project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC) project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC)
...@@ -376,35 +400,60 @@ describe Projects::NotesController do ...@@ -376,35 +400,60 @@ describe Projects::NotesController do
end end
describe 'PUT update' do describe 'PUT update' do
let(:request_params) do context "should update the note with a valid issue" do
{ let(:request_params) do
namespace_id: project.namespace, {
project_id: project, namespace_id: project.namespace,
id: note, project_id: project,
format: :json, id: note,
note: { format: :json,
note: "New comment" note: {
note: "New comment"
}
} }
} end
end
before do before do
sign_in(note.author) sign_in(note.author)
project.add_developer(note.author) project.add_developer(note.author)
end
it "updates the note" do
expect { put :update, request_params }.to change { note.reload.note }
end
end end
context "doesnt update the note" do
let(:issue) { create(:issue, :confidential, project: project) }
let(:note) { create(:note, noteable: issue, project: project) }
it "updates the note" do before do
expect { put :update, request_params }.to change { note.reload.note } sign_in(user)
project.add_guest(user)
end
it "disallows edits when the issue is confidential and the user has guest permissions" do
request_params = {
namespace_id: project.namespace,
project_id: project,
id: note,
format: :json,
note: {
note: "New comment"
}
}
expect { put :update, request_params }.not_to change { note.reload.note }
expect(response).to have_gitlab_http_status(404)
end
end end
end end
describe 'DELETE destroy' do describe 'DELETE destroy' do
let(:request_params) do let(:request_params) do
{ {
namespace_id: project.namespace, namespace_id: project.namespace,
project_id: project, project_id: project,
id: note, id: note,
format: :js format: :js
} }
end end
......
...@@ -522,7 +522,7 @@ describe Notify do ...@@ -522,7 +522,7 @@ describe Notify do
let(:project_snippet) { create(:project_snippet, project: project) } let(:project_snippet) { create(:project_snippet, project: project) }
let(:project_snippet_note) { create(:note_on_project_snippet, project: project, noteable: project_snippet) } let(:project_snippet_note) { create(:note_on_project_snippet, project: project, noteable: project_snippet) }
subject { described_class.note_snippet_email(project_snippet_note.author_id, project_snippet_note.id) } subject { described_class.note_project_snippet_email(project_snippet_note.author_id, project_snippet_note.id) }
it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
let(:model) { project_snippet } let(:model) { project_snippet }
......
...@@ -517,7 +517,7 @@ describe Note do ...@@ -517,7 +517,7 @@ describe Note do
describe '#to_ability_name' do describe '#to_ability_name' do
it 'returns snippet for a project snippet note' do it 'returns snippet for a project snippet note' do
expect(build(:note_on_project_snippet).to_ability_name).to eq('snippet') expect(build(:note_on_project_snippet).to_ability_name).to eq('project_snippet')
end end
it 'returns personal_snippet for a personal snippet note' do it 'returns personal_snippet for a personal snippet note' do
......
...@@ -10,11 +10,50 @@ describe NotePolicy, mdoels: true do ...@@ -10,11 +10,50 @@ describe NotePolicy, mdoels: true do
return @policies if @policies return @policies if @policies
noteable ||= issue noteable ||= issue
note = create(:note, noteable: noteable, author: user, project: project) note = if noteable.is_a?(Commit)
create(:note_on_commit, commit_id: noteable.id, author: user, project: project)
else
create(:note, noteable: noteable, author: user, project: project)
end
@policies = described_class.new(user, note) @policies = described_class.new(user, note)
end end
shared_examples_for 'a discussion with a private noteable' do
let(:noteable) { issue }
let(:policy) { policies(noteable) }
context 'when the note author can no longer see the noteable' do
it 'can not edit nor read the note' do
expect(policy).to be_disallowed(:admin_note)
expect(policy).to be_disallowed(:resolve_note)
expect(policy).to be_disallowed(:read_note)
end
end
context 'when the note author can still see the noteable' do
before do
project.add_developer(user)
end
it 'can edit the note' do
expect(policy).to be_allowed(:admin_note)
expect(policy).to be_allowed(:resolve_note)
expect(policy).to be_allowed(:read_note)
end
end
end
context 'when the project is private' do
let(:project) { create(:project, :private, :repository) }
context 'when the noteable is a commit' do
it_behaves_like 'a discussion with a private noteable' do
let(:noteable) { project.repository.head_commit }
end
end
end
context 'when the project is public' do context 'when the project is public' do
context 'when the note author is not a project member' do context 'when the note author is not a project member' do
it 'can edit a note' do it 'can edit a note' do
...@@ -24,14 +63,48 @@ describe NotePolicy, mdoels: true do ...@@ -24,14 +63,48 @@ describe NotePolicy, mdoels: true do
end end
end end
context 'when the noteable is a snippet' do context 'when the noteable is a project snippet' do
it 'can edit note' do
policies = policies(create(:project_snippet, :public, project: project))
expect(policies).to be_allowed(:admin_note)
expect(policies).to be_allowed(:resolve_note)
expect(policies).to be_allowed(:read_note)
end
context 'when it is private' do
it_behaves_like 'a discussion with a private noteable' do
let(:noteable) { create(:project_snippet, :private, project: project) }
end
end
end
context 'when the noteable is a personal snippet' do
it 'can edit note' do it 'can edit note' do
policies = policies(create(:project_snippet, project: project)) policies = policies(create(:personal_snippet, :public))
expect(policies).to be_allowed(:admin_note) expect(policies).to be_allowed(:admin_note)
expect(policies).to be_allowed(:resolve_note) expect(policies).to be_allowed(:resolve_note)
expect(policies).to be_allowed(:read_note) expect(policies).to be_allowed(:read_note)
end end
context 'when it is private' do
it 'can not edit nor read the note' do
policies = policies(create(:personal_snippet, :private))
expect(policies).to be_disallowed(:admin_note)
expect(policies).to be_disallowed(:resolve_note)
expect(policies).to be_disallowed(:read_note)
end
end
end
context 'when a discussion is confidential' do
before do
issue.update_attribute(:confidential, true)
end
it_behaves_like 'a discussion with a private noteable'
end end
context 'when a discussion is locked' do context 'when a discussion is locked' do
......
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