Merge pull request #3923 from Infisical/daniel/approval-policy-improvements

fix(approval-policies): improve policies handling

backend/src/db/migrations/20250710115149_required-path-on-approval-policies.ts

@@ -0,0 +1,55 @@
+import { Knex } from "knex";
+
+import { TableName } from "../schemas";
+
+export async function up(knex: Knex): Promise<void> {
+  const existingSecretApprovalPolicies = await knex(TableName.SecretApprovalPolicy)
+    .whereNull("secretPath")
+    .orWhere("secretPath", "");
+
+  const existingAccessApprovalPolicies = await knex(TableName.AccessApprovalPolicy)
+    .whereNull("secretPath")
+    .orWhere("secretPath", "");
+
+  // update all the secret approval policies secretPath to be "/**"
+  if (existingSecretApprovalPolicies.length) {
+    await knex(TableName.SecretApprovalPolicy)
+      .whereIn(
+        "id",
+        existingSecretApprovalPolicies.map((el) => el.id)
+      )
+      .update({
+        secretPath: "/**"
+      });
+  }
+
+  // update all the access approval policies secretPath to be "/**"
+  if (existingAccessApprovalPolicies.length) {
+    await knex(TableName.AccessApprovalPolicy)
+      .whereIn(
+        "id",
+        existingAccessApprovalPolicies.map((el) => el.id)
+      )
+      .update({
+        secretPath: "/**"
+      });
+  }
+
+  await knex.schema.alterTable(TableName.SecretApprovalPolicy, (table) => {
+    table.string("secretPath").notNullable().alter();
+  });
+
+  await knex.schema.alterTable(TableName.AccessApprovalPolicy, (table) => {
+    table.string("secretPath").notNullable().alter();
+  });
+}
+
+export async function down(knex: Knex): Promise<void> {
+  await knex.schema.alterTable(TableName.SecretApprovalPolicy, (table) => {
+    table.string("secretPath").nullable().alter();
+  });
+
+  await knex.schema.alterTable(TableName.AccessApprovalPolicy, (table) => {
+    table.string("secretPath").nullable().alter();
+  });
+}

backend/src/db/schemas/access-approval-policies.ts

@@ -11,7 +11,7 @@ export const AccessApprovalPoliciesSchema = z.object({
   id: z.string().uuid(),
   name: z.string(),
   approvals: z.number().default(1),
-  secretPath: z.string().nullable().optional(),
+  secretPath: z.string(),
   envId: z.string().uuid(),
   createdAt: z.date(),
   updatedAt: z.date(),

backend/src/db/schemas/secret-approval-policies.ts

@@ -10,7 +10,7 @@ import { TImmutableDBKeys } from "./models";
 export const SecretApprovalPoliciesSchema = z.object({
   id: z.string().uuid(),
   name: z.string(),
-  secretPath: z.string().nullable().optional(),
+  secretPath: z.string(),
   approvals: z.number().default(1),
   envId: z.string().uuid(),
   createdAt: z.date(),

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

@@ -2,6 +2,7 @@ import { nanoid } from "nanoid";
 import { z } from "zod";
 
 import { ApproverType, BypasserType } from "@app/ee/services/access-approval-policy/access-approval-policy-types";
+import { removeTrailingSlash } from "@app/lib/fn";
 import { EnforcementLevel } from "@app/lib/types";
 import { readLimit, writeLimit } from "@app/server/config/rateLimiter";
 import { verifyAuth } from "@app/server/plugins/auth/verify-auth";
@@ -19,7 +20,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" }).transform(removeTrailingSlash),
         environment: z.string(),
         approvers: z
           .discriminatedUnion("type", [
@@ -174,8 +175,9 @@ export const registerAccessApprovalPolicyRouter = async (server: FastifyZodProvi
         secretPath: z
           .string()
           .trim()
+          .min(1, { message: "Secret path cannot be empty" })
           .optional()
-          .transform((val) => (val === "" ? "/" : val)),
+          .transform((val) => (val ? removeTrailingSlash(val) : val)),
         approvers: z
           .discriminatedUnion("type", [
             z.object({

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

@@ -23,10 +23,8 @@ export const registerSecretApprovalPolicyRouter = async (server: FastifyZodProvi
         environment: z.string(),
         secretPath: z
           .string()
-          .optional()
+          .min(1, { message: "Secret path cannot be empty" })
-          .nullable()
+          .transform((val) => removeTrailingSlash(val)),
-          .default("/")
-          .transform((val) => (val ? removeTrailingSlash(val) : val)),
         approvers: z
           .discriminatedUnion("type", [
             z.object({ type: z.literal(ApproverType.Group), id: z.string() }),
@@ -100,10 +98,10 @@ export const registerSecretApprovalPolicyRouter = async (server: FastifyZodProvi
         approvals: z.number().min(1).default(1),
         secretPath: z
           .string()
+          .trim()
+          .min(1, { message: "Secret path cannot be empty" })
           .optional()
-          .nullable()
+          .transform((val) => (val ? removeTrailingSlash(val) : undefined)),
-          .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-dal.ts

@@ -53,7 +53,7 @@ export interface TAccessApprovalPolicyDALFactory
       envId: string;
       enforcementLevel: string;
       allowedSelfApprovals: boolean;
-      secretPath?: string | null | undefined;
+      secretPath: string;
       deletedAt?: Date | null | undefined;
       environment: {
         id: string;
@@ -93,7 +93,7 @@ export interface TAccessApprovalPolicyDALFactory
         envId: string;
         enforcementLevel: string;
         allowedSelfApprovals: boolean;
-        secretPath?: string | null | undefined;
+        secretPath: string;
         deletedAt?: Date | null | undefined;
         environment: {
           id: string;
@@ -116,7 +116,7 @@ export interface TAccessApprovalPolicyDALFactory
     envId: string;
     enforcementLevel: string;
     allowedSelfApprovals: boolean;
-    secretPath?: string | null | undefined;
+    secretPath: string;
     deletedAt?: Date | null | undefined;
   }>;
   findLastValidPolicy: (
@@ -138,7 +138,7 @@ export interface TAccessApprovalPolicyDALFactory
         envId: string;
         enforcementLevel: string;
         allowedSelfApprovals: boolean;
-        secretPath?: string | null | undefined;
+        secretPath: string;
         deletedAt?: Date | null | undefined;
       }
     | undefined
@@ -190,7 +190,7 @@ export interface TAccessApprovalPolicyServiceFactory {
     envId: string;
     enforcementLevel: string;
     allowedSelfApprovals: boolean;
-    secretPath?: string | null | undefined;
+    secretPath: string;
     deletedAt?: Date | null | undefined;
   }>;
   deleteAccessApprovalPolicy: ({
@@ -214,7 +214,7 @@ export interface TAccessApprovalPolicyServiceFactory {
     envId: string;
     enforcementLevel: string;
     allowedSelfApprovals: boolean;
-    secretPath?: string | null | undefined;
+    secretPath: string;
     deletedAt?: Date | null | undefined;
     environment: {
       id: string;
@@ -252,7 +252,7 @@ export interface TAccessApprovalPolicyServiceFactory {
     envId: string;
     enforcementLevel: string;
     allowedSelfApprovals: boolean;
-    secretPath?: string | null | undefined;
+    secretPath: string;
     deletedAt?: Date | null | undefined;
   }>;
   getAccessApprovalPolicyByProjectSlug: ({
@@ -286,7 +286,7 @@ export interface TAccessApprovalPolicyServiceFactory {
       envId: string;
       enforcementLevel: string;
       allowedSelfApprovals: boolean;
-      secretPath?: string | null | undefined;
+      secretPath: string;
       deletedAt?: Date | null | undefined;
       environment: {
         id: string;
@@ -337,7 +337,7 @@ export interface TAccessApprovalPolicyServiceFactory {
     envId: string;
     enforcementLevel: string;
     allowedSelfApprovals: boolean;
-    secretPath?: string | null | undefined;
+    secretPath: string;
     deletedAt?: Date | null | undefined;
     environment: {
       id: string;

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({
@@ -279,7 +305,11 @@ export const accessApprovalPolicyServiceFactory = ({
     ) as { username: string; sequence?: number }[];
 
     const accessApprovalPolicy = await accessApprovalPolicyDAL.findById(policyId);
-    if (!accessApprovalPolicy) throw new BadRequestError({ message: "Approval policy not found" });
+    if (!accessApprovalPolicy) {
+      throw new NotFoundError({
+        message: `Access approval policy with ID '${policyId}' not found`
+      });
+    }
 
     const currentApprovals = approvals || accessApprovalPolicy.approvals;
     if (
@@ -290,9 +320,18 @@ export const accessApprovalPolicyServiceFactory = ({
       throw new BadRequestError({ message: "Approvals cannot be greater than approvers" });
     }
 
-    if (!accessApprovalPolicy) {
+    if (
-      throw new NotFoundError({ message: `Secret approval policy with ID '${policyId}' not found` });
+      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}'`
+      });
     }
+
     const { permission } = await permissionService.getProjectPermission({
       actor,
       actorId,

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

@@ -122,7 +122,7 @@ export interface TAccessApprovalPolicyServiceFactory {
     envId: string;
     enforcementLevel: string;
     allowedSelfApprovals: boolean;
-    secretPath?: string | null | undefined;
+    secretPath: string;
     deletedAt?: Date | null | undefined;
   }>;
   deleteAccessApprovalPolicy: ({
@@ -146,7 +146,7 @@ export interface TAccessApprovalPolicyServiceFactory {
     envId: string;
     enforcementLevel: string;
     allowedSelfApprovals: boolean;
-    secretPath?: string | null | undefined;
+    secretPath: string;
     deletedAt?: Date | null | undefined;
     environment: {
       id: string;
@@ -218,7 +218,7 @@ export interface TAccessApprovalPolicyServiceFactory {
       envId: string;
       enforcementLevel: string;
       allowedSelfApprovals: boolean;
-      secretPath?: string | null | undefined;
+      secretPath: string;
       deletedAt?: Date | null | undefined;
       environment: {
         id: string;
@@ -269,7 +269,7 @@ export interface TAccessApprovalPolicyServiceFactory {
     envId: string;
     enforcementLevel: string;
     allowedSelfApprovals: boolean;
-    secretPath?: string | null | undefined;
+    secretPath: string;
     deletedAt?: Date | null | undefined;
     environment: {
       id: string;

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

@@ -55,6 +55,26 @@ export const secretApprovalPolicyServiceFactory = ({
   licenseService,
   secretApprovalRequestDAL
 }: TSecretApprovalPolicyServiceFactoryDep) => {
+  const $policyExists = async ({
+    envId,
+    secretPath,
+    policyId
+  }: {
+    envId: string;
+    secretPath: string;
+    policyId?: string;
+  }) => {
+    const policy = await secretApprovalPolicyDAL
+      .findOne({
+        envId,
+        secretPath,
+        deletedAt: null
+      })
+      .catch(() => null);
+
+    return policyId ? policy && policy.id !== policyId : Boolean(policy);
+  };
+
   const createSecretApprovalPolicy = async ({
     name,
     actor,
@@ -106,10 +126,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 +287,18 @@ export const secretApprovalPolicyServiceFactory = ({
       });
     }
 
+    if (
+      await $policyExists({
+        envId: secretApprovalPolicy.envId,
+        secretPath: secretPath || secretApprovalPolicy.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,

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

@@ -4,7 +4,7 @@ import { ApproverType, BypasserType } from "../access-approval-policy/access-app
 
 export type TCreateSapDTO = {
   approvals: number;
-  secretPath?: string | null;
+  secretPath: string;
   environment: string;
   approvers: ({ type: ApproverType.Group; id: string } | { type: ApproverType.User; id?: string; username?: string })[];
   bypassers?: (
@@ -20,7 +20,7 @@ export type TCreateSapDTO = {
 export type TUpdateSapDTO = {
   secretPolicyId: string;
   approvals?: number;
-  secretPath?: string | null;
+  secretPath?: string;
   approvers: ({ type: ApproverType.Group; id: string } | { type: ApproverType.User; id?: string; username?: string })[];
   bypassers?: (
     | { type: BypasserType.Group; id: string }

frontend/src/hooks/api/accessApproval/types.ts

@@ -170,7 +170,7 @@ export type TCreateAccessPolicyDTO = {
   approvers?: Approver[];
   bypassers?: Bypasser[];
   approvals?: number;
-  secretPath?: string;
+  secretPath: string;
   enforcementLevel?: EnforcementLevel;
   allowedSelfApprovals: boolean;
   approvalsRequired?: { numberOfApprovals: number; stepNumber: number }[];

frontend/src/hooks/api/secretApproval/types.ts

@@ -49,7 +49,7 @@ export type TCreateSecretPolicyDTO = {
   workspaceId: string;
   name?: string;
   environment: string;
-  secretPath?: string | null;
+  secretPath: string;
   approvers?: Approver[];
   bypassers?: Bypasser[];
   approvals?: number;
@@ -62,7 +62,7 @@ export type TUpdateSecretPolicyDTO = {
   name?: string;
   approvers?: Approver[];
   bypassers?: Bypasser[];
-  secretPath?: string | null;
+  secretPath?: string;
   approvals?: number;
   allowedSelfApprovals?: boolean;
   enforcementLevel?: EnforcementLevel;

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().min(1),
     approvals: z.number().min(1).default(1),
     userApprovers: z
       .object({ type: z.literal(ApproverType.User), id: z.string() })
@@ -93,20 +93,19 @@ 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"
+      }
-      });
     }
   });
 
@@ -127,6 +126,7 @@ const Form = ({
     control,
     handleSubmit,
     watch,
+    resetField,
     formState: { isSubmitting }
   } = useForm<TFormSchema>({
     resolver: zodResolver(formSchema),
@@ -177,6 +177,7 @@ const Form = ({
       : undefined,
     defaultValues: !editValues
       ? {
+          secretPath: "/",
           sequenceApprovers: [{ approvals: 1 }]
         }
       : undefined
@@ -405,7 +406,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) => {
@@ -465,6 +469,7 @@ const Form = ({
               <FormControl
                 tooltipText="Secret paths support glob patterns. For example, '/**' will match all paths."
                 label="Secret Path"
+                isRequired
                 isError={Boolean(error)}
                 errorText={error?.message}
                 className="flex-1"