- Issue created by @scott_euser
- Merge request !152#3534600: Fix reliability of storing status in user data β (Open) 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
- πΊπΈ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.