[#559] check read permission on all resource keys specified in the co…

…ndition;

add unit test to ThingCommandEnforcementTest;

Signed-off-by: Stefan Maute <stefan.maute@bosch.io>

concierge/service/pom.xml

@@ -58,6 +58,14 @@
             <groupId>org.eclipse.ditto</groupId>
             <artifactId>ditto-concierge-api</artifactId>
         </dependency>
+        <dependency>
+            <groupId>org.eclipse.ditto</groupId>
+            <artifactId>ditto-rql-query</artifactId>
+        </dependency>
+        <dependency>
+            <groupId>org.eclipse.ditto</groupId>
+            <artifactId>ditto-rql-parser</artifactId>
+        </dependency>
 
         <dependency>
             <groupId>com.typesafe.akka</groupId>

concierge/service/src/main/java/org/eclipse/ditto/concierge/service/enforcement/ThingCommandEnforcement.java

@@ -15,13 +15,15 @@
 import static java.util.Objects.requireNonNull;
 import static org.eclipse.ditto.policies.api.Permission.MIN_REQUIRED_POLICY_PERMISSIONS;
 
+import java.text.MessageFormat;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CompletionStage;
 import java.util.function.BiFunction;
+import java.util.stream.Collectors;
 
 import javax.annotation.Nullable;
 
@@ -78,6 +80,8 @@
 import org.eclipse.ditto.policies.model.signals.commands.modify.CreatePolicyResponse;
 import org.eclipse.ditto.policies.model.signals.commands.query.RetrievePolicy;
 import org.eclipse.ditto.policies.model.signals.commands.query.RetrievePolicyResponse;
+import org.eclipse.ditto.rql.parser.internal.RqlPredicateParser;
+import org.eclipse.ditto.rql.query.things.FieldNamesPredicateVisitor;
 import org.eclipse.ditto.things.model.Thing;
 import org.eclipse.ditto.things.model.ThingConstants;
 import org.eclipse.ditto.things.model.ThingId;
@@ -86,6 +90,7 @@
 import org.eclipse.ditto.things.model.signals.commands.exceptions.PolicyInvalidException;
 import org.eclipse.ditto.things.model.signals.commands.exceptions.ThingCommandToAccessExceptionRegistry;
 import org.eclipse.ditto.things.model.signals.commands.exceptions.ThingCommandToModifyExceptionRegistry;
+import org.eclipse.ditto.things.model.signals.commands.exceptions.ThingConditionFailedException;
 import org.eclipse.ditto.things.model.signals.commands.exceptions.ThingNotAccessibleException;
 import org.eclipse.ditto.things.model.signals.commands.exceptions.ThingNotCreatableException;
 import org.eclipse.ditto.things.model.signals.commands.exceptions.ThingNotModifiableException;
@@ -235,7 +240,7 @@ private static boolean isResponseRequired(final WithDittoHeaders withDittoHeader
      * Authorize a thing command by policy enforcer with view restriction for query commands.
      *
      * @param thingCommand the thing command to authorize.
-     * @param policyId Id of the thing's policy.
+     * @param policyId the ID of the thing's policy.
      * @param enforcer the policy enforcer.
      * @return the contextual including message and receiver
      */
@@ -269,7 +274,7 @@ private Contextual<WithDittoHeaders> enforceThingCommandByPolicyEnforcer(
      * Retrieve a thing and its policy and combine them into a response.
      *
      * @param retrieveThing the retrieve-thing command.
-     * @param policyId ID of the thing's policy.
+     * @param policyId the ID of the thing's policy.
      * @param enforcer the enforcer for the command.
      * @return always {@code true}.
      */
@@ -765,25 +770,55 @@ static <T extends ThingCommand<T>> T authorizeByPolicyOrThrow(final Enforcer pol
         final var thingResourceKey = PoliciesResourceType.thingResource(command.getResourcePath());
         final var dittoHeaders = command.getDittoHeaders();
         final var authorizationContext = dittoHeaders.getAuthorizationContext();
+        final var condition = dittoHeaders.getCondition().orElse(null);
 
-        final boolean authorized;
+        final boolean commandAuthorized;
         if (command instanceof MergeThing) {
-            authorized = enforceMergeThingCommand(policyEnforcer, (MergeThing) command, thingResourceKey,
+            commandAuthorized = enforceMergeThingCommand(policyEnforcer, (MergeThing) command, thingResourceKey,
                     authorizationContext);
         } else if (command instanceof ThingModifyCommand) {
-            authorized = policyEnforcer.hasUnrestrictedPermissions(thingResourceKey, authorizationContext,
+            commandAuthorized = policyEnforcer.hasUnrestrictedPermissions(thingResourceKey, authorizationContext,
                     Permission.WRITE);
         } else {
-            authorized = policyEnforcer.hasPartialPermissions(thingResourceKey, authorizationContext, Permission.READ);
+            commandAuthorized =
+                    policyEnforcer.hasPartialPermissions(thingResourceKey, authorizationContext, Permission.READ);
         }
 
-        if (authorized) {
+        if (condition != null && !(command instanceof CreateThing)) {
+            enforceReadPermissionOnCondition(condition, policyEnforcer, authorizationContext);
+        }
+
+        if (commandAuthorized ) {
             return AbstractEnforcement.addEffectedReadSubjectsToThingSignal(command, policyEnforcer);
         } else {
             throw errorForThingCommand(command);
         }
     }
 
+    private static void enforceReadPermissionOnCondition(final String condition,
+            final Enforcer policyEnforcer,
+            final AuthorizationContext authorizationContext) {
+
+        final Set<ResourceKey> resourceKeyList = determineResourceKeys(condition);
+        if(!policyEnforcer.hasUnrestrictedPermissions(resourceKeyList, authorizationContext, Permission.READ)) {
+            throw ThingConditionFailedException.newBuilder(condition)
+                    .message(MessageFormat.format("The resource specified in the condition ''{0}'' could not be " +
+                            "found or the requester had insufficient permissions to access it.", condition))
+                    .description("Check if the ID of your requested Thing was correct and you have sufficient permissions.")
+                    .build();
+        }
+    }
+
+    private static Set<ResourceKey> determineResourceKeys(final String condition) {
+        final FieldNamesPredicateVisitor visitor = new FieldNamesPredicateVisitor();
+        visitor.visit(RqlPredicateParser.parse(condition));
+        final Set<String> extractedFieldNames = visitor.getFieldNames();
+
+        return extractedFieldNames.stream()
+                .map(PoliciesResourceType::thingResource)
+                .collect(Collectors.toSet());
+    }
+
     private static boolean enforceMergeThingCommand(final Enforcer policyEnforcer,
             final MergeThing command, final ResourceKey thingResourceKey,
             final AuthorizationContext authorizationContext) {

concierge/service/src/test/java/org/eclipse/ditto/concierge/service/enforcement/ThingCommandEnforcementTest.java

@@ -22,43 +22,45 @@
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
 
-import org.eclipse.ditto.json.JsonFactory;
-import org.eclipse.ditto.json.JsonObject;
-import org.eclipse.ditto.json.JsonPointer;
-import org.eclipse.ditto.json.JsonValue;
-import org.eclipse.ditto.json.assertions.DittoJsonAssertions;
 import org.eclipse.ditto.base.model.auth.AuthorizationContext;
 import org.eclipse.ditto.base.model.auth.AuthorizationSubject;
 import org.eclipse.ditto.base.model.auth.DittoAuthorizationContextType;
+import org.eclipse.ditto.base.model.common.HttpStatus;
 import org.eclipse.ditto.base.model.exceptions.DittoRuntimeException;
 import org.eclipse.ditto.base.model.headers.DittoHeaders;
 import org.eclipse.ditto.base.model.headers.entitytag.EntityTagMatcher;
 import org.eclipse.ditto.base.model.headers.entitytag.EntityTagMatchers;
 import org.eclipse.ditto.base.model.json.FieldType;
 import org.eclipse.ditto.base.model.json.JsonSchemaVersion;
+import org.eclipse.ditto.json.JsonFactory;
+import org.eclipse.ditto.json.JsonObject;
+import org.eclipse.ditto.json.JsonPointer;
+import org.eclipse.ditto.json.JsonValue;
+import org.eclipse.ditto.json.assertions.DittoJsonAssertions;
+import org.eclipse.ditto.policies.api.commands.sudo.SudoRetrievePolicy;
+import org.eclipse.ditto.policies.api.commands.sudo.SudoRetrievePolicyResponse;
 import org.eclipse.ditto.policies.model.Permissions;
 import org.eclipse.ditto.policies.model.PoliciesModelFactory;
 import org.eclipse.ditto.policies.model.PoliciesResourceType;
 import org.eclipse.ditto.policies.model.Policy;
 import org.eclipse.ditto.policies.model.PolicyId;
 import org.eclipse.ditto.policies.model.PolicyIdInvalidException;
-import org.eclipse.ditto.things.model.Feature;
-import org.eclipse.ditto.things.model.Thing;
-import org.eclipse.ditto.things.model.ThingBuilder;
-import org.eclipse.ditto.things.model.ThingsModelFactory;
-import org.eclipse.ditto.policies.api.commands.sudo.SudoRetrievePolicy;
-import org.eclipse.ditto.policies.api.commands.sudo.SudoRetrievePolicyResponse;
-import org.eclipse.ditto.things.api.Permission;
-import org.eclipse.ditto.things.api.commands.sudo.SudoRetrieveThing;
-import org.eclipse.ditto.things.api.commands.sudo.SudoRetrieveThingResponse;
 import org.eclipse.ditto.policies.model.signals.commands.exceptions.PolicyNotAccessibleException;
 import org.eclipse.ditto.policies.model.signals.commands.modify.CreatePolicy;
 import org.eclipse.ditto.policies.model.signals.commands.modify.CreatePolicyResponse;
 import org.eclipse.ditto.policies.model.signals.commands.query.RetrievePolicy;
 import org.eclipse.ditto.policies.model.signals.commands.query.RetrievePolicyResponse;
+import org.eclipse.ditto.things.api.Permission;
+import org.eclipse.ditto.things.api.commands.sudo.SudoRetrieveThing;
+import org.eclipse.ditto.things.api.commands.sudo.SudoRetrieveThingResponse;
+import org.eclipse.ditto.things.model.Feature;
+import org.eclipse.ditto.things.model.Thing;
+import org.eclipse.ditto.things.model.ThingBuilder;
+import org.eclipse.ditto.things.model.ThingsModelFactory;
 import org.eclipse.ditto.things.model.signals.commands.ThingCommand;
 import org.eclipse.ditto.things.model.signals.commands.exceptions.FeatureNotModifiableException;
 import org.eclipse.ditto.things.model.signals.commands.exceptions.PolicyInvalidException;
+import org.eclipse.ditto.things.model.signals.commands.exceptions.ThingConditionFailedException;
 import org.eclipse.ditto.things.model.signals.commands.exceptions.ThingNotAccessibleException;
 import org.eclipse.ditto.things.model.signals.commands.exceptions.ThingNotModifiableException;
 import org.eclipse.ditto.things.model.signals.commands.modify.CreateThing;
@@ -664,6 +666,45 @@ public void transformModifyThingToCreateThing() {
         }};
     }
 
+    @Test
+    public void rejectModifyWithConditionAndNoReadPermissionForAttributes() {
+        final PolicyId policyId = PolicyId.of("policy:id");
+        final JsonObject thingWithPolicy = newThingWithAttributeWithPolicyId(policyId);
+        final JsonPointer attributePointer = JsonPointer.of("/attributes/testAttr");
+        final JsonObject policy = PoliciesModelFactory.newPolicyBuilder(policyId)
+                .setRevision(1L)
+                .forLabel("authorize-self")
+                .setSubject(GOOGLE, TestSetup.SUBJECT_ID)
+                .setGrantedPermissions(PoliciesResourceType.thingResource(JsonPointer.empty()),
+                        Permissions.newInstance(Permission.READ, Permission.WRITE))
+                .setRevokedPermissions(PoliciesResourceType.thingResource(attributePointer),
+                        Permissions.newInstance(Permission.READ))
+                .build()
+                .toJson(FieldType.all());
+        final SudoRetrieveThingResponse sudoRetrieveThingResponse =
+                SudoRetrieveThingResponse.of(thingWithPolicy, DittoHeaders.empty());
+        final SudoRetrievePolicyResponse sudoRetrievePolicyResponse =
+                SudoRetrievePolicyResponse.of(policyId, policy, DittoHeaders.empty());
+
+        new TestKit(system) {{
+            mockEntitiesActorInstance.setReply(TestSetup.THING_SUDO, sudoRetrieveThingResponse);
+            mockEntitiesActorInstance.setReply(TestSetup.POLICY_SUDO, sudoRetrievePolicyResponse);
+
+            final ActorRef underTest = newEnforcerActor(getRef());
+
+            final DittoHeaders dittoHeaders = headers(V_2).toBuilder()
+                    .condition("eq(attributes/testAttr,\"testString\")")
+                    .build();
+
+            final ThingCommand modifyCommand = getModifyCommand(dittoHeaders);
+            mockEntitiesActorInstance.setReply(modifyCommand);
+            underTest.tell(modifyCommand, getRef());
+            final DittoRuntimeException response = TestSetup.fishForMsgClass(this, DittoRuntimeException.class);
+            assertThat(response.getErrorCode()).isEqualTo(ThingConditionFailedException.ERROR_CODE);
+            assertThat(response.getHttpStatus()).isEqualTo(HttpStatus.PRECONDITION_FAILED);
+        }};
+    }
+
     private ActorRef newEnforcerActor(final ActorRef testActorRef) {
         return TestSetup.newEnforcerActor(system, testActorRef, mockEntitiesActor);
     }
@@ -711,7 +752,11 @@ private ThingCommand getReadCommand() {
     }
 
     private ThingCommand getModifyCommand() {
-        return ModifyFeature.of(TestSetup.THING_ID, Feature.newBuilder().withId("x").build(), headers(V_2));
+        return getModifyCommand( headers(V_2));
+    }
+
+    private ThingCommand getModifyCommand(final DittoHeaders dittoHeaders) {
+        return ModifyFeature.of(TestSetup.THING_ID, Feature.newBuilder().withId("x").build(), dittoHeaders);
     }
 
 }

policies/model/src/main/java/org/eclipse/ditto/policies/model/enforcers/Enforcer.java

@@ -14,12 +14,12 @@
 
 import java.util.Set;
 
+import org.eclipse.ditto.base.model.auth.AuthorizationContext;
+import org.eclipse.ditto.base.model.auth.AuthorizationSubject;
 import org.eclipse.ditto.json.JsonFactory;
 import org.eclipse.ditto.json.JsonField;
 import org.eclipse.ditto.json.JsonFieldSelector;
 import org.eclipse.ditto.json.JsonObject;
-import org.eclipse.ditto.base.model.auth.AuthorizationContext;
-import org.eclipse.ditto.base.model.auth.AuthorizationSubject;
 import org.eclipse.ditto.policies.model.Permissions;
 import org.eclipse.ditto.policies.model.ResourceKey;
 
@@ -55,7 +55,7 @@ default boolean hasUnrestrictedPermissions(final ResourceKey resourceKey,
      * Checks whether for the {@code authorizationContext} either implicitly or explicitly
      * has "GRANT" for the {@code permissions} on the specified set of {@code resourceKeys} considering "REVOKE"s
      * down in the hierarchy, so if there is a REVOKE for the {@code authorizationContext} somewhere down the hierarchy
-     * of any of the {@code resourceKeys}, the result will be {@code false}.
+     * for any of the {@code resourceKeys}, the result will be {@code false}.
      *
      * @param resourceKeys the ResourceKeys (containing Resource type and path) to check the permission(s) for.
      * @param authorizationContext the authorization context to check.

rql/model/src/main/java/org/eclipse/ditto/rql/model/predicates/ast/ComparisonNode.java

@@ -17,7 +17,7 @@
 import javax.annotation.Nullable;
 
 /**
- * Generic comparison node that has a type, a property to compare on and a generic value. Subclasses have to specifiy
+ * Generic comparison node that has a type, a property to compare on and a generic value. Subclasses have to specify
  * what type the value can have.
  *
  * @param <T> Type of the comparison enumeration.

things/service/src/main/java/org/eclipse/ditto/things/service/persistence/actors/strategies/commands/ThingConditionValidator.java

@@ -60,6 +60,8 @@ public static ThingConditionValidator getInstance() {
      */
     public Either<Void, ThingConditionFailedException> validate(final Command<?> command,
             @Nullable final String condition, @Nullable final Thing entity) {
+        checkNotNull(command, "Command");
+
         if (isDefined(command, entity) && (condition != null && entity != null)) {
                 final DittoHeaders dittoHeaders = command.getDittoHeaders();
                 final Criteria criteria = QueryFilterCriteriaFactory.modelBased(RqlPredicateParser.getInstance())
@@ -82,8 +84,6 @@ public Either<Void, ThingConditionFailedException> validate(final Command<?> com
      * @return @{code true} if the command should be applied otherwise @{code false}.
      */
     private boolean isDefined(final Command<?> command, @Nullable final Thing entity) {
-        checkNotNull(command, "Command");
-
         return !(command instanceof CreateThing) && entity != null;
     }