Fix reliability of storing status in user data

Created on 8 July 2025, 27 days ago

Problem/Motivation

Per conversation in https://git.drupalcode.org/project/tfa/-/merge_requests/150#note_547475 (as far as I understand it) there is a worry that status is not reliable based on whether or not plugins are enabled.

Steps to reproduce

From @cmlara:

Looked through the rest of TfaContextTest and did see a trigger on TfaContextTest::hasSkipped() which is a bit interesting i would not have anticipated that, it is related to no data being set in the mocks, tfaSaveTfaData() defaults $status =1 (fail closed) which results in the status being stored as 'enabled' even though none of the expected TFA Setup has occurred (this goes back to the lack of trust in the the status field)

Proposed resolution

TBD

Remaining tasks

TBD

User interface changes

N/A

API changes

N/A

Data model changes

N/A

πŸ“Œ Task
Status

Active

Version

2.0

Component

Code

Created by

πŸ‡¬πŸ‡§United Kingdom scott_euser

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

Merge Requests

Comments & Activities

  • Issue created by @scott_euser
  • πŸ‡¬πŸ‡§United Kingdom scott_euser

    I'm sure its not right, but sort of proof of concept...

    Bit beyond me as the code there is a little hard to follow how different scenarios get stored as user data in different forms

  • πŸ‡¬πŸ‡§United Kingdom scott_euser

    You can see this passing combined with ✨ Provide an alter for user settings Active at https://git.drupalcode.org/issue/tfa-3532729/-/pipelines/541506 but I subsequently reverted

  • Pipeline finished with Success
    27 days ago
    Total: 386s
    #541515
  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    Filling in a bit more info:

    In ✨ Provide an alter for user settings Active I noted the status field was known to 'get out of sync'. I belive I partially was confusing a diffrent scenario where a plugin is listed as configured when isReady() will return false. However it is also likely this code may have caused some confusion in past where we see STATUS=1 with no plugins configured.

    In ✨ Provide an alter for user settings Active we found that when saving TFA skip count for a user that has not configured TFA (would occur where the user has a required role and has not yet provisioned a token) that the the user.data is set to status=1 indicating TFA is enabled.

    This was first noted by @msypes in https://www.drupal.org/project/tfa/issues/2958950#comment-14922059 ✨ Provide TFA enabled status field via views Fixed

    This is questionable since the user has not actually configured TFA, however one could also argue that TFA is enabled due to the fact 'TFA is enforcing a skip count' and that the user is 'within policy' at the point even though no token is required.

    This caused the followup issue of πŸ› Views "TFA status" field wrong output Active to be opened.

    First question we need to ask is do we want skipped counts being set without enabled plugins to be considered 'TFA Enabled' or 'TFA Disabled' for the backend status field as that impacts our operations choice.

    Related, we need to analyze how this would impact the code if we choose disabled, will enforcement of users change, if so in what way and will it be acceptable.

    Currently there is a heavy usage of isReady() against all plugins which may render any immediate concern about this value of little importance, however long term we do want to get to a state where we have a reliable 'is TFA enabled" indicator which we need to consider for this code that 'enabled' may be the appropriate value

    Also related, since this is a default value, is what impact this has on 'fail secure' operations of the code as we work towards 🌱 [META] Convert fail-open code executions to fail-secure alternatives Active . At the moment it sis also likely backed up by isReady() checks however those are heavy operational cost and ideally we would eventually get away from them as this has caused other features such as ✨ Force user to setup TFA when required and there are no remaining skips Needs work to be harder to implement.

  • πŸ‡¬πŸ‡§United Kingdom scott_euser

    First question we need to ask is do we want skipped counts being set without enabled plugins to be considered 'TFA Enabled' or 'TFA Disabled' for the backend status field as that impacts our operations choice.

    Maybe that is a separate issue as a follow-up: let the site builder decide with a setting, and provide a default for new installs + update hook to maintain status quo. Feels like for the scope of this issue the status quo should remain; if skip counts don't care if TFA is enabled or not, this issue should not chain that behaviour right?

  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    Maybe that is a separate issue as a follow-up

    To me this reads as a data model design decision that impacts how all code should written to interpret that value, I can't see us allowing this to be a site owner choice, and really we just need to decide how/when this value should be set to enabled based on what it is intended to indicate.

    There are two different scenarios I'm looking at here that are useful for a dedicated field in the schema:

    • A need to know when TFA has been configured for a user at any time (even if the plugins have been deleted or all plugins report that they are not ready)
    • A need to know the user will be compelled to pass TFA validation, I don't see this as including 'skiped' logins as no validation is actually provided.

    The status field currently provides neither of those. It could however provide the base for both of those two scenarios if when set to TRUE would mean canLoginWithoutTfa() is compelled to return FALSE.

    If we go with that logic we would indeed want to stop setting this to TRUE when skip counts are saved.

    Technical notes:
    Analyzing this set of issues I now see why in the past we have had the TFA list of plugins disagree with the isReady() call to plugins, we purge the data for TFA namespace which deletes TOTP/HOTP/Recovery Codes/Trusted Browser, however 3rd party plugins will not be deleted. This likely explains of our data sync issues experienced in the past of plugins being able to report ready when not listed in TFA, they were most likely contrib plugins.

Production build 0.71.5 2024