From babbacdc961e8f48c8dfa48268cfc58ebb0b4fc3 Mon Sep 17 00:00:00 2001 From: Sheen Capadngan Date: Wed, 5 Mar 2025 19:25:56 +0800 Subject: [PATCH] feat: add secret approval review comment --- ...0250305080145_add-secret-review-comment.ts | 19 ++ .../secret-approval-requests-reviewers.ts | 3 +- .../v1/secret-approval-request-router.ts | 24 +- .../ee/services/audit-log/audit-log-types.ts | 15 +- .../secret-approval-request-dal.ts | 7 +- .../secret-approval-request-service.ts | 8 +- .../secret-approval-request-types.ts | 1 + .../src/hooks/api/auditLogs/constants.tsx | 1 + frontend/src/hooks/api/auditLogs/enums.tsx | 3 +- .../api/secretApprovalRequest/mutation.tsx | 5 +- .../hooks/api/secretApprovalRequest/types.ts | 2 + .../SecretApprovalRequestChanges.tsx | 218 +++++++++++++++--- 12 files changed, 260 insertions(+), 46 deletions(-) create mode 100644 backend/src/db/migrations/20250305080145_add-secret-review-comment.ts diff --git a/backend/src/db/migrations/20250305080145_add-secret-review-comment.ts b/backend/src/db/migrations/20250305080145_add-secret-review-comment.ts new file mode 100644 index 000000000..7d51bb226 --- /dev/null +++ b/backend/src/db/migrations/20250305080145_add-secret-review-comment.ts @@ -0,0 +1,19 @@ +import { Knex } from "knex"; + +import { TableName } from "../schemas"; + +export async function up(knex: Knex): Promise { + if (!(await knex.schema.hasColumn(TableName.SecretApprovalRequestReviewer, "comment"))) { + await knex.schema.alterTable(TableName.SecretApprovalRequestReviewer, (t) => { + t.string("comment"); + }); + } +} + +export async function down(knex: Knex): Promise { + if (await knex.schema.hasColumn(TableName.SecretApprovalRequestReviewer, "comment")) { + await knex.schema.alterTable(TableName.SecretApprovalRequestReviewer, (t) => { + t.dropColumn("comment"); + }); + } +} diff --git a/backend/src/db/schemas/secret-approval-requests-reviewers.ts b/backend/src/db/schemas/secret-approval-requests-reviewers.ts index a5c445587..147646b8d 100644 --- a/backend/src/db/schemas/secret-approval-requests-reviewers.ts +++ b/backend/src/db/schemas/secret-approval-requests-reviewers.ts @@ -13,7 +13,8 @@ export const SecretApprovalRequestsReviewersSchema = z.object({ requestId: z.string().uuid(), createdAt: z.date(), updatedAt: z.date(), - reviewerUserId: z.string().uuid() + reviewerUserId: z.string().uuid(), + comment: z.string().nullable().optional() }); export type TSecretApprovalRequestsReviewers = z.infer; diff --git a/backend/src/ee/routes/v1/secret-approval-request-router.ts b/backend/src/ee/routes/v1/secret-approval-request-router.ts index c6998f105..2c503515c 100644 --- a/backend/src/ee/routes/v1/secret-approval-request-router.ts +++ b/backend/src/ee/routes/v1/secret-approval-request-router.ts @@ -159,7 +159,8 @@ export const registerSecretApprovalRequestRouter = async (server: FastifyZodProv id: z.string() }), body: z.object({ - status: z.enum([ApprovalStatus.APPROVED, ApprovalStatus.REJECTED]) + status: z.enum([ApprovalStatus.APPROVED, ApprovalStatus.REJECTED]), + comment: z.string().optional() }), response: { 200: z.object({ @@ -175,8 +176,25 @@ export const registerSecretApprovalRequestRouter = async (server: FastifyZodProv actorAuthMethod: req.permission.authMethod, actorOrgId: req.permission.orgId, approvalId: req.params.id, - status: req.body.status + status: req.body.status, + comment: req.body.comment }); + + await server.services.auditLog.createAuditLog({ + ...req.auditLogInfo, + orgId: req.permission.orgId, + projectId: review.projectId, + event: { + type: EventType.SECRET_APPROVAL_REQUEST_REVIEWED, + metadata: { + secretApprovalRequestId: review.requestId, + reviewedBy: review.reviewerUserId, + status: review.status as ApprovalStatus, + comment: review.comment || "" + } + } + }); + return { review }; } }); @@ -267,7 +285,7 @@ export const registerSecretApprovalRequestRouter = async (server: FastifyZodProv environment: z.string(), statusChangedByUser: approvalRequestUser.optional(), committerUser: approvalRequestUser, - reviewers: approvalRequestUser.extend({ status: z.string() }).array(), + reviewers: approvalRequestUser.extend({ status: z.string(), comment: z.string().optional() }).array(), secretPath: z.string(), commits: secretRawSchema .omit({ _id: true, environment: true, workspace: true, type: true, version: true }) diff --git a/backend/src/ee/services/audit-log/audit-log-types.ts b/backend/src/ee/services/audit-log/audit-log-types.ts index 85e48872e..3e35c16f8 100644 --- a/backend/src/ee/services/audit-log/audit-log-types.ts +++ b/backend/src/ee/services/audit-log/audit-log-types.ts @@ -22,6 +22,7 @@ import { } from "@app/services/secret-sync/secret-sync-types"; import { KmipPermission } from "../kmip/kmip-enum"; +import { ApprovalStatus } from "../secret-approval-request/secret-approval-request-types"; export type TListProjectAuditLogDTO = { filter: { @@ -165,6 +166,7 @@ export enum EventType { SECRET_APPROVAL_REQUEST = "secret-approval-request", SECRET_APPROVAL_CLOSED = "secret-approval-closed", SECRET_APPROVAL_REOPENED = "secret-approval-reopened", + SECRET_APPROVAL_REQUEST_REVIEWED = "secret-approval-request-reviewed", SIGN_SSH_KEY = "sign-ssh-key", ISSUE_SSH_CREDS = "issue-ssh-creds", CREATE_SSH_CA = "create-ssh-certificate-authority", @@ -1314,6 +1316,16 @@ interface SecretApprovalRequest { }; } +interface SecretApprovalRequestReviewed { + type: EventType.SECRET_APPROVAL_REQUEST_REVIEWED; + metadata: { + secretApprovalRequestId: string; + reviewedBy: string; + status: ApprovalStatus; + comment: string; + }; +} + interface SignSshKey { type: EventType.SIGN_SSH_KEY; metadata: { @@ -2482,4 +2494,5 @@ export type Event = | KmipOperationRevokeEvent | KmipOperationLocateEvent | KmipOperationRegisterEvent - | CreateSecretRequestEvent; + | CreateSecretRequestEvent + | SecretApprovalRequestReviewed; diff --git a/backend/src/ee/services/secret-approval-request/secret-approval-request-dal.ts b/backend/src/ee/services/secret-approval-request/secret-approval-request-dal.ts index f842359bc..5fc869d12 100644 --- a/backend/src/ee/services/secret-approval-request/secret-approval-request-dal.ts +++ b/backend/src/ee/services/secret-approval-request/secret-approval-request-dal.ts @@ -100,6 +100,7 @@ export const secretApprovalRequestDALFactory = (db: TDbClient) => { tx.ref("lastName").withSchema("committerUser").as("committerUserLastName"), tx.ref("reviewerUserId").withSchema(TableName.SecretApprovalRequestReviewer), tx.ref("status").withSchema(TableName.SecretApprovalRequestReviewer).as("reviewerStatus"), + tx.ref("comment").withSchema(TableName.SecretApprovalRequestReviewer).as("reviewerComment"), tx.ref("email").withSchema("secretApprovalReviewerUser").as("reviewerEmail"), tx.ref("username").withSchema("secretApprovalReviewerUser").as("reviewerUsername"), tx.ref("firstName").withSchema("secretApprovalReviewerUser").as("reviewerFirstName"), @@ -162,8 +163,10 @@ export const secretApprovalRequestDALFactory = (db: TDbClient) => { reviewerEmail: email, reviewerLastName: lastName, reviewerUsername: username, - reviewerFirstName: firstName - }) => (userId ? { userId, status, email, firstName, lastName, username } : undefined) + reviewerFirstName: firstName, + reviewerComment: comment + }) => + userId ? { userId, status, email, firstName, lastName, username, comment: comment ?? "" } : undefined }, { key: "approverUserId", diff --git a/backend/src/ee/services/secret-approval-request/secret-approval-request-service.ts b/backend/src/ee/services/secret-approval-request/secret-approval-request-service.ts index 17eecf508..8569ef2a9 100644 --- a/backend/src/ee/services/secret-approval-request/secret-approval-request-service.ts +++ b/backend/src/ee/services/secret-approval-request/secret-approval-request-service.ts @@ -320,6 +320,7 @@ export const secretApprovalRequestServiceFactory = ({ approvalId, actor, status, + comment, actorId, actorAuthMethod, actorOrgId @@ -372,15 +373,18 @@ export const secretApprovalRequestServiceFactory = ({ return secretApprovalRequestReviewerDAL.create( { status, + comment, requestId: secretApprovalRequest.id, reviewerUserId: actorId }, tx ); } - return secretApprovalRequestReviewerDAL.updateById(review.id, { status }, tx); + + return secretApprovalRequestReviewerDAL.updateById(review.id, { status, comment }, tx); }); - return reviewStatus; + + return { ...reviewStatus, projectId: secretApprovalRequest.projectId }; }; const updateApprovalStatus = async ({ diff --git a/backend/src/ee/services/secret-approval-request/secret-approval-request-types.ts b/backend/src/ee/services/secret-approval-request/secret-approval-request-types.ts index 89af253dd..5d6358072 100644 --- a/backend/src/ee/services/secret-approval-request/secret-approval-request-types.ts +++ b/backend/src/ee/services/secret-approval-request/secret-approval-request-types.ts @@ -80,6 +80,7 @@ export type TStatusChangeDTO = { export type TReviewRequestDTO = { approvalId: string; status: ApprovalStatus; + comment?: string; } & Omit; export type TApprovalRequestCountDTO = TProjectPermission; diff --git a/frontend/src/hooks/api/auditLogs/constants.tsx b/frontend/src/hooks/api/auditLogs/constants.tsx index 39c351e38..445bda774 100644 --- a/frontend/src/hooks/api/auditLogs/constants.tsx +++ b/frontend/src/hooks/api/auditLogs/constants.tsx @@ -122,6 +122,7 @@ export const eventToNameMap: { [K in EventType]: string } = { "OIDC group membership mapping assigned user to groups", [EventType.OIDC_GROUP_MEMBERSHIP_MAPPING_REMOVE_USER]: "OIDC group membership mapping removed user from groups", + [EventType.SECRET_APPROVAL_REQUEST_REVIEWED]: "Review Secret Approval Request", [EventType.CREATE_KMIP_CLIENT]: "Create KMIP client", [EventType.UPDATE_KMIP_CLIENT]: "Update KMIP client", [EventType.DELETE_KMIP_CLIENT]: "Delete KMIP client", diff --git a/frontend/src/hooks/api/auditLogs/enums.tsx b/frontend/src/hooks/api/auditLogs/enums.tsx index ed25c6e4a..ad84508e7 100644 --- a/frontend/src/hooks/api/auditLogs/enums.tsx +++ b/frontend/src/hooks/api/auditLogs/enums.tsx @@ -150,5 +150,6 @@ export enum EventType { KMIP_OPERATION_ACTIVATE = "kmip-operation-activate", KMIP_OPERATION_REVOKE = "kmip-operation-revoke", KMIP_OPERATION_LOCATE = "kmip-operation-locate", - KMIP_OPERATION_REGISTER = "kmip-operation-register" + KMIP_OPERATION_REGISTER = "kmip-operation-register", + SECRET_APPROVAL_REQUEST_REVIEWED = "secret-approval-request-reviewed" } diff --git a/frontend/src/hooks/api/secretApprovalRequest/mutation.tsx b/frontend/src/hooks/api/secretApprovalRequest/mutation.tsx index ce584a85f..78e1b37a1 100644 --- a/frontend/src/hooks/api/secretApprovalRequest/mutation.tsx +++ b/frontend/src/hooks/api/secretApprovalRequest/mutation.tsx @@ -13,9 +13,10 @@ export const useUpdateSecretApprovalReviewStatus = () => { const queryClient = useQueryClient(); return useMutation({ - mutationFn: async ({ id, status }) => { + mutationFn: async ({ id, status, comment }) => { const { data } = await apiRequest.post(`/api/v1/secret-approval-requests/${id}/review`, { - status + status, + comment }); return data; }, diff --git a/frontend/src/hooks/api/secretApprovalRequest/types.ts b/frontend/src/hooks/api/secretApprovalRequest/types.ts index a03526dae..433d82855 100644 --- a/frontend/src/hooks/api/secretApprovalRequest/types.ts +++ b/frontend/src/hooks/api/secretApprovalRequest/types.ts @@ -44,6 +44,7 @@ export type TSecretApprovalRequest = { reviewers: { userId: string; status: ApprovalStatus; + comment: string; email: string; firstName: string; lastName: string; @@ -114,6 +115,7 @@ export type TGetSecretApprovalRequestDetails = { export type TUpdateSecretApprovalReviewStatusDTO = { status: ApprovalStatus; + comment?: string; id: string; }; diff --git a/frontend/src/pages/secret-manager/SecretApprovalsPage/components/SecretApprovalRequest/components/SecretApprovalRequestChanges.tsx b/frontend/src/pages/secret-manager/SecretApprovalsPage/components/SecretApprovalRequest/components/SecretApprovalRequestChanges.tsx index 04bee3042..899d46424 100644 --- a/frontend/src/pages/secret-manager/SecretApprovalsPage/components/SecretApprovalRequest/components/SecretApprovalRequestChanges.tsx +++ b/frontend/src/pages/secret-manager/SecretApprovalsPage/components/SecretApprovalRequest/components/SecretApprovalRequestChanges.tsx @@ -1,19 +1,36 @@ import { ReactNode } from "react"; +import { Controller, useForm } from "react-hook-form"; import { + faAngleDown, faArrowLeft, - faCheck, faCheckCircle, faCircle, faCodeBranch, + faComment, faFolder, faXmarkCircle } from "@fortawesome/free-solid-svg-icons"; import { FontAwesomeIcon } from "@fortawesome/react-fontawesome"; +import { zodResolver } from "@hookform/resolvers/zod"; +import { RadioGroup, RadioGroupIndicator, RadioGroupItem } from "@radix-ui/react-radio-group"; import { twMerge } from "tailwind-merge"; +import z from "zod"; import { createNotification } from "@app/components/notifications"; -import { Button, ContentLoader, EmptyState, IconButton, Tooltip } from "@app/components/v2"; +import { + Button, + ContentLoader, + DropdownMenu, + DropdownMenuContent, + DropdownMenuTrigger, + EmptyState, + FormControl, + IconButton, + TextArea, + Tooltip +} from "@app/components/v2"; import { useUser } from "@app/context"; +import { usePopUp } from "@app/hooks"; import { useGetSecretApprovalRequestDetails, useUpdateSecretApprovalReviewStatus @@ -74,6 +91,13 @@ type Props = { onGoBack: () => void; }; +const reviewFormSchema = z.object({ + comment: z.string().trim().optional().default(""), + status: z.nativeEnum(ApprovalStatus) +}); + +type TReviewFormSchema = z.infer; + export const SecretApprovalRequestChanges = ({ approvalRequestId, onGoBack, @@ -94,6 +118,16 @@ export const SecretApprovalRequestChanges = ({ variables } = useUpdateSecretApprovalReviewStatus(); + const { popUp, handlePopUpToggle } = usePopUp(["reviewChanges"] as const); + const { + control, + handleSubmit, + reset, + formState: { isSubmitting } + } = useForm({ + resolver: zodResolver(reviewFormSchema) + }); + const isApproving = variables?.status === ApprovalStatus.APPROVED && isUpdatingRequestStatus; const isRejecting = variables?.status === ApprovalStatus.REJECTED && isUpdatingRequestStatus; @@ -101,23 +135,23 @@ export const SecretApprovalRequestChanges = ({ const canApprove = secretApprovalRequestDetails?.policy?.approvers?.some( ({ userId }) => userId === userSession.id ); + const reviewedUsers = secretApprovalRequestDetails?.reviewers?.reduce< - Record + Record >( (prev, curr) => ({ ...prev, - [curr.userId]: curr.status + [curr.userId]: { status: curr.status, comment: curr.comment } }), {} ); - const hasApproved = reviewedUsers?.[userSession.id] === ApprovalStatus.APPROVED; - const hasRejected = reviewedUsers?.[userSession.id] === ApprovalStatus.REJECTED; - const handleSecretApprovalStatusUpdate = async (status: ApprovalStatus) => { + const handleSecretApprovalStatusUpdate = async (status: ApprovalStatus, comment: string) => { try { await updateSecretApprovalRequestStatus({ id: approvalRequestId, - status + status, + comment }); createNotification({ type: "success", @@ -130,6 +164,16 @@ export const SecretApprovalRequestChanges = ({ text: "Failed to update the request status" }); } + + handlePopUpToggle("reviewChanges", false); + reset({ + comment: "", + status: ApprovalStatus.APPROVED + }); + }; + + const handleSubmitReview = (data: TReviewFormSchema) => { + handleSecretApprovalStatusUpdate(data.status, data.comment); }; if (isSecretApprovalRequestLoading) { @@ -150,7 +194,7 @@ export const SecretApprovalRequestChanges = ({ const isMergable = secretApprovalRequestDetails?.policy?.approvals <= secretApprovalRequestDetails?.policy?.approvers?.filter( - ({ userId }) => reviewedUsers?.[userId] === ApprovalStatus.APPROVED + ({ userId }) => reviewedUsers?.[userId]?.status === ApprovalStatus.APPROVED ).length; const hasMerged = secretApprovalRequestDetails?.hasMerged; @@ -202,27 +246,115 @@ export const SecretApprovalRequestChanges = ({ {!hasMerged && secretApprovalRequestDetails.status === "open" && ( - <> - - - + handlePopUpToggle("reviewChanges", isOpen)} + > + + + + +
+
+
Finish your review
+ ( + +