Code review

eclipse-edc · May 3, 2024 · 14459f7 · 14459f7
1 parent e112987
commit 14459f7
Show file tree

Hide file tree

Showing 26 changed files with 276 additions and 317 deletions.
diff --git a/.gitignore b/.gitignore
@@ -42,6 +42,7 @@ distributions/**/github.properties
 
 .DS_Store
 **/secrets
+!**/secrets-api/**/secrets
 
 **/terraform.tfvars
 **/.terraform*

diff --git a/.../java/org/eclipse/edc/transform/transformer/edc/from/JsonObjectFromSecretTransformer.java b/.../java/org/eclipse/edc/transform/transformer/edc/from/JsonObjectFromSecretTransformer.java
@@ -22,9 +22,10 @@
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
+import static org.eclipse.edc.jsonld.spi.JsonLdKeywords.ID;
 import static org.eclipse.edc.jsonld.spi.JsonLdKeywords.TYPE;
+import static org.eclipse.edc.spi.types.domain.secret.Secret.EDC_SECRET_TYPE;
 import static org.eclipse.edc.spi.types.domain.secret.Secret.EDC_SECRET_VALUE;
-import static org.eclipse.edc.spi.types.domain.secret.Secret.PROPERTY_ID;
 
 public class JsonObjectFromSecretTransformer extends AbstractJsonLdTransformer<Secret, JsonObject> {
 
@@ -38,8 +39,8 @@ public JsonObjectFromSecretTransformer(JsonBuilderFactory jsonFactory) {
     @Override
     public @Nullable JsonObject transform(@NotNull Secret secret, @NotNull TransformerContext context) {
         return jsonFactory.createObjectBuilder()
-                .add(PROPERTY_ID, secret.getId())
-                .add(TYPE, Secret.EDC_SECRET_TYPE)
+                .add(ID, secret.getId())
+                .add(TYPE, EDC_SECRET_TYPE)
                 .add(EDC_SECRET_VALUE, secret.getValue())
                 .build();
     }

diff --git a/...main/java/org/eclipse/edc/transform/transformer/edc/to/JsonObjectToSecretTransformer.java b/...main/java/org/eclipse/edc/transform/transformer/edc/to/JsonObjectToSecretTransformer.java
@@ -22,8 +22,8 @@
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
+import static org.eclipse.edc.jsonld.spi.JsonLdKeywords.ID;
 import static org.eclipse.edc.spi.types.domain.secret.Secret.EDC_SECRET_VALUE;
-import static org.eclipse.edc.spi.types.domain.secret.Secret.PROPERTY_ID;
 
 
 public class JsonObjectToSecretTransformer extends AbstractJsonLdTransformer<JsonObject, Secret> {
@@ -40,7 +40,7 @@ public JsonObjectToSecretTransformer() {
     }
 
     private void transformProperties(String key, JsonValue value, Secret.Builder builder, @NotNull TransformerContext context) {
-        if (PROPERTY_ID.equals(key)) {
+        if (ID.equals(key)) {
             builder.id(transformString(value, context));
         } else if (EDC_SECRET_VALUE.equals(key)) {
             builder.value(transformString(value, context));

diff --git a/...a/org/eclipse/edc/transform/transformer/edc/from/JsonObjectFromSecretTransformerTest.java b/...a/org/eclipse/edc/transform/transformer/edc/from/JsonObjectFromSecretTransformerTest.java
@@ -24,9 +24,10 @@
 import java.util.Map;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.eclipse.edc.jsonld.spi.JsonLdKeywords.ID;
 import static org.eclipse.edc.jsonld.spi.JsonLdKeywords.TYPE;
+import static org.eclipse.edc.spi.types.domain.secret.Secret.EDC_SECRET_TYPE;
 import static org.eclipse.edc.spi.types.domain.secret.Secret.EDC_SECRET_VALUE;
-import static org.eclipse.edc.spi.types.domain.secret.Secret.PROPERTY_ID;
 import static org.mockito.Mockito.mock;
 
 
@@ -46,8 +47,9 @@ void transform_returnJsonObject() {
         var result = transformer.transform(secret, context);
 
         assertThat(result).isNotNull();
-        assertThat(result.getJsonString(PROPERTY_ID).getString()).isEqualTo(secret.getId());
-        assertThat(result.getJsonString(TYPE).getString()).isEqualTo(Secret.EDC_SECRET_TYPE);
+        assertThat(result.getJsonString(ID).getString()).isEqualTo(secret.getId());
+        assertThat(result.getJsonString(TYPE).getString()).isEqualTo(EDC_SECRET_TYPE);
         assertThat(result.getJsonString(EDC_SECRET_VALUE).getString()).isEqualTo(secret.getValue());
     }
+
 }
diff --git a/.../java/org/eclipse/edc/transform/transformer/edc/to/JsonObjectToSecretTransformerTest.java b/.../java/org/eclipse/edc/transform/transformer/edc/to/JsonObjectToSecretTransformerTest.java
@@ -12,18 +12,17 @@
  *
  */
 
-package org.eclipse.edc.core.transform.transformer.edc.to;
+package org.eclipse.edc.transform.transformer.edc.to;
 
 import jakarta.json.Json;
 import jakarta.json.JsonBuilderFactory;
 import org.eclipse.edc.transform.spi.TransformerContext;
-import org.eclipse.edc.transform.transformer.edc.to.JsonObjectToSecretTransformer;
-import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
 import java.util.Map;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatNullPointerException;
 import static org.eclipse.edc.jsonld.spi.JsonLdKeywords.ID;
 import static org.eclipse.edc.jsonld.spi.JsonLdKeywords.TYPE;
 import static org.eclipse.edc.spi.types.domain.secret.Secret.EDC_SECRET_TYPE;
@@ -33,23 +32,16 @@
 
 class JsonObjectToSecretTransformerTest {
 
-    private static final String TEST_SECRET_ID = "SecretId";
-    private static final String TEST_SECRET_KEY = "secret-key";
+    private static final String TEST_SECRET_ID = "secret-id";
     private static final String TEST_SECRET_VALUE = "secret-value";
 
     private final JsonBuilderFactory jsonFactory = Json.createBuilderFactory(Map.of());
-    private final TransformerContext context = mock(TransformerContext.class);
+    private final TransformerContext context = mock();
 
-    private JsonObjectToSecretTransformer transformer;
-
-    @BeforeEach
-    void setUp() {
-        transformer = new JsonObjectToSecretTransformer();
-    }
+    private final JsonObjectToSecretTransformer transformer = new JsonObjectToSecretTransformer();
 
     @Test
     void transform_returnSecret() {
-
         var secret = jsonFactory.createObjectBuilder()
                 .add(ID, TEST_SECRET_ID)
                 .add(TYPE, EDC_SECRET_TYPE)
@@ -64,4 +56,17 @@ void transform_returnSecret() {
 
         verifyNoInteractions(context);
     }
+
+    @Test
+    void transform_withMissingValueInJsonObject_throwsException() {
+        var secret = jsonFactory.createObjectBuilder()
+                .add(ID, TEST_SECRET_ID)
+                .add(TYPE, EDC_SECRET_TYPE)
+                .build();
+
+        assertThatNullPointerException().isThrownBy(() -> transformer.transform(secret, context));
+    }
+
 }
+
+
diff --git a/...c/main/java/org/eclipse/edc/connector/controlplane/services/secret/SecretServiceImpl.java b/...c/main/java/org/eclipse/edc/connector/controlplane/services/secret/SecretServiceImpl.java
@@ -16,13 +16,10 @@
 
 import org.eclipse.edc.connector.secret.spi.observe.SecretObservable;
 import org.eclipse.edc.connector.spi.service.SecretService;
-import org.eclipse.edc.spi.query.QuerySpec;
 import org.eclipse.edc.spi.result.ServiceResult;
 import org.eclipse.edc.spi.security.Vault;
 import org.eclipse.edc.spi.types.domain.secret.Secret;
 
-import java.util.List;
-
 import static java.util.Optional.ofNullable;
 import static org.eclipse.edc.spi.result.ServiceResult.badRequest;
 import static org.eclipse.edc.spi.result.ServiceResult.conflict;
@@ -48,11 +45,6 @@ public Secret findById(String secretId) {
                 .orElse(null);
     }
 
-    @Override
-    public ServiceResult<List<Secret>> search(QuerySpec query) {
-        throw new UnsupportedOperationException("Query operation is not supported for secrets");
-    }
-
     @Override
     public ServiceResult<Secret> create(Secret secret) {
         var existing = findById(secret.getId());

diff --git a/...ava/org/eclipse/edc/protocol/dsp/http/api/configuration/DspApiConfigurationExtension.java b/...ava/org/eclipse/edc/protocol/dsp/http/api/configuration/DspApiConfigurationExtension.java
@@ -36,10 +36,8 @@
 import org.eclipse.edc.transform.transformer.dspace.to.JsonObjectToDataAddressDspaceTransformer;
 import org.eclipse.edc.transform.transformer.edc.from.JsonObjectFromCriterionTransformer;
 import org.eclipse.edc.transform.transformer.edc.from.JsonObjectFromQuerySpecTransformer;
-import org.eclipse.edc.transform.transformer.edc.from.JsonObjectFromSecretTransformer;
 import org.eclipse.edc.transform.transformer.edc.to.JsonObjectToCriterionTransformer;
 import org.eclipse.edc.transform.transformer.edc.to.JsonObjectToQuerySpecTransformer;
-import org.eclipse.edc.transform.transformer.edc.to.JsonObjectToSecretTransformer;
 import org.eclipse.edc.transform.transformer.edc.to.JsonValueToGenericTypeTransformer;
 import org.eclipse.edc.web.jersey.providers.jsonld.JerseyJsonLdInterceptor;
 import org.eclipse.edc.web.jersey.providers.jsonld.ObjectMapperProvider;
@@ -68,7 +66,7 @@
  * parameters.
  */
 @Extension(value = DspApiConfigurationExtension.NAME)
-@Provides({DspApiConfiguration.class, ProtocolWebhook.class})
+@Provides({ DspApiConfiguration.class, ProtocolWebhook.class })
 public class DspApiConfigurationExtension implements ServiceExtension {
 
     public static final String NAME = "Dataspace Protocol API Configuration Extension";
@@ -143,7 +141,6 @@ private void registerTransformers() {
         dspApiTransformerRegistry.register(new JsonObjectFromDataAddressDspaceTransformer(jsonBuilderFactory, mapper));
         dspApiTransformerRegistry.register(new JsonObjectFromQuerySpecTransformer(jsonBuilderFactory));
         dspApiTransformerRegistry.register(new JsonObjectFromCriterionTransformer(jsonBuilderFactory, mapper));
-        dspApiTransformerRegistry.register(new JsonObjectFromSecretTransformer(jsonBuilderFactory));
 
         // JSON-LD to EDC model transformers
         // ODRL Transformers
@@ -154,7 +151,6 @@ private void registerTransformers() {
         dspApiTransformerRegistry.register(new JsonObjectToQuerySpecTransformer());
         dspApiTransformerRegistry.register(new JsonObjectToCriterionTransformer());
         dspApiTransformerRegistry.register(new JsonObjectToDataAddressDspaceTransformer());
-        dspApiTransformerRegistry.register(new JsonObjectToSecretTransformer());
     }
 
 }
diff --git a/...anagement-api/secret-api/build.gradle.kts → ...nagement-api/secrets-api/build.gradle.kts b/...anagement-api/secret-api/build.gradle.kts → ...nagement-api/secrets-api/build.gradle.kts
@@ -24,10 +24,10 @@ dependencies {
     implementation(project(":extensions:common:api:api-core"))
     implementation(project(":extensions:common:api:management-api-configuration"))
     implementation(project(":core:common:lib:validator-lib"))
+    implementation(project(":core:common:lib:transform-lib"))
 
     implementation(libs.jakarta.rsApi)
 
-    testImplementation(project(":core:common:lib:transform-lib"))
     testImplementation(project(":core:control-plane:control-plane-core"))
     testImplementation(project(":core:data-plane-selector:data-plane-selector-core"))
     testImplementation(project(":extensions:common:http"))

diff --git a/...ctor/api/management/secret/SecretApi.java → ...or/api/management/secrets/SecretsApi.java b/...ctor/api/management/secret/SecretApi.java → ...or/api/management/secrets/SecretsApi.java
@@ -12,7 +12,7 @@
  *
  */
 
-package org.eclipse.edc.connector.api.management.secret;
+package org.eclipse.edc.connector.api.management.secrets;
 
 import io.swagger.v3.oas.annotations.OpenAPIDefinition;
 import io.swagger.v3.oas.annotations.Operation;
@@ -23,7 +23,6 @@
 import io.swagger.v3.oas.annotations.parameters.RequestBody;
 import io.swagger.v3.oas.annotations.responses.ApiResponse;
 import io.swagger.v3.oas.annotations.tags.Tag;
-import jakarta.json.JsonArray;
 import jakarta.json.JsonObject;
 import org.eclipse.edc.api.model.ApiCoreSchema;
 
@@ -35,11 +34,11 @@
 import static org.eclipse.edc.spi.types.domain.secret.Secret.EDC_SECRET_VALUE;
 
 @OpenAPIDefinition(
-        info = @Info(description = "This contains the secret management API, which allows a participant to manage the API keys associated to the API they expose.", title = "Secret API"))
+        info = @Info(description = "This contains the secret management API, which allows to add, remove and update secrets in the Vault.", title = "Secret API"))
 @Tag(name = "Secret")
-public interface SecretApi {
+public interface SecretsApi {
 
-    @Operation(description = "Creates a new secret, as a key,value pair",
+    @Operation(description = "Creates a new secret.",
             requestBody = @RequestBody(content = @Content(schema = @Schema(implementation = SecretInputSchema.class))),
             responses = {
                     @ApiResponse(responseCode = "200", description = "Secret was created successfully. Returns the secret Id and created timestamp",
@@ -51,18 +50,6 @@ public interface SecretApi {
     )
     JsonObject createSecret(JsonObject secret);
 
-    @Operation(description = "Request all secrets according to a particular query",
-            requestBody = @RequestBody(
-                    content = @Content(schema = @Schema(implementation = ApiCoreSchema.QuerySpecSchema.class))
-            ),
-            responses = {
-                    @ApiResponse(responseCode = "200", description = "The secrets matching the query",
-                            content = @Content(array = @ArraySchema(schema = @Schema(implementation = SecretOutputSchema.class)))),
-                    @ApiResponse(responseCode = "400", description = "Request body was malformed",
-                            content = @Content(array = @ArraySchema(schema = @Schema(implementation = ApiCoreSchema.ApiErrorDetailSchema.class))))
-            })
-    JsonArray requestSecrets(JsonObject querySpecJson);
-
     @Operation(description = "Gets a secret with the given ID",
             responses = {
                     @ApiResponse(responseCode = "200", description = "The secret",
@@ -75,17 +62,12 @@ public interface SecretApi {
     )
     JsonObject getSecret(String id);
 
-    @Operation(description = "Removes a secret with the given ID if possible. Deleting a secret is only possible if that secret is not yet referenced " +
-            "by a contract agreement, in which case an error is returned. " +
-            "DANGER ZONE: Note that deleting secrets referenced by an asset may lead to unexpected behavior in the system.",
+    @Operation(description = "Removes a secret with the given ID if possible.",
             responses = {
                     @ApiResponse(responseCode = "204", description = "Secret was deleted successfully"),
                     @ApiResponse(responseCode = "400", description = "Request was malformed, e.g. id was null",
                             content = @Content(array = @ArraySchema(schema = @Schema(implementation = ApiCoreSchema.ApiErrorDetailSchema.class)))),
                     @ApiResponse(responseCode = "404", description = "A secret with the given ID does not exist",
-                            content = @Content(array = @ArraySchema(schema = @Schema(implementation = ApiCoreSchema.ApiErrorDetailSchema.class)))),
-                    // TODO: check if it makes sense to check id secret is referenced from any asset
-                    @ApiResponse(responseCode = "409", description = "The secret cannot be deleted, because it is referenced by an asset",
                             content = @Content(array = @ArraySchema(schema = @Schema(implementation = ApiCoreSchema.ApiErrorDetailSchema.class))))
             })
     void removeSecret(String id);
@@ -94,9 +76,9 @@ public interface SecretApi {
             requestBody = @RequestBody(content = @Content(schema = @Schema(implementation = SecretInputSchema.class))),
             responses = {
                     @ApiResponse(responseCode = "204", description = "Secret was updated successfully"),
-                    @ApiResponse(responseCode = "404", description = "Secret could not be updated, because it does not exist."),
                     @ApiResponse(responseCode = "400", description = "Request was malformed, e.g. id was null",
                             content = @Content(array = @ArraySchema(schema = @Schema(implementation = ApiCoreSchema.ApiErrorDetailSchema.class)))),
+                    @ApiResponse(responseCode = "404", description = "Secret could not be updated, because it does not exist.")
             })
     void updateSecret(JsonObject secret);
 
@@ -115,7 +97,6 @@ record SecretInputSchema(
                 {
                     "@context": { "@vocab": "https://w3id.org/edc/v0.0.1/ns/" },
                     "@id": "secret-id",
-                    "key": "secret-key",
                     "value" : "secret-value"
                 }
                 """;
@@ -129,18 +110,14 @@ record SecretOutputSchema(
             @Schema(name = TYPE, example = EDC_SECRET_TYPE)
             String type,
             @Schema(name = EDC_SECRET_VALUE, requiredMode = REQUIRED)
-            String value,
-            long createdAt
+            String value
     ) {
-        // TODO: check key and value names
         public static final String SECRET_OUTPUT_EXAMPLE = """
                 {
                     "@context": { "@vocab": "https://w3id.org/edc/v0.0.1/ns/" },
                     "@id": "secret-id",
                     "@type": "https://w3id.org/edc/v0.0.1/ns/Secret",
-                    "key": "secret-key",
-                    "value": "secret-value",
-                    "createdAt": 1688465655
+                    "value": "secret-value"
                 }
                 """;
     }