Refactoring Elide Security Checks (#1144)

elide-annotations/src/main/java/com/yahoo/elide/security/checks/Check.java

@@ -4,39 +4,9 @@
  * See LICENSE file in project root for terms.
  */
 package com.yahoo.elide.security.checks;
-
-import com.yahoo.elide.security.ChangeSpec;
-import com.yahoo.elide.security.RequestScope;
-import com.yahoo.elide.security.User;
-
-import java.util.Optional;
-
 /**
  * Custom security access that verifies whether a user belongs to a role.
  * Permissions are assigned as a set of checks that grant access to the permission.
- * @param <T> Type of record for Check
  */
-public interface Check<T> {
-
-    /**
-     * Determines whether the user can access the resource.
-     *
-     * @param object Fully modified object
-     * @param requestScope Request scope object
-     * @param changeSpec Summary of modifications
-     * @return true if security check passed
-     */
-    boolean ok(T object, RequestScope requestScope, Optional<ChangeSpec> changeSpec);
-
-    /**
-     * Method reserved for user checks.
-     *
-     * @param user User to check
-     * @return True if user check passes, false otherwise
-     */
-    boolean ok(User user);
-
-    default String checkIdentifier() {
-        return this.getClass().getName();
-    }
+public interface Check {
 }

elide-annotations/src/main/java/com/yahoo/elide/security/checks/OperationCheck.java

@@ -5,7 +5,10 @@
  */
 package com.yahoo.elide.security.checks;
 
-import com.yahoo.elide.security.User;
+import com.yahoo.elide.security.ChangeSpec;
+import com.yahoo.elide.security.RequestScope;
+
+import java.util.Optional;
 
 /**
  * Operation check interface.
@@ -18,10 +21,14 @@
  *
  * @param <T> Type parameter
  */
-public abstract class OperationCheck<T> extends InlineCheck<T> {
-    /* NOTE: Operation checks and user checks are intended to be _distinct_ */
-    @Override
-    public final boolean ok(User user) {
-        throw new UnsupportedOperationException();
-    }
+public abstract class OperationCheck<T> implements Check {
+    /**
+     * Determines whether the user can access the resource.
+     *
+     * @param object Fully modified object
+     * @param requestScope Request scope object
+     * @param changeSpec Summary of modifications
+     * @return true if security check passed
+     */
+    public abstract boolean ok(T object, RequestScope requestScope, Optional<ChangeSpec> changeSpec);
 }

elide-annotations/src/main/java/com/yahoo/elide/security/checks/UserCheck.java

@@ -5,19 +5,18 @@
  */
 package com.yahoo.elide.security.checks;
 
-import com.yahoo.elide.security.ChangeSpec;
-import com.yahoo.elide.security.RequestScope;
-
-import java.util.Optional;
+import com.yahoo.elide.security.User;
 
 /**
  * Custom security access that verifies whether a user belongs to a role.
  * Permissions are assigned as a set of checks that grant access to the permission.
  */
-public abstract class UserCheck extends InlineCheck<Object> {
-    /* NOTE: Operation checks and user checks are intended to be _distinct_ */
-    @Override
-    public final boolean ok(Object object, RequestScope requestScope, Optional<ChangeSpec> changeSpec) {
-        return ok(requestScope.getUser());
-    }
+public abstract class UserCheck implements Check {
+    /**
+     * Method reserved for user checks.
+     *
+     * @param user User to check
+     * @return True if user check passes, false otherwise
+     */
+    public abstract boolean ok(User user);
 }

elide-annotations/src/main/java/com/yahoo/elide/security/checks/prefab/Collections.java

@@ -8,7 +8,7 @@
 
 import com.yahoo.elide.security.ChangeSpec;
 import com.yahoo.elide.security.RequestScope;
-import com.yahoo.elide.security.checks.CommitCheck;
+import com.yahoo.elide.security.checks.OperationCheck;
 
 import java.util.Collection;
 import java.util.Optional;
@@ -27,7 +27,7 @@ private Collections() {
      *
      * @param <T> type collection to be validated
      */
-    public static class AppendOnly<T> extends CommitCheck<T> {
+    public static class AppendOnly<T> extends OperationCheck<T> {
 
         @Override
         public boolean ok(T record, RequestScope requestScope, Optional<ChangeSpec> changeSpec) {
@@ -47,7 +47,7 @@ public boolean ok(T record, RequestScope requestScope, Optional<ChangeSpec> chan
      *
      * @param <T> type parameter
      */
-    public static class RemoveOnly<T> extends CommitCheck<T> {
+    public static class RemoveOnly<T> extends OperationCheck<T> {
 
         @Override
         public boolean ok(T record, RequestScope requestScope, Optional<ChangeSpec> changeSpec) {

elide-annotations/src/main/java/com/yahoo/elide/security/checks/prefab/Common.java

@@ -9,7 +9,6 @@
 import com.yahoo.elide.security.ChangeSpec;
 import com.yahoo.elide.security.PersistentResource;
 import com.yahoo.elide.security.RequestScope;
-import com.yahoo.elide.security.checks.CommitCheck;
 import com.yahoo.elide.security.checks.OperationCheck;
 
 import java.util.Optional;
@@ -42,7 +41,7 @@ public boolean ok(T record, RequestScope requestScope, Optional<ChangeSpec> chan
      * but at the same time allows the removal of the child from the relationship with the existing parent
      * @param <T> the type of object that this check guards
      */
-    public static class FieldSetToNull<T> extends CommitCheck<T> {
+    public static class FieldSetToNull<T> extends OperationCheck<T> {
         @Override
         public boolean ok(T record, RequestScope requestScope, Optional<ChangeSpec> changeSpec) {
             return changeSpec.map((c) -> { return c.getModified() == null; })

elide-core/src/main/java/com/yahoo/elide/parsers/expression/CanPaginateVisitor.java

@@ -208,14 +208,14 @@ public PaginationStatus visitPAREN(ExpressionParser.PARENContext ctx) {
 
     @Override
     public PaginationStatus visitPermissionClass(ExpressionParser.PermissionClassContext ctx) {
-        Check<?> check = getCheck(dictionary, ctx.getText());
+        Check check = getCheck(dictionary, ctx.getText());
 
         //Filter expression checks can always be pushed to the DataStore so pagination is possible
         if (check instanceof FilterExpressionCheck) {
             return PaginationStatus.CAN_PAGINATE;
         }
         if (check instanceof UserCheck) {
-            if (check.ok(scope.getUser())) {
+            if (((UserCheck) check).ok(scope.getUser())) {
                 return PaginationStatus.USER_CHECK_TRUE;
             }
             return PaginationStatus.USER_CHECK_FALSE;

elide-core/src/main/java/com/yahoo/elide/parsers/expression/PermissionToFilterExpressionVisitor.java

@@ -177,7 +177,7 @@ public FilterExpression visitPermissionClass(ExpressionParser.PermissionClassCon
         }
 
         if (check instanceof UserCheck) {
-            boolean userCheckResult = check.ok(requestScope.getUser());
+            boolean userCheckResult = ((UserCheck) check).ok(requestScope.getUser());
             return userCheckResult ? TRUE_USER_CHECK_EXPRESSION : FALSE_USER_CHECK_EXPRESSION;
         }
 

elide-core/src/main/java/com/yahoo/elide/security/FilterExpressionCheck.java

@@ -12,8 +12,8 @@
 import com.yahoo.elide.core.filter.FilterPredicate;
 import com.yahoo.elide.core.filter.expression.FilterExpression;
 import com.yahoo.elide.parsers.expression.FilterExpressionCheckEvaluationVisitor;
-import com.yahoo.elide.security.checks.InlineCheck;
 
+import com.yahoo.elide.security.checks.OperationCheck;
 import lombok.extern.slf4j.Slf4j;
 
 import java.util.Optional;
@@ -27,7 +27,7 @@
  * @param <T> Type of class
  */
 @Slf4j
-public abstract class FilterExpressionCheck<T> extends InlineCheck<T> {
+public abstract class FilterExpressionCheck<T> extends OperationCheck<T> {
 
     @Inject
     protected EntityDictionary dictionary;
@@ -41,12 +41,6 @@ public abstract class FilterExpressionCheck<T> extends InlineCheck<T> {
      */
     public abstract FilterExpression getFilterExpression(Class<?> entityClass, RequestScope requestScope);
 
-    /* NOTE: Filter Expression checks and user checks are intended to be _distinct_ */
-    @Override
-    public final boolean ok(User user) {
-        throw new UnsupportedOperationException();
-    }
-
     /**
      * The filter expression is evaluated in memory if it cannot be pushed to the data store by elide for any reason.
      *

elide-core/src/main/java/com/yahoo/elide/security/permissions/expressions/CheckExpression.java

@@ -15,7 +15,7 @@
 import com.yahoo.elide.security.PersistentResource;
 import com.yahoo.elide.security.RequestScope;
 import com.yahoo.elide.security.checks.Check;
-import com.yahoo.elide.security.checks.InlineCheck;
+import com.yahoo.elide.security.checks.OperationCheck;
 import com.yahoo.elide.security.checks.UserCheck;
 import com.yahoo.elide.security.permissions.ExpressionResult;
 import com.yahoo.elide.security.permissions.ExpressionResultCache;
@@ -76,7 +76,7 @@ public ExpressionResult evaluate(EvaluationMode mode) {
             return result;
         }
 
-        if (mode == EvaluationMode.INLINE_CHECKS_ONLY && ! (check instanceof InlineCheck)) {
+        if (mode == EvaluationMode.INLINE_CHECKS_ONLY && (resource != null && resource.isNewlyCreated())) {
             result = DEFERRED;
             return result;
         }
@@ -112,7 +112,12 @@ public ExpressionResult evaluate(EvaluationMode mode) {
      */
     private ExpressionResult computeCheck() {
         Object entity = (resource == null) ? null : resource.getObject();
-        result = check.ok(entity, requestScope, changeSpec) ? PASS : FAIL;
+
+        if (check instanceof UserCheck) {
+            result = ((UserCheck) check).ok(requestScope.getUser()) ? PASS : FAIL;
+        } else {
+            result = ((OperationCheck) check).ok(entity, requestScope, changeSpec) ? PASS : FAIL;
+        }
         return result;
     }
 

elide-core/src/test/java/com/yahoo/elide/core/utils/ClassScannerTest.java

@@ -44,7 +44,7 @@ public void testGetAllAnnotatedClasses() {
     @Test
     public void testGetAnyAnnotatedClasses() {
         Set<Class<?>> classes = ClassScanner.getAnnotatedClasses(ReadPermission.class, UpdatePermission.class);
-        assertEquals(37, classes.size());
+        assertEquals(36, classes.size());
         for (Class<?> cls : classes) {
             assertTrue(cls.isAnnotationPresent(ReadPermission.class)
                     || cls.isAnnotationPresent(UpdatePermission.class));

elide-core/src/test/java/com/yahoo/elide/parsers/expression/PermissionExpressionVisitorTest.java

@@ -145,7 +145,7 @@ public ExpressionResult evaluate(EvaluationMode ignored) {
             if (check instanceof UserCheck) {
                 result = ((UserCheck) check).ok(null);
             } else {
-                result = check.ok(null, null, null);
+                result = ((OperationCheck) check).ok(null, null, null);
             }
 
             if (result) {