Problem/Motivation
In 34a6ca4dd5cce9e375916e50cd5b6956007918ff the code was re-factored from using a form alter to extending the UserLoginForm.
Around 5 years ago the class was marked @internal in ee0f806896ee167f382935bfb40cefec9d4ad1cb
If reverting to a form alter would allow us not to extend core classes it could make the code much more managble and allow us to reduce the maintenance burden.
Steps to reproduce
Review Code
Proposed resolution
Revert to a form alter, or form alter+service configuration.
Remaining tasks
Patch
User interface changes
None
API changes
Impacted classes are @internal.
Remove TfaUserLogin and and only decorate with form alter and validation calls
Data model changes
None
Issue fork tfa-3391784
Show commands
Start 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:
Comments
Comment #3
cmlaraUploaded a prototype that was based on some code from last year along with some newer edits of what converting to a form alter could look like.
Comment #4
cmlaraThis ends up being larger than I expected it to be. In hindsight it makes sense as much of the TfaUserLoginForm was our unique code.
This does however have the advantage that it should remain generally compatible with core, and I believe contrib as well. The largest advantage is we are no longer extending an internal core class.
The negative is we are tied to the form_state UID value usage.
Key Design Concepts:
All this is backed up by TfaUserSetSubscriber preventing logins occurring during the validation or submit stage for a user who needs to visit the TfaEntryForm.
If tfaLoginFormPreValidation() is removed by another module users can still log in similar to REST by providing token at the end of the password (failsafe to broken site).
Comment #5
cmlaraNote: This currently doesn't work with mail_login as TFA needs to know the account name during the pre_auth stage.
Drupal\user\UserAuthenticationInterface::lookupAccount()could resolve this, although it will not be until D12 that it is fully implemented.I would suggest #3497020: Implement Drupal\user\UserAuthenticationInterface handle that scenario after this issue is merged (unless that issue is merged first).
Comment #6
cmlaraWe likely need a post_update hook to remove/convert the Tfa User Login block.
Comment #7
cmlaraCreated CR: https://www.drupal.org/node/3527521
Added admin facing documentation in GitLab Pages format.
Comment #8
cmlara