Skip to content

Add BearerTokenAuthenticationConverter #8975

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 1 commit into from

Conversation

thecodinglog
Copy link
Contributor

BearerTokenAuthenticationConverter is introduced to solve the problem of not being able to change AuthenticationDetailsSource.

Closes gh-8840

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 PR, @thecodinglog! I've left some feedback inline.

Also, would you be able to update OAuth2ResourceServerConfigurer to use setAuthenticationConverter instead of BearerTokenResolver?

@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 Sep 22, 2020
@jzheaux jzheaux added this to the 5.5.0-M1 milestone Sep 22, 2020
@thecodinglog thecodinglog force-pushed the gh-8840 branch 3 times, most recently from 9a21650 to c5f7982 Compare September 29, 2020 01:35
@jgrandja jgrandja modified the milestones: 5.5.0-M1, 5.5.0-M2 Nov 3, 2020
@jzheaux jzheaux modified the milestones: 5.5.0-M2, 5.5.0-M3 Feb 11, 2021
@benba
Copy link
Contributor

benba commented Feb 25, 2021

Thanks for this PR @thecodinglog , I needed exactly this to implement a feature that also needs to add additional information into the details.

I've tried to use it on the current Spring Security version while waiting for 5.5.0, and I was wandering if it wouldn't be easier if BearerTokenAuthenticationConverter was an interface that extends AuthenticationConverter and define setBearerTokenResolver and setAuthenticationDetailsSource methods.
The implementation would then be into something like DefaultBearerTokenAuthenticationConverter.

The reason I asked this because today, you are forced to subclass BearerTokenAuthenticationConverter to be able to customize BearerTokenAuthenticationFilter (since the BearerTokenAuthenticationFilter.setAuthenticationConverter takes a BearerTokenAuthenticationConverter), and one may need a completely different implementation.

Another option could be to use a bare AuthenticationConverter and handle the backward compatibility in BearerTokenAuthenticationFilter.setBearerTokenResolver with a cast to BearerTokenAuthenticationConverter if the authenticationConverter is of this type.

@thecodinglog
Copy link
Contributor Author

Thanks for such a good opinion @benba .

This PR focused on being able to change the AuthenticationDetailsSource in BearerTokenAuthenticationFilter.
Adding a setter to the BearerTokenAuthenticationFilter is easiest way but there was an opinion that it would be better to make the implementation similar to other AuthenticationConverter implementations. I agreed that and I did like that.

I agree that BearerTokenAuthenticationConverter is more flexible if it is an interface, but I disagree to create an interface where the interface's methods are only setters. Because it doesn't clearly explain what the interface does.

If you need an interface for the setter It would be better to use BearerTokenAuthenticationConverter in combination with interfaces such as AuthenticationConverter and BearerTokenResolverReplacable. But this seems too verbose.

As you mentioned, you may need a completely different implementation for BearerTokenAuthenticationConverter.
For this, I don't like it too, but I didn't make it a final class so that it could be inherited.

I'm not sure I understand the last opinion which you mentioned but you mean that do the setBearerTokenResolver of the BearerTokenAuthenticationFilter class as below?

public void setBearerTokenResolver(BearerTokenResolver bearerTokenResolver) {
    if(bearerTokenResolver instanceof BearerTokenAuthenticationConverter)
        this.authenticationConverter = (BearerTokenAuthenticationConverter)bearerTokenResolver;
    else
        this.authenticationConverter.setBearerTokenResolver(bearerTokenResolver);
}

If so, Since BearerTokenResolver and AuthenticationConverter are interfaces that have completely different functions, but I think that it is not intuitive from the user's point of view.
In order to use the above method, I think the users might carefully read the related documentation or see the implementation to get a sense of how to use it.

Thanks again!😃

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 again, @thecodinglog, and my apologies for the delay in responding. I've left some additional feedback inline.

Also, as a housekeeping item, will you please update the copyright headers in the files in this PR to have an end date of 2021? Thanks!

BearerTokenAuthenticationConverter is introduced to solve the problem of not being able to change AuthenticationDetailsSource.
BearerTokenAuthenticationFilter delegates to BearerTokenAuthenticationConverter the task of creating BearerTokenAuthenticationToken and setting AuthenticationDetailsSource.
BearerTokenAuthenticationConverter is customizable and the customized converter can be used in BearerTokenAuthenticationFilter.

Closes spring-projectsgh-8840
@jzheaux
Copy link
Contributor

jzheaux commented Mar 12, 2021

Thanks, @thecodinglog! This is now merged into master via 31f310f.

I also added a polish via b774e91 that simplified a method name and rearranged some of the methods and fields.

@jzheaux jzheaux closed this Mar 12, 2021
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.

Consider providing setter of authenticationDetailsSource field in BearerTokenAuthenticationFilter
5 participants