Merge branch 'blacklist-license-approval' into 'master'

License-Check approval tooltips

See merge request gitlab-org/gitlab!16371

app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js

@@ -169,6 +169,7 @@ export default class MergeRequestStore {
     this.mergeRequestPipelinesHelpPath = data.merge_request_pipelines_docs_path;
     this.conflictsDocsPath = data.conflicts_docs_path;
     this.ciEnvironmentsStatusPath = data.ci_environments_status_path;
+    this.securityApprovalsHelpPagePath = data.security_approvals_help_page_path;
   }
 
   get isNothingToMergeState() {

app/views/projects/merge_requests/show.html.haml

@@ -24,6 +24,7 @@
 
         window.gl.mrWidgetData.squash_before_merge_help_path = '#{help_page_path("user/project/merge_requests/squash_and_merge")}';
         window.gl.mrWidgetData.troubleshooting_docs_path = '#{help_page_path('user/project/merge_requests/index.md', anchor: 'troubleshooting')}';
+        window.gl.mrWidgetData.security_approvals_help_page_path = '#{help_page_path('user/application_security/index.html', anchor: 'security-approvals-in-merge-requests-ultimate')}';
 
     #js-vue-mr-widget.mr-widget
 

ee/app/assets/javascripts/approvals/components/vulnerability_check_popover.vue → ee/app/assets/javascripts/approvals/components/approval_check_popover.vue

@@ -3,7 +3,7 @@ import { GlLink, GlPopover } from '@gitlab/ui';
 import Icon from '~/vue_shared/components/icon.vue';
 
 export default {
-  name: 'VulnerabilityCheckPopover',
+  name: 'ApprovalCheckPopover',
   components: {
     GlLink,
     GlPopover,
@@ -15,25 +15,42 @@ export default {
       required: false,
       default: '',
     },
+    documentationText: {
+      type: String,
+      required: false,
+      default: '',
+    },
+    title: {
+      type: String,
+      required: true,
+    },
+    text: {
+      type: String,
+      required: false,
+      default: '',
+    },
   },
 };
 </script>
 
 <template>
   <span class="vertical-align-middle text-muted js-help">
-    <icon id="reports-info" name="question" :aria-label="__('help')" :size="14" />
+    <icon
+      ref="reportInfo"
+      name="question"
+      css-classes="text-info"
+      :aria-label="__('help')"
+      :size="14"
+    />
     <gl-popover
-      target="reports-info"
+      :target="() => $refs.reportInfo"
       placement="top"
-      triggers="click"
-      :title="
-        __(
-          'Vulnerability-Check requires one or more merge request approvals only if high or critical security vulnerabilities are detected.',
-        )
-      "
+      triggers="click blur"
+      :title="title"
     >
+      <div class="mb-2">{{ text }}</div>
       <gl-link v-if="documentationLink" target="_blank" :href="documentationLink">
-        <span class="vertical-align-middle">{{ __('Learn more about vulnerability check') }}</span>
+        <span class="vertical-align-middle">{{ documentationText }}</span>
         <icon name="external-link" class="vertical-align-middle" />
       </gl-link>
     </gl-popover>

ee/app/assets/javascripts/approvals/components/approval_check_rule_popover.vue

+<script>
+import ApprovalCheckPopover from './approval_check_popover.vue';
+import { VULNERABILITY_CHECK_NAME, LICENSE_CHECK_NAME, APPROVAL_RULE_CONFIGS } from '../constants';
+
+export default {
+  name: 'ApprovalCheckRulePopover',
+  components: {
+    ApprovalCheckPopover,
+  },
+  props: {
+    rule: {
+      type: Object,
+      required: true,
+    },
+    securityApprovalsHelpPagePath: {
+      type: String,
+      required: false,
+      default: '',
+    },
+  },
+  computed: {
+    showVulnerabilityCheckPopover() {
+      return this.rule.name === VULNERABILITY_CHECK_NAME;
+    },
+    showLicenseCheckPopover() {
+      return this.rule.name === LICENSE_CHECK_NAME;
+    },
+    showApprovalCheckPopover() {
+      return this.showVulnerabilityCheckPopover || this.showLicenseCheckPopover;
+    },
+    approvalRuleConfig() {
+      return APPROVAL_RULE_CONFIGS[this.rule.name];
+    },
+    documentationLink() {
+      /*
+       * The docs for these two rules have the same url & anchor
+       * We get the path from a rails view helper
+       */
+      if (this.showLicenseCheckPopover || this.showApprovalCheckPopover) {
+        return this.securityApprovalsHelpPagePath;
+      }
+      return '';
+    },
+  },
+};
+</script>
+
+<template>
+  <approval-check-popover
+    v-if="showApprovalCheckPopover"
+    :title="approvalRuleConfig.title"
+    :text="approvalRuleConfig.popoverText"
+    :documentation-link="documentationLink"
+    :documentation-text="approvalRuleConfig.documentationText"
+  />
+</template>

ee/app/assets/javascripts/approvals/components/project_settings/project_rules.vue

@@ -3,19 +3,17 @@ import { mapState } from 'vuex';
 import { n__, sprintf } from '~/locale';
 import Icon from '~/vue_shared/components/icon.vue';
 import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.vue';
-import VulnerabilityCheckPopover from '../vulnerability_check_popover.vue';
+import ApprovalCheckRulePopover from '../approval_check_rule_popover.vue';
 import Rules from '../rules.vue';
 import RuleControls from '../rule_controls.vue';
 
-const VULNERABILITY_CHECK_NAME = 'Vulnerability-Check';
-
 export default {
   components: {
     Icon,
     RuleControls,
     Rules,
     UserAvatarList,
-    VulnerabilityCheckPopover,
+    ApprovalCheckRulePopover,
   },
   computed: {
     ...mapState(['settings']),
@@ -58,9 +56,6 @@ export default {
         { name: rule.name, count: rule.approvalsRequired },
       );
     },
-    showVulnerabilityCheckPopover(rule) {
-      return rule.name === VULNERABILITY_CHECK_NAME;
-    },
   },
 };
 </script>
