From 3e3e5843414d2d4b098ad91710caf372d2a6608b Mon Sep 17 00:00:00 2001 From: Maciej Moscicki Date: Wed, 3 Jul 2024 16:18:49 +0200 Subject: [PATCH] Improved topic validation (#1878) * Revert "Revert accidental "improve fallback to remote dc validation" (#1877)" This reverts commit 19beb5856ad4184fdf98057f5bd61aa8cc2d7a0e. * adjust tests --- .../topic/validator/TopicValidator.java | 14 +++++++---- .../topic/validator/TopicValidatorTest.groovy | 3 ++- ...lidatorWithRealApiPreconditionsTest.groovy | 4 +++- .../management/TopicManagementTest.java | 23 +++++++++++-------- 4 files changed, 27 insertions(+), 17 deletions(-) diff --git a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/topic/validator/TopicValidator.java b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/topic/validator/TopicValidator.java index eaa69ef2cd..fd1e22d8da 100644 --- a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/topic/validator/TopicValidator.java +++ b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/topic/validator/TopicValidator.java @@ -9,6 +9,7 @@ import pl.allegro.tech.hermes.api.RetentionTime; import pl.allegro.tech.hermes.api.Topic; import pl.allegro.tech.hermes.management.api.validator.ApiPreconditions; +import pl.allegro.tech.hermes.management.config.TopicProperties; import pl.allegro.tech.hermes.management.domain.auth.RequestUser; import pl.allegro.tech.hermes.management.domain.owner.validator.OwnerIdValidator; import pl.allegro.tech.hermes.management.domain.topic.CreatorRights; @@ -27,18 +28,21 @@ public class TopicValidator { private final TopicLabelsValidator topicLabelsValidator; private final SchemaRepository schemaRepository; private final ApiPreconditions apiPreconditions; + private final TopicProperties topicProperties; @Autowired public TopicValidator(OwnerIdValidator ownerIdValidator, ContentTypeValidator contentTypeValidator, TopicLabelsValidator topicLabelsValidator, SchemaRepository schemaRepository, - ApiPreconditions apiPreconditions) { + ApiPreconditions apiPreconditions, + TopicProperties topicProperties) { this.ownerIdValidator = ownerIdValidator; this.contentTypeValidator = contentTypeValidator; this.topicLabelsValidator = topicLabelsValidator; this.schemaRepository = schemaRepository; this.apiPreconditions = apiPreconditions; + this.topicProperties = topicProperties; } public void ensureCreatedTopicIsValid(Topic created, RequestUser createdBy, CreatorRights creatorRights) { @@ -47,8 +51,8 @@ public void ensureCreatedTopicIsValid(Topic created, RequestUser createdBy, Crea checkContentType(created); checkTopicLabels(created); - if (created.isFallbackToRemoteDatacenterEnabled() && !createdBy.isAdmin()) { - throw new TopicValidationException("User is not allowed to enable fallback to remote datacenter"); + if ((created.isFallbackToRemoteDatacenterEnabled() != topicProperties.isDefaultFallbackToRemoteDatacenterEnabled()) && !createdBy.isAdmin()) { + throw new TopicValidationException("User is not allowed to set non-default fallback to remote datacenter for this topic"); } if (created.getChaos().mode() != ChaosMode.DISABLED && !createdBy.isAdmin()) { @@ -72,8 +76,8 @@ public void ensureUpdatedTopicIsValid(Topic updated, Topic previous, RequestUser checkOwner(updated); checkTopicLabels(updated); - if (!previous.isFallbackToRemoteDatacenterEnabled() && updated.isFallbackToRemoteDatacenterEnabled() && !modifiedBy.isAdmin()) { - throw new TopicValidationException("User is not allowed to enable fallback to remote datacenter"); + if ((previous.isFallbackToRemoteDatacenterEnabled() != updated.isFallbackToRemoteDatacenterEnabled()) && !modifiedBy.isAdmin()) { + throw new TopicValidationException("User is not allowed to update fallback to remote datacenter for this topic"); } if (!previous.getChaos().equals(updated.getChaos()) && !modifiedBy.isAdmin()) { diff --git a/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/topic/validator/TopicValidatorTest.groovy b/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/topic/validator/TopicValidatorTest.groovy index 42a18fbc38..f92a84181a 100644 --- a/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/topic/validator/TopicValidatorTest.groovy +++ b/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/topic/validator/TopicValidatorTest.groovy @@ -32,6 +32,7 @@ class TopicValidatorTest extends Specification { def contentTypeWhitelistValidator = Stub(ContentTypeValidator) def apiPreconditions = Stub(ApiPreconditions) def topicLabelsValidator + def topicProperties = new TopicProperties() @Subject TopicValidator topicValidator @@ -42,7 +43,7 @@ class TopicValidatorTest extends Specification { topicProperties.setAllowedTopicLabels(allowedLabels) topicLabelsValidator = new TopicLabelsValidator(topicProperties) - topicValidator = new TopicValidator(ownerDescriptorValidator, contentTypeWhitelistValidator, topicLabelsValidator, schemaRepository, apiPreconditions) + topicValidator = new TopicValidator(ownerDescriptorValidator, contentTypeWhitelistValidator, topicLabelsValidator, schemaRepository, apiPreconditions, topicProperties) } def "topic with basic properties when creating should be valid"() { diff --git a/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/topic/validator/TopicValidatorWithRealApiPreconditionsTest.groovy b/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/topic/validator/TopicValidatorWithRealApiPreconditionsTest.groovy index 14aa7ec92e..132be2c38a 100644 --- a/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/topic/validator/TopicValidatorWithRealApiPreconditionsTest.groovy +++ b/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/topic/validator/TopicValidatorWithRealApiPreconditionsTest.groovy @@ -2,6 +2,7 @@ package pl.allegro.tech.hermes.management.domain.topic.validator import pl.allegro.tech.hermes.api.RetentionTime import pl.allegro.tech.hermes.management.api.validator.ApiPreconditions +import pl.allegro.tech.hermes.management.config.TopicProperties import pl.allegro.tech.hermes.management.domain.auth.TestRequestUser import pl.allegro.tech.hermes.management.domain.owner.validator.OwnerIdValidator import pl.allegro.tech.hermes.schema.SchemaRepository @@ -27,9 +28,10 @@ class TopicValidatorWithRealApiPreconditionsTest extends Specification { def ownerDescriptorValidator = Stub(OwnerIdValidator) def contentTypeWhitelistValidator = Stub(ContentTypeValidator) def topicLabelsValidator = Stub(TopicLabelsValidator) + def topicProperties = new TopicProperties() @Subject - def topicValidator = new TopicValidator(ownerDescriptorValidator, contentTypeWhitelistValidator, topicLabelsValidator, schemaRepository, new ApiPreconditions()) + def topicValidator = new TopicValidator(ownerDescriptorValidator, contentTypeWhitelistValidator, topicLabelsValidator, schemaRepository, new ApiPreconditions(), topicProperties) @Unroll def "creating and updating topic with up to 7 days retention time should be valid"() { diff --git a/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/TopicManagementTest.java b/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/TopicManagementTest.java index d097604b40..461492ccd9 100644 --- a/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/TopicManagementTest.java +++ b/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/TopicManagementTest.java @@ -603,7 +603,7 @@ public void shouldCreateTopicEvenIfExistsInBrokers() { } @Test - public void shouldNotAllowNonAdminUserCreateTopicWithFallbackToRemoteDatacenterEnabled() { + public void shouldNotAllowNonAdminUserCreateTopicWithNonDefaultFallbackToRemoteDatacenter() { // given TestSecurityProvider.setUserIsAdmin(false); TopicWithSchema topic = topicWithSchema( @@ -619,11 +619,11 @@ public void shouldNotAllowNonAdminUserCreateTopicWithFallbackToRemoteDatacenterE //then response.expectStatus().isBadRequest(); assertThat(response.expectBody(String.class).returnResult().getResponseBody()) - .contains("User is not allowed to enable fallback to remote datacenter"); + .contains("User is not allowed to set non-default fallback to remote datacenter"); } @Test - public void shouldAllowAdminUserCreateTopicWithFallbackToRemoteDatacenterEnabled() { + public void shouldAllowAdminUserCreateTopicWithNonDefaultFallbackToRemoteDatacenterEnabled() { // given TestSecurityProvider.setUserIsAdmin(true); TopicWithSchema topic = topicWithSchema( @@ -641,11 +641,12 @@ public void shouldAllowAdminUserCreateTopicWithFallbackToRemoteDatacenterEnabled } @Test - public void shouldNotAllowNonAdminUserToEnableFallbackToRemoteDatacenter() { + public void shouldNotAllowNonAdminUserToChangeFallbackToRemoteDatacenter() { // given - Topic topic = hermes.initHelper().createTopic(topicWithRandomName().build()); + Topic topic = hermes.initHelper().createTopic(topicWithRandomName() + .withFallbackToRemoteDatacenterEnabled().build()); TestSecurityProvider.setUserIsAdmin(false); - PatchData patchData = PatchData.from(ImmutableMap.of("fallbackToRemoteDatacenterEnabled", true)); + PatchData patchData = PatchData.from(ImmutableMap.of("fallbackToRemoteDatacenterEnabled", false)); // when WebTestClient.ResponseSpec response = hermes.api().updateTopic(topic.getQualifiedName(), patchData); @@ -653,15 +654,17 @@ public void shouldNotAllowNonAdminUserToEnableFallbackToRemoteDatacenter() { //then response.expectStatus().isBadRequest(); assertThat(response.expectBody(String.class).returnResult().getResponseBody()) - .contains("User is not allowed to enable fallback to remote datacenter"); + .contains("User is not allowed to update fallback to remote datacenter for this topic"); } @Test - public void shouldAllowAdminUserToEnableFallbackToRemoteDatacenter() { + public void shouldAllowAdminUserToChangeFallbackToRemoteDatacenter() { // given - Topic topic = hermes.initHelper().createTopic(topicWithRandomName().build()); + Topic topic = hermes.initHelper().createTopic(topicWithRandomName() + .withFallbackToRemoteDatacenterEnabled() + .build()); TestSecurityProvider.setUserIsAdmin(true); - PatchData patchData = PatchData.from(ImmutableMap.of("fallbackToRemoteDatacenterEnabled", true)); + PatchData patchData = PatchData.from(ImmutableMap.of("fallbackToRemoteDatacenterEnabled", false)); // when WebTestClient.ResponseSpec response = hermes.api().updateTopic(topic.getQualifiedName(), patchData);