Merge branch 'fix-epic-link-create-perm' into 'master'

Check user permissions correctly

See merge request gitlab-org/gitlab!24946

ee/app/models/ee/epic.rb

@@ -346,7 +346,7 @@ module EE
       elsif parent.has_ancestor?(self)
         errors.add :parent, "This epic can't be added as it is already assigned to this epic's ancestor"
       elsif !preloaded_parent_group_and_descendants.include?(group)
-        errors.add :parent, "This epic can't be added because parent and child epics must belong to the same group"
+        errors.add :parent, "This epic can't be added because it must belong to the same group as the parent, or subgroup of the parent epic’s group"
       elsif level_depth_exceeded?(parent)
         errors.add :parent, "This epic can't be added as the maximum depth of nested epics would be exceeded"
       end

ee/app/services/epic_links/create_service.rb

@@ -3,6 +3,10 @@
 module EpicLinks
   class CreateService < IssuableLinks::CreateService
     def execute
+      unless can?(current_user, :admin_epic, issuable.group)
+        return error(issuables_not_found_message, 404)
+      end
+
       if issuable.max_hierarchy_depth_achieved?
         return error("This epic can't be added because the parent is already at the maximum depth from its most distant ancestor", 409)
       end
@@ -54,8 +58,6 @@ module EpicLinks
 
     def linkable_issuables(epics)
       @linkable_issuables ||= begin
-        return [] unless can?(current_user, :admin_epic, issuable.group)
-
         epics.select do |epic|
           linkable_epic?(epic)
         end

ee/spec/services/epic_links/create_service_spec.rb

@@ -70,98 +70,108 @@ describe EpicLinks::CreateService do
         context 'when a single epic is given' do
           subject { add_epic([valid_reference]) }
 
-          context 'when an epic from a another group is given' do
-            let(:other_group) { create(:group) }
-            let(:expected_error) { "This epic can't be added because parent and child epics must belong to the same group" }
-            let(:expected_code) { 409 }
+          context 'when a user does not have permissions to add an epic' do
+            include_examples 'returns an error'
+          end
 
+          context 'when a user has permissions to add an epic' do
             before do
-              epic_to_add.update!(group: other_group)
+              group.add_developer(user)
             end
 
-            include_examples 'returns an error'
-          end
-
-          context 'when hierarchy is cyclic' do
-            context 'when given child epic is the same as given parent' do
-              let(:expected_error) { 'Cannot add an epic as a child of itself' }
+            context 'when an epic from another group is given' do
+              let(:other_group) { create(:group) }
+              let(:expected_error) { "This epic can't be added because it must belong to the same group as the parent, or subgroup of the parent epic’s group" }
               let(:expected_code) { 409 }
 
-              subject { add_epic([epic.to_reference(full: true)]) }
+              before do
+                epic_to_add.update!(group: other_group)
+              end
 
               include_examples 'returns an error'
             end
 
-            context 'when given child epic is parent of the given parent' do
-              let(:expected_error) { "This epic can't be added as it is already assigned to this epic's ancestor" }
-              let(:expected_code) { 409 }
+            context 'when hierarchy is cyclic' do
+              context 'when given child epic is the same as given parent' do
+                let(:expected_error) { 'Cannot add an epic as a child of itself' }
+                let(:expected_code) { 409 }
 
-              before do
-                epic.update(parent: epic_to_add)
+                subject { add_epic([epic.to_reference(full: true)]) }
+
+                include_examples 'returns an error'
               end
 
-              include_examples 'returns an error'
-            end
+              context 'when given child epic is parent of the given parent' do
+                let(:expected_error) { "This epic can't be added as it is already assigned to this epic's ancestor" }
+                let(:expected_code) { 409 }
 
-            context 'when new child epic is an ancestor of the given parent' do
-              let(:expected_error) { "This epic can't be added as it is already assigned to this epic's ancestor" }
-              let(:expected_code) { 409 }
+                before do
+                  epic.update(parent: epic_to_add)
+                end
 
