Skip to content

Commit

Permalink
Add name validation to task definitions and workflow definitions (#315)
Browse files Browse the repository at this point in the history
  • Loading branch information
codehackerr authored Nov 27, 2024
1 parent 8fb219f commit 4ae5b18
Show file tree
Hide file tree
Showing 15 changed files with 151 additions and 12 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/*
* Copyright 2020 Conductor Authors.
* <p>
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
* <p>
* http://www.apache.org/licenses/LICENSE-2.0
* <p>
* 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.
*/
package com.netflix.conductor.common.constraints;

import java.lang.annotation.Documented;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

import org.springframework.beans.factory.annotation.Value;

import jakarta.validation.Constraint;
import jakarta.validation.ConstraintValidator;
import jakarta.validation.ConstraintValidatorContext;
import jakarta.validation.Payload;

import static java.lang.annotation.ElementType.FIELD;

/**
* This constraint class validates following things.
*
* <ul>
* <li>1. Name is valid or not
* </ul>
*/
@Documented
@Constraint(validatedBy = ValidNameConstraint.NameValidator.class)
@Target({FIELD})
@Retention(RetentionPolicy.RUNTIME)
public @interface ValidNameConstraint {

String message() default "";

Class<?>[] groups() default {};

Class<? extends Payload>[] payload() default {};

class NameValidator implements ConstraintValidator<ValidNameConstraint, String> {

private static final String NAME_PATTERN = "^[A-Za-z0-9_<>{}#\\s-]+$";
public static final String INVALID_NAME_MESSAGE =
"Allowed characters are alphanumeric, underscores, spaces, hyphens, and special characters like <, >, {, }, #";

@Value("${conductor.app.workflow.name-validation.enabled}")
private boolean nameValidationEnabled;

@Override
public void initialize(ValidNameConstraint constraintAnnotation) {}

@Override
public boolean isValid(String name, ConstraintValidatorContext context) {
boolean valid = name == null || !nameValidationEnabled || name.matches(NAME_PATTERN);
if (!valid) {
context.disableDefaultConstraintViolation();
context.buildConstraintViolationWithTemplate(
"Invalid name '" + name + "'. " + INVALID_NAME_MESSAGE)
.addConstraintViolation();
}
return valid;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@
import com.netflix.conductor.annotations.protogen.ProtoEnum;
import com.netflix.conductor.annotations.protogen.ProtoField;
import com.netflix.conductor.annotations.protogen.ProtoMessage;
import com.netflix.conductor.common.constraints.NoSemiColonConstraint;
import com.netflix.conductor.common.constraints.OwnerEmailMandatoryConstraint;
import com.netflix.conductor.common.constraints.TaskReferenceNameUniqueConstraint;
import com.netflix.conductor.common.constraints.ValidNameConstraint;
import com.netflix.conductor.common.metadata.Auditable;
import com.netflix.conductor.common.metadata.SchemaDef;
import com.netflix.conductor.common.metadata.tasks.TaskType;
Expand All @@ -39,8 +39,7 @@ public enum TimeoutPolicy {

@NotEmpty(message = "WorkflowDef name cannot be null or empty")
@ProtoField(id = 1)
@NoSemiColonConstraint(
message = "Workflow name cannot contain the following set of characters: ':'")
@ValidNameConstraint
private String name;

@ProtoField(id = 2)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*
* Copyright 2024 Conductor Authors.
* <p>
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
* <p>
* http://www.apache.org/licenses/LICENSE-2.0
* <p>
* 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.
*/
package com.netflix.conductor.common.constraints;

import org.junit.Test;
import org.springframework.test.util.ReflectionTestUtils;

import jakarta.validation.ConstraintValidatorContext;

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

public class NameValidatorTest {
@Test
public void nameWithAllowedCharactersIsValid() {
ValidNameConstraint.NameValidator nameValidator = new ValidNameConstraint.NameValidator();
assertTrue(nameValidator.isValid("workflowDef", null));
}

@Test
public void nonAllowedCharactersInNameIsInvalid() {
ValidNameConstraint.NameValidator nameValidator = new ValidNameConstraint.NameValidator();
ConstraintValidatorContext context = mock(ConstraintValidatorContext.class);
ConstraintValidatorContext.ConstraintViolationBuilder builder =
mock(ConstraintValidatorContext.ConstraintViolationBuilder.class);
when(context.buildConstraintViolationWithTemplate(anyString())).thenReturn(builder);

ReflectionTestUtils.setField(nameValidator, "nameValidationEnabled", true);

assertFalse(nameValidator.isValid("workflowDef@", context));
}

// Null should be tested by @NotEmpty or @NotNull
@Test
public void nullIsValid() {
ValidNameConstraint.NameValidator nameValidator = new ValidNameConstraint.NameValidator();
assertTrue(nameValidator.isValid(null, null));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import org.junit.Before;
import org.junit.Test;
import org.springframework.test.context.TestPropertySource;

import com.netflix.conductor.common.metadata.tasks.TaskType;
import com.netflix.conductor.common.metadata.workflow.WorkflowDef;
Expand All @@ -33,6 +34,7 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

@TestPropertySource(properties = "conductor.app.workflow.name-validation.enabled=true")
public class WorkflowDefValidatorTest {

@Before
Expand Down
1 change: 1 addition & 0 deletions common/src/test/resources/application.properties
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
conductor.app.workflow.name-validation.enabled=true
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.springframework.boot.autoconfigure.EnableAutoConfiguration;
import org.springframework.boot.test.context.TestConfiguration;
import org.springframework.context.annotation.Bean;
import org.springframework.test.context.TestPropertySource;
import org.springframework.test.context.junit4.SpringRunner;

import com.netflix.conductor.common.metadata.events.EventHandler;
Expand Down Expand Up @@ -50,6 +51,7 @@

@SuppressWarnings("SpringJavaAutowiredMembersInspection")
@RunWith(SpringRunner.class)
@TestPropertySource(properties = "conductor.app.workflow.name-validation.enabled=true")
@EnableAutoConfiguration
public class MetadataServiceTest {

Expand All @@ -64,6 +66,7 @@ public MetadataDAO metadataDAO() {
public ConductorProperties properties() {
ConductorProperties properties = mock(ConductorProperties.class);
when(properties.isOwnerEmailMandatory()).thenReturn(true);

return properties;
}

Expand Down Expand Up @@ -415,7 +418,7 @@ public void testRegisterWorkflowDefInvalidName() {
assertTrue(messages.contains("WorkflowTask list cannot be empty"));
assertTrue(
messages.contains(
"Workflow name cannot contain the following set of characters: ':'"));
"Invalid name 'invalid:name'. Allowed characters are alphanumeric, underscores, spaces, hyphens, and special characters like <, >, {, }, #"));
throw ex;
}
fail("metadataService.registerWorkflowDef did not throw ConstraintViolationException !");
Expand All @@ -434,7 +437,7 @@ public void testValidateWorkflowDefInvalidName() {
assertTrue(messages.contains("WorkflowTask list cannot be empty"));
assertTrue(
messages.contains(
"Workflow name cannot contain the following set of characters: ':'"));
"Invalid name 'invalid:name'. Allowed characters are alphanumeric, underscores, spaces, hyphens, and special characters like <, >, {, }, #"));
throw ex;
}
fail("metadataService.validateWorkflowDef did not throw ConstraintViolationException !");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,15 @@ import com.fasterxml.jackson.databind.ObjectMapper
import com.netflix.conductor.common.metadata.tasks.TaskDef
import com.netflix.conductor.common.metadata.tasks.TaskResult
import com.netflix.conductor.common.metadata.tasks.TaskType
import com.netflix.conductor.common.metadata.workflow.StartWorkflowRequest
import com.netflix.conductor.common.metadata.workflow.WorkflowDef
import com.netflix.conductor.common.metadata.workflow.WorkflowTask
import com.netflix.conductor.common.run.Workflow
import com.netflix.conductor.core.execution.StartWorkflowInput
import com.netflix.conductor.test.base.AbstractSpecification
import org.springframework.beans.factory.annotation.Autowired
import org.springframework.test.context.TestPropertySource
import spock.lang.Shared

@TestPropertySource(properties = "conductor.app.workflow.name-validation.enabled=true")
class KafkaPublishTaskSpec extends AbstractSpecification {

@Autowired
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@
"spring.datasource.password=root",
"spring.datasource.hikari.maximum-pool-size=8",
"spring.datasource.hikari.minimum-idle=300000",
"conductor.elasticsearch.version=7"
"conductor.elasticsearch.version=7",
"conductor.app.workflow.name-validation.enabled=true"
})
public class MySQLGrpcEndToEndTest extends AbstractGrpcEndToEndTest {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@
@TestPropertySource(
properties = {
"conductor.workflow-execution-lock.type=postgres",
"spring.flyway.clean-disabled=false"
"spring.flyway.clean-disabled=false",
"conductor.app.workflow.name-validation.enabled=true"
})
@SpringBootTest
public class PostgresLockDAOTest {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@
"spring.datasource.password=postgres",
"spring.datasource.hikari.maximum-pool-size=8",
"spring.datasource.hikari.minimum-idle=300000",
"spring.flyway.clean-disabled=true"
"spring.flyway.clean-disabled=true",
"conductor.app.workflow.name-validation.enabled=true"
})
public class PostgresGrpcEndToEndTest extends AbstractGrpcEndToEndTest {

Expand Down
2 changes: 2 additions & 0 deletions server/src/main/resources/application.properties
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ conductor.default-event-queue.type=sqs
#disable locking during workflow execution
conductor.app.workflow-execution-lock-enabled=false
conductor.workflow-execution-lock.type=noop_lock
# enable name validation on workflow/task definitions
conductor.app.workflow.name-validation.enabled=false

#Redis cluster settings for locking module
# conductor.redis-lock.serverType=single
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ package com.netflix.conductor.test.resiliency

import org.springframework.beans.factory.annotation.Autowired
import org.springframework.http.HttpStatus
import org.springframework.test.context.TestPropertySource

import com.netflix.conductor.common.metadata.tasks.Task
import com.netflix.conductor.common.metadata.tasks.TaskResult
Expand All @@ -36,6 +37,7 @@ import com.netflix.conductor.test.base.AbstractResiliencySpecification
* 2. Succeeds
* 3. Doesn't involve QueueDAO
*/
@TestPropertySource(properties = "conductor.app.workflow.name-validation.enabled=true")
class QueueResiliencySpec extends AbstractResiliencySpecification {

@Autowired
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
package com.netflix.conductor.test.resiliency

import org.springframework.beans.factory.annotation.Autowired
import org.springframework.test.context.TestPropertySource

import com.netflix.conductor.common.metadata.tasks.Task
import com.netflix.conductor.common.run.Workflow
Expand All @@ -22,7 +23,7 @@ import com.netflix.conductor.test.base.AbstractResiliencySpecification
import spock.lang.Shared

import static com.netflix.conductor.test.util.WorkflowTestUtil.verifyPolledAndAcknowledgedTask

@TestPropertySource(properties = "conductor.app.workflow.name-validation.enabled=true")
class TaskResiliencySpec extends AbstractResiliencySpecification {

@Autowired
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ conductor.workflow-reconciler.enabled=true
conductor.workflow-repair-service.enabled=false

conductor.app.workflow-execution-lock-enabled=false
conductor.app.workflow.name-validation.enabled=true

conductor.app.workflow-input-payload-size-threshold=10KB
conductor.app.max-workflow-input-payload-size-threshold=10240KB
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@
"conductor.workflow-status-listener.type=queue_publisher",
"conductor.workflow-status-listener.queue-publisher.successQueue=dummy",
"conductor.workflow-status-listener.queue-publisher.failureQueue=dummy",
"conductor.workflow-status-listener.queue-publisher.finalizeQueue=final"
"conductor.workflow-status-listener.queue-publisher.finalizeQueue=final",
"conductor.app.workflow.name-validation.enabled=true"
})
@TestPropertySource(locations = "classpath:application-integrationtest.properties")
public class WorkflowStatusPublisherIntegrationTest {
Expand Down

0 comments on commit 4ae5b18

Please sign in to comment.