diff --git a/CHANGELOG.md b/CHANGELOG.md index c8c159772a..caa0ec799e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ _**For better traceability add the corresponding GitHub issue number in each cha ### Changed - Default policies are now configured using JSON in accordance with the ODRL schema. #542 +- Improved the exception handling for modification attempts on read-only default policies. Such actions now result in a 400 BAD REQUEST response with a user-friendly error message. #734 ## [5.3.0] - 2024-07-15 diff --git a/irs-policy-store/src/main/java/org/eclipse/tractusx/irs/policystore/exceptions/ResourceDoesNotExistException.java b/irs-policy-store/src/main/java/org/eclipse/tractusx/irs/policystore/exceptions/ResourceDoesNotExistException.java new file mode 100644 index 0000000000..cdf31291c0 --- /dev/null +++ b/irs-policy-store/src/main/java/org/eclipse/tractusx/irs/policystore/exceptions/ResourceDoesNotExistException.java @@ -0,0 +1,30 @@ +/******************************************************************************** + * Copyright (c) 2022,2024 Bayerische Motoren Werke Aktiengesellschaft (BMW AG) + * Copyright (c) 2021,2024 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 Apache License, Version 2.0 which is available at + * https://www.apache.org/licenses/LICENSE-2.0. + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + * SPDX-License-Identifier: Apache-2.0 + ********************************************************************************/ +package org.eclipse.tractusx.irs.policystore.exceptions; + +/** + * Resource does not exist exception. + */ +public class ResourceDoesNotExistException extends PolicyStoreException { + + public ResourceDoesNotExistException(final String msg) { + super(msg); + } +} diff --git a/irs-policy-store/src/main/java/org/eclipse/tractusx/irs/policystore/persistence/PolicyPersistence.java b/irs-policy-store/src/main/java/org/eclipse/tractusx/irs/policystore/persistence/PolicyPersistence.java index 610fdc3d9f..e2a5bb55d2 100644 --- a/irs-policy-store/src/main/java/org/eclipse/tractusx/irs/policystore/persistence/PolicyPersistence.java +++ b/irs-policy-store/src/main/java/org/eclipse/tractusx/irs/policystore/persistence/PolicyPersistence.java @@ -41,6 +41,7 @@ import org.eclipse.tractusx.irs.common.persistence.BlobPersistenceException; import org.eclipse.tractusx.irs.edc.client.policy.Policy; import org.eclipse.tractusx.irs.policystore.exceptions.PolicyStoreException; +import org.eclipse.tractusx.irs.policystore.exceptions.ResourceDoesNotExistException; import org.springframework.beans.factory.annotation.Qualifier; import org.springframework.stereotype.Service; @@ -89,7 +90,7 @@ public void delete(final String bpn, final String policyId) { final var policies = readAll(bpn); final var modifiedPolicies = policies.stream().filter(p -> !p.getPolicyId().equals(policyId)).toList(); if (policies.size() == modifiedPolicies.size()) { - throw new PolicyStoreException("Policy with id '" + policyId + "' doesn't exists!"); + throw new ResourceDoesNotExistException("Policy with id '%s' doesn't exists!".formatted(policyId)); } save(bpn, modifiedPolicies); } diff --git a/irs-policy-store/src/main/java/org/eclipse/tractusx/irs/policystore/services/PolicyStoreService.java b/irs-policy-store/src/main/java/org/eclipse/tractusx/irs/policystore/services/PolicyStoreService.java index af273dc556..a5dfed231c 100644 --- a/irs-policy-store/src/main/java/org/eclipse/tractusx/irs/policystore/services/PolicyStoreService.java +++ b/irs-policy-store/src/main/java/org/eclipse/tractusx/irs/policystore/services/PolicyStoreService.java @@ -49,6 +49,7 @@ import org.eclipse.tractusx.irs.edc.client.transformer.EdcTransformer; import org.eclipse.tractusx.irs.policystore.config.DefaultAcceptedPoliciesConfig; import org.eclipse.tractusx.irs.policystore.exceptions.PolicyStoreException; +import org.eclipse.tractusx.irs.policystore.exceptions.ResourceDoesNotExistException; import org.eclipse.tractusx.irs.policystore.models.CreatePolicyRequest; import org.eclipse.tractusx.irs.policystore.models.UpdatePolicyRequest; import org.eclipse.tractusx.irs.policystore.persistence.PolicyPersistence; @@ -63,7 +64,8 @@ @Service @Slf4j @SuppressWarnings({ "PMD.ExcessiveImports", - "PMD.TooManyMethods" + "PMD.TooManyMethods", + "PMD.GodClass" }) public class PolicyStoreService implements AcceptedPoliciesProvider { @@ -107,6 +109,8 @@ public Policy registerPolicy(final CreatePolicyRequest request) { final Policy policy = edcTransformer.transformToIrsPolicy(policyJson); policy.setValidUntil(request.validUntil()); + handleTrialToModifyFallbackPolicy(policy.getPolicyId()); + registeredPolicy = doRegisterPolicy(policy, request.businessPartnerNumber() == null ? BPN_DEFAULT : request.businessPartnerNumber()); @@ -180,20 +184,20 @@ private static boolean containsNoDefaultPolicy(final Map> b public void deletePolicy(final String policyId) { + handleTrialToModifyFallbackPolicy(policyId); + log.info("Getting all policies to find correct BPN"); final List bpnsContainingPolicyId = PolicyHelper.findBpnsByPolicyId(getAllStoredPolicies(), policyId); if (bpnsContainingPolicyId.isEmpty()) { throw new ResponseStatusException(HttpStatus.NOT_FOUND, "Policy with id '%s' not found".formatted(policyId)); - } else if (bpnsContainingPolicyId.stream().noneMatch(StringUtils::isNotEmpty)) { - throw new ResponseStatusException(HttpStatus.BAD_REQUEST, // - "A configured default policy cannot be deleted. " - + "It can be overridden by defining a default policy via the API instead."); } else { try { log.info("Deleting policy with id {}", policyId); bpnsContainingPolicyId.forEach(bpn -> persistence.delete(bpn, policyId)); + } catch (final ResourceDoesNotExistException e) { + throw new ResponseStatusException(HttpStatus.NOT_FOUND, e.getMessage(), e); } catch (final PolicyStoreException e) { throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, e.getMessage(), e); } @@ -201,15 +205,24 @@ public void deletePolicy(final String policyId) { } public void deletePolicyForEachBpn(final String policyId, final List bpnList) { + + handleTrialToModifyFallbackPolicy(policyId); + try { for (final String bpn : bpnList) { persistence.delete(bpn, policyId); } + } catch (final ResourceDoesNotExistException e) { + throw new ResponseStatusException(HttpStatus.NOT_FOUND, e.getMessage(), e); } catch (final PolicyStoreException e) { throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, e.getMessage(), e); } } + private boolean isFallbackPolicyFromConfiguration(final String policyId) { + return this.allowedPoliciesFromConfig.stream().map(Policy::getPolicyId).anyMatch(p -> p.equals(policyId)); + } + public void updatePolicies(final UpdatePolicyRequest request) { for (final String policyId : request.policyIds()) { updatePolicy(policyId, request.validUntil(), @@ -220,6 +233,8 @@ public void updatePolicies(final UpdatePolicyRequest request) { public void updatePolicy(final String policyId, final OffsetDateTime newValidUntil, final List newBusinessPartnerNumbers) { + handleTrialToModifyFallbackPolicy(policyId); + log.info("Updating policy with id {}", policyId); final List businessPartnerNumbersContainingPolicyId = findBusinessPartnerNumbersByPolicyId(policyId); @@ -304,4 +319,14 @@ private List createDefaultPolicyFromConfig(final DefaultAcceptedPolicies return StringMapper.mapFromBase64String(defaultPoliciesConfig.getAcceptedPolicies(), LIST_OF_POLICIES_TYPE); } + private void handleTrialToModifyFallbackPolicy(final String policyId) { + if (isFallbackPolicyFromConfiguration(policyId)) { + throw new ResponseStatusException(HttpStatus.BAD_REQUEST, + ("The policy '%s' is a configured default policy which cannot be modified. " + + "However you can define custom default policies via the API by registering a policy " + + "without a business partner number. " + + "These will take precedence over configured default policies.").formatted(policyId)); + } + } + } diff --git a/irs-policy-store/src/test/java/org/eclipse/tractusx/irs/policystore/controllers/PolicyStoreControllerTest.java b/irs-policy-store/src/test/java/org/eclipse/tractusx/irs/policystore/controllers/PolicyStoreControllerTest.java index f5a43e34e6..f9bb518de0 100644 --- a/irs-policy-store/src/test/java/org/eclipse/tractusx/irs/policystore/controllers/PolicyStoreControllerTest.java +++ b/irs-policy-store/src/test/java/org/eclipse/tractusx/irs/policystore/controllers/PolicyStoreControllerTest.java @@ -68,7 +68,7 @@ public class PolicyStoreControllerTest { "@context": { "odrl": "http://www.w3.org/ns/odrl/2/" }, - "@id": "e917f5f-8dac-49ac-8d10-5b4d254d2b48", + "@id": "%s", "policy": { "odrl:permission": [ { @@ -118,7 +118,8 @@ class RegisterPolicyTests { void registerAllowedPolicy() { // arrange final OffsetDateTime now = OffsetDateTime.now(); - final JsonObject jsonObject = PolicyStoreTestUtil.toJsonObject(REGISTER_POLICY_EXAMPLE_PAYLOAD); + final JsonObject jsonObject = PolicyStoreTestUtil.toJsonObject( + REGISTER_POLICY_EXAMPLE_PAYLOAD.formatted("e917f5f-8dac-49ac-8d10-5b4d254d2b48")); // act final CreatePolicyRequest request = new CreatePolicyRequest(now.plusMinutes(1), null, diff --git a/irs-policy-store/src/test/java/org/eclipse/tractusx/irs/policystore/services/PolicyStoreServiceTest.java b/irs-policy-store/src/test/java/org/eclipse/tractusx/irs/policystore/services/PolicyStoreServiceTest.java index 8022ad32a6..bf27031a8e 100644 --- a/irs-policy-store/src/test/java/org/eclipse/tractusx/irs/policystore/services/PolicyStoreServiceTest.java +++ b/irs-policy-store/src/test/java/org/eclipse/tractusx/irs/policystore/services/PolicyStoreServiceTest.java @@ -149,19 +149,38 @@ void setUp() { @DisplayName("registerPolicy") class RegisterPolicyTests { + @Test + void registerPolicy_whenFallbackPolicyId_shouldReturnHttpStatus400() { + final JsonObject policy = PolicyStoreTestUtil.toJsonObject( + REGISTER_POLICY_EXAMPLE_PAYLOAD.formatted(CONFIGURED_DEFAULT_POLICY_ID)); + assertThatThrownBy(() -> testee.registerPolicy(CreatePolicyRequest.builder() + .businessPartnerNumber("BPNL1234567890AB") + .validUntil(OffsetDateTime.now(clock) + .plusYears(2)) + .payload(policy) + .build())).isInstanceOf( + ResponseStatusException.class) + .hasMessageContaining( + "400 BAD_REQUEST") + .hasMessageContaining( + CONFIGURED_DEFAULT_POLICY_ID); + } + @Test void registerPolicy_withBpnNull_shouldStoreAsDefault() { // ARRANGE final OffsetDateTime now = OffsetDateTime.now(); - final JsonObject jsonObject = PolicyStoreTestUtil.toJsonObject(REGISTER_POLICY_EXAMPLE_PAYLOAD); + final String policyId = "e917f5f-8dac-49ac-8d10-5b4d254d2b48"; + final JsonObject jsonObject = PolicyStoreTestUtil.toJsonObject( + REGISTER_POLICY_EXAMPLE_PAYLOAD.formatted(policyId)); // ACT testee.registerPolicy(new CreatePolicyRequest(now, null, jsonObject)); // ASSERT verify(persistenceMock).save(eq(PolicyStoreService.BPN_DEFAULT), policyCaptor.capture()); - assertThat(policyCaptor.getValue().getPolicyId()).isEqualTo("e917f5f-8dac-49ac-8d10-5b4d254d2b48"); + assertThat(policyCaptor.getValue().getPolicyId()).isEqualTo(policyId); assertThat(policyCaptor.getValue().getValidUntil()).isEqualTo(now); assertThat(policyCaptor.getValue().getPermissions()).hasSize(1); } @@ -171,7 +190,9 @@ void registerPolicy_success() { // ARRANGE final OffsetDateTime now = OffsetDateTime.now(); - final JsonObject jsonObject = PolicyStoreTestUtil.toJsonObject(REGISTER_POLICY_EXAMPLE_PAYLOAD); + final String policyId = "e917f5f-8dac-49ac-8d10-5b4d254d2b48"; + final JsonObject jsonObject = PolicyStoreTestUtil.toJsonObject( + REGISTER_POLICY_EXAMPLE_PAYLOAD.formatted(policyId)); // ACT final OffsetDateTime validUntil = now.plusMonths(1); @@ -182,7 +203,7 @@ void registerPolicy_success() { // ASSERT verify(persistenceMock).save(eq("BPNL00000123ABCD"), policyCaptor.capture()); - assertThat(policyCaptor.getValue().getPolicyId()).isEqualTo("e917f5f-8dac-49ac-8d10-5b4d254d2b48"); + assertThat(policyCaptor.getValue().getPolicyId()).isEqualTo(policyId); assertThat(policyCaptor.getValue().getValidUntil()).isEqualTo(validUntil); assertThat(policyCaptor.getValue().getPermissions()).isNotEmpty(); @@ -538,11 +559,48 @@ void deletePolicy_whenPolicyNotFound_shouldReturnHttpStatus404() { notExistingPolicyId); } + @Test + void deletePolicy_whenFallbackPolicy_shouldReturnHttpStatus400() { + assertThatThrownBy(() -> testee.deletePolicy(CONFIGURED_DEFAULT_POLICY_ID)).isInstanceOf( + ResponseStatusException.class) + .hasMessageContaining( + "400 BAD_REQUEST") + .hasMessageContaining( + CONFIGURED_DEFAULT_POLICY_ID); + } + + @Test + void deletePolicyForEachBpn_whenFallbackPolicy_shouldReturnHttpStatus400() { + assertThatThrownBy(() -> testee.deletePolicyForEachBpn(CONFIGURED_DEFAULT_POLICY_ID, + List.of("BPNL1234567890AB", "BPNL1234567890CD"))).isInstanceOf(ResponseStatusException.class) + .hasMessageContaining("400 BAD_REQUEST") + .hasMessageContaining( + CONFIGURED_DEFAULT_POLICY_ID); + } + } @Nested class UpdatePoliciesTests { + @Test + void updatePolicies_whenFallbackPolicy_shouldReturnHttpStatus400() { + assertThatThrownBy(() -> testee.updatePolicies( + new UpdatePolicyRequest(OffsetDateTime.now(clock), List.of("BPNL1234567890AB", "BPNL1234567890CD"), + List.of(CONFIGURED_DEFAULT_POLICY_ID)))).isInstanceOf(ResponseStatusException.class) + .hasMessageContaining("400 BAD_REQUEST") + .hasMessageContaining(CONFIGURED_DEFAULT_POLICY_ID); + } + + @Test + void updatePolicy_whenFallbackPolicy_shouldReturnHttpStatus400() { + assertThatThrownBy(() -> testee.updatePolicy(CONFIGURED_DEFAULT_POLICY_ID, OffsetDateTime.now(clock), + List.of("BPNL1234567890AB", "BPNL1234567890CD"))).isInstanceOf(ResponseStatusException.class) + .hasMessageContaining("400 BAD_REQUEST") + .hasMessageContaining( + CONFIGURED_DEFAULT_POLICY_ID); + } + @Test void updatePolicies_shouldUpdateBpnAndValidUntil() { // ARRANGE