Skip to content

Commit

Permalink
Improved topic validation (#1878)
Browse files Browse the repository at this point in the history 
* Revert "Revert accidental "improve fallback to remote dc validation" (#1877)"

This reverts commit 19beb58.

* adjust tests
  • Loading branch information
@moscicky
moscicky authored Jul 3, 2024
1 parent 19beb58 commit 3e3e584
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 17 deletions.
14 changes: 9 additions & 5 deletions ...rc/main/java/pl/allegro/tech/hermes/management/domain/topic/validator/TopicValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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) {
Expand All @@ -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()) {
Expand All @@ -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()) {
Expand Down
3 changes: 2 additions & 1 deletion ...groovy/pl/allegro/tech/hermes/management/domain/topic/validator/TopicValidatorTest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"() {
Expand Down
4 changes: 3 additions & 1 deletion ...ermes/management/domain/topic/validator/TopicValidatorWithRealApiPreconditionsTest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"() {
Expand Down
23 changes: 13 additions & 10 deletions ...tionTest/java/pl/allegro/tech/hermes/integrationtests/management/TopicManagementTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -603,7 +603,7 @@ public void shouldCreateTopicEvenIfExistsInBrokers() {
}

@Test
public void shouldNotAllowNonAdminUserCreateTopicWithFallbackToRemoteDatacenterEnabled() {
public void shouldNotAllowNonAdminUserCreateTopicWithNonDefaultFallbackToRemoteDatacenter() {
// given
TestSecurityProvider.setUserIsAdmin(false);
TopicWithSchema topic = topicWithSchema(
Expand All @@ -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(
Expand All @@ -641,27 +641,30 @@ 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);

//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);
Expand Down

0 comments on commit 3e3e584

Please sign in to comment.