fix(Approval Workflows): Workflows keep approval history after deletion (#2834)

* improvement: Approval Workflows can be deleted while maintaining history
Co-authored-by: Daniel Hougaard <daniel@infisical.com>
This commit is contained in:
McPizza
2024-12-09 20:03:45 +01:00
committed by GitHub
parent c648235390
commit 97f85fa8d9
16 changed files with 203 additions and 36 deletions

View File

@@ -0,0 +1,59 @@
import { Knex } from "knex";
import { TableName } from "../schemas";
export async function up(knex: Knex): Promise<void> {
const hasAccessApprovalPolicyDeletedAtColumn = await knex.schema.hasColumn(
TableName.AccessApprovalPolicy,
"deletedAt"
);
const hasSecretApprovalPolicyDeletedAtColumn = await knex.schema.hasColumn(
TableName.SecretApprovalPolicy,
"deletedAt"
);
if (!hasAccessApprovalPolicyDeletedAtColumn) {
await knex.schema.alterTable(TableName.AccessApprovalPolicy, (t) => {
t.timestamp("deletedAt");
});
}
if (!hasSecretApprovalPolicyDeletedAtColumn) {
await knex.schema.alterTable(TableName.SecretApprovalPolicy, (t) => {
t.timestamp("deletedAt");
});
}
await knex.schema.alterTable(TableName.AccessApprovalRequest, (t) => {
t.dropForeign(["privilegeId"]);
// Add the new foreign key constraint with ON DELETE SET NULL
t.foreign("privilegeId").references("id").inTable(TableName.ProjectUserAdditionalPrivilege).onDelete("SET NULL");
});
}
export async function down(knex: Knex): Promise<void> {
const hasAccessApprovalPolicyDeletedAtColumn = await knex.schema.hasColumn(
TableName.AccessApprovalPolicy,
"deletedAt"
);
const hasSecretApprovalPolicyDeletedAtColumn = await knex.schema.hasColumn(
TableName.SecretApprovalPolicy,
"deletedAt"
);
if (hasAccessApprovalPolicyDeletedAtColumn) {
await knex.schema.alterTable(TableName.AccessApprovalPolicy, (t) => {
t.dropColumn("deletedAt");
});
}
if (hasSecretApprovalPolicyDeletedAtColumn) {
await knex.schema.alterTable(TableName.SecretApprovalPolicy, (t) => {
t.dropColumn("deletedAt");
});
}
await knex.schema.alterTable(TableName.AccessApprovalRequest, (t) => {
t.dropForeign(["privilegeId"]);
t.foreign("privilegeId").references("id").inTable(TableName.ProjectUserAdditionalPrivilege).onDelete("CASCADE");
});
}

View File

@@ -15,7 +15,8 @@ export const AccessApprovalPoliciesSchema = z.object({
envId: z.string().uuid(),
createdAt: z.date(),
updatedAt: z.date(),
enforcementLevel: z.string().default("hard")
enforcementLevel: z.string().default("hard"),
deletedAt: z.date().nullable().optional()
});
export type TAccessApprovalPolicies = z.infer<typeof AccessApprovalPoliciesSchema>;

View File

@@ -15,7 +15,8 @@ export const SecretApprovalPoliciesSchema = z.object({
envId: z.string().uuid(),
createdAt: z.date(),
updatedAt: z.date(),
enforcementLevel: z.string().default("hard")
enforcementLevel: z.string().default("hard"),
deletedAt: z.date().nullable().optional()
});
export type TSecretApprovalPolicies = z.infer<typeof SecretApprovalPoliciesSchema>;

View File

@@ -109,7 +109,8 @@ export const registerAccessApprovalRequestRouter = async (server: FastifyZodProv
approvers: z.string().array(),
secretPath: z.string().nullish(),
envId: z.string(),
enforcementLevel: z.string()
enforcementLevel: z.string(),
deletedAt: z.date().nullish()
}),
reviewers: z
.object({

View File

@@ -52,7 +52,8 @@ export const registerSecretApprovalRequestRouter = async (server: FastifyZodProv
})
.array(),
secretPath: z.string().optional().nullable(),
enforcementLevel: z.string()
enforcementLevel: z.string(),
deletedAt: z.date().nullish()
}),
committerUser: approvalRequestUser,
commits: z.object({ op: z.string(), secretId: z.string().nullable().optional() }).array(),
@@ -260,7 +261,8 @@ export const registerSecretApprovalRequestRouter = async (server: FastifyZodProv
approvals: z.number(),
approvers: approvalRequestUser.array(),
secretPath: z.string().optional().nullable(),
enforcementLevel: z.string()
enforcementLevel: z.string(),
deletedAt: z.date().nullish()
}),
environment: z.string(),
statusChangedByUser: approvalRequestUser.optional(),

