Merge branch...

Merge branch '215069-convert-vulnerability-description-from-haml-to-vue-component-on-standalone-vulns-page' into 'master'

Change the vulnerability description from haml to a Vue component

See merge request gitlab-org/gitlab!32370

ee/app/assets/javascripts/pages/projects/security/vulnerabilities/show/index.js

 import Vue from 'vue';
 import HeaderApp from 'ee/vulnerabilities/components/header.vue';
+import DetailsApp from 'ee/vulnerabilities/components/details.vue';
 import FooterApp from 'ee/vulnerabilities/components/footer.vue';
 
 function createHeaderApp() {
@@ -27,6 +28,17 @@ function createHeaderApp() {
   });
 }
 
+function createDetailsApp() {
+  const el = document.getElementById('js-vulnerability-details');
+  const vulnerability = JSON.parse(el.dataset.vulnerabilityJson);
+  const finding = JSON.parse(el.dataset.findingJson);
+
+  return new Vue({
+    el,
+    render: h => h(DetailsApp, { props: { vulnerability, finding } }),
+  });
+}
+
 function createFooterApp() {
   const el = document.getElementById('js-vulnerability-footer');
 
@@ -72,5 +84,6 @@ function createFooterApp() {
 
 window.addEventListener('DOMContentLoaded', () => {
   createHeaderApp();
+  createDetailsApp();
   createFooterApp();
 });

ee/app/assets/javascripts/vulnerabilities/components/detail_item.vue

+<script>
+import { GlSprintf } from '@gitlab/ui';
+
+export default {
+  components: { GlSprintf },
+  props: {
+    sprintfMessage: { type: String, required: true },
+  },
+  computed: {
+    valueName() {
+      // Get the name of the placeholder that's not %{labelStart} or %{labelEnd}.
+      return this.sprintfMessage.match(/%{(?!(labelStart|labelEnd))(.+)}/)[2];
+    },
+  },
+};
+</script>
+
+<template>
+  <li :data-testid="valueName">
+    <gl-sprintf :message="sprintfMessage">
+      <template #label="{ content }">
+        <strong>{{ content }}</strong>
+      </template>
+      <template #[valueName]>
+        <slot></slot>
+      </template>
+    </gl-sprintf>
+  </li>
+</template>

ee/app/assets/javascripts/vulnerabilities/components/details.vue

+<script>
+import { GlLink } from '@gitlab/ui';
+import SeverityBadge from 'ee/vue_shared/security_reports/components/severity_badge.vue';
+import DetailItem from './detail_item.vue';
+
+export default {
+  name: 'VulnerabilityDetails',
+  components: { GlLink, SeverityBadge, DetailItem },
+  props: {
+    vulnerability: {
+      type: Object,
+      required: true,
+    },
+    finding: {
+      type: Object,
+      required: true,
+    },
+  },
+  computed: {
+    location() {
+      return this.finding.location || {};
+    },
+    fileText() {
+      return (this.location.file || '') + (this.lineNumber ? `:${this.lineNumber}` : '');
+    },
+    fileUrl() {
+      return (this.location.blob_path || '') + (this.lineNumber ? `#L${this.lineNumber}` : '');
+    },
+    lineNumber() {
+      const { start_line: start, end_line: end } = this.location;
+      return end > start ? `${start}-${end}` : start;
+    },
+  },
+};
+</script>
+
+<template>
+  <div class="md">
+    <h1 class="mt-3 mb-2 border-bottom-0" data-testid="title">{{ vulnerability.title }}</h1>
+    <h3 class="mt-0">{{ __('Description') }}</h3>
+    <p data-testid="description">{{ finding.description }}</p>
+
+    <ul>
+      <detail-item :sprintf-message="__('%{labelStart}Severity:%{labelEnd} %{severity}')">
+        <severity-badge :severity="vulnerability.severity" class="gl-display-inline ml-1" />
+      </detail-item>
+      <detail-item :sprintf-message="__('%{labelStart}Confidence:%{labelEnd} %{confidence}')"
+        >{{ vulnerability.confidence }}
+      </detail-item>
+      <detail-item :sprintf-message="__('%{labelStart}Report Type:%{labelEnd} %{reportType}')"
+        >{{ vulnerability.report_type }}
+      </detail-item>
+      <detail-item
+        v-if="location.image"
+        :sprintf-message="__('%{labelStart}Image:%{labelEnd} %{image}')"
+        >{{ location.image }}
+      </detail-item>
+      <detail-item
+        v-if="location.operating_system"
+        :sprintf-message="__('%{labelStart}Namespace:%{labelEnd} %{namespace}')"
+        >{{ location.operating_system }}
+      </detail-item>
+    </ul>
+
+    <template v-if="location.file">
+      <h3>{{ __('Location') }}</h3>
+      <ul>
+        <detail-item :sprintf-message="__('%{labelStart}File:%{labelEnd} %{file}')">
+          <gl-link :href="fileUrl" target="_blank">{{ fileText }}</gl-link>
+        </detail-item>
+        <detail-item
+          v-if="location.class"
+          :sprintf-message="__('%{labelStart}Class:%{labelEnd} %{class}')"
+          >{{ location.class }}
+        </detail-item>
+        <detail-item
+          v-if="location.method"
+          :sprintf-message="__('%{labelStart}Method:%{labelEnd} %{method}')"
+        >
+          <code>{{ location.method }}</code>
+        </detail-item>
+      </ul>
+    </template>
+
+    <template v-if="finding.links && finding.links.length">
+      <h3>{{ __('Links') }}</h3>
+      <ul>
+        <li v-for="link in finding.links" :key="link.url">
+          <gl-link
+            :href="link.url"
+            data-testid="link"
+            target="_blank"
+            :aria-label="__('Third Party Advisory Link')"
+            :title="link.url"
+          >
+            {{ link.url }}
+          </gl-link>
+        </li>
+      </ul>
+    </template>
+
+    <template v-if="finding.identifiers && finding.identifiers.length">
+      <h3>{{ __('Identifiers') }}</h3>
+      <ul>
+        <li v-for="identifier in finding.identifiers" :key="identifier.url">
+          <gl-link :href="identifier.url" data-testid="identifier" target="_blank">
+            {{ identifier.name }}
+          </gl-link>
+        </li>
+      </ul>
+    </template>
+  </div>
+</template>

ee/app/helpers/vulnerabilities_helper.rb

@@ -13,7 +13,7 @@ module VulnerabilitiesHelper
       pipeline_json: vulnerability_pipeline_data(pipeline).to_json,
       has_mr: !!vulnerability.finding.merge_request_feedback.try(:merge_request_iid),
       vulnerability_feedback_help_path: help_page_path('user/application_security/index', anchor: 'interacting-with-the-vulnerabilities'),
-      finding_json: vulnerability_finding_data(vulnerability.finding).to_json,
+      finding_json: vulnerability_finding_data(vulnerability).to_json,
       create_mr_url: create_vulnerability_feedback_merge_request_path(vulnerability.finding.project),
       timestamp: Time.now.to_i
     }
@@ -30,11 +30,11 @@ module VulnerabilitiesHelper
     }
   end
 
-  def vulnerability_finding_data(finding)
-    finding = Vulnerabilities::FindingSerializer.new(current_user: current_user).represent(finding)
+  def vulnerability_finding_data(vulnerability)
+    finding = Vulnerabilities::FindingSerializer.new(current_user: current_user).represent(vulnerability.finding)
     remediation = finding[:remediations]&.first
 
-    finding.slice(
+    data = finding.slice(
       :description,
       :identifiers,
       :links,
@@ -47,20 +47,14 @@ module VulnerabilitiesHelper
     ).merge(
       solution: remediation ? remediation['summary'] : finding[:solution]
     )
-  end
 
-  def vulnerability_file_link(vulnerability)
-    finding = vulnerability.finding
-    location = finding.location
-    branch = finding.pipelines&.last&.sha || vulnerability.project.default_branch
-    link_text = location['file']
-    link_path = project_blob_path(vulnerability.project, tree_join(branch, location['file']))
+    if data[:location]['file']
+      branch = vulnerability.finding.pipelines&.last&.sha || vulnerability.project.default_branch
+      path = project_blob_path(vulnerability.project, tree_join(branch, data[:location]['file']))
 
-    if location['start_line']
-      link_text += ":#{location['start_line']}"
-      link_path += "#L#{location['start_line']}"
+      data[:location]['blob_path'] = path
     end
 
-    link_to link_text, link_path, target: '_blank', rel: 'noopener noreferrer'
+    data
   end
 end

ee/app/views/projects/security/vulnerabilities/show.html.haml

@@ -3,55 +3,7 @@
 - breadcrumb_title @vulnerability.id
 - page_title @vulnerability.title
 - page_description @vulnerability.description
-- finding = @vulnerability.finding
-- location = finding.location
 
 #js-vulnerability-header{ data: vulnerability_data(@vulnerability, @pipeline) }
-
-.issue-details.issuable-details
-  .detail-page-description.p-0.my-3
-    %h2.title= @vulnerability.title
-    .description
-      .md
-        %h3= "Description"
-        %p= finding.description
-        %ul
-          %li= _("Severity: %{severity}") % { severity: @vulnerability.severity }
-          %li= _("Confidence: %{confidence}") % { confidence: @vulnerability.confidence }
-          %li= _("Report Type: %{report_type}") % { report_type: @vulnerability.report_type }
-
-
-          - if location['image']
-            %li= _("Image: %{image}") % { image: location['image'] }
-
-          - if location['operating_system']
-            %li= _("Namespace: %{namespace}") % { namespace: location['operating_system'] }
-
-        - if location['file']
-          %h3= _('Location')
-          %ul
-            %li
-              = _('File:')
-              = vulnerability_file_link(@vulnerability)
-            - if location['class']
-              %li
-                = _('Class:')
-                = location['class']
-            - if location['method']
-              %li
-                = _('Method:')
-                %code= location['method']
-
-        - if finding.links.any?
-          %h3= _('Links')
-          %ul
-            - finding.links.each do |link|
-              %li= link_to link['url'], link['url'], target: '_blank', rel: 'noopener noreferrer', 'aria-label': _('Third Party Advisory Link'), title: link['url']
-
-        - if finding.identifiers.any?
-          %h3= _('Identifiers')
-          %ul
-            - finding.identifiers.each do |identifier|
-              %li= link_to identifier.name, identifier.url, target: '_blank', rel: 'noopener noreferrer'
-
+#js-vulnerability-details{ data: vulnerability_data(@vulnerability, @pipeline) }
 #js-vulnerability-footer{ data: vulnerability_data(@vulnerability, @pipeline) }

ee/spec/controllers/projects/security/vulnerabilities_controller_spec.rb

@@ -34,12 +34,6 @@ RSpec.describe Projects::Security::VulnerabilitiesController do
         expect(response.body).to have_text(vulnerability.title)
       end
 
-      it 'renders the file location' do
-        show_vulnerability
-
-        expect(response.body).to have_text(vulnerability.finding.location['file'])
-      end
-
       it 'renders the solution card' do
         show_vulnerability
 

ee/spec/frontend/vulnerabilities/details_spec.js

+import { mount } from '@vue/test-utils';
+import { GlLink } from '@gitlab/ui';
+import SeverityBadge from 'ee/vue_shared/security_reports/components/severity_badge.vue';
+import VulnerabilityDetails from 'ee/vulnerabilities/components/details.vue';
+
+describe('Vulnerability Details', () => {
+  let wrapper;
+
+  const vulnerability = {
+    title: 'some title',
+    severity: 'bad severity',
+    confidence: 'high confidence',
+    report_type: 'nice report_type',
+  };
+
+  const finding = {
+    description: 'finding description',
+  };
+
+  const createWrapper = findingOverrides => {
+    const propsData = {
+      vulnerability,
+      finding: { ...finding, ...findingOverrides },
+    };
+
+    wrapper = mount(VulnerabilityDetails, { propsData });
+  };
+
+  const getById = id => wrapper.find(`[data-testid="${id}"]`);
+  const getAllById = id => wrapper.findAll(`[data-testid="${id}"]`);
+  const getText = id => getById(id).text();
+
+  afterEach(() => {
+    wrapper.destroy();
+  });
+
+  it('shows the properties that should always be shown', () => {
+    createWrapper();
+    expect(getText('title')).toBe(vulnerability.title);
+    expect(getText('description')).toBe(finding.description);
+    expect(wrapper.find(SeverityBadge).props('severity')).toBe(vulnerability.severity);
+    expect(getText('confidence')).toBe(`Confidence: ${vulnerability.confidence}`);
+    expect(getText('reportType')).toBe(`Report Type: ${vulnerability.report_type}`);
+
+    expect(getById('image').exists()).toBeFalsy();
+    expect(getById('os').exists()).toBeFalsy();
+    expect(getById('file').exists()).toBeFalsy();
+    expect(getById('class').exists()).toBeFalsy();
+    expect(getById('method').exists()).toBeFalsy();
+    expect(getAllById('link')).toHaveLength(0);
+    expect(getAllById('identifier')).toHaveLength(0);
+  });
+
+  it('shows the location image if it exists', () => {
+    createWrapper({ location: { image: 'some image' } });
+    expect(getText('image')).toBe(`Image: some image`);
+  });
+
+  it('shows the operating system if it exists', () => {
+    createWrapper({ location: { operating_system: 'linux' } });
+    expect(getText('namespace')).toBe(`Namespace: linux`);
+  });
+
+  it('shows the finding class if it exists', () => {
+    createWrapper({ location: { file: 'file', class: 'class name' } });
+    expect(getText('class')).toBe(`Class: class name`);
+  });
+
+  it('shows the finding method if it exists', () => {
+    createWrapper({ location: { file: 'file', method: 'method name' } });
+    expect(getText('method')).toBe(`Method: method name`);
+  });
+
+  it('shows the links if they exist', () => {
+    createWrapper({ links: [{ url: '0' }, { url: '1' }, { url: '2' }] });
+    const links = getAllById('link');
+    expect(links).toHaveLength(3);
+
+    links.wrappers.forEach((link, index) => {
+      expect(link.attributes('target')).toBe('_blank');
+      expect(link.attributes('href')).toBe(index.toString());
+      expect(link.text()).toBe(index.toString());
+    });
+  });
+
+  it('shows the finding identifiers if they exist', () => {
+    createWrapper({
+      identifiers: [{ url: '0', name: '00' }, { url: '1', name: '11' }, { url: '2', name: '22' }],
+    });
+
+    const identifiers = getAllById('identifier');
+    expect(identifiers).toHaveLength(3);
+
+    const checkIdentifier = index => {
+      const identifier = identifiers.at(index);
+      expect(identifier.attributes('target')).toBe('_blank');
+      expect(identifier.attributes('href')).toBe(index.toString());
+      expect(identifier.text()).toBe(`${index}${index}`);
+    };
+
+    for (let i = 0; i < identifiers.length; i += 1) {
+      checkIdentifier(i);
+    }
+  });
+
+  describe('file link', () => {
+    const file = () => getById('file').find(GlLink);
+
+    it('shows only the file name if there is no start line', () => {
+      createWrapper({ location: { file: 'test.txt', blob_path: 'blob_path.txt' } });
+      expect(file().attributes('target')).toBe('_blank');
+      expect(file().attributes('href')).toBe('blob_path.txt');
+      expect(file().text()).toBe('test.txt');
+    });
+
+    it('shows the correct line number when there is a start line', () => {
+      createWrapper({ location: { file: 'test.txt', start_line: 24, blob_path: 'blob.txt' } });
+      expect(file().attributes('target')).toBe('_blank');
+      expect(file().attributes('href')).toBe('blob.txt#L24');
+      expect(file().text()).toBe('test.txt:24');
+    });
+
+    it('shows the correct line numbers when there is a start and end line', () => {
+      createWrapper({
+        location: { file: 'test.txt', start_line: 24, end_line: 27, blob_path: 'blob.txt' },
+      });
+      expect(file().attributes('target')).toBe('_blank');
+      expect(file().attributes('href')).toBe('blob.txt#L24-27');
+      expect(file().text()).toBe('test.txt:24-27');
+    });
+
+    it('shows only the start line when the end line is the same', () => {
+      createWrapper({
+        location: { file: 'test.txt', start_line: 24, end_line: 24, blob_path: 'blob.txt' },
+      });
+      expect(file().attributes('target')).toBe('_blank');
+      expect(file().attributes('href')).toBe('blob.txt#L24');
+      expect(file().text()).toBe('test.txt:24');
+    });
+  });
+});

ee/spec/helpers/vulnerabilities_helper_spec.rb

@@ -4,9 +4,10 @@ require 'spec_helper'
 
 RSpec.describe VulnerabilitiesHelper do
   let_it_be(:user) { build(:user) }
-  let_it_be(:vulnerability) { create(:vulnerability, :with_findings, title: "My vulnerability") }
-  let_it_be(:project) { vulnerability.project }
-  let_it_be(:finding) { vulnerability.finding }
+  let_it_be(:project) { create(:project, :repository, :public) }
+  let_it_be(:pipeline) { create(:ci_pipeline, :success, project: project) }
+  let_it_be(:finding) { create(:vulnerabilities_occurrence, pipelines: [pipeline], project: project, severity: :high) }
+  let_it_be(:vulnerability) { create(:vulnerability, title: "My vulnerability", project: project, findings: [finding]) }
   let(:vulnerability_serializer_hash) do
     vulnerability.slice(
       :id,
@@ -102,9 +103,7 @@ RSpec.describe VulnerabilitiesHelper do
   end
 
   describe '#vulnerability_finding_data' do
-    let(:finding) { build(:vulnerabilities_occurrence) }
-
-    subject { helper.vulnerability_finding_data(finding) }
+    subject { helper.vulnerability_finding_data(vulnerability) }
 
     it 'returns finding information' do
       expect(subject).to match(
@@ -119,43 +118,18 @@ RSpec.describe VulnerabilitiesHelper do
         remediations: finding.remediations,
         solution: kind_of(String)
       )
-    end
-  end
 
-  describe '#vulnerability_file_link' do
-    let(:project) { create(:project, :repository, :public) }
-    let(:pipeline) { create(:ci_pipeline, :success, project: project) }
-    let(:finding) { create(:vulnerabilities_occurrence, pipelines: [pipeline], project: project, severity: :high) }
-    let(:vulnerability) { create(:vulnerability, findings: [finding], project: project) }
-
-    subject { helper.vulnerability_file_link(vulnerability) }
-
-    it 'returns a link to the vulnerability file location' do
-      expect(subject).to include(
-        vulnerability.finding.location['file'],
-        "#{vulnerability.finding.location['start_line']}",
-        vulnerability.finding.pipelines&.last&.sha
-      )
+      expect(subject[:location]['blob_path']).to match(kind_of(String))
     end
 
-    context 'when vulnerability is not linked to a commit' do
-      it 'uses the default branch' do
-        vulnerability.finding.pipelines = []
-        vulnerability.finding.save
-
-        expect(subject).to include(
-          vulnerability.project.default_branch
-        )
+    context 'when there is no file' do
+      before do
+        vulnerability.finding.location['file'] = nil
+        vulnerability.finding.location.delete('blob_path')
       end
-    end
-
-    context 'when vulnerability is not on a specific line' do
-      it 'does not include a reference to the line number' do
-        vulnerability.finding.location['start_line'] = nil
-        vulnerability.finding.save
 
-        expect(subject).not_to include('#L')
-        expect(subject).not_to match(/#{vulnerability.finding.location['file']}:\d*/)
+      it 'does not have a blob_path if there is no file' do
+        expect(subject[:location]).not_to have_key('blob_path')
       end
     end
   end

locale/gitlab.pot

@@ -373,6 +373,30 @@ msgstr ""
 msgid "%{issuesSize} issues with a limit of %{maxIssueCount}"
 msgstr ""
 
+msgid "%{labelStart}Class:%{labelEnd} %{class}"
+msgstr ""
+
+msgid "%{labelStart}Confidence:%{labelEnd} %{confidence}"
+msgstr ""
+
+msgid "%{labelStart}File:%{labelEnd} %{file}"
+msgstr ""
+
+msgid "%{labelStart}Image:%{labelEnd} %{image}"
+msgstr ""
+
+msgid "%{labelStart}Method:%{labelEnd} %{method}"
+msgstr ""
+
+msgid "%{labelStart}Namespace:%{labelEnd} %{namespace}"
+msgstr ""
+
+msgid "%{labelStart}Report Type:%{labelEnd} %{reportType}"
+msgstr ""
+
+msgid "%{labelStart}Severity:%{labelEnd} %{severity}"
+msgstr ""
+
 msgid "%{label_for_message} unavailable"
 msgstr ""
 
@@ -4386,9 +4410,6 @@ msgstr ""
 msgid "Class"
 msgstr ""
 
-msgid "Class:"
-msgstr ""
-
 msgid "Classification Label (optional)"
 msgstr ""
 
@@ -5741,9 +5762,6 @@ msgstr ""
 msgid "Confidence"
 msgstr ""
 
-msgid "Confidence: %{confidence}"
-msgstr ""
-
 msgid "Confidential"
 msgstr ""
 
@@ -9658,9 +9676,6 @@ msgstr ""
 msgid "File upload error."
 msgstr ""
 
-msgid "File:"
-msgstr ""
-
 msgid "Files"
 msgstr ""
 
@@ -11630,9 +11645,6 @@ msgstr ""
 msgid "Image URL"
 msgstr ""
 
-msgid "Image: %{image}"
-msgstr ""
-
 msgid "ImageDiffViewer|2-up"
 msgstr ""
 
@@ -13756,9 +13768,6 @@ msgstr ""
 msgid "Method"
 msgstr ""
 
-msgid "Method:"
-msgstr ""
-
 msgid "Metric was successfully added."
 msgstr ""
 
@@ -14348,9 +14357,6 @@ msgstr ""
 msgid "Namespace is empty"
 msgstr ""
 
-msgid "Namespace: %{namespace}"
-msgstr ""
-
 msgid "Namespaces to index"
 msgstr ""
 
@@ -18415,9 +18421,6 @@ msgstr ""
 msgid "Report %{display_issuable_type} that are abusive, inappropriate or spam."
 msgstr ""
 
-msgid "Report Type: %{report_type}"
-msgstr ""
-
 msgid "Report abuse"
 msgstr ""
 
@@ -20060,9 +20063,6 @@ msgstr ""
 msgid "Severity"
 msgstr ""
 
-msgid "Severity: %{severity}"
-msgstr ""
-
 msgid "Shards (%{shards})"
 msgstr ""