Skip to content

Commit

Permalink
Missing credentials result in 401 UNAUTHORIZED responses from conjure…
Browse files Browse the repository at this point in the history 
…-undertow (#656)

Missing credentials result in 401 UNAUTHORIZED responses from conjure-undertow
Previously we responded with a 400 bad request status, which isn't as accurate as we would like.
  • Loading branch information
@carterkozak @bulldozer-bot
carterkozak authored and bulldozer-bot[bot] committed Nov 26, 2019
1 parent b8512f3 commit f773fdd
Show file tree
Hide file tree
Showing 8 changed files with 118 additions and 37 deletions.
7 changes: 7 additions & 0 deletions changelog/@unreleased/pr-656.v2.yml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
type: improvement
improvement:
description: |-
Missing credentials result in 401 UNAUTHORIZED responses from conjure-undertow
Previously we responded with a 400 bad request status, which isn't as accurate as we would like.
links:
- https://github.com/palantir/conjure-java/pull/656
6 changes: 3 additions & 3 deletions conjure-java-core/src/test/java/com/palantir/conjure/java/UndertowServiceEteTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,12 +219,12 @@ public void java_url_client_receives_ok_with_complete_request() throws IOExcepti
}

@Test
public void java_url_client_receives_bad_request_without_authheader() throws IOException {
public void java_url_client_receives_unauthorized_without_authheader() throws IOException {
HttpURLConnection httpUrlConnection = preparePostRequest();
sendPostRequestData(
httpUrlConnection,
CLIENT_OBJECT_MAPPER.writeValueAsString(StringAliasExample.of("foo")));
assertThat(httpUrlConnection.getResponseCode()).isEqualTo(400);
assertThat(httpUrlConnection.getResponseCode()).isEqualTo(401);
}

@Test
Expand All @@ -251,7 +251,7 @@ public void java_url_client_receives_unprocessable_entity_with_null_body() throw
try (InputStream responseBody = httpUrlConnection.getErrorStream()) {
SerializableError error = CLIENT_OBJECT_MAPPER.readValue(responseBody, SerializableError.class);
assertThat(error.errorCode()).isEqualTo("INVALID_ARGUMENT");
assertThat(error.errorName()).isEqualTo("Default:InvalidArgument");
assertThat(error.errorName()).isEqualTo("Conjure:UnprocessableEntity");
}
}

