adjust review comments;

removed some duplicate log statements; only check for partial permission to enforce live events; Signed-off-by: Stefan Maute <stefan.maute@bosch.io>
eclipse-ditto · Sep 29, 2021 · 93858ef · 93858ef
1 parent c880317
commit 93858ef
Show file tree

Hide file tree

Showing 5 changed files with 41 additions and 68 deletions.
diff --git a/.../src/main/java/org/eclipse/ditto/concierge/service/enforcement/LiveSignalEnforcement.java b/.../src/main/java/org/eclipse/ditto/concierge/service/enforcement/LiveSignalEnforcement.java
@@ -179,23 +179,21 @@ private CompletionStage<Contextual<WithDittoHeaders>> enforceLiveCommandResponse
         final Optional<Cache<String, ActorRef>> responseReceiversOptional = context.getResponseReceivers();
         if (responseReceiversOptional.isPresent()) {
             final Cache<String, ActorRef> responseReceivers = responseReceiversOptional.get();
-            return returnWithMessageToReceiver(responseReceivers, liveResponse, correlationId, enforcer);
+            return returnCommandResponseContextual(responseReceivers, liveResponse, correlationId, enforcer);
         } else {
-            if (log().isDebugEnabled()) {
-                log().debug("Got live response when global dispatching is inactive: <{}>", liveResponse);
-            } else {
-                log().info("Got live response when global dispatching is inactive: <{}> with correlation ID <{}>",
+            log().info("Got live response when global dispatching is inactive: <{}> with correlation ID <{}>",
                         liveResponse.getType(),
                         liveResponse.getDittoHeaders().getCorrelationId().orElse(""));
-            }
+
             return CompletableFuture.completedFuture(withMessageToReceiver(null, null));
         }
     }
 
