Period: last_verified_step is stored in units of the code period that was active when the code was checked. TotpService::verifyAndGetStep() calculates the step with the current period, and UserTotpManager::matchesTotpCode() rejects any step <= last_verified_step. If the period is raised (for example from 30 to 60 seconds), the current step number roughly halves, so every TOTP code is rejected, practically forever.
Length: changing otp_digits breaks every existing enrollment, because authenticator apps keep generating codes of the old length.
The settings form allows both changes without any warning.
Generated with the help of an LLM.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | 2026-11-05-08-51-summary.md | 4.8 KB | jurgenhaas |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #4
mugesh.s commentedI've opened MR !4 for this.
Approach
I went with the first proposed option: store the code length and validity period with each enrollment and verify that user's codes with those values. Changing the settings now only affects new enrollments.
Storing the last-verified timestamp alone wouldn't fix the lockout. Authenticator apps keep the period and digits from the
otpauth://URI they scanned at enrollment. After a period change, the app and the server compute codes for different time steps, so no code matches even without the replay check.Changes
auth_login_plus_update_8003()addsotp_digitsandotp_periodcolumns toauth_login_plus_user_totpand fills existing enrollments with the current settings.UserTotpManager::confirmEnrollment()stores the values, andmatchesTotpCode()verifies with them.last_verified_stepnow always uses the same period, so replay protection works as before.TotpService::getProvisioningUri()andverifyAndGetStep()take digits and period as optional arguments.UserEnrollFormkeeps the values with the pending secret, so the QR code and the confirmation match even if an administrator changes the settings in between.SettingsFormnow notes that these values only apply to new enrollments. I didn't add a warning or block, because changing them no longer affects enrolled users.Tests
A new Kernel test,
UserTotpManagerParametersTest, covers:Known limitation
On sites that already changed these settings before this update, the update hook stores the current values. Users who enrolled under the old values can't be recovered automatically and still need an admin reset or re-enrollment.
@jurgenhaas, could you review, especially the update hook and the fallback behaviour?
Comment #5
jurgenhaasReviewed MR !4 at a96521a. The approach is right: authenticator apps keep the digits and period from the scanned URI, so storing them per enrollment is the real fix. Replay protection stays correct because
last_verified_step and verification now use the same period. The pipeline is green, and the phpunit job ran all four new kernel tests.
The update hook is idempotent through fieldExists() and the otp_period = 0 guard. Backfilling from current config is the only option, since nothing else records the old values. The runtime fallback mostly
matters between deploy and updb: the columns don't exist yet, so logins keep using config as before. The hook has no coverage, though. Please add a kernel test that drops both columns, inserts a legacy row and
runs auth_login_plus_update_8003(). A second run after changing the config should leave the stored values untouched.
Optional: for the case in this issue, a raised period, rows with last_verified_step above intdiv(time(), otp_period) + otp_window are provably stale. The hook could report their count, so admins know whom to
reset. Nits: named arguments would read better than the positional NULL in the verifyAndGetStep() calls. The drift tolerance seconds in SettingsForm now only hold for users enrolled with the current period.
More detail is in the attached 2026-11-05-08-51-summary.md.
AI-Generated: Yes (agent reviewed the MR diff and CI results and drafted this comment; reviewed by jurgenhaas).
Comment #6
mugesh.s commentedThanks for the thorough review, @jurgenhaas.
I've pushed the following to MR !4:
UpdateHook8003Testdropsotp_digitsandotp_period, inserts a legacy row and runsauth_login_plus_update_8003(). It checks that both columns exist and the row gets the configured values, then changes the config, runs the hook again and checks the stored values are unchanged.last_verified_stepis beyondintdiv(time(), otp_period) + otp_window, with their user IDs and the admin page to reset them. The test covers a period raised from 30 to 60 seconds, plus the case with nothing to report. As you noted, a lowered period or a changed code length can't be detected this way.digits:,period:) in bothverifyAndGetStep()calls.SettingsFormnow says the seconds only apply to users enrolled with the current validity period.The pipeline is green. Setting back to Needs review.