Skip to content

feat: deploy approval UX and four-eyes principle#47

Merged
TerrifiedBug merged 5 commits intomainfrom
feat/deploy-approval-ux-improvements
Mar 7, 2026
Merged

feat: deploy approval UX and four-eyes principle#47
TerrifiedBug merged 5 commits intomainfrom
feat/deploy-approval-ux-improvements

Conversation

@TerrifiedBug
Copy link
Copy Markdown
Owner

Summary

  • Moved deploy approval toggle out of the edit environment form into a dedicated Deploy Settings section with a live-save switch (greyed out for non-admins)
  • Implemented four-eyes principle: editors can now approve/reject other editors' deploy requests (self-approval remains blocked)
  • Added admin warning banner in the deploy dialog when deploying directly on an approval-required environment

Test plan

  • Verify the "Deploy Settings" section appears on the environment detail page with the approval toggle
  • Confirm the toggle is disabled/greyed for EDITOR users and functional for ADMINs
  • As an editor, submit a deploy request on an approval-required environment
  • As a different editor, verify you can approve/reject the pending request
  • As the requesting editor, verify you cannot approve your own request
  • As an admin, verify the amber warning banner appears when deploying to an approval-required environment
  • Verify admins can still deploy directly without approval

- Fleet: collapse node labels into a compact count button with popover
- Fleet: replace native confirm() with styled ConfirmDialog for maintenance mode
- Dashboard: add drag, resize, and reorder support for custom view panels using react-grid-layout
- Pipelines: center the SLI health dot in the Health column
- Profile: display user role and super admin status in personal info
…types stub

- Add queryClient.invalidateQueries for dashboard.listViews on
  updateView mutation success so remounting loads the persisted layout
- Remove @types/react-grid-layout (deprecated stub, v2 ships own types)
- Use a ref (filtersRef) to hold latest filter values so the debounce
  timer reads current pipelineIds/nodeIds when it fires, not the values
  captured at scheduling time
- Clear pending debounce timer on component unmount to prevent stale
  mutations after navigation
- Move filtersRef.current assignment into a useEffect to satisfy
  react-hooks/refs lint rule (no ref writes during render)
- Wrap panels cast in useMemo to stabilize the dependency for
  downstream useMemo hooks
- Move deploy approval toggle from edit form to dedicated "Deploy Settings"
  section with live-save toggle, greyed out for non-admins
- Allow editors to approve/reject other editors' deploy requests (four-eyes
  principle) — self-approval still blocked
- Show admin warning banner on deploy dialog when bypassing approval
- Include configYaml in pending requests for editor reviewers
@github-actions github-actions bot added dependencies Pull requests that update a dependency file feature labels Mar 7, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 7, 2026

Greptile Summary

This PR introduces the four-eyes deploy approval workflow and relocates the approval toggle into a dedicated Deploy Settings section. The majority of the changes are well-implemented — the self-approval guard in approveDeployRequest is correctly enforced server-side, the admin warning banner is wired up properly, and the dashboard layout refactor is solid. However, there are two issues in src/server/routers/deploy.ts that need to be addressed before merging:

  • configYaml now exposed to EDITOR role — the field was previously restricted to admins because it contains resolved, plaintext secrets injected at deploy-request time. The PR widens access to any EDITOR who can review, leaking secrets to a broader role.
  • rejectDeployRequest lacks the self-rejection guardapproveDeployRequest correctly throws FORBIDDEN when a user tries to approve their own request, but the parallel rejectDeployRequest procedure has no equivalent check. An editor can reject their own pending request directly via the API, bypassing both the cancelDeployRequest path and the audit trail intent.

Confidence Score: 2/5

  • Not safe to merge — one issue leaks decrypted secrets to a broader role and one leaves the four-eyes workflow bypassable server-side.
  • Both issues are in the same security-critical file (deploy.ts). The configYaml exposure is a direct secret leak to EDITOR-role users; the missing self-rejection guard means the four-eyes invariant can be circumvented via direct API call. All other changed files are clean.
  • src/server/routers/deploy.ts — specifically the listPendingRequests configYaml guard and the rejectDeployRequest mutation.

Important Files Changed

