From 8e838ea12e37ae10db6f844a8bcf4a1158c89976 Mon Sep 17 00:00:00 2001 From: Dimitris Athanasiou Date: Mon, 2 Jul 2018 12:48:04 +0100 Subject: [PATCH] [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. --- .../xpack/core/ml/job/config/MlFilter.java | 9 +++++++- .../action/PutFilterActionRequestTests.java | 2 +- .../core/ml/job/config/MlFilterTests.java | 21 +++++++++++++++---- .../rest-api-spec/test/ml/filter_crud.yml | 12 +++++++++++ .../smoke-test-ml-with-security/build.gradle | 1 + 5 files changed, 39 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/MlFilter.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/MlFilter.java index b45ce73f124..c55ba401a2f 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/MlFilter.java +++ b/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.ObjectParser; 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 @@ public class MlFilter implements ToXContentObject, Writeable { private final SortedSet items; private MlFilter(String id, String description, SortedSet 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 class MlFilter implements ToXContentObject, Writeable { } 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); } } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/action/PutFilterActionRequestTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/action/PutFilterActionRequestTests.java index dfc3f5f37f4..bed0ab775af 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/action/PutFilterActionRequestTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/action/PutFilterActionRequestTests.java @@ -12,7 +12,7 @@ import org.elasticsearch.xpack.core.ml.job.config.MlFilterTests; public class PutFilterActionRequestTests extends AbstractStreamableXContentTestCase { - private final String filterId = randomAlphaOfLengthBetween(1, 20); + private final String filterId = MlFilterTests.randomValidFilterId(); @Override protected Request createTestInstance() { diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/MlFilterTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/MlFilterTests.java index c8d8527dc01..a89250330f0 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/MlFilterTests.java +++ b/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 java.util.TreeSet; 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 { @@ -30,7 +33,12 @@ public class MlFilterTests extends AbstractSerializingTestCase { } 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 @@ public class MlFilterTests extends AbstractSerializingTestCase { } 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) null).build()); + () -> MlFilter.builder(randomValidFilterId()).setItems((SortedSet) null).build()); assertEquals(MlFilter.ITEMS.getPreferredName() + " must not be null", ex.getMessage()); } @@ -89,6 +97,11 @@ public class MlFilterTests extends AbstractSerializingTestCase { } } + 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")); diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/filter_crud.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/filter_crud.yml index d787e07b8c2..6e9579a0613 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/filter_crud.yml +++ b/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: diff --git a/x-pack/qa/smoke-test-ml-with-security/build.gradle b/x-pack/qa/smoke-test-ml-with-security/build.gradle index 58e5eca3600..2a12aa2f28d 100644 --- a/x-pack/qa/smoke-test-ml-with-security/build.gradle +++ b/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',