From e5fb1ac8085c11dc80a99617936de053859e4a7a Mon Sep 17 00:00:00 2001 From: = Date: Thu, 12 Jun 2025 15:31:41 +0530 Subject: [PATCH] feat: updated ui based on review --- ...0250603094506_access-request-sequential.ts | 11 +++ .../access-approval-policy-service.ts | 4 +- .../access-approval-request-dal.ts | 27 ++++++- .../access-approval-request-service.ts | 5 +- .../access-controls/access-requests.mdx | 4 +- .../src/hooks/api/accessApproval/types.ts | 3 +- .../AccessApprovalRequest.tsx | 8 +- .../components/ReviewAccessModal.tsx | 75 +++++++++++++------ 8 files changed, 102 insertions(+), 35 deletions(-) diff --git a/backend/src/db/migrations/20250603094506_access-request-sequential.ts b/backend/src/db/migrations/20250603094506_access-request-sequential.ts index 67043050a..901e9e7ac 100644 --- a/backend/src/db/migrations/20250603094506_access-request-sequential.ts +++ b/backend/src/db/migrations/20250603094506_access-request-sequential.ts @@ -14,6 +14,17 @@ export async function up(knex: Knex): Promise { if (!hasApprovalRequiredColumn) t.integer("approvalsRequired").nullable(); }); } + + // set rejected status for all access request that was rejected and still has status pending + await knex(TableName.AccessApprovalRequest) + .leftJoin( + TableName.AccessApprovalRequestReviewer, + `${TableName.AccessApprovalRequest}.id`, + `${TableName.AccessApprovalRequestReviewer}.requestId` + ) + .where(`${TableName.AccessApprovalRequest}.status` as "status", "pending") + .where(`${TableName.AccessApprovalRequestReviewer}.status` as "status", "rejected") + .update(`${TableName.AccessApprovalRequest}.status` as "status", "rejected"); } export async function down(knex: Knex): Promise { 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 612b9a340..0007868df 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 @@ -42,7 +42,7 @@ type TAccessApprovalPolicyServiceFactoryDep = { projectMembershipDAL: Pick; groupDAL: TGroupDALFactory; userDAL: Pick; - accessApprovalRequestDAL: Pick; + accessApprovalRequestDAL: Pick; additionalPrivilegeDAL: Pick; accessApprovalRequestReviewerDAL: Pick; orgMembershipDAL: Pick; @@ -481,6 +481,8 @@ export const accessApprovalPolicyServiceFactory = ({ ); } + await accessApprovalRequestDAL.resetReviewByPolicyId(doc.id, tx); + return doc; }); return { 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 78dfddcc6..b1acc9986 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 @@ -173,8 +173,7 @@ export const accessApprovalRequestDALFactory = (db: TDbClient) => { permissions: doc.privilegePermissions } : null, - - isApproved: !!doc.policyDeletedAt || !!doc.privilegeId || doc.status !== ApprovalStatus.PENDING + isApproved: doc.status === ApprovalStatus.APPROVED }), childrenMapper: [ { @@ -556,5 +555,27 @@ export const accessApprovalRequestDALFactory = (db: TDbClient) => { } }; - return { ...accessApprovalRequestOrm, findById, findRequestsWithPrivilegeByPolicyIds, getCount }; + const resetReviewByPolicyId = async (policyId: string, tx?: Knex) => { + try { + await (tx || db)(TableName.AccessApprovalRequestReviewer) + .leftJoin( + TableName.AccessApprovalRequest, + `${TableName.AccessApprovalRequest}.id`, + `${TableName.AccessApprovalRequestReviewer}.requestId` + ) + .where(`${TableName.AccessApprovalRequest}.status` as "status", ApprovalStatus.PENDING) + .where(`${TableName.AccessApprovalRequest}.policyId` as "policyId", policyId) + .del(); + } catch (error) { + throw new DatabaseError({ error, name: "ResetReviewByPolicyId" }); + } + }; + + return { + ...accessApprovalRequestOrm, + findById, + findRequestsWithPrivilegeByPolicyIds, + getCount, + resetReviewByPolicyId + }; }; 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 3747af252..8fafc6b85 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 @@ -380,9 +380,10 @@ export const accessApprovalRequestServiceFactory = ({ } const existingReviews = await accessApprovalRequestReviewerDAL.find({ requestId: accessApprovalRequest.id }); - if (existingReviews.some((review) => review.status === ApprovalStatus.REJECTED)) { - throw new BadRequestError({ message: "The request has already been rejected by another reviewer" }); + if (accessApprovalRequest.status !== ApprovalStatus.PENDING) { + throw new BadRequestError({ message: "The request has been closed" }); } + const reviewsGroupById = groupBy( existingReviews.filter((review) => review.status === ApprovalStatus.APPROVED), (i) => i.reviewerUserId diff --git a/docs/documentation/platform/access-controls/access-requests.mdx b/docs/documentation/platform/access-controls/access-requests.mdx index bfc370d10..cc0add8d8 100644 --- a/docs/documentation/platform/access-controls/access-requests.mdx +++ b/docs/documentation/platform/access-controls/access-requests.mdx @@ -14,8 +14,8 @@ This functionality works in the following way: A step policy enables a sequential approval workflow in which approvals must follow the designated chain. - ![Access Request - Policies](/images/platform/access-controls/access-request-policies.png) + + ![Access Request Policies](/images/platform/access-controls/access-request-policies.png) 2. When a developer requests access to one of such sensitive resources, the request is visible in the dashboard, and the corresponding eligible approvers get an email notification about it. ![Access Request Create](/images/platform/access-controls/request-access.png) diff --git a/frontend/src/hooks/api/accessApproval/types.ts b/frontend/src/hooks/api/accessApproval/types.ts index ac94e0468..40725a16c 100644 --- a/frontend/src/hooks/api/accessApproval/types.ts +++ b/frontend/src/hooks/api/accessApproval/types.ts @@ -1,5 +1,6 @@ import { EnforcementLevel, PolicyType } from "../policies/enums"; import { TProjectPermission } from "../roles/types"; +import { ApprovalStatus } from "../secretApprovalRequest/types"; import { WorkspaceEnv } from "../workspace/types"; export type TAccessApprovalPolicy = { @@ -75,7 +76,7 @@ export type TAccessApprovalRequest = { permissions: TProjectPermission[]; isApproved: boolean; } | null; - + status: ApprovalStatus; policy: { id: string; name: string; 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 5bcc12f18..86483e2fd 100644 --- a/frontend/src/pages/secret-manager/SecretApprovalsPage/components/AccessApprovalRequest/AccessApprovalRequest.tsx +++ b/frontend/src/pages/secret-manager/SecretApprovalsPage/components/AccessApprovalRequest/AccessApprovalRequest.tsx @@ -398,11 +398,9 @@ export const AccessApprovalRequest = ({ )}
- {details.isApprover && ( - - {details.displayData.label} - - )} + + {details.displayData.label} +
diff --git a/frontend/src/pages/secret-manager/SecretApprovalsPage/components/AccessApprovalRequest/components/ReviewAccessModal.tsx b/frontend/src/pages/secret-manager/SecretApprovalsPage/components/AccessApprovalRequest/components/ReviewAccessModal.tsx index a7a92980e..878052d83 100644 --- a/frontend/src/pages/secret-manager/SecretApprovalsPage/components/AccessApprovalRequest/components/ReviewAccessModal.tsx +++ b/frontend/src/pages/secret-manager/SecretApprovalsPage/components/AccessApprovalRequest/components/ReviewAccessModal.tsx @@ -212,15 +212,40 @@ export const ReviewAccessRequestModal = ({ (approverChain?.approvals || 1); const hasRejected = reviewers.filter((el) => el.status === ApprovalStatus.REJECTED).length; - return { ...approverChain, reviewers, hasApproved, hasRejected }; }); - return { approvers, membersGroupById, projectGroupsGroupById }; + const currentSequenceApprover = approvers?.find((el) => !el.hasApproved); + const currentSequence = currentSequenceApprover?.sequence || 1; + const isMyReviewInThisSequence = currentSequenceApprover?.reviewers.find( + (i) => i.userId === user.id + ); + + return { + approvers, + membersGroupById, + projectGroupsGroupById, + currentSequence, + isMyReviewInThisSequence + }; }, [request, policies]); - const hasRejected = request.reviewers.find((el) => el.status === ApprovalStatus.REJECTED); + const hasRejected = request.status === ApprovalStatus.REJECTED; + const hasApproved = request.status === ApprovalStatus.APPROVED; const isReviewedByMe = request.reviewers.find((i) => i.userId === user.id); + const shouldBlockRequestActions = + hasRejected || + hasApproved || + isReviewedByMe || + (!approverSequence?.isMyReviewInThisSequence && !canBypass); + + const renderCompletedMessages = () => { + if (hasRejected) return "This request has been rejected."; + if (hasApproved) return "This request has been approved."; + if (isReviewedByMe) return "You have reviewed this request."; + return "You are not the reviewer in this step."; + }; + return ( (
-
{index + 1}
+ {index + 1}
{index !== (approverSequence?.approvers?.length || 0) - 1 && (
- +
Reviewers
{approver.reviewers.map((el, idx) => ( -
+
{el.username}
))}
- {hasRejected || isReviewedByMe ? ( + {approverSequence.isMyReviewInThisSequence && + request.status === ApprovalStatus.PENDING && ( +
+ Awaiting review from you. +
+ )} + {shouldBlockRequestActions ? (
- {isReviewedByMe - ? "You have reviewed this request." - : "This request has been rejected."} + {renderCompletedMessages()}
) : ( <>