From b24634131aab9910fbc265b2812652e70a1e5e3f Mon Sep 17 00:00:00 2001 From: Daniel Widdis Date: Tue, 2 Apr 2024 11:23:47 -0700 Subject: [PATCH] Silently ignore content on APIs that don't require it (#639) Signed-off-by: Daniel Widdis --- CHANGELOG.md | 3 +++ .../rest/RestDeleteWorkflowAction.java | 10 +++++----- .../rest/RestDeprovisionWorkflowAction.java | 9 +++++---- .../rest/RestGetWorkflowAction.java | 10 +++++----- .../rest/RestGetWorkflowStateAction.java | 9 ++++----- .../rest/RestGetWorkflowStepAction.java | 4 ++++ .../rest/RestDeleteWorkflowActionTests.java | 15 --------------- .../RestDeprovisionWorkflowActionTests.java | 18 ------------------ .../rest/RestGetWorkflowActionTests.java | 15 --------------- .../rest/RestGetWorkflowStateActionTests.java | 18 ------------------ .../rest/RestGetWorkflowStepActionTests.java | 15 --------------- 11 files changed, 26 insertions(+), 100 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a98926aaf..2afbf4018 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,9 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) ### Features ### Enhancements ### Bug Fixes +### Bug Fixes +- Silently ignore content on APIs that don't require it ([#639](https://github.com/opensearch-project/flow-framework/pull/639)) + ### Infrastructure ### Documentation ### Maintenance diff --git a/src/main/java/org/opensearch/flowframework/rest/RestDeleteWorkflowAction.java b/src/main/java/org/opensearch/flowframework/rest/RestDeleteWorkflowAction.java index fc18f103d..a7c8711dd 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestDeleteWorkflowAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestDeleteWorkflowAction.java @@ -69,11 +69,11 @@ protected BaseRestHandler.RestChannelConsumer prepareRequest(RestRequest request RestStatus.FORBIDDEN ); } - // Validate content - if (request.hasContent()) { - // BaseRestHandler will give appropriate error message - return channel -> channel.sendResponse(null); - } + + // Always consume content to silently ignore it + // https://github.com/opensearch-project/flow-framework/issues/578 + request.content(); + // Validate params if (workflowId == null) { throw new FlowFrameworkException("workflow_id cannot be null", RestStatus.BAD_REQUEST); diff --git a/src/main/java/org/opensearch/flowframework/rest/RestDeprovisionWorkflowAction.java b/src/main/java/org/opensearch/flowframework/rest/RestDeprovisionWorkflowAction.java index 08c8d6722..0d0bf8b64 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestDeprovisionWorkflowAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestDeprovisionWorkflowAction.java @@ -64,10 +64,11 @@ protected BaseRestHandler.RestChannelConsumer prepareRequest(RestRequest request RestStatus.FORBIDDEN ); } - // Validate content - if (request.hasContent()) { - throw new FlowFrameworkException("deprovision request should have no payload", RestStatus.BAD_REQUEST); - } + + // Always consume content to silently ignore it + // https://github.com/opensearch-project/flow-framework/issues/578 + request.content(); + // Validate params if (workflowId == null) { throw new FlowFrameworkException("workflow_id cannot be null", RestStatus.BAD_REQUEST); diff --git a/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java b/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java index 0710657c2..81141a380 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java @@ -69,11 +69,11 @@ protected BaseRestHandler.RestChannelConsumer prepareRequest(RestRequest request RestStatus.FORBIDDEN ); } - // Validate content - if (request.hasContent()) { - // BaseRestHandler will give appropriate error message - return channel -> channel.sendResponse(null); - } + + // Always consume content to silently ignore it + // https://github.com/opensearch-project/flow-framework/issues/578 + request.content(); + // Validate params if (workflowId == null) { throw new FlowFrameworkException("workflow_id cannot be null", RestStatus.BAD_REQUEST); diff --git a/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowStateAction.java b/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowStateAction.java index 45fe9921a..b95c75a56 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowStateAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowStateAction.java @@ -66,11 +66,10 @@ protected BaseRestHandler.RestChannelConsumer prepareRequest(RestRequest request ); } - // Validate content - if (request.hasContent()) { - // BaseRestHandler will give appropriate error message - return channel -> channel.sendResponse(null); - } + // Always consume content to silently ignore it + // https://github.com/opensearch-project/flow-framework/issues/578 + request.content(); + // Validate params if (workflowId == null) { throw new FlowFrameworkException("workflow_id cannot be null", RestStatus.BAD_REQUEST); diff --git a/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowStepAction.java b/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowStepAction.java index b9a6deb38..b889cede7 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowStepAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowStepAction.java @@ -71,6 +71,10 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli ); } + // Always consume content to silently ignore it + // https://github.com/opensearch-project/flow-framework/issues/578 + request.content(); + Map params = request.hasParam(WORKFLOW_STEP) ? Map.of(WORKFLOW_STEP, request.param(WORKFLOW_STEP)) : Collections.emptyMap(); diff --git a/src/test/java/org/opensearch/flowframework/rest/RestDeleteWorkflowActionTests.java b/src/test/java/org/opensearch/flowframework/rest/RestDeleteWorkflowActionTests.java index e52a2fb2a..8d787fda2 100644 --- a/src/test/java/org/opensearch/flowframework/rest/RestDeleteWorkflowActionTests.java +++ b/src/test/java/org/opensearch/flowframework/rest/RestDeleteWorkflowActionTests.java @@ -9,9 +9,7 @@ package org.opensearch.flowframework.rest; import org.opensearch.client.node.NodeClient; -import org.opensearch.core.common.bytes.BytesArray; import org.opensearch.core.rest.RestStatus; -import org.opensearch.core.xcontent.MediaTypeRegistry; import org.opensearch.flowframework.common.FlowFrameworkSettings; import org.opensearch.rest.RestHandler; import org.opensearch.rest.RestRequest; @@ -55,19 +53,6 @@ public void testRestDeleteWorkflowActionRoutes() { assertEquals(this.getPath, routes.get(0).getPath()); } - public void testInvalidRequestWithContent() { - RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withMethod(RestRequest.Method.DELETE) - .withPath(this.getPath) - .withContent(new BytesArray("request body"), MediaTypeRegistry.JSON) - .build(); - - FakeRestChannel channel = new FakeRestChannel(request, false, 1); - IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> { - restDeleteWorkflowAction.handleRequest(request, channel, nodeClient); - }); - assertEquals("request [DELETE /_plugins/_flow_framework/workflow/{workflow_id}] does not support having a body", ex.getMessage()); - } - public void testNullWorkflowId() throws Exception { // Request with no params diff --git a/src/test/java/org/opensearch/flowframework/rest/RestDeprovisionWorkflowActionTests.java b/src/test/java/org/opensearch/flowframework/rest/RestDeprovisionWorkflowActionTests.java index cdf25a474..e9a2c5a47 100644 --- a/src/test/java/org/opensearch/flowframework/rest/RestDeprovisionWorkflowActionTests.java +++ b/src/test/java/org/opensearch/flowframework/rest/RestDeprovisionWorkflowActionTests.java @@ -9,9 +9,7 @@ package org.opensearch.flowframework.rest; import org.opensearch.client.node.NodeClient; -import org.opensearch.core.common.bytes.BytesArray; import org.opensearch.core.rest.RestStatus; -import org.opensearch.core.xcontent.MediaTypeRegistry; import org.opensearch.flowframework.common.FlowFrameworkSettings; import org.opensearch.rest.RestHandler.Route; import org.opensearch.rest.RestRequest; @@ -71,22 +69,6 @@ public void testNullWorkflowId() throws Exception { assertTrue(channel.capturedResponse().content().utf8ToString().contains("workflow_id cannot be null")); } - public void testInvalidRequestWithContent() { - RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withMethod(RestRequest.Method.POST) - .withPath(this.deprovisionWorkflowPath) - .withContent(new BytesArray("request body"), MediaTypeRegistry.JSON) - .build(); - - FakeRestChannel channel = new FakeRestChannel(request, false, 1); - IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> { - deprovisionWorkflowRestAction.handleRequest(request, channel, nodeClient); - }); - assertEquals( - "request [POST /_plugins/_flow_framework/workflow/{workflow_id}/_deprovision] does not support having a body", - ex.getMessage() - ); - } - public void testFeatureFlagNotEnabled() throws Exception { when(flowFrameworkFeatureEnabledSetting.isFlowFrameworkEnabled()).thenReturn(false); RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withMethod(RestRequest.Method.POST) diff --git a/src/test/java/org/opensearch/flowframework/rest/RestGetWorkflowActionTests.java b/src/test/java/org/opensearch/flowframework/rest/RestGetWorkflowActionTests.java index a34eab679..02e5c9879 100644 --- a/src/test/java/org/opensearch/flowframework/rest/RestGetWorkflowActionTests.java +++ b/src/test/java/org/opensearch/flowframework/rest/RestGetWorkflowActionTests.java @@ -9,9 +9,7 @@ package org.opensearch.flowframework.rest; import org.opensearch.client.node.NodeClient; -import org.opensearch.core.common.bytes.BytesArray; import org.opensearch.core.rest.RestStatus; -import org.opensearch.core.xcontent.MediaTypeRegistry; import org.opensearch.flowframework.common.FlowFrameworkSettings; import org.opensearch.rest.RestHandler; import org.opensearch.rest.RestRequest; @@ -55,19 +53,6 @@ public void testRestGetWorkflowActionRoutes() { assertEquals(this.getPath, routes.get(0).getPath()); } - public void testInvalidRequestWithContent() { - RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withMethod(RestRequest.Method.GET) - .withPath(this.getPath) - .withContent(new BytesArray("request body"), MediaTypeRegistry.JSON) - .build(); - - FakeRestChannel channel = new FakeRestChannel(request, false, 1); - IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> { - restGetWorkflowAction.handleRequest(request, channel, nodeClient); - }); - assertEquals("request [GET /_plugins/_flow_framework/workflow/{workflow_id}] does not support having a body", ex.getMessage()); - } - public void testNullWorkflowId() throws Exception { // Request with no params diff --git a/src/test/java/org/opensearch/flowframework/rest/RestGetWorkflowStateActionTests.java b/src/test/java/org/opensearch/flowframework/rest/RestGetWorkflowStateActionTests.java index 649309440..d82165898 100644 --- a/src/test/java/org/opensearch/flowframework/rest/RestGetWorkflowStateActionTests.java +++ b/src/test/java/org/opensearch/flowframework/rest/RestGetWorkflowStateActionTests.java @@ -9,9 +9,7 @@ package org.opensearch.flowframework.rest; import org.opensearch.client.node.NodeClient; -import org.opensearch.core.common.bytes.BytesArray; import org.opensearch.core.rest.RestStatus; -import org.opensearch.core.xcontent.MediaTypeRegistry; import org.opensearch.flowframework.common.FlowFrameworkSettings; import org.opensearch.rest.RestHandler; import org.opensearch.rest.RestRequest; @@ -75,22 +73,6 @@ public void testNullWorkflowId() throws Exception { assertTrue(channel.capturedResponse().content().utf8ToString().contains("workflow_id cannot be null")); } - public void testInvalidRequestWithContent() { - RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withMethod(RestRequest.Method.GET) - .withPath(this.getPath) - .withContent(new BytesArray("request body"), MediaTypeRegistry.JSON) - .build(); - - FakeRestChannel channel = new FakeRestChannel(request, false, 1); - IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> { - restGetWorkflowStateAction.handleRequest(request, channel, nodeClient); - }); - assertEquals( - "request [GET /_plugins/_flow_framework/workflow/{workflow_id}/_status] does not support having a body", - ex.getMessage() - ); - } - public void testFeatureFlagNotEnabled() throws Exception { when(flowFrameworkFeatureEnabledSetting.isFlowFrameworkEnabled()).thenReturn(false); RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withMethod(RestRequest.Method.GET) diff --git a/src/test/java/org/opensearch/flowframework/rest/RestGetWorkflowStepActionTests.java b/src/test/java/org/opensearch/flowframework/rest/RestGetWorkflowStepActionTests.java index c7ebbf71e..867ab4eef 100644 --- a/src/test/java/org/opensearch/flowframework/rest/RestGetWorkflowStepActionTests.java +++ b/src/test/java/org/opensearch/flowframework/rest/RestGetWorkflowStepActionTests.java @@ -11,9 +11,7 @@ import org.opensearch.client.Client; import org.opensearch.client.node.NodeClient; import org.opensearch.core.action.ActionListener; -import org.opensearch.core.common.bytes.BytesArray; import org.opensearch.core.rest.RestStatus; -import org.opensearch.core.xcontent.MediaTypeRegistry; import org.opensearch.flowframework.common.FlowFrameworkSettings; import org.opensearch.flowframework.exception.FlowFrameworkException; import org.opensearch.flowframework.indices.FlowFrameworkIndicesHandler; @@ -85,19 +83,6 @@ public void testRestGetWorkflowStepActionRoutes() { assertEquals(this.getPath, routes.get(0).getPath()); } - public void testInvalidRequestWithContent() { - RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withMethod(RestRequest.Method.GET) - .withPath(this.getPath) - .withContent(new BytesArray("request body"), MediaTypeRegistry.JSON) - .build(); - - FakeRestChannel channel = new FakeRestChannel(request, false, 1); - IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> { - restGetWorkflowStepAction.handleRequest(request, channel, nodeClient); - }); - assertEquals("request [GET /_plugins/_flow_framework/workflow/_steps] does not support having a body", ex.getMessage()); - } - public void testWorkflowSteps() throws Exception { RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withMethod(RestRequest.Method.GET) .withPath(this.getPath + "?workflow_step=create_connector")