Merge branch '9023-fix-commenting-in-epics-permissions' into 'master'

Fix commenting on epics permissions

Closes #9023

See merge request gitlab-org/gitlab-ee!9783

ee/app/policies/epic_policy.rb

@@ -8,7 +8,7 @@ class EpicPolicy < BasePolicy
     enable :read_note
   end
 
-  rule { can?(:update_epic) }.policy do
+  rule { can?(:read_epic) & ~anonymous }.policy do
     enable :create_note
   end
 

ee/changelogs/unreleased/9023-fix-commenting-in-epics-permissions.yml

+---
+title: Allow guests to comment on epics
+merge_request: 9783
+author:
+type: added

ee/spec/policies/epic_policy_spec.rb

@@ -2,137 +2,136 @@ require 'spec_helper'
 
 describe EpicPolicy do
   include ExternalAuthorizationServiceHelpers
+
   let(:user) { create(:user) }
+  let(:epic) { create(:epic, group: group) }
 
-  def permissions(user, group)
-    epic = create(:epic, group: group)
+  subject { described_class.new(user, epic) }
 
-    described_class.new(user, epic)
+  shared_examples 'can comment on epics' do
+    it { is_expected.to be_allowed(:create_note, :award_emoji) }
   end
 
-  context 'when epics feature is disabled' do
-    let(:group) { create(:group, :public) }
-
-    it 'no one can read epics' do
-      group.add_owner(user)
-
-      expect(permissions(user, group))
-        .to be_disallowed(:read_epic, :read_epic_iid, :update_epic, :destroy_epic, :admin_epic, :create_epic)
-    end
+  shared_examples 'cannot comment on epics' do
+    it { is_expected.to be_disallowed(:create_note, :award_emoji) }
   end
 
-  context 'when epics feature is enabled' do
-    before do
-      stub_licensed_features(epics: true)
+  shared_examples 'can only read epics' do
+    it do
+      is_expected.to be_allowed(:read_epic, :read_epic_iid)
+      is_expected.to be_disallowed(:update_epic, :destroy_epic, :admin_epic, :create_epic)
+    end
   end
 
-    context 'when an epic is in a private group' do
-      let(:group) { create(:group, :private) }
-
-      it 'anonymous user can not read epics' do
-        expect(permissions(nil, group))
-          .to be_disallowed(:read_epic, :read_epic_iid, :update_epic, :destroy_epic, :admin_epic, :create_epic)
+  shared_examples 'can manage epics' do
+    it { is_expected.to be_allowed(:read_epic, :read_epic_iid, :update_epic, :admin_epic, :create_epic) }
   end
 
-      it 'user who is not a group member can not read epics' do
-        expect(permissions(user, group))
-          .to be_disallowed(:read_epic, :read_epic_iid, :update_epic, :destroy_epic, :admin_epic, :create_epic)
+  shared_examples 'all epic permissions disabled' do
+    it { is_expected.to be_disallowed(:read_epic, :read_epic_iid, :update_epic, :destroy_epic, :admin_epic, :create_epic, :create_note, :award_emoji) }
   end
 
-      it 'guest group member can only read epics' do
+  shared_examples 'group member permissions' do
+    context 'guest group member' do
+      before do
         group.add_guest(user)
+      end
 
-        expect(permissions(user, group)).to be_allowed(:read_epic, :read_epic_iid)
-        expect(permissions(user, group)).to be_disallowed(:update_epic, :destroy_epic, :admin_epic, :create_epic)
+      it_behaves_like 'can only read epics'
+      it_behaves_like 'can comment on epics'
     end
 
-      it 'reporter group member can manage epics' do
+    context 'reporter group member' do
+      before do
         group.add_reporter(user)
-
-        expect(permissions(user, group)).to be_disallowed(:destroy_epic)
-        expect(permissions(user, group))
-          .to be_allowed(:read_epic, :read_epic_iid, :update_epic, :admin_epic, :create_epic)
       end
 
-      it 'only group owner can destroy epics' do
-        group.add_owner(user)
+      it_behaves_like 'can manage epics'
+      it_behaves_like 'can comment on epics'
 
-        expect(permissions(user, group))
-          .to be_allowed(:read_epic, :read_epic_iid, :update_epic, :destroy_epic, :admin_epic, :create_epic)
+      it 'cannot destroy epics' do
+        is_expected.to be_disallowed(:destroy_epic)
       end
     end
 
-    context 'when an epic is in an internal group' do
-      let(:group) { create(:group, :internal) }
-
-      it 'anonymous user can not read epics' do
-        expect(permissions(nil, group))
-          .to be_disallowed(:read_epic, :read_epic_iid, :update_epic, :destroy_epic, :admin_epic, :create_epic)
+    context 'group owner' do
+      before do
+        group.add_owner(user)
       end
 
-      it 'user who is not a group member can only read epics' do
-        expect(permissions(user, group)).to be_allowed(:read_epic, :read_epic_iid)
-        expect(permissions(user, group)).to be_disallowed(:update_epic, :destroy_epic, :admin_epic, :create_epic)
+      it_behaves_like 'can manage epics'
+      it_behaves_like 'can comment on epics'
+
+      it 'can destroy epics' do
+        is_expected.to be_allowed(:destroy_epic)
+      end
+    end
   end
 
-      it 'guest group member can only read epics' do
-        group.add_guest(user)
+  context 'when epics feature is disabled' do
+    let(:group) { create(:group, :public) }
 
-        expect(permissions(user, group)).to be_allowed(:read_epic, :read_epic_iid)
-        expect(permissions(user, group)).to be_disallowed(:update_epic, :destroy_epic, :admin_epic, :create_epic)
+    before do
+      group.add_owner(user)
     end
 
-      it 'reporter group member can manage epics' do
-        group.add_reporter(user)
+    it_behaves_like 'all epic permissions disabled'
+  end
 
-        expect(permissions(user, group)).to be_disallowed(:destroy_epic)
-        expect(permissions(user, group))
-          .to be_allowed(:read_epic, :read_epic_iid, :update_epic, :admin_epic, :create_epic)
+  context 'when epics feature is enabled' do
+    before do
+      stub_licensed_features(epics: true)
     end
 
-      it 'only group owner can destroy epics' do
-        group.add_owner(user)
+    context 'when an epic is in a private group' do
+      let(:group) { create(:group, :private) }
 
-        expect(permissions(user, group))
-          .to be_allowed(:read_epic, :read_epic_iid, :update_epic, :destroy_epic, :admin_epic, :create_epic)
-      end
-    end
+      context 'anonymous user' do
+        let(:user) { nil }
 
-    context 'when an epic is in a public group' do
-      let(:group) { create(:group, :public) }
+        it_behaves_like 'all epic permissions disabled'
+      end
 
-      it 'anonymous user can only read epics' do
-        expect(permissions(nil, group)).to be_allowed(:read_epic, :read_epic_iid)
-        expect(permissions(nil, group)).to be_disallowed(:update_epic, :destroy_epic, :admin_epic, :create_epic)
+      context 'user who is not a group member' do
+        it_behaves_like 'all epic permissions disabled'
       end
 
-      it 'user who is not a group member can only read epics' do
-        expect(permissions(user, group)).to be_allowed(:read_epic, :read_epic_iid)
-        expect(permissions(user, group)).to be_disallowed(:update_epic, :destroy_epic, :admin_epic, :create_epic)
+      it_behaves_like 'group member permissions'
     end
 
-      it 'guest group member can only read epics' do
-        group.add_guest(user)
+    context 'when an epic is in an internal group' do
+      let(:group) { create(:group, :internal) }
+
+      context 'anonymous user' do
+        let(:user) { nil }
 
-        expect(permissions(user, group)).to be_allowed(:read_epic, :read_epic_iid)
-        expect(permissions(user, group)).to be_disallowed(:update_epic, :destroy_epic, :admin_epic, :create_epic)
+        it_behaves_like 'all epic permissions disabled'
       end
 
-      it 'reporter group member can manage epics' do
-        group.add_reporter(user)
+      context 'user who is not a group member' do
+        it_behaves_like 'can only read epics'
+        it_behaves_like 'can comment on epics'
+      end
 
-        expect(permissions(user, group)).to be_disallowed(:destroy_epic)
-        expect(permissions(user, group))
-          .to be_allowed(:read_epic, :read_epic_iid, :update_epic, :admin_epic, :create_epic)
+      it_behaves_like 'group member permissions'
     end
 
-      it 'only group owner can destroy epics' do
-        group.add_owner(user)
+    context 'when an epic is in a public group' do
+      let(:group) { create(:group, :public) }
 
-        expect(permissions(user, group))
-          .to be_allowed(:read_epic, :read_epic_iid, :update_epic, :destroy_epic, :admin_epic, :create_epic)
+      context 'anonymous user' do
+        let(:user) { nil }
+
+        it_behaves_like 'can only read epics'
+        it_behaves_like 'cannot comment on epics'
       end
+
+      context 'user who is not a group member' do
+        it_behaves_like 'can only read epics'
+        it_behaves_like 'can comment on epics'
       end
+
+      it_behaves_like 'group member permissions'
     end
 
     context 'when external authorization is enabled' do
@@ -143,12 +142,13 @@ describe EpicPolicy do
         group.add_owner(user)
       end
 
-    it 'does not allow any epic permissions' do
+      it 'does not call external authorization service' do
         expect(EE::Gitlab::ExternalAuthorization).not_to receive(:access_allowed?)
 
-      expect(permissions(user, group))
-        .not_to be_allowed(:read_epic, :read_epic_iid, :update_epic,
-                           :destroy_epic, :admin_epic, :create_epic)
+        subject
+      end
+
+      it_behaves_like 'all epic permissions disabled'
     end
   end
 end