Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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");
});
}
3 changes: 2 additions & 1 deletion backend/src/db/schemas/access-approval-policies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
Expand Down
3 changes: 2 additions & 1 deletion backend/src/db/schemas/secret-approval-policies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
Expand Down
3 changes: 2 additions & 1 deletion backend/src/ee/routes/v1/access-approval-request-router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down
6 changes: 4 additions & 2 deletions backend/src/ee/routes/v1/secret-approval-request-router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
};
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -21,7 +25,7 @@ import {
TUpdateAccessApprovalPolicy
} from "./access-approval-policy-types";

type TSecretApprovalPolicyServiceFactoryDep = {
type TAccessApprovalPolicyServiceFactoryDep = {
projectDAL: TProjectDALFactory;
permissionService: Pick<TPermissionServiceFactory, "getProjectPermission">;
accessApprovalPolicyDAL: TAccessApprovalPolicyDALFactory;
Expand All @@ -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>;
Expand All @@ -41,8 +48,11 @@ export const accessApprovalPolicyServiceFactory = ({
permissionService,
projectEnvDAL,
projectDAL,
userDAL
}: TSecretApprovalPolicyServiceFactoryDep) => {
userDAL,
accessApprovalRequestDAL,
additionalPrivilegeDAL,
accessApprovalRequestReviewerDAL
}: TAccessApprovalPolicyServiceFactoryDep) => {
const createAccessApprovalPolicy = async ({
name,
actor,
Expand Down Expand Up @@ -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;
};

Expand Down Expand Up @@ -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;
};

Expand Down Expand Up @@ -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 };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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,
Expand All @@ -141,7 +143,7 @@ export const accessApprovalRequestDALFactory = (db: TDbClient) => {
}
: null,

isApproved: !!doc.privilegeId
isApproved: !!doc.policyDeletedAt || !!doc.privilegeId
DanielHougaard marked this conversation as resolved.
Show resolved Hide resolved
}),
childrenMapper: [
{
Expand Down Expand Up @@ -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) => {
Expand All @@ -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,
Expand Down Expand Up @@ -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"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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" });
DanielHougaard marked this conversation as resolved.
Show resolved Hide resolved
}

const approverIds: string[] = [];
const approverGroupIds: string[] = [];
Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
};
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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>;
Expand All @@ -44,7 +47,8 @@ export const secretApprovalPolicyServiceFactory = ({
secretApprovalPolicyApproverDAL,
projectEnvDAL,
userDAL,
licenseService
licenseService,
secretApprovalRequestDAL
}: TSecretApprovalPolicyServiceFactoryDep) => {
const createSecretApprovalPolicy = async ({
name,
Expand Down Expand Up @@ -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;
});
McPizza0 marked this conversation as resolved.
Show resolved Hide resolved
return { ...deletedPolicy, projectId: sapPolicy.projectId, environment: sapPolicy.environment };
};

const getSecretApprovalPolicyByProjectId = async ({
Expand All @@ -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;
};

Expand All @@ -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(
Expand Down
Loading
Loading