-              before do
-                # epic_to_add -> epic1 -> epic2 -> epic
-                epic1 = create(:epic, group: group, parent: epic_to_add)
-                epic2 = create(:epic, group: group, parent: epic1)
-                epic.update(parent: epic2)
+                include_examples 'returns an error'
               end
 
-              include_examples 'returns an error'
+              context 'when new child epic is an ancestor of the given parent' do
+                let(:expected_error) { "This epic can't be added as it is already assigned to this epic's ancestor" }
+                let(:expected_code) { 409 }
+
+                before do
+                  # epic_to_add -> epic1 -> epic2 -> epic
+                  epic1 = create(:epic, group: group, parent: epic_to_add)
+                  epic2 = create(:epic, group: group, parent: epic1)
+                  epic.update(parent: epic2)
+                end
+
+                include_examples 'returns an error'
+              end
             end
-          end
 
-          context 'when adding an epic that is already a child of the parent epic' do
-            before do
-              epic_to_add.update(parent: epic)
+            context 'when adding an epic that is already a child of the parent epic' do
+              before do
+                epic_to_add.update(parent: epic)
+              end
+
+              let(:expected_error) { "This epic can't be added as it is already assigned to the parent" }
+              let(:expected_code) { 409 }
+
+              include_examples 'returns an error'
             end
 
-            let(:expected_error) { "This epic can't be added as it is already assigned to the parent" }
-            let(:expected_code) { 409 }
+            context 'when adding to an Epic that is already at maximum depth' do
+              before do
+                epic1 = create(:epic, group: group)
+                epic2 = create(:epic, group: group, parent: epic1)
+                epic3 = create(:epic, group: group, parent: epic2)
+                epic4 = create(:epic, group: group, parent: epic3)
 
-            include_examples 'returns an error'
-          end
+                epic.update(parent: epic4)
+              end
 
-          context 'when adding to an Epic that is already at maximum depth' do
-            before do
-              epic1 = create(:epic, group: group)
-              epic2 = create(:epic, group: group, parent: epic1)
-              epic3 = create(:epic, group: group, parent: epic2)
-              epic4 = create(:epic, group: group, parent: epic3)
+              let(:expected_error) { "This epic can't be added because the parent is already at the maximum depth from its most distant ancestor" }
+              let(:expected_code) { 409 }
 
-              epic.update(parent: epic4)
+              include_examples 'returns an error'
             end
 
-            let(:expected_error) { "This epic can't be added because the parent is already at the maximum depth from its most distant ancestor" }
-            let(:expected_code) { 409 }
-
-            include_examples 'returns an error'
-          end
+            context 'when total depth after adding would exceed depth limit' do
+              let(:expected_error) { "This epic can't be added as the maximum depth of nested epics would be exceeded" }
+              let(:expected_code) { 409 }
 
-          context 'when total depth after adding would exceed limit' do
-            let(:expected_error) { "This epic can't be added as the maximum depth of nested epics would be exceeded" }
-            let(:expected_code) { 409 }
+              before do
+                epic1 = create(:epic, group: group)
 
-            before do
-              epic1 = create(:epic, group: group)
+                epic.update(parent: epic1) # epic is on level 2
 
-              epic.update(parent: epic1) # epic is on level 2
+                # epic_to_add has 3 children (level 4 including epic_to_add)
+                # that would mean level 6 after relating epic_to_add on epic
+                epic2 = create(:epic, group: group, parent: epic_to_add)
+                epic3 = create(:epic, group: group, parent: epic2)
+                create(:epic, group: group, parent: epic3)
+              end
 
-              # epic_to_add has 3 children (level 4 including epic_to_add)
-              # that would mean level 6 after relating epic_to_add on epic
-              epic2 = create(:epic, group: group, parent: epic_to_add)
-              epic3 = create(:epic, group: group, parent: epic2)
-              create(:epic, group: group, parent: epic3)
+              include_examples 'returns an error'
             end
