From 7c7af347fceabfa7f787a5010d2a82da09bb2621 Mon Sep 17 00:00:00 2001 From: Scott Wilson Date: Fri, 20 Jun 2025 12:25:28 -0700 Subject: [PATCH] improvements: address feedback and fix bugs --- .../v1/access-approval-request-router.ts | 4 +- .../access-approval-request-dal.ts | 13 ++- .../access-approval-request-service.ts | 6 +- .../access-approval-request-types.ts | 2 +- .../src/ee/services/license/license-fns.ts | 2 +- .../secret-approval-request-dal.ts | 79 ++++++++------- .../src/hooks/api/accessApproval/queries.tsx | 21 ++-- .../src/hooks/api/accessApproval/types.ts | 2 +- .../AccessApprovalRequest.tsx | 95 ++++++++++++++----- .../SecretApprovalRequest.tsx | 8 +- 10 files changed, 146 insertions(+), 86 deletions(-) diff --git a/backend/src/ee/routes/v1/access-approval-request-router.ts b/backend/src/ee/routes/v1/access-approval-request-router.ts index 65fcf3c86..8a6b2be88 100644 --- a/backend/src/ee/routes/v1/access-approval-request-router.ts +++ b/backend/src/ee/routes/v1/access-approval-request-router.ts @@ -89,7 +89,7 @@ export const registerAccessApprovalRequestRouter = async (server: FastifyZodProv schema: { querystring: z.object({ projectSlug: z.string().trim(), - authorProjectMembershipId: z.string().trim().optional(), + authorUserId: z.string().trim().optional(), envSlug: z.string().trim().optional() }), response: { @@ -143,7 +143,7 @@ export const registerAccessApprovalRequestRouter = async (server: FastifyZodProv handler: async (req) => { const { requests } = await server.services.accessApprovalRequest.listApprovalRequests({ projectSlug: req.query.projectSlug, - authorProjectMembershipId: req.query.authorProjectMembershipId, + authorUserId: req.query.authorUserId, envSlug: req.query.envSlug, actor: req.permission.type, actorId: req.permission.id, diff --git a/backend/src/ee/services/access-approval-request/access-approval-request-dal.ts b/backend/src/ee/services/access-approval-request/access-approval-request-dal.ts index c69c55041..33e9f7a32 100644 --- a/backend/src/ee/services/access-approval-request/access-approval-request-dal.ts +++ b/backend/src/ee/services/access-approval-request/access-approval-request-dal.ts @@ -725,16 +725,17 @@ export const accessApprovalRequestDALFactory = (db: TDbClient): TAccessApprovalR ) .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")); + .select(db.ref("reviewerUserId").withSchema(TableName.AccessApprovalRequestReviewer).as("reviewerUserId")) + .select(db.ref("deletedAt").withSchema(TableName.AccessApprovalPolicy).as("policyDeletedAt")); const formattedRequests = sqlNestRelationships({ data: accessRequests, key: "id", parentMapper: (doc) => ({ - ...AccessApprovalRequestsSchema.parse(doc) + ...AccessApprovalRequestsSchema.parse(doc), + isPolicyDeleted: Boolean(doc.policyDeletedAt) }), childrenMapper: [ { @@ -751,7 +752,8 @@ export const accessApprovalRequestDALFactory = (db: TDbClient): TAccessApprovalR (req) => !req.privilegeId && !req.reviewers.some((r) => r.status === ApprovalStatus.REJECTED) && - req.status === ApprovalStatus.PENDING + req.status === ApprovalStatus.PENDING && + !req.isPolicyDeleted ); // an approval is finalized if there are any rejections, a privilege ID is set or the number of approvals is equal to the number of approvals required. @@ -759,7 +761,8 @@ export const accessApprovalRequestDALFactory = (db: TDbClient): TAccessApprovalR (req) => req.privilegeId || req.reviewers.some((r) => r.status === ApprovalStatus.REJECTED) || - req.status !== ApprovalStatus.PENDING + req.status !== ApprovalStatus.PENDING || + req.isPolicyDeleted ); return { pendingCount: pendingApprovals.length, finalizedCount: finalizedApprovals.length }; diff --git a/backend/src/ee/services/access-approval-request/access-approval-request-service.ts b/backend/src/ee/services/access-approval-request/access-approval-request-service.ts index 70d491bf0..5a3af5aa5 100644 --- a/backend/src/ee/services/access-approval-request/access-approval-request-service.ts +++ b/backend/src/ee/services/access-approval-request/access-approval-request-service.ts @@ -275,7 +275,7 @@ export const accessApprovalRequestServiceFactory = ({ const listApprovalRequests: TAccessApprovalRequestServiceFactory["listApprovalRequests"] = async ({ projectSlug, - authorProjectMembershipId, + authorUserId, envSlug, actor, actorOrgId, @@ -300,8 +300,8 @@ export const accessApprovalRequestServiceFactory = ({ const policies = await accessApprovalPolicyDAL.find({ projectId: project.id }); let requests = await accessApprovalRequestDAL.findRequestsWithPrivilegeByPolicyIds(policies.map((p) => p.id)); - if (authorProjectMembershipId) { - requests = requests.filter((request) => request.requestedByUserId === actorId); + if (authorUserId) { + requests = requests.filter((request) => request.requestedByUserId === authorUserId); } if (envSlug) { diff --git a/backend/src/ee/services/access-approval-request/access-approval-request-types.ts b/backend/src/ee/services/access-approval-request/access-approval-request-types.ts index fb3e78de0..2550f2a96 100644 --- a/backend/src/ee/services/access-approval-request/access-approval-request-types.ts +++ b/backend/src/ee/services/access-approval-request/access-approval-request-types.ts @@ -31,7 +31,7 @@ export type TCreateAccessApprovalRequestDTO = { export type TListApprovalRequestsDTO = { projectSlug: string; - authorProjectMembershipId?: string; + authorUserId?: string; envSlug?: string; } & Omit; diff --git a/backend/src/ee/services/license/license-fns.ts b/backend/src/ee/services/license/license-fns.ts index c2db3e6e7..f3f43ef2c 100644 --- a/backend/src/ee/services/license/license-fns.ts +++ b/backend/src/ee/services/license/license-fns.ts @@ -40,7 +40,7 @@ export const getDefaultOnPremFeatures = (): TFeatureSet => ({ status: null, trial_end: null, has_used_trial: true, - secretApproval: false, + secretApproval: true, secretRotation: false, caCrl: false, instanceUserManagement: false, 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 543b1e722..5e1e546d6 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 @@ -315,7 +315,6 @@ 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") @@ -347,7 +346,7 @@ export const secretApprovalRequestDALFactory = (db: TDbClient) => { try { // akhilmhdh: If ever u wanted a 1 to so many relationship connected with pagination // this is the place u wanna look at. - const query = (tx || db.replicaNode())(TableName.SecretApprovalRequest) + const innerQuery = (tx || db.replicaNode())(TableName.SecretApprovalRequest) .join(TableName.SecretFolder, `${TableName.SecretApprovalRequest}.folderId`, `${TableName.SecretFolder}.id`) .join(TableName.Environment, `${TableName.SecretFolder}.envId`, `${TableName.Environment}.id`) .join( @@ -434,24 +433,31 @@ export const secretApprovalRequestDALFactory = (db: TDbClient) => { db.ref("email").withSchema("committerUser").as("committerUserEmail"), db.ref("username").withSchema("committerUser").as("committerUserUsername"), db.ref("firstName").withSchema("committerUser").as("committerUserFirstName"), - db.ref("lastName").withSchema("committerUser").as("committerUserLastName"), - - db.raw(`count(*) OVER() as total_count`) + db.ref("lastName").withSchema("committerUser").as("committerUserLastName") ) - .orderBy("createdAt", "desc"); + .distinctOn(`${TableName.SecretApprovalRequest}.id`) + .as("inner"); + + const query = (tx || db) + .select("*") + .select(db.raw("count(*) OVER() as total_count")) + .from(innerQuery) + .orderBy("createdAt", "desc") as typeof innerQuery; if (search) { - void query - .whereRaw(`CONCAT_WS(' ', ??, ??) ilike ?`, [ - db.ref("firstName").withSchema("committerUser"), - db.ref("lastName").withSchema("committerUser"), - `%${search}%` - ]) - .orWhereRaw(`?? ilike ?`, [db.ref("username").withSchema("committerUser"), `%${search}%`]) - .orWhereRaw(`?? ilike ?`, [db.ref("email").withSchema("committerUser"), `%${search}%`]) - .orWhereILike(`${TableName.Environment}.name`, `%${search}%`) - .orWhereILike(`${TableName.Environment}.slug`, `%${search}%`) - .orWhereILike(`${TableName.SecretApprovalPolicy}.secretPath`, `%${search}%`); + void query.where((qb) => { + void qb + .whereRaw(`CONCAT_WS(' ', ??, ??) ilike ?`, [ + db.ref("firstName").withSchema("committerUser"), + db.ref("lastName").withSchema("committerUser"), + `%${search}%` + ]) + .orWhereRaw(`?? ilike ?`, [db.ref("username").withSchema("committerUser"), `%${search}%`]) + .orWhereRaw(`?? ilike ?`, [db.ref("email").withSchema("committerUser"), `%${search}%`]) + .orWhereILike(`${TableName.Environment}.name`, `%${search}%`) + .orWhereILike(`${TableName.Environment}.slug`, `%${search}%`) + .orWhereILike(`${TableName.SecretApprovalPolicy}.secretPath`, `%${search}%`); + }); } const docs = await (tx || db) @@ -544,7 +550,7 @@ export const secretApprovalRequestDALFactory = (db: TDbClient) => { try { // akhilmhdh: If ever u wanted a 1 to so many relationship connected with pagination // this is the place u wanna look at. - const query = (tx || db.replicaNode())(TableName.SecretApprovalRequest) + const innerQuery = (tx || db.replicaNode())(TableName.SecretApprovalRequest) .join(TableName.SecretFolder, `${TableName.SecretApprovalRequest}.folderId`, `${TableName.SecretFolder}.id`) .join(TableName.Environment, `${TableName.SecretFolder}.envId`, `${TableName.Environment}.id`) .join( @@ -631,24 +637,31 @@ export const secretApprovalRequestDALFactory = (db: TDbClient) => { db.ref("email").withSchema("committerUser").as("committerUserEmail"), db.ref("username").withSchema("committerUser").as("committerUserUsername"), db.ref("firstName").withSchema("committerUser").as("committerUserFirstName"), - db.ref("lastName").withSchema("committerUser").as("committerUserLastName"), - - db.raw(`count(*) OVER() as total_count`) + db.ref("lastName").withSchema("committerUser").as("committerUserLastName") ) - .orderBy("createdAt", "desc"); + .distinctOn(`${TableName.SecretApprovalRequest}.id`) + .as("inner"); + + const query = (tx || db) + .select("*") + .select(db.raw("count(*) OVER() as total_count")) + .from(innerQuery) + .orderBy("createdAt", "desc") as typeof innerQuery; if (search) { - void query - .whereRaw(`CONCAT_WS(' ', ??, ??) ilike ?`, [ - db.ref("firstName").withSchema("committerUser"), - db.ref("lastName").withSchema("committerUser"), - `%${search}%` - ]) - .orWhereRaw(`?? ilike ?`, [db.ref("username").withSchema("committerUser"), `%${search}%`]) - .orWhereRaw(`?? ilike ?`, [db.ref("email").withSchema("committerUser"), `%${search}%`]) - .orWhereILike(`${TableName.Environment}.name`, `%${search}%`) - .orWhereILike(`${TableName.Environment}.slug`, `%${search}%`) - .orWhereILike(`${TableName.SecretApprovalPolicy}.secretPath`, `%${search}%`); + void query.where((qb) => { + void qb + .whereRaw(`CONCAT_WS(' ', ??, ??) ilike ?`, [ + db.ref("firstName").withSchema("committerUser"), + db.ref("lastName").withSchema("committerUser"), + `%${search}%` + ]) + .orWhereRaw(`?? ilike ?`, [db.ref("username").withSchema("committerUser"), `%${search}%`]) + .orWhereRaw(`?? ilike ?`, [db.ref("email").withSchema("committerUser"), `%${search}%`]) + .orWhereILike(`${TableName.Environment}.name`, `%${search}%`) + .orWhereILike(`${TableName.Environment}.slug`, `%${search}%`) + .orWhereILike(`${TableName.SecretApprovalPolicy}.secretPath`, `%${search}%`); + }); } const rankOffset = offset + 1; diff --git a/frontend/src/hooks/api/accessApproval/queries.tsx b/frontend/src/hooks/api/accessApproval/queries.tsx index 6370f4a59..f5478cd1b 100644 --- a/frontend/src/hooks/api/accessApproval/queries.tsx +++ b/frontend/src/hooks/api/accessApproval/queries.tsx @@ -65,11 +65,11 @@ const fetchApprovalPolicies = async ({ projectSlug }: TGetAccessApprovalRequests const fetchApprovalRequests = async ({ projectSlug, envSlug, - authorProjectMembershipId + authorUserId }: TGetAccessApprovalRequestsDTO) => { const { data } = await apiRequest.get<{ requests: TAccessApprovalRequest[] }>( "/api/v1/access-approvals/requests", - { params: { projectSlug, envSlug, authorProjectMembershipId } } + { params: { projectSlug, envSlug, authorUserId } } ); return data.requests.map((request) => ({ @@ -109,12 +109,12 @@ export const useGetAccessRequestsCount = ({ export const useGetAccessApprovalPolicies = ({ projectSlug, envSlug, - authorProjectMembershipId, + authorUserId, options = {} }: TGetAccessApprovalRequestsDTO & TReactQueryOptions) => useQuery({ queryKey: accessApprovalKeys.getAccessApprovalPolicies(projectSlug), - queryFn: () => fetchApprovalPolicies({ projectSlug, envSlug, authorProjectMembershipId }), + queryFn: () => fetchApprovalPolicies({ projectSlug, envSlug, authorUserId }), ...options, enabled: Boolean(projectSlug) && (options?.enabled ?? true) }); @@ -122,16 +122,13 @@ export const useGetAccessApprovalPolicies = ({ export const useGetAccessApprovalRequests = ({ projectSlug, envSlug, - authorProjectMembershipId, + authorUserId, options = {} }: TGetAccessApprovalRequestsDTO & TReactQueryOptions) => useQuery({ - queryKey: accessApprovalKeys.getAccessApprovalRequests( - projectSlug, - envSlug, - authorProjectMembershipId - ), - queryFn: () => fetchApprovalRequests({ projectSlug, envSlug, authorProjectMembershipId }), + queryKey: accessApprovalKeys.getAccessApprovalRequests(projectSlug, envSlug, authorUserId), + queryFn: () => fetchApprovalRequests({ projectSlug, envSlug, authorUserId }), ...options, - enabled: Boolean(projectSlug) && (options?.enabled ?? true) + enabled: Boolean(projectSlug) && (options?.enabled ?? true), + placeholderData: (previousData) => previousData }); diff --git a/frontend/src/hooks/api/accessApproval/types.ts b/frontend/src/hooks/api/accessApproval/types.ts index 40725a16c..28330b3ad 100644 --- a/frontend/src/hooks/api/accessApproval/types.ts +++ b/frontend/src/hooks/api/accessApproval/types.ts @@ -148,7 +148,7 @@ export type TCreateAccessRequestDTO = { export type TGetAccessApprovalRequestsDTO = { projectSlug: string; envSlug?: string; - authorProjectMembershipId?: string; + authorUserId?: string; }; export type TGetAccessPolicyApprovalCountDTO = { diff --git a/frontend/src/pages/secret-manager/SecretApprovalsPage/components/AccessApprovalRequest/AccessApprovalRequest.tsx b/frontend/src/pages/secret-manager/SecretApprovalsPage/components/AccessApprovalRequest/AccessApprovalRequest.tsx index 9153c139b..f8b935e29 100644 --- a/frontend/src/pages/secret-manager/SecretApprovalsPage/components/AccessApprovalRequest/AccessApprovalRequest.tsx +++ b/frontend/src/pages/secret-manager/SecretApprovalsPage/components/AccessApprovalRequest/AccessApprovalRequest.tsx @@ -3,6 +3,7 @@ import { useCallback, useMemo, useState } from "react"; import { faArrowUpRightFromSquare, + faBan, faBookOpen, faCheck, faCheckCircle, @@ -12,10 +13,12 @@ import { faMagnifyingGlass, faPlus, faSearch, - faUser + faStopwatch, + faUser, + IconDefinition } from "@fortawesome/free-solid-svg-icons"; import { FontAwesomeIcon } from "@fortawesome/react-fontawesome"; -import { formatDistance } from "date-fns"; +import { format, formatDistance } from "date-fns"; import { AnimatePresence, motion } from "framer-motion"; import { twMerge } from "tailwind-merge"; @@ -133,7 +136,7 @@ export const AccessApprovalRequest = ({ isPending: areRequestsPending } = useGetAccessApprovalRequests({ projectSlug, - authorProjectMembershipId: requestedByFilter, + authorUserId: requestedByFilter, envSlug: envFilter }); @@ -203,9 +206,15 @@ export const AccessApprovalRequest = ({ const canBypass = !request.policy.bypassers.length || request.policy.bypassers.includes(user.id); - let displayData: { label: string; type: "primary" | "danger" | "success" } = { + let displayData: { + label: string; + type: "primary" | "danger" | "success"; + tooltipContent?: string; + icon: IconDefinition | null; + } = { label: "", - type: "primary" + type: "primary", + icon: null }; const isExpired = @@ -213,20 +222,42 @@ export const AccessApprovalRequest = ({ request.isApproved && new Date() > new Date(request.privilege.temporaryAccessEndTime || ("" as string)); - if (isExpired) displayData = { label: "Access Expired", type: "danger" }; - else if (isAccepted) displayData = { label: "Access Granted", type: "success" }; - else if (isRejectedByAnyone) displayData = { label: "Rejected", type: "danger" }; + if (isExpired) + displayData = { + label: "Access Expired", + type: "danger", + icon: faStopwatch, + tooltipContent: request.privilege?.temporaryAccessEndTime + ? `Expired ${format(request.privilege.temporaryAccessEndTime, "M/d/yyyy h:mm aa")}` + : undefined + }; + else if (isAccepted) + displayData = { + label: "Access Granted", + type: "success", + icon: faCheck, + tooltipContent: `Granted ${format(request.updatedAt, "M/d/yyyy h:mm aa")}` + }; + else if (isRejectedByAnyone) + displayData = { + label: "Rejected", + type: "danger", + icon: faBan, + tooltipContent: `Rejected ${format(request.updatedAt, "M/d/yyyy h:mm aa")}` + }; else if (userReviewStatus === ApprovalStatus.APPROVED) { displayData = { label: `Pending ${request.policy.approvals - request.reviewers.length} review${ request.policy.approvals - request.reviewers.length > 1 ? "s" : "" }`, - type: "primary" + type: "primary", + icon: faClipboardCheck }; } else if (!isReviewedByUser) displayData = { label: "Review Required", - type: "primary" + type: "primary", + icon: faClipboardCheck }; return { @@ -266,6 +297,8 @@ export const AccessApprovalRequest = ({ [generateRequestDetails, membersGroupById, user, setSelectedRequest, handlePopUpOpen] ); + const isFiltered = Boolean(search || envFilter || requestedByFilter); + return ( - {!!requestCount && requestCount.finalizedCount} Completed + {!!requestCount && requestCount.finalizedCount} Closed
@@ -365,7 +398,7 @@ export const AccessApprovalRequest = ({
- {filteredRequests?.length === 0 && !search && ( + {filteredRequests?.length === 0 && !isFiltered && (
)} - {Boolean(!filteredRequests?.length && search && !areRequestsPending) && ( + {Boolean(!filteredRequests?.length && isFiltered && !areRequestsPending) && (
- +
)} {!!filteredRequests?.length && @@ -494,13 +533,19 @@ export const AccessApprovalRequest = ({ Requested By You
)} - - - {details.displayData.label} - + +
+ + {details.displayData.icon && ( + + )} + {details.displayData.label} + +
+ diff --git a/frontend/src/pages/secret-manager/SecretApprovalsPage/components/SecretApprovalRequest/SecretApprovalRequest.tsx b/frontend/src/pages/secret-manager/SecretApprovalsPage/components/SecretApprovalRequest/SecretApprovalRequest.tsx index e2b10b826..eafb2ac9d 100644 --- a/frontend/src/pages/secret-manager/SecretApprovalsPage/components/SecretApprovalRequest/SecretApprovalRequest.tsx +++ b/frontend/src/pages/secret-manager/SecretApprovalsPage/components/SecretApprovalRequest/SecretApprovalRequest.tsx @@ -126,6 +126,8 @@ export const SecretApprovalRequest = () => { const isRequestListEmpty = !isApprovalRequestLoading && secretApprovalRequests?.length === 0; + const isFiltered = Boolean(searchFilter || envFilter || committerFilter); + return ( {isSecretApprovalScreen ? ( @@ -292,7 +294,7 @@ export const SecretApprovalRequest = () => {
- {isRequestListEmpty && !searchFilter && ( + {isRequestListEmpty && !isFiltered && (
{ ); })} {Boolean( - !secretApprovalRequests.length && searchFilter && !isApprovalRequestLoading + !secretApprovalRequests.length && isFiltered && !isApprovalRequestLoading ) && (
- +
)} {Boolean(totalApprovalCount) && (