Skip to content
Snippets Groups Projects

Frontend: Enable WebAuthn device registration without TOTP

Merged Eduardo Sanz García requested to merge eduardosanz/webauthn-without-totp-frontend into master

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
image image
image image
image image
image
image image

The whole process using Chrome:

Screen_Recording_2023-02-13_at_21.01.36

How to set up and validate locally

  1. In rails console, enable the feature flag: Feature.enable(:webauthn_without_totp)
  2. Go to https://gdk.test:3443/-/profile/two_factor_auth
  3. Select Set up new device. It should be available even if the two-factor authentication using TOTP is disabled.
  4. 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.

Edited by Adil Farrukh

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Jon Glassman requested review from @jglassman1

    requested review from @jglassman1

  • added 1 commit

    • 5ac97918 - Suggestion from technical writer

    Compare with previous version

  • A deleted user added backend label

    added backend label

  • 3 Warnings
    :warning: This merge request is quite big (702 lines changed), please consider splitting it into multiple merge requests.
    :warning: 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.
    :warning:

    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:

    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 current availability (@halilcoban) (UTC+1, same timezone as @eduardosanz) Douglas Barbosa Alexandre current availability (@dbalexandre) (UTC+0, 1 hour behind @eduardosanz)
    frontend Diana Zubova current availability (@dzubova) (UTC+1, same timezone as @eduardosanz) Denys Mishunov current availability (@dmishunov) (UTC+1, same timezone as @eduardosanz)
    ~"group::authentication and authorization" Reviewer review is optional for ~"group::authentication and authorization" Imre Farkas current availability (@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 :repeat: danger-review job that generated this comment.

    Generated by :no_entry_sign: Danger

  • :wave: @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.

  • Eduardo Sanz García changed target branch from remove-totp-req-webauthn to remove-totp-req-webauthn-backend

    changed target branch from remove-totp-req-webauthn to remove-totp-req-webauthn-backend

  • A deleted user added feature flag label

    added feature flag label

  • Eduardo Sanz García added 3144 commits

    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

    Compare with previous version

  • Eduardo Sanz García changed the description

    changed the description

  • added 1 commit

    • 4d00bdce - Fix current WebAuthn workflow test

    Compare with previous version

  • Allure report

    allure-report-publisher generated test report!

    e2e-review-qa: :exclamation: test report for d11bcbad

    expand 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: :x: test report for d11bcbad

    expand 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  | ❌     |
    +------------------+--------+--------+---------+-------+-------+--------+
  • added 1 commit

    • 592395db - Fix current WebAuthn workflow test

    Compare with previous version

  • Eduardo Sanz García changed the description

    changed the description

  • Chris Micek approved this merge request

    approved this merge request

  • :wave: @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:

  • Chris Micek unapproved this merge request

    unapproved this merge request

  • added 4 commits

    • 448eecf9 - Enable WebAuthn device registration without TOTP
    • 0db0a8cb - Suggestion from technical writer
    • 4f97739b - Fix failing U2F tests
    • d8fc7e53 - Fix current WebAuthn workflow test

    Compare with previous version

    • 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

  • Aboobacker MK deleted the remove-totp-req-webauthn-backend branch. This merge request now targets the master branch

    deleted the remove-totp-req-webauthn-backend branch. This merge request now targets the master branch

    • Resolved by Peter Hegman

      Bundle size analysis [beta]

      This compares changes in bundle size for entry points between the commits c957be81 and d11bcbad

      :sparkles: Special assets

      Entrypoint / 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 :no_entry_sign: Danger

  • Eduardo Sanz García changed the description

    changed the description

  • added 289 commits

    Compare with previous version

  • Bogdan Denkovych
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading