From 267a909600e02f0728fec1765adf817acc03d813 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Mon, 2 Apr 2018 19:05:47 +0200 Subject: [PATCH] Remove edit_note and update_note abilities in favor of admin_note --- app/helpers/notes_helper.rb | 4 -- app/models/ability.rb | 4 -- app/policies/issuable_policy.rb | 2 - app/policies/note_policy.rb | 6 +-- app/serializers/note_entity.rb | 2 +- app/views/shared/notes/_note.html.haml | 2 +- spec/models/ability_spec.rb | 56 -------------------------- spec/policies/note_policy_spec.rb | 4 -- 8 files changed, 3 insertions(+), 77 deletions(-) diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index 27ed48fdbc7..7f67574a428 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -6,10 +6,6 @@ module NotesHelper end end - def note_editable?(note) - Ability.can_edit_note?(current_user, note) - end - def note_supports_quick_actions?(note) Notes::QuickActionsService.supported?(note) end diff --git a/app/models/ability.rb b/app/models/ability.rb index 6dae49f38dc..618d4af4272 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -46,10 +46,6 @@ class Ability end end - def can_edit_note?(user, note) - allowed?(user, :edit_note, note) - end - def allowed?(user, action, subject = :global, opts = {}) if subject.is_a?(Hash) opts, subject = subject, :global diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb index e86d1c8f98e..b431d376e3d 100644 --- a/app/policies/issuable_policy.rb +++ b/app/policies/issuable_policy.rb @@ -18,9 +18,7 @@ class IssuablePolicy < BasePolicy rule { locked & ~is_project_member }.policy do prevent :create_note - prevent :update_note prevent :admin_note prevent :resolve_note - prevent :edit_note end end diff --git a/app/policies/note_policy.rb b/app/policies/note_policy.rb index d4cb5a77e63..2f6c579906c 100644 --- a/app/policies/note_policy.rb +++ b/app/policies/note_policy.rb @@ -8,14 +8,10 @@ class NotePolicy < BasePolicy condition(:editable, scope: :subject) { @subject.editable? } - rule { ~editable | anonymous }.prevent :edit_note - - rule { is_author | admin }.enable :edit_note - rule { can?(:master_access) }.enable :edit_note + rule { ~editable }.prevent :admin_note rule { is_author }.policy do enable :read_note - enable :update_note enable :admin_note enable :resolve_note end diff --git a/app/serializers/note_entity.rb b/app/serializers/note_entity.rb index c964aa9c99b..fa516fee6ec 100644 --- a/app/serializers/note_entity.rb +++ b/app/serializers/note_entity.rb @@ -15,7 +15,7 @@ class NoteEntity < API::Entities::Note expose :current_user do expose :can_edit do |note| - Ability.can_edit_note?(request.current_user, note) + Ability.allowed?(request.current_user, :admin_note, note) end end diff --git a/app/views/shared/notes/_note.html.haml b/app/views/shared/notes/_note.html.haml index bf359774ead..893a7f26ebd 100644 --- a/app/views/shared/notes/_note.html.haml +++ b/app/views/shared/notes/_note.html.haml @@ -2,7 +2,7 @@ - return if note.cross_reference_not_visible_for?(current_user) - show_image_comment_badge = local_assigns.fetch(:show_image_comment_badge, false) -- note_editable = note_editable?(note) +- note_editable = can?(current_user, :admin_note, note) - note_counter = local_assigns.fetch(:note_counter, 0) %li.timeline-entry{ id: dom_id(note), diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index cd175dba6da..199f49d0bf2 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -7,62 +7,6 @@ describe Ability do end end - describe '.can_edit_note?' do - let(:project) { create(:project) } - let(:note) { create(:note_on_issue, project: project) } - - context 'using an anonymous user' do - it 'returns false' do - expect(described_class.can_edit_note?(nil, note)).to be_falsy - end - end - - context 'using a system note' do - it 'returns false' do - system_note = create(:note, system: true) - user = create(:user) - - expect(described_class.can_edit_note?(user, system_note)).to be_falsy - end - end - - context 'using users with different access levels' do - let(:user) { create(:user) } - - it 'returns true for the author' do - expect(described_class.can_edit_note?(note.author, note)).to be_truthy - end - - it 'returns false for a guest user' do - project.add_guest(user) - - expect(described_class.can_edit_note?(user, note)).to be_falsy - end - - it 'returns false for a developer' do - project.add_developer(user) - - expect(described_class.can_edit_note?(user, note)).to be_falsy - end - - it 'returns true for a master' do - project.add_master(user) - - expect(described_class.can_edit_note?(user, note)).to be_truthy - end - - it 'returns true for a group owner' do - group = create(:group) - project.project_group_links.create( - group: group, - group_access: Gitlab::Access::MASTER) - group.add_owner(user) - - expect(described_class.can_edit_note?(user, note)).to be_truthy - end - end - end - describe '.users_that_can_read_project' do context 'using a public project' do it 'returns all the users' do diff --git a/spec/policies/note_policy_spec.rb b/spec/policies/note_policy_spec.rb index 58d36a2c84e..e8096358f7d 100644 --- a/spec/policies/note_policy_spec.rb +++ b/spec/policies/note_policy_spec.rb @@ -18,7 +18,6 @@ describe NotePolicy, mdoels: true do context 'when the project is public' do context 'when the note author is not a project member' do it 'can edit a note' do - expect(policies).to be_allowed(:update_note) expect(policies).to be_allowed(:admin_note) expect(policies).to be_allowed(:resolve_note) expect(policies).to be_allowed(:read_note) @@ -29,7 +28,6 @@ describe NotePolicy, mdoels: true do it 'can edit note' do policies = policies(create(:project_snippet, project: project)) - expect(policies).to be_allowed(:update_note) expect(policies).to be_allowed(:admin_note) expect(policies).to be_allowed(:resolve_note) expect(policies).to be_allowed(:read_note) @@ -47,7 +45,6 @@ describe NotePolicy, mdoels: true do end it 'can edit a note' do - expect(policies).to be_allowed(:update_note) expect(policies).to be_allowed(:admin_note) expect(policies).to be_allowed(:resolve_note) expect(policies).to be_allowed(:read_note) @@ -56,7 +53,6 @@ describe NotePolicy, mdoels: true do context 'when the note author is not a project member' do it 'can not edit a note' do - expect(policies).to be_disallowed(:update_note) expect(policies).to be_disallowed(:admin_note) expect(policies).to be_disallowed(:resolve_note) end