Skip to content

Enable customization of BearerTokenResolver by adding a setter for JwtClaimIssuerConverter on JwtIssuerAuthenticationManagerResolver #9168

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

Closed
wants to merge 5 commits into from

Conversation

arvidOtt
Copy link
Contributor

Description
When using the JwtIssuerAuthenticationManagerResolver there should be a way to replace the DefaultBearerTokenResolver (or here the entire JwtClaimIssuerConverter) by a custom implementation which is requested in #8535.

I decided to set the entire JwtClaimIssuerConverter because it is declared as type Converter<HttpServletRequest,String> and implementing a setter or new constructor on it would required to change the type of the attribute directly to JwtClaimIssuerConverter class.

Context
I use the JwtIssuerAuthenticationManagerResolver in a web socket backend which receives the JWT not in the Authorization Header but in a custom X-Authorization Cookie Header. Therefore, I need to use my own BearerTokenResolver implementation which gets the JWT from there. This seems not to be possible at the moment.

… BearerTokenResolver in the JwtIssuerAuthenticationManagerResolver class (spring-projectsgh-8535)
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 29, 2020
@arvidOtt
Copy link
Contributor Author

arvidOtt commented Oct 29, 2020

@jzheaux this is basically a slim version of #9005. I hope one of them can be merged soon.

Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

Thanks, @arvidOtt! I've left some feedback inline.

Also, would you please add unit tests to demonstrate that the feature works?

*
* @since 5.5
*/
public void setIssuerConverter(Converter<HttpServletRequest, String> issuerConverter) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of setIssuerConverter, could you do:

public void setBearerTokenResolver(BearerTokenResolver bearerTokenResolver) {
    this.issuerConverter = new JwtClaimIssuerConverter(bearerTokenResolver);
}

That way, the API uses a familiar interface that they most likely already configured in their app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay :) Then I would also need to add another constructor to the JwtClaimIssuerConverter to pass the resolver / converter correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jzheaux I hope this is the way your feedback was meant.

Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, @arvidOtt! I've left some additional feedback inline.

@@ -130,9 +130,28 @@ public AuthenticationManager resolve(HttpServletRequest request) {
return authenticationManager;
}

/**
* Set a custom issuer converter
Copy link
Contributor

Choose a reason for hiding this comment

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

Will you please update the JavaDoc so that it talks about BearerTokenResolver instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

private BearerTokenResolver resolver;

JwtClaimIssuerConverter() {
this.resolver = new DefaultBearerTokenResolver();
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be more efficient to do:

this(new DefaulBearerTokenResolver());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

…icationManagerResolver; reuse contructors of JwtAuthenticationManagerResolvers
@arvidOtt
Copy link
Contributor Author

@jzheaux I adapted the PR according to your feedback. Will write some test cases for the setter method next 🧪

…olver, JwtIssuerReactiveAuthenticationManagerResolver
@arvidOtt
Copy link
Contributor Author

arvidOtt commented Nov 2, 2020

Added unit tests for the new feature.

@jzheaux jzheaux self-assigned this Nov 3, 2020
@jzheaux jzheaux added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 3, 2020
@jzheaux
Copy link
Contributor

jzheaux commented Nov 3, 2020

Thanks, @arvidOtt! This is now merged into master via d0d655e.

@jzheaux jzheaux closed this Nov 3, 2020
@jzheaux jzheaux added this to the 5.5.0-M1 milestone Nov 3, 2020
@arvidOtt
Copy link
Contributor Author

arvidOtt commented Nov 3, 2020

@jzheaux thank you for your support!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants