fix(approval-policies): improve policies handling

backend/src/ee/routes/v1/access-approval-policy-router.ts

@@ -19,7 +19,7 @@ export const registerAccessApprovalPolicyRouter = async (server: FastifyZodProvi
       body: z.object({
         projectSlug: z.string().trim(),
         name: z.string().optional(),
-        secretPath: z.string().trim().default("/"),
+        secretPath: z.string().trim().min(1, { message: "Secret path cannot be empty" }),
         environment: z.string(),
         approvers: z
           .discriminatedUnion("type", [
@@ -171,11 +171,7 @@ export const registerAccessApprovalPolicyRouter = async (server: FastifyZodProvi
       }),
       body: z.object({
         name: z.string().optional(),
-        secretPath: z
+        secretPath: z.string().trim().optional(),
-          .string()
-          .trim()
-          .optional()
-          .transform((val) => (val === "" ? "/" : val)),
         approvers: z
           .discriminatedUnion("type", [
             z.object({

backend/src/ee/routes/v1/secret-approval-policy-router.ts

@@ -102,8 +102,7 @@ export const registerSecretApprovalPolicyRouter = async (server: FastifyZodProvi
           .string()
           .optional()
           .nullable()
-          .transform((val) => (val ? removeTrailingSlash(val) : val))
+          .transform((val) => (val ? removeTrailingSlash(val) : val)),
-          .transform((val) => (val === "" ? "/" : val)),
         enforcementLevel: z.nativeEnum(EnforcementLevel).optional(),
         allowedSelfApprovals: z.boolean().default(true)
       }),

backend/src/ee/services/access-approval-policy/access-approval-policy-service.ts

@@ -60,6 +60,26 @@ export const accessApprovalPolicyServiceFactory = ({
   accessApprovalRequestReviewerDAL,
   orgMembershipDAL
 }: TAccessApprovalPolicyServiceFactoryDep): TAccessApprovalPolicyServiceFactory => {
+  const $policyExists = async ({
+    envId,
+    secretPath,
+    policyId
+  }: {
+    envId: string;
+    secretPath: string;
+    policyId?: string;
+  }) => {
+    const policy = await accessApprovalPolicyDAL
+      .findOne({
+        envId,
+        secretPath,
+        deletedAt: null
+      })
+      .catch(() => null);
+
+    return policyId ? policy && policy.id !== policyId : Boolean(policy);
+  };
+
   const createAccessApprovalPolicy: TAccessApprovalPolicyServiceFactory["createAccessApprovalPolicy"] = async ({
     name,
     actor,
@@ -106,6 +126,12 @@ export const accessApprovalPolicyServiceFactory = ({
     const env = await projectEnvDAL.findOne({ slug: environment, projectId: project.id });
     if (!env) throw new NotFoundError({ message: `Environment with slug '${environment}' not found` });
 
+    if (await $policyExists({ envId: env.id, secretPath })) {
+      throw new BadRequestError({
+        message: `A policy for secret path '${secretPath}' already exists in environment '${environment}'`
+      });
+    }
+
     let approverUserIds = userApprovers;
     if (userApproverNames.length) {
       const approverUsersInDB = await userDAL.find({
@@ -290,6 +316,24 @@ export const accessApprovalPolicyServiceFactory = ({
       throw new BadRequestError({ message: "Approvals cannot be greater than approvers" });
     }
 
+    // Case: Previously we allowed secret path to be null, but now we don't.
+    // This check ensures that we have a secret path to match with for finding conflicting policies.
+    if (!secretPath && !accessApprovalPolicy.secretPath) {
+      throw new BadRequestError({ message: "Secret path is required to update the policy" });
+    }
+
+    if (
+      await $policyExists({
+        envId: accessApprovalPolicy.envId,
+        secretPath: secretPath || accessApprovalPolicy.secretPath || "",
+        policyId: accessApprovalPolicy.id
+      })
+    ) {
+      throw new BadRequestError({
+        message: `A policy for secret path '${secretPath}' already exists in environment '${accessApprovalPolicy.environment.slug}'`
+      });
+    }
+
     if (!accessApprovalPolicy) {
       throw new NotFoundError({ message: `Secret approval policy with ID '${policyId}' not found` });
     }

backend/src/ee/services/secret-approval-policy/secret-approval-policy-service.ts

@@ -5,6 +5,7 @@ import { TPermissionServiceFactory } from "@app/ee/services/permission/permissio
 import { ProjectPermissionActions, ProjectPermissionSub } from "@app/ee/services/permission/project-permission";
 import { BadRequestError, NotFoundError } from "@app/lib/errors";
 import { removeTrailingSlash } from "@app/lib/fn";
+import { logger } from "@app/lib/logger";
 import { containsGlobPatterns } from "@app/lib/picomatch";
 import { TProjectEnvDALFactory } from "@app/services/project-env/project-env-dal";
 import { TUserDALFactory } from "@app/services/user/user-dal";
@@ -55,6 +56,27 @@ export const secretApprovalPolicyServiceFactory = ({
   licenseService,
   secretApprovalRequestDAL
 }: TSecretApprovalPolicyServiceFactoryDep) => {
+  const $policyExists = async ({
+    envId,
+    secretPath,
+    policyId
+  }: {
+    envId: string;
+    secretPath?: string | null;
+    policyId?: string;
+  }) => {
+    const policy = await secretApprovalPolicyDAL
+      .findOne({
+        envId,
+        // For environment-wide policies, we store the path as an empty string, even though the column is nullable; for that reason we check for an empty string.
+        ...(secretPath ? { secretPath } : { secretPath: "" }),
+        deletedAt: null
+      })
+      .catch(() => null);
+
+    return policyId ? policy && policy.id !== policyId : Boolean(policy);
+  };
+
   const createSecretApprovalPolicy = async ({
     name,
     actor,
@@ -106,10 +128,17 @@ export const secretApprovalPolicyServiceFactory = ({
     }
 
     const env = await projectEnvDAL.findOne({ slug: environment, projectId });
-    if (!env)
+    if (!env) {
       throw new NotFoundError({
         message: `Environment with slug '${environment}' not found in project with ID ${projectId}`
       });
+    }
+
+    if (await $policyExists({ envId: env.id, secretPath })) {
+      throw new BadRequestError({
+        message: `A policy for secret path '${secretPath}' already exists in environment '${environment}'`
+      });
+    }
 
     let groupBypassers: string[] = [];
     let bypasserUserIds: string[] = [];
@@ -260,6 +289,12 @@ export const secretApprovalPolicyServiceFactory = ({
       });
     }
 
+    if (await $policyExists({ envId: secretApprovalPolicy.envId, secretPath, policyId: secretApprovalPolicy.id })) {
+      throw new BadRequestError({
+        message: `A policy for secret path '${secretPath}' already exists in environment '${secretApprovalPolicy.environment.slug}'`
+      });
+    }
+
     const { permission } = await permissionService.getProjectPermission({
       actor,
       actorId,

frontend/src/pages/secret-manager/SecretApprovalsPage/components/ApprovalPolicyList/components/AccessPolicyModal.tsx

@@ -55,7 +55,7 @@ const formSchema = z
   .object({
     environment: z.object({ slug: z.string(), name: z.string() }),
     name: z.string().optional(),
-    secretPath: z.string().optional(),
+    secretPath: z.string().trim().optional(),
     approvals: z.number().min(1).default(1),
     userApprovers: z
       .object({ type: z.literal(ApproverType.User), id: z.string() })
@@ -93,20 +93,27 @@ const formSchema = z
       .optional()
   })
   .superRefine((data, ctx) => {
-    if (
+    if (data.policyType === PolicyType.ChangePolicy) {
-      data.policyType === PolicyType.ChangePolicy &&
+      if (!(data.groupApprovers.length || data.userApprovers.length)) {
-      !(data.groupApprovers.length || data.userApprovers.length)
+        ctx.addIssue({
-    ) {
+          path: ["userApprovers"],
-      ctx.addIssue({
+          code: z.ZodIssueCode.custom,
-        path: ["userApprovers"],
+          message: "At least one approver should be provided"
-        code: z.ZodIssueCode.custom,
+        });
-        message: "At least one approver should be provided"
+        ctx.addIssue({
-      });
+          path: ["groupApprovers"],
-      ctx.addIssue({
+          code: z.ZodIssueCode.custom,
-        path: ["groupApprovers"],
+          message: "At least one approver should be provided"
-        code: z.ZodIssueCode.custom,
+        });
-        message: "At least one approver should be provided"
+      }
-      });
+    } else if (data.policyType === PolicyType.AccessPolicy) {
+      if (!data.secretPath) {
+        ctx.addIssue({
+          path: ["secretPath"],
+          code: z.ZodIssueCode.custom,
+          message: "Secret path cannot be empty"
+        });
+      }
     }
   });
 
@@ -127,6 +134,7 @@ const Form = ({
     control,
     handleSubmit,
     watch,
+    resetField,
     formState: { isSubmitting }
   } = useForm<TFormSchema>({
     resolver: zodResolver(formSchema),
@@ -177,6 +185,7 @@ const Form = ({
       : undefined,
     defaultValues: !editValues
       ? {
+          secretPath: "/",
           sequenceApprovers: [{ approvals: 1 }]
         }
       : undefined
@@ -405,7 +414,10 @@ const Form = ({
                 <Select
                   isDisabled={isEditMode}
                   value={value}
-                  onValueChange={(val) => onChange(val as PolicyType)}
+                  onValueChange={(val) => {
+                    onChange(val as PolicyType);
+                    resetField("secretPath");
+                  }}
                   className="w-full border border-mineshaft-500"
                 >
                   {Object.values(PolicyType).map((policyType) => {