Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Be more tolerant of user input for auth codes #518

Merged
merged 19 commits into from
Mar 3, 2023

Conversation

dd32
Copy link
Member

@dd32 dd32 commented Feb 14, 2023

Currently the TOTP (and backup codes / email codes, but that's less relevant) user input is tollerant of leading/trailing spaces, but doesn't accept TOTP codes that are entered how most apps display them: 123 456.

This PR does several things:

  • Adds a placeholder value to the fields, so as to convey the format expected.
  • Makes it tolerant of whitespace in the middle of the number (123 456)
  • Removes sanitize_text_field() and uses REQUEST & wp_unslash() consistently between the providers. sanitize_text_field() has been removed as it strips characters from the provided code.. which shouldn't really be needed.. as invalid input is an invalid code.
  • Adds a unit test to cover the invalid characters / whitespace changes.

Examples using TOTP, the others are similar just with 78 affixed.

Before After Invalid Input
Screenshot 2023-02-14 at 1 36 24 pm
Screenshot 2023-02-14 at 1 36 31 pm
Screenshot 2023-02-14 at 1 35 59 pm Screenshot 2023-02-14 at 1 36 06 pm

After Feedback from Ian:

Before After
Screenshot 2023-02-14 at 1 36 24 pm Screenshot 2023-02-15 at 11 40 24 am
Screenshot 2023-02-15 at 11 40 47 am
(Spaces in input highlighted)
Screenshot 2023-02-14 at 1 36 31 pm Entering a character other than numbers and spaces ([0-9 ]*)
are stripped and do not "enter" into the field,
avoiding the "does not match expected format" error.

Notably, it doesn't handle invalid input (non-numeric/space) well. But at least now the "Match the format requested" is more obvious based on the placeholder value presented.

Copy link
Member

@iandunn iandunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍🏻

tests/providers/class-two-factor-backup-codes.php Outdated Show resolved Hide resolved
providers/class-two-factor-backup-codes.php Outdated Show resolved Hide resolved
<input type="tel" name="two-factor-totp-authcode" id="two-factor-totp-authcode" class="input" value="" size="20" pattern="[0-9]*" />
<?php
/* translators: Example auth code. */
$placeholder = sprintf( __( 'eg. %s', 'two-factor' ), '123456' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$placeholder = sprintf( __( 'eg. %s', 'two-factor' ), '123456' );
$placeholder = sprintf( __( 'eg. %s', 'two-factor' ), '123 456' );

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've taken a slightly different approach, which I'd like your review on @iandunn.

Instead of treating it like a string number, or groups of two digits, I've instead increased the width between all the characters, which both makes it easier to read, and due to the increased space, "feels like" (to me) that there's less of a requirement for spaces on the input (as it's auto-spaced).

Adding a space between the chunks like this is still possible, and ends up like this:

Current with extra space user input
Screenshot 2023-02-15 at 11 52 57 am Screenshot 2023-02-15 at 11 55 46 am
(no user-input spaces)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 , I like this better than e.g., 123 456. I lean towards 123 456 being best, though. IMO blocks of numbers like in auth codes have two problems:

  1. they're hard to read because the numbers all run together
  2. they're hard to remember because the sequence is too long

Adding extra (uniform) space between each letter solves 1, but not 2. Grouping the numbers into small blocks like 123 456 solves both problems IMO.

I don't feel strongly, though. I think 1 2 3 4 5 6 is an improvement on the current situation 👍🏻

providers/class-two-factor-backup-codes.php Outdated Show resolved Hide resolved
providers/class-two-factor-backup-codes.php Outdated Show resolved Hide resolved
providers/class-two-factor-email.php Outdated Show resolved Hide resolved
providers/class-two-factor-totp.php Outdated Show resolved Hide resolved
@dd32 dd32 force-pushed the fix/user-tolerant-input branch from 95e82f9 to 3ed9089 Compare February 15, 2023 01:39
@jeffpaul
Copy link
Member

jeffpaul commented Feb 15, 2023

FWIW, GitHub itself uses XXXXXX as the placeholder text, but I concur with you here that 1 2 3 4 5 6 seems "better".

Screenshot 2023-02-15 at 2 47 50 PM

@dd32
Copy link
Member Author

dd32 commented Feb 17, 2023

GitHub itself uses XXXXXX as the placeholder text

I've always been partial to the Stripe UI (and similar ones) that use dedicated boxes. Those that use this format however are less flexible of singular input boxes that just "accept a two factor challenge response" (ie. TOTP/Backup/Email/SMS/etc code)

Screenshot 2023-02-17 at 1 00 06 pm

@dd32 dd32 merged commit 5d72721 into WordPress:master Mar 3, 2023
r-a-y added a commit to r-a-y/bp-two-factor that referenced this pull request Aug 15, 2024
No longer needed as of two-factor v0.8.0.

See WordPress/two-factor#518
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backup Codes Emailed Codes TOTP Time-based One-time Passwords
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants