From a38d14880c505c5d079eab23fd214e608b73e1ac Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Sun, 19 Jul 2026 14:30:11 +0200 Subject: [PATCH] fix(deployment): surface GitHub approval failures instead of a generic 500 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When GitHub rejected an in-app deployment approval/decline, the service threw a ResponseStatusException(502, "GitHub rejected the request: ...") — but the GlobalExceptionHandler's generic Exception handler caught it first (advice handlers run before Spring's ResponseStatusExceptionResolver) and flattened it to a 500 "internal server error", discarding the status and message. Every ResponseStatusException across the app was affected. - Add a dedicated @ExceptionHandler(ResponseStatusException.class) that preserves the intended status + reason (and keeps these handled outcomes out of Sentry; 5xx logged at WARN, 4xx at DEBUG). - Introduce GitHubReviewException (extends IOException) carrying GitHub's HTTP status so the review path can react to a 401 (expired brokered token) with an actionable message telling the reviewer to sign out and back in, while still returning 502 to the client (not 401, which would bounce them to login). Root cause of the 401s themselves (expired Keycloak-brokered GitHub token, not refreshed) is tracked separately. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../DeploymentReviewActionService.java | 20 +++++- .../helios/error/GlobalExceptionHandler.java | 34 ++++++++++ .../helios/github/GitHubReviewException.java | 27 ++++++++ .../cit/aet/helios/github/GitHubService.java | 3 +- ...ntApprovalControllerErrorHandlingTest.java | 67 +++++++++++++++++++ .../DeploymentReviewActionServiceTest.java | 28 ++++++++ 6 files changed, 176 insertions(+), 3 deletions(-) create mode 100644 server/application-server/src/main/java/de/tum/cit/aet/helios/github/GitHubReviewException.java create mode 100644 server/application-server/src/test/java/de/tum/cit/aet/helios/deployment/approval/DeploymentApprovalControllerErrorHandlingTest.java diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/approval/DeploymentReviewActionService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/approval/DeploymentReviewActionService.java index 2974ec773..294f3639d 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/approval/DeploymentReviewActionService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/approval/DeploymentReviewActionService.java @@ -3,6 +3,7 @@ import de.tum.cit.aet.helios.environment.Environment; import de.tum.cit.aet.helios.environment.EnvironmentService; import de.tum.cit.aet.helios.environment.EnvironmentService.ReviewerResolution; +import de.tum.cit.aet.helios.github.GitHubReviewException; import de.tum.cit.aet.helios.github.GitHubService; import de.tum.cit.aet.helios.heliosdeployment.HeliosDeployment; import de.tum.cit.aet.helios.heliosdeployment.HeliosDeploymentRepository; @@ -146,8 +147,10 @@ private DeploymentApprovalRequest review( currentLogin, heliosDeploymentId, e.getMessage()); - throw new ResponseStatusException( - HttpStatus.BAD_GATEWAY, "GitHub rejected the request: " + e.getMessage(), e); + // Keep the client-facing status at 502: this is an upstream GitHub failure, not the Helios + // request being unauthenticated. Returning 401 here would make the client treat the user's + // Helios session as expired and bounce them to login. + throw new ResponseStatusException(HttpStatus.BAD_GATEWAY, gitHubFailureReason(e), e); } // GitHub accepted — finalise this reviewer's row and consume siblings. @@ -213,6 +216,19 @@ private static boolean isTerminallyResolved(DeploymentApprovalRequest r) { || r.getState() == DeploymentApprovalRequest.State.DECLINED; } + /** + * Turns a GitHub review failure into a reviewer-facing message. An {@code HTTP 401} means the + * impersonated user's GitHub token (brokered via Keycloak) has expired — the reviewer can only + * recover by re-authenticating — so we say so explicitly instead of leaking the raw status. + */ + private static String gitHubFailureReason(IOException e) { + if (e instanceof GitHubReviewException gh && gh.getHttpStatus() == 401) { + return "GitHub rejected the request because your GitHub authorization has expired. " + + "Please sign out of Helios and sign in again, then retry."; + } + return "GitHub rejected the request: " + e.getMessage(); + } + private static String buildAuditComment(String login, boolean approve, String userComment) { String base = (approve ? "Approved by @" : "Declined by @") + login + " via Helios (in-app)"; diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/error/GlobalExceptionHandler.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/error/GlobalExceptionHandler.java index 9474ae693..65fcf6081 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/error/GlobalExceptionHandler.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/error/GlobalExceptionHandler.java @@ -14,6 +14,7 @@ import lombok.extern.log4j.Log4j2; import org.springframework.http.HttpHeaders; import org.springframework.http.HttpStatus; +import org.springframework.http.HttpStatusCode; import org.springframework.http.ResponseEntity; import org.springframework.security.access.AccessDeniedException; import org.springframework.security.authorization.AuthorizationDeniedException; @@ -22,6 +23,7 @@ import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.ResponseStatus; import org.springframework.web.bind.annotation.RestControllerAdvice; +import org.springframework.web.server.ResponseStatusException; @Log4j2 @RestControllerAdvice @@ -138,6 +140,38 @@ public ResponseEntity handleTestFailureAnalysisRateLimitException( .body(error); } + // -- EXPLICIT STATUS : ResponseStatusException ------- + // Endpoints throw ResponseStatusException to signal a specific status + reason (e.g. a 502 when + // GitHub rejects an approval). Without this handler the generic Exception handler below would + // catch it first (advice handlers run before Spring's ResponseStatusExceptionResolver) and + // flatten every one of them to a 500 "internal server error", discarding both the status and + // the message the caller intended for the user. + @ExceptionHandler(ResponseStatusException.class) + public ResponseEntity handleResponseStatusException( + ResponseStatusException ex, HttpServletRequest request) { + + HttpStatusCode status = ex.getStatusCode(); + HttpStatus resolved = HttpStatus.resolve(status.value()); + ApiError error = new ApiError(); + error.setStatus(status.value()); + error.setError(resolved != null ? resolved.getReasonPhrase() : "Error"); + error.setMessage(ex.getReason() != null ? ex.getReason() : "Request failed"); + error.setPath(request.getRequestURI()); + error.setTimestamp(Instant.now()); + + // These are deliberate, handled outcomes (auth, conflicts, upstream rejections), so keep them + // out of Sentry; log server-side faults (5xx) at WARN for visibility, client faults at DEBUG. + if (status.is5xxServerError()) { + log.warn("Request to {} failed with {}: {}", request.getRequestURI(), status.value(), + error.getMessage()); + } else { + log.debug("Request to {} rejected with {}: {}", request.getRequestURI(), status.value(), + error.getMessage()); + } + + return new ResponseEntity<>(error, status); + } + // -- 500 INTERNAL SERVER ERROR (FALLBACK) ------------- @ExceptionHandler({Exception.class, IOException.class}) public ResponseEntity handleGeneralException(Exception ex, HttpServletRequest request) { diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/github/GitHubReviewException.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/github/GitHubReviewException.java new file mode 100644 index 000000000..c82ed5949 --- /dev/null +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/github/GitHubReviewException.java @@ -0,0 +1,27 @@ +package de.tum.cit.aet.helios.github; + +import java.io.IOException; + +/** + * Raised when GitHub rejects a pending-deployment review (approve/reject) call. Carries the + * upstream HTTP status so callers can react to it — most importantly {@code 401}, which means the + * impersonated user's GitHub token (brokered through Keycloak) has expired and the reviewer needs + * to re-authenticate. + * + *

