fix: WebAuthn Discoverable Credential (Resident Credential) #35374

matthewloft · 2022-08-18T11:52:35Z

Description of Change

Enables support for Webauthn discoverable credentials (aka resident
credentials). This allows users to authenticate without first having to
select or type a username.

To decide if discoverable credentials are supported, the class
'AuthenticatorCommon', in the chrome content code, indirectly calls the
method 'context::WebAuthenticationDelegate.SupportsResidentKeys(..)'.
The default implementation of this returns false, leaving it up to
specific implementations to override.

This change adds a new class 'ElectronWebAuthenticationDelegate' to
subclass 'WebAuthenticationDelegate' and override the behaviour of the
'SupportsResidentKeys' method to return true.
The implementation is copied from the Chrome browser equivalent
'ChromeWebAuthenticationDelegate', though the chrome class includes
other methods that don't seem to be required for this functionality.

The 'ElectronContentClient' class was also updated to store an instance
of 'ElectronWebAuthenticationDelegate', and to provide an accessor
method, GetWebAuthenticationDelegate().

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant documentation is changed or added
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Added support for Webauthn discoverable keys (aka resident keys), allowing users to authenticate without first having to select or type a username.

…33353 Enables support for Webauthn discoverable credentials (aka resident credentials). This allows users to authenticate without first having to select or type a username. To decide if discoverable credentials are supported, the class 'AuthenticatorCommon', in the chrome content code, indirectly calls the method 'context::WebAuthenticationDelegate.SupportsResidentKeys(..)'. The default implementation of this returns false, leaving it up to specific implementations to override. This change adds a new class 'ElectronWebAuthenticationDelegate' to subclass 'WebAuthenticationDelegate' and override the behaviour of the 'SupportsResidentKeys' method to return true. The implementation is copied from the Chrome browser equivalent 'ChromeWebAuthenticationDelegate', though the chrome class includes other methods that don't seem to be required for this functionality. The 'ElectronContentClient' class was also updated to store an instance of 'ElectronWebAuthenticationDelegate', and to provide an accessor method, GetWebAuthenticationDelegate().

…credential

welcome · 2022-08-18T11:52:37Z

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

fix: don't overwrite prevent_default if default wasn't prevented
feat: add app.isPackaged() method
docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

Follow the JavaScript, C++, and Python coding style.
Run npm run lint locally to catch formatting errors earlier.
Document any user-facing changes you've made following the documentation styleguide.
Include tests when adding/changing behavior.
Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

codebytere

