ui and backend improvements

2026-09-22 13:39:35 +00:00 · 2025-05-21 19:46:47 -04:00
parent 7041b88b9d
commit 4d173ad163
3 changed files with 129 additions and 88 deletions
--- 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
@@ -386,7 +386,21 @@ export const accessApprovalRequestServiceFactory = ({
    }

    const reviewStatus = await accessApprovalRequestReviewerDAL.transaction(async (tx) => {
-      const review = await accessApprovalRequestReviewerDAL.findOne(
+      const isBreakGlassApprovalAttempt =
+        policy.enforcementLevel === EnforcementLevel.Soft &&
+        actorId === accessApprovalRequest.requestedByUserId &&
+        status === ApprovalStatus.APPROVED;
+
+      let reviewForThisActorProcessing: {
+        id: string;
+        requestId: string;
+        reviewerUserId: string;
+        status: string;
+        createdAt: Date;
+        updatedAt: Date;
+      };
+
+      const existingReviewByActorInTx = await accessApprovalRequestReviewerDAL.findOne(
        {
          requestId: accessApprovalRequest.id,
          reviewerUserId: actorId
@@ -394,69 +408,82 @@ export const accessApprovalRequestServiceFactory = ({
        tx
      );

-      if (review) {
-        throw new BadRequestError({ message: "You have already reviewed this request" });
-      }
-
-      const newReview = await accessApprovalRequestReviewerDAL.create(
-        {
-          status,
-          requestId: accessApprovalRequest.id,
-          reviewerUserId: actorId
-        },
-        tx
-      );
-
-      const allReviews = [...existingReviews, newReview];
-      const approvedReviews = allReviews.filter((r) => r.status === ApprovalStatus.APPROVED);
-
-      if (status === ApprovalStatus.APPROVED && approvedReviews.length >= policy.approvals) {
-        if (accessApprovalRequest.isTemporary && !accessApprovalRequest.temporaryRange) {
-          throw new BadRequestError({ message: "Temporary range is required for temporary access" });
-        }
-
-        let privilegeId: string | null = null;
-
-        if (!accessApprovalRequest.isTemporary && !accessApprovalRequest.temporaryRange) {
-          // Permanent access
-          const privilege = await additionalPrivilegeDAL.create(
-            {
-              userId: accessApprovalRequest.requestedByUserId,
-              projectId: accessApprovalRequest.projectId,
-              slug: `requested-privilege-${slugify(alphaNumericNanoId(12))}`,
-              permissions: JSON.stringify(accessApprovalRequest.permissions)
-            },
-            tx
-          );
-          privilegeId = privilege.id;
+      // Check if review exists for actor
+      if (existingReviewByActorInTx) {
+        // Check if breakglass re-approval
+        if (isBreakGlassApprovalAttempt && existingReviewByActorInTx.status === ApprovalStatus.APPROVED) {
+          reviewForThisActorProcessing = existingReviewByActorInTx;
        } else {
-          // Temporary access
-          const relativeTempAllocatedTimeInMs = ms(accessApprovalRequest.temporaryRange!);
-          const startTime = new Date();
-
-          const privilege = await additionalPrivilegeDAL.create(
-            {
-              userId: accessApprovalRequest.requestedByUserId,
-              projectId: accessApprovalRequest.projectId,
-              slug: `requested-privilege-${slugify(alphaNumericNanoId(12))}`,
-              permissions: JSON.stringify(accessApprovalRequest.permissions),
-              isTemporary: true, // Explicitly set to true for the privilege
-              temporaryMode: ProjectUserAdditionalPrivilegeTemporaryMode.Relative,
-              temporaryRange: accessApprovalRequest.temporaryRange!,
-              temporaryAccessStartTime: startTime,
-              temporaryAccessEndTime: new Date(startTime.getTime() + relativeTempAllocatedTimeInMs)
-            },
-            tx
-          );
-          privilegeId = privilege.id;
+          throw new BadRequestError({ message: "You have already reviewed this request" });
        }
-        await accessApprovalRequestDAL.updateById(accessApprovalRequest.id, { privilegeId }, tx);
+      } else {
+        reviewForThisActorProcessing = await accessApprovalRequestReviewerDAL.create(
+          {
+            status,
+            requestId: accessApprovalRequest.id,
+            reviewerUserId: actorId
+          },
+          tx
+        );
      }

-      const isSoftEnforcement = policy.enforcementLevel === EnforcementLevel.Soft;
-      const wasSelfRequestAndReview = actorId === accessApprovalRequest.requestedByUserId;
+      const otherReviews = existingReviews.filter((er) => er.reviewerUserId !== actorId);
+      const allUniqueReviews = [...otherReviews, reviewForThisActorProcessing];

-      if (isSoftEnforcement && wasSelfRequestAndReview && status === ApprovalStatus.APPROVED) {
+      const approvedReviews = allUniqueReviews.filter((r) => r.status === ApprovalStatus.APPROVED);
+      const meetsStandardApprovalThreshold = approvedReviews.length >= policy.approvals;
+
+      if (
+        reviewForThisActorProcessing.status === ApprovalStatus.APPROVED &&
+        (meetsStandardApprovalThreshold || isBreakGlassApprovalAttempt)
+      ) {
+        const currentRequestState = await accessApprovalRequestDAL.findById(accessApprovalRequest.id, tx);
+        let privilegeIdToSet = currentRequestState?.privilegeId || null;
+
+        if (!privilegeIdToSet) {
+          if (accessApprovalRequest.isTemporary && !accessApprovalRequest.temporaryRange) {
+            throw new BadRequestError({ message: "Temporary range is required for temporary access" });
+          }
+
+          if (!accessApprovalRequest.isTemporary && !accessApprovalRequest.temporaryRange) {
+            // Permanent access
+            const privilege = await additionalPrivilegeDAL.create(
+              {
+                userId: accessApprovalRequest.requestedByUserId,
+                projectId: accessApprovalRequest.projectId,
+                slug: `requested-privilege-${slugify(alphaNumericNanoId(12))}`,
+                permissions: JSON.stringify(accessApprovalRequest.permissions)
+              },
+              tx
+            );
+            privilegeIdToSet = privilege.id;
+          } else {
+            // Temporary access
+            const relativeTempAllocatedTimeInMs = ms(accessApprovalRequest.temporaryRange!);
+            const startTime = new Date();
+
+            const privilege = await additionalPrivilegeDAL.create(
+              {
+                userId: accessApprovalRequest.requestedByUserId,
+                projectId: accessApprovalRequest.projectId,
+                slug: `requested-privilege-${slugify(alphaNumericNanoId(12))}`,
+                permissions: JSON.stringify(accessApprovalRequest.permissions),
+                isTemporary: true, // Explicitly set to true for the privilege
+                temporaryMode: ProjectUserAdditionalPrivilegeTemporaryMode.Relative,
+                temporaryRange: accessApprovalRequest.temporaryRange!,
+                temporaryAccessStartTime: startTime,
+                temporaryAccessEndTime: new Date(startTime.getTime() + relativeTempAllocatedTimeInMs)
+              },
+              tx
+            );
+            privilegeIdToSet = privilege.id;
+          }
+          await accessApprovalRequestDAL.updateById(accessApprovalRequest.id, { privilegeId: privilegeIdToSet }, tx);
+        }
+      }
+
+      // Send notification if this was a breakglass approval
+      if (isBreakGlassApprovalAttempt) {
        const cfg = getConfig();
        const actingUser = await userDAL.findById(actorId, tx);

@@ -491,7 +518,7 @@ export const accessApprovalRequestServiceFactory = ({
          }
        }
      }
-      return newReview;
+      return reviewForThisActorProcessing;
    });

    return reviewStatus;
--- a/frontend/src/pages/secret-manager/SecretApprovalsPage/components/AccessApprovalRequest/AccessApprovalRequest.tsx
+++ b/frontend/src/pages/secret-manager/SecretApprovalsPage/components/AccessApprovalRequest/AccessApprovalRequest.tsx
@@ -86,6 +86,7 @@ export const AccessApprovalRequest = ({
    | (TAccessApprovalRequest & {
        user: { firstName?: string; lastName?: string; email?: string } | null;
        isRequestedByCurrentUser: boolean;
+        isSelfApproveAllowed: boolean;
        isApprover: boolean;
      })
    | null
@@ -351,18 +352,24 @@ export const AccessApprovalRequest = ({
                    role="button"
                    tabIndex={0}
                    onClick={() => {
-                      if (
+                      // Whether the request has already been approved / rejected / reviewed
+                      const isInactive =
                        details.isAccepted ||
                        details.isReviewedByUser ||
-                        details.isRejectedByAnyone ||
-                        (!details.isApprover &&
-                          !(
-                            details.isSoftEnforcement &&
-                            details.isRequestedByCurrentUser &&
-                            canBypassApprovalPermission
-                          ))
-                      )
-                        return;
+                        details.isRejectedByAnyone;
+
+                      // Whether the current user can bypass policy
+                      const canBypass =
+                        details.isSoftEnforcement &&
+                        details.isRequestedByCurrentUser &&
+                        canBypassApprovalPermission;
+
+                      // Whether the current user can approve
+                      const canApprove =
+                        details.isApprover &&
+                        (!details.isRequestedByCurrentUser || details.isSelfApproveAllowed);
+
+                      if (isInactive || (!canApprove && !canBypass)) return;

                      if (
                        membersGroupById?.[request.requestedByUserId].user ||
@@ -376,6 +383,7 @@ export const AccessApprovalRequest = ({
                              ? user
                              : membersGroupById?.[request.requestedByUserId].user,
                          isRequestedByCurrentUser: details.isRequestedByCurrentUser,
+                          isSelfApproveAllowed: details.isSelfApproveAllowed,
                          isApprover: details.isApprover
                        });
                      }
@@ -383,18 +391,24 @@ export const AccessApprovalRequest = ({
                      handlePopUpOpen("reviewRequest");
                    }}
                    onKeyDown={(evt) => {
-                      if (
+                      // Whether the request has already been approved / rejected / reviewed
+                      const isInactive =
                        details.isAccepted ||
                        details.isReviewedByUser ||
-                        details.isRejectedByAnyone ||
-                        (!details.isApprover &&
-                          !(
-                            details.isSoftEnforcement &&
-                            details.isRequestedByCurrentUser &&
-                            canBypassApprovalPermission
-                          ))
-                      )
-                        return;
+                        details.isRejectedByAnyone;
+
+                      // Whether the current user can bypass policy
+                      const canBypass =
+                        details.isSoftEnforcement &&
+                        details.isRequestedByCurrentUser &&
+                        canBypassApprovalPermission;
+
+                      // Whether the current user can approve
+                      const canApprove =
+                        details.isApprover &&
+                        (!details.isRequestedByCurrentUser || details.isSelfApproveAllowed);
+
+                      if (isInactive || (!canApprove && !canBypass)) return;

                      if (evt.key === "Enter") {
                        if (
@@ -409,6 +423,7 @@ export const AccessApprovalRequest = ({
                                ? user
                                : membersGroupById?.[request.requestedByUserId].user,
                            isRequestedByCurrentUser: details.isRequestedByCurrentUser,
+                            isSelfApproveAllowed: details.isSelfApproveAllowed,
                            isApprover: details.isApprover
                          });
                        }
--- a/frontend/src/pages/secret-manager/SecretApprovalsPage/components/AccessApprovalRequest/components/ReviewAccessModal.tsx
+++ b/frontend/src/pages/secret-manager/SecretApprovalsPage/components/AccessApprovalRequest/components/ReviewAccessModal.tsx
@@ -26,6 +26,7 @@ export const ReviewAccessRequestModal = ({
  request: TAccessApprovalRequest & {
    user: { firstName?: string; lastName?: string; email?: string } | null;
    isRequestedByCurrentUser: boolean;
+    isSelfApproveAllowed: boolean;
    isApprover: boolean;
  };
  projectSlug: string;
@@ -155,7 +156,6 @@ export const ReviewAccessRequestModal = ({
            )}{" "}
            is requesting access to the following resource:
          </span>
-
          <div className="mb-2 mt-4 border-l border-blue-500 bg-blue-500/20 px-3 py-2 text-mineshaft-200">
            <div className="mb-1 lowercase">
              <span className="font-bold capitalize">Requested path: </span>
@@ -179,16 +179,16 @@ export const ReviewAccessRequestModal = ({
              </div>
            )}
          </div>
-
          <div className="space-x-2">
            <Button
              isLoading={isLoading === "approved"}
              isDisabled={
                !!isLoading ||
-                (!request.isApprover &&
-                  !bypassApproval &&
-                  isSoftEnforcement &&
-                  canBypassApprovalPermission)
+                (!(
+                  request.isApprover &&
+                  (!request.isRequestedByCurrentUser || request.isSelfApproveAllowed)
+                ) &&
+                  !bypassApproval)
              }
              onClick={() => handleReview("approved")}
              className="mt-4"
@@ -207,10 +207,9 @@ export const ReviewAccessRequestModal = ({
              Reject Request
            </Button>
          </div>
-
          {isSoftEnforcement &&
            request.isRequestedByCurrentUser &&
-            !request.isApprover &&
+            !(request.isApprover && request.isSelfApproveAllowed) &&
            canBypassApprovalPermission && (
              <div className="mt-2 flex flex-col space-y-2">
                <Checkbox