Frontend: Enable WebAuthn device registration without TOTP
What does this MR do and why?
This MR enables the posibility of register a WebAuthn device without the need to set up TOTP as 2FA and resolves Frontend: Enable WebAuthn device registration w... (!111659 - merged)
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
button 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.
A new set of tests are needed in spec/features/webauthn_spec.rb
when the webauthn_without_totp
is enable. However, I will do that in a follow-up so it doesn't clash with the changes made in that file in the backend MR.
Changelog: changed
Screenshots or screen recordings
before | after |
---|---|
![]() |
![]() |
![]() |
![]() |
![]() |
![]() |
![]() |
|
![]() |
![]() |
The whole process using Chrome:
Screen_Recording_2023-02-13_at_21.01.36
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.
- [s] 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
added 1 commit
- 64411807 - Enable WebAuthn device registration without TOTP
added pipeline:run-all-e2e label
mentioned in merge request !110678 (closed)
added 1 commit
- 50fa7e0a - Enable WebAuthn device registration without TOTP
- Resolved by Eduardo Sanz García
@jglassman1, would you mind reviewing this MR? Thanks!
requested review from @eduardosanz
mentioned in merge request !111317 (merged)
- Resolved by Chris Micek
@chrismicek, would you mind reviewing the slightly updated UX in this MR? Thanks!
requested review from @chrismicek
added Technical Writing label
- Resolved by Eduardo Sanz García
requested review from @jglassman1
- A deleted user
added backend label
3 Warnings This merge request is quite big (702 lines changed), please consider splitting it into multiple merge requests. 66c6e452: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 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.
Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend Halil Coban (
@halilcoban
) (UTC+1, same timezone as@eduardosanz
)Douglas Barbosa Alexandre (
@dbalexandre
) (UTC+0, 1 hour behind@eduardosanz
)frontend Diana Zubova (
@dzubova
) (UTC+1, same timezone as@eduardosanz
)Denys Mishunov (
@dmishunov
) (UTC+1, same timezone as@eduardosanz
)~"group::authentication and authorization" Reviewer review is optional for ~"group::authentication and authorization" Imre Farkas (
@ifarkas
) (UTC+1, same timezone as@eduardosanz
)To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Danger @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.
- A deleted user
added feature flag label
removed feature flag label
added 3144 commits
-
5ac97918...cd0713a5 - 3141 commits from branch
remove-totp-req-webauthn-backend
- fcdbb445 - Enable WebAuthn device registration without TOTP
- 1806122a - Suggestion from technical writer
- ca14e047 - Fix failing U2F tests
Toggle commit list-
5ac97918...cd0713a5 - 3141 commits from branch
Allure report
allure-report-publisher
generated test report!e2e-review-qa:
test report for d11bcbadexpand test summary
+-----------------------------------------------------------------------+ | suites summary | +------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------+--------+--------+---------+-------+-------+--------+ | Create | 28 | 0 | 1 | 0 | 29 | ✅ | | Manage | 31 | 0 | 5 | 0 | 36 | ✅ | | Govern | 27 | 0 | 5 | 5 | 32 | ❗ | | Plan | 49 | 0 | 1 | 0 | 50 | ✅ | | Verify | 12 | 0 | 0 | 1 | 12 | ❗ | | Monitor | 4 | 0 | 0 | 0 | 4 | ✅ | | Framework sanity | 9 | 0 | 1 | 0 | 10 | ✅ | | Package | 0 | 0 | 1 | 0 | 1 | ➖ | +------------------+--------+--------+---------+-------+-------+--------+ | Total | 160 | 0 | 14 | 6 | 174 | ❗ | +------------------+--------+--------+---------+-------+-------+--------+
e2e-package-and-test:
test report for d11bcbadexpand test summary
+-----------------------------------------------------------------------+ | suites summary | +------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------+--------+--------+---------+-------+-------+--------+ | Create | 752 | 2 | 122 | 12 | 876 | ❌ | | Plan | 312 | 0 | 0 | 5 | 312 | ❗ | | Package | 82 | 0 | 103 | 5 | 185 | ❗ | | Verify | 249 | 0 | 10 | 5 | 259 | ❗ | | Fulfillment | 12 | 0 | 110 | 5 | 122 | ❗ | | Growth | 0 | 0 | 10 | 0 | 10 | ➖ | | Manage | 379 | 0 | 31 | 3 | 410 | ❗ | | Secure | 35 | 0 | 5 | 0 | 40 | ✅ | | Release | 30 | 0 | 0 | 0 | 30 | ✅ | | ModelOps | 0 | 0 | 5 | 0 | 5 | ➖ | | Govern | 220 | 0 | 10 | 0 | 230 | ✅ | | Framework sanity | 0 | 0 | 7 | 0 | 7 | ➖ | | Configure | 1 | 0 | 15 | 0 | 16 | ✅ | | Systems | 19 | 0 | 0 | 0 | 19 | ✅ | | Analytics | 11 | 0 | 0 | 0 | 11 | ✅ | | GitLab Metrics | 2 | 0 | 1 | 0 | 3 | ✅ | | Monitor | 42 | 0 | 0 | 0 | 42 | ✅ | | Data Stores | 11 | 0 | 3 | 0 | 14 | ✅ | +------------------+--------+--------+---------+-------+-------+--------+ | Total | 2157 | 2 | 432 | 35 | 2591 | ❌ | +------------------+--------+--------+---------+-------+-------+--------+
mentioned in merge request gitlab-com/www-gitlab-com!117506 (merged)
@chrismicek
, thanks for approving this merge request.This is the first time the merge request is approved. To ensure full test coverage, a new pipeline will be started shortly.
For more info, please refer to the following links:
added pipeline:mr-approved label
- Resolved by Eduardo Sanz García
@sheldonled, would you mind doing the first frontend review? Thanks!
@vburton, could you do the test review? Thanks!
@bdenkovych, could you do the backend review? Thanks!
This is a frontend MR. There is only these files that need to be considered for backend review:
app/helpers/device_registration_helper.rb
spec/helpers/device_registration_helper_spec.rb
spec/features/u2f_spec.rb
spec/frontend/fixtures/u2f.rb
spec/frontend/fixtures/webauthn.rb
I would like this MR is scheduled for %15.9 and there are a few follow-ups. Any chances you could do the review within the next 24 hours? Thank you very much!
requested review from @sheldonled, @vburton, and @bdenkovych
- Resolved by Peter Hegman
Bundle size analysis [beta]
This compares changes in bundle size for entry points between the commits c957be81 and d11bcbad
Special assetsEntrypoint / Name Size before Size after Diff Diff in percent average 3.49 MB 3.49 MB - 0.0 % mainChunk 2 MB 2 MB - 0.0 %
Note: We do not have exact data for c957be81. So we have used data from: 71a6f3b8.
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
Danger
added 289 commits
-
d8fc7e53...a4e5b108 - 285 commits from branch
master
- d13bf201 - Enable WebAuthn device registration without TOTP
- 596c428e - Suggestion from technical writer
- 6c40d4eb - Fix failing U2F tests
- d70414f5 - Fix current WebAuthn workflow test
Toggle commit list-
d8fc7e53...a4e5b108 - 285 commits from branch
- Resolved by Eduardo Sanz García
- Resolved by Eduardo Sanz García
- Resolved by Dylan Griffith
- Resolved by Eduardo Sanz García