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

Do the correct challenge when more than one auth mechanism is used #16446

Merged

Conversation

sberyozkin
Copy link
Member

Fixes #13363

I started working on this PR months ago but only now completing it.
This PR does the following:

  • tries to use the mechanism with the matching HTTP Authorization scheme - both for the challenge (to fix this actual issue) and the attempted authentication.
  • Adds the tests and also removes the unused BasicAuthenticationMechanism code and deprecates the constructors accepting the mechanism name - as its code is written in terms of basic scheme.

@quarkus-bot
Copy link

quarkus-bot bot commented Apr 12, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building bfa1383

Status Name Step Test failures Logs Raw logs
Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs

@sberyozkin
Copy link
Member Author

Caused by: org.apache.maven.wagon.authorization.AuthorizationException: Authorization failed for https://dl.bintray.com/davided/artifacts/org/hibernate/reactive/hibernate-reactive-core/1.0.0.CR1.Vertx4/hibernate-reactive-core-1.0.0.CR1.Vertx4.pom 403 Forbidden

Not sure why it is happening

@famod
Copy link
Member

famod commented Apr 12, 2021

@sberyozkin
Copy link
Member Author

@famod thanks...

Copy link
Member

@stuartwdouglas stuartwdouglas left a comment

Choose a reason for hiding this comment

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

LGTM.

Ideally we should also have some way of sorting these for the case where there is no challenge. I think at the moment the order is random.

@gsmet gsmet force-pushed the select_matching_auth_mechanism branch from bfa1383 to 5a7e88d Compare April 12, 2021 23:28
@gsmet
Copy link
Member

gsmet commented Apr 12, 2021

Rebased to get the latest CI fixes.

@quarkus-bot
Copy link

quarkus-bot bot commented Apr 13, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 5a7e88d

Status Name Step Test failures Logs Raw logs
JVM Tests - JDK 11 Build Test failures Logs Raw logs
✔️ JVM Tests - JDK 11 Windows
JVM Tests - JDK 15 Build Test failures Logs Raw logs
JVM Tests - JDK 8 Build Test failures Logs Raw logs
MicroProfile TCKs Tests Verify with Maven Test failures Logs Raw logs
Native Tests - Messaging Build ⚠️ Check → Logs Raw logs
Native Tests - Security2 Build Test failures Logs Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 integration-tests/oidc

io.quarkus.it.keycloak.BearerTokenAuthorizationTest.testVerificationFailedNoBearerToken line 141 - More details - Source on GitHub


⚙️ JVM Tests - JDK 15 #

📦 integration-tests/kafka

io.quarkus.it.kafka.SaslKafkaConsumerTest.testReception line 48 - More details - Source on GitHub

io.quarkus.it.kafka.SslKafkaConsumerTest.testReception line 56 - More details - Source on GitHub

📦 integration-tests/oidc

io.quarkus.it.keycloak.BearerTokenAuthorizationTest.testVerificationFailedNoBearerToken line 141 - More details - Source on GitHub


⚙️ JVM Tests - JDK 8 #

📦 integration-tests/oidc

io.quarkus.it.keycloak.BearerTokenAuthorizationTest.testVerificationFailedNoBearerToken line 141 - More details - Source on GitHub


⚙️ MicroProfile TCKs Tests #

📦 tcks/tcks/microprofile-fault-tolerance

org.eclipse.microprofile.fault.tolerance.tck.CircuitBreakerRetryTest.testCircuitOpenWithMultiTimeoutsAsync line 487 - More details - Source on GitHub


⚙️ Native Tests - Security2 #

📦 integration-tests/oidc

io.quarkus.it.keycloak.BearerTokenAuthorizationInGraalITCase.testVerificationFailedNoBearerToken - More details - Source on GitHub

@gsmet
Copy link
Member

gsmet commented Apr 13, 2021

@sberyozkin the BearerTokenAuthorizationTest.testVerificationFailedNoBearerToken test failure looks suspicious, could you have a look?

@gsmet
Copy link
Member

gsmet commented Apr 13, 2021

java.lang.AssertionError: 
1 expectation failed.
Expected header "WWW-Authenticate" was not "Bearer", was "null". Headers are:
Content-Length=0

@gsmet
Copy link
Member

gsmet commented Apr 13, 2021

@sberyozkin what I don't understand is how the test is passing on Windows given it fails consistently on Linux. Could you have a look at what's going on because this is very weird.

@sberyozkin
Copy link
Member Author

Hmm, io.quarkus.it.keycloak.BearerTokenAuthorizationTest.testVerificationFailedNoBearerToken - I missed it, sorry, was focusing on making the form test pass - as this one was definitely passing the last time I looked at it - will look into it

@gsmet
Copy link
Member

gsmet commented Apr 13, 2021

The mystery is how this thing passed on Windows.

@sberyozkin
Copy link
Member Author

sberyozkin commented Apr 13, 2021

Hi Guillaume @gsmet It is my fault - this test in fact could never have been passed after all - I was looking at this PR awhile back and I was just under impression yesterday that all was good and then only test which had to be fixed was the form/basic auth test (I recall I could not make it work at a time - .preemptive() qualifier had to be added) - while obviously I only typed this Bearer auth test to express the intention (looks like I was following the test-first development style :-) ), further confused by what I saw were relevant changes around returning Bearer scheme (but not in OIDC but smallrye-jwt).
Pushing the fix now (with both smallrye-jwt and oidc tests updated), sorry about it

@sberyozkin
Copy link
Member Author

@gsmet re the Windows CI passing:

 --- maven-compiler-plugin:3.8.1-jboss-1:testCompile (default-testCompile) @ quarkus-integration-test-oidc ---
2021-04-13T01:35:06.6318636Z [INFO] Changes detected - recompiling the module!
2021-04-13T01:35:06.6321590Z [INFO] Compiling 2 source files to D:\a\quarkus\quarkus\integration-tests\oidc\target\test-classes
2021-04-13T01:35:07.5582436Z [INFO] 
2021-04-13T01:35:07.5588598Z [INFO] --- maven-surefire-plugin:3.0.0-M5:test (default-test) @ quarkus-integration-test-oidc ---
2021-04-13T01:35:07.5590278Z [INFO] Tests are skipped.

@famod
Copy link
Member

famod commented Apr 13, 2021

I guess this is due to the fact that on Windows no containers are started (-D*-containers is not set).

@sberyozkin sberyozkin force-pushed the select_matching_auth_mechanism branch from 5a7e88d to 9a0ecff Compare April 13, 2021 11:04
@sberyozkin
Copy link
Member Author

@famod I see, thanks - so I've also updated integration-tests/oidc-wiremock to have this test covered on Windows

@gsmet
Copy link
Member

gsmet commented Apr 13, 2021

@famod do you know if it's supported by GitHub Actions so start containers on Windows?

@famod
Copy link
Member

famod commented Apr 13, 2021

@sberyozkin
Copy link
Member Author

sberyozkin commented Apr 13, 2021

Oh dear, got a bit of shock :-), but Windows build failed with the extensions\resteasy-reactive\rest-client-reactive test problem; JDK 8 failure is not related either...

@quarkus-bot
Copy link

quarkus-bot bot commented Apr 13, 2021

Failing Jobs - Building 9a0ecff

Status Name Step Test failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 11 Windows Build Test failures Logs Raw logs
✔️ JVM Tests - JDK 15
JVM Tests - JDK 8 Build Test failures Logs Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 11 Windows #

📦 extensions/resteasy-reactive/rest-client-reactive/deployment

io.quarkus.rest.client.reactive.registerclientheaders.RegisterClientHeadersTest.shouldPassIncomingHeaders - More details - Source on GitHub


⚙️ JVM Tests - JDK 8 #

📦 integration-tests/kafka

io.quarkus.it.kafka.SaslKafkaConsumerTest.testReception line 48 - More details - Source on GitHub

io.quarkus.it.kafka.SslKafkaConsumerTest.testReception line 56 - More details - Source on GitHub

@sberyozkin
Copy link
Member Author

Hi @stuartwdouglas OK, thanks, merging shortly now that the build is green (as far as this PR is concerned).

Ideally we should also have some way of sorting these for the case where there is no challenge. I think at the moment the order is random.

Can you please open an issue with a few more details and I can have a look ? (I'm not quite sure I understand the case - do you mean something related to for ex to OIDC CodeAuthenticationMechanism reporting in some cases AuthenticationCompletionException leading to an immediate 401 as opposed to AuthenticationFailedException leading to the challenge or something else ?)

@sberyozkin
Copy link
Member Author

Hi @gsmet, @stuartwdouglas I've decided to remove a backport label as, even though it addresses a bug, it is arguably a bit too sensitive for a backport (the user reported the workaround was available). Please add it back if you think it is important to backport it

@sberyozkin sberyozkin merged commit 2ceef3d into quarkusio:main Apr 13, 2021
@quarkus-bot quarkus-bot bot added this to the 2.0 - main milestone Apr 13, 2021
@sberyozkin sberyozkin deleted the select_matching_auth_mechanism branch April 13, 2021 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JWT authentication failure responds with Basic Auth challenge when both JWT and basic mechanisms are enabled
4 participants