Draft: Enable WebAuthn device registration without TOTP
What does this MR do and why?
Replaced the JQuery application to register WebAuthn devices by a Vue component.
Made the WebAuthn device registration possible without TOTP. Therefore,
the Set up new device
is always available.
Increased security by adding a required password field to be able to register a new device.
We also introduced a few minor UI improvements.
Changelog: changed
Screenshots or screen recordings
before | after |
---|---|
![]() |
![]() |
![]() |
![]() |
![]() |
![]() |
![]() |
![]() |
![]() |
![]() |
The whole process using Chrome:
Screen_Recording_2023-02-10_at_09.56.35
How to set up and validate locally
- In rails console, enable the feature flag:
Feature.enable(:webauthn_without_totp)
- Go to https://gdk.test:3443/-/profile/two_factor_auth
- Select
Set up new device
. It should be available even if the two-factor authentication using TOTP is disabled. - Follow the workflow.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Merge request reports
Activity
changed milestone to %15.9
assigned to @eduardosanz
added devopsmanage sectiondev labels
mentioned in merge request !107438 (closed)
- A deleted user
added backend label
2 Warnings This merge request is quite big (559 lines changed), please consider splitting it into multiple merge requests. featureaddition and featureenhancement merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the Technical Writer counterpart.
For more information, see:
- The Handbook page on merge request types.
- The definition of done documentation.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
DangerBundle size analysis [beta]
This compares changes in bundle size for entry points between the commits 754c7c48 and 13fc7d78
Special assetsEntrypoint / Name Size before Size after Diff Diff in percent average 3.56 MB 3.56 MB +1.27 KB 0.0 % mainChunk 1.95 MB 1.95 MB - 0.0 % Significant Growth: 10Expand
Entrypoint / Name Size before Size after Diff Diff in percent pages.admin.sessions 91.63 KB 160.28 KB +68.65 KB 74.9 % pages.groups.omniauth_callbacks 100.13 KB 168.78 KB +68.65 KB 68.6 % pages.ldap.omniauth_callbacks 79.42 KB 148.07 KB +68.65 KB 86.4 % pages.omniauth_callbacks 79.42 KB 148.07 KB +68.65 KB 86.4 % pages.sessions 79.45 KB 148.1 KB +68.65 KB 86.4 % pages.trial_registrations.new 102.3 KB 170.95 KB +68.65 KB 67.1 % pages.trials 79.45 KB 148.1 KB +68.65 KB 86.4 % pages.sessions.new 255.21 KB 298.96 KB +43.75 KB 17.1 % pages.trials.apply 280.01 KB 290.3 KB +10.3 KB 3.7 % pages.trials.select 288.19 KB 298.48 KB +10.3 KB 3.6 %
Your MR has at least one entrypoint growing significantly (more > 1 KB or 2%). If you write new or extend existing features, this is expected and there is nothing to worry about.
Please consider pinging someone from the FE Foundations (
@leipert
,@markrian
,@mikegreiling
,@ohoral
or@pgascouvaillancourt
) for review, if you are unsure about the size increase.Note: We do not have exact data for 754c7c48. So we have used data from: b137aee9.
The intended commit has no webpack pipeline, so we chose the last commit with one before it.Please look at the full report for more details
Read more about how this report works.
Generated by
DangerAllure report
allure-report-publisher
generated test report!e2e-review-qa:
test report for 99c9c8e0expand test summary
+-----------------------------------------------------------------------+ | suites summary | +------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------+--------+--------+---------+-------+-------+--------+ | Plan | 49 | 0 | 1 | 0 | 50 | ✅ | | Manage | 31 | 2 | 3 | 0 | 36 | ❌ | | Govern | 27 | 0 | 5 | 0 | 32 | ✅ | | Verify | 12 | 0 | 0 | 0 | 12 | ✅ | | Create | 28 | 0 | 1 | 0 | 29 | ✅ | | Monitor | 4 | 0 | 0 | 0 | 4 | ✅ | | Framework sanity | 9 | 0 | 1 | 0 | 10 | ✅ | | Package | 0 | 0 | 1 | 0 | 1 | ➖ | +------------------+--------+--------+---------+-------+-------+--------+ | Total | 160 | 2 | 12 | 0 | 174 | ❌ | +------------------+--------+--------+---------+-------+-------+--------+
@eduardosanz, please can you answer the question: Should this have a feature flag? to help with code review for the Authentication and Authorization group.This nudge was added by this triage-ops policy.
added 1 commit
- a0f61aed - Enable WebAuthn device registration without TOTP
@jglassman1, please, could you review the sentences added to
gitlab.pot
? Thanks!Thanks @eduardosanz I've made a small suggestion to make it more active, and asked one question.
requested review from @jglassman1
49085 49091 msgid "Your commit email is used for web based operations, such as edits and merges." 49086 49092 msgstr "" 49087 49093 49094 msgid "Your current password is required to register a new device." @dmoraBerlin, I wanted to make you aware about this reimplementation of the WebAuthn device registration. There is a video in the description. Next week I will add you as a reviewer. Thanks!
@eduardosanz thanks! Please mention me directly, when ready.
16612 16615 msgid "Examples" 16613 16616 msgstr "" 16614 16617 16618 msgid "Except for USB security keys, we recommend including the browser vendor and computer name in the device name." Question: Is there any guidance for USB security keys? Should the user include another piece of information?
There is no specific guidance for USB security keys.
I don't think the message is accurate or clear. I am trying to encourage users to include the browser (device) and computer names to make sure they understand that some WebAuthn devices only work under the same browser-computer combo used during the device registration.
USB security key can be carried around so you can sign-in from any computer.
Safari offers storing newly created passkeys in iCloud so it can be used in Safari on other devices (iPad, iPhones, or Mac computers), if sign-in with the same Apple ID on both devices.
But for other browsers this is not the case: If one uses a non-Safari browser, the person can't expect to be able to sign in using a different computer. In addition, if the browser cache is cleared users will be unable to sign-in.
I personally would use these device names:
- YubiKey [model if I have several] (just device, no computer name needed)
- Safari (just device, no computer name needed if I sync the passkey)
- Safari on laptop (device and computer name when I don't use sync the passkey)
- Chrome Beta on laptop (device and computer name)
Chrome, Chrome Beta, Chrome Canary and Chrome Dev are different devices. Firefox and Firefox Developer Editions are also different devices.
Two-factor authentication using WebAuthn is fragile. The user must always save their recovery codes.
I wonder if an info alert is better suited to explain all the above. I really would like to avoid mention any particular browser vendor. Would something like this be understandable?
You can lose access to your account: * Always save recovery codes (only available the first time you set a two-factor authentication method). * Clearing the cache of your browser will unable to sign-in. * Some WebAuthn devices, like USB security keys, or browsers that allow you to store the passkey in the cloud, can be used on different computers[1]. * Other WebAuthn devices can only be used on the same computer[1] where the device was registered.
[1] I used the word
computer
and nodevice
to avoid confusion withWebAuthn device
Edited by Eduardo Sanz GarcíaThanks @eduardosanz that all makes sense. I think there's a couple of possible approaches:
-
Keep all the information in an info alert, but put the strongest message upfront rather than in an unordered list. For example:
16630 msgid "Except for USB security keys, we recommend including the browser vendor and computer name in the device name." 16630 msgid "You must save your recovery codes when you first set up two-factor authentication with your WebAuthn 16631 device, so you do not lose access to your account. When managing your WebAuthn device, you should be aware that: 16632 * If you clear your browser cache, you must re-authenticate before you can sign in. 16633 * Some WebAuthn devices, like USB security keys or browsers that allow you to store the passkey in the cloud, can 16634 be used on different computers. 16635 * Other WebAuthn devices can only be used on the same computer where the device was registered." -
Keep the alert short but link to documentation for the supporting information (assuming that this type of alert supports linking. For example:
16630 msgid "Except for USB security keys, we recommend including the browser vendor and computer name in the device name." 16630 msgid "You must save your recovery codes when you first set up two-factor authentication with your WebAuthn 16631 device, so you do not lose access to your account. See the documentation on managing your WebAuthn device for 16632 more information."
What do you think?
-
Ok great. If you're happy with this suggestion, please apply it and then I'll approve.
changed this line in version 3 of the diff
mentioned in issue #378844 (closed)
mentioned in merge request !111317 (merged)
added 1237 commits
-
a0f61aed...82572aee - 1236 commits from branch
master
- 13fc7d78 - Enable WebAuthn device registration without TOTP
-
a0f61aed...82572aee - 1236 commits from branch
I am closing this MR in favour of !111659 (merged)