-    private CompletionStage<Contextual<WithDittoHeaders>> returnWithMessageToReceiver(
+    private CompletionStage<Contextual<WithDittoHeaders>> returnCommandResponseContextual(
             final Cache<String, ActorRef> responseReceivers, final CommandResponse<?> liveResponse,
             final String correlationId, final Enforcer enforcer) {
         return responseReceivers.get(correlationId).thenApply(responseReceiverEntry -> {
+            final Contextual<WithDittoHeaders> commandResponseContextual;
             if (responseReceiverEntry.isPresent()) {
                 responseReceivers.invalidate(correlationId);
                 final ActorRef responseReceiver = responseReceiverEntry.get();
@@ -206,18 +204,15 @@ private CompletionStage<Contextual<WithDittoHeaders>> returnWithMessageToReceive
                 } else {
                     response = liveResponse;
                 }
-
-                log().debug("Scheduling CommandResponse <{}> to original sender <{}>", liveResponse,
+                log().info("Scheduling CommandResponse <{}> to original sender <{}>", liveResponse,
                         responseReceiver);
-                return withMessageToReceiver(response, responseReceiver);
+                commandResponseContextual = withMessageToReceiver(response, responseReceiver);
             } else {
-                if (log().isDebugEnabled()) {
-                    log().debug("Got <{}> with unknown correlation ID: <{}>", liveResponse.getType(), liveResponse);
-                } else {
-                    log().info("Got <{}> with unknown correlation ID: <{}>", liveResponse.getType(), correlationId);
-                }
-                return withMessageToReceiver(null, null);
+                log().info("Got <{}> with unknown correlation ID: <{}>", liveResponse.getType(), correlationId);
+                commandResponseContextual = withMessageToReceiver(null, null);
             }
+
+            return commandResponseContextual;
         });
     }
 
@@ -248,7 +243,7 @@ private CompletionStage<Contextual<WithDittoHeaders>> enforceLiveSignal(final St
     private CompletionStage<Contextual<WithDittoHeaders>> enforceLiveEvent(final Signal<?> liveSignal,
             final Enforcer enforcer) {
 
-        final boolean authorized = enforcer.hasUnrestrictedPermissions(
+        final boolean authorized = enforcer.hasPartialPermissions(
                 PoliciesResourceType.thingResource(liveSignal.getResourcePath()),
                 liveSignal.getDittoHeaders().getAuthorizationContext(), WRITE);
 
@@ -277,7 +272,6 @@ static boolean isLiveSignal(final Signal<?> signal) {
 
     private CompletionStage<Contextual<WithDittoHeaders>> enforceMessageCommand(final MessageCommand<?, ?> command,
             final Enforcer enforcer) {
-
         if (isAuthorized(command, enforcer)) {
             return publishMessageCommand(command, enforcer);
         } else {

diff --git a/...rc/main/java/org/eclipse/ditto/concierge/service/enforcement/ThingCommandEnforcement.java b/...rc/main/java/org/eclipse/ditto/concierge/service/enforcement/ThingCommandEnforcement.java
@@ -127,7 +127,7 @@ public final class ThingCommandEnforcement
     /**
      * Json fields that are always shown regardless of authorization.
      */
-    public static final JsonFieldSelector THING_QUERY_COMMAND_RESPONSE_ALLOWLIST =
+    private static final JsonFieldSelector THING_QUERY_COMMAND_RESPONSE_ALLOWLIST =
             JsonFactory.newFieldSelector(Thing.JsonFields.ID);
 
     private final ActorRef thingsShardRegion;
@@ -496,7 +496,7 @@ private CompletionStage<Contextual<WithDittoHeaders>> enforceCreateThingForNonex
      * @param enforcer the enforcer.
      * @return response with view on entity restricted by enforcer.
      */
-    public static <T extends ThingQueryCommandResponse<T>> T buildJsonViewForThingQueryCommandResponse(
+    static <T extends ThingQueryCommandResponse<T>> T buildJsonViewForThingQueryCommandResponse(
             final ThingQueryCommandResponse<T> response, final Enforcer enforcer) {
 
         final JsonValue entity = response.getEntity();

diff --git a/.../test/java/org/eclipse/ditto/concierge/service/enforcement/LiveSignalEnforcementTest.java b/.../test/java/org/eclipse/ditto/concierge/service/enforcement/LiveSignalEnforcementTest.java
@@ -19,9 +19,6 @@
 import static org.eclipse.ditto.concierge.service.enforcement.TestSetup.SUBJECT_ID;
 import static org.eclipse.ditto.concierge.service.enforcement.TestSetup.THING;
 import static org.eclipse.ditto.concierge.service.enforcement.TestSetup.newThingWithPolicyId;
-import static org.eclipse.ditto.concierge.service.enforcement.TestSetup.readCommand;
-import static org.eclipse.ditto.concierge.service.enforcement.TestSetup.readCommandResponse;
-import static org.eclipse.ditto.concierge.service.enforcement.TestSetup.writeCommand;
 import static org.eclipse.ditto.policies.model.SubjectIssuer.GOOGLE;
 
 import java.util.UUID;
@@ -58,6 +55,8 @@
 import org.eclipse.ditto.things.model.signals.commands.exceptions.EventSendNotAllowedException;
 import org.eclipse.ditto.things.model.signals.commands.exceptions.FeatureNotModifiableException;
 import org.eclipse.ditto.things.model.signals.commands.exceptions.ThingNotAccessibleException;
+import org.eclipse.ditto.things.model.signals.commands.modify.ModifyFeature;
+import org.eclipse.ditto.things.model.signals.commands.query.RetrieveThing;
 import org.eclipse.ditto.things.model.signals.commands.query.RetrieveThingResponse;
 import org.eclipse.ditto.things.model.signals.events.AttributeModified;
 import org.eclipse.ditto.things.model.signals.events.ThingEvent;
@@ -140,10 +139,10 @@ public void rejectLiveThingCommandByPolicy() {
             mockEntitiesActorInstance.setReply(TestSetup.POLICY_SUDO, sudoRetrievePolicyResponse);
 
             final ActorRef underTest = newEnforcerActor(getRef());
-            underTest.tell(readCommand(headers()), getRef());
+            underTest.tell(getRetrieveThingCommand(headers()), getRef());
             TestSetup.fishForMsgClass(this, ThingNotAccessibleException.class);
 
-            underTest.tell(writeCommand(headers()), getRef());
+            underTest.tell(getModifyFeatureCommand(headers()), getRef());
             expectMsgClass(FeatureNotModifiableException.class);
         }};
     }
@@ -171,7 +170,7 @@ public void acceptLiveThingCommandByPolicy() {
 
             final ActorRef underTest = newEnforcerActor(getRef());
 
-            final ThingCommand<?> write = writeCommand(headers());
+            final ThingCommand<?> write = getModifyFeatureCommand(headers());
             mockEntitiesActorInstance.setReply(write);
             underTest.tell(write, getRef());
             final DistributedPubSubMediator.Publish publish =
@@ -180,7 +179,7 @@ public void acceptLiveThingCommandByPolicy() {
             assertThat(publish.msg()).isInstanceOf(ThingCommand.class);
             assertThat((CharSequence) ((ThingCommand<?>) publish.msg()).getEntityId()).isEqualTo(write.getEntityId());
 
-            final ThingCommand<?> read = readCommand(headers());
+            final ThingCommand<?> read = getRetrieveThingCommand(headers());
             final RetrieveThingResponse retrieveThingResponse =
                     RetrieveThingResponse.of(TestSetup.THING_ID, JsonFactory.newObject(), DittoHeaders.empty());
             mockEntitiesActorInstance.setReply(retrieveThingResponse);
@@ -221,7 +220,7 @@ public void retrieveLiveThingCommandAndResponseByPolicy() {
             final ActorRef underTest = newEnforcerActor(getRef());
 
             final DittoHeaders headers = headers();
-            final ThingCommand<?> read = readCommand(headers);
+            final ThingCommand<?> read = getRetrieveThingCommand(headers);
 
             underTest.tell(read, getRef());
             final DistributedPubSubMediator.Publish publishRead =
@@ -231,7 +230,7 @@ public void retrieveLiveThingCommandAndResponseByPolicy() {
             assertThat((CharSequence) ((ThingCommand<?>) publishRead.msg()).getEntityId()).isEqualTo(
                     read.getEntityId());
 
-            final ThingCommandResponse<?> readResponse = readCommandResponse(headers);
+            final ThingCommandResponse<?> readResponse = getRetrieveThingResponse(headers);
             final Thing expectedThing = THING.toBuilder()
                     .removeFeatureProperty(FEATURE_ID, JsonPointer.of(FEATURE_PROPERTY_2))
                     .build();
@@ -393,10 +392,22 @@ private static DittoHeaders headers() {
                                 AuthorizationSubject.newInstance(String.format("%s:%s", GOOGLE, SUBJECT_ID))))
                 .channel("live")
                 .schemaVersion(JsonSchemaVersion.V_2)
-                .correlationId(UUID.randomUUID().toString())
+                .randomCorrelationId()
                 .build();
     }
 
+    private static ThingCommand<?> getRetrieveThingCommand(final DittoHeaders headers) {
+        return RetrieveThing.of(TestSetup.THING_ID, headers);
+    }
+
+    private static ThingCommandResponse<?> getRetrieveThingResponse(final DittoHeaders headers) {
+        return RetrieveThingResponse.of(TestSetup.THING_ID, THING, null, null, headers);
+    }
+
+    private static ThingCommand<?> getModifyFeatureCommand(final DittoHeaders headers) {
+        return ModifyFeature.of(TestSetup.THING_ID, TestSetup.FEATURE, headers);
+    }
+
     private static MessageCommand<?, ?> thingMessageCommand() {
         final Message<Object> message = Message.newBuilder(
                         MessageBuilder.newHeadersBuilder(MessageDirection.TO, TestSetup.THING_ID, "my-subject")

diff --git a/...erge/service/src/test/java/org/eclipse/ditto/concierge/service/enforcement/TestSetup.java b/...erge/service/src/test/java/org/eclipse/ditto/concierge/service/enforcement/TestSetup.java
@@ -13,7 +13,6 @@
 package org.eclipse.ditto.concierge.service.enforcement;
 
 import static org.eclipse.ditto.base.model.json.JsonSchemaVersion.V_2;
-import static org.eclipse.ditto.policies.model.SubjectIssuer.GOOGLE;
 
 import java.util.HashSet;
 import java.util.Set;
@@ -22,10 +21,7 @@
 
 import javax.annotation.Nullable;
 
-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.headers.DittoHeaders;
 import org.eclipse.ditto.base.model.json.FieldType;
 import org.eclipse.ditto.base.model.signals.SignalWithEntityId;
 import org.eclipse.ditto.concierge.service.common.CachesConfig;
@@ -54,10 +50,6 @@
 import org.eclipse.ditto.things.model.ThingId;
 import org.eclipse.ditto.things.model.ThingsModelFactory;
 import org.eclipse.ditto.things.model.signals.commands.ThingCommand;
-import org.eclipse.ditto.things.model.signals.commands.ThingCommandResponse;
-import org.eclipse.ditto.things.model.signals.commands.modify.ModifyFeature;
-import org.eclipse.ditto.things.model.signals.commands.query.RetrieveThing;
-import org.eclipse.ditto.things.model.signals.commands.query.RetrieveThingResponse;
 import org.eclipse.ditto.things.model.signals.events.ThingEvent;
 
 import com.github.benmanes.caffeine.cache.Caffeine;
@@ -134,17 +126,17 @@ static class EnforcerActorBuilder {
         @Nullable private ActorRef conciergeForwarder;
         @Nullable private PreEnforcer preEnforcer;
 
-        EnforcerActorBuilder(final ActorSystem system, final ActorRef testActorRef,
-                final ActorRef thingsShardRegion, final ActorRef policiesShardRegion) {
+        EnforcerActorBuilder(final ActorSystem system, final ActorRef testActorRef, final ActorRef thingsShardRegion,
+                final ActorRef policiesShardRegion) {
             this.system = system;
             this.testActorRef = testActorRef;
             this.thingsShardRegion = thingsShardRegion;
             this.policiesShardRegion = policiesShardRegion;
             this.puSubMediatorRef = testActorRef;
         }
 
-        EnforcerActorBuilder(final ActorSystem system, final ActorRef testActorRef,
-                final ActorRef thingsShardRegion, final ActorRef policiesShardRegion, final ActorRef puSubMediatorRef) {
+        EnforcerActorBuilder(final ActorSystem system, final ActorRef testActorRef, final ActorRef thingsShardRegion,
+                final ActorRef policiesShardRegion, final ActorRef puSubMediatorRef) {
             this.system = system;
             this.testActorRef = testActorRef;
             this.thingsShardRegion = thingsShardRegion;
@@ -200,41 +192,19 @@ static String createUniqueName() {
         return "conciergeForwarder-" + UUID.randomUUID();
     }
 
-    public static DittoHeaders headers() {
-        return DittoHeaders.newBuilder()
-                .authorizationContext(
-                        AuthorizationContext.newInstance(DittoAuthorizationContextType.UNSPECIFIED, SUBJECT,
-                                AuthorizationSubject.newInstance(String.format("%s:%s", GOOGLE, SUBJECT_ID))))
-                .schemaVersion(V_2)
-                .correlationId(UUID.randomUUID().toString())
-                .build();
-    }
-
     public static ThingBuilder.FromScratch newThing() {
         return ThingsModelFactory.newThingBuilder()
                 .setId(THING_ID)
                 .setRevision(1L);
     }
 
-    public static JsonObject newThingWithPolicyId(final CharSequence policyId) {
+    public static JsonObject newThingWithPolicyId(final PolicyId policyId) {
         return newThing()
-                .setPolicyId(PolicyId.of(policyId))
+                .setPolicyId(policyId)
                 .build()
                 .toJson(V_2, FieldType.all());
     }
 
-    public static ThingCommand<?> readCommand(final DittoHeaders headers) {
-        return RetrieveThing.of(TestSetup.THING_ID, headers);
-    }
-
-    public static ThingCommandResponse<?> readCommandResponse(final DittoHeaders headers) {
-        return RetrieveThingResponse.of(TestSetup.THING_ID, THING, null, null, headers);
-    }
-
-    public static ThingCommand<?> writeCommand(final DittoHeaders headers) {
-        return ModifyFeature.of(TestSetup.THING_ID, FEATURE, headers);
-    }
-
     /**
      * Similar to {@link TestKit#expectMsgClass(Class)} but ignores other messages occurring while waiting for a
      * message of the passed {@code clazz}.

diff --git a/...est/java/org/eclipse/ditto/concierge/service/enforcement/ThingCommandEnforcementTest.java b/...est/java/org/eclipse/ditto/concierge/service/enforcement/ThingCommandEnforcementTest.java
@@ -752,8 +752,6 @@ private ActorRef newEnforcerActor(final ActorRef testActorRef, final ActorRef co
                 .setConciergeForwarder(conciergeForwarderRef).build();
     }
 
-
-
     private DittoHeaders headers(final JsonSchemaVersion schemaVersion) {
         return DittoHeaders.newBuilder()
                 .authorizationContext(