Skip to content

AO3-7442 Add TOTP 2FA to users - #5823

Open
EchoEkhi wants to merge 13 commits into
otwcode:masterfrom
EchoEkhi:AO3-7442
Open

AO3-7442 Add TOTP 2FA to users#5823
EchoEkhi wants to merge 13 commits into
otwcode:masterfrom
EchoEkhi:AO3-7442

Conversation

@EchoEkhi

Copy link
Copy Markdown
Contributor

Pull Request Checklist

Issue

https://otwarchive.atlassian.net/browse/AO3-7442

Credit

EchoEkhi (He/Him)

@github-actions github-actions Bot added Has Migrations Contains migrations and therefore needs special attention when deploying Awaiting Review labels May 18, 2026
@pmonfort
pmonfort self-requested a review August 6, 2026 04:21

@pmonfort pmonfort left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for working on this! I left a few comments

Comment thread app/controllers/users/sessions_controller.rb Outdated
Comment thread app/controllers/users/sessions_controller.rb Outdated
Comment thread app/controllers/users/sessions_controller.rb
Comment thread app/views/users/totp/reauthenticate_create.html.erb Outdated
Comment thread app/views/users/totp/reauthenticate_create.html.erb Outdated
Comment thread spec/controllers/users/totp_controller_spec.rb Outdated
Comment thread app/controllers/users/totp_controller.rb Outdated

session[:otp_user_id] = user.id

session[:pwned] = user.respond_to?(:password_pwned?) && user.password_pwned?(user_params[:password]) if params[:user] && params[:user][:password]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you try to keep lines to 80 characters max? There are several lines over 100 characters in the PR.

@lydia-theda lydia-theda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Back in May I took a quick look at the message wording so here are those comments.

Comment thread app/models/user.rb Outdated

serialize :otp_backup_codes, type: Array, coder: YAML, yaml: { permitted_classes: [String] }

# Generate a TOTP secret it it does not already exist

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Generate a TOTP secret it it does not already exist
# Generate a TOTP secret if it does not already exist

Comment thread config/locales/controllers/en.yml Outdated
create:
incorrect_code: Incorrect verification code. Your code may have expired, or you may need to set up your authenticator app again.
page_title: Two-Step Verification Backup Codes
success: Successfully enabled two-step verification; please make note of your backup codes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In the interest of being explicitly clear to users, I think we should change "make note of" to more closely mirror the language used elsewhere about saving the backup codes. Something more like this?

(I haven't checked whether .write_codes_down appears on this same page, or if this particular phrasing will conflict with anything else. But a word like "save" is more clear than "note".)

Suggested change
success: Successfully enabled two-step verification; please make note of your backup codes.
success: Two-step verification was successfully enabled. Please save your backup codes in case you lose access to your authenticator app.

Comment thread config/locales/controllers/en.yml Outdated
reset_backup_codes:
not_enabled: Please enable two-step verification first.
page_title: Two-Step Verification Backup Codes
success: Successfully reset your backup codes; please make note of your new backup codes, your old backup codes will no longer work.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

An initial attempt at aligning the wording with other messages across the site:

Suggested change
success: Successfully reset your backup codes; please make note of your new backup codes, your old backup codes will no longer work.
success: Your backup codes have been successfully regenerated. Please save your new backup codes, as your old backup codes will no longer work.

Comment thread config/locales/views/en.yml Outdated
heading: About Two-Step Verification
app_setup:
heading: 'Step 1: Set up your authenticator app'
instructions: Use your authenticator app to scan the QR code or enter the manual setup key. If you are using the same device the app is installed on, click the import settings button. The app will give you a 6-digit code, which you'll use in the next step.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's try to avoid the use of plain "app", because there are many people who use that word to refer to AO3.

Suggested change
instructions: Use your authenticator app to scan the QR code or enter the manual setup key. If you are using the same device the app is installed on, click the import settings button. The app will give you a 6-digit code, which you'll use in the next step.
instructions: Use your authenticator app to scan the QR code or enter the manual setup key. If you are using the same device your authenticator app is installed on, click the import settings button. The authenticator app will give you a 6-digit code, which you'll use in the next step.

Comment thread config/locales/views/en.yml Outdated
qr_scan_note: Scan with your authenticator app
enter_code:
heading: 'Step 2: Enter your 6-digit code'
instructions: Enter the 6-digit code from your app to enable two-step verification.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
instructions: Enter the 6-digit code from your app to enable two-step verification.
instructions: Enter the 6-digit code from your authenticator app to enable two-step verification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Coder Has Actioned Review Has Migrations Contains migrations and therefore needs special attention when deploying

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants