From f2d55de1731fb91dbc35463cd8474f22916baa83 Mon Sep 17 00:00:00 2001 From: lbwexler Date: Sun, 15 Sep 2024 17:29:01 -0400 Subject: [PATCH 1/5] More complete exception handling in AccessInterceptor and AuthenticationService.allowRequest, and HoistFilter -- Leave no request related exception uncaught! Auth Related exceptions are returned as opaque Use 'NamedVariant' for handleException --- .../hoist/security/AccessInterceptor.groovy | 89 ++++++++----------- .../groovy/io/xh/hoist/HoistFilter.groovy | 36 ++++---- .../hoist/exception/ExceptionHandler.groovy | 25 +++--- .../security/BaseAuthenticationService.groovy | 32 ++++--- 4 files changed, 90 insertions(+), 92 deletions(-) diff --git a/grails-app/controllers/io/xh/hoist/security/AccessInterceptor.groovy b/grails-app/controllers/io/xh/hoist/security/AccessInterceptor.groovy index cd653a58..d837c1a4 100644 --- a/grails-app/controllers/io/xh/hoist/security/AccessInterceptor.groovy +++ b/grails-app/controllers/io/xh/hoist/security/AccessInterceptor.groovy @@ -28,63 +28,50 @@ 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 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) throw new NotFoundException() + + // 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 + } + + 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' diff --git a/src/main/groovy/io/xh/hoist/HoistFilter.groovy b/src/main/groovy/io/xh/hoist/HoistFilter.groovy index 07258997..1d63ef9a 100644 --- a/src/main/groovy/io/xh/hoist/HoistFilter.groovy +++ b/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 + ) } } } diff --git a/src/main/groovy/io/xh/hoist/exception/ExceptionHandler.groovy b/src/main/groovy/io/xh/hoist/exception/ExceptionHandler.groovy index 95dd25b7..91d3c8fd 100644 --- a/src/main/groovy/io/xh/hoist/exception/ExceptionHandler.groovy +++ b/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() } } diff --git a/src/main/groovy/io/xh/hoist/security/BaseAuthenticationService.groovy b/src/main/groovy/io/xh/hoist/security/BaseAuthenticationService.groovy index 0db054d6..b63f5a11 100644 --- a/src/main/groovy/io/xh/hoist/security/BaseAuthenticationService.groovy +++ b/src/main/groovy/io/xh/hoist/security/BaseAuthenticationService.groovy @@ -8,6 +8,7 @@ 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 @@ -15,7 +16,11 @@ 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 } /** From e23c40b493a3db78d81a59841c89c18057322c9d Mon Sep 17 00:00:00 2001 From: lbwexler Date: Sun, 15 Sep 2024 17:31:28 -0400 Subject: [PATCH 2/5] Changelog message --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f28d8558..e87b298e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,9 @@ * Improvements to `RestController` to better support editing Domain Objects defined with secondary domain objects. +* Improved handling/rendering of exceptions during Authentication and Authorization checking of + requests. + ## 21.0.1 - 2024-09-05 ### 🐞 Bug Fixes From 3a25d4d6745de2cae6bed0639df15a78e49e7b37 Mon Sep 17 00:00:00 2001 From: Anselm McClain Date: Tue, 17 Sep 2024 10:18:10 -0700 Subject: [PATCH 3/5] Minor comment updates --- .../io/xh/hoist/security/AccessInterceptor.groovy | 13 ++++++------- .../hoist/exception/NotAuthorizedException.groovy | 10 ++++++---- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/grails-app/controllers/io/xh/hoist/security/AccessInterceptor.groovy b/grails-app/controllers/io/xh/hoist/security/AccessInterceptor.groovy index d837c1a4..1be1dd08 100644 --- a/grails-app/controllers/io/xh/hoist/security/AccessInterceptor.groovy +++ b/grails-app/controllers/io/xh/hoist/security/AccessInterceptor.groovy @@ -29,28 +29,27 @@ class AccessInterceptor implements LogSupport { boolean before() { try { - - // 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.) + // 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 404 + // 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 method annotations, and return true or 401 + // 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())) + (access instanceof Access && identityService.user.hasAllRoles(access.value())) ) { return true } diff --git a/src/main/groovy/io/xh/hoist/exception/NotAuthorizedException.groovy b/src/main/groovy/io/xh/hoist/exception/NotAuthorizedException.groovy index 8148e86a..9a50deb2 100644 --- a/src/main/groovy/io/xh/hoist/exception/NotAuthorizedException.groovy +++ b/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'). */ From ae9216aa02f3a8fabc2ec5b3899145e1cb13d6c0 Mon Sep 17 00:00:00 2001 From: Anselm McClain Date: Tue, 17 Sep 2024 10:33:30 -0700 Subject: [PATCH 4/5] Harden AccessInterceptor check for websocket handshake --- .../io/xh/hoist/security/AccessInterceptor.groovy | 9 +++++++-- .../xh/hoist/websocket/HoistWebSocketConfigurer.groovy | 4 +++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/grails-app/controllers/io/xh/hoist/security/AccessInterceptor.groovy b/grails-app/controllers/io/xh/hoist/security/AccessInterceptor.groovy index 1be1dd08..7b081c3b 100644 --- a/grails-app/controllers/io/xh/hoist/security/AccessInterceptor.groovy +++ b/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 @@ -72,8 +74,11 @@ class AccessInterceptor implements LogSupport { // Implementation //------------------------ 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) } } diff --git a/src/main/groovy/io/xh/hoist/websocket/HoistWebSocketConfigurer.groovy b/src/main/groovy/io/xh/hoist/websocket/HoistWebSocketConfigurer.groovy index 90f7c715..cc0c3032 100644 --- a/src/main/groovy/io/xh/hoist/websocket/HoistWebSocketConfigurer.groovy +++ b/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('*') } From 34e9c8ee8561c21ccf057cf06d58b1080f92520b Mon Sep 17 00:00:00 2001 From: Anselm McClain Date: Tue, 17 Sep 2024 10:54:27 -0700 Subject: [PATCH 5/5] Changelog cleanup --- CHANGELOG.md | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e3eeb5a9..cef75996 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,34 +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 checking of - requests. +* 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