Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Short Circuit Filter Expression Security If User Checks would pass/fail. #1275

Merged
merged 9 commits into from
Apr 21, 2020
Original file line number Diff line number Diff line change
@@ -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();
Original file line number Diff line number Diff line change
@@ -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.
*
Original file line number Diff line number Diff line change
@@ -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);
}
}
Original file line number Diff line number Diff line change
@@ -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() {

Original file line number Diff line number Diff line change
@@ -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);
Original file line number Diff line number Diff line change
@@ -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);
}

/**
Original file line number Diff line number Diff line change
@@ -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());
}
}
Original file line number Diff line number Diff line change
@@ -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);