Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use BytesRestResponse constructor with contentType in asRestResponse #3717

Merged
merged 14 commits into from
Nov 16, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.opensearch.common.settings.Settings;
import org.opensearch.common.util.concurrent.ThreadContext;
import org.opensearch.common.xcontent.XContentFactory;
import org.opensearch.common.xcontent.XContentType;
import org.opensearch.core.xcontent.XContentBuilder;
import org.opensearch.env.Environment;
import org.opensearch.security.auth.HTTPAuthenticator;
Expand Down Expand Up @@ -284,10 +285,12 @@
public Optional<SecurityResponse> reRequestAuthentication(final SecurityRequest request, AuthCredentials creds) {
final Map<String, String> headers = new HashMap<>();
String responseBody = "";
String contentType = null;

Check warning on line 288 in src/main/java/com/amazon/dlic/auth/http/kerberos/HTTPSpnegoAuthenticator.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/com/amazon/dlic/auth/http/kerberos/HTTPSpnegoAuthenticator.java#L288

Added line #L288 was not covered by tests
SecurityResponse response;
final String negotiateResponseBody = getNegotiateResponseBody();
if (negotiateResponseBody != null) {
responseBody = negotiateResponseBody;
headers.putAll(SecurityResponse.CONTENT_TYPE_APP_JSON);
contentType = XContentType.JSON.mediaType();

Check warning on line 293 in src/main/java/com/amazon/dlic/auth/http/kerberos/HTTPSpnegoAuthenticator.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/com/amazon/dlic/auth/http/kerberos/HTTPSpnegoAuthenticator.java#L293

Added line #L293 was not covered by tests

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should understand who populates the header if we explicitly don't populate here?

}

if (creds == null || creds.getNativeCredentials() == null) {
Expand All @@ -296,7 +299,12 @@
headers.put("WWW-Authenticate", "Negotiate " + Base64.getEncoder().encodeToString((byte[]) creds.getNativeCredentials()));
}

return Optional.of(new SecurityResponse(SC_UNAUTHORIZED, headers, responseBody));
if (contentType != null) {
response = new SecurityResponse(SC_UNAUTHORIZED, headers, responseBody, contentType);

Check warning on line 303 in src/main/java/com/amazon/dlic/auth/http/kerberos/HTTPSpnegoAuthenticator.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/com/amazon/dlic/auth/http/kerberos/HTTPSpnegoAuthenticator.java#L303

Added line #L303 was not covered by tests
} else {
response = new SecurityResponse(SC_UNAUTHORIZED, headers, responseBody);

Check warning on line 305 in src/main/java/com/amazon/dlic/auth/http/kerberos/HTTPSpnegoAuthenticator.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/com/amazon/dlic/auth/http/kerberos/HTTPSpnegoAuthenticator.java#L305

Added line #L305 was not covered by tests
}
return Optional.of(response);

Check warning on line 307 in src/main/java/com/amazon/dlic/auth/http/kerberos/HTTPSpnegoAuthenticator.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/com/amazon/dlic/auth/http/kerberos/HTTPSpnegoAuthenticator.java#L307

Added line #L307 was not covered by tests
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -226,10 +226,10 @@

String responseBodyString = DefaultObjectMapper.objectMapper.writeValueAsString(responseBody);

return Optional.of(new SecurityResponse(HttpStatus.SC_OK, SecurityResponse.CONTENT_TYPE_APP_JSON, responseBodyString));
return Optional.of(new SecurityResponse(HttpStatus.SC_OK, null, responseBodyString, XContentType.JSON.mediaType()));
} catch (JsonProcessingException e) {
log.warn("Error while parsing JSON for /_opendistro/_security/api/authtoken", e);
return Optional.of(new SecurityResponse(HttpStatus.SC_BAD_REQUEST, new Exception("JSON could not be parsed")));
return Optional.of(new SecurityResponse(HttpStatus.SC_BAD_REQUEST, "JSON could not be parsed"));

Check warning on line 232 in src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java#L232

Added line #L232 was not covered by tests
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ public boolean authenticate(final SecurityRequestChannel request) {
log.debug("Rejecting REST request because of blocked address: {}", request.getRemoteAddress().orElse(null));
}

request.queueForSending(new SecurityResponse(SC_UNAUTHORIZED, new Exception("Authentication finally failed")));
request.queueForSending(new SecurityResponse(SC_UNAUTHORIZED, "Authentication finally failed"));
return false;
}

