Introduce OIDC Database Token State Manager extension

[michalvavrik]

closes: #13239

New extension uses the Reactive SQL clients to store and retrieve tokens in a database, instead of storing them in a session cookie (which is default behavior). Currently Hibernate ORM and Hibernate Reactive doesn't work together, which means I could not leverage Hibernate Reactive, for the most applications are likely to use the Hibernate ORM extension directly. On the other hand, basing this on JDBC driver and block during authentication seems like a very bad design, because it would lead to thread switching. I don't think we should provide something so wrong (blocking auth), users can always write it themselves if necessary. As far as suggestion to use Panache goes, I also think it's not suitable (1. Hibernate Reactive vs ORM, 2. multitenancy is not supported).

[quarkus-bot]

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

• title should count at least 2 words to describe the change properly
• title should preferably start with an uppercase character (if it makes sense!)

_{This message is automatically generated by a bot.}

[github-actions]

🙈 The PR is closed and the preview is expired.

[michalvavrik]

Wow, these failures by name seems very much related, though I only extracted 2 methods from OIDC module. I'm going to check today how that could result in this.

[sberyozkin]

Hey @michalvavrik Thanks for preparing this new feature 👍 , a few tests are failing :-), but I think they'll get green as I believe you don't need to change anything in the default token manager, there is nothing to secure in the cookie value returned from the DB one - tokens are secured in the DB.

I'll prepare an email to the quarkus-dev with some justifications for the new feature

Thanks

[emmanuelbernard]

Is it a 30/70% PR review state or aimed ot be merged? If the latter,m I'd love ot see the doc changes and how we plan to expose that to users.

[michalvavrik]

Is it a 30/70% PR review state or aimed ot be merged? If the latter,m I'd love ot see the doc changes and how we plan to expose that to users.

I'll let @sberyozkin answer, I suppose Sergey will review when he has time. Only thing I know about now is that I have one unused property in recorder that I plan to remove when someone requires any changes at all.

[sberyozkin]

HI @emmanuelbernard @michalvavrik, indeed, update to the https://quarkus.io/guides/security-oidc-code-flow-authentication#handling-and-controlling-the-lifetime-of-authentication, should be part of the PR.

Michal, I believe we just need to add another subsection there, probably named Stateful token management, mention that users can register custom TokenStateManager beans to save the tokens in DB/distributed cache etc and Quarkus offers this extension which is activated by simply adding it as a dependency alongside the preferred reactive DB client, and list which ones are supported and show some example config. If you'd like I can contribute, thanks

[sberyozkin]

@michalvavrik It looks perfect. Thanks for updating JavaDocs for DefaultTokenStateManager too along the way 👍 , thanks

[sberyozkin]

Hi @yrodiere, can you please check a few of DB related classes in this PR, overall, the idea is to let uses persist 3 OIDC tokens (ID, access, refresh) in DB, query them, and remove, on demand and with a scheduled removal of expired entries.

The code in the oidc-db-token-state-manager extension...

Thanks

[yrodiere]

Hi @yrodiere, can you please check a few of DB related classes in this PR, overall, the idea is to let uses persist 3 OIDC tokens (ID, access, refresh) in DB, query them, and remove, on demand and with a scheduled removal of expired entries.

The code in the oidc-db-token-state-manager extension...

Thanks

Hey, this seems to use the reactive SQL clients, and unfortunately I know as little as you do in that area, probably less :)

It's probably better to ask @tsegismont :)

EDIT: But FWIW I agree with the decision not to use Hibernate ORM or Hibernate Reactive here, as it would certainly cause trouble for application developers, and #13425 is only one of them.

[sberyozkin]

Thanks for the feedback @yrodiere and also for giving your support to the @michalvavrik's design decision :-).

Overall, the DB related code is very simple, even I can understand it :-), I'd just like to have an expert to have a quick look, @tsegismont, if you can find a few mins to have a look it would be great, should not take much time, thanks

[quarkus-bot]

Failing Jobs - Building 0c1f591

Status	Name	Step	Failures	Logs	Raw logs	Build scan
✖	JVM Tests - JDK 11	Build	⚠️ Check →	Logs	Raw logs
✔️	JVM Tests - JDK 17
✖	JVM Tests - JDK 20	Build	Failures	Logs	Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 20 #

- Failing: extensions/hibernate-orm/deployment integration-tests/virtual-threads/mailer-virtual-threads 
! Skipped: extensions/flyway/deployment extensions/hibernate-envers/deployment extensions/hibernate-reactive/deployment and 103 more

📦 extensions/hibernate-orm/deployment

✖ io.quarkus.hibernate.orm.mapping.timezone.TimezoneDefaultStorageNormalizeTest. - More details - Source on GitHub

java.lang.RuntimeException: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:705)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

📦 integration-tests/virtual-threads/mailer-virtual-threads

✖ io.quarkus.virtual.mail.RunOnVirtualThreadTest.test line 25 - More details - Source on GitHub

java.net.SocketTimeoutException: Read timed out
	at java.base/sun.nio.ch.NioSocketImpl.timedRead(NioSocketImpl.java:278)
	at java.base/sun.nio.ch.NioSocketImpl.implRead(NioSocketImpl.java:304)

[sberyozkin]

@michalvavrik I think I'm going to merge it soon enough, as I said the DB code is straightforward, a basic entity is saved/retrieved. But we'll see what users will report and tune whatever will be required - I guess it is hard to spot any problem until users actually start stressing it. Overall it is comprehensively tested

[michalvavrik]

@michalvavrik I think I'm going to merge it soon enough, as I said the DB code is straightforward, a basic entity is saved/retrieved. But we'll see what users will report and tune whatever will be required - I guess it is hard to spot any problem until users actually start stressing it. Overall it is comprehensively tested

Thanks @sberyozkin . This morning I was actually looking for additional reviewers, but I didn't see between contributor to this reactive client anyone more fit than people you already asked . I can imagine some optimalization there could be, but it should be safe code anyway (also we have tests).

[sberyozkin]

This doc build failure was not here the previous time and I've seen it happening sometimes

[tsegismont]

@michalvavrik @sberyozkin sorry for the late reply

The OidcDbTokenStateManagerInitializer looks good to me. Perhaps it can be simplified a bit by not using mutiny (not useful here), but it's only a matter of readability.

[michalvavrik]

If you're absolutely sure the corresponding Vert.x instance is closed when this bean instance is destroyed, then you may omit to properly cancel the timer.
From my perspective it would be safer (and does not cost much) to do it explicitly.

I agree, fix is already merged here #36366.

[michalvavrik]

Perhaps it can be simplified a bit by not using mutiny (not useful here), but it's only a matter of readability.

I'll add it to my list and address it eventually, frankly using Mutiny was simpler to me as I'm used to it (it's pretty much omnipresent in Quarkus). Thanks for your feedback @tsegismont .