Fix reliability of storing status in user data

Created on 8 July 2025, 5 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
    5 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.

Production build 0.71.5 2024