Expand Down
1 change: 1 addition & 0 deletions conjure-java-undertow-runtime/build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ dependencies {
testImplementation 'org.mockito:mockito-core'
testImplementation 'org.mockito:mockito-junit-jupiter'
testImplementation 'org.junit.jupiter:junit-jupiter'
testImplementation 'com.palantir.conjure.java.api:test-utils'

compileOnly 'org.immutables:value::annotations'
testCompileOnly 'org.immutables:value::annotations'
Expand Down
58 changes: 43 additions & 15 deletions ...c/main/java/com/palantir/conjure/java/undertow/runtime/ConjureAuthorizationExtractor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@

package com.palantir.conjure.java.undertow.runtime;

import com.palantir.conjure.java.api.errors.ErrorType;
import com.palantir.conjure.java.api.errors.ServiceException;
import com.palantir.conjure.java.undertow.lib.AuthorizationExtractor;
import com.palantir.conjure.java.undertow.lib.PlainSerDe;
import com.palantir.logsafe.Preconditions;
import com.palantir.tokens.auth.AuthHeader;
import com.palantir.tokens.auth.BearerToken;
import com.palantir.tokens.auth.UnverifiedJsonWebToken;
import io.undertow.server.HttpServerExchange;
import io.undertow.server.handlers.Cookie;
import io.undertow.util.HeaderValues;
import io.undertow.util.Headers;
import java.util.Optional;
Expand All @@ -38,6 +40,16 @@
*/
final class ConjureAuthorizationExtractor implements AuthorizationExtractor {

private static final String USER_ID_KEY = "userId";
private static final String SESSION_ID_KEY = "sessionId";
private static final String TOKEN_ID_KEY = "tokenId";
private static Consumer<String> sessionIdSetter = sessionId -> MDC.put(SESSION_ID_KEY, sessionId);
private static Consumer<String> tokenIdSetter = tokenId -> MDC.put(TOKEN_ID_KEY, tokenId);
private static final ErrorType MISSING_CREDENTIAL_ERROR_TYPE = ErrorType.create(
ErrorType.Code.UNAUTHORIZED, "Conjure:MissingCredentials");
private static final ErrorType MALFORMED_CREDENTIAL_ERROR_TYPE = ErrorType.create(
ErrorType.Code.UNAUTHORIZED, "Conjure:MalformedCredentials");

private final PlainSerDe plainSerDe;

ConjureAuthorizationExtractor(PlainSerDe plainSerDe) {
Expand All @@ -50,12 +62,8 @@ final class ConjureAuthorizationExtractor implements AuthorizationExtractor {
*/
@Override
public AuthHeader header(HttpServerExchange exchange) {
HeaderValues authorization = exchange.getRequestHeaders().get(Headers.AUTHORIZATION);
// Do not use Iterables.getOnlyElement because it includes values in the exception message.
// We do not want credential material logged to disk, even if it's marked unsafe.
Preconditions.checkArgument(authorization != null && authorization.size() == 1,
"One Authorization header value is required");
return setState(exchange, AuthHeader.valueOf(authorization.get(0)));
AuthHeader authHeader = parseAuthHeader(exchange);
return setState(exchange, authHeader);
}

/**
Expand All @@ -64,16 +72,19 @@ public AuthHeader header(HttpServerExchange exchange) {
*/
@Override
public BearerToken cookie(HttpServerExchange exchange, String cookieName) {
return setState(exchange,
plainSerDe.deserializeBearerToken(exchange.getRequestCookies().get(cookieName).getValue()));
Cookie cookie = exchange.getRequestCookies().get(cookieName);
if (cookie == null) {
throw new ServiceException(MISSING_CREDENTIAL_ERROR_TYPE);
}
try {
return setState(
exchange,
plainSerDe.deserializeBearerToken(cookie.getValue()));
} catch (RuntimeException e) {
throw new ServiceException(MALFORMED_CREDENTIAL_ERROR_TYPE, e);
}
}

private static final String USER_ID_KEY = "userId";
private static final String SESSION_ID_KEY = "sessionId";
private static final String TOKEN_ID_KEY = "tokenId";
private static Consumer<String> sessionIdSetter = sessionId -> MDC.put(SESSION_ID_KEY, sessionId);
private static Consumer<String> tokenIdSetter = tokenId -> MDC.put(TOKEN_ID_KEY, tokenId);

/**
* Attempts to extract a {@link UnverifiedJsonWebToken JSON Web Token} from the
* {@link BearerToken} value, and populates the SLF4J {@link MDC} with
Expand All @@ -96,4 +107,21 @@ private static AuthHeader setState(HttpServerExchange exchange, AuthHeader authH
setState(exchange, authHeader.getBearerToken());
return authHeader;
}

private static AuthHeader parseAuthHeader(HttpServerExchange exchange) {
HeaderValues authorization = exchange.getRequestHeaders().get(Headers.AUTHORIZATION);
// Do not use Iterables.getOnlyElement because it includes values in the exception message.
// We do not want credential material logged to disk, even if it's marked unsafe.
if (authorization == null) {
throw new ServiceException(MISSING_CREDENTIAL_ERROR_TYPE);
}
if (authorization.size() != 1) {
throw new ServiceException(MALFORMED_CREDENTIAL_ERROR_TYPE);
}
try {
return AuthHeader.valueOf(authorization.get(0));
} catch (RuntimeException e) {
throw new ServiceException(MALFORMED_CREDENTIAL_ERROR_TYPE, e);
}
}
}
3 changes: 1 addition & 2 deletions ...w-runtime/src/main/java/com/palantir/conjure/java/undertow/runtime/ConjureExceptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,7 @@ private static void frameworkException(
Serializer<SerializableError> serializer,
FrameworkException frameworkException) {
int statusCode = frameworkException.getStatusCode();
ErrorType errorType = statusCode / 100 == 4 ? ErrorType.INVALID_ARGUMENT : ErrorType.INTERNAL;
ServiceException exception = new ServiceException(errorType, frameworkException);
ServiceException exception = new ServiceException(frameworkException.getErrorType(), frameworkException);
log(exception);
writeResponse(exchange, Optional.of(SerializableError.forException(exception)), statusCode, serializer);
}
Expand Down
24 changes: 19 additions & 5 deletions ...-runtime/src/main/java/com/palantir/conjure/java/undertow/runtime/FrameworkException.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
package com.palantir.conjure.java.undertow.runtime;

import com.google.common.collect.ImmutableList;
import com.google.errorprone.annotations.CompileTimeConstant;
import com.palantir.conjure.java.api.errors.ErrorType;
import com.palantir.logsafe.Arg;
import com.palantir.logsafe.SafeLoggable;
import io.undertow.util.StatusCodes;
Expand All @@ -25,23 +27,31 @@
/** Internal type to signal a conjure protocol-level failure with a specific response code. */
final class FrameworkException extends RuntimeException implements SafeLoggable {

private static final ErrorType UNPROCESSABLE_ENTITY = ErrorType.create(
ErrorType.Code.INVALID_ARGUMENT, "Conjure:UnprocessableEntity");
private static final ErrorType UNSUPPORTED_MEDIA_TYPE = ErrorType.create(
ErrorType.Code.INVALID_ARGUMENT, "Conjure:UnsupportedMediaType");

private final String logMessage;
private final List<Arg<?>> arguments;
private final int statusCode;
private final ErrorType errorType;

private FrameworkException(String message, int statusCode, Throwable cause, Arg<?>... args) {
private FrameworkException(String message, ErrorType errorType, int statusCode, Throwable cause, Arg<?>... args) {
super(renderMessage(message, args), cause);
this.logMessage = message;
this.arguments = ImmutableList.copyOf(args);
this.statusCode = statusCode;
this.errorType = errorType;
}

static FrameworkException unprocessableEntity(String message, Throwable cause, Arg<?>... args) {
return new FrameworkException(message, StatusCodes.UNPROCESSABLE_ENTITY, cause, args);
static FrameworkException unprocessableEntity(
@CompileTimeConstant String message, Throwable cause, Arg<?>... args) {
return new FrameworkException(message, UNPROCESSABLE_ENTITY, StatusCodes.UNPROCESSABLE_ENTITY, cause, args);
}

static FrameworkException unsupportedMediaType(String message, Arg<?>... args) {
return new FrameworkException(message, StatusCodes.UNSUPPORTED_MEDIA_TYPE, null, args);
static FrameworkException unsupportedMediaType(@CompileTimeConstant String message, Arg<?>... args) {
return new FrameworkException(message, UNSUPPORTED_MEDIA_TYPE, StatusCodes.UNSUPPORTED_MEDIA_TYPE, null, args);
}

@Override
Expand All @@ -58,6 +68,10 @@ int getStatusCode() {
return statusCode;
}

ErrorType getErrorType() {
return errorType;
}

private static String renderMessage(String message, Arg<?>... args) {
if (args.length == 0) {
return message;
Expand Down
49 changes: 40 additions & 9 deletions ...a-undertow-runtime/src/test/java/com/palantir/conjure/java/undertow/runtime/AuthTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,19 @@

package com.palantir.conjure.java.undertow.runtime;

import static com.palantir.conjure.java.api.testing.Assertions.assertThatServiceExceptionThrownBy;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import com.palantir.conjure.java.api.errors.ErrorType;
import com.palantir.conjure.java.undertow.HttpServerExchanges;
import com.palantir.conjure.java.undertow.lib.UndertowRuntime;
import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;
import com.palantir.tokens.auth.AuthHeader;
import com.palantir.tokens.auth.BearerToken;
import io.undertow.server.HttpServerExchange;
import io.undertow.server.handlers.CookieImpl;
import io.undertow.util.Headers;
import org.junit.jupiter.api.Test;


public final class AuthTest {

private static final UndertowRuntime CONTEXT = ConjureUndertowRuntime.builder().build();
Expand All @@ -44,18 +44,49 @@ public void testParseAuthHeader() {
@Test
public void testAuthHeaderNotPresent() {
HttpServerExchange exchange = HttpServerExchanges.createStub();
assertThatThrownBy(() -> CONTEXT.auth().header(exchange))
.isInstanceOf(SafeIllegalArgumentException.class)
.hasMessage("One Authorization header value is required");
assertThatServiceExceptionThrownBy(() -> CONTEXT.auth().header(exchange))
.hasType(ErrorType.create(ErrorType.Code.UNAUTHORIZED, "Conjure:MissingCredentials"));
}

@Test
public void testAuthHeaderEmptyValue() {
HttpServerExchange exchange = HttpServerExchanges.createStub();
exchange.getRequestHeaders().add(Headers.AUTHORIZATION, "");
assertThatServiceExceptionThrownBy(() -> CONTEXT.auth().header(exchange))
.hasType(ErrorType.create(ErrorType.Code.UNAUTHORIZED, "Conjure:MalformedCredentials"));
}

@Test
public void testAuthHeaderMultipleValues() {
HttpServerExchange exchange = HttpServerExchanges.createStub();
exchange.getRequestHeaders().add(Headers.AUTHORIZATION, "Bearer foo");
exchange.getRequestHeaders().add(Headers.AUTHORIZATION, "Bearer bar");
assertThatThrownBy(() -> CONTEXT.auth().header(exchange))
.isInstanceOf(SafeIllegalArgumentException.class)
.hasMessage("One Authorization header value is required");
assertThatServiceExceptionThrownBy(() -> CONTEXT.auth().header(exchange))
.hasType(ErrorType.create(ErrorType.Code.UNAUTHORIZED, "Conjure:MalformedCredentials"));
}

@Test
public void testParseAuthCookie() {
BearerToken expected = BearerToken.valueOf("token");
String cookieName = "Auth-Token";
HttpServerExchange exchange = HttpServerExchanges.createStub();
exchange.getRequestCookies().put(cookieName, new CookieImpl(cookieName, "token"));
assertThat(CONTEXT.auth().cookie(exchange, cookieName)).isEqualTo(expected);
}

@Test
public void testAuthCookieNotPresent() {
HttpServerExchange exchange = HttpServerExchanges.createStub();
assertThatServiceExceptionThrownBy(() -> CONTEXT.auth().cookie(exchange, "any"))
.hasType(ErrorType.create(ErrorType.Code.UNAUTHORIZED, "Conjure:MissingCredentials"));
}

@Test
public void testAuthCookieEmptyValue() {
String cookieName = "Auth-Token";
HttpServerExchange exchange = HttpServerExchanges.createStub();
exchange.getRequestCookies().put(cookieName, new CookieImpl(cookieName, ""));
assertThatServiceExceptionThrownBy(() -> CONTEXT.auth().cookie(exchange, cookieName))
.hasType(ErrorType.create(ErrorType.Code.UNAUTHORIZED, "Conjure:MalformedCredentials"));
}
}
7 changes: 4 additions & 3 deletions versions.lock
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava (1 c
com.google.j2objc:j2objc-annotations:1.3 (1 constraints: b809eda0)
com.palantir.conjure:conjure-api-objects:4.5.1 (2 constraints: fb219fa8)
com.palantir.conjure:conjure-generator-common:4.5.1 (2 constraints: 9f13b761)
com.palantir.conjure.java.api:errors:2.9.0 (3 constraints: a232e465)
com.palantir.conjure.java.api:errors:2.9.0 (4 constraints: c043f199)
com.palantir.conjure.java.runtime:conjure-java-jackson-serialization:4.43.1 (5 constraints: 3066e609)
com.palantir.ri:resource-identifier:1.0.1 (1 constraints: 0405f135)
com.palantir.safe-logging:preconditions:1.12.0 (11 constraints: eebb969b)
Expand Down Expand Up @@ -68,6 +68,7 @@ com.netflix.feign:feign-slf4j:8.18.0 (1 constraints: c718909e)
com.palantir.conjure:conjure-core:4.5.1 (1 constraints: 0c050f36)
com.palantir.conjure.java.api:service-config:2.9.0 (1 constraints: ed13c862)
com.palantir.conjure.java.api:ssl-config:2.9.0 (2 constraints: 4825f9f6)
com.palantir.conjure.java.api:test-utils:2.9.0 (1 constraints: 0d051036)
com.palantir.conjure.java.runtime:client-config:4.43.1 (3 constraints: b0470cd9)
com.palantir.conjure.java.runtime:conjure-java-jaxrs-client:4.43.1 (1 constraints: 3e05513b)
com.palantir.conjure.java.runtime:conjure-java-jersey-server:4.43.1 (1 constraints: 3e05513b)
Expand Down Expand Up @@ -121,7 +122,7 @@ net.bytebuddy:byte-buddy-agent:1.9.10 (1 constraints: 450b52de)
net.sourceforge.argparse4j:argparse4j:0.8.1 (1 constraints: 430d3a1f)
org.apache.commons:commons-text:1.2 (1 constraints: ae10289b)
org.apiguardian:apiguardian-api:1.1.0 (6 constraints: 7c64a2c6)
org.assertj:assertj-core:3.14.0 (3 constraints: 3128c548)
org.assertj:assertj-core:3.14.0 (4 constraints: 7c39bfcd)
org.eclipse.jetty:jetty-continuation:9.4.11.v20180605 (2 constraints: 74211443)
org.eclipse.jetty:jetty-http:9.4.11.v20180605 (3 constraints: 0931f051)
org.eclipse.jetty:jetty-io:9.4.11.v20180605 (3 constraints: f02f89af)
Expand Down Expand Up @@ -163,7 +164,7 @@ org.junit.platform:junit-platform-commons:1.5.2 (2 constraints: d720664a)
org.junit.platform:junit-platform-engine:1.5.2 (2 constraints: 24213073)
org.junit.vintage:junit-vintage-engine:5.4.2 (1 constraints: 0d051236)
org.jvnet:animal-sniffer-annotation:1.0 (1 constraints: f20b95eb)
org.mockito:mockito-core:3.1.0 (2 constraints: c4135a64)
org.mockito:mockito-core:3.1.0 (3 constraints: db24a5f7)
org.mockito:mockito-junit-jupiter:3.1.0 (1 constraints: 0605fd35)
org.objenesis:objenesis:2.6 (2 constraints: ec1819aa)
org.opentest4j:opentest4j:1.2.0 (2 constraints: cd205b49)
Expand Down

0 comments on commit f773fdd

Please sign in to comment.