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

Support for two or more authentications for a single request #42935

Merged
merged 1 commit into from
Sep 5, 2024

Conversation

michalvavrik
Copy link
Member

@michalvavrik michalvavrik commented Sep 1, 2024

Copy link

github-actions bot commented Sep 1, 2024

🙈 The PR is closed and the preview is expired.

This comment has been minimized.

This comment has been minimized.

@michalvavrik
Copy link
Member Author

io.quarkus.it.oidc.OidcMtlsTest.testGetIdentityNames

Interesting, I have run that test like 10 times, including native mode, never failed. I am going to look what is different in CI.

@michalvavrik
Copy link
Member Author

So what has changed is that I am getting exception here https://github.com/quarkusio/quarkus/blob/main/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcProviderClient.java#L176 for OIDC client is is now missing.

@michalvavrik
Copy link
Member Author

@sberyozkin do you think you could investigate failures? they are caused by #41866 and I don't see anything wrong about OIDC setup in this PR. You have way better chance to figure it quickly as I think it might be a bug introduced in #41866. Thank you

@sberyozkin
Copy link
Member

Hi @michalvavrik I'm pretty sure the OIDC client reg work is unrelated - it is only about registering client dynamically, in fact, independently of quarkus-oidc. My bet is there is a race condition somewhere :-)

@sberyozkin
Copy link
Member

@michalvavrik Major thanks by the way for picking up the old PR and concluding this work, it will be a very important feature

@michalvavrik
Copy link
Member Author

michalvavrik commented Sep 2, 2024

Hi @michalvavrik I'm pretty sure the OIDC client reg work is unrelated - it is only about registering client dynamically, in fact, independently of quarkus-oidc. My bet is there is a race condition somewhere :-)

Hmm, but I reverted that registration and this worked like a charm. I really think that your PR is related, but if you want, I can investigate it myself. NP, I just thought you are better poised to it. The fact is that this line now throws exception and it didn't: https://github.com/quarkusio/quarkus/blob/main/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcProviderClient.java#L176

I can investigate though.

@michalvavrik Major thanks by the way for picking up the old PR and concluding this work, it will be a very important feature

I actually enjoyed it, it was nice little work that had to be done in Vert.X HTTP, nothing else :-)

@sberyozkin
Copy link
Member

@michalvavrik May be https://github.com/quarkusio/quarkus/pull/41866/files#diff-67404a43520250b1ebd5a0c945e070e60d70d4a3ba06b15e3fac747f820320cc are related though I'm not seeing how, I had to tweak the conditions for starting Keycloak Dev Services (to support starting even if the default tenant is disabled, and avoid registering default client). It looks like the default client is not registered by dev service

@sberyozkin
Copy link
Member

@michalvavrik
Copy link
Member Author

@michalvavrik

It must be this one:

https://github.com/quarkusio/quarkus/pull/41866/files#diff-67404a43520250b1ebd5a0c945e070e60d70d4a3ba06b15e3fac747f820320ccR855

Though again, I'm not sure exactly now why I did it

Cool, I knew I'll figure it out :-) Thanks for having a look after all.

@sberyozkin
Copy link
Member

@michalvavrik Let me try to run OIDC client reg tests without those client id/secret updates to remind myself why I did it

@sberyozkin
Copy link
Member

#42947 has been merged, please rebase

This comment has been minimized.

Copy link

quarkus-bot bot commented Sep 2, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 13e8f26.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

Copy link

quarkus-bot bot commented Sep 3, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 13e8f26.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 integration-tests/opentelemetry

io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest.testOTelInjections - History

  • Condition with Lambda expression in io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest was not fulfilled within 5 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest was not fulfilled within 5 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest.reset(OpenTelemetryInjectionsTest.java:26)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)

@michalvavrik
Copy link
Member Author

/cc @stuartwdouglas I believe I addressed all your comments. Not sure if you are busy these days, but if you find a moment, please have a look.

Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

Looks perfect to me. For OIDC Mutual TLS binding work, I can get a map of current security identities, get the MTLS one and extract the required data from the certificate to match them against the token

@sberyozkin
Copy link
Member

@michalvavrik Thanks, only minor JavaDocs comment, indeed, lets see if Stuart can comment during the next couple of days

@sberyozkin
Copy link
Member

Let's merge now

@sberyozkin sberyozkin merged commit bd33359 into quarkusio:main Sep 5, 2024
55 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Sep 5, 2024
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Sep 5, 2024
@michalvavrik michalvavrik deleted the feature/inclusive-auth branch September 5, 2024 09:07
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.

Support for inclusive authentication
2 participants