Allow guests to comment on epics

Previously only reporters and above were allowed

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,153 +2,153 @@ 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) }
+  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 '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)
-      end
+  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 '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 'group member permissions' do
+    context 'guest group member' do
+      before do
+        group.add_guest(user)
       end
 
-      it 'guest group member can only read epics' do
-        group.add_guest(user)
+      it_behaves_like 'can only read epics'
+      it_behaves_like 'can comment on epics'
+    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)
+    context 'reporter group member' do
+      before do
+        group.add_reporter(user)
       end
 
-      it 'reporter group member can manage epics' do
-        group.add_reporter(user)
+      it_behaves_like 'can manage epics'
+      it_behaves_like 'can comment on epics'
 
-        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 'cannot destroy epics' do
+        is_expected.to be_disallowed(:destroy_epic)
       end
+    end
 
-      it 'only group owner can destroy epics' do
+    context 'group owner' do
+      before do
         group.add_owner(user)
+      end
+
+      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 'can destroy epics' do
+        is_expected.to be_allowed(:destroy_epic)
       end
     end
+  end
 
-    context 'when an epic is in an internal group' do
-      let(:group) { create(:group, :internal) }
+  context 'when epics feature is disabled' do
+    let(:group) { create(:group, :public) }
+
+    before do
+      group.add_owner(user)
+    end
 
-      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)
+    it_behaves_like 'all epic permissions disabled'
+  end
+
+  context 'when epics feature is enabled' do
+    before do
+      stub_licensed_features(epics: true)
+    end
+
+    context 'when an epic is in a private group' do
+      let(:group) { create(:group, :private) }
+
+      context 'anonymous user' do
+        let(:user) { nil }
+
+        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)
+      context 'user who is not a group member' do
+        it_behaves_like 'all epic permissions disabled'
       end
 
-      it 'guest group member can only read epics' do
-        group.add_guest(user)
+      it_behaves_like 'group member permissions'
+    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)
-      end
+    context 'when an epic is in an internal group' do
+      let(:group) { create(:group, :internal) }
 
-      it 'reporter group member can manage epics' do
-        group.add_reporter(user)
+      context 'anonymous user' do
+        let(:user) { nil }
 
-        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 'all epic permissions disabled'
       end
 
-      it 'only group owner can destroy epics' do
-        group.add_owner(user)
-
-        expect(permissions(user, group))
-          .to be_allowed(:read_epic, :read_epic_iid, :update_epic, :destroy_epic, :admin_epic, :create_epic)
+      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 an epic is in a public group' do
       let(:group) { create(:group, :public) }
 
-      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)
-      end
+      context 'anonymous user' do
+        let(:user) { nil }
 
-      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 only read epics'
+        it_behaves_like 'cannot comment on epics'
       end
 
-      it 'guest group member can only read epics' do
-        group.add_guest(user)
-
-        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)
+      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 'reporter group member can manage epics' do
-        group.add_reporter(user)
+      it_behaves_like 'group member permissions'
+    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)
-      end
+    context 'when external authorization is enabled' do
+      let(:group) { create(:group) }
 
-      it 'only group owner can destroy epics' do
+      before do
+        enable_external_authorization_service_check
         group.add_owner(user)
-
-        expect(permissions(user, group))
-          .to be_allowed(:read_epic, :read_epic_iid, :update_epic, :destroy_epic, :admin_epic, :create_epic)
       end
-    end
-  end
-
-  context 'when external authorization is enabled' do
-    let(:group) { create(:group) }
 
-    before do
-      enable_external_authorization_service_check
-      group.add_owner(user)
-    end
+      it 'does not call external authorization service' do
+        expect(EE::Gitlab::ExternalAuthorization).not_to receive(:access_allowed?)
 
-    it 'does not allow any epic permissions' do
-      expect(EE::Gitlab::ExternalAuthorization).not_to receive(:access_allowed?)
+        subject
+      end
 
-      expect(permissions(user, group))
-        .not_to be_allowed(:read_epic, :read_epic_iid, :update_epic,
-                           :destroy_epic, :admin_epic, :create_epic)
+      it_behaves_like 'all epic permissions disabled'
     end
   end
 end