Fix oauth2 and jwt authentificaor interference#13662
Fix oauth2 and jwt authentificaor interference#13662Praveen2112 merged 1 commit intotrinodb:masterfrom
Conversation
s2lomon
force-pushed
the
fix-oauth2-jwt-interference
branch
from
699e01d to
621112a
Compare
There was a problem hiding this comment.
Do we want to just catch java.lang.IllegalArgumentException for now?
There was a problem hiding this comment.
Can we narrow down the catching ? What if the token fails because it is damaged or expired ?
There was a problem hiding this comment.
Ok so this catch is so wide on purpose. Right now there are plenty of reasons why token deserialization might fail. We might be having invalid token (on purpose - like in this bug scenario), token might have expired - meaning that both access token and refresh token are either expired or soon to be expired, encryption keys might have been rolled out (after restart) etc. All of the above suggests that we need to discard the token, but we should not request any specific handling of these errors. Rather I think we should just discard this token and issue another challenge to the client - so business as usual. I could narrow catching to IllegalArgumentException (with allowing the possibility that some other valid case might not be handled atm), but then I would like to add this to official contract of the TokenPairSerializer interface as a throws. Catching implementation specific exceptions is not a best design decision.
andrewdibiasio6
left a comment
andrewdibiasio6
left a comment
There was a problem hiding this comment.
LGTM! Thanks for the fix.
core/trino-main/src/main/java/io/trino/server/security/oauth2/OAuth2Authenticator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Can we narrow down the catching ? What if the token fails because it is damaged or expired ?
There was a problem hiding this comment.
There is a typo in the commit message.
user couldn't log in by using standard jwt token.
Can you please explain why in the commit message too?
core/trino-main/src/main/java/io/trino/server/security/oauth2/OAuth2Authenticator.java
Outdated
Show resolved
Hide resolved
|
Looks good but test is definitely needed. Please take a look at: |
s2lomon
force-pushed
the
fix-oauth2-jwt-interference
branch
2 times, most recently
from
11eb861 to
83b5471
Compare
s2lomon
force-pushed
the
fix-oauth2-jwt-interference
branch
from
83b5471 to
734cc1e
Compare
|
@s2lomon the build is red |
s2lomon
force-pushed
the
fix-oauth2-jwt-interference
branch
from
734cc1e to
67eb882
Compare
There was a problem hiding this comment.
Can test with the config that causes the issue i.e http-server.authentication.type=oauth2,JWT ?
Before this fix, when oauth2 with refresh token is enabled along with jwt authenticator, user couldn't log in by using standard jwt token. It was occuring due to incorect handling of tokens that are in different format than the one issued by the OAuth2 implementation to store refresh tokens. After the fix in such case OAuth2 will just issue another challenge to the client.
s2lomon
force-pushed
the
fix-oauth2-jwt-interference
branch
from
67eb882 to
7604166
Compare
5 participants
Description
Beore this fix, when oauth2 with refresh token is enabled
along with jwt authenticator, user couldn't log in
by using standard jwt token. After the fix, every attempt of sending
token that can't be consumed by oauth2 authenticator will result in
standard AuthenticationError, that plays along the contract of Authentificators
Fixes #13575
OAuth2 Authentification - core
Brings back possibility of using both OAuth2 and jwt at the same time.
Related issues, pull requests, and links
Documentation
(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.
Release notes
( ) No release notes entries required.
(x) Release notes entries required with the following suggested text: