Merge branch...

Merge branch '9462-security-report-crashes-when-vulnerability-associated-issuables-deleted' into 'master'

Resolve "Deletion of vulnerability-associated issuables prevents security report from loading"

Closes #9462

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

ee/app/assets/javascripts/security_dashboard/components/security_dashboard_table_row.vue

@@ -42,7 +42,9 @@ export default {
       return Boolean(this.vulnerability.dismissal_feedback);
     },
     hasIssue() {
-      return Boolean(this.vulnerability.issue_feedback);
+      return Boolean(
+        this.vulnerability.issue_feedback && this.vulnerability.issue_feedback.issue_iid,
+      );
     },
     canDismissVulnerability() {
       const path = this.vulnerability.vulnerability_feedback_dismissal_path;

ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/mutations.js

@@ -93,11 +93,17 @@ export default {
     Vue.set(state.modal.data.reportType, 'value', vulnerability.report_type);
     Vue.set(state.modal.data.confidence, 'value', vulnerability.confidence);
     Vue.set(state.modal, 'vulnerability', vulnerability);
-    Vue.set(state.modal.vulnerability, 'hasIssue', Boolean(vulnerability.issue_feedback));
+    Vue.set(
+      state.modal.vulnerability,
+      'hasIssue',
+      Boolean(vulnerability.issue_feedback && vulnerability.issue_feedback.issue_iid),
+    );
     Vue.set(
       state.modal.vulnerability,
       'hasMergeRequest',
-      Boolean(vulnerability.merge_request_feedback),
+      Boolean(
+        vulnerability.merge_request_feedback && vulnerability.merge_request.merge_request_iid,
+      ),
     );
     Vue.set(state.modal.vulnerability, 'isDismissed', Boolean(vulnerability.dismissal_feedback));
     Vue.set(state.modal, 'error', null);

ee/app/assets/javascripts/vue_shared/security_reports/store/utils.js

@@ -53,13 +53,13 @@ function enrichVulnerabilityWithfeedback(vulnerability, feedback = []) {
           isDismissed: true,
           dismissalFeedback: fb,
         };
-      } else if (fb.feedback_type === 'issue') {
+      } else if (fb.feedback_type === 'issue' && fb.issue_iid) {
         return {
           ...vuln,
           hasIssue: true,
           issue_feedback: fb,
         };
-      } else if (fb.feedback_type === 'merge_request') {
+      } else if (fb.feedback_type === 'merge_request' && fb.merge_request_iid) {
         return {
           ...vuln,
           hasMergeRequest: true,

ee/app/controllers/projects/vulnerability_feedback_controller.rb

@@ -12,6 +12,7 @@ class Projects::VulnerabilityFeedbackController < Projects::ApplicationControlle
   def index
     # TODO: Move to finder or list service
     @vulnerability_feedback = @project.vulnerability_feedback.with_associations
+
     if params[:category].present?
       @vulnerability_feedback = @vulnerability_feedback
         .where(category: Vulnerabilities::Feedback.categories[params[:category]])

ee/app/models/vulnerabilities/feedback.rb

@@ -29,5 +29,26 @@ module Vulnerabilities
     scope :all_preloaded, -> do
       preload(:author, :project, :issue, :merge_request, :pipeline)
     end
+
+    def self.find_or_init_for(feedback_params)
+      validate_enums(feedback_params)
+
+      record = find_or_initialize_by(feedback_params.slice(:category, :feedback_type, :project_fingerprint))
+      record.assign_attributes(feedback_params)
+      record
+    end
+
+    # Rails 5.0 does not properly handle validation of enums in select queries such as find_or_initialize_by.
+    # This method, and calls to it can be removed when we are on Rails 5.2.
+    def self.validate_enums(feedback_params)
+      unless feedback_types.include?(feedback_params[:feedback_type])
+
+        raise ArgumentError.new("'#{feedback_params[:feedback_type]}' is not a valid feedback_type")
+      end
+
+      unless categories.include?(feedback_params[:category])
+        raise ArgumentError.new("'#{feedback_params[:category]}' is not a valid category")
+      end
+    end
   end
 end

ee/app/serializers/vulnerabilities/feedback_entity.rb

@@ -17,19 +17,19 @@ class Vulnerabilities::FeedbackEntity < Grape::Entity
     end
   end
 
-  expose :issue_iid, if: -> (feedback, _) { feedback.issue? } do |feedback|
+  expose :issue_iid, if: -> (feedback, _) { feedback.issue.present? } do |feedback|
     feedback.issue.iid
   end
 
-  expose :issue_url, if: -> (feedback, _) { feedback.issue? } do |feedback|
+  expose :issue_url, if: -> (feedback, _) { feedback.issue.present? } do |feedback|
     project_issue_url(feedback.project, feedback.issue)
   end
 
-  expose :merge_request_iid, if: -> (feedback, _) { feedback.merge_request? } do |feedback|
+  expose :merge_request_iid, if: -> (feedback, _) { feedback.merge_request.present? } do |feedback|
     feedback.merge_request.iid
   end
 
-  expose :merge_request_path, if: -> (feedback, _) { feedback.merge_request? } do |feedback|
+  expose :merge_request_path, if: -> (feedback, _) { feedback.merge_request.present? } do |feedback|
     project_merge_request_path(feedback.project, feedback.merge_request)
   end
 

ee/app/services/vulnerability_feedback_module/create_service.rb

@@ -3,7 +3,7 @@
 module VulnerabilityFeedbackModule
   class CreateService < ::BaseService
     def execute
-      vulnerability_feedback = @project.vulnerability_feedback.new(@params)
+      vulnerability_feedback = @project.vulnerability_feedback.find_or_init_for(@params)
       vulnerability_feedback.author = @current_user
 
       if vulnerability_feedback.issue? &&

ee/changelogs/unreleased/9462-security-report-crashes-when-vulnerability-associated-issuables-deleted.yml

+---
+title: Resolve Deletion of vulnerability-associated issuables prevents security report
+  from loading
+merge_request: 10016
+author:
+type: fixed

ee/spec/javascripts/security_dashboard/components/security_dashboard_table_row_spec.js

@@ -108,4 +108,55 @@ describe('Security Dashboard Table Row', () => {
       expect(vm.$el.textContent).toContain('DISMISSED');
     });
   });
+
+  describe('with valid issue feedback', () => {
+    const vulnerability = mockDataVulnerabilities[3];
+
+    beforeEach(() => {
+      props = { vulnerability };
+      vm = mountComponentWithStore(Component, { store, props });
+    });
+
+    afterEach(() => {
+      vm.$destroy();
+    });
+
+    it('should have a `ic-issue-created` class', () => {
+      expect(vm.$el.querySelectorAll('.ic-issue-created')).toHaveLength(1);
+    });
+  });
+
+  describe('with invalid issue feedback', () => {
+    const vulnerability = mockDataVulnerabilities[6];
+
+    beforeEach(() => {
+      props = { vulnerability };
+      vm = mountComponentWithStore(Component, { store, props });
+    });
+
+    afterEach(() => {
+      vm.$destroy();
+    });
+
+    it('should not have a `ic-issue-created` class', () => {
+      expect(vm.$el.querySelectorAll('.ic-issue-created')).toHaveLength(0);
+    });
+  });
+
+  describe('with no issue feedback', () => {
+    const vulnerability = mockDataVulnerabilities[0];
+
+    beforeEach(() => {
+      props = { vulnerability };
+      vm = mountComponentWithStore(Component, { store, props });
+    });
+
+    afterEach(() => {
+      vm.$destroy();
+    });
+
+    it('should not have a `ic-issue-created` class', () => {
+      expect(vm.$el.querySelectorAll('.ic-issue-created')).toHaveLength(0);
+    });
+  });
 });

ee/spec/javascripts/security_dashboard/store/vulnerabilities/data/mock_data_vulnerabilities.json

@@ -406,5 +406,79 @@
         "url": "https://crypto.stackexchange.com/questions/31428/pbewithmd5anddes-cipher-does-not-check-for-integrity-first"
       }
     ]
+  },
+  {
+    "id": 7,
+    "report_type": "sast",
+    "name": "Insecure variable usage",
+    "severity": "high",
+    "confidence": "low",
+    "scanner": {
+      "external_id": "find_sec_bugs",
+      "name": "Find Security Bugs"
+    },
+    "identifiers": [
+      {
+        "external_type": "CVE",
+        "external_id": "CVE-2018-1234",
+        "name": "CVE-2018-1234",
+        "url": "http://cve.mitre.org/cgi-bin/cvename.cgi?name=2018-1234"
+      },
+      {
+        "external_type": "CVE",
+        "external_id": "CVE-2018-1234",
+        "name": "CVE-2018-1234",
+        "url": "http://cve.mitre.org/cgi-bin/cvename.cgi?name=2018-1234"
+      }
+    ],
+    "project_fingerprint": "4e5b6966dd100170b4b1ad599c7058cce91b57b4",
+    "project": {
+      "id": 1,
+      "name": "project1",
+      "full_path": "/namespace1/project1",
+      "full_name": "Gitab.org / security-products / codequality"
+    },
+    "dismissal_feedback": null,
+    "issue_feedback": {
+      "id": 7,
+      "project_id": 1,
+      "author": {
+        "id": 8,
+        "name": "John Doe9",
+        "username": "user8",
+        "state": "active",
+        "avatar_url": "https://www.gravatar.com/avatar/51798cfc94af924ac2dffb7083baa6f4?s=80&d=identicon",
+        "web_url": "http://localhost/user8",
+        "status_tooltip_html": null,
+        "path": "/user8"
+      },
+      "issue_iid": null,
+      "pipeline": {
+        "id": 3,
+        "path": "/namespace6/project6/pipelines/3"
+      },
+      "issue_url": null,
+      "category": "sast",
+      "feedback_type": "issue",
+      "branch": "master",
+      "project_fingerprint": "4e5b6966dd100170b4b1ad599c7058cce91b57b4"
+    },
+    "vulnerability_feedback_issue_path": "https://example.com/vulnerability_feedback",
+    "vulnerability_feedback_dismissal_path": "https://example.com/vulnerability_feedback",
+    "description": "The cipher does not provide data integrity update 1",
+    "solution": "GCM mode introduces an HMAC into the resulting encrypted data, providing integrity of the result.",
+    "location": {
+      "file": "maven/src/main/java/com/gitlab/security_products/tests/App.java",
+      "start_line": 29,
+      "end_line": 29,
+      "class": "com.gitlab.security_products.tests.App",
+      "method": "insecureCypher"
+    },
+    "links": [
+      {
+        "name": "Cipher does not check for integrity first?",
+        "url": "https://crypto.stackexchange.com/questions/31428/pbewithmd5anddes-cipher-does-not-check-for-integrity-first"
+      }
+    ]
   }
 ]

