From 0569c7e692812792a99c095bf59fbfe1082a175b Mon Sep 17 00:00:00 2001 From: Daniel Hougaard Date: Fri, 4 Jul 2025 03:14:43 +0400 Subject: [PATCH 1/5] fix(approval-policies): improve policies handling --- .../v1/access-approval-policy-router.ts | 8 +--- .../v1/secret-approval-policy-router.ts | 3 +- .../access-approval-policy-service.ts | 44 +++++++++++++++++++ .../secret-approval-policy-service.ts | 37 +++++++++++++++- .../components/AccessPolicyModal.tsx | 44 ++++++++++++------- 5 files changed, 111 insertions(+), 25 deletions(-) diff --git a/backend/src/ee/routes/v1/access-approval-policy-router.ts b/backend/src/ee/routes/v1/access-approval-policy-router.ts index 74545579c..bca745ee5 100644 --- a/backend/src/ee/routes/v1/access-approval-policy-router.ts +++ b/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 - .string() - .trim() - .optional() - .transform((val) => (val === "" ? "/" : val)), + secretPath: z.string().trim().optional(), approvers: z .discriminatedUnion("type", [ z.object({ diff --git a/backend/src/ee/routes/v1/secret-approval-policy-router.ts b/backend/src/ee/routes/v1/secret-approval-policy-router.ts index ebe1345b3..2fe13e0d2 100644 --- a/backend/src/ee/routes/v1/secret-approval-policy-router.ts +++ b/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 === "" ? "/" : val)), + .transform((val) => (val ? removeTrailingSlash(val) : val)), enforcementLevel: z.nativeEnum(EnforcementLevel).optional(), allowedSelfApprovals: z.boolean().default(true) }), diff --git a/backend/src/ee/services/access-approval-policy/access-approval-policy-service.ts b/backend/src/ee/services/access-approval-policy/access-approval-policy-service.ts index 5976f5fff..fa50ab558 100644 --- a/backend/src/ee/services/access-approval-policy/access-approval-policy-service.ts +++ b/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` }); } diff --git a/backend/src/ee/services/secret-approval-policy/secret-approval-policy-service.ts b/backend/src/ee/services/secret-approval-policy/secret-approval-policy-service.ts index cb497aa7d..d1979440c 100644 --- a/backend/src/ee/services/secret-approval-policy/secret-approval-policy-service.ts +++ b/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, diff --git a/frontend/src/pages/secret-manager/SecretApprovalsPage/components/ApprovalPolicyList/components/AccessPolicyModal.tsx b/frontend/src/pages/secret-manager/SecretApprovalsPage/components/ApprovalPolicyList/components/AccessPolicyModal.tsx index 3c378cded..e5b1825ef 100644 --- a/frontend/src/pages/secret-manager/SecretApprovalsPage/components/ApprovalPolicyList/components/AccessPolicyModal.tsx +++ b/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 ( - data.policyType === PolicyType.ChangePolicy && - !(data.groupApprovers.length || data.userApprovers.length) - ) { - ctx.addIssue({ - path: ["userApprovers"], - code: z.ZodIssueCode.custom, - message: "At least one approver should be provided" - }); - ctx.addIssue({ - path: ["groupApprovers"], - code: z.ZodIssueCode.custom, - message: "At least one approver should be provided" - }); + if (data.policyType === PolicyType.ChangePolicy) { + if (!(data.groupApprovers.length || data.userApprovers.length)) { + ctx.addIssue({ + path: ["userApprovers"], + code: z.ZodIssueCode.custom, + message: "At least one approver should be provided" + }); + ctx.addIssue({ + path: ["groupApprovers"], + 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({ resolver: zodResolver(formSchema), @@ -177,6 +185,7 @@ const Form = ({ : undefined, defaultValues: !editValues ? { + secretPath: "/", sequenceApprovers: [{ approvals: 1 }] } : undefined @@ -405,7 +414,10 @@ const Form = ({