Skip to content

Enable JS on 2FA page specs to catch validation errors

Kushal Pandya requested to merge kp-add-2fa-recovery-page-tests into master

What does this MR do?

In order to address #20605 (closed), we had done a UX enhancement in !22868 (merged) which added attributes inputmode & pattern to 2FA code input field. Given that the input field we use to input 2FA code (a 6 digit numeric-only code) on authentication page is also re-used for recovery code (an alphanumeric string) input, the UX enhancement MR prevented users from entering alphanumeric string in the input field (due to pattern="[0-9]*"), it led to a ~S1 regression affecting multiple users and customers.

Ideally, the bug should've been caught by specs present in spec/features/users/login_spec.rb. Since the presence of pattern attribute in input field triggers an HTML5 form validation (handled by JavaScript in our case via app/assets/javascripts/gl_field_error.js), the error never got triggered during tests as the spec file doesn't have :js enabled and all the specs bypassed the validation we had in place.

This MR enables :js on the spec file and updates the tests to account for the 2FA field validation.

After the changes made in this MR, if we again set inputmode & pattern onto the 2FA input field like we originally did, it will correctly catch the error and several tests would fail as follows;

Failures:

  1) Login with two-factor authentication with valid username/password using one-time code blocks login with invalid code
     Failure/Error: expect(page).to have_content('Invalid two-factor code')
       expected to find text "Invalid two-factor code" in "GitLab Enterprise Edition Open source software to collaborate on code Manage Git repositories with fine-grained access controls that keep your code secure. Perform code reviews and enhance collaboration with merge requests. Each project can also have an issue tracker and a wiki. Two-Factor Authentication Two-factor authentication code This field is required. Enter the code from the two-factor app on your mobile device. If you've lost your device, you may enter one of your recovery codes. Explore Help About GitLab"
     # ./spec/features/users/login_spec.rb:203:in `block (5 levels) in <top (required)>'
     # ./spec/spec_helper.rb:251:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:251:in `block (2 levels) in <top (required)>'

  2) Login with two-factor authentication with valid username/password using one-time code allows login with invalid code, then valid code
     Failure/Error: expect(page).to have_content('Invalid two-factor code')
       expected to find text "Invalid two-factor code" in "GitLab Enterprise Edition Open source software to collaborate on code Manage Git repositories with fine-grained access controls that keep your code secure. Perform code reviews and enhance collaboration with merge requests. Each project can also have an issue tracker and a wiki. Two-Factor Authentication Two-factor authentication code This field is required. Enter the code from the two-factor app on your mobile device. If you've lost your device, you may enter one of your recovery codes. Explore Help About GitLab"
     # ./spec/features/users/login_spec.rb:212:in `block (5 levels) in <top (required)>'
     # ./spec/spec_helper.rb:251:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:251:in `block (2 levels) in <top (required)>'

  3) Login with two-factor authentication with valid username/password using backup code with valid code allows login
     Failure/Error: expect(current_path).to eq root_path

       expected: "/"
            got: "/users/sign_in"

       (compared using ==)
     # ./spec/features/users/login_spec.rb:246:in `block (6 levels) in <top (required)>'
     # ./spec/spec_helper.rb:251:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:251:in `block (2 levels) in <top (required)>'

  4) Login with two-factor authentication with valid username/password using backup code with valid code invalidates the used code
     Failure/Error:
       expect { enter_code(codes.sample) }
         .to change { user.reload.otp_backup_codes.size }.by(-1)

       expected `user.reload.otp_backup_codes.size` to have changed by -1, but was changed by 0
     # ./spec/features/users/login_spec.rb:254:in `block (6 levels) in <top (required)>'
     # ./spec/spec_helper.rb:251:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:251:in `block (2 levels) in <top (required)>'

  5) Login with two-factor authentication with valid username/password using backup code with valid code invalidates backup codes twice in a row
     Failure/Error:
       expect { enter_code(random_code) }
         .to change { user.reload.otp_backup_codes.size }.by(-1)

       expected `user.reload.otp_backup_codes.size` to have changed by -1, but was changed by 0
     # ./spec/features/users/login_spec.rb:265:in `block (6 levels) in <top (required)>'
     # ./spec/spec_helper.rb:251:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:251:in `block (2 levels) in <top (required)>'

  6) Login with two-factor authentication with valid username/password using backup code with valid code triggers ActiveSession.cleanup for the user
     Failure/Error:
       expect(adapter.send(counter))
         .to receive(:increment)
         .exactly(@exactly || :once)

       (Double "user_authenticated - Counter of successful authentication events").increment(*(any args))
           expected: 1 time with any arguments
           received: 0 times with any arguments
     # ./spec/support/matchers/metric_counter_matcher.rb:5:in `block (2 levels) in <main>'
     # ./spec/spec_helper.rb:251:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:251:in `block (2 levels) in <top (required)>'

  7) Login with two-factor authentication with valid username/password using backup code with invalid code blocks login
     Failure/Error: expect(page).to have_content('Invalid two-factor code.')
       expected to find text "Invalid two-factor code." in "GitLab Enterprise Edition Open source software to collaborate on code Manage Git repositories with fine-grained access controls that keep your code secure. Perform code reviews and enhance collaboration with merge requests. Each project can also have an issue tracker and a wiki. Two-Factor Authentication Two-factor authentication code This field is required. Enter the code from the two-factor app on your mobile device. If you've lost your device, you may enter one of your recovery codes. Explore Help About GitLab"
     # ./spec/features/users/login_spec.rb:297:in `block (6 levels) in <top (required)>'
     # ./spec/spec_helper.rb:251:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:251:in `block (2 levels) in <top (required)>'

Finished in 3 minutes 34.9 seconds (files took 42.33 seconds to load)
49 examples, 7 failures

Failed examples:

rspec ./spec/features/users/login_spec.rb:197 # Login with two-factor authentication with valid username/password using one-time code blocks login with invalid code
rspec ./spec/features/users/login_spec.rb:206 # Login with two-factor authentication with valid username/password using one-time code allows login with invalid code, then valid code
rspec ./spec/features/users/login_spec.rb:239 # Login with two-factor authentication with valid username/password using backup code with valid code allows login
rspec ./spec/features/users/login_spec.rb:249 # Login with two-factor authentication with valid username/password using backup code with valid code invalidates the used code
rspec ./spec/features/users/login_spec.rb:258 # Login with two-factor authentication with valid username/password using backup code with valid code invalidates backup codes twice in a row
rspec ./spec/features/users/login_spec.rb:275 # Login with two-factor authentication with valid username/password using backup code with valid code triggers ActiveSession.cleanup for the user
rspec ./spec/features/users/login_spec.rb:286 # Login with two-factor authentication with valid username/password using backup code with invalid code blocks login

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

#196812 (closed)

Edited by Kushal Pandya

Merge request reports