Extends {@link IOException} so existing {@code throws IOException} signatures and + * {@code catch (IOException ...)} handlers on the review path keep working unchanged. + */ +public class GitHubReviewException extends IOException { + + private final int httpStatus; + + public GitHubReviewException(int httpStatus, String message) { + super(message); + this.httpStatus = httpStatus; + } + + /** The HTTP status GitHub returned (e.g. 401 for an expired/invalid token). */ + public int getHttpStatus() { + return httpStatus; + } +} diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/github/GitHubService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/github/GitHubService.java index 2eaad7cc0..008ba9238 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/github/GitHubService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/github/GitHubService.java @@ -578,7 +578,8 @@ private void reviewPendingDeployment( runId, response.code(), errorBody); - throw new IOException( + throw new GitHubReviewException( + response.code(), "GitHub pending-deployment " + state + " failed: HTTP " + response.code()); } log.info("Successfully set deployment state '{}' for run ID {}", state, runId); diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/deployment/approval/DeploymentApprovalControllerErrorHandlingTest.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/deployment/approval/DeploymentApprovalControllerErrorHandlingTest.java new file mode 100644 index 000000000..ae2347e4c --- /dev/null +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/deployment/approval/DeploymentApprovalControllerErrorHandlingTest.java @@ -0,0 +1,67 @@ +package de.tum.cit.aet.helios.deployment.approval; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.Mockito.when; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import de.tum.cit.aet.helios.auth.AuthService; +import de.tum.cit.aet.helios.error.GlobalExceptionHandler; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.webmvc.test.autoconfigure.AutoConfigureMockMvc; +import org.springframework.boot.webmvc.test.autoconfigure.WebMvcTest; +import org.springframework.context.annotation.Import; +import org.springframework.http.HttpStatus; +import org.springframework.test.context.ContextConfiguration; +import org.springframework.test.context.bean.override.mockito.MockitoBean; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.web.server.ResponseStatusException; + +/** + * Verifies that {@link GlobalExceptionHandler} lets a deliberate {@link ResponseStatusException} + * keep its status and reason on the way to the client, instead of the generic {@code Exception} + * handler flattening it to a 500 "internal server error". This is what makes an approval that + * GitHub rejects (e.g. an expired token → 502) show the reviewer a real, actionable message. + */ +@AutoConfigureMockMvc(addFilters = false) +@Import(GlobalExceptionHandler.class) +@ContextConfiguration(classes = DeploymentApprovalController.class) +@WebMvcTest(DeploymentApprovalController.class) +class DeploymentApprovalControllerErrorHandlingTest { + + private static final String EXPIRED_AUTH_REASON = + "GitHub rejected the request because your GitHub authorization has expired. " + + "Please sign out of Helios and sign in again, then retry."; + + @Autowired private MockMvc mockMvc; + + @MockitoBean private DeploymentReviewActionService reviewActionService; + @MockitoBean private DeploymentApprovalRequestRepository approvalRequestRepository; + @MockitoBean private AuthService authService; + + @Test + void badGatewayFromServiceReachesClientWithStatusAndReason() throws Exception { + when(reviewActionService.approveAsCurrentUser(anyLong(), any())) + .thenThrow(new ResponseStatusException(HttpStatus.BAD_GATEWAY, EXPIRED_AUTH_REASON)); + + mockMvc + .perform(post("/api/deployments/{deploymentId}/approve", 9436L)) + .andExpect(status().isBadGateway()) + .andExpect(jsonPath("$.status").value(502)) + .andExpect(jsonPath("$.message").value(EXPIRED_AUTH_REASON)); + } + + @Test + void unexpectedExceptionStillFallsBackToInternalServerError() throws Exception { + when(reviewActionService.approveAsCurrentUser(anyLong(), any())) + .thenThrow(new RuntimeException("boom")); + + mockMvc + .perform(post("/api/deployments/{deploymentId}/approve", 9436L)) + .andExpect(status().isInternalServerError()) + .andExpect(jsonPath("$.message").value("Error: An internal server error occurred")); + } +} diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/deployment/approval/DeploymentReviewActionServiceTest.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/deployment/approval/DeploymentReviewActionServiceTest.java index b0beaad91..4a5758730 100644 --- a/server/application-server/src/test/java/de/tum/cit/aet/helios/deployment/approval/DeploymentReviewActionServiceTest.java +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/deployment/approval/DeploymentReviewActionServiceTest.java @@ -16,6 +16,7 @@ import de.tum.cit.aet.helios.environment.Environment; import de.tum.cit.aet.helios.environment.EnvironmentService; import de.tum.cit.aet.helios.environment.EnvironmentService.ReviewerResolution; +import de.tum.cit.aet.helios.github.GitHubReviewException; import de.tum.cit.aet.helios.github.GitHubService; import de.tum.cit.aet.helios.gitrepo.GitRepository; import de.tum.cit.aet.helios.heliosdeployment.HeliosDeployment; @@ -171,6 +172,33 @@ void marksFailedAtGitHubAndReturnsBadGatewayWhenGitHubCallThrows() throws IOExce DeploymentApprovalRequest.State.FAILED_AT_GITHUB, rowCaptor.getValue().getState()); } + @Test + void returnsActionableExpiredAuthMessageWhenGitHubRejectsWith401() throws IOException { + Fixture f = new Fixture(); + f.reviewersAre("alice", "bob"); + doThrow(new GitHubReviewException(401, "GitHub pending-deployment approved failed: HTTP 401")) + .when(f.gitHubService) + .approveDeploymentOnBehalfOfUser( + anyString(), anyLong(), any(), anyString(), anyString()); + + ResponseStatusException e = + assertThrows( + ResponseStatusException.class, + () -> f.service().approveAsCurrentUser(DEPLOYMENT_ID, f.userWithLogin(REVIEWER))); + + // Still 502 (upstream failure, not a Helios auth failure) but with a reviewer-actionable + // reason rather than the raw "HTTP 401". + assertEquals(HttpStatus.BAD_GATEWAY, e.getStatusCode()); + assertEquals(true, e.getReason().contains("expired")); + assertEquals(true, e.getReason().contains("sign in again")); + + ArgumentCaptor rowCaptor = + ArgumentCaptor.forClass(DeploymentApprovalRequest.class); + verify(f.approvalRequestRepository).save(rowCaptor.capture()); + assertEquals( + DeploymentApprovalRequest.State.FAILED_AT_GITHUB, rowCaptor.getValue().getState()); + } + @Test void consumesSiblingPendingRowsAfterSuccessfulApproval() throws IOException { Fixture f = new Fixture();