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();