diff --git a/app/assets/javascripts/notes/components/comment_form.vue b/app/assets/javascripts/notes/components/comment_form.vue index 87b55b19c08afc341f950b94224ea7f53cd4bfcc..fc6f0e0ecf7430d054545d98985e228f38cbba53 100644 --- a/app/assets/javascripts/notes/components/comment_form.vue +++ b/app/assets/javascripts/notes/components/comment_form.vue @@ -117,10 +117,7 @@ export default { return this.getNoteableData.current_user.can_create_note; }, canSetInternalNote() { - return ( - this.getNoteableData.current_user.can_create_confidential_note && - (this.isIssue || this.isEpic) - ); + return this.getNoteableData.current_user.can_create_confidential_note; }, issueActionButtonTitle() { const openOrClose = this.isOpen ? 'close' : 'reopen'; diff --git a/app/models/note.rb b/app/models/note.rb index 953632ba9109cf467842f77d5f6f5c87810c2004..10ab85b24b75aee9e284fb780325d9f29043dea3 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -935,7 +935,7 @@ def ensure_note_type_can_be_confidential end def noteable_can_have_confidential_note? - for_issue? + for_issuable? end def set_internal_flag diff --git a/app/policies/merge_request_policy.rb b/app/policies/merge_request_policy.rb index 49f9225a1d36cacbac2152f7b5cae70cb3291d5c..27bcd816a08e8fd06da34eb9a12e6be3e2afd5e7 100644 --- a/app/policies/merge_request_policy.rb +++ b/app/policies/merge_request_policy.rb @@ -14,6 +14,7 @@ class MergeRequestPolicy < IssuablePolicy rule { ~can?(:read_merge_request) }.policy do prevent :create_note prevent :accept_merge_request + prevent :mark_note_as_internal end rule { can_approve }.policy do @@ -43,6 +44,10 @@ class MergeRequestPolicy < IssuablePolicy enable :set_merge_request_metadata end + rule { can?(:reporter_access) }.policy do + enable :mark_note_as_internal + end + private def can_approve? diff --git a/app/serializers/merge_request_noteable_entity.rb b/app/serializers/merge_request_noteable_entity.rb index 04b801e29adb20e907c4a401acfec4005d3138ca..4122cb5f9a4543f0563639f2bc01057d5ece7a33 100644 --- a/app/serializers/merge_request_noteable_entity.rb +++ b/app/serializers/merge_request_noteable_entity.rb @@ -49,6 +49,10 @@ class MergeRequestNoteableEntity < IssuableEntity expose :can_update do |merge_request| can?(current_user, :update_merge_request, merge_request) end + + expose :can_create_confidential_note do |merge_request| + can?(request.current_user, :mark_note_as_internal, merge_request) + end end expose :locked_discussion_docs_path, if: -> (merge_request) { merge_request.discussion_locked? } do |merge_request| diff --git a/ee/app/models/ee/note.rb b/ee/app/models/ee/note.rb index 12075ad57d1143648bd7ae10b9b9029fafc583e4..85a74bff2dc67ae616c06454be048d75533a90e9 100644 --- a/ee/app/models/ee/note.rb +++ b/ee/app/models/ee/note.rb @@ -132,10 +132,5 @@ def group_reporter?(user, group) def banzai_context_params { group: noteable.group, label_url_method: :group_epics_url } end - - override :noteable_can_have_confidential_note? - def noteable_can_have_confidential_note? - for_epic? || super - end end end diff --git a/ee/spec/fixtures/api/schemas/entities/merge_request_noteable.json b/ee/spec/fixtures/api/schemas/entities/merge_request_noteable.json index 6be0ae47f3f55d18207969ccf32faaf75bdd93b0..1c78345b5a27edd640ea2c84637683ae14707d60 100644 --- a/ee/spec/fixtures/api/schemas/entities/merge_request_noteable.json +++ b/ee/spec/fixtures/api/schemas/entities/merge_request_noteable.json @@ -24,11 +24,13 @@ "type": "object", "required": [ "can_create_note", - "can_update" + "can_update", + "can_create_confidential_note" ], "properties": { "can_create_note": { "type": "boolean" }, - "can_update": { "type": "boolean" } + "can_update": { "type": "boolean" }, + "can_create_confidential_note": { "type": "boolean" } }, "additionalProperties": false }, diff --git a/ee/spec/models/note_spec.rb b/ee/spec/models/note_spec.rb index b8e282d91867b9fb1dd7a1cfd7a8dd9180effc77..8355dfb7baa6c7914ad29633ac36662e17e3b9f6 100644 --- a/ee/spec/models/note_spec.rb +++ b/ee/spec/models/note_spec.rb @@ -13,6 +13,24 @@ let(:set_mentionable_text) { ->(txt) { subject.note = txt } } end + describe 'validation' do + describe 'confidentiality' do + context 'for a new note' do + let(:note_params) { { confidential: true, noteable: noteable, project: noteable.project } } + + subject(:note) { build(:note, **note_params) } + + context 'when noteable is a epic' do + let_it_be(:noteable) { create(:epic) } + + it 'can not be set confidential' do + expect(note).to be_valid + end + end + end + end + end + describe '#ensure_namespace_id' do context 'for an epic note' do let_it_be(:epic) { create(:epic) } diff --git a/spec/fixtures/api/schemas/entities/merge_request_noteable.json b/spec/fixtures/api/schemas/entities/merge_request_noteable.json index 6f3c29b16e9ef527feee7411c7ba7ea8652b9ac4..2e0d7f998a24e825edf94d1049a7311406c43c05 100644 --- a/spec/fixtures/api/schemas/entities/merge_request_noteable.json +++ b/spec/fixtures/api/schemas/entities/merge_request_noteable.json @@ -24,11 +24,13 @@ "type": "object", "required": [ "can_create_note", - "can_update" + "can_update", + "can_create_confidential_note" ], "properties": { "can_create_note": { "type": "boolean" }, - "can_update": { "type": "boolean" } + "can_update": { "type": "boolean" }, + "can_create_confidential_note": { "type": "boolean" } }, "additionalProperties": false }, diff --git a/spec/frontend/notes/components/comment_form_spec.js b/spec/frontend/notes/components/comment_form_spec.js index 8f761476c7c99ed7813526457f507939457be11e..849490af554b0180911579147c34aa409381c07c 100644 --- a/spec/frontend/notes/components/comment_form_spec.js +++ b/spec/frontend/notes/components/comment_form_spec.js @@ -619,7 +619,7 @@ describe('issue_comment_form component', () => { noteableType | rendered | message ${'Issue'} | ${true} | ${'render'} ${'Epic'} | ${true} | ${'render'} - ${'MergeRequest'} | ${false} | ${'not render'} + ${'MergeRequest'} | ${true} | ${'render'} `( 'should $message checkbox when noteableType is $noteableType', ({ noteableType, rendered }) => { diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 597950596422e30363f51a069d55ed019f7116b2..8f755e4741b7efc63558071fd990f1a8a5ecc7bc 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -176,9 +176,17 @@ expect(subject).to be_valid end - context 'when noteable is not allowed to have confidential notes' do + context 'when noteable is a merge request' do let_it_be(:noteable) { create(:merge_request) } + it 'can not be set confidential' do + expect(subject).to be_valid + end + end + + context 'when noteable is not allowed to have confidential notes' do + let_it_be(:noteable) { create(:snippet) } + it 'can not be set confidential' do expect(subject).not_to be_valid expect(subject.errors[:confidential]).to include('can not be set for this resource') diff --git a/spec/policies/merge_request_policy_spec.rb b/spec/policies/merge_request_policy_spec.rb index c21e1244402d4fd544f7feb6be63f3f5c266bdab..12b38fe1d93216d67883f3e23ca770679260c13c 100644 --- a/spec/policies/merge_request_policy_spec.rb +++ b/spec/policies/merge_request_policy_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe MergeRequestPolicy do +RSpec.describe MergeRequestPolicy, feature_category: :code_review_workflow do include ExternalAuthorizationServiceHelpers let_it_be(:guest) { create(:user) } @@ -23,7 +23,8 @@ def permissions(user, merge_request) create_todo approve_merge_request create_note - update_subscription].freeze + update_subscription + mark_note_as_internal].freeze shared_examples_for 'a denied user' do let(:perms) { permissions(subject, merge_request) } @@ -47,6 +48,7 @@ def permissions(user, merge_request) :create_merge_request_from | false :approve_merge_request | false :update_merge_request | false + :mark_note_as_internal | true end with_them do