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
Expand Up @@ -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;
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Up @@ -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() {

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

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -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
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down