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

Revisit options challenge endpoints #45132

Merged
merged 5 commits into from
Dec 17, 2024

Conversation

ynojima
Copy link
Member

@ynojima ynojima commented Dec 15, 2024

Addresses #45091, Follow-up for #44105

  • Changes /q/webauthn/register-options-challenge and /q/webauthn/login-options-challenge endpoints from POST to GET
    • They now accepts parameters as query parameters instead of request body JSON
  • The username parameters are renamed to username from name or userName for consistency
  • Make user parameter of loginCLientSteps method optional

I couldn't update the webauthn-*.svg files as I don't have the original files.

This comment has been minimized.

Copy link

github-actions bot commented Dec 15, 2024

🙈 The PR is closed and the preview is expired.

This comment has been minimized.

@ynojima ynojima force-pushed the revisit-options-challenge-endpoints branch from 239adb8 to 37c204f Compare December 15, 2024 02:46
@quarkus-bot quarkus-bot bot added area/docstyle issues related for manual docstyle review area/oidc area/vertx labels Dec 15, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

Besides the JS function, this is great. I can take care of updating the migration guide https://github.com/quarkusio/quarkus/wiki/Migration-Guide-3.18/#webauthn and the Quickstart (https://github.com/quarkusio/quarkus-quickstarts/tree/main/security-webauthn-quickstart and https://github.com/quarkusio/quarkus-quickstarts/tree/main/security-webauthn-reactive-quickstart if it actually turns out we need to update them) when we merge this.

Although, perhaps this is our last chance to rename userName to username and make this lowercase, what do you think?

@@ -215,13 +214,12 @@
if (!self.loginOptionsChallengePath) {
return Promise.reject('Login challenge path missing from the initial configuration!');
}
return self.fetchWithCsrf(self.loginOptionsChallengePath, {
method: 'POST',
return self.fetchWithCsrf(self.loginOptionsChallengePath + "?" + new URLSearchParams({name: user.name}).toString(), {
Copy link
Member

Choose a reason for hiding this comment

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

https://quarkus.io/version/main/guides/security-webauthn#invoke-login documents that the user name is optional for this method. It doesn't really say whether it's the entire user parameter that is optional, but the previous code used to do user || {} so it was in practice optional. Perhaps we should keep it entirely optional?

@ynojima
Copy link
Member Author

ynojima commented Dec 17, 2024

Although, perhaps this is our last chance to rename userName to username and make this lowercase, what do you think?

I prefer username.

@FroMage
Copy link
Member

FroMage commented Dec 17, 2024

Although, perhaps this is our last chance to rename userName to username and make this lowercase, what do you think?

I prefer username.

Alright, let's change it to username then, fix this now. Do you mind doing it as part of this PR?

@ynojima
Copy link
Member Author

ynojima commented Dec 17, 2024

Although, perhaps this is our last chance to rename userName to username and make this lowercase, what do you think?

I prefer username.

Alright, let's change it to username then, fix this now. Do you mind doing it as part of this PR?

I've added some commits to rename userName to username.
Also, I made a pull-request to quarkus-quickstarts: quarkusio/quarkus-quickstarts#1482

This comment has been minimized.

This comment has been minimized.

@ynojima ynojima force-pushed the revisit-options-challenge-endpoints branch from bf86e3c to 85938e7 Compare December 17, 2024 14:25

This comment has been minimized.

@ynojima ynojima force-pushed the revisit-options-challenge-endpoints branch from 85938e7 to 0da01e2 Compare December 17, 2024 14:54

This comment has been minimized.

This comment has been minimized.

@ynojima
Copy link
Member Author

ynojima commented Dec 17, 2024

@FroMage I've addressed userName renaming and webauthn.js issue. could you review again?
Also, I made a pull-request to quarkus-quickstarts: quarkusio/quarkus-quickstarts#1482

Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

Looks great. Could you rebase your commits to group them better? In particular the WIP and userName->username ones could be merged.

This comment has been minimized.

@FroMage
Copy link
Member

FroMage commented Dec 17, 2024

The quickstart failure is expected, since you have a PR pending there.

@ynojima
Copy link
Member Author

ynojima commented Dec 17, 2024

@FroMage Oh, I recalled why I marked it WIP.
I couldn't update the webauthn-.svg files as I don't have the original files. Could you update the webauthn-.svg files?

@ynojima ynojima force-pushed the revisit-options-challenge-endpoints branch from 0da01e2 to 5aeb831 Compare December 17, 2024 15:35
@FroMage
Copy link
Member

FroMage commented Dec 17, 2024

@FroMage Oh, I recalled why I marked it WIP. I couldn't update the webauthn-.svg files as I don't have the original files. Could you update the webauthn-.svg files?

Oh, oops. Let me see…

@ynojima
Copy link
Member Author

ynojima commented Dec 17, 2024

Rebased to main and merged some commits that are close in content

This comment has been minimized.

ynojima and others added 5 commits December 17, 2024 16:53
Change register-options-challenge endpoint and login-options-challenge endpoint from POST to GET
Update webauthn.js regarding change from POST to GET
Update security-webauthn.adoc
Correct query parameter name from name to userName
@FroMage FroMage force-pushed the revisit-options-challenge-endpoints branch from 5aeb831 to 2859983 Compare December 17, 2024 15:53
@FroMage
Copy link
Member

FroMage commented Dec 17, 2024

OK, should be good.

@FroMage FroMage merged commit e67b126 into quarkusio:main Dec 17, 2024
6 of 8 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.18 - main milestone Dec 17, 2024
@FroMage
Copy link
Member

FroMage commented Dec 17, 2024

I merged this and the Quickstarts, and updated the migration docs. Thanks a lot @ynojima !

jedla97 added a commit to jedla97/quarkus-test-suite that referenced this pull request Dec 18, 2024
…EGISTER_CHALLENGE_OPTIONS_URL endpoint

For more info about these changes see quarkusio/quarkus#45132
michalvavrik pushed a commit to quarkus-qe/quarkus-test-suite that referenced this pull request Dec 18, 2024
…EGISTER_CHALLENGE_OPTIONS_URL endpoint

For more info about these changes see quarkusio/quarkus#45132
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.

3 participants