Review fixes:

- Review envName from endpoint params and derive it - Use variables in logic blocks - New function on frontend + memoization
2026-09-22 13:39:35 +00:00 · 2025-05-23 12:05:38 -04:00
parent e81991c545
commit 804f8be07d
6 changed files with 112 additions and 149 deletions
--- a/backend/src/ee/routes/v1/access-approval-request-router.ts
+++ b/backend/src/ee/routes/v1/access-approval-request-router.ts
@@ -155,7 +155,6 @@ export const registerAccessApprovalRequestRouter = async (server: FastifyZodProv
      }),
      body: z.object({
        status: z.enum([ApprovalStatus.APPROVED, ApprovalStatus.REJECTED]),
-        envName: z.string().optional(), // For logging
        bypassReason: z.string().min(10).max(1000).optional()
      }),
      response: {
@@ -173,7 +172,6 @@ export const registerAccessApprovalRequestRouter = async (server: FastifyZodProv
        actorAuthMethod: req.permission.authMethod,
        requestId: req.params.requestId,
        status: req.body.status,
-        envName: req.body.envName,
        bypassReason: req.body.bypassReason
      });

--- 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
@@ -326,7 +326,6 @@ export const accessApprovalRequestServiceFactory = ({
    actorId,
    actorAuthMethod,
    actorOrgId,
-    envName,
    bypassReason
  }: TReviewAccessRequestDTO) => {
    const accessApprovalRequest = await accessApprovalRequestDAL.findById(requestId);
@@ -334,7 +333,7 @@ export const accessApprovalRequestServiceFactory = ({
      throw new NotFoundError({ message: `Secret approval request with ID '${requestId}' not found` });
    }

-    const { policy } = accessApprovalRequest;
+    const { policy, environment } = accessApprovalRequest;
    if (policy.deletedAt) {
      throw new BadRequestError({
        message: "The policy associated with this access request has been deleted."
@@ -354,14 +353,15 @@ export const accessApprovalRequestServiceFactory = ({
      throw new ForbiddenRequestError({ message: "You are not a member of this project" });
    }

-    if (
-      !policy.allowedSelfApprovals &&
-      actorId === accessApprovalRequest.requestedByUserId &&
-      !(
-        policy.enforcementLevel === EnforcementLevel.Soft &&
-        permission.can(ProjectPermissionApprovalActions.AllowAccessBypass, ProjectPermissionSub.SecretApproval)
-      )
-    ) {
+    const isSelfApproval = actorId === accessApprovalRequest.requestedByUserId;
+    const isSoftEnforcement = policy.enforcementLevel === EnforcementLevel.Soft;
+    const canBypassApproval = permission.can(
+      ProjectPermissionApprovalActions.AllowAccessBypass,
+      ProjectPermissionSub.SecretApproval
+    );
+    const cannotBypassUnderSoftEnforcement = !(isSoftEnforcement && canBypassApproval);
+
+    if (!policy.allowedSelfApprovals && isSelfApproval && cannotBypassUnderSoftEnforcement) {
      throw new BadRequestError({
        message: "Failed to review access approval request. Users are not authorized to review their own request."
      });
@@ -508,7 +508,7 @@ export const accessApprovalRequestServiceFactory = ({
                  requesterEmail: actingUser.email,
                  bypassReason: bypassReason || "No reason provided",
                  secretPath: policy.secretPath || "/",
-                  environment: envName || "Unknown",
+                  environment,
                  approvalUrl: `${cfg.SITE_URL}/secret-manager/${project.id}/approval`,
                  requestType: "access"
                },
--- a/frontend/src/hooks/api/accessApproval/mutation.tsx
+++ b/frontend/src/hooks/api/accessApproval/mutation.tsx
@@ -129,30 +129,27 @@ export const useReviewAccessRequest = () => {
      requestId: string;
      status: "approved" | "rejected";
      projectSlug: string;
-      envName?: string;
      envSlug?: string;
      requestedBy?: string;
      bypassReason?: string;
    }
  >({
-    mutationFn: async ({ requestId, status, envName, bypassReason }) => {
+    mutationFn: async ({ requestId, status, bypassReason }) => {
      const { data } = await apiRequest.post(
        `/api/v1/access-approvals/requests/${requestId}/review`,
        {
          status,
-          envName,
          bypassReason
        }
      );
      return data;
    },
-    onSuccess: (_, { projectSlug, envSlug, requestedBy, envName, bypassReason }) => {
+    onSuccess: (_, { projectSlug, envSlug, requestedBy, bypassReason }) => {
      queryClient.invalidateQueries({
        queryKey: accessApprovalKeys.getAccessApprovalRequests(
          projectSlug,
          envSlug,
          requestedBy,
-          envName,
          bypassReason
        )
      });
--- a/frontend/src/hooks/api/accessApproval/queries.tsx
+++ b/frontend/src/hooks/api/accessApproval/queries.tsx
@@ -23,13 +23,8 @@ export const accessApprovalKeys = {
    projectSlug: string,
    envSlug?: string,
    requestedBy?: string,
-    envName?: string,
    bypassReason?: string
-  ) =>
-    [
-      { projectSlug, envSlug, requestedBy, envName, bypassReason },
-      "access-approvals-requests"
-    ] as const,
+  ) => [{ projectSlug, envSlug, requestedBy, bypassReason }, "access-approvals-requests"] as const,
  getAccessApprovalRequestCount: (projectSlug: string) =>
    [{ projectSlug }, "access-approval-request-count"] as const
 };
--- a/frontend/src/pages/secret-manager/SecretApprovalsPage/components/AccessApprovalRequest/AccessApprovalRequest.tsx
+++ b/frontend/src/pages/secret-manager/SecretApprovalsPage/components/AccessApprovalRequest/AccessApprovalRequest.tsx
@@ -1,6 +1,6 @@
 /* eslint-disable no-nested-ternary */
 /* eslint-disable react/jsx-no-useless-fragment */
-import { useMemo, useState } from "react";
+import { useCallback, useMemo, useState } from "react";
 import {
  faCheck,
  faCheckCircle,
@@ -150,56 +150,105 @@ export const AccessApprovalRequest = ({
    return requests;
  }, [requests, statusFilter, requestedByFilter, envFilter]);

-  const generateRequestDetails = (request: TAccessApprovalRequest) => {
-    const isReviewedByUser = request.reviewers.findIndex(({ member }) => member === user.id) !== -1;
-    const isRejectedByAnyone = request.reviewers.some(
-      ({ status }) => status === ApprovalStatus.REJECTED
-    );
-    const isApprover = request.policy.approvers.indexOf(user.id || "") !== -1;
-    const isAccepted = request.isApproved;
-    const isSoftEnforcement = request.policy.enforcementLevel === EnforcementLevel.Soft;
-    const isRequestedByCurrentUser = request.requestedByUserId === user.id;
-    const isSelfApproveAllowed = request.policy.allowedSelfApprovals;
-    const userReviewStatus = request.reviewers.find(({ member }) => member === user.id)?.status;
+  const generateRequestDetails = useCallback(
+    (request: TAccessApprovalRequest) => {
+      const isReviewedByUser =
+        request.reviewers.findIndex(({ member }) => member === user.id) !== -1;
+      const isRejectedByAnyone = request.reviewers.some(
+        ({ status }) => status === ApprovalStatus.REJECTED
+      );
+      const isApprover = request.policy.approvers.indexOf(user.id || "") !== -1;
+      const isAccepted = request.isApproved;
+      const isSoftEnforcement = request.policy.enforcementLevel === EnforcementLevel.Soft;
+      const isRequestedByCurrentUser = request.requestedByUserId === user.id;
+      const isSelfApproveAllowed = request.policy.allowedSelfApprovals;
+      const userReviewStatus = request.reviewers.find(({ member }) => member === user.id)?.status;

-    let displayData: { label: string; type: "primary" | "danger" | "success" } = {
-      label: "",
-      type: "primary"
-    };
-
-    const isExpired =
-      request.privilege &&
-      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" };
-    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"
-      };
-    } else if (!isReviewedByUser)
-      displayData = {
-        label: "Review Required",
+      let displayData: { label: string; type: "primary" | "danger" | "success" } = {
+        label: "",
        type: "primary"
      };

-    return {
-      displayData,
-      isReviewedByUser,
-      isRejectedByAnyone,
-      isApprover,
-      userReviewStatus,
-      isAccepted,
-      isSoftEnforcement,
-      isRequestedByCurrentUser,
-      isSelfApproveAllowed
-    };
-  };
+      const isExpired =
+        request.privilege &&
+        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" };
+      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"
+        };
+      } else if (!isReviewedByUser)
+        displayData = {
+          label: "Review Required",
+          type: "primary"
+        };
+
+      return {
+        displayData,
+        isReviewedByUser,
+        isRejectedByAnyone,
+        isApprover,
+        userReviewStatus,
+        isAccepted,
+        isSoftEnforcement,
+        isRequestedByCurrentUser,
+        isSelfApproveAllowed
+      };
+    },
+    [user]
+  );
+
+  const handleSelectRequest = useCallback(
+    (request: TAccessApprovalRequest) => {
+      const details = generateRequestDetails(request);
+
+      // Whether the request has already been approved / rejected / reviewed
+      const isInactive =
+        details.isAccepted || details.isReviewedByUser || 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 || details.isRequestedByCurrentUser) {
+        setSelectedRequest({
+          ...request,
+          user:
+            details.isRequestedByCurrentUser || !membersGroupById?.[request.requestedByUserId].user
+              ? user
+              : membersGroupById?.[request.requestedByUserId].user,
+          isRequestedByCurrentUser: details.isRequestedByCurrentUser,
+          isSelfApproveAllowed: details.isSelfApproveAllowed,
+          isApprover: details.isApprover
+        });
+      }
+
+      handlePopUpOpen("reviewRequest");
+    },
+    [
+      generateRequestDetails,
+      canBypassApprovalPermission,
+      membersGroupById,
+      user,
+      setSelectedRequest,
+      handlePopUpOpen
+    ]
+  );

  return (
    <div>
@@ -351,84 +400,10 @@ export const AccessApprovalRequest = ({
                    className="flex w-full cursor-pointer px-8 py-4 hover:bg-mineshaft-700 aria-disabled:opacity-80"
                    role="button"
                    tabIndex={0}
-                    onClick={() => {
-                      // Whether the request has already been approved / rejected / reviewed
-                      const isInactive =
-                        details.isAccepted ||
-                        details.isReviewedByUser ||
-                        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 ||
-                        details.isRequestedByCurrentUser
-                      ) {
-                        setSelectedRequest({
-                          ...request,
-                          user:
-                            details.isRequestedByCurrentUser ||
-                            !membersGroupById?.[request.requestedByUserId].user
-                              ? user
-                              : membersGroupById?.[request.requestedByUserId].user,
-                          isRequestedByCurrentUser: details.isRequestedByCurrentUser,
-                          isSelfApproveAllowed: details.isSelfApproveAllowed,
-                          isApprover: details.isApprover
-                        });
-                      }
-
-                      handlePopUpOpen("reviewRequest");
-                    }}
+                    onClick={() => handleSelectRequest(request)}
                    onKeyDown={(evt) => {
-                      // Whether the request has already been approved / rejected / reviewed
-                      const isInactive =
-                        details.isAccepted ||
-                        details.isReviewedByUser ||
-                        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 (
-                          membersGroupById?.[request.requestedByUserId].user ||
-                          details.isRequestedByCurrentUser
-                        ) {
-                          setSelectedRequest({
-                            ...request,
-                            user:
-                              details.isRequestedByCurrentUser ||
-                              !membersGroupById?.[request.requestedByUserId].user
-                                ? user
-                                : membersGroupById?.[request.requestedByUserId].user,
-                            isRequestedByCurrentUser: details.isRequestedByCurrentUser,
-                            isSelfApproveAllowed: details.isSelfApproveAllowed,
-                            isApprover: details.isApprover
-                          });
-                        }
-
-                        handlePopUpOpen("reviewRequest");
+                        handleSelectRequest(request);
                      }
                    }}
                  >
--- a/frontend/src/pages/secret-manager/SecretApprovalsPage/components/AccessApprovalRequest/components/ReviewAccessModal.tsx
+++ b/frontend/src/pages/secret-manager/SecretApprovalsPage/components/AccessApprovalRequest/components/ReviewAccessModal.tsx
@@ -104,7 +104,6 @@ export const ReviewAccessRequestModal = ({
          requestId: request.id,
          status,
          projectSlug,
-          envName: accessDetails.env,
          envSlug: selectedEnvSlug,
          requestedBy: selectedRequester,
          bypassReason: bypassApproval ? bypassReason : undefined
@@ -129,7 +128,6 @@ export const ReviewAccessRequestModal = ({
      bypassReason,
      reviewAccessRequest,
      request,
-      accessDetails.env,
      selectedEnvSlug,
      selectedRequester,
      onOpenChange