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.

Sign up for GitHub

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

Jump to bottom

Add param converters for authentication credentials #1374

Merged
merged 5 commits into from Jan 6, 2020
Merged

Add param converters for authentication credentials #1374

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d59a161
Add param converters for authentication
pkoenig10 Dec 12, 2019
1040dde
Add generated changelog entries
pkoenig10 Dec 13, 2019
b52caa3
Limit to Conjure auth parameters
pkoenig10 Dec 18, 2019
6a3dd09
Don't return 401 for null authentication credentials
pkoenig10 Jan 2, 2020
a303b52
Comments
pkoenig10 Jan 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions changelog/@unreleased/pr-1374.v2.yml
View file
Open in desktop
@@ -0,0 +1,7 @@
type: improvement
improvement:
description: Missing or malformed credentials will result in a 401 UNAUTHORIZED
from Jersey servers. Previously we responded with a 400, which isn't as accurate
as we would like.
links:
- https://github.com/palantir/conjure-java-runtime/pull/1374
8 changes: 4 additions & 4 deletions .../java/com/palantir/conjure/java/client/jaxrs/feignimpl/GuavaOptionalAwareDecoderTest.java
View file
Open in desktop
Expand Up @@ -82,17 +82,17 @@ public void testThrowsNotFound() {
public void testThrowsNotAuthorized() {
assertThatThrownBy(() -> service.getThrowsNotAuthorized(null))
.isInstanceOfSatisfying(RemoteException.class, e -> {
assertThat(e.getMessage()).contains("RemoteException: javax.ws.rs.NotAuthorizedException");
assertThat(e.getError().errorCode()).isEqualTo("javax.ws.rs.NotAuthorizedException");
assertThat(e.getMessage()).contains("RemoteException: UNAUTHORIZED (Default:Unauthorized)");
assertThat(e.getError().errorCode()).isEqualTo("UNAUTHORIZED");
});
}

@Test
public void testOptionalThrowsNotAuthorized() {
assertThatThrownBy(() -> service.getOptionalThrowsNotAuthorized(null))
.isInstanceOfSatisfying(RemoteException.class, e -> {
assertThat(e.getMessage()).contains("RemoteException: javax.ws.rs.NotAuthorizedException");
assertThat(e.getError().errorCode()).isEqualTo("javax.ws.rs.NotAuthorizedException");
assertThat(e.getMessage()).contains("RemoteException: UNAUTHORIZED (Default:Unauthorized)");
assertThat(e.getError().errorCode()).isEqualTo("UNAUTHORIZED");
});
}

Expand Down
8 changes: 4 additions & 4 deletions .../java/com/palantir/conjure/java/client/jaxrs/feignimpl/Java8OptionalAwareDecoderTest.java
View file
Open in desktop
Expand Up @@ -82,17 +82,17 @@ public void testThrowsNotFound() {
public void testThrowsNotAuthorized() {
assertThatThrownBy(() -> service.getThrowsNotAuthorized(null))
.isInstanceOfSatisfying(RemoteException.class, e -> {
assertThat(e.getMessage()).contains("RemoteException: javax.ws.rs.NotAuthorizedException");
assertThat(e.getError().errorCode()).isEqualTo("javax.ws.rs.NotAuthorizedException");
assertThat(e.getMessage()).contains("RemoteException: UNAUTHORIZED (Default:Unauthorized)");
assertThat(e.getError().errorCode()).isEqualTo("UNAUTHORIZED");
});
}

@Test
public void testOptionalThrowsNotAuthorized() {
assertThatThrownBy(() -> service.getOptionalThrowsNotAuthorized(null))
.isInstanceOfSatisfying(RemoteException.class, e -> {
assertThat(e.getMessage()).contains("RemoteException: javax.ws.rs.NotAuthorizedException");
assertThat(e.getError().errorCode()).isEqualTo("javax.ws.rs.NotAuthorizedException");
assertThat(e.getMessage()).contains("RemoteException: UNAUTHORIZED (Default:Unauthorized)");
assertThat(e.getError().errorCode()).isEqualTo("UNAUTHORIZED");
});
}

Expand Down
1 change: 1 addition & 0 deletions conjure-java-jersey-server/build.gradle
View file
Open in desktop
Expand Up @@ -13,6 +13,7 @@ dependencies {
implementation "com.fasterxml.jackson.module:jackson-module-afterburner"
implementation "com.jcraft:jzlib"
implementation "com.palantir.safe-logging:safe-logging"
implementation 'com.palantir.tokens:auth-tokens'
implementation "com.palantir.tracing:tracing-jersey"
implementation project(':conjure-java-jackson-serialization')

Expand Down
78 changes: 78 additions & 0 deletions ...c/main/java/com/palantir/conjure/java/server/jersey/AuthHeaderParamConverterProvider.java
View file
Open in desktop
@@ -0,0 +1,78 @@
/*
* (c) Copyright 2017 Palantir Technologies Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.palantir.conjure.java.server.jersey;

import com.palantir.logsafe.Preconditions;
import com.palantir.tokens.auth.AuthHeader;
import java.lang.annotation.Annotation;
import java.lang.reflect.Type;
import javax.ws.rs.HeaderParam;
import javax.ws.rs.core.HttpHeaders;
import javax.ws.rs.ext.ParamConverter;
import javax.ws.rs.ext.ParamConverterProvider;
import javax.ws.rs.ext.Provider;
import org.glassfish.jersey.internal.inject.Custom;

@Custom
@Provider
public final class AuthHeaderParamConverterProvider implements ParamConverterProvider {
private final AuthHeaderParamConverter paramConverter = new AuthHeaderParamConverter();

@Override
@SuppressWarnings("unchecked")
public <T> ParamConverter<T> getConverter(
final Class<T> rawType,
final Type _genericType,
final Annotation[] annotations) {
return AuthHeader.class.equals(rawType) && hasAuthAnnotation(annotations)
? (ParamConverter<T>) paramConverter
: null;
}

public static final class AuthHeaderParamConverter implements ParamConverter<AuthHeader> {
@Override
public AuthHeader fromString(final String value) {
if (value == null) {
throw UnauthorizedException.missingCredentials();
}
carterkozak marked this conversation as resolved.
Show resolved Hide resolved
try {
return AuthHeader.valueOf(value);
} catch (RuntimeException e) {
throw UnauthorizedException.malformedCredentials(e);
}
}

@Override
public String toString(final AuthHeader value) {
Preconditions.checkArgument(value != null);
return value.toString();
}
}

private static boolean hasAuthAnnotation(Annotation[] annotations) {
for (Annotation annotation : annotations) {
if (annotation.annotationType() == HeaderParam.class) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
if (annotation.annotationType() == HeaderParam.class) {
if (HeaderParam.class.equals(annotation.annotationType())) {

String value = ((HeaderParam) annotation).value();
if (value.equals(HttpHeaders.AUTHORIZATION)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, safer around nulls:

Suggested change
if (value.equals(HttpHeaders.AUTHORIZATION)) {
if (HttpHeaders.AUTHORIZATION.equals(value)) {

return true;
}
}
}

return false;
}
}
74 changes: 74 additions & 0 deletions .../main/java/com/palantir/conjure/java/server/jersey/BearerTokenParamConverterProvider.java
View file
Open in desktop
@@ -0,0 +1,74 @@
/*
* (c) Copyright 2017 Palantir Technologies Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.palantir.conjure.java.server.jersey;

import com.palantir.logsafe.Preconditions;
import com.palantir.tokens.auth.BearerToken;
import java.lang.annotation.Annotation;
import java.lang.reflect.Type;
import javax.ws.rs.CookieParam;
import javax.ws.rs.ext.ParamConverter;
import javax.ws.rs.ext.ParamConverterProvider;
import javax.ws.rs.ext.Provider;
import org.glassfish.jersey.internal.inject.Custom;

@Custom
@Provider
public final class BearerTokenParamConverterProvider implements ParamConverterProvider {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not confident this is correct, while cookie auth uses the BearerToken type, we can define additional parameters of type BearerToken that aren't the conjure-defined auth token.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mention this in the PR description:

These param converters will apply to all AuthHeader and BearerToken parameters. However it is very rare for these types to be used outside of the endpoint auth. And most of those cases are to perform some sort of delegate authentication, so a 401 is not inappropriate.

I don't think I've ever seen a BearerToken used as an argument outside of cookie auth. And the only times I've seen a AuthHeader used as argument outside of header auth is for delegating authorization, in which case you could argue that a 401 is not inappropriate.

I don't think there is another way to achieve the desired behavior. If you have other suggestions, I'm happy to hear them. But I think the benefit of responding with 401 for invalid auth credentials outweighs the minor downside of this applying to other parameters of the same type.

Copy link
Member Author

@pkoenig10 pkoenig10 Dec 18, 2019 •
edited

Choose a reason for hiding this comment

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

I've updated this PR to only apply to Conjure auth parameters. This means:

  • AuthHeader parameter must be annotated with @HeaderParam and have a value of Authorization
  • BearerToken parameter must be annotated with @CookieParam and have any value

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds reasonable, I don't think we need the additional specificity for AuthHeader because conjure only uses that type for auth components, bearer tokens use the BearerToken type.

Copy link
Member Author

@pkoenig10 pkoenig10 Dec 18, 2019 •
edited

Choose a reason for hiding this comment

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

I would prefer to not respond with 401 for parameters we know aren't the Conjure auth parameter. It's possible to have other AuthHeader header params by using external imports. We actually do this for a number of endpoints and use a second AuthHeader header param to represent a delegating user. It makes more sense to return 400 for these parameters since they are effectively arguments that just happen to be passed as headers.

Because Conjure doesn't allow arbitrary cookie parameters, this implementation will exactly match all auth parameters and nothing else.

private final BearerTokenParamConverter paramConverter = new BearerTokenParamConverter();

@Override
@SuppressWarnings("unchecked")
public <T> ParamConverter<T> getConverter(
final Class<T> rawType,
final Type _genericType,
final Annotation[] annotations) {
return BearerToken.class.equals(rawType) && hasAuthAnnotation(annotations)
? (ParamConverter<T>) paramConverter
: null;
}

public static final class BearerTokenParamConverter implements ParamConverter<BearerToken> {
@Override
public BearerToken fromString(final String value) {
if (value == null) {
throw UnauthorizedException.missingCredentials();
}
try {
return BearerToken.valueOf(value);
} catch (RuntimeException e) {
throw UnauthorizedException.malformedCredentials(e);
}
}

@Override
public String toString(final BearerToken value) {
Preconditions.checkArgument(value != null);
return value.toString();
}
}

private static boolean hasAuthAnnotation(Annotation[] annotations) {
for (Annotation annotation : annotations) {
if (annotation.annotationType() == CookieParam.class) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
if (annotation.annotationType() == CookieParam.class) {
if (CookieParam.class.equals(annotation.annotationType())) {

return true;
}
}

return false;
}
}
6 changes: 5 additions & 1 deletion ...ey-server/src/main/java/com/palantir/conjure/java/server/jersey/ConjureJerseyFeature.java
View file
Open in desktop
Expand Up @@ -53,6 +53,10 @@ public boolean configure(FeatureContext context) {
// Cbor handling
context.register(new JacksonCBORProvider(ObjectMappers.newCborServerObjectMapper()));

// Auth handling
context.register(AuthHeaderParamConverterProvider.class);
context.register(BearerTokenParamConverterProvider.class);

// Optional handling
context.register(GuavaOptionalMessageBodyWriter.class);
context.register(GuavaOptionalParamConverterProvider.class);
Expand All @@ -65,7 +69,7 @@ public boolean configure(FeatureContext context) {
context.register(Java8OptionalLongMessageBodyWriter.class);
context.register(Java8OptionalLongParamConverterProvider.class);

// DateTime
// DateTime handling
context.register(InstantParamConverterProvider.class);
context.register(ZonedDateTimeParamConverterProvider.class);
context.register(OffsetDateTimeParamConverterProvider.class);
Expand Down
2 changes: 2 additions & 0 deletions ...ain/java/com/palantir/conjure/java/server/jersey/GuavaOptionalParamConverterProvider.java
View file
Open in desktop
Expand Up @@ -23,11 +23,13 @@
import javax.ws.rs.ext.ParamConverter;
import javax.ws.rs.ext.ParamConverterProvider;
import javax.ws.rs.ext.Provider;
import org.glassfish.jersey.internal.inject.Custom;
import org.glassfish.jersey.internal.inject.InjectionManager;
import org.glassfish.jersey.internal.inject.Providers;
import org.glassfish.jersey.internal.util.ReflectionHelper;
import org.glassfish.jersey.internal.util.collection.ClassTypePair;

@Custom
Copy link
Member Author

Choose a reason for hiding this comment

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

This annotation ensures that our custom param converters are considered first.
Otherwise the order in which the param converter providers are considered is arbitrary:
https://github.com/jersey/jersey/blob/2.25.1/core-server/src/main/java/org/glassfish/jersey/server/internal/inject/ParamConverterFactory.java#L58-L101

For example, here is an order that was used when I was debugging tests locally:
param-converter-providers

This is especially important for the AuthHeaderParamConverterProvider and BearerTokenParamConverterProvider to ensure that we don't fallback to the built-in AggregatedProvider which contains the TypeValueOf param converter (which is used currently for AuthHeader and BearerToken).

Copy link
Contributor

Choose a reason for hiding this comment

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

Something along these lines would make an excellent comment in the code :-)

@Provider
public final class GuavaOptionalParamConverterProvider implements ParamConverterProvider {
private final InjectionManager injectionManager;
Expand Down
2 changes: 2 additions & 0 deletions .../src/main/java/com/palantir/conjure/java/server/jersey/InstantParamConverterProvider.java
View file
Open in desktop
Expand Up @@ -23,7 +23,9 @@
import javax.ws.rs.ext.ParamConverter;
import javax.ws.rs.ext.ParamConverterProvider;
import javax.ws.rs.ext.Provider;
import org.glassfish.jersey.internal.inject.Custom;

@Custom
@Provider
public final class InstantParamConverterProvider implements ParamConverterProvider {
private final InstantParamConverter paramConverter = new InstantParamConverter();
Expand Down
2 changes: 2 additions & 0 deletions ...va/com/palantir/conjure/java/server/jersey/Java8OptionalDoubleParamConverterProvider.java
View file
Open in desktop
Expand Up @@ -26,7 +26,9 @@
import javax.ws.rs.ext.ParamConverter;
import javax.ws.rs.ext.ParamConverterProvider;
import javax.ws.rs.ext.Provider;
import org.glassfish.jersey.internal.inject.Custom;

@Custom
@Provider
public final class Java8OptionalDoubleParamConverterProvider implements ParamConverterProvider {
private final OptionalDoubleParamConverter paramConverter = new OptionalDoubleParamConverter();
Expand Down
2 changes: 2 additions & 0 deletions .../java/com/palantir/conjure/java/server/jersey/Java8OptionalIntParamConverterProvider.java
View file
Open in desktop
Expand Up @@ -26,7 +26,9 @@
import javax.ws.rs.ext.ParamConverter;
import javax.ws.rs.ext.ParamConverterProvider;
import javax.ws.rs.ext.Provider;
import org.glassfish.jersey.internal.inject.Custom;

@Custom
@Provider
public final class Java8OptionalIntParamConverterProvider implements ParamConverterProvider {
private final OptionalIntParamConverter paramConverter = new OptionalIntParamConverter();
Expand Down
2 changes: 2 additions & 0 deletions ...java/com/palantir/conjure/java/server/jersey/Java8OptionalLongParamConverterProvider.java
View file
Open in desktop
Expand Up @@ -26,7 +26,9 @@
import javax.ws.rs.ext.ParamConverter;
import javax.ws.rs.ext.ParamConverterProvider;
import javax.ws.rs.ext.Provider;
import org.glassfish.jersey.internal.inject.Custom;

@Custom
@Provider
public final class Java8OptionalLongParamConverterProvider implements ParamConverterProvider {
private final OptionalLongParamConverter paramConverter = new OptionalLongParamConverter();
Expand Down
2 changes: 2 additions & 0 deletions ...ain/java/com/palantir/conjure/java/server/jersey/Java8OptionalParamConverterProvider.java
View file
Open in desktop
Expand Up @@ -24,11 +24,13 @@
import javax.ws.rs.ext.ParamConverter;
import javax.ws.rs.ext.ParamConverterProvider;
import javax.ws.rs.ext.Provider;
import org.glassfish.jersey.internal.inject.Custom;
import org.glassfish.jersey.internal.inject.InjectionManager;
import org.glassfish.jersey.internal.inject.Providers;
import org.glassfish.jersey.internal.util.ReflectionHelper;
import org.glassfish.jersey.internal.util.collection.ClassTypePair;

@Custom
@Provider
public final class Java8OptionalParamConverterProvider implements ParamConverterProvider {
private final InjectionManager injectionManager;
Expand Down
2 changes: 2 additions & 0 deletions ...in/java/com/palantir/conjure/java/server/jersey/OffsetDateTimeParamConverterProvider.java
View file
Open in desktop
Expand Up @@ -23,7 +23,9 @@
import javax.ws.rs.ext.ParamConverter;
import javax.ws.rs.ext.ParamConverterProvider;
import javax.ws.rs.ext.Provider;
import org.glassfish.jersey.internal.inject.Custom;

@Custom
@Provider
public final class OffsetDateTimeParamConverterProvider implements ParamConverterProvider {
private final OffsetDateTimeParamConverter paramConverter = new OffsetDateTimeParamConverter();
Expand Down
55 changes: 55 additions & 0 deletions ...y-server/src/main/java/com/palantir/conjure/java/server/jersey/UnauthorizedException.java
View file
Open in desktop
@@ -0,0 +1,55 @@
/*
* (c) Copyright 2019 Palantir Technologies Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.palantir.conjure.java.server.jersey;

import com.palantir.conjure.java.api.errors.ErrorType;
import javax.ws.rs.WebApplicationException;
import javax.ws.rs.core.Response.Status;

final class UnauthorizedException extends WebApplicationException {

private static final ErrorType MISSING_CREDENTIALS_ERROR_TYPE = ErrorType.create(
ErrorType.Code.UNAUTHORIZED,
"Conjure:MissingCredentials");
private static final ErrorType MALFORMED_CREDENTIALS_ERROR_TYPE = ErrorType.create(
ErrorType.Code.UNAUTHORIZED,
"Conjure:MalformedCredentials");

private final ErrorType errorType;

static UnauthorizedException missingCredentials() {
return new UnauthorizedException(MISSING_CREDENTIALS_ERROR_TYPE);
}

static UnauthorizedException malformedCredentials(Throwable throwable) {
return new UnauthorizedException(MALFORMED_CREDENTIALS_ERROR_TYPE, throwable);
}

private UnauthorizedException(ErrorType errorType) {
super(Status.UNAUTHORIZED);
this.errorType = errorType;
}

private UnauthorizedException(ErrorType errorType, Throwable throwable) {
super(throwable, Status.UNAUTHORIZED);
this.errorType = errorType;
}

public ErrorType getErrorType() {
return errorType;
}
}
11 changes: 10 additions & 1 deletion .../src/main/java/com/palantir/conjure/java/server/jersey/WebApplicationExceptionMapper.java
View file
Open in desktop
Expand Up @@ -22,6 +22,7 @@
import java.util.UUID;
import javax.ws.rs.BadRequestException;
import javax.ws.rs.ForbiddenException;
import javax.ws.rs.NotAuthorizedException;
import javax.ws.rs.NotFoundException;
import javax.ws.rs.WebApplicationException;
import javax.ws.rs.core.Response;
Expand Down Expand Up @@ -54,7 +55,15 @@ public Response toResponse(WebApplicationException exception) {
log.error("Error handling request", SafeArg.of("errorInstanceId", errorInstanceId), exception);
}

if (exception instanceof ForbiddenException) {
if (exception instanceof NotAuthorizedException) {
return JsonExceptionMapper.createResponse(
ErrorType.UNAUTHORIZED,
errorInstanceId);
} else if (exception instanceof UnauthorizedException) {
return JsonExceptionMapper.createResponse(
((UnauthorizedException) exception).getErrorType(),
errorInstanceId);
} else if (exception instanceof ForbiddenException) {
return JsonExceptionMapper.createResponse(
ErrorType.PERMISSION_DENIED,
errorInstanceId);
Expand Down