[ML] Validate ML filter_id (#31535)

Like job and datafeed ids, the filter id
should be validated with the same rules
to avoid document ids that can be problematic.

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',