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

Should security-web-authn guide and quickstart use Hibernate Reactive by default? #36438

Closed
gsmet opened this issue Oct 12, 2023 · 9 comments · Fixed by #38299
Closed

Should security-web-authn guide and quickstart use Hibernate Reactive by default? #36438

gsmet opened this issue Oct 12, 2023 · 9 comments · Fixed by #38299

Comments

@gsmet
Copy link
Member

gsmet commented Oct 12, 2023

The security-web-authn guide and quickstart are using Hibernate Reactive and I don't think it's something mandatory when using security-web-authn?

If not, I think we would rather have a security-web-authn quickstart that is not using HR and maybe rename the existing one to security-web-authn-reactive.

And the guide should be based on the non-reactive one.

As now mentioned in the Hibernate Reactive guide, we are recommending using Hibernate ORM for most workloads, except when you need high concurrency so Hibernate ORM should be the default for our guides when HR is not mandatory.
Especially since it makes things a tad more complex and might have bring more complexity than needed to explain the concepts of this particular feature.

/cc @sberyozkin I'm not sure who worked on that, could you coordinate this (if it makes sense and I'm not mistaken that HR is not mandatory)?

(This issue is part of various things noted when I added the :topics: to the guides)

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 12, 2023

/cc @DavideD (hibernate-reactive), @Sanne (hibernate-reactive), @gavinking (hibernate-reactive), @sberyozkin (security)

@sberyozkin
Copy link
Member

@FroMage has implemented the feature, Steph, please investigate

@sberyozkin
Copy link
Member

Also CC-ing @michalvavrik who may have an opinion about it, given his recent work on the OIDC DB token state manager, where he chose to use reactive DB clients

@michalvavrik
Copy link
Member

Hey, I can't say more to this topic than you already know. I'm sure of that, but I'll summarize:

  • if you go with Hibernate ORM, this https://quarkus.io/guides/security-webauthn#exposing-your-entities-to-quarkus-webauthn implementation of WebAuthnUserProvider will need to block, in case you have non blocking endpoint etc. it seems unnecessary switching of threads. Maybe @gsmet you can quantify impact and say it's not a big deal?
  • especially with proactive auth, you are almost certain to run on IO thread, similarly lazy auth on preferred stacks like RESTEasy Reactive where security checks are invoked from ServerRestHandler.
  • Lately I've added io.quarkus.security.spi.runtime.BlockingSecurityExecutor that you can inject as a bean and use it for db access when you switch to Hiberante ORM

Also, I'm happy to help with rewriting this to Hibernate ORM, but judging by my queue of Quarkus issue, I can't start before 2 weeks from now.

@FroMage
Copy link
Member

FroMage commented Oct 17, 2023

Did I make it use Hibernate Reactive? I can't recall. My pet project using WebAuthn uses ORM, since Renarde doesn't support HR yet.

@michalvavrik feel free to change the guide and quickstart, and check out https://github.com/FroMage/quarkus-renarde-todo/blob/main/src/main/java/util/MyWebAuthnSetup.java for a working example with ORM.

@FroMage FroMage removed their assignment Oct 17, 2023
@Doogiemuc
Copy link

Just wanted to give a +1 for this. The "beginners guide" for WebAuthn should not be using Reactive. Keep it simple. (I had to rewrite it for my own app too.)

@michalvavrik
Copy link
Member

Sorry, I little bit forget about this, I'll have a look when I'm back in the middle of January. Apologies.

Just wanted to give a +1 for this. The "beginners guide" for WebAuthn should not be using Reactive. Keep it simple. (I had to rewrite it for my own app too.)

Reactive approach has pros during auth described in previous comments, I understand your position, but I'm definitely not excited about this change, it's more like no-one supported my position, hence we shall migrate.

@Doogiemuc
Copy link

I tried to create a REALLY minimal demo of quarkus-webauhn. No reactive. Even no database or real entities at all. Just mocks.

https://github.com/Doogiemuc/quarkus-webauthn-minimal-demo

But I am having some problems. When trying to register a new user on Mac Safari with Fingerprint I get the error:

HTTP Request to /q/webauthn/callback failed, error id: 9b447afd-1358-4e5c-9478-f9f674be4cae-1: io.vertx.ext.auth.webauthn.impl.attestation.AttestationException: AAGUID is not 00000000-0000-0000-0000-000000000000!

I see that the class NoneAttestation.java seem to check for this specific AAGUID. But where or how can I set it during register?

@FroMage
Copy link
Member

FroMage commented Jan 4, 2024

I'm not sure, but could you file a new issue for this please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

5 participants