Expand All @@ -224,7 +224,7 @@ public boolean authenticate(final SecurityRequestChannel request) {

if (!isInitialized()) {
log.error("Not yet initialized (you may need to run securityadmin)");
request.queueForSending(new SecurityResponse(SC_SERVICE_UNAVAILABLE, new Exception("OpenSearch Security not initialized.")));
request.queueForSending(new SecurityResponse(SC_SERVICE_UNAVAILABLE, "OpenSearch Security not initialized."));
return false;
}

Expand Down Expand Up @@ -354,11 +354,7 @@ public boolean authenticate(final SecurityRequestChannel request) {
log.error("Cannot authenticate rest user because admin user is not permitted to login via HTTP");
auditLog.logFailedLogin(authenticatedUser.getName(), true, null, request);
request.queueForSending(
new SecurityResponse(
SC_FORBIDDEN,
null,
"Cannot authenticate user because admin user is not permitted to login via HTTP"
)
new SecurityResponse(SC_FORBIDDEN, "Cannot authenticate user because admin user is not permitted to login via HTTP")
);
return false;
}
Expand Down Expand Up @@ -429,7 +425,7 @@ public boolean authenticate(final SecurityRequestChannel request) {
notifyIpAuthFailureListeners(request, authCredentials);

request.queueForSending(
challengeResponse.orElseGet(() -> new SecurityResponse(SC_UNAUTHORIZED, null, "Authentication finally failed"))
challengeResponse.orElseGet(() -> new SecurityResponse(SC_UNAUTHORIZED, "Authentication finally failed"))
);
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,15 @@
package org.opensearch.security.filter;

import java.io.IOException;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import org.apache.http.HttpHeaders;

import org.opensearch.common.xcontent.XContentFactory;
import org.opensearch.common.xcontent.XContentType;
import org.opensearch.core.rest.RestStatus;
import org.opensearch.rest.BytesRestResponse;
import org.opensearch.rest.RestResponse;
Expand All @@ -26,26 +30,63 @@ public class SecurityResponse {
public static final Map<String, String> CONTENT_TYPE_APP_JSON = Map.of(HttpHeaders.CONTENT_TYPE, "application/json");

private final int status;
private final Map<String, String> headers;
private Map<String, List<String>> headers;
private final String body;
private final String contentType;

public SecurityResponse(final int status, final Exception e) {
this.status = status;
this.headers = CONTENT_TYPE_APP_JSON;
populateHeaders(CONTENT_TYPE_APP_JSON);
this.body = generateFailureMessage(e);
this.contentType = XContentType.JSON.mediaType();
}

public SecurityResponse(final int status, String body) {
this.status = status;
this.body = body;
this.contentType = null;
}

public SecurityResponse(final int status, final Map<String, String> headers, final String body) {
this.status = status;
this.headers = headers;
populateHeaders(headers);
this.body = body;
this.contentType = null;
}

public SecurityResponse(final int status, final Map<String, String> headers, final String body, String contentType) {
this.status = status;
this.body = body;
this.contentType = contentType;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

call other constructor here

populateHeaders(headers);
}

private void populateHeaders(Map<String, String> headers) {
if (headers != null) {
headers.entrySet().forEach(entry -> addHeader(entry.getKey(), entry.getValue()));
}
}

/**
* Add a custom header.
*/
public void addHeader(String name, String value) {
if (headers == null) {
headers = new HashMap<>(2);
}
List<String> header = headers.get(name);
if (header == null) {
header = new ArrayList<>();
headers.put(name, header);
}
header.add(value);
}

public int getStatus() {
return status;
}

public Map<String, String> getHeaders() {
public Map<String, List<String>> getHeaders() {
return headers;
}

Expand All @@ -54,9 +95,14 @@ public String getBody() {
}

public RestResponse asRestResponse() {
final RestResponse restResponse = new BytesRestResponse(RestStatus.fromCode(getStatus()), getBody());
final RestResponse restResponse;
if (this.contentType != null) {
restResponse = new BytesRestResponse(RestStatus.fromCode(getStatus()), this.contentType, getBody());
} else {
restResponse = new BytesRestResponse(RestStatus.fromCode(getStatus()), getBody());
}
if (getHeaders() != null) {
getHeaders().forEach(restResponse::addHeader);
getHeaders().entrySet().forEach(entry -> { entry.getValue().forEach(value -> restResponse.addHeader(entry.getKey(), value)); });

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should understand why the header approach was not working and which part of the code is populating the content type header now?

}
return restResponse;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@
}
log.debug(err);

request.queueForSending(new SecurityResponse(HttpStatus.SC_UNAUTHORIZED, null, err));
request.queueForSending(new SecurityResponse(HttpStatus.SC_UNAUTHORIZED, err));
return;
}
}
Expand Down Expand Up @@ -288,7 +288,7 @@
} catch (SSLPeerUnverifiedException e) {
log.error("No ssl info", e);
auditLog.logSSLException(requestChannel, e);
requestChannel.queueForSending(new SecurityResponse(HttpStatus.SC_FORBIDDEN, new Exception("No ssl info")));
requestChannel.queueForSending(new SecurityResponse(HttpStatus.SC_FORBIDDEN, e));

Check warning on line 291 in src/main/java/org/opensearch/security/filter/SecurityRestFilter.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/filter/SecurityRestFilter.java#L291

Added line #L291 was not covered by tests
return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import javax.net.ssl.SSLEngine;
import javax.net.ssl.SSLParameters;

import com.google.common.collect.Lists;
import org.apache.hc.client5.http.config.RequestConfig;
import org.apache.hc.client5.http.impl.async.HttpAsyncClientBuilder;
import org.apache.hc.client5.http.impl.nio.PoolingAsyncClientConnectionManagerBuilder;
Expand All @@ -52,6 +53,7 @@
import org.apache.hc.core5.ssl.SSLContexts;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;

import org.opensearch.action.index.IndexRequest;
import org.opensearch.action.index.IndexResponse;
import org.opensearch.action.support.WriteRequest.RefreshPolicy;
Expand All @@ -62,8 +64,6 @@
import org.opensearch.client.RestHighLevelClient;
import org.opensearch.common.xcontent.XContentType;

import com.google.common.collect.Lists;

public class HttpClient implements Closeable {

public static class HttpClientBuilder {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.apache.http.HttpStatus;

import org.opensearch.common.xcontent.XContentFactory;
import org.opensearch.common.xcontent.XContentType;
import org.opensearch.core.rest.RestStatus;
import org.opensearch.security.filter.SecurityRequest;
import org.opensearch.security.filter.SecurityResponse;
Expand Down Expand Up @@ -113,7 +114,7 @@ public Optional<SecurityResponse> checkRequestIsAllowed(final SecurityRequest re
// if allowlisting is enabled but the request is not allowlisted, then return false, otherwise true.
if (this.enabled && !requestIsAllowlisted(request)) {
return Optional.of(
new SecurityResponse(HttpStatus.SC_FORBIDDEN, SecurityResponse.CONTENT_TYPE_APP_JSON, generateFailureMessage(request))
new SecurityResponse(HttpStatus.SC_FORBIDDEN, null, generateFailureMessage(request), XContentType.JSON.mediaType())
);
}
return Optional.empty();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import org.apache.http.HttpStatus;

import org.opensearch.common.xcontent.XContentType;
import org.opensearch.security.filter.SecurityRequest;
import org.opensearch.security.filter.SecurityResponse;

Expand Down Expand Up @@ -111,7 +112,7 @@ public Optional<SecurityResponse> checkRequestIsAllowed(final SecurityRequest re
// if whitelisting is enabled but the request is not whitelisted, then return false, otherwise true.
if (this.enabled && !requestIsWhitelisted(request)) {
return Optional.of(
new SecurityResponse(HttpStatus.SC_FORBIDDEN, SecurityResponse.CONTENT_TYPE_APP_JSON, generateFailureMessage(request))
new SecurityResponse(HttpStatus.SC_FORBIDDEN, null, generateFailureMessage(request), XContentType.JSON.mediaType())
);
}
return Optional.empty();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -887,7 +887,7 @@ private AuthenticateHeaders getAutenticateHeaders(HTTPSamlAuthenticator samlAuth
RestRequest restRequest = new FakeRestRequest(ImmutableMap.of(), new HashMap<String, String>());
SecurityResponse response = sendToAuthenticator(samlAuthenticator, restRequest).orElseThrow();

String wwwAuthenticateHeader = response.getHeaders().get("WWW-Authenticate");
String wwwAuthenticateHeader = response.getHeaders().get("WWW-Authenticate").get(0);

Assert.assertNotNull(wwwAuthenticateHeader);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*
* Modifications Copyright OpenSearch Contributors. See
* GitHub history for details.
*/

package org.opensearch.security.filter;

import org.apache.http.HttpStatus;
import org.junit.Test;

import org.opensearch.common.xcontent.XContentType;
import org.opensearch.core.rest.RestStatus;
import org.opensearch.rest.RestResponse;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;

public class SecurityResponseTests {

@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()));
}
}
Loading