ee/spec/javascripts/security_dashboard/store/vulnerabilities/mutations_spec.js

@@ -314,12 +314,33 @@ describe('vulnerabilities module mutations', () => {
       });
 
       it('should set hasIssue when the vulnerabilitiy has a related issue', () => {
-        const payload = { vulnerability: { ...vulnerability, issue_feedback: 'I am an issue' } };
+        const payload = {
+          vulnerability: {
+            ...vulnerability,
+            issue_feedback: {
+              issue_iid: 123,
+            },
+          },
+        };
         mutations[types.SET_MODAL_DATA](state, payload);
 
         expect(state.modal.vulnerability.hasIssue).toEqual(true);
       });
 
+      it('should not set hasIssue when the issue_iid in null', () => {
+        const payload = {
+          vulnerability: {
+            ...vulnerability,
+            issue_feedback: {
+              issue_iid: null,
+            },
+          },
+        };
+        mutations[types.SET_MODAL_DATA](state, payload);
+
+        expect(state.modal.vulnerability.hasIssue).toEqual(false);
+      });
+
       it('should nullify the modal links', () => {
         const payload = { vulnerability: { ...vulnerability, links: [] } };
         mutations[types.SET_MODAL_DATA](state, payload);

ee/spec/javascripts/vue_shared/security_reports/mock_data.js

@@ -1196,7 +1196,7 @@ export const sastFeedbacks = [
     id: 3,
     project_id: 17,
     author_id: 1,
-    issue_id: null,
+    issue_iid: null,
     pipeline_id: 132,
     category: 'sast',
     feedback_type: 'dismissal',
@@ -1207,7 +1207,7 @@ export const sastFeedbacks = [
     id: 4,
     project_id: 17,
     author_id: 1,
-    issue_id: 123,
+    issue_iid: 123,
     pipeline_id: 132,
     category: 'sast',
     feedback_type: 'issue',
@@ -1221,7 +1221,7 @@ export const dependencyScanningFeedbacks = [
     id: 3,
     project_id: 17,
     author_id: 1,
-    issue_id: null,
+    issue_iid: null,
     pipeline_id: 132,
     category: 'dependency_scanning',
     feedback_type: 'dismissal',
@@ -1232,7 +1232,7 @@ export const dependencyScanningFeedbacks = [
     id: 4,
     project_id: 17,
     author_id: 1,
-    issue_id: 123,
+    issue_iid: 123,
     pipeline_id: 132,
     category: 'dependency_scanning',
     feedback_type: 'issue',
@@ -1246,7 +1246,7 @@ export const dastFeedbacks = [
     id: 3,
     project_id: 17,
     author_id: 1,
-    issue_id: null,
+    issue_iid: null,
     pipeline_id: 132,
     category: 'container_scanning',
     feedback_type: 'dismissal',
@@ -1257,7 +1257,7 @@ export const dastFeedbacks = [
     id: 4,
     project_id: 17,
     author_id: 1,
-    issue_id: 123,
+    issue_iid: 123,
     pipeline_id: 132,
     category: 'container_scanning',
     feedback_type: 'issue',
@@ -1271,7 +1271,7 @@ export const containerScanningFeedbacks = [
     id: 3,
     project_id: 17,
     author_id: 1,
-    issue_id: null,
+    issue_iid: null,
     pipeline_id: 132,
     category: 'container_scanning',
     feedback_type: 'dismissal',
@@ -1282,7 +1282,7 @@ export const containerScanningFeedbacks = [
     id: 4,
     project_id: 17,
     author_id: 1,
-    issue_id: 123,
+    issue_iid: 123,
     pipeline_id: 132,
     category: 'container_scanning',
     feedback_type: 'issue',

ee/spec/models/vulnerabilities/feedback_spec.rb

@@ -21,4 +21,74 @@ describe Vulnerabilities::Feedback do
     it { is_expected.to validate_presence_of(:category) }
     it { is_expected.to validate_presence_of(:project_fingerprint) }
   end
+
+  describe '#find_or_init_for' do
+    let(:group) { create(:group) }
+    let(:project) { create(:project, :public, :repository, namespace: group) }
+    let(:user) { create(:user) }
+    let(:pipeline) { create(:ci_pipeline, project: project) }
+
+    let(:feedback_params) do
+      {
+        feedback_type: 'dismissal', pipeline_id: pipeline.id, category: 'sast',
+        project_fingerprint: '418291a26024a1445b23fe64de9380cdcdfd1fa8',
+        vulnerability_data: {
+          category: 'sast',
+          priority: 'Low', line: '41',
+          file: 'subdir/src/main/java/com/gitlab/security_products/tests/App.java',
+          cve: '818bf5dacb291e15d9e6dc3c5ac32178:PREDICTABLE_RANDOM',
+          name: 'Predictable pseudorandom number generator',
+          description: 'Description of Predictable pseudorandom number generator',
+          tool: 'find_sec_bugs'
+        }
+      }
+    end
+
+    context 'when params are valid' do
+      subject(:feedback) { described_class.find_or_init_for(feedback_params) }
+
+      before do
+        feedback.author = user
+        feedback.project = project
+      end
+
+      it 'inits the feedback' do
+        is_expected.to be_new_record
+      end
+
+      it 'finds the existing feedback' do
+        feedback.save!
+
+        existing_feedback = described_class.find_or_init_for(feedback_params)
+
+        expect(existing_feedback).to eq(feedback)
+      end
+
+      context 'when attempting to save duplicate' do
+        it 'raises ActiveRecord::RecordInvalid' do
+          duplicate = described_class.find_or_init_for(feedback_params)
+          duplicate.author = user
+          duplicate.project = project
+
+          feedback.save!
+
+          expect { duplicate.save! }.to raise_error(ActiveRecord::RecordInvalid)
+        end
+      end
+    end
+
+    context 'when params are invalid' do
+      it 'raises ArgumentError when given a bad feedback_type value' do
+        feedback_params[:feedback_type] = 'foo'
+
+        expect { described_class.find_or_init_for(feedback_params) }.to raise_error(ArgumentError, /feedback_type/)
+      end
+
+      it 'raises ArgumentError when given a bad category value' do
+        feedback_params[:category] = 'foo'
+
+        expect { described_class.find_or_init_for(feedback_params) }.to raise_error(ArgumentError, /category/)
+      end
+    end
+  end
 end

ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb

@@ -12,4 +12,52 @@ describe Vulnerabilities::FeedbackEntity do
 
     it { is_expected.to include(:project_id, :author, :category, :feedback_type) }
   end
+
+  context 'when issue is present' do
+    let(:feedback) { build(:vulnerability_feedback, :issue ) }
+    let(:entity) { described_class.represent(feedback) }
+
+    subject { entity.as_json }
+
+    it 'exposes issue information' do
+      is_expected.to include(:issue_iid)
+      is_expected.to include(:issue_url)
+    end
+  end
+
+  context 'when issue is not present' do
+    let(:feedback) { build(:vulnerability_feedback, feedback_type: 'issue', issue: nil) }
+    let(:entity) { described_class.represent(feedback) }
+
+    subject { entity.as_json }
+
+    it 'does not expose issue information' do
+      is_expected.not_to include(:issue_iid)
+      is_expected.not_to include(:issue_url)
+    end
+  end
+
+  context 'when merge request is present' do
+    let(:feedback) { build(:vulnerability_feedback, :merge_request ) }
+    let(:entity) { described_class.represent(feedback) }
+
+    subject { entity.as_json }
+
+    it 'exposes merge request information' do
+      is_expected.to include(:merge_request_iid)
+      is_expected.to include(:merge_request_path)
+    end
+  end
+
+  context 'when merge request is not present' do
+    let(:feedback) { build(:vulnerability_feedback, feedback_type: 'merge_request', merge_request: nil) }
+    let(:entity) { described_class.represent(feedback) }
+
+    subject { entity.as_json }
+
+    it 'does not expose merge request information' do
+      is_expected.not_to include(:merge_request_iid)
+      is_expected.not_to include(:merge_request_path)
+    end
+  end
 end

ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb

@@ -74,6 +74,19 @@ describe VulnerabilityFeedbackModule::CreateService, '#execute' do
         expect(feedback.merge_request).to be_nil
       end
 
+      it 'updates the feedback when it already exists' do
+        result
+
+        expect { described_class.new(project, user, feedback_params.merge(feedback_type: 'issue')).execute }.not_to change(Vulnerabilities::Feedback, :count)
+      end
+
+      it 'creates a new issue when feedback already exists and issue has been deleted' do
+        result
+
+        expect { result[:vulnerability_feedback].issue.destroy}.to change(Issue, :count).by(-1)
+        expect { described_class.new(project, user, feedback_params.merge(feedback_type: 'issue')).execute }.to change(Issue, :count).by(1)
+      end
+
       it 'delegates the Issue creation to CreateFromVulnerabilityDataService' do
         expect_any_instance_of(Issues::CreateFromVulnerabilityDataService)
           .to receive(:execute).once.and_call_original
@@ -191,5 +204,21 @@ describe VulnerabilityFeedbackModule::CreateService, '#execute' do
         expect(result[:message]).to eq("'foo' is not a valid feedback_type")
       end
     end
+
+    context 'when category is invalid' do
+      let(:feedback_params) do
+        {
+          feedback_type: 'dismissal', pipeline_id: pipeline.id, category: 'foo',
+          project_fingerprint: '418291a26024a1445b23fe64de9380cdcdfd1fa8'
+        }
+      end
+
+      let(:result) { described_class.new(project, user, feedback_params).execute }
+
+      it 'returns error with correct message' do
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to eq("'foo' is not a valid category")
+      end
+    end
   end
 end