-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Webauthn improvements : docs, customisable cookies, virtual thread support #38373
Conversation
b1bdcfc
to
c050fdb
Compare
For @cescoffier 's review, it's mostly about the logic in https://github.com/quarkusio/quarkus/pull/38373/files#diff-493a6f5a982ad741f9a8c8b0a8485833005410f7e2fb3062f4c53290a7238dfeR44 that I'm not entirely convinced about. There are tests, though, but a logic/sanity check would not hurt. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The virtual thread part looks good to me!
First failure is my fault, but the second one about import sorting is annoying, locally it's not run:
|
Oh great, so I have to run the build on java < 20, then java 20. |
4374f43
to
acb3550
Compare
🙈 The PR is closed and the preview is expired. |
This comment has been minimized.
This comment has been minimized.
Yeah, the quickstarts have their own PR at quarkusio/quarkus-quickstarts#1377 how do we get out of this kind of chicken/egg issue @gsmet ? |
Hi Steph, @FroMage, thanks for the PR. This is for Cheers |
This PR doesn't change any mechanics of webauthn beyond giving the ability to configure the cookie names in use, and allowing virtual threads where blocking calls were also allowed. As such, it can be reviewed without any webauthn knowledge. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FroMage Apologies for a delay, while I definitely want to start helping with more concrete reviews, I'm falling behind right now and don't want to hold this important PR with various fixes, it should be part of 3.9 for sure. @cescoffier has already approved, so please go ahead with the merge when you are ready, thanks
@gsmet what's the procedure for merging inter-dependent PRs? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Resolving several issues in discussions
Because users get confused as to where to configure it. I also added it to the docs.
So it's more logical: register->callback, login->callback
And that we're not overriding the challenge part
Not sure what that failure was about, rebased and retrying. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I don't understand why the
|
Status for workflow
|
Status | Name | Step | Failures | Logs | Raw logs | Build scan |
---|---|---|---|---|---|---|
✖ | Quickstarts Compilation - JDK 17 | Compile Quickstarts |
Failures | Logs | Raw logs | 🚧 |
You can consult the Develocity build scans.
Failures
⚙️ Quickstarts Compilation - JDK 17 #
- Failing: security-webauthn-quickstart security-webauthn-reactive-quickstart
📦 security-webauthn-quickstart
✖ Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:testCompile (default-testCompile) on project security-webauthn-quickstart: Compilation failure
📦 security-webauthn-reactive-quickstart
✖ Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:testCompile (default-testCompile) on project security-webauthn-reactive-quickstart: Compilation failure
Status for workflow
|
Quickstart failure is normal, as there's a separate PR for it. |
No answer about quickstart, let's just merge this and get this done. |
This is mostly docs improvements, but also #38352 and #38351