fix: serve error page without login and show a clear message for stale edits

.github/workflows/workflow.yml

@@ -118,7 +118,9 @@ jobs:
 
       - name: Run E2E tests
         run: make test-e2e GAMMA_VERSION=${{ steps.gamma-version.outputs.version }}
-        timeout-minutes: 20
+        # Each test boots fresh Postgres, Redis and Gamma containers (~13-20 s per
+        # test depending on the runner), so 60+ tests can exceed 20 minutes.
+        timeout-minutes: 30
 
       - uses: actions/upload-artifact@v7
         timeout-minutes: 20

app/src/main/java/it/chalmers/gamma/adapter/primary/api/ApiExceptionAdvice.java

@@ -1,6 +1,7 @@
 package it.chalmers.gamma.adapter.primary.api;
 
 import it.chalmers.gamma.app.authentication.AccessGuard;
+import it.chalmers.gamma.app.common.StaleDomainObjectException;
 import org.springframework.core.Ordered;
 import org.springframework.core.annotation.Order;
 import org.springframework.http.HttpStatus;
@@ -15,7 +16,7 @@
 @RestControllerAdvice(basePackages = "it.chalmers.gamma.adapter.primary.api")
 public class ApiExceptionAdvice {
 
-  record ApiErrorBody(int status, String error, String message) {}
+  public record ApiErrorBody(int status, String error, String message) {}
 
   @ExceptionHandler(ResponseStatusException.class)
   public ResponseEntity<ApiErrorBody> handleResponseStatusException(ResponseStatusException ex) {
@@ -28,6 +29,12 @@ public ResponseEntity<ApiErrorBody> handleAccessDeniedException(
     return createResponse(HttpStatus.FORBIDDEN, "FORBIDDEN");
   }
 
+  @ExceptionHandler(StaleDomainObjectException.class)
+  public ResponseEntity<ApiErrorBody> handleStaleDomainObjectException(
+      StaleDomainObjectException ex) {
+    return createResponse(HttpStatus.CONFLICT, ex.getMessage());
+  }
+
   private ResponseEntity<ApiErrorBody> createResponse(HttpStatusCode statusCode, String reason) {
     HttpStatus status = HttpStatus.resolve(statusCode.value());
     String error = status != null ? status.getReasonPhrase() : "Unknown";

app/src/main/java/it/chalmers/gamma/adapter/primary/web/GammaErrorController.java

@@ -1,5 +1,6 @@
 package it.chalmers.gamma.adapter.primary.web;
 
+import it.chalmers.gamma.adapter.primary.api.ApiExceptionAdvice;
 import it.chalmers.gamma.app.oauth2.GammaAuthorizationService;
 import it.chalmers.gamma.security.authentication.AuthenticationExtractor;
 import it.chalmers.gamma.security.authentication.UserAuthentication;
@@ -12,39 +13,45 @@
 import org.slf4j.LoggerFactory;
 import org.springframework.boot.webmvc.error.ErrorController;
 import org.springframework.http.HttpStatus;
+import org.springframework.http.HttpStatusCode;
+import org.springframework.http.MediaType;
+import org.springframework.http.ResponseEntity;
 import org.springframework.stereotype.Controller;
-import org.springframework.web.bind.annotation.GetMapping;
 import org.springframework.web.bind.annotation.RequestHeader;
+import org.springframework.web.bind.annotation.RequestMapping;
 import org.springframework.web.servlet.ModelAndView;
 
+/**
+ * Renders errors for all request methods. Like Spring Boot's BasicErrorController, clients that
+ * accept HTML get an error page while other clients (e.g. API clients) get a JSON body.
+ */
 @Controller
+@RequestMapping("/error")
 public class GammaErrorController implements ErrorController {
 
   private static final Logger LOGGER = LoggerFactory.getLogger(GammaErrorController.class);
 
-  @GetMapping("/error")
-  public ModelAndView handleRuntimeException(
+  @RequestMapping(produces = MediaType.TEXT_HTML_VALUE)
+  public ModelAndView handleErrorHtml(
       @RequestHeader(value = "HX-Request", required = false) boolean htmxRequest,
       HttpServletResponse response,
       HttpServletRequest request) {
-    Object statusCodeString = request.getAttribute(RequestDispatcher.ERROR_STATUS_CODE);
-
-    int statusCode = statusCodeString == null ? 500 : Integer.parseInt(statusCodeString.toString());
-
-    Exception exception = (Exception) request.getAttribute(RequestDispatcher.ERROR_EXCEPTION);
+    HttpStatusCode status = getStatus(request);
+    Exception exception = getException(request);
     if (exception != null) {
       LOGGER.error("Caught error, rendering error page...", exception);
     }
 
     String page = "pages/error";
 
-    if (HttpStatus.valueOf(statusCode) == HttpStatus.NOT_FOUND) {
+    if (status.value() == HttpStatus.NOT_FOUND.value()) {
       page = "pages/404";
     } else if (exception instanceof GammaAuthorizationService.UserNotAllowedRuntimeException) {
       page = "pages/no-access-to-client";
     }
 
     ModelAndView mv = new ModelAndView();
+    mv.setStatus(status);
     if (htmxRequest) {
       response.addHeader("HX-Retarget", "body");
       response.addHeader("HX-Reswap", "innerHTML");
@@ -55,8 +62,10 @@ public ModelAndView handleRuntimeException(
       mv.addObject("page", page);
     }
 
-    if (AuthenticationExtractor.getAuthentication() instanceof UserAuthentication userAuth
-        && userAuth.isAdmin()) {
+    mv.addObject("statusCode", status.value());
+    mv.addObject("statusReason", getReasonPhrase(status));
+
+    if (isAdmin()) {
       if (exception != null) {
         mv.addObject("exceptionMessage", exception.getMessage());
 
@@ -65,13 +74,69 @@ public ModelAndView handleRuntimeException(
         exception.printStackTrace(pw);
         mv.addObject("exceptionStackTrace", sw.toString());
       } else {
-        Object errorMessage = request.getAttribute(RequestDispatcher.ERROR_MESSAGE);
+        String errorMessage = getErrorMessage(request);
         if (errorMessage != null) {
-          mv.addObject("exceptionMessage", errorMessage.toString());
+          mv.addObject("exceptionMessage", errorMessage);
         }
       }
     }
 
     return mv;
   }
+
+  @RequestMapping
+  public ResponseEntity<ApiExceptionAdvice.ApiErrorBody> handleError(HttpServletRequest request) {
+    HttpStatusCode status = getStatus(request);
+    Exception exception = getException(request);
+    if (exception != null) {
+      LOGGER.error("Caught error, returning error response...", exception);
+    }
+
+    String reason = getReasonPhrase(status);
+    String message = null;
+    if (isAdmin()) {
+      message = exception != null ? exception.getMessage() : getErrorMessage(request);
+    }
+    if (message == null || message.isBlank()) {
+      message = reason;
+    }
+
+    return ResponseEntity.status(status)
+        .contentType(MediaType.APPLICATION_JSON)
+        .body(new ApiExceptionAdvice.ApiErrorBody(status.value(), reason, message));
+  }
+
+  private static HttpStatusCode getStatus(HttpServletRequest request) {
+    Object statusCode = request.getAttribute(RequestDispatcher.ERROR_STATUS_CODE);
+    if (statusCode == null) {
+      return HttpStatus.INTERNAL_SERVER_ERROR;
+    }
+
+    try {
+      return HttpStatusCode.valueOf(Integer.parseInt(statusCode.toString()));
+    } catch (IllegalArgumentException e) {
+      return HttpStatus.INTERNAL_SERVER_ERROR;
+    }
+  }
+
+  private static String getReasonPhrase(HttpStatusCode status) {
+    HttpStatus resolved = HttpStatus.resolve(status.value());
+    return resolved != null ? resolved.getReasonPhrase() : "Error";
+  }
+
+  private static Exception getException(HttpServletRequest request) {
+    return request.getAttribute(RequestDispatcher.ERROR_EXCEPTION) instanceof Exception exception
+        ? exception
+        : null;
+  }
+
+  private static String getErrorMessage(HttpServletRequest request) {
+    Object errorMessage = request.getAttribute(RequestDispatcher.ERROR_MESSAGE);
+    return errorMessage != null ? errorMessage.toString() : null;
+  }
+
+  private static boolean isAdmin() {
+    return AuthenticationExtractor.getAuthentication() instanceof UserAuthentication userAuth
+        && userAuth.isAdmin();
+  }
 }

app/src/main/java/it/chalmers/gamma/adapter/primary/web/ThymeleafAdvice.java

@@ -1,6 +1,7 @@
 package it.chalmers.gamma.adapter.primary.web;
 
 import it.chalmers.gamma.app.authentication.AccessGuard;
+import it.chalmers.gamma.app.common.StaleDomainObjectException;
 import it.chalmers.gamma.security.authentication.AuthenticationExtractor;
 import it.chalmers.gamma.security.authentication.UserAuthentication;
 import jakarta.servlet.http.HttpServletRequest;
@@ -64,6 +65,26 @@ public ModelAndView handleMaxSizeException(HttpServletResponse response) {
     return new ModelAndView("common/content-too-large");
   }
 
+  @ExceptionHandler(StaleDomainObjectException.class)
+  public ModelAndView handleStaleDomainObjectException(
+      HttpServletRequest request, HttpServletResponse response) {
+    boolean htmxRequest = "true".equals(request.getHeader("HX-Request"));
+
+    ModelAndView mv = new ModelAndView();
+    mv.setStatus(HttpStatus.CONFLICT);
+    if (htmxRequest) {
+      response.addHeader("HX-Retarget", "#alerts");
+      response.addHeader("HX-Reswap", "afterbegin");
+
+      mv.setViewName("common/stale-object");
+    } else {
+      mv.setViewName("index");
+      mv.addObject("page", "pages/stale-object");
+    }
+
+    return mv;
+  }
+
   @ResponseStatus(HttpStatus.FORBIDDEN)
   @ExceptionHandler(AccessDeniedException.class)
   public void handleAccessDeniedException(HttpServletResponse response) throws IOException {

app/src/main/java/it/chalmers/gamma/adapter/secondary/jpa/util/MutableEntity.java

@@ -1,5 +1,6 @@
 package it.chalmers.gamma.adapter.secondary.jpa.util;
 
+import it.chalmers.gamma.app.common.StaleDomainObjectException;
 import jakarta.persistence.Column;
 import jakarta.persistence.MappedSuperclass;
 import java.time.Instant;
@@ -55,6 +56,4 @@ public int getVersion() {
   }
 
   public static class IllegalEntityStateException extends RuntimeException {}
-
-  public static class StaleDomainObjectException extends RuntimeException {}
 }

app/src/main/java/it/chalmers/gamma/app/common/StaleDomainObjectException.java

@@ -0,0 +1,13 @@
+package it.chalmers.gamma.app.common;
+
+/**
+ * Thrown when trying to save a domain object that has been modified by someone else since it was
+ * loaded. Mutable domain objects carry a version, and an update is only accepted if the provided
+ * version matches the currently stored version (optimistic locking).
+ */
+public class StaleDomainObjectException extends RuntimeException {
+
+  public StaleDomainObjectException() {
+    super("The object has been modified by someone else since it was loaded");
+  }
+}

app/src/main/java/it/chalmers/gamma/security/SecurityFiltersConfig.java

@@ -14,6 +14,7 @@
 import it.chalmers.gamma.app.user.domain.UserId;
 import it.chalmers.gamma.security.api.ApiAuthenticationFilter;
 import it.chalmers.gamma.security.api.ApiAuthenticationProvider;
+import jakarta.servlet.DispatcherType;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 import org.springframework.core.annotation.Order;
@@ -202,6 +203,12 @@ SecurityFilterChain webSecurityFilterChain(
                     .anonymous()
                     .requestMatchers(HttpMethod.GET, "/robots.txt")
                     .permitAll()
+                    // The error page must be reachable without logging in, otherwise errors
+                    // are turned into redirects to the login page.
+                    .dispatcherTypeMatchers(DispatcherType.ERROR)
+                    .permitAll()
+                    .requestMatchers(HttpMethod.GET, "/error")
+                    .permitAll()
                     .anyRequest()
                     .authenticated())
         .formLogin(new LoginCustomizer())

app/src/main/resources/templates/common/header-without-nav.html

@@ -18,12 +18,14 @@ <h1>
     </div>
     <div id="login-container">
         <th:block sec:authorize="isAuthenticated()">
-            <div th:text="|Hey, ${#authentication.getDetails().gammaUser().nick().value()}!|"></div>
-            <form th:action="@{/logout}" th:method="post" >
-                <button class="outline contrast" data-loading-disable>
-                    Logout
-                </button>
-            </form>
+            <th:block th:if="${#authentication.details instanceof T(it.chalmers.gamma.security.authentication.UserAuthentication)}">
+                <div th:text="|Hey, ${#authentication.details.gammaUser().nick().value()}!|"></div>
+                <form th:action="@{/logout}" th:method="post" >
+                    <button class="outline contrast" data-loading-disable>
+                        Logout
+                    </button>
+                </form>
+            </th:block>
         </th:block>
     </div>
 </header>

app/src/main/resources/templates/common/header.html

@@ -84,12 +84,14 @@ <h1>
     </div>
     <div>
         <th:block sec:authorize="isAuthenticated()">
-            <div th:text="|Hey, ${#authentication.getDetails().gammaUser().nick().value()}!|"></div>
-            <form th:action="@{/logout}" th:method="post" >
-                <button class="outline contrast" data-loading-disable>
-                    Logout
-                </button>
-            </form>
+            <th:block th:if="${#authentication.details instanceof T(it.chalmers.gamma.security.authentication.UserAuthentication)}">
+                <div th:text="|Hey, ${#authentication.details.gammaUser().nick().value()}!|"></div>
+                <form th:action="@{/logout}" th:method="post" >
+                    <button class="outline contrast" data-loading-disable>
+                        Logout
+                    </button>
+                </form>
+            </th:block>
         </th:block>
     </div>
 </header>

app/src/main/resources/templates/common/stale-object.html

@@ -0,0 +1,16 @@
+<div data-hx-swap-oob="afterbegin:#alerts">
+    <div role="alert"
+         style="opacity: 0"
+         class="error"
+         _="on load transition *opacity to 1">
+        <span>
+            This item was modified by someone else while you were editing it.
+            Reload the page to get the latest version, then apply your changes again.
+        </span>
+        <button type="button"
+                class="outline contrast"
+                _="on click call window.location.reload()">
+            Reload
+        </button>
+    </div>
+</div>

app/src/main/resources/templates/pages/error.html

@@ -1,7 +1,7 @@
 <header th:replace="~{common/header}"></header>
 <main>
     <article>
-        <header>
+        <header th:text="|${statusCode ?: 500} - ${statusReason ?: 'Internal Server Error'}|">
             500 - Internal Server Error
         </header>
         <p>Something went wrong...</p>

app/src/main/resources/templates/pages/stale-object.html

@@ -0,0 +1,15 @@
+<header th:replace="~{common/header}"></header>
+<main>
+    <article>
+        <header>
+            409 - Conflict
+        </header>
+        <p>
+            This item was modified by someone else while you were editing it.
+            Reload the page to get the latest version, then apply your changes again.
+        </p>
+        <footer>
+            <button type="button" _="on click call history.back()">Go back</button>
+        </footer>
+    </article>
+</main>