Add param converters for authentication credentials #1374

pkoenig10 · 2019-12-12T23:45:26Z

Before this PR

Malformed authentication credentials will result in a 400. This is unfortunate because it will propagate as a 500.

After this PR

Malformed authentication credentials will result in a 401.

This change is the equivalent to part of palantir/conjure-java#656 for Jersey servers.

changelog-app · 2019-12-12T23:45:31Z

Generate changelog in `changelog/@unreleased`

Type

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.

Check the box to generate changelog(s)

Generate changelog entry

pkoenig10 · 2019-12-12T23:49:09Z

.../src/main/java/com/palantir/conjure/java/server/jersey/AuthHeaderParamConverterProvider.java

+        @Override
+        public AuthHeader fromString(final String value) {
+            if (value == null) {
+                throw new UnauthorizedException(MISSING_CREDENTIAL_ERROR_TYPE);


This needs to be a WebApplicationException. Otherwise it will get wrapped in a ExtractorException which will get mapped to a 400:
https://github.com/jersey/jersey/blob/2.25.1/core-server/src/main/java/org/glassfish/jersey/server/internal/inject/AbstractParamValueExtractor.java#L137

The jaxrs NotAuthorizedException doesn't provide constructors that can be used in this scenario so we need to create our own. I chose the name UnauthorizedException to disambiguate.

pkoenig10 · 2019-12-12T23:53:15Z

...c/main/java/com/palantir/conjure/java/server/jersey/GuavaOptionalParamConverterProvider.java

 import org.glassfish.jersey.internal.inject.Providers;
 import org.glassfish.jersey.internal.util.ReflectionHelper;
 import org.glassfish.jersey.internal.util.collection.ClassTypePair;

+@Custom


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:

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).

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

carterkozak · 2019-12-13T15:45:47Z

...src/main/java/com/palantir/conjure/java/server/jersey/BearerTokenParamConverterProvider.java

+
+@Custom
+@Provider
+public final class BearerTokenParamConverterProvider implements ParamConverterProvider {


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.

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.

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

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.

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.

.../src/main/java/com/palantir/conjure/java/server/jersey/AuthHeaderParamConverterProvider.java

stale · 2020-01-01T15:53:06Z

This PR has been automatically marked as stale because it has not been touched in the last 14 days. If you'd like to keep it open, please leave a comment or add the 'long-lived' label, otherwise it'll be closed in 7 days.

pkoenig10 · 2020-01-02T16:52:49Z

@carterkozak are there any other changes we should make in this PR to try to gracefully handle applications that rely on tokens being nullable?

carterkozak · 2020-01-02T16:56:13Z

Let's add test coverage to make sure this change doesn't modify null token behavior, this way we can decouple the risky piece from the type of exception thrown from our infrastructure. We can hold off on null handling until the office opens up and we can discuss more broadly :-)

carterkozak

Generally looks good to me, thanks! Just a couple small nits

carterkozak · 2020-01-06T18:18:19Z

...src/main/java/com/palantir/conjure/java/server/jersey/BearerTokenParamConverterProvider.java

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


nit:

Suggested change

if (annotation.annotationType() == CookieParam.class) {

if (CookieParam.class.equals(annotation.annotationType())) {

carterkozak · 2020-01-06T18:18:52Z

.../src/main/java/com/palantir/conjure/java/server/jersey/AuthHeaderParamConverterProvider.java

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


nit:

Suggested change

if (annotation.annotationType() == HeaderParam.class) {

if (HeaderParam.class.equals(annotation.annotationType())) {

carterkozak · 2020-01-06T18:19:52Z

.../src/main/java/com/palantir/conjure/java/server/jersey/AuthHeaderParamConverterProvider.java

+        for (Annotation annotation : annotations) {
+            if (annotation.annotationType() == HeaderParam.class) {
+                String value = ((HeaderParam) annotation).value();
+                if (value.equals(HttpHeaders.AUTHORIZATION)) {


nit, safer around nulls:

Suggested change

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

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

carterkozak · 2020-01-06T18:20:32Z

...c/main/java/com/palantir/conjure/java/server/jersey/GuavaOptionalParamConverterProvider.java

 import org.glassfish.jersey.internal.inject.Providers;
 import org.glassfish.jersey.internal.util.ReflectionHelper;
 import org.glassfish.jersey.internal.util.collection.ClassTypePair;

+@Custom


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

carterkozak · 2020-01-06T19:11:58Z

Thanks! 👍

svc-autorelease · 2020-01-06T19:15:14Z

Released 4.47.0

pkoenig10 added the autorelease label Dec 12, 2019

policy-bot bot requested a review from dansanduleac December 12, 2019 23:45

pkoenig10 commented Dec 12, 2019

View reviewed changes

pkoenig10 changed the title ~~Add param converters for authentication~~ Add param converters for authentication credentials Dec 12, 2019

pkoenig10 force-pushed the pkoenig10/authParamConverter branch 3 times, most recently from 8fea8cd to 17ea493 Compare December 13, 2019 00:48

carterkozak reviewed Dec 13, 2019

View reviewed changes

pkoenig10 force-pushed the pkoenig10/authParamConverter branch from 817a86b to fec09ff Compare December 13, 2019 17:47

pkoenig10 added 2 commits December 16, 2019 09:58

Add param converters for authentication

d59a161

Add generated changelog entries

1040dde

pkoenig10 force-pushed the pkoenig10/authParamConverter branch from fec09ff to 1040dde Compare December 16, 2019 17:58

Limit to Conjure auth parameters

b52caa3

pkoenig10 force-pushed the pkoenig10/authParamConverter branch from 9124eae to b52caa3 Compare December 18, 2019 06:57

pkoenig10 mentioned this pull request Dec 18, 2019

Throw typed exception for malformed tokens palantir/auth-tokens#228

Closed

carterkozak reviewed Dec 18, 2019

View reviewed changes

.../src/main/java/com/palantir/conjure/java/server/jersey/AuthHeaderParamConverterProvider.java Show resolved Hide resolved

stale bot added the stale label Jan 1, 2020

stale bot removed the stale label Jan 2, 2020

Don't return 401 for null authentication credentials

6a3dd09

carterkozak reviewed Jan 6, 2020

View reviewed changes

Comments

a303b52

carterkozak added the merge when ready label Jan 6, 2020

bulldozer-bot bot merged commit 1ea97f0 into develop Jan 6, 2020

bulldozer-bot bot deleted the pkoenig10/authParamConverter branch January 6, 2020 19:15

pkoenig10 mentioned this pull request Jan 6, 2020

Respond with 401 when missing credentials #1389

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add param converters for authentication credentials #1374

Add param converters for authentication credentials #1374

pkoenig10 commented Dec 12, 2019 •

edited

changelog-app bot commented Dec 12, 2019 •

edited by pkoenig10

pkoenig10 Dec 12, 2019

pkoenig10 Dec 12, 2019

carterkozak Jan 6, 2020

carterkozak Dec 13, 2019

pkoenig10 Dec 13, 2019

pkoenig10 Dec 18, 2019 •

edited

carterkozak Dec 18, 2019

pkoenig10 Dec 18, 2019 •

edited

stale bot commented Jan 1, 2020

pkoenig10 commented Jan 2, 2020

carterkozak commented Jan 2, 2020

carterkozak left a comment

carterkozak Jan 6, 2020

carterkozak Jan 6, 2020

carterkozak Jan 6, 2020

carterkozak Jan 6, 2020

carterkozak commented Jan 6, 2020

svc-autorelease commented Jan 6, 2020

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

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

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

Add param converters for authentication credentials #1374

Add param converters for authentication credentials #1374

Conversation

pkoenig10 commented Dec 12, 2019 • edited

Before this PR

After this PR

changelog-app bot commented Dec 12, 2019 • edited by pkoenig10

Generate changelog in changelog/@unreleased

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkoenig10 Dec 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkoenig10 Dec 18, 2019 • edited

Choose a reason for hiding this comment

stale bot commented Jan 1, 2020

pkoenig10 commented Jan 2, 2020

carterkozak commented Jan 2, 2020

carterkozak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carterkozak commented Jan 6, 2020

svc-autorelease commented Jan 6, 2020

pkoenig10 commented Dec 12, 2019 •

edited

changelog-app bot commented Dec 12, 2019 •

edited by pkoenig10

Generate changelog in `changelog/@unreleased`

pkoenig10 Dec 18, 2019 •

edited

pkoenig10 Dec 18, 2019 •

edited