feat: require explicit admin override for self-approval; fix :run PID file (#216)

## Summary

- Admins reviewing their own pushes now follow the same path as regular
users — `ROLE_ADMIN` alone no longer bypasses the self-approval identity
check. Admins either self-certify (via `ROLE_SELF_CERTIFY` + repo
permission, same as any user) or must explicitly activate an **Enable
admin override** toggle in the dashboard UI for break-glass situations.
- The admin override toggle is hidden when self-certify is already
available, so the aggressive red warning only surfaces when it is
actually needed.
- `selfApproval=true` is only recorded in the audit log when the
override flag is explicitly used — admin self-certify approvals are no
longer incorrectly flagged.
- Fix `:run` PID file: `applicationDefaultJvmArgs` only applies to
generated distribution scripts, not the Gradle `JavaExec` run task.
Added `-Dgitproxyjava.pidfile` explicitly to `tasks.named('run')` in
both `build.gradle` files so `./gradlew :stop` works correctly after
`ctrl+c`.
- Local dev config: grant `thomas-cooper` `ROLE_SELF_CERTIFY` so the
self-certify path is exercisable without needing the admin override.

## Test plan

- [ ] Log in as an admin user who pushed a commit — confirm blue
self-certify banner appears (not red override warning)
- [ ] Log in as an admin without self-certify permission — confirm amber
"not permitted" banner and "Enable admin override" link
- [ ] Click "Enable admin override" — confirm red warning appears and
approve button becomes active
- [ ] Approve via override — confirm audit log entry has `selfApproval:
true`
- [ ] Approve via self-certify — confirm audit log entry has
`selfApproval: false`
- [ ] Admin approving someone else's push — confirm no change in
behaviour (bypass still applies)
- [ ] `./gradlew :git-proxy-java-dashboard:run` then `ctrl+c` then
`./gradlew :git-proxy-java-dashboard:stop` — confirm "Stopping … (PID:
…)" rather than "No PID file found"
- [ ] `./gradlew :git-proxy-java-dashboard:test` passes

Author: coopernetes

git-proxy-java-dashboard/build.gradle

