Add AggregationStorePermissionExecutor #2086
Conversation
| public <A extends Annotation> ExpressionResult checkPermission(Class<A> annotationClass, | ||
| PersistentResource resource, | ||
| Set<String> requestedFields) { | ||
| return ExpressionResult.PASS; |
There was a problem hiding this comment.
This would be cleaner if you check user checks on the requested fields for the requested resource.
| ChangeSpec changeSpec, | ||
| Class<A> annotationClass, | ||
| String field) { | ||
| return null; |
There was a problem hiding this comment.
How about throwing UnsupportedOperationException here?
| */ | ||
| public FilterExpression buildEntityFilterExpression(Type<?> forType, RequestScope requestScope) { | ||
| Class<? extends Annotation> annotationClass = ReadPermission.class; | ||
| ParseTree classPermissions = entityDictionary.getPermissionsForClass(forType, annotationClass); |
There was a problem hiding this comment.
YOu can combine this line and the one above.
| Class<A> annotationClass, | ||
| Set<String> requestedFields) { | ||
| Supplier<Expression> expressionSupplier = () -> | ||
| expressionBuilder.buildUserCheckAnyExpression( |
There was a problem hiding this comment.
This needs to change to match the RFC
(entityRule AND (field1Rule OR field2Rule ... OR fieldNRule)
| @Override | ||
| public <A extends Annotation> ExpressionResult checkPermission(Class<A> annotationClass, | ||
| PersistentResource resource, | ||
| Set<String> requestedFields) { | ||
| return ExpressionResult.PASS; | ||
| Expression expression = expressionBuilder.buildUserCheckAnyExpression( |
There was a problem hiding this comment.
Can we add a Preconditions check or maybe return FAIL if annotationClass is anything other than a ReadPermission for all of these checks?
| ParseTree classPermissions = entityDictionary.getPermissionsForClass(resourceClass, annotationClass); | ||
| Expression entityExpression = normalizedExpressionFromParseTree(classPermissions, leafBuilderFn); | ||
|
|
||
| Expression anyFiledExpression = buildAnyFieldExpression( |
| requestedFields, | ||
| requestScope); | ||
|
|
||
| return executeExpressions(expression, annotationClass, Expression.EvaluationMode.ALL_CHECKS); |
There was a problem hiding this comment.
This should actually call checkPermissions with expressionExecutor set to Optional.empty(). That's how user only checks are evaluated.
There was a problem hiding this comment.
Alternatively, call checkOnlyUserPermissions like elsewhere in this file.
| return checkOnlyUserPermissions( | ||
| resourceClass, | ||
| annotationClass, | ||
| Optional.empty(), |
There was a problem hiding this comment.
I found a bug in the user check cache in Elide 5. The user check cache will look at either one field or no fields (this case). However, this check could be called multiple times with different fields. We really ought to make the cache always map to a set of requested fields. Optional.empty() isn't right here, but we can't do the right thing until this bug is fixed.
|
|
||
| // evaluated expression = (user none or filter check) | ||
| Assertions.assertEquals( | ||
| ExpressionResult.DEFERRED, |
There was a problem hiding this comment.
This doesn't seem right to me. The filter check should never apply at the field level.
There was a problem hiding this comment.
The field level check should not inherit the entity level check here.
|
|
||
| // evaluated expression = (user none or filter check) AND (user all OR user none) | ||
| Assertions.assertEquals( | ||
| ExpressionResult.DEFERRED, |
There was a problem hiding this comment.
I'm not sure this tests makes sense either. Let's discuss. I don't think DEFERRED is a valid return scenario.
| com.yahoo.elide.core.RequestScope scope = bindAndgetRequestScope(Model1.class); | ||
| PermissionExecutor executor = scope.getPermissionExecutor(); | ||
|
|
||
| FilterExpression expression = executor.getReadPermissionFilter(ClassType.of(Model1.class), Collections.emptySet()).orElse(null); |
There was a problem hiding this comment.
To make these tests realistic, the set of requested fields should not be empty - but rather set to the all the fields.
| return ExpressionResult.FAIL; | ||
| } | ||
|
|
||
| Expression expression = expressionBuilder.buildUserCheckAnyExpression( |
There was a problem hiding this comment.
I'm not sure this is what you want here from the Permission Builder. UserCheckAnyExpression will look at a combination of field and entity for each field.
| Assertions.assertEquals( | ||
| ExpressionResult.DEFERRED, | ||
| ExpressionResult.PASS, |
| @@ -130,6 +129,13 @@ public long getMetric() { | |||
| new PersistentResource(new Model("dim1", 0, 1), "1", scope), | |||
| new HashSet<>(Arrays.asList("filterDim", "metric")))); | |||
|
|
|||
| // evaluated expression = (user none OR null) | |||
| Assertions.assertEquals( | |||
| ExpressionResult.PASS, | |||
Resolves #2048
Description
Add AggregationStorePermissionExecutor for all models managed by Aggregation Datastore
License
I confirm that this contribution is made under an Apache 2.0 license and that I have the authority necessary to make this contribution on behalf of its copyright owner.