@@ -69,7 +64,7 @@ export default {
   <rules :rules="rules">
     <template slot="thead" slot-scope="{ name, members, approvalsRequired }">
       <tr class="d-none d-sm-table-row">
-        <th v-if="settings.allowMultiRule">{{ name }}</th>
+        <th v-if="settings.allowMultiRule" class="w-25">{{ name }}</th>
         <th class="w-50">{{ members }}</th>
         <th>{{ approvalsRequired }}</th>
         <th></th>
@@ -79,7 +74,10 @@ export default {
       <td class="d-table-cell d-sm-none js-summary">{{ summaryText(rule) }}</td>
       <td v-if="settings.allowMultiRule" class="d-none d-sm-table-cell js-name">
         {{ rule.name }}
-        <vulnerability-check-popover v-if="showVulnerabilityCheckPopover(rule)" />
+        <approval-check-rule-popover
+          :rule="rule"
+          :security-approvals-help-page-path="settings.securityApprovalsHelpPagePath"
+        />
       </td>
       <td class="d-none d-sm-table-cell js-members">
         <user-avatar-list :items="rule.approvers" :img-size="24" />

ee/app/assets/javascripts/approvals/constants.js

+import { __ } from '~/locale';
+
 export const TYPE_USER = 'user';
 export const TYPE_GROUP = 'group';
 export const TYPE_HIDDEN_GROUPS = 'hidden_groups';
 
 export const RULE_TYPE_FALLBACK = 'fallback';
 export const RULE_TYPE_REGULAR = 'regular';
+export const RULE_TYPE_REPORT_APPROVER = 'report_approver';
 export const RULE_TYPE_CODE_OWNER = 'code_owner';
+
+export const VULNERABILITY_CHECK_NAME = 'Vulnerability-Check';
+export const LICENSE_CHECK_NAME = 'License-Check';
+
+export const APPROVAL_RULE_CONFIGS = {
+  [VULNERABILITY_CHECK_NAME]: {
+    title: __('Vulnerability-Check'),
+    popoverText: __(
+      'A merge request approval is required when a security report contains a new vulnerability of high, critical, or unknown severity.',
+    ),
+    documentationText: __('Learn more about Vulnerability-Check'),
+  },
+  [LICENSE_CHECK_NAME]: {
+    title: __('License-Check'),
+    popoverText: __(
+      'A merge request approval is required when the license compliance report contains a blacklisted license.',
+    ),
+    documentationText: __('Learn more about License-Check'),
+  },
+};

ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals.vue

@@ -242,6 +242,7 @@ export default {
       :suggested-approvers="approvals.suggested_approvers"
       :approval-rules="mr.approvalRules"
       :is-loading-rules="isLoadingRules"
+      :security-approvals-help-page-path="mr.securityApprovalsHelpPagePath"
     />
   </mr-widget-container>
 </template>

ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_footer.vue

@@ -32,6 +32,11 @@ export default {
       required: false,
       default: false,
     },
+    securityApprovalsHelpPagePath: {
+      type: String,
+      required: false,
+      default: '',
+    },
   },
   computed: {
     isCollapsed() {
@@ -76,7 +81,10 @@ export default {
       </template>
     </div>
     <div v-if="!isCollapsed && approvalRules.length" class="border-top">
-      <approvals-list :approval-rules="approvalRules" />
+      <approvals-list
+        :approval-rules="approvalRules"
+        :security-approvals-help-page-path="securityApprovalsHelpPagePath"
+      />
     </div>
   </div>
 </template>

ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_list.vue

@@ -2,6 +2,7 @@
 import _ from 'underscore';
 import { sprintf, __ } from '~/locale';
 import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.vue';
+import ApprovalCheckRulePopover from 'ee/approvals/components/approval_check_rule_popover.vue';
 import { RULE_TYPE_CODE_OWNER } from 'ee/approvals/constants';
 import ApprovedIcon from './approved_icon.vue';
 
@@ -9,12 +10,18 @@ export default {
   components: {
     UserAvatarList,
     ApprovedIcon,
+    ApprovalCheckRulePopover,
   },
   props: {
     approvalRules: {
       type: Array,
       required: true,
     },
+    securityApprovalsHelpPagePath: {
+      type: String,
+      required: false,
+      default: '',
+    },
   },
   computed: {
     sections() {
@@ -72,7 +79,7 @@ export default {
     <thead class="thead-white text-nowrap">
       <tr class="d-none d-sm-table-row">
         <th class="w-0"></th>
-        <th>{{ s__('MRApprovals|Approvers') }}</th>
+        <th class="w-25">{{ s__('MRApprovals|Approvers') }}</th>
         <th class="w-50"></th>
         <th>{{ s__('MRApprovals|Pending approvals') }}</th>
         <th>{{ s__('MRApprovals|Approved by') }}</th>
@@ -88,7 +95,13 @@ export default {
       <tr v-for="rule in rules" :key="rule.id">
         <td class="w-0"><approved-icon :is-approved="rule.approved" /></td>
         <td :colspan="rule.fallback ? 2 : 1">
-          <div class="d-none d-sm-block js-name" :class="rule.nameClass">{{ rule.name }}</div>
+          <div class="d-none d-sm-block js-name" :class="rule.nameClass">
+            {{ rule.name }}
+            <approval-check-rule-popover
+              :rule="rule"
+              :security-approvals-help-page-path="securityApprovalsHelpPagePath"
+            />
+          </div>
           <div class="d-flex d-sm-none flex-column js-summary">
             <span>{{ summaryText(rule) }}</span>
             <user-avatar-list

ee/app/assets/javascripts/vue_merge_request_widget/mappers.js

@@ -4,6 +4,7 @@ import {
   RULE_TYPE_REGULAR,
   RULE_TYPE_FALLBACK,
   RULE_TYPE_CODE_OWNER,
+  RULE_TYPE_REPORT_APPROVER,
 } from 'ee/approvals/constants';
 
 function mapApprovalRule(rule, settings) {
@@ -35,11 +36,14 @@ function getApprovalRuleNamesLeft(data) {
   // Filter out empty names (fallback rule has no name) because the empties would look weird.
   const regularRules = (rulesLeft[RULE_TYPE_REGULAR] || []).map(x => x.name).filter(x => x);
 
+  // Report Approvals
+  const reportApprovalRules = (rulesLeft[RULE_TYPE_REPORT_APPROVER] || []).map(x => x.name);
+
   // If there are code owners that need to approve, only mention that once.
   // As the names of code owner rules are patterns that don't mean much out of context.
   const codeOwnerRules = rulesLeft[RULE_TYPE_CODE_OWNER] ? [__('Code Owners')] : [];
 
-  return [...regularRules, ...codeOwnerRules];
+  return [...regularRules, ...reportApprovalRules, ...codeOwnerRules];
 }
 
 /**

ee/app/views/projects/_merge_request_approvals_settings_form.html.haml

@@ -7,7 +7,8 @@
     'project_path': expose_path(api_v4_projects_path(id: @project.id)),
     'settings_path': expose_path(api_v4_projects_approval_settings_path(id: @project.id)),
     'rules_path': expose_path(api_v4_projects_approval_settings_rules_path(id: @project.id)),
-    'allow_multi_rule': @project.multiple_approval_rules_available?.to_s } }
+    'allow_multi_rule': @project.multiple_approval_rules_available?.to_s,
+    'security_approvals_help_page_path': help_page_path('user/application_security/index.html', anchor: 'security-approvals-in-merge-requests-ultimate')} }
     .text-center.prepend-top-default
       = sprite_icon('spinner', size: 24, css_class: 'gl-spinner')
 

ee/changelogs/unreleased/blacklist-license-approval.yml

+---
+title: Add License-Check approval UI
+merge_request: 16371
+author:
+type: added

ee/spec/frontend/approvals/components/approval_check_rule_popover_spec.js

+import Vue from 'vue';
+import { shallowMount, createLocalVue } from '@vue/test-utils';
+import { TEST_HOST } from 'helpers/test_constants';
+import component from 'ee/approvals/components/approval_check_rule_popover.vue';
+import {
+  VULNERABILITY_CHECK_NAME,
+  LICENSE_CHECK_NAME,
+  APPROVAL_RULE_CONFIGS,
+} from 'ee/approvals/constants';
+
+describe('Approval Check Popover', () => {
+  let wrapper;
+
+  beforeEach(() => {
+    const localVue = createLocalVue();
+    wrapper = shallowMount(component, {
+      localVue,
+      propsData: { rule: {} },
+      sync: false,
+    });
+  });
+
+  describe('computed props', () => {
+    const securityApprovalsHelpPagePath = `${TEST_HOST}/documentation`;
+
+    beforeEach(done => {
+      wrapper.setProps({ securityApprovalsHelpPagePath });
+      Vue.nextTick(done);
+    });
+
+    describe('showVulnerabilityCheckPopover', () => {
+      it('return true if the rule type is "Vulnerability-Check"', done => {
+        wrapper.setProps({ rule: { name: VULNERABILITY_CHECK_NAME } });
+        Vue.nextTick(() => {
+          expect(wrapper.vm.showVulnerabilityCheckPopover).toBe(true);
+          done();
+        });
+      });
+      it('return false if the rule type is "Vulnerability-Check"', done => {
+        wrapper.setProps({ rule: { name: 'FooRule' } });
+        Vue.nextTick(() => {
+          expect(wrapper.vm.showVulnerabilityCheckPopover).toBe(false);
+          done();
+        });
+      });
+    });
+
+    describe('showLicenseCheckPopover', () => {
+      it('return true if the rule type is "License-Check"', done => {
+        wrapper.setProps({ rule: { name: LICENSE_CHECK_NAME } });
+        Vue.nextTick(() => {
+          expect(wrapper.vm.showLicenseCheckPopover).toBe(true);
+          done();
+        });
+      });
+      it('return false if the rule type is "License-Check"', done => {
+        wrapper.setProps({ rule: { name: 'FooRule' } });
+        Vue.nextTick(() => {
+          expect(wrapper.vm.showLicenseCheckPopover).toBe(false);
+          done();
+        });
+      });
+    });
+
+    describe('approvalConfig', () => {
+      it('returns "Vulberability-Check" config', done => {
+        wrapper.setProps({ rule: { name: VULNERABILITY_CHECK_NAME } });
+        Vue.nextTick(() => {
+          expect(wrapper.vm.approvalRuleConfig.title).toBe(
+            APPROVAL_RULE_CONFIGS[VULNERABILITY_CHECK_NAME].title,
+          );
+          expect(wrapper.vm.approvalRuleConfig.popoverText).toBe(
+            APPROVAL_RULE_CONFIGS[VULNERABILITY_CHECK_NAME].popoverText,
+          );
+          expect(wrapper.vm.approvalRuleConfig.documentationText).toBe(
+            APPROVAL_RULE_CONFIGS[VULNERABILITY_CHECK_NAME].documentationText,
+          );
+          done();
+        });
+      });
+      it('returns "License-Check" config', done => {
+        wrapper.setProps({ rule: { name: LICENSE_CHECK_NAME } });
+        Vue.nextTick(() => {
+          expect(wrapper.vm.approvalRuleConfig.title).toBe(
+            APPROVAL_RULE_CONFIGS[LICENSE_CHECK_NAME].title,
+          );
+          expect(wrapper.vm.approvalRuleConfig.popoverText).toBe(
+            APPROVAL_RULE_CONFIGS[LICENSE_CHECK_NAME].popoverText,
+          );
+          expect(wrapper.vm.approvalRuleConfig.documentationText).toBe(
+            APPROVAL_RULE_CONFIGS[LICENSE_CHECK_NAME].documentationText,
+          );
+          done();
+        });
+      });
+      it('returns an undefined config', done => {
+        wrapper.setProps({ rule: { name: 'FooRule' } });
+        Vue.nextTick(() => {
+          expect(wrapper.vm.approvalConfig).toBe(undefined);
+          done();
+        });
+      });
+    });
+
+    describe('documentationLink', () => {
+      it('returns documentation link for "License-Check"', done => {
+        wrapper.setProps({ rule: { name: 'License-Check' } });
+        Vue.nextTick(() => {
+          expect(wrapper.vm.documentationLink).toBe(securityApprovalsHelpPagePath);
+          done();
+        });
+      });
+      it('returns documentation link for "Vulnerability-Check"', done => {
+        wrapper.setProps({ rule: { name: 'Vulnerability-Check' } });
+        Vue.nextTick(() => {
+          expect(wrapper.vm.documentationLink).toBe(securityApprovalsHelpPagePath);
+          done();
+        });
+      });
+      it('returns empty text', done => {
+        const text = '';
+        wrapper.setProps({ rule: { name: 'FooRule' } });
+        Vue.nextTick(() => {
+          expect(wrapper.vm.documentationLink).toBe(text);
+          done();
+        });
+      });
+    });
+  });
+});

ee/spec/frontend/vue_mr_widget/mappers_spec.js

 import { mapApprovalsResponse } from 'ee/vue_merge_request_widget/mappers';
-import { RULE_TYPE_REGULAR, RULE_TYPE_CODE_OWNER } from 'ee/approvals/constants';
+import {
+  RULE_TYPE_REGULAR,
+  RULE_TYPE_CODE_OWNER,
+  RULE_TYPE_REPORT_APPROVER,
+} from 'ee/approvals/constants';
 
 describe('EE MR Widget mappers', () => {
   let data;
@@ -30,6 +34,21 @@ describe('EE MR Widget mappers', () => {
         );
       });
 
+      it('approvalRuleNamesLeft includes report approvers', () => {
+        data.approval_rules_left.push(
+          { name: 'License-Check', rule_type: RULE_TYPE_REPORT_APPROVER },
+          { name: 'Vulnerability-Check', rule_type: RULE_TYPE_REPORT_APPROVER },
+        );
+
+        const result = mapApprovalsResponse(data);
+
+        expect(result).toEqual(
+          jasmine.objectContaining({
+            approvalRuleNamesLeft: ['Lorem', 'Ipsum', 'License-Check', 'Vulnerability-Check'],
+          }),
+        );
+      });
+
       it('approvalRuleNamesLeft includes "Code Owners" if any', () => {
         data.approval_rules_left.push(
           { name: 'src/foo', rule_type: RULE_TYPE_CODE_OWNER },

ee/spec/javascripts/approvals/components/vulnerability_check_popover_spec.js → ee/spec/javascripts/approvals/components/approval_check_popover_spec.js

@@ -2,15 +2,18 @@ import Vue from 'vue';
 import { mount, createLocalVue } from '@vue/test-utils';
 import { GlPopover } from '@gitlab/ui';
 import { TEST_HOST } from 'spec/test_constants';
-import component from 'ee/approvals/components/vulnerability_check_popover.vue';
+import component from 'ee/approvals/components/approval_check_popover.vue';
 
-describe('Vulnerability Check Popover', () => {
+describe('Approval Check Popover', () => {
   let wrapper;
 
   beforeEach(() => {
     const localVue = createLocalVue();
     wrapper = mount(component, {
       localVue,
+      propsData: {
+        title: 'Title',
+      },
       sync: false,
     });
   });

locale/gitlab.pot

@@ -653,6 +653,12 @@ msgstr ""
 msgid "A member of the abuse team will review your report as soon as possible."
 msgstr ""
 
+msgid "A merge request approval is required when a security report contains a new vulnerability of high, critical, or unknown severity."
+msgstr ""
+
+msgid "A merge request approval is required when the license compliance report contains a blacklisted license."
+msgstr ""
+
 msgid "A new branch will be created in your fork and a new merge request will be started."
 msgstr ""
 
@@ -9464,6 +9470,12 @@ msgstr ""
 msgid "Learn more about Kubernetes"
 msgstr ""
 
+msgid "Learn more about License-Check"
+msgstr ""
+
+msgid "Learn more about Vulnerability-Check"
+msgstr ""
+
 msgid "Learn more about Web Terminal"
 msgstr ""
 
@@ -9488,9 +9500,6 @@ msgstr ""
 msgid "Learn more about the dependency list"
 msgstr ""
 
-msgid "Learn more about vulnerability check"
-msgstr ""
-
 msgid "Learn more in the"
 msgstr ""
 
@@ -9527,6 +9536,9 @@ msgstr ""
 msgid "License Compliance"
 msgstr ""
 
+msgid "License-Check"
+msgstr ""
+
 msgid "LicenseCompliance|Add a license"
 msgstr ""
 
@@ -18216,7 +18228,7 @@ msgstr ""
 msgid "Vulnerabilities over time"
 msgstr ""
 
-msgid "Vulnerability-Check requires one or more merge request approvals only if high or critical security vulnerabilities are detected."
+msgid "Vulnerability-Check"
 msgstr ""
 
 msgid "VulnerabilityChart|%{formattedStartDate} to today"