Watcher: Properly url encode room names (elastic/x-pack-elasticsearch#2896)

Room names in hipchat were not properly URL encoded, thus room names
with spaces would not work as expected. This fixes all the hipchat
accounts by properly using spaces.

Also the hipchat tests are reenabled, as the IT team gave me new access to hipchat, 
allowing to create a fresh set of oauth tokens for the integration account type.

The HipchatServiceTests have also been converted to XPackSingleNodeTestCase

relates elastic/x-pack-elasticsearch#2371
relates elastic/x-pack-elasticsearch#2429

Original commit: elastic/x-pack-elasticsearch@9f8872f686

plugin/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java

@@ -372,7 +372,7 @@ public class Watcher implements ActionPlugin {
         return Arrays.asList(registry, watcherClient, inputRegistry, historyStore, triggerService, triggeredWatchParser,
                 watcherLifeCycleService, executionService, triggerEngineListener, watcherService, watchParser,
                 configuredTriggerEngine, triggeredWatchStore, watcherSearchTemplateService, watcherIndexTemplateRegistry,
-                slackService, pagerDutyService);
+                slackService, pagerDutyService, hipChatService);
     }
 
     protected TriggerEngine getTriggerEngine(Clock clock, ScheduleRegistry scheduleRegistry) {

plugin/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/IntegrationAccount.java

@@ -95,10 +95,11 @@ public class IntegrationAccount extends HipChatAccount {
     }
 
     private HttpRequest buildRoomRequest(String room, final HipChatMessage message, HttpProxy proxy) {
+        String urlEncodedRoom = HttpRequest.encodeUrl(room);
         HttpRequest.Builder builder = server.httpRequest()
                 .method(HttpMethod.POST)
                 .scheme(Scheme.HTTPS)
-                .path("/v2/room/" + room + "/notification")
+                .path("/v2/room/" + urlEncodedRoom + "/notification")
                 .setHeader("Content-Type", "application/json")
                 .setHeader("Authorization", "Bearer " + authToken)
                 .body(Strings.toString((xbuilder, params) -> {

plugin/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/UserAccount.java

@@ -23,6 +23,8 @@ import org.elasticsearch.xpack.watcher.notification.hipchat.HipChatMessage.Forma
 import org.elasticsearch.xpack.watcher.actions.hipchat.HipChatAction;
 
 import java.io.IOException;
+import java.net.URI;
+import java.net.URISyntaxException;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
@@ -108,10 +110,11 @@ public class UserAccount extends HipChatAccount {
     }
 
     public HttpRequest buildRoomRequest(String room, final HipChatMessage message, HttpProxy proxy) {
+        String urlEncodedRoom = encodeRoom(room);
         HttpRequest.Builder builder = server.httpRequest()
                 .method(HttpMethod.POST)
                 .scheme(Scheme.HTTPS)
-                .path("/v2/room/" + room + "/notification")
+                .path("/v2/room/" + urlEncodedRoom + "/notification")
                 .setHeader("Content-Type", "application/json")
                 .setHeader("Authorization", "Bearer " + authToken)
                 .body(Strings.toString((xbuilder, params) -> {
@@ -133,6 +136,18 @@ public class UserAccount extends HipChatAccount {
         return builder.build();
     }
 
+    // this specific hipchat API does not accept application-form encoding, but requires real URL encoding
+    // spaces must not be replaced with a plus, but rather with %20
+    // this workaround ensures, that this happens
+    private String encodeRoom(String text) {
+        try {
+            return new URI("//", "", "", text, null).getRawQuery();
+        } catch (URISyntaxException e) {
+            throw new IllegalArgumentException("failed to URL encode text [" + text + "]", e);
+        }
+
+    }
+
     public HttpRequest buildUserRequest(String user, final HipChatMessage message, HttpProxy proxy) {
         HttpRequest.Builder builder = server.httpRequest()
                 .method(HttpMethod.POST)

plugin/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/V1Account.java

@@ -103,7 +103,7 @@ public class V1Account extends HipChatAccount {
             builder.proxy(proxy);
         }
         StringBuilder body = new StringBuilder();
-        body.append("room_id=").append(room);
+        body.append("room_id=").append(HttpRequest.encodeUrl(room));
         body.append("&from=").append(HttpRequest.encodeUrl(message.from));
         body.append("&message=").append(HttpRequest.encodeUrl(message.body));
         if (message.format != null) {

plugin/src/test/java/org/elasticsearch/xpack/watcher/notification/hipchat/IntegrationAccountTests.java

@@ -24,6 +24,7 @@ import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 public class IntegrationAccountTests extends ESTestCase {
+
     public void testSettings() throws Exception {
         String accountName = "_name";
 
@@ -113,11 +114,12 @@ public class IntegrationAccountTests extends ESTestCase {
 
     public void testSend() throws Exception {
         HttpClient httpClient = mock(HttpClient.class);
+        String room = "Room with Spaces";
         IntegrationAccount account = new IntegrationAccount("_name", Settings.builder()
                 .put("host", "_host")
                 .put("port", "443")
                 .put("auth_token", "_token")
-                .put("room", "_room")
+                .put("room", room)
                 .build(), HipChatServer.DEFAULT, httpClient, mock(Logger.class));
 
         HipChatMessage.Format format = randomFrom(HipChatMessage.Format.values());
@@ -128,7 +130,8 @@ public class IntegrationAccountTests extends ESTestCase {
         HttpRequest req = HttpRequest.builder("_host", 443)
                 .method(HttpMethod.POST)
                 .scheme(Scheme.HTTPS)
-                .path("/v2/room/_room/notification")
+                // url encoded already
+                .path("/v2/room/Room+with+Spaces/notification")
                 .setHeader("Content-Type", "application/json")
                 .setHeader("Authorization", "Bearer _token")
                 .body(Strings.toString((builder, params) -> {

plugin/src/test/java/org/elasticsearch/xpack/watcher/notification/hipchat/UserAccountTests.java

@@ -12,18 +12,23 @@ import org.elasticsearch.common.settings.SettingsException;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.xpack.watcher.common.http.HttpClient;
 import org.elasticsearch.xpack.watcher.common.http.HttpMethod;
+import org.elasticsearch.xpack.watcher.common.http.HttpProxy;
 import org.elasticsearch.xpack.watcher.common.http.HttpRequest;
 import org.elasticsearch.xpack.watcher.common.http.HttpResponse;
 import org.elasticsearch.xpack.watcher.common.http.Scheme;
 import org.elasticsearch.xpack.watcher.common.text.TextTemplate;
 import org.elasticsearch.xpack.watcher.test.MockTextTemplateEngine;
+import org.mockito.ArgumentCaptor;
 
 import java.util.HashMap;
 
 import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
 import static org.hamcrest.Matchers.arrayContaining;
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.nullValue;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
@@ -265,6 +270,29 @@ public class UserAccountTests extends ESTestCase {
         assertThat(message.format, is(nullValue()));
     }
 
+    public void testRoomNameIsUrlEncoded() throws Exception {
+        Settings settings = Settings.builder()
+                .put("user", "testuser")
+                .put("auth_token", "awesome-auth-token")
+                .build();
+        HipChatServer hipChatServer = mock(HipChatServer.class);
+        HttpClient httpClient = mock(HttpClient.class);
+        UserAccount account = new UserAccount("notify-monitoring", settings, hipChatServer, httpClient, logger);
+
+        TextTemplate[] rooms = new TextTemplate[] { new TextTemplate("Room with Spaces")};
+        HipChatMessage.Template template =
+                new HipChatMessage.Template(new TextTemplate("body"), rooms, null, "sender", HipChatMessage.Format.TEXT, null, true);
+
+        HipChatMessage message = account.render("watchId", "actionId", new MockTextTemplateEngine(), template, new HashMap<>());
+        account.send(message, HttpProxy.NO_PROXY);
+
+        ArgumentCaptor<HttpRequest> captor = ArgumentCaptor.forClass(HttpRequest.class);
+        verify(httpClient).execute(captor.capture());
+        assertThat(captor.getAllValues(), hasSize(1));
+        assertThat(captor.getValue().path(), not(containsString("Room with Spaces")));
+        assertThat(captor.getValue().path(), containsString("Room%20with%20Spaces"));
+    }
+
     private UserAccount createUserAccount(Settings settings) {
         HipChatServer hipChatServer = mock(HipChatServer.class);
         HttpClient httpClient = mock(HttpClient.class);

plugin/src/test/java/org/elasticsearch/xpack/watcher/notification/hipchat/V1AccountTests.java

@@ -110,7 +110,8 @@ public class V1AccountTests extends ESTestCase {
         HipChatMessage.Format format = randomFrom(HipChatMessage.Format.values());
         HipChatMessage.Color color = randomFrom(HipChatMessage.Color.values());
         Boolean notify = randomBoolean();
-        HipChatMessage message = new HipChatMessage("_body", new String[] { "_r1", "_r2" }, null, "_from", format, color, notify);
+        HipChatMessage message = new HipChatMessage("_body", new String[] { "Room with Spaces", "_r2" }, null, "_from", format, 
+                                                    color, notify);
 
         HttpRequest req1 = HttpRequest.builder("_host", 443)
                 .method(HttpMethod.POST)
@@ -120,7 +121,7 @@ public class V1AccountTests extends ESTestCase {
                 .setParam("format", "json")
                 .setParam("auth_token", "_token")
                 .body(new StringBuilder()
-                        .append("room_id=").append("_r1&")
+                        .append("room_id=").append("Room+with+Spaces&")
                         .append("from=").append("_from&")
                         .append("message=").append("_body&")
                         .append("message_format=").append(format.value()).append("&")

plugin/src/test/java/org/elasticsearch/xpack/watcher/test/integration/HipChatServiceTests.java

@@ -5,21 +5,29 @@
  */
 package org.elasticsearch.xpack.watcher.test.integration;
 
-import org.apache.lucene.util.LuceneTestCase;
 import org.elasticsearch.ExceptionsHelper;
 import org.elasticsearch.action.search.SearchResponse;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.plugins.Plugin;
+import org.elasticsearch.script.MockMustacheScriptEngine;
 import org.elasticsearch.test.junit.annotations.Network;
 import org.elasticsearch.test.junit.annotations.TestLogging;
+import org.elasticsearch.xpack.XPackPlugin;
+import org.elasticsearch.xpack.XPackSettings;
+import org.elasticsearch.xpack.XPackSingleNodeTestCase;
+import org.elasticsearch.xpack.watcher.actions.hipchat.HipChatAction;
+import org.elasticsearch.xpack.watcher.client.WatcherClient;
+import org.elasticsearch.xpack.watcher.condition.AlwaysCondition;
+import org.elasticsearch.xpack.watcher.history.HistoryStore;
 import org.elasticsearch.xpack.watcher.notification.hipchat.HipChatAccount;
 import org.elasticsearch.xpack.watcher.notification.hipchat.HipChatMessage;
 import org.elasticsearch.xpack.watcher.notification.hipchat.HipChatService;
 import org.elasticsearch.xpack.watcher.notification.hipchat.SentMessages;
-import org.elasticsearch.xpack.watcher.actions.hipchat.HipChatAction;
-import org.elasticsearch.xpack.watcher.condition.AlwaysCondition;
-import org.elasticsearch.xpack.watcher.test.AbstractWatcherIntegrationTestCase;
 import org.elasticsearch.xpack.watcher.transport.actions.put.PutWatchResponse;
 
+import java.util.Arrays;
+import java.util.Collection;
+
 import static org.elasticsearch.index.query.QueryBuilders.boolQuery;
 import static org.elasticsearch.index.query.QueryBuilders.termQuery;
 import static org.elasticsearch.search.builder.SearchSourceBuilder.searchSource;
@@ -34,21 +42,20 @@ import static org.hamcrest.Matchers.notNullValue;
 
 @Network
 @TestLogging("org.elasticsearch.xpack.watcher.common.http:TRACE")
-public class HipChatServiceTests extends AbstractWatcherIntegrationTestCase {
+public class HipChatServiceTests extends XPackSingleNodeTestCase {
+
     @Override
-    protected boolean timeWarped() {
+    protected Collection<Class<? extends Plugin>> getPlugins() {
-        return true;
+        return Arrays.asList(XPackPlugin.class, MockMustacheScriptEngine.TestPlugin.class);
     }
 
     @Override
-    protected boolean enableSecurity() {
+    protected Settings nodeSettings() {
-        return false;
-    }
-
-    @Override
-    protected Settings nodeSettings(int nodeOrdinal) {
         return Settings.builder()
-                .put(super.nodeSettings(nodeOrdinal))
+                .put(super.nodeSettings())
+                .put(XPackSettings.WATCHER_ENABLED.getKey(), true)
+                .put(XPackSettings.SECURITY_ENABLED.getKey(), false)
+                .put(XPackSettings.MONITORING_ENABLED.getKey(), false)
 
                 // this is for the `test-watcher-integration` group level integration in HipChat
                 .put("xpack.notification.hipchat.account.integration_account.profile", "integration")
@@ -58,7 +65,7 @@ public class HipChatServiceTests extends AbstractWatcherIntegrationTestCase {
 
                 // this is for the Watcher Test account in HipChat
                 .put("xpack.notification.hipchat.account.user_account.profile", "user")
-                .put("xpack.notification.hipchat.account.user_account.auth_token", "12rNQUuQ0wObfRVeoVD8OeoAnosCT8tSTV5UjsII")
+                .put("xpack.notification.hipchat.account.user_account.auth_token", "4UefsFLvKRw01EMN5vo3oyoY6BLiz7IQBQbGug8K")
 
                 // this is for the `test-watcher-v1` notification token
                 .put("xpack.notification.hipchat.account.v1_account.profile", "v1")
@@ -66,12 +73,11 @@ public class HipChatServiceTests extends AbstractWatcherIntegrationTestCase {
                 .build();
     }
 
-    @LuceneTestCase.AwaitsFix(bugUrl = "https://github.com/elastic/infra/issues/2726")
     public void testSendMessageV1Account() throws Exception {
-        HipChatService service = getInstanceFromMaster(HipChatService.class);
+        HipChatService service = getInstanceFromNode(HipChatService.class);
         HipChatMessage hipChatMessage = new HipChatMessage(
                 "HipChatServiceTests#testSendMessage_V1Account",
-                new String[] { "test-watcher", "test-watcher-2" },
+                new String[] { "test-watcher", "test-watcher-2", "test watcher with spaces" },
                 null, // users are unsupported in v1
                 "watcher-tests",
                 HipChatMessage.Format.TEXT,
@@ -81,12 +87,11 @@ public class HipChatServiceTests extends AbstractWatcherIntegrationTestCase {
         HipChatAccount account = service.getAccount("v1_account");
         assertThat(account, notNullValue());
         SentMessages messages = account.send(hipChatMessage, null);
-        assertSentMessagesAreValid(2, messages);
+        assertSentMessagesAreValid(3, messages);
     }
 
-    @LuceneTestCase.AwaitsFix(bugUrl = "https://github.com/elastic/infra/issues/2726")
     public void testSendMessageIntegrationAccount() throws Exception {
-        HipChatService service = getInstanceFromMaster(HipChatService.class);
+        HipChatService service = getInstanceFromNode(HipChatService.class);
         HipChatMessage.Color color = randomFrom(HipChatMessage.Color.values());
         HipChatMessage hipChatMessage = new HipChatMessage(
                 "HipChatServiceTests#testSendMessage_IntegrationAccount colored " + color.value(),
@@ -103,15 +108,14 @@ public class HipChatServiceTests extends AbstractWatcherIntegrationTestCase {
         assertSentMessagesAreValid(1, messages);
     }
 
-    @LuceneTestCase.AwaitsFix(bugUrl = "https://github.com/elastic/infra/issues/2726")
     public void testSendMessageUserAccount() throws Exception {
-        HipChatService service = getInstanceFromMaster(HipChatService.class);
+        HipChatService service = getInstanceFromNode(HipChatService.class);
         HipChatMessage.Color color = randomFrom(HipChatMessage.Color.values());
         HipChatMessage hipChatMessage = new HipChatMessage(
                 "HipChatServiceTests#testSendMessage_UserAccount colored " + color.value(),
-                new String[] { "test-watcher", "test-watcher-2" },
+                new String[] { "test-watcher", "test-watcher-2", "test watcher with spaces" },
                 new String[] { "watcher@elastic.co" },
-                null, // custom "from" is not supported by integration profiles
+                null,
                 HipChatMessage.Format.TEXT,
                 color,
                 false);
@@ -119,10 +123,9 @@ public class HipChatServiceTests extends AbstractWatcherIntegrationTestCase {
         HipChatAccount account = service.getAccount("user_account");
         assertThat(account, notNullValue());
         SentMessages messages = account.send(hipChatMessage, null);
-        assertSentMessagesAreValid(3, messages);
+        assertSentMessagesAreValid(4, messages);
     }
 
-    @LuceneTestCase.AwaitsFix(bugUrl = "https://github.com/elastic/infra/issues/2726")
     public void testWatchWithHipChatAction() throws Exception {
         HipChatAccount.Profile profile = randomFrom(HipChatAccount.Profile.values());
         HipChatMessage.Color color = randomFrom(HipChatMessage.Color.values());
@@ -132,7 +135,7 @@ public class HipChatServiceTests extends AbstractWatcherIntegrationTestCase {
             case USER:
                 account = "user_account";
                 actionBuilder = hipchatAction(account, "_message")
-                        .addRooms("test-watcher", "test-watcher-2")
+                        .addRooms("test-watcher", "test-watcher-2", "test watcher with spaces")
                         .addUsers("watcher@elastic.co")
                         .setFormat(HipChatMessage.Format.TEXT)
                         .setColor(color)
@@ -151,7 +154,7 @@ public class HipChatServiceTests extends AbstractWatcherIntegrationTestCase {
                 assertThat(profile, is(HipChatAccount.Profile.V1));
                 account = "v1_account";
                 actionBuilder = hipchatAction(account, "_message")
-                        .addRooms("test-watcher", "test-watcher-2")
+                        .addRooms("test-watcher", "test-watcher-2", "test watcher with spaces")
                         .setFrom("watcher-test")
                         .setFormat(HipChatMessage.Format.TEXT)
                         .setColor(color)
@@ -159,7 +162,8 @@ public class HipChatServiceTests extends AbstractWatcherIntegrationTestCase {
         }
 
         String id = randomAlphaOfLength(10);
-        PutWatchResponse putWatchResponse = watcherClient().preparePutWatch(id).setSource(watchBuilder()
+        WatcherClient watcherClient = new WatcherClient(client());
+        PutWatchResponse putWatchResponse = watcherClient.preparePutWatch(id).setSource(watchBuilder()
                 .trigger(schedule(interval("10m")))
                 .input(simpleInput("ref", "HipChatServiceTests#testWatchWithHipChatAction"))
                 .condition(AlwaysCondition.INSTANCE)
@@ -168,29 +172,27 @@ public class HipChatServiceTests extends AbstractWatcherIntegrationTestCase {
 
         assertThat(putWatchResponse.isCreated(), is(true));
 
-        timeWarp().trigger(id);
+        watcherClient.prepareExecuteWatch(id).setRecordExecution(true).execute().actionGet();
-        flush();
-        refresh();
 
-        assertWatchWithMinimumPerformedActionsCount(id, 1L, false);
+        client().admin().indices().prepareRefresh(HistoryStore.INDEX_PREFIX_WITH_TEMPLATE + "*").execute().actionGet();
-
+        SearchResponse response = client().prepareSearch(HistoryStore.INDEX_PREFIX_WITH_TEMPLATE + "*")
-        SearchResponse response = searchHistory(searchSource().query(boolQuery()
+                .setSource(searchSource().query(boolQuery()
                 .must(termQuery("result.actions.id", "hipchat"))
                 .must(termQuery("result.actions.type", "hipchat"))
                 .must(termQuery("result.actions.status", "success"))
                 .must(termQuery("result.actions.hipchat.account", account))
-                .must(termQuery("result.actions.hipchat.sent_messages.status", "success"))));
+                .must(termQuery("result.actions.hipchat.sent_messages.status", "success"))))
+                .get();
 
         assertThat(response, notNullValue());
         assertThat(response.getHits().getTotalHits(), is(1L));
     }
 
-    @LuceneTestCase.AwaitsFix(bugUrl = "https://github.com/elastic/infra/issues/2726")
     public void testDefaultValuesForColorAndFormatWorks() {
-        HipChatService service = getInstanceFromMaster(HipChatService.class);
+        HipChatService service = getInstanceFromNode(HipChatService.class);
         HipChatMessage hipChatMessage = new HipChatMessage(
                 "HipChatServiceTests#testSendMessage_UserAccount with default Color and text",
-                new String[] { "test-watcher" },
+                new String[] { "test-watcher", "test-watcher-2", "test watcher with spaces" },
                 new String[] { "watcher@elastic.co" },
                 null, // custom "from" is not supported by integration profiles
                 null,
@@ -200,7 +202,7 @@ public class HipChatServiceTests extends AbstractWatcherIntegrationTestCase {
         HipChatAccount account = service.getAccount("user_account");
         assertThat(account, notNullValue());
         SentMessages messages = account.send(hipChatMessage, null);
-        assertSentMessagesAreValid(2, messages);
+        assertSentMessagesAreValid(4, messages);
     }
 
     private void assertSentMessagesAreValid(int expectedMessageSize, SentMessages messages) {