[ML] Validate ML filter_id

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/MlFilter.java

@@ -15,6 +15,9 @@
 import org.elasticsearch.common.xcontent.ToXContentObject;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.xpack.core.ml.MlMetaIndex;
+import org.elasticsearch.xpack.core.ml.job.messages.Messages;
+import org.elasticsearch.xpack.core.ml.utils.ExceptionsHelper;
+import org.elasticsearch.xpack.core.ml.utils.MlStrings;
 
 import java.io.IOException;
 import java.util.Arrays;
@@ -57,7 +60,7 @@ private static ObjectParser<Builder, Void> createParser(boolean ignoreUnknownFie
  private final SortedSet<String> items;
 
  private MlFilter(String id, String description, SortedSet<String> items) {
- this.id = Objects.requireNonNull(id, ID.getPreferredName() + " must not be null");
+ this.id = Objects.requireNonNull(id);
  this.description = description;
  this.items = Objects.requireNonNull(items, ITEMS.getPreferredName() + " must not be null");
  }
@@ -178,6 +181,10 @@ public Builder setItems(String... items) {
  }
 
  public MlFilter build() {
+ ExceptionsHelper.requireNonNull(id, MlFilter.ID.getPreferredName());
+ if (!MlStrings.isValidId(id)) {
+ throw ExceptionsHelper.badRequestException(Messages.getMessage(Messages.INVALID_ID, ID.getPreferredName(), id));
+ }
  return new MlFilter(id, description, items);
  }
  }

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/action/PutFilterActionRequestTests.java

@@ -12,7 +12,7 @@
 
 public class PutFilterActionRequestTests extends AbstractStreamableXContentTestCase<Request> {
 
- private final String filterId = randomAlphaOfLengthBetween(1, 20);
+ private final String filterId = MlFilterTests.randomValidFilterId();
 
  @Override
  protected Request createTestInstance() {

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/MlFilterTests.java

@@ -5,6 +5,8 @@
  */
 package org.elasticsearch.xpack.core.ml.job.config;
 
+import com.carrotsearch.randomizedtesting.generators.CodepointSetGenerator;
+import org.elasticsearch.ElasticsearchStatusException;
 import org.elasticsearch.common.io.stream.Writeable.Reader;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.json.JsonXContent;
@@ -17,6 +19,7 @@
 import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.startsWith;
 
 public class MlFilterTests extends AbstractSerializingTestCase<MlFilter> {
 
@@ -30,7 +33,12 @@ protected MlFilter createTestInstance() {
  }
 
  public static MlFilter createRandom() {
- return createRandom(randomAlphaOfLengthBetween(1, 20));
+ return createRandom(randomValidFilterId());
+ }
+
+ public static String randomValidFilterId() {
+ CodepointSetGenerator generator = new CodepointSetGenerator("abcdefghijklmnopqrstuvwxyz".toCharArray());
+ return generator.ofCodePointsLength(random(), 10, 10);
  }
 
  public static MlFilter createRandom(String filterId) {
@@ -58,13 +66,13 @@ protected MlFilter doParseInstance(XContentParser parser) {
  }
 
  public void testNullId() {
- NullPointerException ex = expectThrows(NullPointerException.class, () -> MlFilter.builder(null).build());
- assertEquals(MlFilter.ID.getPreferredName() + " must not be null", ex.getMessage());
+ Exception ex = expectThrows(IllegalArgumentException.class, () -> MlFilter.builder(null).build());
+ assertEquals("[filter_id] must not be null.", ex.getMessage());
  }
 
  public void testNullItems() {
  NullPointerException ex = expectThrows(NullPointerException.class,
- () -> MlFilter.builder(randomAlphaOfLength(20)).setItems((SortedSet<String>) null).build());
+ () -> MlFilter.builder(randomValidFilterId()).setItems((SortedSet<String>) null).build());
  assertEquals(MlFilter.ITEMS.getPreferredName() + " must not be null", ex.getMessage());
  }
 
@@ -89,6 +97,11 @@ public void testLenientParser() throws IOException {
  }
  }
 
+ public void testInvalidId() {
+ ElasticsearchStatusException e = expectThrows(ElasticsearchStatusException.class, () -> MlFilter.builder("Invalid id").build());
+ assertThat(e.getMessage(), startsWith("Invalid filter_id; 'Invalid id' can contain lowercase"));
+ }
+
  public void testItemsAreSorted() {
  MlFilter filter = MlFilter.builder("foo").setItems("c", "b", "a").build();
  assertThat(filter.getItems(), contains("a", "b", "c"));

x-pack/plugin/src/test/resources/rest-api-spec/test/ml/filter_crud.yml

@@ -109,6 +109,18 @@ setup:
  filter_id: "filter-foo"
  from: 0
  size: 1
+
+---
+"Test create filter given invalid filter_id":
+ - do:
+ catch: bad_request
+ xpack.ml.put_filter:
+ filter_id: Invalid
+ body: >
+ {
+ "description": "this id is invalid due to an upper case character"
+ }
+
 ---
 "Test create filter api":
  - do:

x-pack/qa/smoke-test-ml-with-security/build.gradle

@@ -39,6 +39,7 @@ integTestRunner {
  'ml/delete_model_snapshot/Test delete snapshot missing job_id',
  'ml/delete_model_snapshot/Test delete with in-use model',
  'ml/filter_crud/Test create filter api with mismatching body ID',
+ 'ml/filter_crud/Test create filter given invalid filter_id',
  'ml/filter_crud/Test get filter API with bad ID',
  'ml/filter_crud/Test invalid param combinations',
  'ml/filter_crud/Test non-existing filter',