-
-            include_examples 'returns an error'
           end
         end
 
@@ -174,75 +184,85 @@ describe EpicLinks::CreateService do
             )
           end
 
-          context 'when adding epics that are already a child of the parent epic' do
-            let(:expected_error) { 'Epic(s) already assigned' }
-            let(:expected_code) { 409 }
-
-            before do
-              epic_to_add.update(parent: epic)
-              another_epic.update(parent: epic)
-            end
-
+          context 'when a user dos not have permissions to add an epic' do
             include_examples 'returns an error'
           end
 
-          context 'when total depth after adding would exceed limit' do
+          context 'when a user has permissions to add an epic' do
             before do
-              epic1 = create(:epic, group: group)
+              group.add_developer(user)
+            end
 
-              epic.update(parent: epic1) # epic is on level 2
+            context 'when adding epics that are already a child of the parent epic' do
+              let(:expected_error) { 'Epic(s) already assigned' }
+              let(:expected_code) { 409 }
 
-              # epic_to_add has 3 children (level 4 including epic_to_add)
-              # that would mean level 6 after relating epic_to_add on epic
-              epic2 = create(:epic, group: group, parent: epic_to_add)
-              epic3 = create(:epic, group: group, parent: epic2)
-              create(:epic, group: group, parent: epic3)
+              before do
+                epic_to_add.update(parent: epic)
+                another_epic.update(parent: epic)
+              end
+
+              include_examples 'returns an error'
             end
 
-            let(:another_epic) { create(:epic) }
+            context 'when total depth after adding would exceed limit' do
+              before do
+                epic1 = create(:epic, group: group)
 
-            include_examples 'returns an error'
-          end
+                epic.update(parent: epic1) # epic is on level 2
 
-          context 'when an epic from a another group is given' do
-            let(:other_group) { create(:group) }
+                # epic_to_add has 3 children (level 4 including epic_to_add)
+                # that would mean level 6 after relating epic_to_add on epic
+                epic2 = create(:epic, group: group, parent: epic_to_add)
+                epic3 = create(:epic, group: group, parent: epic2)
+                create(:epic, group: group, parent: epic3)
+              end
 
-            before do
-              epic_to_add.update!(group: other_group)
+              let(:another_epic) { create(:epic) }
+
+              include_examples 'returns an error'
             end
 
-            include_examples 'returns an error'
-          end
+            context 'when an epic from a another group is given' do
+              let(:other_group) { create(:group) }
 
-          context 'when hierarchy is cyclic' do
-            context 'when given child epic is the same as given parent' do
-              subject { add_epic([epic.to_reference(full: true), another_epic.to_reference(full: true)]) }
+              before do
+                epic_to_add.update!(group: other_group)
+              end
 
               include_examples 'returns an error'
             end
 
-            context 'when given child epic is parent of the given parent' do
-              before do
-                epic.update(parent: epic_to_add)
+            context 'when hierarchy is cyclic' do
+              context 'when given child epic is the same as given parent' do
+                subject { add_epic([epic.to_reference(full: true), another_epic.to_reference(full: true)]) }
+
+                include_examples 'returns an error'
               end
 
+              context 'when given child epic is parent of the given parent' do
+                before do
+                  epic.update(parent: epic_to_add)
+                end
+
+                include_examples 'returns an error'
+              end
+            end
+
+            context 'when the reference list is empty' do
+              subject { add_epic([]) }
+
               include_examples 'returns an error'
             end
           end
         end
       end
 
-      context 'when user has permissions to link the epic' do
+      context 'when everything is ok' do
         before do
           group.add_developer(user)
         end
 
-        context 'when the reference list is empty' do
-          subject { add_epic([]) }
-
-          include_examples 'returns an error'
-        end
-
         context 'when a correct reference is given' do
           subject { add_epic([valid_reference]) }