From 5fe73968ff5998e38e42d1b56f70778cbefb1704 Mon Sep 17 00:00:00 2001 From: Stephen Crawford Date: Wed, 15 Nov 2023 16:06:49 -0500 Subject: [PATCH 1/2] Add Security response tests Signed-off-by: Stephen Crawford --- .../security/filter/SecurityResponse.java | 2 + .../filter/SecurityResponseTests.java | 116 ++++++++++++++++++ 2 files changed, 118 insertions(+) diff --git a/src/main/java/org/opensearch/security/filter/SecurityResponse.java b/src/main/java/org/opensearch/security/filter/SecurityResponse.java index 5041936d2e..87e34f2342 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityResponse.java +++ b/src/main/java/org/opensearch/security/filter/SecurityResponse.java @@ -77,8 +77,10 @@ public void addHeader(String name, String value) { List header = headers.get(name); if (header == null) { header = new ArrayList<>(); + System.out.println("Adding attribute header of " + value); headers.put(name, header); } + System.out.println("Adding header of " + value); header.add(value); } diff --git a/src/test/java/org/opensearch/security/filter/SecurityResponseTests.java b/src/test/java/org/opensearch/security/filter/SecurityResponseTests.java index 228c91fb60..deb47cd8d5 100644 --- a/src/test/java/org/opensearch/security/filter/SecurityResponseTests.java +++ b/src/test/java/org/opensearch/security/filter/SecurityResponseTests.java @@ -11,7 +11,11 @@ package org.opensearch.security.filter; +import java.util.List; +import java.util.Map; +import org.apache.hc.core5.http.HttpHeaders; import org.apache.http.HttpStatus; +import org.apache.http.protocol.HTTP; import org.junit.Test; import org.opensearch.common.xcontent.XContentType; @@ -23,12 +27,124 @@ public class SecurityResponseTests { + + /** + * This test should check whether a basic constructor with the JSON content type is successfully converted to RestResponse + */ @Test public void testSecurityResponseHasSingleContentType() { final SecurityResponse response = new SecurityResponse(HttpStatus.SC_OK, null, "foo bar", XContentType.JSON.mediaType()); + final RestResponse restResponse = response.asRestResponse(); + assertThat(restResponse.status(), equalTo(RestStatus.OK)); + assertThat(restResponse.contentType(), equalTo(XContentType.JSON.mediaType())); + } + + /** + * This test should check whether adding a new HTTP Header for the content type takes the argument or the added header (should take arg.) + */ + @Test + public void testSecurityResponseMultipleContentTypesUsesPassed() { + final SecurityResponse response = new SecurityResponse(HttpStatus.SC_OK, null, "foo bar", XContentType.JSON.mediaType()); + response.addHeader(HttpHeaders.CONTENT_TYPE, "plain/text"); + assertThat(response.getHeaders().get("Content-Type"), equalTo(List.of("plain/text"))); + final RestResponse restResponse = response.asRestResponse(); + assertThat(restResponse.contentType(), equalTo(XContentType.JSON.mediaType())); + assertThat(restResponse.status(), equalTo(RestStatus.OK)); + } + + /** + * This test should check whether specifying no content type correctly uses JSON + */ + @Test + public void testSecurityResponseDefaultContentTypeIsJson() { + final SecurityResponse response = new SecurityResponse(HttpStatus.SC_OK, null, "foo bar"); + final RestResponse restResponse = response.asRestResponse(); + assertThat(restResponse.contentType(), equalTo(XContentType.JSON.mediaType())); + assertThat(restResponse.status(), equalTo(RestStatus.OK)); // This fails because it is a text/plain but we should pick one default type and stick to it IMO + } + + /** + * This test checks whether adding a new ContentType header actually changes the converted content type header (it should not) + */ + @Test + public void testSecurityResponseSetHeaderContentTypeDoesNothing() { + final SecurityResponse response = new SecurityResponse(HttpStatus.SC_OK, null, "foo bar"); + response.addHeader(HttpHeaders.CONTENT_TYPE, XContentType.JSON.mediaType()); + final RestResponse restResponse = response.asRestResponse(); + assertThat(restResponse.contentType(), equalTo("text/plain; charset=UTF-8")); + assertThat(restResponse.status(), equalTo(RestStatus.OK)); + } + + /** + * This test should check whether adding a multiple new HTTP Headers for the content type takes the argument or the added header (should take arg.) + */ + @Test + public void testSecurityResponseAddMultipleContentTypeHeaders() { + final SecurityResponse response = new SecurityResponse(HttpStatus.SC_OK, null, "foo bar", XContentType.JSON.mediaType()); + response.addHeader(HttpHeaders.CONTENT_TYPE, "plain/text"); + assertThat(response.getHeaders().get("Content-Type"), equalTo(List.of("plain/text"))); + response.addHeader(HttpHeaders.CONTENT_TYPE, "newContentType"); + assertThat(response.getHeaders().get("Content-Type"), equalTo(List.of("plain/text", "newContentType"))); + final RestResponse restResponse = response.asRestResponse(); + assertThat(restResponse.status(), equalTo(RestStatus.OK)); + } + + /** + * This test confirms that fake content types work for conversion + */ + @Test + public void testSecurityResponseFakeContentTypeArgumentPasses() { + final SecurityResponse response = new SecurityResponse(HttpStatus.SC_OK, null, "foo bar", "testType"); + final RestResponse restResponse = response.asRestResponse(); + assertThat(restResponse.contentType(), equalTo("testType")); + assertThat(restResponse.status(), equalTo(RestStatus.OK)); + } + /** + * This test checks that types passed as part of the Headers parameter in the argument do not overwrite actual Content Type + */ + @Test + public void testSecurityResponseContentTypeInConstructorHeader() { + final SecurityResponse response = new SecurityResponse(HttpStatus.SC_OK, Map.of("Content-Type", "testType"), "foo bar"); + assertThat(response.getHeaders().get("Content-Type"), equalTo(List.of("testType"))); final RestResponse restResponse = response.asRestResponse(); + assertThat(restResponse.contentType(), equalTo(XContentType.JSON.mediaType())); assertThat(restResponse.status(), equalTo(RestStatus.OK)); + } + + /** + * This test confirms the same as above but with a conflicting content type arg + */ + @Test + public void testSecurityResponseContentTypeInConstructorHeaderConflicts() { + final SecurityResponse response = new SecurityResponse(HttpStatus.SC_OK, Map.of("Content-Type", "testType"), "foo bar", XContentType.JSON.mediaType()); + assertThat(response.getHeaders().get("Content-Type"), equalTo(List.of("testType"))); + final RestResponse restResponse = response.asRestResponse(); assertThat(restResponse.contentType(), equalTo(XContentType.JSON.mediaType())); + assertThat(restResponse.status(), equalTo(RestStatus.OK)); + } + + /** + * This test should check whether unauthorized requests are converted properly + */ + @Test + public void testSecurityResponseUnauthorizedRequestWithPlainTextContentType(){ + final SecurityResponse response = new SecurityResponse(HttpStatus.SC_UNAUTHORIZED, null, "foo bar"); + response.addHeader(HttpHeaders.CONTENT_TYPE, "application/json"); + final RestResponse restResponse = response.asRestResponse(); + assertThat(restResponse.contentType(), equalTo("text/plain; charset=UTF-8")); + assertThat(restResponse.status(), equalTo(RestStatus.UNAUTHORIZED)); + } + + /** + * This test should check whether forbidden requests are converted properly + */ + @Test + public void testSecurityResponseForbiddenRequestWithPlainTextContentType(){ + final SecurityResponse response = new SecurityResponse(HttpStatus.SC_FORBIDDEN, null, "foo bar"); + response.addHeader(HttpHeaders.CONTENT_TYPE, "application/json"); + final RestResponse restResponse = response.asRestResponse(); + assertThat(restResponse.contentType(), equalTo("text/plain; charset=UTF-8")); + assertThat(restResponse.status(), equalTo(RestStatus.FORBIDDEN)); } } From 147be07434d31a44aecd9a11f961e193969b7adb Mon Sep 17 00:00:00 2001 From: Stephen Crawford Date: Wed, 15 Nov 2023 16:09:46 -0500 Subject: [PATCH 2/2] remove prints Signed-off-by: Stephen Crawford --- .../java/org/opensearch/security/filter/SecurityResponse.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/main/java/org/opensearch/security/filter/SecurityResponse.java b/src/main/java/org/opensearch/security/filter/SecurityResponse.java index 87e34f2342..5041936d2e 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityResponse.java +++ b/src/main/java/org/opensearch/security/filter/SecurityResponse.java @@ -77,10 +77,8 @@ public void addHeader(String name, String value) { List header = headers.get(name); if (header == null) { header = new ArrayList<>(); - System.out.println("Adding attribute header of " + value); headers.put(name, header); } - System.out.println("Adding header of " + value); header.add(value); }