Backend code refactor

db/migrate/20171107130813_create_epic_issues_table.rb

@@ -5,16 +5,10 @@ class CreateEpicIssuesTable < ActiveRecord::Migration
 
   disable_ddl_transaction!
 
-  def up
+  def change
     create_table :epic_issues do |t|
-      t.references :epic, null: false, index: true, foreign_key: true
-      t.references :issue, null: false, index: { unique: true }, foreign_key: true
-
-      t.timestamps_with_timezone
+      t.references :epic, null: false, index: true, foreign_key: { on_delete: :cascade }
+      t.references :issue, null: false, index: { unique: true }, foreign_key: { on_delete: :cascade }
     end
   end
-
-  def down
-    drop_table :epic_issues
-  end
 end

ee/app/controllers/groups/epic_issues_controller.rb

@@ -3,6 +3,7 @@ class Groups::EpicIssuesController < Groups::EpicsController
 
   skip_before_action :authorize_destroy_issuable!
   before_action :authorize_admin_epic!, only: [:create, :destroy]
+  before_action :authorize_issue_link_association!, only: :destroy
 
   private
 
@@ -11,8 +12,7 @@ class Groups::EpicIssuesController < Groups::EpicsController
   end
 
   def destroy_service
-    epic_issue = EpicIssue.find(params[:id])
-    EpicIssues::DestroyService.new(epic_issue, current_user)
+    EpicIssues::DestroyService.new(link, current_user)
   end
 
   def issues
@@ -22,4 +22,12 @@ class Groups::EpicIssuesController < Groups::EpicsController
   def authorize_admin_epic!
     render_403 unless can?(current_user, :admin_epic, epic)
   end
+
+  def authorize_issue_link_association!
+    render_404 if link.epic != epic
+  end
+
+  def link
+    @link ||= EpicIssue.find(params[:id])
+  end
 end

ee/app/controllers/projects/issue_links_controller.rb

 module Projects
   class IssueLinksController < Projects::ApplicationController
     include IssuableLinks
+
     before_action :authorize_admin_issue_link!, only: [:create, :destroy]
+    before_action :authorize_issue_link_association!, only: :destroy
 
     private
 
@@ -13,6 +15,10 @@ module Projects
       render_403 unless can?(current_user, :admin_issue_link, @project)
     end
 
+    def authorize_issue_link_association!
+      render_404 if link.target != issue && link.source != issue
+    end
+
     def issue
       @issue ||=
         IssuesFinder.new(current_user, project_id: @project.id)
@@ -25,8 +31,11 @@ module Projects
     end
 
     def destroy_service
-      issue_link = IssueLink.find(params[:id])
-      IssueLinks::DestroyService.new(issue_link, current_user)
+      IssueLinks::DestroyService.new(link, current_user)
+    end
+
+    def link
+      @link ||= IssueLink.find(params[:id])
     end
   end
 end

ee/app/services/epic_issues/create_service.rb

@@ -5,7 +5,7 @@ module EpicIssues
     def relate_issues(referenced_issue)
       link = EpicIssue.find_or_initialize_by(issue: referenced_issue)
       link.epic = issuable
-      link.save
+      link.save!
     end
 
     def create_notes?
@@ -17,7 +17,9 @@ module EpicIssues
     end
 
     def linkable_issues(issues)
-      issues.select { |issue| can?(current_user, :admin_epic_issue, issue) && issue.project.group == issuable.group }
+      return [] unless  can?(current_user, :admin_epic, issuable.group)
+
+      issues.select { |issue| issue.project.group == issuable.group }
     end
   end
 end

spec/ee/spec/controllers/groups/epic_issues_controller_spec.rb

@@ -112,6 +112,28 @@ describe Groups::EpicIssuesController do
       end
     end
 
+    context 'when the epic from the association does not equal epic from the path' do
+      subject do
+        delete :destroy, group_id: group, epic_id: another_epic.to_param, id: epic_issue.id
+      end
+
+      let(:another_epic) { create(:epic, group: group) }
+
+      before do
+        group.add_developer(user)
+      end
+
+      it 'returns status 404' do
+        subject
+
+        expect(response.status).to eq(404)
+      end
+
+      it 'does not destroy the link' do
+        expect { subject }.not_to change { EpicIssue.count }.from(1)
+      end
+    end
+
     context 'when the epic_issue record does not exixst' do
       it 'returns status 404' do
         delete :destroy, group_id: group, epic_id: epic.to_param, id: 9999

spec/requests/projects/issue_links_controller_spec.rb

@@ -124,6 +124,29 @@ describe Projects::IssueLinksController do
         expect(json_response).to eq('issues' => list_service_response.as_json)
       end
     end
+
+    context 'when non of issues of the link is not the issue requested in the path' do
+      let(:referenced_issue) { create(:issue, project: project) }
+      let(:another_issue) { create(:issue, project: project) }
+      let(:issue) { create(:issue, project: project) }
+      let(:user_role) { :developer }
+
+      let!(:issue_link) { create :issue_link, source: another_issue, target: referenced_issue }
+
+      subject do
+        delete namespace_project_issue_link_path(issue_links_params(id: issue_link.id))
+      end
+
+      it 'returns 404' do
+        subject
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+
+      it 'does not delete the link' do
+        expect { subject }.not_to change { IssueLink.count }.from(1)
+      end
+    end
   end
 
   def issue_links_params(opts = {})