Filename Overview
src/server/routers/deploy.ts Four-eyes principle added correctly (self-approval blocked server-side), but two issues: configYaml with decrypted secrets is now exposed to EDITOR role, and rejectDeployRequest lacks the self-rejection guard that approveDeployRequest has.
src/components/flow/deploy-dialog.tsx Admin warning banner and four-eyes UI (isOwnRequest / canReview) implemented correctly; client-side self-review guard is backed by the server-side check in approveDeployRequest.
src/app/(dashboard)/environments/[id]/page.tsx Deploy approval toggle moved to a dedicated "Deploy Settings" section with a live-save switch; disabled for non-admins. Clean refactor with no correctness issues.
src/components/dashboard/custom-view.tsx Dashboard panels migrated to react-grid-layout with drag/resize/persist. Layout saved via debounced mutation using a stable ref pattern to avoid stale closure issues.
src/server/routers/dashboard.ts Layout array added to createView and updateView filters schema with proper Zod validation.
src/app/(dashboard)/fleet/page.tsx Native confirm() replaced with a proper ConfirmDialog for maintenance mode entry; labels collapsed into a popover. No issues found.

Sequence Diagram

sequenceDiagram
    participant EditorA as Editor A (Requester)
    participant EditorB as Editor B (Reviewer)
    participant Admin as Admin
    participant Server as tRPC Server
    participant DB as Database

    EditorA->>Server: deploy.agent (requiresApproval=true)
    Server->>DB: Create DeployRequest (PENDING)
    Server-->>EditorA: { requestId }

    EditorB->>Server: deploy.listPendingRequests
    Server->>DB: findMany (PENDING)
    DB-->>Server: requests (configYaml excluded for editors ⚠️ currently included)
    Server-->>EditorB: pending requests

    alt Four-eyes: EditorB reviews
        EditorB->>Server: deploy.approveDeployRequest
        Server->>DB: check requestedById ≠ reviewerId
        Server->>DB: updateMany status=APPROVED (atomic)
        Server->>Server: deployAgent(configYaml snapshot)
        Server-->>EditorB: deploy result
    else EditorB rejects
        EditorB->>Server: deploy.rejectDeployRequest
        Note over Server: ⚠️ No self-rejection guard
        Server->>DB: updateMany status=REJECTED (atomic)
        Server-->>EditorB: { rejected: true }
    end

    alt Admin bypass
        Admin->>Server: deploy.agent (direct, no approval needed)
        Server-->>Admin: deploy result (with amber warning shown in UI)
    end
Loading

Comments Outside Diff (2)

  1. src/server/routers/deploy.ts, line 1330-1333 (link)

    Decrypted secrets now exposed to EDITOR role

    The original comment explicitly stated: configYaml only included for admins — contains decrypted secrets. This PR changes the guard from isAdmin (ADMIN or SUPER_ADMIN) to canReview (ADMIN or EDITOR), meaning every EDITOR with review access can now retrieve the full resolved config YAML — including any plaintext secrets, certificates, and tokens that were injected by secret-resolver.ts at deploy-request time.

    Editors reviewing a deploy request don't need the raw YAML to make an approve/reject decision — they can consult the pipeline graph in the editor instead. The configYaml field should remain restricted to ADMIN/SUPER_ADMIN, or a redacted preview (with secret values masked) should be produced specifically for the reviewer view.

    Rule Used: ## Security & Cryptography Review Rules

    When revi... (source)

  2. src/server/routers/deploy.ts, line 1348-1404 (link)

    Missing self-rejection guard in rejectDeployRequest

    approveDeployRequest correctly enforces the four-eyes principle server-side:

    if (request.requestedById === ctx.session.user.id) {
      throw new TRPCError({ code: "FORBIDDEN", message: "Cannot approve your own deploy request" });
    }

    rejectDeployRequest has no equivalent check. Now that EDITOR can call rejectDeployRequest, the original requester can reject their own pending request directly through the API (bypassing the frontend isOwnRequest guard), effectively cancelling it without using the dedicated cancelDeployRequest path. This circumvents the four-eyes workflow and also confuses the audit trail (an "EDITOR-rejected" record when the actor is the original requester).

    Add the same guard immediately after the NOT_FOUND check:

    if (request.requestedById === ctx.session.user.id) {
      throw new TRPCError({ code: "FORBIDDEN", message: "Cannot reject your own deploy request" });
    }

    Rule Used: ## Security & Cryptography Review Rules

    When revi... (source)

Last reviewed commit: b0603c9

@TerrifiedBug TerrifiedBug merged commit d91d33c into main Mar 7, 2026
12 checks passed
@TerrifiedBug TerrifiedBug deleted the feat/deploy-approval-ux-improvements branch March 8, 2026 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant