Return 404 when bad action name is provided (instead of 500) (#348)

* Return 404 when bad action name is provided (instead of 500) + tighten up logging * + tighten up logging, based on greg's feedback
xh · Apr 12, 2024 · 2786a8a · 2786a8a
1 parent 714dcfc
commit 2786a8a
Showing 1 changed file with 27 additions and 20 deletions.
diff --git a/grails-app/controllers/io/xh/hoist/security/AccessInterceptor.groovy b/grails-app/controllers/io/xh/hoist/security/AccessInterceptor.groovy
@@ -10,12 +10,15 @@ package io.xh.hoist.security
 import groovy.transform.CompileStatic
 import io.xh.hoist.exception.ExceptionHandler
 import io.xh.hoist.exception.NotAuthorizedException
-import io.xh.hoist.user.HoistUser
+import io.xh.hoist.exception.NotFoundException
+import io.xh.hoist.log.LogSupport
 import io.xh.hoist.user.IdentityService
 import java.lang.reflect.Method
 
+import static org.springframework.util.ReflectionUtils.findMethod
+
 @CompileStatic
-class AccessInterceptor {
+class AccessInterceptor implements LogSupport {
 
     IdentityService identityService
     ExceptionHandler xhExceptionHandler
@@ -33,27 +36,21 @@ class AccessInterceptor {
             return true
         }
 
-        // Ignore improperly mapped requests -- these will be handled via url 404 mapping
+        // Get controller method, or 404
         Class clazz = controllerClass?.clazz
-        if (!clazz) {
-            return true
-        }
-
-
-        String actionNm = actionName ?: controllerClass.defaultAction
-        Method method = clazz.getMethod(actionNm)
+        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) ?:
+            method.getAnnotation(AccessAll) ?:
                 clazz.getAnnotation(Access) as Access ?:
-                clazz.getAnnotation(AccessAll) as AccessAll
-
-        if (access instanceof Access) {
-            HoistUser user = identityService.getUser()
-            return user.hasAllRoles(access.value()) ? true : handleUnauthorized()
-        }
+                    clazz.getAnnotation(AccessAll) as AccessAll
 
-        if (access instanceof AccessAll) {
+        if (access instanceof AccessAll ||
+            (access instanceof Access && identityService.user.hasAllRoles(access.value()))
+        ) {
             return true
         }
 
@@ -71,8 +68,18 @@ class AccessInterceptor {
             """)
         xhExceptionHandler.handleException(
             exception: ex,
-            logTo: identityService,
-            logMessage: [_action: actionName],
+            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