Improve exception handling/rendering during auth checks (#405)

* More complete exception handling in `AccessInterceptor`, `AuthenticationService.allowRequest`, and `HoistFilter` 
 - leave no request related exception uncaught!
* Return auth-related exceptions as opaque HTTP status codes
* Use `@NamedVariant` for `handleException`
* Harden `AccessInterceptor` check for websocket handshake
* Changelog cleanup

---------

Co-authored-by: Anselm McClain <[email protected]>

CHANGELOG.md

@@ -3,31 +3,29 @@
 ## 22.0-SNAPSHOT
 
 ### 💥 Breaking Changes (upgrade difficulty: 🟢 LOW)
-* All `Timer`, `Cache`, and `CachedValue` object require a 'name' property.  This property was
-previously optional in many cases, but is now required in order to support new cluster features,
-logging, and admin tools.  The new `BaseService.resources` property now will give access to all
-resources by name, if needed and replaces `BaseService.timers`.
 
-* `BaseService` methods `getIMap()`, `getReplicatedMap()` and `getISet()` have been changed to
-  `createIMap()`, `createReplicatedMap()` and `createISet()`, respectively.  This change provides
-  a consistent interface for all resources on BaseService and is not expected to impact most
- applications.
+* Updated `Timer`, `Cache`, and `CachedValue` objects to require a `name` property. Names are now
+  mandatory to better support new cluster features, logging, and Admin Console tooling.
+* Migrated `BaseService` methods `getIMap()`, `getReplicatedMap()` and `getISet()` to
+  `createIMap()`, `createReplicatedMap()` and `createISet()`, respectively. Not expected to impact
+  most apps, as these APIs are new and only used for distributed, multi-instance data.
 
 ### 🎁 New Features
-* `Cache` and `CachedValue` should now be created using a factory on `BaseService`.  This streamlined
-interface reduces boilerplate, and provides a consistent interface with `Timer`.
 
-### ⚙️ Technical
-
-* Improvements to `Timer` to avoid extra executions when primary instance changes.
+* Added new `BaseService` factories to create `Cache` and `CachedValue` objects. This streamlined
+  interface reduces boilerplate and is consistent with `Timer` creation.
+* Improved `Timer` to maintain consistent execution timing across primary instance changes.
+* Improved `RestController` to support domain objects linked to a non-primary `DataSource`.
 
-* Updated `ClusterService` to use Hoist's `InstanceNotFoundException` class to designate routine.
+### ⚙️ Technical
 
 * Exposed `/xh/ping` as whitelisted route for basic uptime/reachability checks. Retained legacy
   `/ping` alias, but prefer this new path going forward.
-
-* Improvements to `RestController` to better support editing Domain Objects defined with secondary
-  domain objects.
+* Improved handling + rendering of exceptions during authentication and authorization requests.
+* Updated `ClusterService` to use Hoist's `InstanceNotFoundException`, ensuring that common errors
+  thrown due to instance changes are marked as routine and don't spam error reporting.
+* Added new `BaseService.resources` property to track and provide access to `Cache` objects and
+  `Timer`s by name, replacing `BaseService.timers`.
 
 ## 21.0.1 - 2024-09-05
 

grails-app/controllers/io/xh/hoist/security/AccessInterceptor.groovy

@@ -13,6 +13,8 @@ import io.xh.hoist.exception.NotAuthorizedException
 import io.xh.hoist.exception.NotFoundException
 import io.xh.hoist.log.LogSupport
 import io.xh.hoist.user.IdentityService
+import io.xh.hoist.websocket.HoistWebSocketConfigurer
+
 import java.lang.reflect.Method
 
 import static org.springframework.util.ReflectionUtils.findMethod
@@ -28,66 +30,55 @@ class AccessInterceptor implements LogSupport {
     }
 
     boolean before() {
-
-        // Ignore Websockets -- these are destined for a non-controller based endpoint
-        // established via a spring-websocket configuration mapping. (Note this is *not* currently
-        // built into Hoist but is checked / allowed for here.)
-        if (isWebSocketHandshake()) {
-            return true
-        }
-
-        // Get controller method, or 404
-        Class clazz = controllerClass?.clazz
-        String actionNm = actionName ?: controllerClass?.defaultAction
-        Method method = clazz && actionNm ? findMethod(clazz, actionNm) : null
-        if (!method) return handleNotFound()
-
-       // Eval method annotations, and return true or 401
-        def access = method.getAnnotation(Access) ?:
-            method.getAnnotation(AccessAll) ?:
-                clazz.getAnnotation(Access) as Access ?:
-                    clazz.getAnnotation(AccessAll) as AccessAll
-
-        if (access instanceof AccessAll ||
-            (access instanceof Access && identityService.user.hasAllRoles(access.value()))
-        ) {
-            return true
+        try {
+            // Ignore websockets - these are destined for a non-controller based endpoint
+            // established via a spring-websocket configuration mapping.
+            // Note that websockets are not always enabled by Hoist apps but must be supported here.
+            if (isWebSocketHandshake()) {
+                return true
+            }
+
+            // Get controller method, or throw 404 (Not Found).
+            Class clazz = controllerClass?.clazz
+            String actionNm = actionName ?: controllerClass?.defaultAction
+            Method method = clazz && actionNm ? findMethod(clazz, actionNm) : null
+            if (!method) throw new NotFoundException()
+
+            // Eval @Access annotations, return true if allowed, or throw 403 (Forbidden).
+            def access = method.getAnnotation(Access) ?:
+                method.getAnnotation(AccessAll) ?:
+                    clazz.getAnnotation(Access) as Access ?:
+                        clazz.getAnnotation(AccessAll) as AccessAll
+
+            if (access instanceof AccessAll ||
+                    (access instanceof Access && identityService.user.hasAllRoles(access.value()))
+            ) {
+                return true
+            }
+
+            def username = identityService.username ?: 'UNKNOWN'
+            throw new NotAuthorizedException(
+                "You do not have the required role(s) for this action. Currently logged in as: $username."
+            )
+        } catch (Exception e) {
+            xhExceptionHandler.handleException(
+                exception: e,
+                logMessage: [controller: controllerClass?.name, action: actionName],
+                logTo: this,
+                renderTo: response
+            )
         }
-
-        return handleUnauthorized()
     }
 
     //------------------------
     // Implementation
     //------------------------
-    private boolean handleUnauthorized() {
-        def username = identityService.username ?: 'UNKNOWN',
-            ex = new NotAuthorizedException("""
-                    You do not have the application role(s) required.
-                    Currently logged in as: $username.
-            """)
-        xhExceptionHandler.handleException(
-            exception: ex,
-            logTo: this,
-            logMessage: [controller: controllerClass?.name, action: actionName],
-            renderTo: response
-        )
-        return false
-    }
-
-    private boolean handleNotFound() {
-        xhExceptionHandler.handleException(
-            exception: new NotFoundException(),
-            logTo: this,
-            logMessage: [controller: controllerClass?.name, action: actionName],
-            renderTo: response
-        )
-        return false
-    }
-
     private boolean isWebSocketHandshake() {
-        def upgradeHeader = request?.getHeader('upgrade')
-        return upgradeHeader == 'websocket'
+        def req = getRequest(),
+            upgradeHeader = req?.getHeader('upgrade'),
+            uri = req?.requestURI
+
+        return upgradeHeader == 'websocket' && uri?.endsWith(HoistWebSocketConfigurer.WEBSOCKET_PATH)
     }
 
 }

src/main/groovy/io/xh/hoist/HoistFilter.groovy

@@ -10,13 +10,15 @@ package io.xh.hoist
 import groovy.transform.CompileStatic
 import io.xh.hoist.exception.InstanceNotAvailableException
 import io.xh.hoist.log.LogSupport
-import io.xh.hoist.security.BaseAuthenticationService
-import io.xh.hoist.util.Utils
 
 import javax.servlet.*
 import javax.servlet.http.HttpServletRequest
 import javax.servlet.http.HttpServletResponse
 
+import static io.xh.hoist.util.Utils.authenticationService
+import static io.xh.hoist.util.Utils.exceptionHandler
+import static io.xh.hoist.util.Utils.instanceReady
+
 /**
  * Main Filter for all requests in Hoist.
  *
@@ -34,27 +36,21 @@ class HoistFilter implements Filter, LogSupport {
         HttpServletRequest httpRequest = (HttpServletRequest) request
         HttpServletResponse httpResponse = (HttpServletResponse) response
 
-        // Need to be *ready* before even attempting auth.
-        if (!Utils.instanceReady) {
-            Utils.exceptionHandler.handleException(
-                exception: new InstanceNotAvailableException('Application may be initializing. Please try again shortly.'),
-                renderTo: httpResponse,
-                logTo: this
-            )
-            return
-        }
+        try {
+            // Need to be *ready* before even attempting auth.
+            if (!instanceReady) {
+                throw new InstanceNotAvailableException('Application may be initializing. Please try again shortly.')
+            }
 
-        BaseAuthenticationService authSvc = Utils.authenticationService
-        if (authSvc.allowRequest(httpRequest, httpResponse)) {
-            try {
+            if (authenticationService.allowRequest(httpRequest, httpResponse)) {
                 chain.doFilter(request, response)
-            } catch (Throwable t) {
-                Utils.exceptionHandler.handleException(
-                    exception: t,
-                    renderTo: httpResponse,
-                    logTo: this
-                )
             }
+        } catch (Throwable t) {
+            exceptionHandler.handleException(
+                exception: t,
+                renderTo: httpResponse,
+                logTo: this
+            )
         }
     }
 }

src/main/groovy/io/xh/hoist/exception/ExceptionHandler.groovy

@@ -9,6 +9,8 @@ package io.xh.hoist.exception
 
 import grails.util.GrailsUtil
 import groovy.transform.CompileStatic
+import groovy.transform.NamedParam
+import groovy.transform.NamedVariant
 import io.xh.hoist.json.JSONSerializer
 import io.xh.hoist.log.LogSupport
 
@@ -37,25 +39,26 @@ class ExceptionHandler {
      * Used by BaseController, ClusterRequest, Timer, and AccessInterceptor to handle
      * otherwise unhandled exception.
      */
-    void handleException(Map options) {
-        Throwable t = options.exception as Throwable
-        HttpServletResponse renderTo = options.renderTo as HttpServletResponse
-        LogSupport logTo = options.logTo as LogSupport
-        Object logMessage = options.logMessage
-
-        t = preprocess(t)
+    @NamedVariant
+    void handleException(
+        @NamedParam(required = true) Throwable exception,
+        @NamedParam HttpServletResponse renderTo,
+        @NamedParam LogSupport logTo,
+        @NamedParam Object logMessage
+    ) {
+        exception = preprocess(exception)
         if (logTo) {
             if (logMessage) {
-                shouldLogDebug(t) ? logTo.logDebug(logMessage, t) : logTo.logError(logMessage, t)
+                shouldLogDebug(exception) ? logTo.logDebug(logMessage, exception) : logTo.logError(logMessage, exception)
             } else {
-                shouldLogDebug(t) ? logTo.logDebug(t) : logTo.logError(t)
+                shouldLogDebug(exception) ? logTo.logDebug(exception) : logTo.logError(exception)
             }
         }
 
         if (renderTo) {
-            renderTo.setStatus(getHttpStatus(t))
+            renderTo.setStatus(getHttpStatus(exception))
             renderTo.setContentType('application/json')
-            renderTo.writer.write(JSONSerializer.serialize(t))
+            renderTo.writer.write(JSONSerializer.serialize(exception))
             renderTo.flushBuffer()
         }
     }

src/main/groovy/io/xh/hoist/exception/NotAuthorizedException.groovy

@@ -12,10 +12,12 @@ import static org.apache.hc.core5.http.HttpStatus.SC_FORBIDDEN
 /**
  * Exception for use when the authenticated user does not have access to the resource in question.
  *
- * This exception is thrown by Hoist's AccessInterceptor class if the user does not have roles
- * required by a controller's @Access annotation. Applications may also throw this exception, or
- * subclasses of it, directly in response to requests they cannot fulfill due to auth-related
- * constraints.
+ * This exception is thrown by Hoist's {@link io.xh.hoist.security.BaseAuthenticationService} if
+ * an authenticated user is not found and by {@link io.xh.hoist.security.AccessInterceptor} if an
+ * authenticated user does not have roles required by a controller's `@Access` annotation.
+ *
+ * Applications may also throw this exception, or subclasses of it, directly in response to requests
+ * they cannot fulfill due to auth-related constraints.
  *
  * Instances of this exception will be sent to clients with HttpStatus 403 ('Forbidden').
  */

src/main/groovy/io/xh/hoist/security/BaseAuthenticationService.groovy

@@ -8,14 +8,19 @@ package io.xh.hoist.security
 
 import groovy.transform.CompileStatic
 import io.xh.hoist.BaseService
+import io.xh.hoist.exception.HttpException
 import io.xh.hoist.exception.NotAuthorizedException
 import io.xh.hoist.user.HoistUser
 import io.xh.hoist.user.IdentityService
 
 import javax.servlet.http.HttpServletRequest
 import javax.servlet.http.HttpServletResponse
 
+import static io.xh.hoist.util.Utils.getExceptionHandler
 import static java.util.Collections.emptyMap
+import static org.apache.hc.core5.http.HttpStatus.SC_SERVER_ERROR
+
+
 
 /**
  * Abstract base service for processing and confirming user authentications and evaluating incoming
@@ -95,24 +100,31 @@ abstract class BaseAuthenticationService extends BaseService {
     //--------------------
     /**
      * Called once on every request to ensure request is authenticated before passing through to
-     * rest of the framework. Not typically overridden - see completeAuthentication() as main entry
+     * rest of the framework. Not for override - see completeAuthentication() as main entry
      * point for subclass implementation.
      */
     boolean allowRequest(HttpServletRequest request, HttpServletResponse response) {
-        if (identityService.findAuthUser(request) || isWhitelist(request)) {
-            return true
-        }
+        try {
+            if (identityService.findAuthUser(request) || isWhitelist(request)) {
+                return true
+            }
 
-        def complete = completeAuthentication(request, response)
-        if (!complete) return false
+            if (!completeAuthentication(request, response)) {
+                return false
+            }
 
-        if (!identityService.findAuthUser(request)) {
-            response.setStatus(401)
+            if (!identityService.findAuthUser(request)) {
+                throw new NotAuthorizedException()
+            }
+
+            return true
+        } catch (Throwable e) {
+            // Do *not* render auth exception to unverified client. Log and return opaque response
+            exceptionHandler.handleException(exception: e, logTo: this)
+            response.setStatus(e instanceof HttpException ? e.statusCode : SC_SERVER_ERROR)
             response.flushBuffer()
             return false
         }
-
-        return true
     }
 
     /**

src/main/groovy/io/xh/hoist/websocket/HoistWebSocketConfigurer.groovy

@@ -15,10 +15,12 @@ import org.springframework.web.socket.server.support.HttpSessionHandshakeInterce
 @EnableWebSocket
 class HoistWebSocketConfigurer implements WebSocketConfigurer {
 
+    static final String WEBSOCKET_PATH = '/xhWebSocket'
+
     @Override
     void registerWebSocketHandlers(WebSocketHandlerRegistry registry) {
         def handler = new PerConnectionWebSocketHandler(HoistWebSocketHandler.class)
-        registry.addHandler(handler, '/xhWebSocket')
+        registry.addHandler(handler, WEBSOCKET_PATH)
             .addInterceptors(new HttpSessionHandshakeInterceptor())
             .setAllowedOrigins('*')
     }