- Issue created by @cmlara
- πΊπΈUnited States cmlara
Preliminary testing with a decorator appears to indicate this would be very doable and very maintainable long term.
We do however appear to need to implement β¨ Increase security with single failure for login, password, and tfa Postponed: needs info first as we will need both the password and the token to be passed through to UserAuthInterface::authenticate() as part of the user_login_form.
This points to the most significant negative aspet of implementing this feature, that any contrib calls to UserAuthInterface::authenticate() now need to provide a TFA token (or be accepted with a LoginPlugin). The Browser plugin may help sites mitigate this concern to some degree, however that isn't a full solution.
We might need to provide a method to allow overwriting existing forms to prompt for tokens, we could possibly do that via a hook_form_alter() call where we inject our own additional field and validator A similar request is open in #3323866: Offer TFA flow for other use cases than login β that perhaps is a viable solution as well. This however could be done in a followup issue and site owners will just need not note to users (or provide their own form_alter) that login fields must also include an OTP.
- πΊπΈUnited States cmlara
β¨ Increase security with single failure for login, password, and tfa Postponed: needs info may cause bigger security concerns so we need to implement this without that change.
Using the per-request memory cache should be sufficient, though it does make it easier for 3rd party code to disable TFA during authentication. However once code has that level of control it could just login a user directly making this minimal risk from our side.
- Merge request !29Issue #3378072: Explore decorating the user.auth service β (Merged) created by cmlara
- last update
over 1 year ago 40 pass - Status changed to Needs review
over 1 year ago 9:49pm 6 August 2023 - πΊπΈUnited States cmlara
Here is the first run a this. We likely should add validateRequest onto the ValidationInterface API, however I have left that out on this changeset so it can be discussed separately. A plugin must implement validateRequest() to approve a submission through the decorator.
Sequence of operations:
TfaUserAuth is installed as a decorator to the user.auth service.When using the TfaLoginForm:
A flag is set during the validateAuthentication() stage to allow the parent call to user.auth service to bypass the TFA service. The TfaLoginForm/EntryForm will continue to validate TFA tokens using the existing workflow.When not using the TfaLoginForm:
The TfaUserAuth decorator will follow a similar workflow to the TfaLoginForm/Entryform, it will check if TFA is enabled globally and for the user, that the default plugin reports ready(), attempt to to use a login plugin, than poll the default validation plugin and finally poll all other configured validation plugins. If at any step in the flow a plugin approves the request the token portion (if present) will be stripped from the end of the password, and the new string will be passed through the rest of the user.auth decoration stack and the result from that call passed up the chain.This means that TOTP and HOTP for example could be used through the user.login.http route by appending the token to the end of the password, so that
{ "username": "user", "password": "password" }
becomes:
{ "username": "user", "password": "password123456" }
The new tokenLength() method on the API allows the submitted password string to be analyzed in case it contains variable length data that can not be known until it is submitted. Plugins are left to their own design on how they encode the token and determine its length when submitted as part of a password string.
- πΊπΈUnited States cmlara
This will also partially solve π SA-CONTRIB-2023-030 and 2.x Active
- last update
over 1 year ago 40 pass - Status changed to Fixed
about 1 year ago 9:48pm 20 November 2023 -
cmlara β
committed 803ad159 on 2.x
Issue #3378072: Decorate the user.auth service.
-
cmlara β
committed 803ad159 on 2.x
Automatically closed - issue fixed for 2 weeks with no activity.