Skip to content
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

OAuth2 Auth provider incorrectly validating Access Tokens #673

Closed

Conversation

fposch
Copy link

@fposch fposch commented Feb 8, 2024

Motivation:

PR containing code for fix in #654

@vietj
Copy link
Member

vietj commented Feb 9, 2024

I can't see any test for this

@vietj
Copy link
Member

vietj commented Feb 9, 2024

@pmlopes could you have a look ?

@fposch
Copy link
Author

fposch commented Feb 9, 2024

I can't see any test for this

I didn't write a test because I couldn't see any for the provider. A full maven build for the whole vertx-auth module passes locally but that's as far as I ran tests locally.

@fposch fposch force-pushed the 654-incorrectly-token-validation branch from 4e60d23 to d1de398 Compare February 21, 2024 08:46
@fposch
Copy link
Author

fposch commented Feb 21, 2024

Test issues partially fixed, reusing the audience ArrayList from JWTOptions did unintendedly modify its content, using a copy now.

@fposch
Copy link
Author

fposch commented Feb 21, 2024

Now only the following two tests are failing in OAuth2Keycloak14IT

  • discoverGetTokenFromFrontEndPerformAuthWithBackend
  • discoverGetTokenFromFrontEndPerformAuthWithBorkendWillFail

Tests are expecting validation to fail but that isn't happening with the suggested ID-token-only validation. I don't want to tamper with existing tests too much, so your opinion on this one would be appreciated.

@fposch
Copy link
Author

fposch commented Mar 5, 2024

@pmlopes @vietj May I ask for an update?

My recent changes to suggested fix from #654 did resolve some of the regression but the ones are broken with new logic only validating ID-tokens. Writing a meaningful test is above my paygrade and I wouldn't dare to just remove the test cases.

Let me know what you think,

@fposch
Copy link
Author

fposch commented Jul 23, 2024

There hasn't been any reply, neither on the related issue nor here for months and I'm not sure how else to address this.

@fposch
Copy link
Author

fposch commented Oct 9, 2024

We decided to tackle this issue by this issue by managing audiences in our application in a controlled manner, so no need to fix token handling from our perspective -> closing the PR and leave it to the creator of the related issue how to deal with it.

See also #654

@fposch fposch closed this Oct 9, 2024
@fposch fposch deleted the 654-incorrectly-token-validation branch October 9, 2024 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants