Merge branch '341420-security-policy-already-exists' into 'master'

Fix bug with assignSecurityPolicyProject

See merge request gitlab-org/gitlab!76960

ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_execution_policy/lib/constants.js

@@ -5,7 +5,7 @@ export const DEFAULT_MR_TITLE = s__('SecurityOrchestration|Update scan execution
 export const GRAPHQL_ERROR_MESSAGE = s__(
   'SecurityOrchestration|There was a problem creating the new security policy',
 );
-export const NO_RULE_MESSAGE = s__('SecurityOrhestration|No rules defined - policy will not run.');
+export const NO_RULE_MESSAGE = s__('SecurityOrchestration|No rules defined - policy will not run.');
 
 export const SECURITY_POLICY_ACTIONS = {
   APPEND: 'APPEND',

ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_execution_policy/lib/utils.js

@@ -14,26 +14,6 @@ const checkForErrors = ({ errors }) => {
   }
 };
 
-/**
- * Creates a new security policy project and assigns it to the current project
- * @param {String} projectPath
- * @returns {Object} contains the new security policy project and any errors
- */
-const assignSecurityPolicyProject = async (projectPath) => {
-  const {
-    data: {
-      securityPolicyProjectCreate: { project, errors },
-    },
-  } = await gqClient.mutate({
-    mutation: createPolicyProject,
-    variables: {
-      projectPath,
-    },
-  });
-
-  return { ...project, branch: project?.branch?.rootRef, errors };
-};
-
 /**
  * Creates a merge request for the changes to the policy file
  * @param {Object} payload contains the path to the parent project, the branch to merge on the project, and the branch to merge into
@@ -102,14 +82,6 @@ export const modifyPolicy = async ({
   projectPath,
   yamlEditorValue,
 }) => {
-  let currentAssignedPolicyProject = assignedPolicyProject;
-
-  if (!currentAssignedPolicyProject.fullPath) {
-    currentAssignedPolicyProject = await assignSecurityPolicyProject(projectPath);
-  }
-
-  checkForErrors(currentAssignedPolicyProject);
-
   const newPolicyCommitBranch = await updatePolicy({
     action,
     name,
@@ -120,12 +92,34 @@ export const modifyPolicy = async ({
   checkForErrors(newPolicyCommitBranch);
 
   const mergeRequest = await createMergeRequest({
-    projectPath: currentAssignedPolicyProject.fullPath,
+    projectPath: assignedPolicyProject.fullPath,
     sourceBranch: newPolicyCommitBranch.branch,
-    targetBranch: currentAssignedPolicyProject.branch,
+    targetBranch: assignedPolicyProject.branch,
   });
 
   checkForErrors(mergeRequest);
 
-  return { mergeRequest, policyProject: currentAssignedPolicyProject };
+  return mergeRequest;
+};
+
+/**
+ * Creates a new security policy project and assigns it to the current project
+ * @param {String} projectPath
+ * @returns {Object} contains the new security policy project and any errors
+ */
+export const assignSecurityPolicyProject = async (projectPath) => {
+  const {
+    data: {
+      securityPolicyProjectCreate: { project, errors },
+    },
+  } = await gqClient.mutate({
+    mutation: createPolicyProject,
+    variables: {
+      projectPath,
+    },
+  });
+
+  checkForErrors({ errors });
+
+  return { ...project, branch: project?.branch?.rootRef, errors };
 };

ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_execution_policy/scan_execution_policy_editor.vue

@@ -5,6 +5,7 @@ import { __, s__ } from '~/locale';
 import { EDITOR_MODES, EDITOR_MODE_YAML } from '../constants';
 import PolicyEditorLayout from '../policy_editor_layout.vue';
 import {
+  assignSecurityPolicyProject,
   DEFAULT_SCAN_EXECUTION_POLICY,
   fromYaml,
   GRAPHQL_ERROR_MESSAGE,
@@ -60,6 +61,7 @@ export default {
       error: '',
       isCreatingMR: false,
       isRemovingPolicy: false,
+      newlyCreatedPolicyProject: null,
       policy: fromYaml(yamlEditorValue),
       yamlEditorValue,
     };
@@ -77,6 +79,13 @@ export default {
         this.$emit('error', error.message);
       }
     },
+    async getSecurityPolicyProject() {
+      if (!this.newlyCreatedPolicyProject && !this.assignedPolicyProject.fullPath) {
+        this.newlyCreatedPolicyProject = await assignSecurityPolicyProject(this.projectPath);
+      }
+
+      return this.newlyCreatedPolicyProject || this.assignedPolicyProject;
+    },
     async handleModifyPolicy(act) {
       const action =
         act ||
@@ -88,15 +97,17 @@ export default {
       this.setLoadingFlag(action, true);
 
       try {
-        const { mergeRequest, policyProject } = await modifyPolicy({
+        const assignedPolicyProject = await this.getSecurityPolicyProject();
+
+        const mergeRequest = await modifyPolicy({
           action,
-          assignedPolicyProject: this.assignedPolicyProject,
+          assignedPolicyProject,
           name: this.originalName || fromYaml(this.yamlEditorValue)?.name,
           projectPath: this.projectPath,
           yamlEditorValue: this.yamlEditorValue,
         });
 
-        this.redirectToMergeRequest({ mergeRequest, policyProject });
+        this.redirectToMergeRequest({ mergeRequest, assignedPolicyProject });
       } catch (e) {
         this.handleError(e);
         this.setLoadingFlag(action, false);
@@ -109,11 +120,11 @@ export default {
         this.isCreatingMR = val;
       }
     },
-    redirectToMergeRequest({ mergeRequest, policyProject }) {
+    redirectToMergeRequest({ mergeRequest, assignedPolicyProject }) {
       visitUrl(
         joinPaths(
           gon.relative_url_root || '/',
-          policyProject.fullPath,
+          assignedPolicyProject.fullPath,
           '/-/merge_requests',
           mergeRequest.id,
         ),

ee/spec/frontend/threat_monitoring/components/policy_editor/scan_execution_policy/lib/utils_spec.js

-import { modifyPolicy } from 'ee/threat_monitoring/components/policy_editor/scan_execution_policy/lib/utils';
+import {
+  assignSecurityPolicyProject,
+  modifyPolicy,
+} from 'ee/threat_monitoring/components/policy_editor/scan_execution_policy/lib/utils';
 import { DEFAULT_ASSIGNED_POLICY_PROJECT } from 'ee/threat_monitoring/constants';
 import createPolicyProject from 'ee/threat_monitoring/graphql/mutations/create_policy_project.mutation.graphql';
 import createScanExecutionPolicy from 'ee/threat_monitoring/graphql/mutations/create_scan_execution_policy.mutation.graphql';
@@ -33,7 +36,7 @@ const mockApolloResponses = (shouldReject) => {
         data: {
           securityPolicyProjectCreate: {
             project: newAssignedPolicyProject,
-            errors: [],
+            errors: shouldReject ? [error] : [],
           },
         },
       });
@@ -55,30 +58,42 @@ const mockApolloResponses = (shouldReject) => {
   };
 };
 
-describe('modifyPolicy', () => {
-  it('returns the policy project and merge request on success when a policy project does not exist', async () => {
+describe('assignSecurityPolicyProject', () => {
+  it('returns the newly created policy project', async () => {
     gqClient.mutate.mockImplementation(mockApolloResponses());
 
-    const { mergeRequest, policyProject } = await modifyPolicy(
-      createSavePolicyInput(DEFAULT_ASSIGNED_POLICY_PROJECT),
-    );
+    const newlyCreatedPolicyProject = await assignSecurityPolicyProject(projectPath);
 
-    expect(policyProject).toStrictEqual({
-      id: newAssignedPolicyProject.id,
-      fullPath: newAssignedPolicyProject.fullPath,
-      branch: newAssignedPolicyProject.branch.rootRef,
+    expect(newlyCreatedPolicyProject).toStrictEqual({
+      branch: 'main',
+      id: '02',
       errors: [],
+      fullPath: 'path/to/new-project',
     });
+  });
+
+  it('throws when an error is detected', async () => {
+    gqClient.mutate.mockImplementation(mockApolloResponses(true));
+
+    await expect(assignSecurityPolicyProject(projectPath)).rejects.toThrowError(error);
+  });
+});
+
+describe('modifyPolicy', () => {
+  it('returns the policy project and merge request on success when a policy project does not exist', async () => {
+    gqClient.mutate.mockImplementation(mockApolloResponses());
+
+    const mergeRequest = await modifyPolicy(createSavePolicyInput(DEFAULT_ASSIGNED_POLICY_PROJECT));
+
     expect(mergeRequest).toStrictEqual({ id: '01', errors: [] });
   });
 
   it('returns the policy project and merge request on success when a policy project does exist', async () => {
     gqClient.mutate.mockImplementation(mockApolloResponses());
 
-    const { mergeRequest, policyProject } = await modifyPolicy(createSavePolicyInput());
+    const mergeRequest = await modifyPolicy(createSavePolicyInput());
 
     expect(mergeRequest).toStrictEqual({ id: '01', errors: [] });
-    expect(policyProject).toStrictEqual(defaultAssignedPolicyProject);
   });
 
   it('throws when an error is detected', async () => {

ee/spec/frontend/threat_monitoring/components/policy_editor/scan_execution_policy/scan_execution_policy_editor_spec.js

 import { shallowMount } from '@vue/test-utils';
 import { GlEmptyState } from '@gitlab/ui';
+import waitForPromises from 'helpers/wait_for_promises';
 import PolicyEditorLayout from 'ee/threat_monitoring/components/policy_editor/policy_editor_layout.vue';
 import {
   DEFAULT_SCAN_EXECUTION_POLICY,
@@ -20,7 +21,16 @@ jest.mock('~/lib/utils/url_utility', () => ({
   visitUrl: jest.fn().mockName('visitUrlMock'),
 }));
 
+const newlyCreatedPolicyProject = {
+  branch: 'main',
+  fullPath: 'path/to/new-project',
+};
+
 jest.mock('ee/threat_monitoring/components/policy_editor/scan_execution_policy/lib', () => ({
+  assignSecurityPolicyProject: jest.fn().mockResolvedValue({
+    branch: 'main',
+    fullPath: 'path/to/new-project',
+  }),
   DEFAULT_SCAN_EXECUTION_POLICY: jest.requireActual(
     'ee/threat_monitoring/components/policy_editor/scan_execution_policy/lib',
   ).DEFAULT_SCAN_EXECUTION_POLICY,
@@ -33,10 +43,7 @@ jest.mock('ee/threat_monitoring/components/policy_editor/scan_execution_policy/l
   SECURITY_POLICY_ACTIONS: jest.requireActual(
     'ee/threat_monitoring/components/policy_editor/scan_execution_policy/lib',
   ).SECURITY_POLICY_ACTIONS,
-  modifyPolicy: jest.fn().mockResolvedValue({
-    mergeRequest: { id: '2' },
-    policyProject: { fullPath: 'tests' },
-  }),
+  modifyPolicy: jest.fn().mockResolvedValue({ id: '2' }),
 }));
 
 describe('ScanExecutionPolicyEditor', () => {
@@ -44,6 +51,10 @@ describe('ScanExecutionPolicyEditor', () => {
   const defaultProjectPath = 'path/to/project';
   const policyEditorEmptyStateSvgPath = 'path/to/svg';
   const scanExecutionDocumentationPath = 'path/to/docs';
+  const assignedPolicyProject = {
+    branch: 'main',
+    fullPath: 'path/to/existing-project',
+  };
 
   const factory = ({ propsData = {}, provide = {} } = {}) => {
     wrapper = shallowMount(ScanExecutionPolicyEditor, {
@@ -63,7 +74,13 @@ describe('ScanExecutionPolicyEditor', () => {
   };
 
   const factoryWithExistingPolicy = () => {
-    return factory({ propsData: { existingPolicy: mockDastScanExecutionObject, isEditing: true } });
+    return factory({
+      propsData: {
+        assignedPolicyProject,
+        existingPolicy: mockDastScanExecutionObject,
+        isEditing: true,
+      },
+    });
   };
 
   const findEmptyState = () => wrapper.findComponent(GlEmptyState);
@@ -86,21 +103,21 @@ describe('ScanExecutionPolicyEditor', () => {
     });
 
     it.each`
-      status                            | action                             | event              | factoryFn                    | yamlEditorValue
-      ${'to save a new policy'}         | ${SECURITY_POLICY_ACTIONS.APPEND}  | ${'save-policy'}   | ${factory}                   | ${DEFAULT_SCAN_EXECUTION_POLICY}
-      ${'to update an existing policy'} | ${SECURITY_POLICY_ACTIONS.REPLACE} | ${'save-policy'}   | ${factoryWithExistingPolicy} | ${mockDastScanExecutionManifest}
-      ${'to delete an existing policy'} | ${SECURITY_POLICY_ACTIONS.REMOVE}  | ${'remove-policy'} | ${factoryWithExistingPolicy} | ${mockDastScanExecutionManifest}
+      status                            | action                             | event              | factoryFn                    | yamlEditorValue                  | currentlyAssignedPolicyProject
+      ${'to save a new policy'}         | ${SECURITY_POLICY_ACTIONS.APPEND}  | ${'save-policy'}   | ${factory}                   | ${DEFAULT_SCAN_EXECUTION_POLICY} | ${newlyCreatedPolicyProject}
+      ${'to update an existing policy'} | ${SECURITY_POLICY_ACTIONS.REPLACE} | ${'save-policy'}   | ${factoryWithExistingPolicy} | ${mockDastScanExecutionManifest} | ${assignedPolicyProject}
+      ${'to delete an existing policy'} | ${SECURITY_POLICY_ACTIONS.REMOVE}  | ${'remove-policy'} | ${factoryWithExistingPolicy} | ${mockDastScanExecutionManifest} | ${assignedPolicyProject}
     `(
       'navigates to the new merge request when "modifyPolicy" is emitted $status',
-      async ({ action, event, factoryFn, yamlEditorValue }) => {
+      async ({ action, event, factoryFn, yamlEditorValue, currentlyAssignedPolicyProject }) => {
         factoryFn();
         await wrapper.vm.$nextTick();
         findPolicyEditorLayout().vm.$emit(event);
-        await wrapper.vm.$nextTick();
+        await waitForPromises();
         expect(modifyPolicy).toHaveBeenCalledTimes(1);
         expect(modifyPolicy).toHaveBeenCalledWith({
           action,
-          assignedPolicyProject: DEFAULT_ASSIGNED_POLICY_PROJECT,
+          assignedPolicyProject: currentlyAssignedPolicyProject,
           name:
             action === SECURITY_POLICY_ACTIONS.APPEND
               ? fromYaml(yamlEditorValue).name
@@ -110,7 +127,9 @@ describe('ScanExecutionPolicyEditor', () => {
         });
         await wrapper.vm.$nextTick();
         expect(visitUrl).toHaveBeenCalled();
-        expect(visitUrl).toHaveBeenCalledWith('/tests/-/merge_requests/2');
+        expect(visitUrl).toHaveBeenCalledWith(
+          `/${currentlyAssignedPolicyProject.fullPath}/-/merge_requests/2`,
+        );
       },
     );
   });

locale/gitlab.pot

@@ -31363,6 +31363,9 @@ msgstr ""
 msgid "SecurityOrchestration|New policy"
 msgstr ""
 
+msgid "SecurityOrchestration|No rules defined - policy will not run."
+msgstr ""
+
 msgid "SecurityOrchestration|Only owners can update Security Policy Project"
 msgstr ""
 
@@ -31447,9 +31450,6 @@ msgstr ""
 msgid "SecurityOrchestration|view results"
 msgstr ""
 
-msgid "SecurityOrhestration|No rules defined - policy will not run."
-msgstr ""
-
 msgid "SecurityPolicies|+%{count} more"
 msgstr ""