From 32dbfba0c2bdb8a8cec28ba7b237db0d9371491f Mon Sep 17 00:00:00 2001 From: Jack Conradson Date: Wed, 12 Jul 2017 07:56:08 -0700 Subject: [PATCH] Disallow lang to specified in requests where a stored script is used. (elastic/x-pack-elasticsearch#1949) Requests that execute a stored script will no longer be allowed to specify the lang of the script. This information is stored in the cluster state making only an id necessary to execute against. Putting a stored script will still require a lang. Original commit: elastic/x-pack-elasticsearch@926a7b2d8604ab482d1af67b09ae7b5a498498ba --- .../xpack/common/text/TextTemplate.java | 9 +-------- .../xpack/common/text/TextTemplateEngine.java | 3 ++- .../SecurityIndexSearcherWrapper.java | 4 +++- .../search/WatcherSearchTemplateRequest.java | 8 -------- .../search/WatcherSearchTemplateService.java | 4 +++- .../xpack/common/text/TextTemplateTests.java | 11 +++++++---- .../watcher/support/WatcherUtilsTests.java | 12 ++++++++---- .../WatcherSearchTemplateRequestTests.java | 2 +- .../test/integration/BasicWatcherTests.java | 2 +- .../transform/TransformIntegrationTests.java | 2 +- .../transform/script/ScriptTransformTests.java | 17 +++++------------ .../test/watcher_mustache/30_search_input.yml | 1 - 12 files changed, 32 insertions(+), 43 deletions(-) diff --git a/plugin/src/main/java/org/elasticsearch/xpack/common/text/TextTemplate.java b/plugin/src/main/java/org/elasticsearch/xpack/common/text/TextTemplate.java index 1f1060d57b0..804bf3c0a50 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/common/text/TextTemplate.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/common/text/TextTemplate.java @@ -47,7 +47,7 @@ public class TextTemplate implements ToXContent { if (params == null) { params = new HashMap<>(); } - this.script = new Script(type, Script.DEFAULT_TEMPLATE_LANG, template, options, params); + this.script = new Script(type, type == ScriptType.STORED ? null : Script.DEFAULT_TEMPLATE_LANG, template, options, params); this.inlineTemplate = null; } @@ -116,13 +116,6 @@ public class TextTemplate implements ToXContent { return new TextTemplate(parser.text()); } else { Script template = Script.parse(parser, Script.DEFAULT_TEMPLATE_LANG); - - // for deprecation of stored script namespaces the default lang is ignored, - // so the template lang must be set for a stored script - if (template.getType() == ScriptType.STORED) { - template = new Script(ScriptType.STORED, Script.DEFAULT_TEMPLATE_LANG, template.getIdOrCode(), template.getParams()); - } - return new TextTemplate(template); } } diff --git a/plugin/src/main/java/org/elasticsearch/xpack/common/text/TextTemplateEngine.java b/plugin/src/main/java/org/elasticsearch/xpack/common/text/TextTemplateEngine.java index 89ec30d4400..0906d99eb3b 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/common/text/TextTemplateEngine.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/common/text/TextTemplateEngine.java @@ -51,7 +51,8 @@ public class TextTemplateEngine extends AbstractComponent { options.put(Script.CONTENT_TYPE_OPTION, mediaType); } - Script script = new Script(textTemplate.getType(), "mustache", template, options, mergedModel); + Script script = new Script(textTemplate.getType(), + textTemplate.getType() == ScriptType.STORED ? null : "mustache", template, options, mergedModel); TemplateScript.Factory compiledTemplate = service.compile(script, Watcher.SCRIPT_TEMPLATE_CONTEXT); return compiledTemplate.newInstance(model).execute(); } diff --git a/plugin/src/main/java/org/elasticsearch/xpack/security/authz/accesscontrol/SecurityIndexSearcherWrapper.java b/plugin/src/main/java/org/elasticsearch/xpack/security/authz/accesscontrol/SecurityIndexSearcherWrapper.java index b1a2bf8f0b5..3ca9f6093ef 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/security/authz/accesscontrol/SecurityIndexSearcherWrapper.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/security/authz/accesscontrol/SecurityIndexSearcherWrapper.java @@ -58,6 +58,7 @@ import org.elasticsearch.index.shard.ShardUtils; import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptService; +import org.elasticsearch.script.ScriptType; import org.elasticsearch.script.TemplateScript; import org.elasticsearch.xpack.security.authc.Authentication; import org.elasticsearch.xpack.security.authz.AuthorizationService; @@ -272,7 +273,8 @@ public class SecurityIndexSearcherWrapper extends IndexSearcherWrapper { userModel.put("metadata", Collections.unmodifiableMap(user.metadata())); params.put("_user", userModel); // Always enforce mustache script lang: - script = new Script(script.getType(), "mustache", script.getIdOrCode(), script.getOptions(), params); + script = new Script(script.getType(), + script.getType() == ScriptType.STORED ? null : "mustache", script.getIdOrCode(), script.getOptions(), params); TemplateScript compiledTemplate = scriptService.compile(script, TemplateScript.CONTEXT).newInstance(script.getParams()); return compiledTemplate.execute(); } else { diff --git a/plugin/src/main/java/org/elasticsearch/xpack/watcher/support/search/WatcherSearchTemplateRequest.java b/plugin/src/main/java/org/elasticsearch/xpack/watcher/support/search/WatcherSearchTemplateRequest.java index 5f6eac779df..bf368ea6e36 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/watcher/support/search/WatcherSearchTemplateRequest.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/watcher/support/search/WatcherSearchTemplateRequest.java @@ -56,7 +56,6 @@ public class WatcherSearchTemplateRequest implements ToXContentObject { public WatcherSearchTemplateRequest(String[] indices, String[] types, SearchType searchType, IndicesOptions indicesOptions, Script template) { - assert template == null || Script.DEFAULT_TEMPLATE_LANG.equals(template.getLang()); this.indices = indices; this.types = types; this.searchType = searchType; @@ -248,13 +247,6 @@ public class WatcherSearchTemplateRequest implements ToXContentObject { DEFAULT_INDICES_OPTIONS); } else if (TEMPLATE_FIELD.match(currentFieldName)) { template = Script.parse(parser, Script.DEFAULT_TEMPLATE_LANG); - - // for deprecation of stored script namespaces the default lang is ignored, - // so the template lang must be set for a stored script - if (template.getType() == ScriptType.STORED) { - template = new Script( - ScriptType.STORED, Script.DEFAULT_TEMPLATE_LANG, template.getIdOrCode(), template.getParams()); - } } else { throw new ElasticsearchParseException("could not read search request. unexpected object field [" + currentFieldName + "]"); diff --git a/plugin/src/main/java/org/elasticsearch/xpack/watcher/support/search/WatcherSearchTemplateService.java b/plugin/src/main/java/org/elasticsearch/xpack/watcher/support/search/WatcherSearchTemplateService.java index b8fd2591ed2..42396141015 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/watcher/support/search/WatcherSearchTemplateService.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/watcher/support/search/WatcherSearchTemplateService.java @@ -14,6 +14,7 @@ import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptService; +import org.elasticsearch.script.ScriptType; import org.elasticsearch.script.TemplateScript; import org.elasticsearch.search.builder.SearchSourceBuilder; import org.elasticsearch.xpack.watcher.Watcher; @@ -48,7 +49,8 @@ public class WatcherSearchTemplateService extends AbstractComponent { watcherContextParams.putAll(source.getParams()); } // Templates are always of lang mustache: - Script template = new Script(source.getType(), "mustache", source.getIdOrCode(), source.getOptions(), watcherContextParams); + Script template = new Script(source.getType(), source.getType() == ScriptType.STORED ? null : "mustache", + source.getIdOrCode(), source.getOptions(), watcherContextParams); TemplateScript.Factory compiledTemplate = scriptService.compile(template, Watcher.SCRIPT_TEMPLATE_CONTEXT); return compiledTemplate.newInstance(template.getParams()).execute(); } diff --git a/plugin/src/test/java/org/elasticsearch/xpack/common/text/TextTemplateTests.java b/plugin/src/test/java/org/elasticsearch/xpack/common/text/TextTemplateTests.java index 17a08024331..d4e84c67370 100644 --- a/plugin/src/test/java/org/elasticsearch/xpack/common/text/TextTemplateTests.java +++ b/plugin/src/test/java/org/elasticsearch/xpack/common/text/TextTemplateTests.java @@ -62,7 +62,7 @@ public class TextTemplateTests extends ESTestCase { } }; - when(service.compile(new Script(type, lang, templateText, + when(service.compile(new Script(type, type == ScriptType.STORED ? null : lang, templateText, type == ScriptType.INLINE ? Collections.singletonMap("content_type", "text/plain") : null, merged), Watcher.SCRIPT_TEMPLATE_CONTEXT)).thenReturn(compiledTemplate); @@ -84,7 +84,7 @@ public class TextTemplateTests extends ESTestCase { } }; - when(service.compile(new Script(type, lang, templateText, + when(service.compile(new Script(type, type == ScriptType.STORED ? null : lang, templateText, type == ScriptType.INLINE ? Collections.singletonMap("content_type", "text/plain") : null, model), Watcher.SCRIPT_TEMPLATE_CONTEXT)).thenReturn(compiledTemplate); @@ -114,7 +114,8 @@ public class TextTemplateTests extends ESTestCase { public void testParser() throws Exception { ScriptType type = randomScriptType(); - TextTemplate template = templateBuilder(type, "_template", singletonMap("param_key", "param_val")); + TextTemplate template = + templateBuilder(type, "_template", singletonMap("param_key", "param_val")); XContentBuilder builder = jsonBuilder().startObject(); switch (type) { case INLINE: @@ -134,7 +135,9 @@ public class TextTemplateTests extends ESTestCase { } public void testParserParserSelfGenerated() throws Exception { - TextTemplate template = templateBuilder(randomScriptType(), "_template", singletonMap("param_key", "param_val")); + ScriptType type = randomScriptType(); + TextTemplate template = + templateBuilder(type, "_template", singletonMap("param_key", "param_val")); XContentBuilder builder = jsonBuilder().value(template); BytesReference bytes = builder.bytes(); diff --git a/plugin/src/test/java/org/elasticsearch/xpack/watcher/support/WatcherUtilsTests.java b/plugin/src/test/java/org/elasticsearch/xpack/watcher/support/WatcherUtilsTests.java index 63b86f57ce3..a9ae8359bec 100644 --- a/plugin/src/test/java/org/elasticsearch/xpack/watcher/support/WatcherUtilsTests.java +++ b/plugin/src/test/java/org/elasticsearch/xpack/watcher/support/WatcherUtilsTests.java @@ -96,6 +96,7 @@ public class WatcherUtilsTests extends ESTestCase { BytesReference expectedSource = null; Script expectedTemplate = null; WatcherSearchTemplateRequest request; + boolean stored = false; if (randomBoolean()) { Map params = new HashMap<>(); if (randomBoolean()) { @@ -106,7 +107,8 @@ public class WatcherUtilsTests extends ESTestCase { } String text = randomAlphaOfLengthBetween(1, 5); ScriptType scriptType = randomFrom(ScriptType.values()); - expectedTemplate = new Script(scriptType, "mustache", text, params); + stored = scriptType == ScriptType.STORED; + expectedTemplate = new Script(scriptType, stored ? null : "mustache", text, params); request = new WatcherSearchTemplateRequest(expectedIndices, expectedTypes, expectedSearchType, expectedIndicesOptions, expectedTemplate); } else { @@ -130,7 +132,7 @@ public class WatcherUtilsTests extends ESTestCase { assertThat(result.getSearchType(), equalTo(expectedSearchType)); assertNotNull(result.getTemplate()); - assertThat(result.getTemplate().getLang(), equalTo("mustache")); + assertThat(result.getTemplate().getLang(), equalTo(stored ? null : "mustache")); if (expectedSource == null) { assertThat(result.getTemplate().getIdOrCode(), equalTo(expectedTemplate.getIdOrCode())); assertThat(result.getTemplate().getType(), equalTo(expectedTemplate.getType())); @@ -194,6 +196,7 @@ public class WatcherUtilsTests extends ESTestCase { builder.rawField("body", source); } Script template = null; + boolean stored = false; if (randomBoolean()) { Map params = new HashMap<>(); if (randomBoolean()) { @@ -204,7 +207,8 @@ public class WatcherUtilsTests extends ESTestCase { } String text = randomAlphaOfLengthBetween(1, 5); ScriptType scriptType = randomFrom(ScriptType.values()); - template = new Script(scriptType, "mustache", text, params); + stored = scriptType == ScriptType.STORED; + template = new Script(scriptType, stored ? null : "mustache", text, params); builder.field("template", template); } builder.endObject(); @@ -228,7 +232,7 @@ public class WatcherUtilsTests extends ESTestCase { assertThat(result.getTemplate().getIdOrCode(), equalTo(template.getIdOrCode())); assertThat(result.getTemplate().getType(), equalTo(template.getType())); assertThat(result.getTemplate().getParams(), equalTo(template.getParams())); - assertThat(result.getTemplate().getLang(), equalTo("mustache")); + assertThat(result.getTemplate().getLang(), equalTo(stored ? null : "mustache")); } } diff --git a/plugin/src/test/java/org/elasticsearch/xpack/watcher/support/search/WatcherSearchTemplateRequestTests.java b/plugin/src/test/java/org/elasticsearch/xpack/watcher/support/search/WatcherSearchTemplateRequestTests.java index 6001734e368..b6111cb97e2 100644 --- a/plugin/src/test/java/org/elasticsearch/xpack/watcher/support/search/WatcherSearchTemplateRequestTests.java +++ b/plugin/src/test/java/org/elasticsearch/xpack/watcher/support/search/WatcherSearchTemplateRequestTests.java @@ -20,7 +20,7 @@ public class WatcherSearchTemplateRequestTests extends ESTestCase { public void testFromXContentWithTemplateDefaultLang() throws IOException { String source = "{\"template\":{\"id\":\"default-script\", \"params\":{\"foo\":\"bar\"}}}"; - assertTemplate(source, "default-script", "mustache", singletonMap("foo", "bar")); + assertTemplate(source, "default-script", null, singletonMap("foo", "bar")); } public void testFromXContentWithTemplateCustomLang() throws IOException { diff --git a/plugin/src/test/java/org/elasticsearch/xpack/watcher/test/integration/BasicWatcherTests.java b/plugin/src/test/java/org/elasticsearch/xpack/watcher/test/integration/BasicWatcherTests.java index 2a5b5010892..b9f13ff1c8e 100644 --- a/plugin/src/test/java/org/elasticsearch/xpack/watcher/test/integration/BasicWatcherTests.java +++ b/plugin/src/test/java/org/elasticsearch/xpack/watcher/test/integration/BasicWatcherTests.java @@ -235,7 +235,7 @@ public class BasicWatcherTests extends AbstractWatcherIntegrationTestCase { .setContent(jsonBuilder().startObject().field("template").value(searchSourceBuilder).endObject().bytes(), XContentType.JSON) .get()); - Script template = new Script(ScriptType.STORED, "mustache", "my-template", Collections.emptyMap()); + Script template = new Script(ScriptType.STORED, null, "my-template", Collections.emptyMap()); WatcherSearchTemplateRequest searchRequest = new WatcherSearchTemplateRequest(new String[]{"events"}, new String[0], SearchType.DEFAULT, WatcherSearchTemplateRequest.DEFAULT_INDICES_OPTIONS, template); testConditionSearch(searchRequest); diff --git a/plugin/src/test/java/org/elasticsearch/xpack/watcher/transform/TransformIntegrationTests.java b/plugin/src/test/java/org/elasticsearch/xpack/watcher/transform/TransformIntegrationTests.java index 1d4d9968c09..b1282264175 100644 --- a/plugin/src/test/java/org/elasticsearch/xpack/watcher/transform/TransformIntegrationTests.java +++ b/plugin/src/test/java/org/elasticsearch/xpack/watcher/transform/TransformIntegrationTests.java @@ -123,7 +123,7 @@ public class TransformIntegrationTests extends AbstractWatcherIntegrationTestCas .setLang("mockscript") .setContent(new BytesArray("{\"script\" : \"['key3' : ctx.payload.key1 + ctx.payload.key2]\"}"), XContentType.JSON) .get()); - script = new Script(ScriptType.STORED, "mockscript", "my-script", Collections.emptyMap()); + script = new Script(ScriptType.STORED, null, "my-script", Collections.emptyMap()); } // put a watch that has watch level transform: diff --git a/plugin/src/test/java/org/elasticsearch/xpack/watcher/transform/script/ScriptTransformTests.java b/plugin/src/test/java/org/elasticsearch/xpack/watcher/transform/script/ScriptTransformTests.java index 7229f7460f0..767f89aecdc 100644 --- a/plugin/src/test/java/org/elasticsearch/xpack/watcher/transform/script/ScriptTransformTests.java +++ b/plugin/src/test/java/org/elasticsearch/xpack/watcher/transform/script/ScriptTransformTests.java @@ -58,7 +58,7 @@ public class ScriptTransformTests extends ESTestCase { ScriptService service = mock(ScriptService.class); ScriptType type = randomFrom(ScriptType.values()); Map params = Collections.emptyMap(); - Script script = new Script(type, "_lang", "_script", params); + Script script = new Script(type, type == ScriptType.STORED ? null : "_lang", "_script", params); ExecutableScript.Factory factory = mock(ExecutableScript.Factory.class); when(service.compile(script, Watcher.SCRIPT_EXECUTABLE_CONTEXT)).thenReturn(factory); ExecutableScriptTransform transform = new ExecutableScriptTransform(new ScriptTransform(script), logger, service); @@ -86,7 +86,7 @@ public class ScriptTransformTests extends ESTestCase { ScriptService service = mock(ScriptService.class); ScriptType type = randomFrom(ScriptType.values()); Map params = Collections.emptyMap(); - Script script = new Script(type, "_lang", "_script", params); + Script script = new Script(type, type == ScriptType.STORED ? null : "_lang", "_script", params); ExecutableScript.Factory factory = mock(ExecutableScript.Factory.class); when(service.compile(script, Watcher.SCRIPT_EXECUTABLE_CONTEXT)).thenReturn(factory); ExecutableScriptTransform transform = new ExecutableScriptTransform(new ScriptTransform(script), logger, service); @@ -112,7 +112,7 @@ public class ScriptTransformTests extends ESTestCase { ScriptService service = mock(ScriptService.class); ScriptType type = randomFrom(ScriptType.values()); Map params = Collections.emptyMap(); - Script script = new Script(type, "_lang", "_script", params); + Script script = new Script(type, type == ScriptType.STORED ? null : "_lang", "_script", params); ExecutableScript.Factory factory = mock(ExecutableScript.Factory.class); when(service.compile(script, Watcher.SCRIPT_EXECUTABLE_CONTEXT)).thenReturn(factory); ExecutableScriptTransform transform = new ExecutableScriptTransform(new ScriptTransform(script), logger, service); @@ -186,10 +186,9 @@ public class ScriptTransformTests extends ESTestCase { public void testScriptConditionParserBadLang() throws Exception { ScriptTransformFactory transformFactory = new ScriptTransformFactory(Settings.builder().build(), createScriptService(tp)); - ScriptType scriptType = randomFrom(ScriptType.values()); String script = "return true"; XContentBuilder builder = jsonBuilder().startObject() - .field(scriptTypeField(scriptType), script) + .field(scriptTypeField(ScriptType.INLINE), script) .field("lang", "not_a_valid_lang") .startObject("params").field("key", "value").endObject() .endObject(); @@ -199,13 +198,7 @@ public class ScriptTransformTests extends ESTestCase { parser.nextToken(); ScriptTransform scriptCondition = transformFactory.parseTransform("_watch", parser); Exception e = expectThrows(IllegalArgumentException.class, () -> transformFactory.createExecutable(scriptCondition)); - if (scriptType == ScriptType.STORED) { - assertThat(e.getMessage(), containsString("unable to get stored script with unsupported lang [not_a_valid_lang]")); - assertWarnings("specifying the field [lang] for executing stored scripts is deprecated;" + - " use only the field [id] to specify an "); - } else { - assertThat(e.getMessage(), containsString("script_lang not supported [not_a_valid_lang]")); - } + assertThat(e.getMessage(), containsString("script_lang not supported [not_a_valid_lang]")); } static String scriptTypeField(ScriptType type) { diff --git a/qa/smoke-test-watcher-with-mustache/src/test/resources/rest-api-spec/test/watcher_mustache/30_search_input.yml b/qa/smoke-test-watcher-with-mustache/src/test/resources/rest-api-spec/test/watcher_mustache/30_search_input.yml index 24ceb3fcd3d..b39e05e4bc7 100644 --- a/qa/smoke-test-watcher-with-mustache/src/test/resources/rest-api-spec/test/watcher_mustache/30_search_input.yml +++ b/qa/smoke-test-watcher-with-mustache/src/test/resources/rest-api-spec/test/watcher_mustache/30_search_input.yml @@ -157,5 +157,4 @@ setup: # makes sure that the mustache template snippets have been resolved correctly: - match: { "watch_record.result.input.search.request.body.query.bool.must.0.term.value": "val_2" } - match: { "watch_record.result.input.search.request.template.id": "search-template" } - - match: { "watch_record.result.input.search.request.template.lang": "mustache" } - match: { "watch_record.result.input.search.request.template.params.num": 2 }