From 345a293c8fbc01cc39735bc782f91fda3137487a Mon Sep 17 00:00:00 2001 From: Joel Bartelheimer Date: Wed, 8 Sep 2021 18:57:58 +0200 Subject: [PATCH] [#559] Add unit tests for new field name visitor and little refactoring * Add JavaDocs * Add static factory * return unmodifiable set * better namings * and other minor simplifications. Signed-off-by: Joel Bartelheimer --- .../enforcement/ThingCommandEnforcement.java | 44 ++++++++--------- .../things/FieldNamesPredicateVisitor.java | 18 ++++++- .../FieldNamesPredicateVisitorTest.java | 47 +++++++++++++++++++ 3 files changed, 86 insertions(+), 23 deletions(-) create mode 100644 rql/query/src/test/java/org/eclipse/ditto/rql/query/things/FieldNamesPredicateVisitorTest.java diff --git a/concierge/service/src/main/java/org/eclipse/ditto/concierge/service/enforcement/ThingCommandEnforcement.java b/concierge/service/src/main/java/org/eclipse/ditto/concierge/service/enforcement/ThingCommandEnforcement.java index 237344b1dd..fbf073acc2 100644 --- a/concierge/service/src/main/java/org/eclipse/ditto/concierge/service/enforcement/ThingCommandEnforcement.java +++ b/concierge/service/src/main/java/org/eclipse/ditto/concierge/service/enforcement/ThingCommandEnforcement.java @@ -786,11 +786,10 @@ static > T authorizeByPolicyOrThrow(final Enforcer pol policyEnforcer.hasPartialPermissions(thingResourceKey, authorizationContext, Permission.READ); } - dittoHeaders.getCondition().ifPresent(condition -> { - if (!(command instanceof CreateThing)) { - enforceReadPermissionOnCondition(condition, policyEnforcer, authorizationContext, dittoHeaders); - } - }); + final var condition = dittoHeaders.getCondition(); + if (!(command instanceof CreateThing) && condition.isPresent()) { + enforceReadPermissionOnCondition(condition.get(), policyEnforcer, dittoHeaders); + } if (commandAuthorized) { return AbstractEnforcement.addEffectedReadSubjectsToThingSignal(command, policyEnforcer); @@ -801,18 +800,19 @@ static > T authorizeByPolicyOrThrow(final Enforcer pol private static void enforceReadPermissionOnCondition(final String condition, final Enforcer policyEnforcer, - final AuthorizationContext authorizationContext, final DittoHeaders dittoHeaders) { - final RootNode rootNode = validateCondition(condition, dittoHeaders); - final Set resourceKeyList = determineResourceKeys(rootNode, dittoHeaders); - if (!policyEnforcer.hasUnrestrictedPermissions(resourceKeyList, authorizationContext, Permission.READ)) { + final var authorizationContext = dittoHeaders.getAuthorizationContext(); + final var rootNode = tryParseRqlCondition(condition, dittoHeaders); + final var resourceKey = determineResourceKeys(rootNode, dittoHeaders); + + if (!policyEnforcer.hasUnrestrictedPermissions(resourceKey, authorizationContext, Permission.READ)) { throw ThingConditionFailedException.newBuilderForInsufficientPermission(condition) .build(); } } - private static RootNode validateCondition(final String condition, final DittoHeaders dittoHeaders) { + private static RootNode tryParseRqlCondition(final String condition, final DittoHeaders dittoHeaders) { try { return RqlPredicateParser.getInstance().parse(condition); } catch (final ParserException e) { @@ -823,23 +823,25 @@ private static RootNode validateCondition(final String condition, final DittoHea } private static Set determineResourceKeys(final RootNode rootNode, final DittoHeaders dittoHeaders) { - final FieldNamesPredicateVisitor visitor = new FieldNamesPredicateVisitor(); + final var visitor = FieldNamesPredicateVisitor.getNewInstance(); visitor.visit(rootNode); - final Set extractedFieldNames = visitor.getFieldNames(); + final var extractedFieldNames = visitor.getFieldNames(); return extractedFieldNames.stream() - .map(fieldName -> { - try { - return PoliciesResourceType.thingResource(fieldName); - } catch (JsonPointerInvalidException e) { - throw ThingConditionInvalidException.newBuilder(fieldName, e.getDescription().orElse("")) - .dittoHeaders(dittoHeaders) - .build(); - } - }) + .map(fieldName -> tryGetResourceKey(fieldName, dittoHeaders)) .collect(Collectors.toSet()); } + private static ResourceKey tryGetResourceKey(final String fieldName, final DittoHeaders dittoHeaders) { + try { + return PoliciesResourceType.thingResource(fieldName); + } catch (final JsonPointerInvalidException e) { + throw ThingConditionInvalidException.newBuilder(fieldName, e.getDescription().orElse("")) + .dittoHeaders(dittoHeaders) + .build(); + } + } + private static boolean enforceMergeThingCommand(final Enforcer policyEnforcer, final MergeThing command, final ResourceKey thingResourceKey, final AuthorizationContext authorizationContext) { diff --git a/rql/query/src/main/java/org/eclipse/ditto/rql/query/things/FieldNamesPredicateVisitor.java b/rql/query/src/main/java/org/eclipse/ditto/rql/query/things/FieldNamesPredicateVisitor.java index 8306ec1e8b..cc51df29fd 100644 --- a/rql/query/src/main/java/org/eclipse/ditto/rql/query/things/FieldNamesPredicateVisitor.java +++ b/rql/query/src/main/java/org/eclipse/ditto/rql/query/things/FieldNamesPredicateVisitor.java @@ -12,6 +12,7 @@ */ package org.eclipse.ditto.rql.query.things; +import java.util.Collections; import java.util.HashSet; import java.util.Set; @@ -31,10 +32,23 @@ @NotThreadSafe public final class FieldNamesPredicateVisitor implements PredicateVisitor { - private final Set fieldNames = new HashSet<>(); + private final Set fieldNames; + private FieldNamesPredicateVisitor() { + fieldNames = new HashSet<>(); + } + + public static FieldNamesPredicateVisitor getNewInstance() { + return new FieldNamesPredicateVisitor(); + } + + /** + * Returns all names from all fields visited by this visitor. + * + * @return all field names. + */ public Set getFieldNames() { - return fieldNames; + return Collections.unmodifiableSet(fieldNames); } @Override diff --git a/rql/query/src/test/java/org/eclipse/ditto/rql/query/things/FieldNamesPredicateVisitorTest.java b/rql/query/src/test/java/org/eclipse/ditto/rql/query/things/FieldNamesPredicateVisitorTest.java new file mode 100644 index 0000000000..7d3360c531 --- /dev/null +++ b/rql/query/src/test/java/org/eclipse/ditto/rql/query/things/FieldNamesPredicateVisitorTest.java @@ -0,0 +1,47 @@ +/* + * Copyright (c) 2021 Contributors to the Eclipse Foundation + * + * See the NOTICE file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License 2.0 which is available at + * http://www.eclipse.org/legal/epl-2.0 + * + * SPDX-License-Identifier: EPL-2.0 + */ +package org.eclipse.ditto.rql.query.things; + +import java.util.Set; + +import org.assertj.core.api.JUnitSoftAssertions; +import org.eclipse.ditto.rql.parser.internal.RqlPredicateParser; +import org.junit.Rule; +import org.junit.Test; + +/** + * Unit test for {@link FieldNamesPredicateVisitor} + */ +public final class FieldNamesPredicateVisitorTest { + + @Rule + public final JUnitSoftAssertions softly = new JUnitSoftAssertions(); + + @Test + public void verifyCorrectFiltering() { + softly.assertThat(extractFieldsFromRql("and(eq(attributes/foo,1),exists(_created))")) + .containsExactlyInAnyOrder("attributes/foo", "_created"); + softly.assertThat(extractFieldsFromRql("or(exists(attributes/foo),exists(attributes/foo2))")) + .containsExactlyInAnyOrder("attributes/foo", "attributes/foo2"); + softly.assertThat(extractFieldsFromRql("eq(features/*/definition,\"t:t:1\")")) + .containsExactlyInAnyOrder("features/*/definition"); + softly.assertThat(extractFieldsFromRql("eq(/attributes/complex,\"!#$%&'(features)*+,/:;=?@[\\\\]{|}\\\" äaZ0\")")) + .containsExactlyInAnyOrder("/attributes/complex"); + } + + private static Set extractFieldsFromRql(final String filter) { + final FieldNamesPredicateVisitor fieldNameVisitor = FieldNamesPredicateVisitor.getNewInstance(); + fieldNameVisitor.visit(RqlPredicateParser.parse(filter)); + return fieldNameVisitor.getFieldNames(); + } +}