chore: fix scope in case of empty one from policy expression (#3933)

* chore: fix scope in case of empty one from policy expression * chore: dependencies file * Update data-protocols/dsp/dsp-http-core/src/main/java/org/eclipse/edc/protocol/dsp/dispatcher/DspHttpRemoteMessageDispatcherImpl.java Co-authored-by: ndr_brt <[email protected]> --------- Co-authored-by: ndr_brt <[email protected]>
eclipse-edc · Feb 28, 2024 · 17f2e98 · 17f2e98
1 parent fcc8994
commit 17f2e98
Show file tree

Hide file tree

Showing 3 changed files with 100 additions and 12 deletions.
diff --git a/DEPENDENCIES b/DEPENDENCIES
@@ -125,7 +125,7 @@ maven/mavencentral/io.netty/netty-tcnative-classes/2.0.56.Final, Apache-2.0, app
 maven/mavencentral/io.netty/netty-transport-native-unix-common/4.1.86.Final, Apache-2.0 AND BSD-3-Clause AND MIT, approved, CQ20926
 maven/mavencentral/io.netty/netty-transport/4.1.86.Final, Apache-2.0 AND BSD-3-Clause AND MIT, approved, CQ20926
 maven/mavencentral/io.opentelemetry.instrumentation/opentelemetry-instrumentation-annotations/1.32.0, Apache-2.0, approved, #11684
-maven/mavencentral/io.opentelemetry.proto/opentelemetry-proto/1.1.0-alpha, None, restricted, #12956
+maven/mavencentral/io.opentelemetry.proto/opentelemetry-proto/1.1.0-alpha, Apache-2.0, approved, #12956
 maven/mavencentral/io.opentelemetry/opentelemetry-api/1.32.0, Apache-2.0, approved, #11682
 maven/mavencentral/io.opentelemetry/opentelemetry-context/1.32.0, Apache-2.0, approved, #11683
 maven/mavencentral/io.prometheus/simpleclient/0.16.0, Apache-2.0, approved, clearlydefined

diff --git a/...main/java/org/eclipse/edc/protocol/dsp/dispatcher/DspHttpRemoteMessageDispatcherImpl.java b/...main/java/org/eclipse/edc/protocol/dsp/dispatcher/DspHttpRemoteMessageDispatcherImpl.java
@@ -53,18 +53,16 @@
  */
 public class DspHttpRemoteMessageDispatcherImpl implements DspHttpRemoteMessageDispatcher {
 
+    private static final String AUDIENCE_CLAIM = "aud";
+    private static final String SCOPE_CLAIM = "scope";
     private final Map<Class<? extends RemoteMessage>, MessageHandler<?, ?>> handlers = new HashMap<>();
     private final Map<Class<? extends RemoteMessage>, PolicyScope<? extends RemoteMessage>> policyScopes = new HashMap<>();
     private final EdcHttpClient httpClient;
     private final IdentityService identityService;
     private final PolicyEngine policyEngine;
     private final TokenDecorator tokenDecorator;
-
     private final AudienceResolver audienceResolver;
 
-    private static final String AUDIENCE_CLAIM = "aud";
-    private static final String SCOPE_CLAIM = "scope";
-
 
     public DspHttpRemoteMessageDispatcherImpl(EdcHttpClient httpClient,
                                               IdentityService identityService,
@@ -92,7 +90,7 @@ public <T, M extends RemoteMessage> CompletableFuture<StatusResult<T>> dispatch(
 
         var request = handler.requestFactory.createRequest(message);
 
-        var tokenParametersBuilder = tokenDecorator.decorate(TokenParameters.Builder.newInstance());
+        var tokenParametersBuilder = TokenParameters.Builder.newInstance();
 
         var policyScope = policyScopes.get(message.getClass());
         if (policyScope != null) {
@@ -103,10 +101,17 @@ public <T, M extends RemoteMessage> CompletableFuture<StatusResult<T>> dispatch(
             var policyProvider = (Function<M, Policy>) policyScope.policyProvider;
             policyEngine.evaluate(policyScope.scope, policyProvider.apply(message), context);
 
-            tokenParametersBuilder.claims(SCOPE_CLAIM, String.join(" ", requestScopeBuilder.build().getScopes()));
+            var scopes = requestScopeBuilder.build().getScopes();
+
+            // Only add the scope claim if there are scopes returned from the policy engine evaluation
+            if (!scopes.isEmpty()) {
+                tokenParametersBuilder.claims(SCOPE_CLAIM, String.join(" ", scopes));
+            }
 
         }
 
+        tokenParametersBuilder = tokenDecorator.decorate(tokenParametersBuilder);
+
         var tokenParameters = tokenParametersBuilder
                 .claims(AUDIENCE_CLAIM, audienceResolver.resolve(message)) // enforce the audience, ignore anything a decorator might have set
                 .build();
@@ -123,17 +128,17 @@ public <T, M extends RemoteMessage> CompletableFuture<StatusResult<T>> dispatch(
                 .orElse(failure -> failedFuture(new EdcException(format("Unable to obtain credentials: %s", failure.getFailureDetail()))));
     }
 
-    @Override
-    public <M extends RemoteMessage> void registerPolicyScope(Class<M> messageClass, String scope, Function<M, Policy> policyProvider) {
-        policyScopes.put(messageClass, new PolicyScope<>(messageClass, scope, policyProvider));
-    }
-
     @Override
     public <M extends RemoteMessage, R> void registerMessage(Class<M> clazz, DspHttpRequestFactory<M> requestFactory,
                                                              DspHttpResponseBodyExtractor<R> bodyExtractor) {
         handlers.put(clazz, new MessageHandler<>(requestFactory, bodyExtractor));
     }
 
+    @Override
+    public <M extends RemoteMessage> void registerPolicyScope(Class<M> messageClass, String scope, Function<M, Policy> policyProvider) {
+        policyScopes.put(messageClass, new PolicyScope<>(messageClass, scope, policyProvider));
+    }
+
     @NotNull
     private <T> StatusResult<T> handleResponse(Response response, Class<T> responseType, DspHttpResponseBodyExtractor<T> bodyExtractor) {
         try (var responseBody = response.body()) {

diff --git a/.../java/org/eclipse/edc/protocol/dsp/dispatcher/DspHttpRemoteMessageDispatcherImplTest.java b/.../java/org/eclipse/edc/protocol/dsp/dispatcher/DspHttpRemoteMessageDispatcherImplTest.java
@@ -104,16 +104,60 @@ void protocol_returnDsp() {
         assertThat(dispatcher.protocol()).isEqualTo(DATASPACE_PROTOCOL_HTTP);
     }
 
+    @Test
+    void dispatch_noScope() {
+        var authToken = "token";
+        Map<String, Object> additional = Map.of("foo", "bar");
+        var policy = Policy.Builder.newInstance().build();
+        when(tokenDecorator.decorate(any())).thenAnswer(a -> a.getArgument(0, TokenParameters.Builder.class).claims(additional));
+        when(requestFactory.createRequest(any())).thenReturn(new Request.Builder().url("http://url").build());
+        when(httpClient.executeAsync(any(), isA(List.class))).thenReturn(completedFuture(dummyResponse(200)));
+        when(identityService.obtainClientCredentials(any()))
+                .thenReturn(Result.success(TokenRepresentation.Builder.newInstance().token(authToken).build()));
+
+        dispatcher.registerPolicyScope(TestMessage.class, "scope.test", (m) -> policy);
+
+        when(policyEngine.evaluate(eq("scope.test"), eq(policy), any())).thenReturn(Result.success());
+
+        dispatcher.registerMessage(TestMessage.class, requestFactory, mock());
+
+        var message = new TestMessage();
+        var result = dispatcher.dispatch(String.class, message);
+
+        assertThat(result).succeedsWithin(timeout);
+
+        var captor = ArgumentCaptor.forClass(TokenParameters.class);
+        verify(identityService).obtainClientCredentials(captor.capture());
+        verify(httpClient).executeAsync(argThat(r -> authToken.equals(r.headers().get("Authorization"))), isA(List.class));
+        verify(requestFactory).createRequest(message);
+        assertThat(captor.getValue()).satisfies(tr -> {
+            assertThat(tr.getStringClaim(SCOPE_CLAIM)).isNull();
+            assertThat(tr.getStringClaim(AUDIENCE_CLAIM)).isEqualTo(AUDIENCE_VALUE);
+            assertThat(tr.getClaims()).containsAllEntriesOf(additional);
+        });
+
+    }
+
     @Test
     void dispatch_ensureTokenDecoratorScope() {
         var authToken = "token";
         Map<String, Object> additional = Map.of("foo", "bar");
+        var policy = Policy.Builder.newInstance().build();
         when(tokenDecorator.decorate(any())).thenAnswer(a -> a.getArgument(0, TokenParameters.Builder.class).claims(additional).claims(SCOPE_CLAIM, "test-scope"));
         when(requestFactory.createRequest(any())).thenReturn(new Request.Builder().url("http://url").build());
         when(httpClient.executeAsync(any(), isA(List.class))).thenReturn(completedFuture(dummyResponse(200)));
         when(identityService.obtainClientCredentials(any()))
                 .thenReturn(Result.success(TokenRepresentation.Builder.newInstance().token(authToken).build()));
 
+        dispatcher.registerPolicyScope(TestMessage.class, "scope.test", (m) -> policy);
+
+        when(policyEngine.evaluate(eq("scope.test"), eq(policy), any())).thenAnswer(a -> {
+            PolicyContext context = a.getArgument(2);
+            var builder = context.getContextData(RequestScope.Builder.class);
+            builder.scope("policy-test-scope");
+            return Result.success();
+        });
+
         dispatcher.registerMessage(TestMessage.class, requestFactory, mock());
 
         var message = new TestMessage();
@@ -133,6 +177,45 @@ void dispatch_ensureTokenDecoratorScope() {
 
     }
 
+    @Test
+    void dispatch_PolicyEvaluatedScope() {
+        var authToken = "token";
+        Map<String, Object> additional = Map.of("foo", "bar");
+        var policy = Policy.Builder.newInstance().build();
+        when(tokenDecorator.decorate(any())).thenAnswer(a -> a.getArgument(0, TokenParameters.Builder.class).claims(additional));
+        when(requestFactory.createRequest(any())).thenReturn(new Request.Builder().url("http://url").build());
+        when(httpClient.executeAsync(any(), isA(List.class))).thenReturn(completedFuture(dummyResponse(200)));
+        when(identityService.obtainClientCredentials(any()))
+                .thenReturn(Result.success(TokenRepresentation.Builder.newInstance().token(authToken).build()));
+
+        dispatcher.registerPolicyScope(TestMessage.class, "scope.test", (m) -> policy);
+
+        when(policyEngine.evaluate(eq("scope.test"), eq(policy), any())).thenAnswer(a -> {
+            PolicyContext context = a.getArgument(2);
+            var builder = context.getContextData(RequestScope.Builder.class);
+            builder.scope("policy-test-scope");
+            return Result.success();
+        });
+
+        dispatcher.registerMessage(TestMessage.class, requestFactory, mock());
+
+        var message = new TestMessage();
+        var result = dispatcher.dispatch(String.class, message);
+
+        assertThat(result).succeedsWithin(timeout);
+
+        var captor = ArgumentCaptor.forClass(TokenParameters.class);
+        verify(identityService).obtainClientCredentials(captor.capture());
+        verify(httpClient).executeAsync(argThat(r -> authToken.equals(r.headers().get("Authorization"))), isA(List.class));
+        verify(requestFactory).createRequest(message);
+        assertThat(captor.getValue()).satisfies(tr -> {
+            assertThat(tr.getStringClaim(SCOPE_CLAIM)).isEqualTo("policy-test-scope");
+            assertThat(tr.getStringClaim(AUDIENCE_CLAIM)).isEqualTo(AUDIENCE_VALUE);
+            assertThat(tr.getClaims()).containsAllEntriesOf(additional);
+        });
+
+    }
+
     @Test
     void dispatch_messageNotRegistered_throwException() {
         assertThat(dispatcher.dispatch(String.class, new TestMessage())).failsWithin(timeout)