View File

@@ -139,5 +139,10 @@ export const accessApprovalPolicyDALFactory = (db: TDbClient) => {
}
};
return { ...accessApprovalPolicyOrm, find, findById };
const softDeleteById = async (policyId: string, tx?: Knex) => {
const softDeletedPolicy = await accessApprovalPolicyOrm.updateById(policyId, { deletedAt: new Date() }, tx);
return softDeletedPolicy;
};
return { ...accessApprovalPolicyOrm, find, findById, softDeleteById };
};

View File

@@ -8,7 +8,11 @@ import { TProjectEnvDALFactory } from "@app/services/project-env/project-env-dal
import { TProjectMembershipDALFactory } from "@app/services/project-membership/project-membership-dal";
import { TUserDALFactory } from "@app/services/user/user-dal";
import { TAccessApprovalRequestDALFactory } from "../access-approval-request/access-approval-request-dal";
import { TAccessApprovalRequestReviewerDALFactory } from "../access-approval-request/access-approval-request-reviewer-dal";
import { ApprovalStatus } from "../access-approval-request/access-approval-request-types";
import { TGroupDALFactory } from "../group/group-dal";
import { TProjectUserAdditionalPrivilegeDALFactory } from "../project-user-additional-privilege/project-user-additional-privilege-dal";
import { TAccessApprovalPolicyApproverDALFactory } from "./access-approval-policy-approver-dal";
import { TAccessApprovalPolicyDALFactory } from "./access-approval-policy-dal";
import {
@@ -21,7 +25,7 @@ import {
TUpdateAccessApprovalPolicy
} from "./access-approval-policy-types";
type TSecretApprovalPolicyServiceFactoryDep = {
type TAccessApprovalPolicyServiceFactoryDep = {
projectDAL: TProjectDALFactory;
permissionService: Pick<TPermissionServiceFactory, "getProjectPermission">;
accessApprovalPolicyDAL: TAccessApprovalPolicyDALFactory;
@@ -30,6 +34,9 @@ type TSecretApprovalPolicyServiceFactoryDep = {
projectMembershipDAL: Pick<TProjectMembershipDALFactory, "find">;
groupDAL: TGroupDALFactory;
userDAL: Pick<TUserDALFactory, "find">;
accessApprovalRequestDAL: Pick<TAccessApprovalRequestDALFactory, "update" | "find">;
additionalPrivilegeDAL: Pick<TProjectUserAdditionalPrivilegeDALFactory, "delete">;
accessApprovalRequestReviewerDAL: Pick<TAccessApprovalRequestReviewerDALFactory, "update">;
};
export type TAccessApprovalPolicyServiceFactory = ReturnType<typeof accessApprovalPolicyServiceFactory>;
@@ -41,8 +48,11 @@ export const accessApprovalPolicyServiceFactory = ({
permissionService,
projectEnvDAL,
projectDAL,
userDAL
}: TSecretApprovalPolicyServiceFactoryDep) => {
userDAL,
accessApprovalRequestDAL,
additionalPrivilegeDAL,
accessApprovalRequestReviewerDAL
}: TAccessApprovalPolicyServiceFactoryDep) => {
const createAccessApprovalPolicy = async ({
name,
actor,
@@ -189,7 +199,7 @@ export const accessApprovalPolicyServiceFactory = ({
);
// ForbiddenError.from(permission).throwUnlessCan(ProjectPermissionActions.Read, ProjectPermissionSub.SecretApproval);
const accessApprovalPolicies = await accessApprovalPolicyDAL.find({ projectId: project.id });
const accessApprovalPolicies = await accessApprovalPolicyDAL.find({ projectId: project.id, deletedAt: null });
return accessApprovalPolicies;
};
@@ -326,7 +336,29 @@ export const accessApprovalPolicyServiceFactory = ({
ProjectPermissionSub.SecretApproval
);
await accessApprovalPolicyDAL.deleteById(policyId);
await accessApprovalPolicyDAL.transaction(async (tx) => {
await accessApprovalPolicyDAL.softDeleteById(policyId, tx);
const allAccessApprovalRequests = await accessApprovalRequestDAL.find({ policyId });
if (allAccessApprovalRequests.length) {
const accessApprovalRequestsIds = allAccessApprovalRequests.map((request) => request.id);
const privilegeIdsArray = allAccessApprovalRequests
.map((request) => request.privilegeId)
.filter((id): id is string => id != null);
if (privilegeIdsArray.length) {
await additionalPrivilegeDAL.delete({ $in: { id: privilegeIdsArray } }, tx);
}
await accessApprovalRequestReviewerDAL.update(
{ $in: { id: accessApprovalRequestsIds }, status: ApprovalStatus.PENDING },
{ status: ApprovalStatus.REJECTED },
tx
);
}
});
return policy;
};
@@ -356,7 +388,11 @@ export const accessApprovalPolicyServiceFactory = ({
const environment = await projectEnvDAL.findOne({ projectId: project.id, slug: envSlug });
if (!environment) throw new NotFoundError({ message: `Environment with slug '${envSlug}' not found` });
const policies = await accessApprovalPolicyDAL.find({ envId: environment.id, projectId: project.id });
const policies = await accessApprovalPolicyDAL.find({
envId: environment.id,
projectId: project.id,
deletedAt: null
});
if (!policies) throw new NotFoundError({ message: `No policies found in environment with slug '${envSlug}'` });
return { count: policies.length };

View File

@@ -61,7 +61,8 @@ export const accessApprovalRequestDALFactory = (db: TDbClient) => {
db.ref("approvals").withSchema(TableName.AccessApprovalPolicy).as("policyApprovals"),
db.ref("secretPath").withSchema(TableName.AccessApprovalPolicy).as("policySecretPath"),
db.ref("enforcementLevel").withSchema(TableName.AccessApprovalPolicy).as("policyEnforcementLevel"),
db.ref("envId").withSchema(TableName.AccessApprovalPolicy).as("policyEnvId")
db.ref("envId").withSchema(TableName.AccessApprovalPolicy).as("policyEnvId"),
db.ref("deletedAt").withSchema(TableName.AccessApprovalPolicy).as("policyDeletedAt")
)
.select(db.ref("approverUserId").withSchema(TableName.AccessApprovalPolicyApprover))
@@ -118,7 +119,8 @@ export const accessApprovalRequestDALFactory = (db: TDbClient) => {
approvals: doc.policyApprovals,
secretPath: doc.policySecretPath,
enforcementLevel: doc.policyEnforcementLevel,
envId: doc.policyEnvId
envId: doc.policyEnvId,
deletedAt: doc.policyDeletedAt
},
requestedByUser: {
userId: doc.requestedByUserId,
@@ -141,7 +143,7 @@ export const accessApprovalRequestDALFactory = (db: TDbClient) => {
}
: null,
isApproved: !!doc.privilegeId
isApproved: !!doc.policyDeletedAt || !!doc.privilegeId
}),
childrenMapper: [
{
@@ -252,7 +254,8 @@ export const accessApprovalRequestDALFactory = (db: TDbClient) => {
tx.ref("slug").withSchema(TableName.Environment).as("environment"),
tx.ref("secretPath").withSchema(TableName.AccessApprovalPolicy).as("policySecretPath"),
tx.ref("enforcementLevel").withSchema(TableName.AccessApprovalPolicy).as("policyEnforcementLevel"),
tx.ref("approvals").withSchema(TableName.AccessApprovalPolicy).as("policyApprovals")
tx.ref("approvals").withSchema(TableName.AccessApprovalPolicy).as("policyApprovals"),
tx.ref("deletedAt").withSchema(TableName.AccessApprovalPolicy).as("policyDeletedAt")
);
const findById = async (id: string, tx?: Knex) => {
@@ -271,7 +274,8 @@ export const accessApprovalRequestDALFactory = (db: TDbClient) => {
name: el.policyName,
approvals: el.policyApprovals,
secretPath: el.policySecretPath,
enforcementLevel: el.policyEnforcementLevel
enforcementLevel: el.policyEnforcementLevel,
deletedAt: el.policyDeletedAt
},
requestedByUser: {
userId: el.requestedByUserId,
@@ -363,6 +367,7 @@ export const accessApprovalRequestDALFactory = (db: TDbClient) => {
)
.where(`${TableName.Environment}.projectId`, projectId)
.where(`${TableName.AccessApprovalPolicy}.deletedAt`, null)
.select(selectAllTableCols(TableName.AccessApprovalRequest))
.select(db.ref("status").withSchema(TableName.AccessApprovalRequestReviewer).as("reviewerStatus"))
.select(db.ref("reviewerUserId").withSchema(TableName.AccessApprovalRequestReviewer).as("reviewerUserId"));

View File

@@ -130,6 +130,9 @@ export const accessApprovalRequestServiceFactory = ({
message: `No policy in environment with slug '${environment.slug}' and with secret path '${secretPath}' was found.`
});
}
if (policy.deletedAt) {
throw new BadRequestError({ message: "The policy linked to this request has been deleted" });
}
const approverIds: string[] = [];
const approverGroupIds: string[] = [];
@@ -309,6 +312,12 @@ export const accessApprovalRequestServiceFactory = ({
}
const { policy } = accessApprovalRequest;
if (policy.deletedAt) {
throw new BadRequestError({
message: "The policy associated with this access request has been deleted."
});
}
const { membership, hasRole } = await permissionService.getProjectPermission(
actor,
actorId,

View File

@@ -177,5 +177,10 @@ export const secretApprovalPolicyDALFactory = (db: TDbClient) => {
}
};
return { ...secretApprovalPolicyOrm, findById, find };
const softDeleteById = async (policyId: string, tx?: Knex) => {
const softDeletedPolicy = await secretApprovalPolicyOrm.updateById(policyId, { deletedAt: new Date() }, tx);
return softDeletedPolicy;
};
return { ...secretApprovalPolicyOrm, findById, find, softDeleteById };
};

View File

@@ -11,6 +11,8 @@ import { TUserDALFactory } from "@app/services/user/user-dal";
import { ApproverType } from "../access-approval-policy/access-approval-policy-types";
import { TLicenseServiceFactory } from "../license/license-service";
import { TSecretApprovalRequestDALFactory } from "../secret-approval-request/secret-approval-request-dal";
import { RequestState } from "../secret-approval-request/secret-approval-request-types";
import { TSecretApprovalPolicyApproverDALFactory } from "./secret-approval-policy-approver-dal";
import { TSecretApprovalPolicyDALFactory } from "./secret-approval-policy-dal";
import {
@@ -34,6 +36,7 @@ type TSecretApprovalPolicyServiceFactoryDep = {
userDAL: Pick<TUserDALFactory, "find">;
secretApprovalPolicyApproverDAL: TSecretApprovalPolicyApproverDALFactory;
licenseService: Pick<TLicenseServiceFactory, "getPlan">;
secretApprovalRequestDAL: Pick<TSecretApprovalRequestDALFactory, "update">;
};
export type TSecretApprovalPolicyServiceFactory = ReturnType<typeof secretApprovalPolicyServiceFactory>;
@@ -44,7 +47,8 @@ export const secretApprovalPolicyServiceFactory = ({
secretApprovalPolicyApproverDAL,
projectEnvDAL,
userDAL,
licenseService
licenseService,
secretApprovalRequestDAL
}: TSecretApprovalPolicyServiceFactoryDep) => {
const createSecretApprovalPolicy = async ({
name,
@@ -301,8 +305,16 @@ export const secretApprovalPolicyServiceFactory = ({
});
}
await secretApprovalPolicyDAL.deleteById(secretPolicyId);
return sapPolicy;
const deletedPolicy = await secretApprovalPolicyDAL.transaction(async (tx) => {
await secretApprovalRequestDAL.update(
{ policyId: secretPolicyId, status: RequestState.Open },
{ status: RequestState.Closed },
tx
);
const updatedPolicy = await secretApprovalPolicyDAL.softDeleteById(secretPolicyId, tx);
return updatedPolicy;
});
return { ...deletedPolicy, projectId: sapPolicy.projectId, environment: sapPolicy.environment };
};
const getSecretApprovalPolicyByProjectId = async ({
@@ -321,7 +333,7 @@ export const secretApprovalPolicyServiceFactory = ({
);
ForbiddenError.from(permission).throwUnlessCan(ProjectPermissionActions.Read, ProjectPermissionSub.SecretApproval);
const sapPolicies = await secretApprovalPolicyDAL.find({ projectId });
const sapPolicies = await secretApprovalPolicyDAL.find({ projectId, deletedAt: null });
return sapPolicies;
};
@@ -334,7 +346,7 @@ export const secretApprovalPolicyServiceFactory = ({
});
}
const policies = await secretApprovalPolicyDAL.find({ envId: env.id });
const policies = await secretApprovalPolicyDAL.find({ envId: env.id, deletedAt: null });
if (!policies.length) return;
// this will filter policies either without scoped to secret path or the one that matches with secret path
const policiesFilteredByPath = policies.filter(

View File

@@ -111,7 +111,8 @@ export const secretApprovalRequestDALFactory = (db: TDbClient) => {
tx.ref("secretPath").withSchema(TableName.SecretApprovalPolicy).as("policySecretPath"),
tx.ref("envId").withSchema(TableName.SecretApprovalPolicy).as("policyEnvId"),
tx.ref("enforcementLevel").withSchema(TableName.SecretApprovalPolicy).as("policyEnforcementLevel"),
tx.ref("approvals").withSchema(TableName.SecretApprovalPolicy).as("policyApprovals")
tx.ref("approvals").withSchema(TableName.SecretApprovalPolicy).as("policyApprovals"),
tx.ref("deletedAt").withSchema(TableName.SecretApprovalPolicy).as("policyDeletedAt")
);
const findById = async (id: string, tx?: Knex) => {
@@ -147,7 +148,8 @@ export const secretApprovalRequestDALFactory = (db: TDbClient) => {
approvals: el.policyApprovals,
secretPath: el.policySecretPath,
enforcementLevel: el.policyEnforcementLevel,
envId: el.policyEnvId
envId: el.policyEnvId,
deletedAt: el.policyDeletedAt
}
}),
childrenMapper: [
@@ -222,6 +224,11 @@ export const secretApprovalRequestDALFactory = (db: TDbClient) => {
`${TableName.SecretApprovalRequest}.policyId`,
`${TableName.SecretApprovalPolicyApprover}.policyId`
)
.join(
TableName.SecretApprovalPolicy,
`${TableName.SecretApprovalRequest}.policyId`,
`${TableName.SecretApprovalPolicy}.id`
)
.where({ projectId })
.andWhere(
(bd) =>
@@ -229,6 +236,7 @@ export const secretApprovalRequestDALFactory = (db: TDbClient) => {
.where(`${TableName.SecretApprovalPolicyApprover}.approverUserId`, userId)
.orWhere(`${TableName.SecretApprovalRequest}.committerUserId`, userId)
)
.andWhere((bd) => void bd.where(`${TableName.SecretApprovalPolicy}.deletedAt`, null))
.select("status", `${TableName.SecretApprovalRequest}.id`)
.groupBy(`${TableName.SecretApprovalRequest}.id`, "status")
.count("status")

View File

@@ -232,10 +232,10 @@ export const secretApprovalRequestServiceFactory = ({
type: KmsDataKey.SecretManager,
projectId
});
const encrypedSecrets = await secretApprovalRequestSecretDAL.findByRequestIdBridgeSecretV2(
const encryptedSecrets = await secretApprovalRequestSecretDAL.findByRequestIdBridgeSecretV2(
secretApprovalRequest.id
);
secrets = encrypedSecrets.map((el) => ({
secrets = encryptedSecrets.map((el) => ({
...el,
secretKey: el.key,
id: el.id,
@@ -274,8 +274,8 @@ export const secretApprovalRequestServiceFactory = ({
}));
} else {
if (!botKey) throw new NotFoundError({ message: `Project bot key not found`, name: "BotKeyNotFound" }); // CLI depends on this error message. TODO(daniel): Make API check for name BotKeyNotFound instead of message
const encrypedSecrets = await secretApprovalRequestSecretDAL.findByRequestId(secretApprovalRequest.id);
secrets = encrypedSecrets.map((el) => ({
const encryptedSecrets = await secretApprovalRequestSecretDAL.findByRequestId(secretApprovalRequest.id);
secrets = encryptedSecrets.map((el) => ({
...el,
...decryptSecretWithBot(el, botKey),
secret: el.secret
@@ -323,6 +323,12 @@ export const secretApprovalRequestServiceFactory = ({
}
const { policy } = secretApprovalRequest;
if (policy.deletedAt) {
throw new BadRequestError({
message: "The policy associated with this secret approval request has been deleted."
});
}
const { hasRole } = await permissionService.getProjectPermission(
ActorType.USER,
actorId,
@@ -383,6 +389,12 @@ export const secretApprovalRequestServiceFactory = ({
}
const { policy } = secretApprovalRequest;
if (policy.deletedAt) {
throw new BadRequestError({
message: "The policy associated with this secret approval request has been deleted."
});
}
const { hasRole } = await permissionService.getProjectPermission(
ActorType.USER,
actorId,
@@ -433,6 +445,12 @@ export const secretApprovalRequestServiceFactory = ({
}
const { policy, folderId, projectId } = secretApprovalRequest;
if (policy.deletedAt) {
throw new BadRequestError({
message: "The policy associated with this secret approval request has been deleted."
});
}
const { hasRole } = await permissionService.getProjectPermission(
ActorType.USER,
actorId,

View File

@@ -414,7 +414,8 @@ export const registerRoutes = async (
permissionService,
secretApprovalPolicyDAL,
licenseService,
userDAL
userDAL,
secretApprovalRequestDAL
});
const tokenService = tokenServiceFactory({ tokenDAL: authTokenDAL, userDAL, orgMembershipDAL });
@@ -994,7 +995,10 @@ export const registerRoutes = async (
projectEnvDAL,
projectMembershipDAL,
projectDAL,
userDAL
userDAL,
accessApprovalRequestDAL,
additionalPrivilegeDAL: projectUserAdditionalPrivilegeDAL,
accessApprovalRequestReviewerDAL
});
const accessApprovalRequestService = accessApprovalRequestServiceFactory({

View File

@@ -18,15 +18,15 @@ export type TAccessApprovalPolicy = {
approvers?: Approver[];
};
export enum ApproverType{
export enum ApproverType {
User = "user",
Group = "group"
}
export type Approver ={
export type Approver = {
id: string;
type: ApproverType;
}
};
export type TAccessApprovalRequest = {
id: string;
@@ -70,6 +70,7 @@ export type TAccessApprovalRequest = {
secretPath?: string | null;
envId: string;
enforcementLevel: EnforcementLevel;
deletedAt: Date | null;
};
reviewers: {

View File

@@ -130,12 +130,14 @@ export const AccessApprovalRequest = ({
if (statusFilter === "open")
return requests?.filter(
(request) =>
!request.policy.deletedAt &&
!request.isApproved &&
!request.reviewers.some((reviewer) => reviewer.status === ApprovalStatus.REJECTED)
);
if (statusFilter === "close")
return requests?.filter(
(request) =>
request.policy.deletedAt ||
request.isApproved ||
request.reviewers.some((reviewer) => reviewer.status === ApprovalStatus.REJECTED)
);
@@ -144,8 +146,6 @@ export const AccessApprovalRequest = ({
}, [requests, statusFilter, requestedByFilter, envFilter]);
const generateRequestDetails = (request: TAccessApprovalRequest) => {
console.log(request);
const isReviewedByUser = request.reviewers.findIndex(({ member }) => member === user.id) !== -1;
const isRejectedByAnyone = request.reviewers.some(
({ status }) => status === ApprovalStatus.REJECTED