diff --git a/backend/src/db/migrations/20250710115149_required-path-on-approval-policies.ts b/backend/src/db/migrations/20250710115149_required-path-on-approval-policies.ts new file mode 100644 index 000000000..b5f5abef7 --- /dev/null +++ b/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 { + 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 { + await knex.schema.alterTable(TableName.SecretApprovalPolicy, (table) => { + table.string("secretPath").nullable().alter(); + }); + + await knex.schema.alterTable(TableName.AccessApprovalPolicy, (table) => { + table.string("secretPath").nullable().alter(); + }); +} diff --git a/backend/src/db/schemas/access-approval-policies.ts b/backend/src/db/schemas/access-approval-policies.ts index 19a98675f..ea57c54d2 100644 --- a/backend/src/db/schemas/access-approval-policies.ts +++ b/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(), diff --git a/backend/src/db/schemas/secret-approval-policies.ts b/backend/src/db/schemas/secret-approval-policies.ts index 8b9174456..0273e617c 100644 --- a/backend/src/db/schemas/secret-approval-policies.ts +++ b/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(), 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..177f5e1fd 100644 --- a/backend/src/ee/routes/v1/access-approval-policy-router.ts +++ b/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({ 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..46b2544b2 100644 --- a/backend/src/ee/routes/v1/secret-approval-policy-router.ts +++ b/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() - .nullable() - .default("/") - .transform((val) => (val ? removeTrailingSlash(val) : val)), + .min(1, { message: "Secret path cannot be empty" }) + .transform((val) => removeTrailingSlash(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) : val)) - .transform((val) => (val === "" ? "/" : val)), + .transform((val) => (val ? removeTrailingSlash(val) : undefined)), enforcementLevel: z.nativeEnum(EnforcementLevel).optional(), allowedSelfApprovals: z.boolean().default(true) }), diff --git a/backend/src/ee/services/access-approval-policy/access-approval-policy-dal.ts b/backend/src/ee/services/access-approval-policy/access-approval-policy-dal.ts index 9fa48ca15..995534f8f 100644 --- a/backend/src/ee/services/access-approval-policy/access-approval-policy-dal.ts +++ b/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; 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..fa487d0b7 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({ @@ -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) { - throw new NotFoundError({ message: `Secret approval policy with ID '${policyId}' not found` }); + 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}'` + }); } + const { permission } = await permissionService.getProjectPermission({ actor, actorId, diff --git a/backend/src/ee/services/access-approval-policy/access-approval-policy-types.ts b/backend/src/ee/services/access-approval-policy/access-approval-policy-types.ts index 6806c7123..f3f195914 100644 --- a/backend/src/ee/services/access-approval-policy/access-approval-policy-types.ts +++ b/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; 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..80127c071 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 @@ -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, diff --git a/backend/src/ee/services/secret-approval-policy/secret-approval-policy-types.ts b/backend/src/ee/services/secret-approval-policy/secret-approval-policy-types.ts index ed074336c..ba5334e5c 100644 --- a/backend/src/ee/services/secret-approval-policy/secret-approval-policy-types.ts +++ b/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 } diff --git a/frontend/src/hooks/api/accessApproval/types.ts b/frontend/src/hooks/api/accessApproval/types.ts index cde04ee61..70e9b883e 100644 --- a/frontend/src/hooks/api/accessApproval/types.ts +++ b/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 }[]; diff --git a/frontend/src/hooks/api/secretApproval/types.ts b/frontend/src/hooks/api/secretApproval/types.ts index 15afcf119..eeb734115 100644 --- a/frontend/src/hooks/api/secretApproval/types.ts +++ b/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; 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..40d116c5b 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().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 ( - 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" + }); + } } }); @@ -127,6 +126,7 @@ const Form = ({ control, handleSubmit, watch, + resetField, formState: { isSubmitting } } = useForm({ resolver: zodResolver(formSchema), @@ -177,6 +177,7 @@ const Form = ({ : undefined, defaultValues: !editValues ? { + secretPath: "/", sequenceApprovers: [{ approvals: 1 }] } : undefined @@ -405,7 +406,10 @@ const Form = ({