@@ -116,6 +116,7 @@ tasks.named('run') {
     // Default keeps the dev experience simple; override in shell for a stricter key.
     environment 'GITPROXY_API_KEY', System.getenv('GITPROXY_API_KEY') ?: 'change-me-in-production'
     jvmArgs "-Dlog4j2.configurationFile=${rootDir}/git-proxy-java-server/src/main/resources/log4j2-local.xml"
+    jvmArgs "-Dgitproxyjava.pidfile=${buildDir}/git-proxy-java.pid"
 }
 
 tasks.register('stop') {

git-proxy-java-dashboard/frontend/src/api.ts

@@ -98,6 +98,7 @@ export async function approvePush(
     reviewerEmail: string
     reason: string
     attestations?: Record<string, string>
+    adminOverride?: boolean
   },
 ) {
   const res = await apiFetch(`/api/push/${id}/authorise`, {

git-proxy-java-dashboard/frontend/src/pages/PushDetail.tsx

@@ -449,6 +449,7 @@ export function PushDetail({ currentUser }: PushDetailProps) {
   const [canceling, setCanceling] = useState(false)
   const [attestationQuestions, setAttestationQuestions] = useState<AttestationQuestion[]>([])
   const [attestationAnswers, setAttestationAnswers] = useState<Record<string, string>>({})
+  const [adminOverrideEnabled, setAdminOverrideEnabled] = useState(false)
 
   // Diff state — loaded separately so it never blocks the main page render
   const [diffContent, setDiffContent] = useState<string | null>(null)
@@ -470,6 +471,7 @@ export function PushDetail({ currentUser }: PushDetailProps) {
     setActionError('')
     setOpenSteps({})
     setAttestationAnswers({})
+    setAdminOverrideEnabled(false)
     setDiffContent(null)
     setDiffLines(0)
     try {
@@ -550,6 +552,7 @@ export function PushDetail({ currentUser }: PushDetailProps) {
         reviewerEmail: currentUser?.emails[0]?.email ?? '',
         reason: reviewReason,
         attestations: attestationQuestions.length > 0 ? attestationAnswers : undefined,
+        adminOverride: adminOverrideEnabled || undefined,
       })
       await load(record.id)
     } catch (e) {
@@ -887,15 +890,15 @@ export function PushDetail({ currentUser }: PushDetailProps) {
               const isAdmin = currentUser?.authorities?.includes('ROLE_ADMIN') ?? false
               // canCurrentUserSelfCertify is computed server-side and requires BOTH the
               // ROLE_SELF_CERTIFY authority AND a SELF_CERTIFY repo permission row for this push's
-              // path. A user with the role but no per-repo permission gets `false` here, so the
-              // self-certify banner stays hidden and the approve button is gated behind the
-              // standard self-review block.
+              // path. A user with the role but no per-repo permission gets `false` here.
               const canSelfCertify = record.canCurrentUserSelfCertify ?? false
               const isPusher =
                 !!currentUser?.username &&
                 !!record.resolvedUser &&
                 currentUser.username === record.resolvedUser
-              const isSelfReview = isPusher && !isAdmin && !canSelfCertify
+              // Admins are treated the same as regular users for their own pushes: they need
+              // self-certify permissions or must explicitly activate the admin override.
+              const isSelfReview = isPusher && !canSelfCertify
               const canCancel = isAdmin || isPusher
               const attestationsComplete = attestationQuestions
                 .filter((q) => q.required)
@@ -924,7 +927,7 @@ export function PushDetail({ currentUser }: PushDetailProps) {
                       </span>
                     )}
                   </div>
-                  {isSelfReview && (
+                  {isSelfReview && !adminOverrideEnabled && (
                     <div className="flex gap-2 text-sm text-amber-800 bg-amber-50 border border-amber-200 rounded px-3 py-2 mb-3">
                       <span>⚠</span>
                       <span>
@@ -933,16 +936,24 @@ export function PushDetail({ currentUser }: PushDetailProps) {
                       </span>
                     </div>
                   )}
-                  {canSelfCertify && isPusher && !isAdmin && (
+                  {canSelfCertify && isPusher && (
                     <div className="flex gap-2 text-sm text-blue-800 bg-blue-50 border border-blue-200 rounded px-3 py-2 mb-3">
-                      <span>ℹ</span>
+                      <span>{'\u2139\uFE0F'}</span>
                       <span>
                         You are self-certifying your own push. This approval will be permanently
                         recorded in the audit log.
                       </span>
                     </div>
                   )}
-                  {isAdmin && isPusher && (
+                  {isAdmin && isPusher && !canSelfCertify && !adminOverrideEnabled && (
+                    <button
+                      onClick={() => setAdminOverrideEnabled(true)}
+                      className="w-full text-left text-xs text-gray-500 hover:text-gray-700 underline mb-3"
+                    >
+                      Enable admin override
+                    </button>
+                  )}
+                  {isAdmin && isPusher && adminOverrideEnabled && (
                     <div className="flex gap-2 text-sm text-red-800 bg-red-50 border border-red-200 rounded px-3 py-2 mb-3">
                       <span>⚠</span>
                       <span>
@@ -962,7 +973,7 @@ export function PushDetail({ currentUser }: PushDetailProps) {
                           key={q.id}
                           question={q}
                           value={attestationAnswers[q.id] ?? ''}
-                          disabled={isSelfReview}
+                          disabled={isSelfReview && !adminOverrideEnabled}
                           onChange={(val) =>
                             setAttestationAnswers((prev) => ({ ...prev, [q.id]: val }))
                           }
@@ -977,15 +988,15 @@ export function PushDetail({ currentUser }: PushDetailProps) {
                     placeholder={
                       'Reason (required for both approve and reject)\nDescribe the basis for your decision...'
                     }
-                    disabled={isSelfReview}
+                    disabled={isSelfReview && !adminOverrideEnabled}
                     className="w-full border border-gray-300 rounded px-3 py-2 text-sm mb-3 resize-none focus:outline-none focus:ring-2 focus:ring-blue-300 disabled:bg-gray-50 disabled:text-gray-400"
                   />
                   {actionError && <div className="text-red-600 text-sm mb-3">{actionError}</div>}
                   <div className="flex gap-3">
                     <button
                       onClick={handleApprove}
                       disabled={
-                        isSelfReview ||
+                        (isSelfReview && !adminOverrideEnabled) ||
                         saving ||
                         canceling ||
                         !reviewReason.trim() ||
@@ -997,7 +1008,12 @@ export function PushDetail({ currentUser }: PushDetailProps) {
                     </button>
                     <button
                       onClick={handleReject}
-                      disabled={isSelfReview || saving || canceling || !reviewReason.trim()}
+                      disabled={
+                        (isSelfReview && !adminOverrideEnabled) ||
+                        saving ||
+                        canceling ||
+                        !reviewReason.trim()
+                      }
                       className="px-4 py-2 text-sm font-medium rounded bg-red-600 text-white hover:bg-red-700 disabled:opacity-40 disabled:cursor-not-allowed disabled:bg-gray-400"
                     >
                       ✗ Reject

git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/PushController.java

@@ -138,10 +138,11 @@ public ResponseEntity<PushRecord> getById(@PathVariable String id) {
     }
 
     /**
-     * Computes whether the currently authenticated user is permitted to self-approve this specific push. Mirrors the
-     * server-side enforcement in {@link #checkReviewerIdentity}: the user must be the resolved pusher, hold the
-     * {@code ROLE_SELF_CERTIFY} authority (capability gate), and have a matching {@code SELF_CERTIFY} repo permission
-     * row (per-repo entitlement). Admins do not need self-certify — they can approve anything via the regular path.
+     * Computes whether the currently authenticated user is permitted to self-approve this specific push via the
+     * self-certify path. The user must be the resolved pusher, hold the {@code ROLE_SELF_CERTIFY} authority (capability
+     * gate), and have a matching {@code SELF_CERTIFY} repo permission row (per-repo entitlement). Applies to both
+     * regular users and admins — admins who hold self-certify permissions follow this path rather than the admin
+     * override path.
      */
     private boolean computeCanCurrentUserSelfCertify(PushRecord record) {
         Authentication auth = SecurityContextHolder.getContext().getAuthentication();
@@ -190,7 +191,11 @@ private static void stripDiffContent(PushRecord record) {
      * question answers keyed by question ID.
      */
     public record ApproveBody(
-            String reviewerUsername, String reviewerEmail, String reason, Map<String, String> attestations) {}
+            String reviewerUsername,
+            String reviewerEmail,
+            String reason,
+            Map<String, String> attestations,
+            boolean adminOverride) {}
 
     /**
      * Approve a push. Body: { "reviewerUsername": "...", "reviewerEmail": "...", "reason": "...", "attestations": {
@@ -206,7 +211,7 @@ public ResponseEntity<?> approve(@PathVariable String id, @RequestBody ApproveBo
                         return ResponseEntity.badRequest()
                                 .body(Map.of("error", "Push is not in PENDING status: " + record.getStatus()));
                     }
-                    ResponseEntity<?> identityError = checkReviewerIdentity(record);
+                    ResponseEntity<?> identityError = checkReviewerIdentity(record, body.adminOverride());
                     if (identityError != null) return identityError;
 
                     // Validate required attestation questions are answered
@@ -220,7 +225,7 @@ public ResponseEntity<?> approve(@PathVariable String id, @RequestBody ApproveBo
                             .reviewerUsername(resolveReviewerFromApproveBody(body, auth))
                             .reviewerEmail(body.reviewerEmail())
                             .reason(body.reason())
-                            .selfApproval(isSelfApproval(record, auth))
+                            .selfApproval(isSelfApproval(record, auth, body.adminOverride()))
                             .answers(body.attestations())
                             .build();
                     var updated = pushStore.approve(id, attestation);
@@ -277,7 +282,7 @@ public ResponseEntity<?> reject(@PathVariable String id, @RequestBody Map<String
                         return ResponseEntity.badRequest()
                                 .body(Map.of("error", "Push is not in PENDING status: " + record.getStatus()));
                     }
-                    ResponseEntity<?> identityError = checkReviewerIdentity(record);
+                    ResponseEntity<?> identityError = checkReviewerIdentity(record, true);
                     if (identityError != null) return identityError;
                     Authentication auth = SecurityContextHolder.getContext().getAuthentication();
                     var attestation = Attestation.builder()
@@ -286,7 +291,7 @@ public ResponseEntity<?> reject(@PathVariable String id, @RequestBody Map<String
                             .reviewerUsername(resolveReviewer(body))
                             .reviewerEmail(body.get("reviewerEmail"))
                             .reason(reason)
-                            .selfApproval(isSelfApproval(record, auth))
+                            .selfApproval(isSelfApproval(record, auth, true))
                             .build();
                     var updated = pushStore.reject(id, attestation);
                     return ResponseEntity.ok(updated);
@@ -298,25 +303,37 @@ public ResponseEntity<?> reject(@PathVariable String id, @RequestBody Map<String
      * Validates that the current session user may review the given push record:
      *
      * <ol>
-     *   <li>ROLE_ADMIN bypasses all identity checks — admins may approve/reject any push.
-     *   <li>The pusher must have been resolved to a proxy user — if not, we cannot guarantee identity.
-     *   <li>Self-review: allowed only when the reviewer has both {@code ROLE_SELF_CERTIFY} (the capability, attested by
-     *       the org's IdP/IAM via {@code auth.role-mappings} or the local {@code users[].roles} block) and a
+     *   <li>ROLE_ADMIN reviewing someone else's push: always permitted.
+     *   <li>ROLE_ADMIN self-review with {@code adminOverride=true}: permitted; recorded as an admin override in the
+     *       audit log.
+     *   <li>Self-review (admin or otherwise) without override: allowed only when the reviewer has both
+     *       {@code ROLE_SELF_CERTIFY} (the capability, attested by the org's IdP/IAM via {@code auth.role-mappings} or
+     *       the local {@code users[].roles} block) and a
      *       {@link org.finos.gitproxy.permission.RepoPermission.Operations#SELF_CERTIFY} repo permission entry for this
-     *       specific repository (the per-repo entitlement). Both must be present.
+     *       specific repository. Both must be present.
+     *   <li>The pusher must have been resolved to a proxy user — if not, we cannot guarantee identity.
      *   <li>Non-self reviewer: by default any authenticated user may review. When
      *       {@code server.require-review-permission: true}, the user must have a REVIEW (or PUSH_AND_REVIEW) permission
      *       for the repo.
      * </ol>
      *
+     * @param adminOverride {@code true} when the caller has explicitly activated the admin override on the approve
+     *     endpoint; ignored for non-admin users and for admins reviewing someone else's push
      * @return a 403 response if the check fails, {@code null} if the reviewer is permitted to proceed
      */
-    private ResponseEntity<?> checkReviewerIdentity(PushRecord record) {
+    private ResponseEntity<?> checkReviewerIdentity(PushRecord record, boolean adminOverride) {
         Authentication auth = SecurityContextHolder.getContext().getAuthentication();
-        if (isAdmin(auth)) return null;
-
         String reviewer = auth != null ? auth.getName() : null;
         String pusherProxyUser = record.getResolvedUser();
+        boolean isSelfReview = pusherProxyUser != null && pusherProxyUser.equals(reviewer);
+
+        // Admins reviewing someone else's push always bypass identity checks.
+        if (isAdmin(auth) && !isSelfReview) return null;
+
+        // Admins reviewing their own push: permitted only when the override flag is explicit.
+        // Without it they fall through to the same self-certify check as a regular user, so
+        // having ROLE_ADMIN alone does not bypass the two-gate self-approval requirement.
+        if (isAdmin(auth) && adminOverride) return null;
 
         if (pusherProxyUser == null) {
             return ResponseEntity.status(HttpStatus.FORBIDDEN)
@@ -326,11 +343,9 @@ private ResponseEntity<?> checkReviewerIdentity(PushRecord record) {
                                     "Pusher identity has not been resolved to a proxy user; approval requires verified identity"));
         }
 
-        if (pusherProxyUser.equals(reviewer)) {
+        if (isSelfReview) {
             // Self-review requires two independent checks:
             // 1. ROLE_SELF_CERTIFY — the capability, granted via auth.role-mappings or users[].roles in config.
-            //    This is the pre-requisite gate: it must be attested by the org's IdP/IAM before any per-repo
-            //    entitlement can take effect.
             // 2. A SELF_CERTIFY repo permission entry for this specific repo — the per-repo entitlement.
             boolean hasSelfCertifyRole = auth != null
                     && auth.getAuthorities().stream().anyMatch(a -> "ROLE_SELF_CERTIFY".equals(a.getAuthority()));
@@ -400,12 +415,10 @@ private static boolean isAdmin(Authentication auth) {
         return auth != null && auth.getAuthorities().stream().anyMatch(a -> "ROLE_ADMIN".equals(a.getAuthority()));
     }
 
-    private static boolean isSelfApproval(PushRecord record, Authentication auth) {
-        if (!isAdmin(auth)) {
-            // Non-admin self-reviews are permitted only via an explicit SELF_CERTIFY grant —
-            // that is expected behaviour, not an admin override.
-            return false;
-        }
+    private static boolean isSelfApproval(PushRecord record, Authentication auth, boolean adminOverride) {
+        // Only flag as an admin override when the admin explicitly activated it.
+        // Admin self-reviews via SELF_CERTIFY and all non-admin self-reviews are expected behaviour, not overrides.
+        if (!isAdmin(auth) || !adminOverride) return false;
         String pusher = record.getResolvedUser();
         String reviewer = auth != null ? auth.getName() : null;
         return pusher != null && pusher.equals(reviewer);

git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/PushControllerTest.java

@@ -52,7 +52,11 @@ class PushControllerTest {
 
     /** Empty approve body — no attestations, mirrors previous Map.of() usage. */
     private static PushController.ApproveBody approveBody() {
-        return new PushController.ApproveBody(null, null, null, null);
+        return new PushController.ApproveBody(null, null, null, null, false);
+    }
+
+    private static PushController.ApproveBody approveBodyWithAdminOverride() {
+        return new PushController.ApproveBody(null, null, null, null, true);
     }
 
     @AfterEach
@@ -288,25 +292,47 @@ void unresolvedPusher_returns403() {
         }
 
         @Test
-        void admin_bypassesIdentityChecks_returns200() {
+        void admin_selfApproval_withoutOverride_returns403() {
+            when(pushStore.findById("p1")).thenReturn(Optional.of(blockedPush("p1", "alice")));
+            loginAs("alice", true); // same user as pusher, no adminOverride flag
+
+            assertEquals(
+                    HttpStatus.FORBIDDEN,
+                    controller.approve("p1", approveBody()).getStatusCode());
+        }
+
+        @Test
+        void admin_selfApproval_withOverride_returns200() {
             when(pushStore.findById("p1")).thenReturn(Optional.of(blockedPush("p1", "alice")));
             when(pushStore.approve(eq("p1"), any())).thenReturn(approvedPush("p1"));
-            loginAs("alice", true); // same user as pusher — admin bypass
+            loginAs("alice", true);
 
-            assertEquals(HttpStatus.OK, controller.approve("p1", approveBody()).getStatusCode());
+            assertEquals(
+                    HttpStatus.OK,
+                    controller.approve("p1", approveBodyWithAdminOverride()).getStatusCode());
         }
 
         @Test
-        void admin_selfApproval_flaggedInAttestation() {
+        void admin_selfApproval_withOverride_flaggedInAttestation() {
             when(pushStore.findById("p1")).thenReturn(Optional.of(blockedPush("p1", "alice")));
             when(pushStore.approve(eq("p1"), any())).thenReturn(approvedPush("p1"));
             loginAs("alice", true);
 
-            controller.approve("p1", approveBody());
+            controller.approve("p1", approveBodyWithAdminOverride());
 
             verify(pushStore).approve(eq("p1"), argThat(a -> a.isSelfApproval()));
         }
 
+        @Test
+        void admin_selfApproval_withoutOverride_notFlaggedAsAdminOverride() {
+            when(pushStore.findById("p1")).thenReturn(Optional.of(blockedPush("p1", "alice")));
+            loginAs("alice", true);
+
+            // Returns 403 — no interaction with pushStore at all
+            controller.approve("p1", approveBody());
+            verify(pushStore, org.mockito.Mockito.never()).approve(any(), any());
+        }
+
         @Test
         void selfCertify_nonAdmin_notFlaggedAsAdminOverride() throws Exception {
             when(pushStore.findById("p1")).thenReturn(Optional.of(blockedPush("p1", "alice")));