Fix isTfaDisabled

Created on 11 March 2025, 25 days ago

Problem/Motivation

TfaUserAuthenticationController::login calls TfaLoginContextTrait::isTfaDisabled()
If a user has TFA disabled TfaUserDataTrait::tfaGetTfaData retruns an empty array.
This MR fixes this. I am not sure why in isTfaDisabled the code only checks for status and plugins in the user_tfa_data.

Steps to reproduce

Disable TFA for a user and log in. In our case we login via Rest and not the normal Drupal login form. The TfaUserAuthenticationController nevertheless gets call directly.

Proposed resolution

return TRUE for isTfaDisabled if the userData is empty.

πŸ› Bug report
Status

Active

Version

1.10

Component

Code

Created by

πŸ‡¨πŸ‡­Switzerland milanbombschliip

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @milanbombschliip
  • Pipeline finished with Canceled
    25 days ago
    Total: 345s
    #445548
  • Pipeline finished with Failed
    25 days ago
    Total: 1084s
    #445554
  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    In our case we login via Rest and not the normal Drupal login form.

    I'll note per SA-CONTRIB-2023-030 REST should be disabled in 8.x-.1.x deployments. The rest login protection in 8.x-1.x is a failsafe to block access not to allow access.

    2.x has the foundations of allowing rest to function (however it is not yet suitable for deployment outside of development labs)

    I am not sure why in isTfaDisabled the code only checks for status and plugins in the user_tfa_data.

    A number of actors involved.

    IIRC I have seen times where the data did not get set for a plugin meaning we can't trust the user list and have needed to fall back to the plugins (it creates a mess for the rest of the code).

    From a 'fail secure' standpoint (ref: 🌱 [META] Convert fail-open code executions to fail-secure alternatives Active ) the we should not operate on an assumption that 'no data means not enabled'. Longer term I've theorized we likely need to have a specific record on all users that explicitly states "no data here" (eg for this case it would be something like a tfa_disabled = TRUE record, however that would need to be done in 2.x as a data model change.

Production build 0.71.5 2024