Short Circuit Filter Expression Security If User Checks would pass/fa…

…il. (#1275)

* Fleshed out optimization to bypass relationship fetches for filter security evaluation if User Check would pass/fail

* Added unit tests for changes to permission executor

* Finished unit tests

* added explicit check that data store transaction was not invoked

* Fixing bug

* Inspection comment

* Minor fix

Co-authored-by: Aaron Klish <[email protected]>

elide-core/src/main/java/com/yahoo/elide/core/VerifyFieldAccessFilterExpressionVisitor.java

@@ -13,6 +13,8 @@
 import com.yahoo.elide.core.filter.expression.FilterExpressionVisitor;
 import com.yahoo.elide.core.filter.expression.NotFilterExpression;
 import com.yahoo.elide.core.filter.expression.OrFilterExpression;
+import com.yahoo.elide.security.PermissionExecutor;
+import com.yahoo.elide.security.permissions.ExpressionResult;
 
 import java.util.Collections;
 import java.util.Objects;
@@ -40,6 +42,15 @@ public VerifyFieldAccessFilterExpressionVisitor(PersistentResource<?> resource)
     public Boolean visitPredicate(FilterPredicate filterPredicate) {
         RequestScope requestScope = resource.getRequestScope();
         Set<PersistentResource> val = Collections.singleton(resource);
+
+        ExpressionResult result = evaluateUserChecks(filterPredicate.getPath());
+
+        if (result == ExpressionResult.PASS) {
+            return true;
+        } else if (result == ExpressionResult.FAIL) {
+            return false;
+        }
+
         for (Path.PathElement pathElement : filterPredicate.getPath().getPathElements()) {
             String fieldName = pathElement.getFieldName();
 
@@ -74,6 +85,28 @@ private Stream<PersistentResource> getValueChecked(PersistentResource<?> resourc
         return resource.getRelationChecked(fieldName, Optional.empty(), Optional.empty(), Optional.empty()).stream();
     }
 
+    private ExpressionResult evaluateUserChecks(Path path) {
+        PermissionExecutor executor = resource.getRequestScope().getPermissionExecutor();
+
+        ExpressionResult result = ExpressionResult.PASS;
+
+        for (Path.PathElement element : path.getPathElements()) {
+            try {
+                result = executor.checkUserPermissions(
+                        element.getType(),
+                        ReadPermission.class,
+                        element.getFieldName());
+            } catch (ForbiddenAccessException e) {
+                return ExpressionResult.FAIL;
+            }
+
+            if (result != ExpressionResult.PASS) {
+                return result;
+            }
+        }
+        return result;
+    }
+
     @Override
     public Boolean visitAndExpression(AndFilterExpression expression) {
         FilterExpression left = expression.getLeft();

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

@@ -88,6 +88,17 @@ <A extends Annotation> ExpressionResult checkSpecificFieldPermissionsDeferred(Pe
      */
     <A extends Annotation> ExpressionResult checkUserPermissions(Class<?> resourceClass, Class<A> annotationClass);
 
+    /**
+     * Check strictly user permissions on an entity field.
+     *
+     * @param <A> type parameter
+     * @param resourceClass Resource class
+     * @param annotationClass Annotation class
+     * @param field The entity field
+     */
+    public <A extends Annotation> ExpressionResult checkUserPermissions(Class<?> resourceClass,
+                                                                        Class<A> annotationClass,
+                                                                        String field);
     /**
      * Get the read filter, if defined.
      *

elide-core/src/main/java/com/yahoo/elide/security/executors/ActivePermissionExecutor.java

@@ -491,4 +491,31 @@ public String printCheckStats() {
     public boolean isVerbose() {
         return verbose;
     }
+
+    /**
+     * Check strictly user permissions on an entity field.
+     *
+     * @param <A> type parameter
+     * @param resourceClass Resource class
+     * @param annotationClass Annotation class
+     * @param field The entity field
+     */
+    @Override
+    public <A extends Annotation> ExpressionResult checkUserPermissions(Class<?> resourceClass,
+                                                                        Class<A> annotationClass,
+                                                                        String field) {
+        Supplier<Expression> expressionSupplier = () -> {
+            return expressionBuilder.buildUserCheckFieldExpressions(
+                    resourceClass,
+                    requestScope,
+                    annotationClass,
+                    field);
+        };
+
+        return checkOnlyUserPermissions(
+                resourceClass,
+                annotationClass,
+                Optional.of(field),
+                expressionSupplier);
+    }
 }

elide-core/src/main/java/com/yahoo/elide/security/executors/BypassPermissionExecutor.java

@@ -54,6 +54,13 @@ public Optional<FilterExpression> getReadPermissionFilter(Class<?> resourceClass
         return Optional.empty();
     }
 
+    @Override
+    public <A extends Annotation> ExpressionResult checkUserPermissions(Class<?> resourceClass,
+                                                                        Class<A> annotationClass,
+                                                                        String field) {
+        return ExpressionResult.PASS;
+    }
+
     @Override
     public void executeCommitChecks() {
 

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

@@ -97,6 +97,14 @@ public static PermissionCondition create(
         this.field = Optional.empty();
     }
 
+    PermissionCondition(Class<? extends Annotation> permission, Class<?> entityClass, String field) {
+        this.permission = permission;
+        this.resource = Optional.empty();
+        this.entityClass = entityClass;
+        this.changes = Optional.empty();
+        this.field = Optional.of(field);
+    }
+
     PermissionCondition(Class<? extends Annotation> permission, PersistentResource resource, String field) {
         this.permission = permission;
         this.resource = Optional.of(resource);

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

@@ -124,22 +124,26 @@ public <A extends Annotation> Expression buildAnyFieldExpressions(final Persiste
      * <p>
      * NOTE: This method returns _NO_ commit checks.
      *
-     * @param resource        Resource
+     * @param resourceClass   Resource Class
+     * @param scope           The request scope.
      * @param annotationClass Annotation class
      * @param field           Field to check (if null only check entity-level)
      * @param <A>             type parameter
      * @return User check expression to evaluate
      */
-    public <A extends Annotation> Expression buildUserCheckFieldExpressions(final PersistentResource resource,
+    public <A extends Annotation> Expression buildUserCheckFieldExpressions(final Class<?> resourceClass,
+                                                                             final RequestScope scope,
                                                                              final Class<A> annotationClass,
                                                                              final String field) {
-        Class<?> resourceClass = resource.getResourceClass();
         if (!entityDictionary.entityHasChecksForPermission(resourceClass, annotationClass)) {
             return SUCCESSFUL_EXPRESSION;
         }
 
-        final Function<Check, Expression> leafBuilderFn = leafBuilder(resource, null);
-        return buildSpecificFieldExpression(new PermissionCondition(annotationClass, resource, field), leafBuilderFn);
+        final Function<Check, Expression> leafBuilderFn = (check) ->
+                new CheckExpression(check, null, scope, null, cache);
+
+        return buildSpecificFieldExpression(new PermissionCondition(annotationClass, resourceClass, field),
+                leafBuilderFn);
     }
 
     /**

elide-core/src/test/java/com/yahoo/elide/core/VerifyFieldAccessFilterExpressionVisitorTest.java

@@ -7,7 +7,9 @@
 
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -16,11 +18,14 @@
 import com.yahoo.elide.core.exceptions.ForbiddenAccessException;
 import com.yahoo.elide.core.filter.FilterPredicate;
 import com.yahoo.elide.core.filter.InPredicate;
+import com.yahoo.elide.core.filter.dialect.RSQLFilterDialect;
 import com.yahoo.elide.core.filter.expression.AndFilterExpression;
+import com.yahoo.elide.core.filter.expression.FilterExpression;
 import com.yahoo.elide.core.filter.expression.NotFilterExpression;
 import com.yahoo.elide.core.filter.expression.OrFilterExpression;
 import com.yahoo.elide.security.PermissionExecutor;
 
+import com.yahoo.elide.security.permissions.ExpressionResult;
 import example.Author;
 import example.Book;
 
@@ -158,4 +163,77 @@ public void testReject() throws Exception {
 
         verify(permissionExecutor, times(5)).checkSpecificFieldPermissions(resource, null, ReadPermission.class, HOME);
     }
+
+    @Test
+    public void testShortCircuitReject() throws Exception {
+        RSQLFilterDialect dialect = new RSQLFilterDialect(scope.getDictionary());
+        FilterExpression expression =
+                dialect.parseFilterExpression("genre==foo", Book.class, true);
+
+        Book book = new Book();
+        PersistentResource<Book> resource = new PersistentResource<>(book, null, "", scope);
+
+        PermissionExecutor permissionExecutor = scope.getPermissionExecutor();
+
+        when(permissionExecutor.checkUserPermissions(Book.class, ReadPermission.class, GENRE))
+                .thenThrow(ForbiddenAccessException.class);
+
+        VerifyFieldAccessFilterExpressionVisitor visitor = new VerifyFieldAccessFilterExpressionVisitor(resource);
+        // restricted HOME field
+        assertFalse(expression.accept(visitor));
+
+        verify(permissionExecutor, times(1)).checkUserPermissions(Book.class, ReadPermission.class, GENRE);
+        verify(permissionExecutor, never()).checkSpecificFieldPermissions(resource, null, ReadPermission.class, GENRE);
+    }
+
+    @Test
+    public void testShortCircuitDeferred() throws Exception {
+        RSQLFilterDialect dialect = new RSQLFilterDialect(scope.getDictionary());
+        FilterExpression expression =
+                dialect.parseFilterExpression("genre==foo", Book.class, true);
+
+        Book book = new Book();
+        PersistentResource<Book> resource = new PersistentResource<>(book, null, "", scope);
+
+        PermissionExecutor permissionExecutor = scope.getPermissionExecutor();
+
+        when(permissionExecutor.checkUserPermissions(Book.class, ReadPermission.class, GENRE))
+                .thenReturn(ExpressionResult.DEFERRED);
+        when(permissionExecutor.checkSpecificFieldPermissions(resource, null, ReadPermission.class, GENRE))
+                .thenThrow(ForbiddenAccessException.class);
+
+        VerifyFieldAccessFilterExpressionVisitor visitor = new VerifyFieldAccessFilterExpressionVisitor(resource);
+        // restricted HOME field
+        assertFalse(expression.accept(visitor));
+
+        verify(permissionExecutor, times(1)).checkUserPermissions(Book.class, ReadPermission.class, GENRE);
+        verify(permissionExecutor, times(1)).checkSpecificFieldPermissions(resource, null, ReadPermission.class, GENRE);
+    }
+
+    @Test
+    public void testShortCircuitPass() throws Exception {
+        RSQLFilterDialect dialect = new RSQLFilterDialect(scope.getDictionary());
+        FilterExpression expression =
+                dialect.parseFilterExpression("authors.name==foo", Book.class, true);
+
+        Book book = new Book();
+        PersistentResource<Book> resource = new PersistentResource<>(book, null, "", scope);
+
+        PermissionExecutor permissionExecutor = scope.getPermissionExecutor();
+        DataStoreTransaction tx = scope.getTransaction();
+
+        when(permissionExecutor.checkUserPermissions(Book.class, ReadPermission.class, "authors"))
+                .thenReturn(ExpressionResult.PASS);
+        when(permissionExecutor.checkUserPermissions(Author.class, ReadPermission.class, NAME))
+                .thenReturn(ExpressionResult.PASS);
+
+        VerifyFieldAccessFilterExpressionVisitor visitor = new VerifyFieldAccessFilterExpressionVisitor(resource);
+        // restricted HOME field
+        assertTrue(expression.accept(visitor));
+
+        verify(permissionExecutor, times(1)).checkUserPermissions(Book.class, ReadPermission.class, "authors");
+        verify(permissionExecutor, times(1)).checkUserPermissions(Author.class, ReadPermission.class, "name");
+        verify(permissionExecutor, never()).checkSpecificFieldPermissions(resource, null, ReadPermission.class, GENRE);
+        verify(tx, never()).getRelation(any(), any(), any(), any(), any(), any(), any());
+    }
 }

elide-core/src/test/java/com/yahoo/elide/security/PermissionExecutorTest.java

@@ -5,6 +5,7 @@
  */
 package com.yahoo.elide.security;
 
+import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import com.yahoo.elide.ElideSettings;
@@ -21,6 +22,7 @@
 import com.yahoo.elide.security.checks.OperationCheck;
 import com.yahoo.elide.security.checks.UserCheck;
 
+import com.yahoo.elide.security.permissions.ExpressionResult;
 import example.TestCheckMappings;
 
 import org.junit.jupiter.api.Test;
@@ -413,6 +415,40 @@ public void testUserCheckCache() {
         requestScope.getPermissionExecutor().checkPermission(ReadPermission.class, resource, cspec);
     }
 
+    @Test
+    public void testUserCheckOnFieldSuccess() {
+        PersistentResource resource = newResource(OpenBean.class);
+        RequestScope requestScope = resource.getRequestScope();
+        ExpressionResult result = requestScope.getPermissionExecutor().checkUserPermissions(OpenBean.class,
+                ReadPermission.class,
+                "open");
+
+        assertEquals(ExpressionResult.PASS, result);
+    }
+
+    @Test
+    public void testUserCheckOnFieldFailure() {
+        PersistentResource resource = newResource(SampleBean.class);
+        RequestScope requestScope = resource.getRequestScope();
+        assertThrows(
+                ForbiddenAccessException.class,
+                () -> requestScope.getPermissionExecutor().checkUserPermissions(SampleBean.class,
+                ReadPermission.class,
+                "cannotSeeMe"));
+    }
+
+    @Test
+    public void testUserCheckOnFieldDeferred() {
+        PersistentResource resource = newResource(SampleBean.class);
+        RequestScope requestScope = resource.getRequestScope();
+
+        ExpressionResult result = requestScope.getPermissionExecutor().checkUserPermissions(SampleBean.class,
+                ReadPermission.class,
+                "allVisible");
+
+        assertEquals(ExpressionResult.DEFERRED, result);
+    }
+
     public <T> PersistentResource<T> newResource(T obj, Class<T> cls) {
         EntityDictionary dictionary = new EntityDictionary(TestCheckMappings.MAPPINGS);
         dictionary.bindEntity(cls);