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.

Sign up for GitHub

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

Jump to bottom

DEV: Add UI for passkeys (3/3) #23853

Merged
merged 11 commits into from Oct 13, 2023
Merged

DEV: Add UI for passkeys (3/3) #23853

merged 11 commits into from Oct 13, 2023

Conversation

pmusaraj
Copy link
Contributor

@pmusaraj pmusaraj commented Oct 9, 2023 •
edited

See #23587 and #23586 for the backend implementation.

  1. Registering a passkey:
passkey-register-rename.mp4
  1. Authenticating with a passkey
passkeys-auth.mp4

This is all behind a hidden site setting, so it's still in an early stage, not production-ready yet.

@pmusaraj pmusaraj changed the base branch from main to passkeys-part-2 October 9, 2023 19:52
This was referenced Oct 9, 2023
Closed
Merged
Base automatically changed from passkeys-part-2 to main October 11, 2023 18:36
@pmusaraj pmusaraj marked this pull request as ready for review October 11, 2023 19:47
jjaffeux
jjaffeux reviewed Oct 12, 2023
View reviewed changes
app/assets/javascripts/discourse/app/lib/webauthn.js
return errorCallback(I18n.t("login.security_key_support_missing_error"));
}

return navigator.credentials
Copy link
Contributor

Choose a reason for hiding this comment

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

await?

jjaffeux
jjaffeux reviewed Oct 13, 2023
View reviewed changes
app/assets/javascripts/discourse/app/components/user-preferences/user-passkeys.gjs
return;
}

this.router.refresh();
Copy link
Contributor

Choose a reason for hiding this comment

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

this returns a transition, I think it would be more correct to await here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, here the user is adding a new key, and we're about to show them a modal to name that key, but in the meantime, the key's already been stored. The call to this.router.refresh is updating the list of keys that is shown behind the dialog's overlay. In a sense, this is a background action, and the next item in the flow (showing the rename dialog) doesn't depend on the router refresh. In that sense, adding await isn't useful. (I do get that adding await would be more correct/consistent, but it would also be a tiny bit slower.)

jjaffeux
jjaffeux reviewed Oct 13, 2023
View reviewed changes
app/assets/javascripts/discourse/app/components/user-preferences/user-passkeys.gjs
Comment on lines +125 to +129
didConfirm: () => {
this.args.model.deletePasskey(id).then(() => {
this.router.refresh();
});
},
Copy link
Contributor

@jjaffeux jjaffeux Oct 13, 2023 •
edited

Choose a reason for hiding this comment

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

There are various cases like this where we have promises which are kinda lost, ultimately it can make the UI complicated to reason about, for example here you need some schedule("afterRender" which should really not
be necessary from what I see

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, the afterRender is here for a different reason, it has to do with showing subsequent dialogs (already not a good idea) and the didConfirm event... I'll look into a refactor for that shortly.

@pmusaraj pmusaraj merged commit 1a70817 into main Oct 13, 2023
17 checks passed
@pmusaraj pmusaraj deleted the passkeys-part-3-rebased branch October 13, 2023 16:24
@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/support-passwordless-login-with-passkeys/229259/34

gschlager
gschlager reviewed Jan 30, 2024
View reviewed changes
app/assets/javascripts/discourse/app/components/user-preferences/user-passkeys.gjs
Comment on lines +200 to +207
<span class="prefix">
{{this.addedPrefix}}
</span>
{{this.formatDate
passkey.created_at
format="medium"
leaveAgo="true"
}}
Copy link
Member

@gschlager gschlager Jan 30, 2024 •
edited

Choose a reason for hiding this comment

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

@pmusaraj Ideally the date would be a parameter of the translation. Joining those strings together makes it harder to translate. The same applies to the usage of {{this.lastUsedPrefix}} a couple of lines below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call @gschlager, should be addressed now in #26178

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

@gschlager gschlager gschlager left review comments

@jjaffeux jjaffeux jjaffeux approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
4 participants
@pmusaraj @discoursebot @jjaffeux @gschlager