A few small changes, but overall looks good! I think we should look towards adding WebAuthn specs (Node.js does it so I know it's doable) but i recognize that might be a lot of work for the scope of this PR so once comments are addressed i'm happy to move forward w/ this!

shell/browser/webauthn/electron_authenticator_request_delegate.cc

shell/browser/webauthn/electron_authenticator_request_delegate.h

shell/browser/webauthn/electron_authenticator_request_delegate.cc

* docs: Update Quick Start Guide for Electron 12 With `contextIsolation` enabled by default in Electron 12, the Getting Started Guide no longer works as it is written. In order for the basic example to display values from `process.versions`, we need to add a `preload.js` to the example. * fix: annotate preload code block with a language * docs: update quick-start Fiddle example to use preload to provide version info * fix: ensure example files end in a newline * docs: add security warning to instructions for turning off contextIsolation Co-authored-by: John Kleinschmidt <jkleinsc@github.com> * docs: treat preload as an adjective instead of a noun Co-authored-by: John Kleinschmidt <jkleinsc@github.com> Co-authored-by: John Kleinschmidt <jkleinsc@github.com>

…credential

matthewloft · 2022-08-23T13:58:25Z

Thanks for the review, and appreciate your comments.

I hadn't added tests as I wasn't sure of the best approach.
The Chrome browser has some C++ unit tests I didn't see the equivalent in Electron.
This change is intended to support physical security keys (I tested with a Yubikey on Windows). The original base class method WebAuthenticationDelegate::SupportsResidentKeys() seems to already return true for virtual authenticators that might be present in an automated test, so could obscure the results of this change.

codebytere

lgtm 🥳

Just fix these two lint issues:

shell/browser/webauthn/electron_authenticator_request_delegate.h:5:  (cpplint) #ifndef header guard has wrong style, please use: ELECTRON_SHELL_BROWSER_WEBAUTHN_ELECTRON_AUTHENTICATOR_REQUEST_DELEGATE_H_  [build/header_guard] [5]
shell/browser/webauthn/electron_authenticator_request_delegate.h:23:  (cpplint) #endif line should be "#endif  // ELECTRON_SHELL_BROWSER_WEBAUTHN_ELECTRON_AUTHENTICATOR_REQUEST_DELEGATE_H_"  [build/header_guard] [5]
Total errors found: 2

matthewloft · 2022-08-24T12:34:28Z

Thanks for the approval, have made the lint changes.

…credential

matthewloft · 2022-09-08T23:01:30Z

Hi @codebytere ,
What is required to add backport labels?
I can't see how to add them, and it looks like they are needed to complete the checks.
Thanks!

welcome · 2022-09-20T22:47:44Z

Congrats on merging your first pull request! 🎉🎉🎉

release-clerk · 2022-09-20T22:47:45Z

Release Notes Persisted

Added support for Webauthn discoverable keys (aka resident keys), allowing users to authenticate without first having to select or type a username.

…#35374) * fix: WebAuthn Discoverable Credential (Resident Credential) electron#33353 Enables support for Webauthn discoverable credentials (aka resident credentials). This allows users to authenticate without first having to select or type a username. To decide if discoverable credentials are supported, the class 'AuthenticatorCommon', in the chrome content code, indirectly calls the method 'context::WebAuthenticationDelegate.SupportsResidentKeys(..)'. The default implementation of this returns false, leaving it up to specific implementations to override. This change adds a new class 'ElectronWebAuthenticationDelegate' to subclass 'WebAuthenticationDelegate' and override the behaviour of the 'SupportsResidentKeys' method to return true. The implementation is copied from the Chrome browser equivalent 'ChromeWebAuthenticationDelegate', though the chrome class includes other methods that don't seem to be required for this functionality. The 'ElectronContentClient' class was also updated to store an instance of 'ElectronWebAuthenticationDelegate', and to provide an accessor method, GetWebAuthenticationDelegate(). * Remove redundant, commented-out code * style: comment cleanup * style: updated comments and formatting based on pull request review * style: fix lint error on header guard clause

matthewloft added 6 commits August 15, 2022 15:50

Remove redundant, commented-out code

cde3049

style: comment cleanup

c009f7f

Merge branch 'electron:main' into defect-33353-Webauthn-discoverable-…

ae82279

…credential

Merge branch 'electron:main' into defect-33353-Webauthn-discoverable-…

c6ee135

…credential

Merge branch 'electron:main' into defect-33353-Webauthn-discoverable-…

05fd98a

…credential

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Aug 18, 2022

matthewloft mentioned this pull request Aug 18, 2022

[Bug]: WebAuthn Discoverable Credential (Resident Credential) #33353

Closed

3 tasks

codebytere requested changes Aug 22, 2022

View reviewed changes

MarshallOfSound reviewed Aug 22, 2022

View reviewed changes

shell/browser/webauthn/electron_authenticator_request_delegate.cc Outdated Show resolved Hide resolved

shell/browser/webauthn/electron_authenticator_request_delegate.cc Outdated Show resolved Hide resolved

matthewloft added 2 commits August 23, 2022 17:24

Merge branch 'electron:main' into defect-33353-Webauthn-discoverable-…

1d2a5a6

…credential

style: updated comments and formatting based on pull request review

10e20e2

codebytere changed the title ~~fix: WebAuthn Discoverable Credential (Resident Credential) #33353~~ fix: WebAuthn Discoverable Credential (Resident Credential) Aug 24, 2022

codebytere approved these changes Aug 24, 2022

View reviewed changes

codebytere added the semver/patch backwards-compatible bug fixes label Aug 24, 2022

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Aug 24, 2022

style: fix lint error on header guard clause

6b0deb0

Merge branch 'electron:main' into defect-33353-Webauthn-discoverable-…

0060d0b

…credential

matthewloft mentioned this pull request Sep 2, 2022

[Bug]: Appveyor test setup fails on some PRs #35564

Closed

3 tasks

Merge branch 'electron:main' into defect-33353-Webauthn-discoverable-…

706f31e

…credential

erickzhao added the no-backport label Sep 19, 2022

Merge branch 'main' into defect-33353-Webauthn-discoverable-credential

d97a21e

matthewloft requested a review from MarshallOfSound September 20, 2022 22:44

MarshallOfSound approved these changes Sep 20, 2022

View reviewed changes

MarshallOfSound merged commit 4935fd2 into electron:main Sep 20, 2022

matthewloft deleted the defect-33353-Webauthn-discoverable-credential branch September 20, 2022 22:58

commit-lint bot mentioned this pull request Jan 16, 2023

[pull] main from electron:main Blaquez-home/electron#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: WebAuthn Discoverable Credential (Resident Credential) #35374

fix: WebAuthn Discoverable Credential (Resident Credential) #35374

matthewloft commented Aug 18, 2022 •

edited by codebytere

welcome bot commented Aug 18, 2022

codebytere left a comment

matthewloft commented Aug 23, 2022

codebytere left a comment •

edited

matthewloft commented Aug 24, 2022

matthewloft commented Sep 8, 2022

welcome bot commented Sep 20, 2022

release-clerk bot commented Sep 20, 2022

fix: WebAuthn Discoverable Credential (Resident Credential) #35374

fix: WebAuthn Discoverable Credential (Resident Credential) #35374

Conversation

matthewloft commented Aug 18, 2022 • edited by codebytere

Description of Change

Checklist

Release Notes

welcome bot commented Aug 18, 2022

codebytere left a comment

Choose a reason for hiding this comment

matthewloft commented Aug 23, 2022

codebytere left a comment • edited

Choose a reason for hiding this comment

matthewloft commented Aug 24, 2022

matthewloft commented Sep 8, 2022

welcome bot commented Sep 20, 2022

release-clerk bot commented Sep 20, 2022

matthewloft commented Aug 18, 2022 •

edited by codebytere

codebytere left a comment •

edited