token_type_hint should be used as a hint only

[rozagerardo]

Issue #175
Added support to find an OAuth2Authorization based on a token, and using the TokenType parameter only as a hint instead of as a hard constraint.

[rozagerardo]

The TokenType is a hint; that means it will simply indicate with which supported token type we should start

[jgrandja]

Thanks for the PR @rozagerardo. Please see review comment.

[jgrandja]

Please remove findByTokenWithHint() and modify findByToken() instead.

[rozagerardo]

Just to confirm we're on the same page here @jgrandja :

As you surely know, the findByToken() method is used in several other places apart from the classes involved in this endpoint (OAuth2AuthorizationCodeAuthenticationProvider, OAuth2ClientAuthenticationProvider, OAuth2RefreshTokenAuthenticationProvider, OAuth2AuthorizationEndpointFilter).

We probably don't want to change the behavior for these cases, so I'll need to add a boolean parameter to the findByToken() method, in order to indicate if we want the TokenType param to be used just as a hint, or as a strict constraint in the search.

I know it's not critical, but if so, should we also overload the method to make this parameter 'optional' (so that we don't have to change the existing method invocations)?

[jgrandja]

We probably don't want to change the behavior for these cases

Agreed

so I'll need to add a boolean parameter to the findByToken() method, in order to indicate if we want the TokenType param to be used just as a hint, or as a strict constraint in the search

I would prefer to not add an extra parameter or overload the operation. I'd prefer to keep it simple. Given that TokenType is @Nullable, I'm tempted to completely ignore the token_type_hint request parameter and pass in null for the revocation (and introspection) flows. I think this will work. Can you please give this a try.

[rozagerardo]

Sure, that would work, and the spec indicates this is a valid approach as well @jgrandja :

token_type_hint OPTIONAL. [...] An authorization server MAY ignore
this parameter, particularly if it is able to detect the
token type automatically.

Just a quick side note; IMO it would still make sense to define a way (that is, a method) to allow using the parameter as a hint (as part of the OAuth2AuthorizationService interface), and then rely on the implementation to decide how to implement that; for example, the in-memory implementation might simply call the findByToken with a null value (because the hint doesn't really represent a critical improvement in this case), a db implementation might want to benefit from this possibility.

Having said this, I don't think this is a critical decision (at least not at this point); I think ignoring the token_type_hint is ok, we can decide later on if it makes sense to actually support it.

Thus, I now made the changes, let me know if there is something else I should work on :)

BTW the build failed (for just one check), but it doesn't seem related to my change (in fact, the last commit contained just a minor formatting change):

Could not resolve org.springframework:spring-expression:5.3.1.
> Could not get resource 'https://repo.spring.io/libs-milestone/org/springframework/spring-expression/5.3.1/spring-expression-5.3.1.pom'.
> Could not GET 'https://repo.spring.io/libs-milestone/org/springframework/spring-expression/5.3.1/spring-expression-5.3.1.pom'.
> repo.spring.io

[jgrandja]

we can decide later on if it makes sense to actually support it

Agreed

[jgrandja]

Thanks for the updates @rozagerardo ! This is now in master.