- Issue created by @delzhand
- 🇺🇸United States cmlara
Login will fail using REST, but succeed when using the drupal login form using the same credentials.
Officially we do not support REST authentication in the 8.x-1.x branch.
TfaUserAuthenticationController’s job is to shut down the route in response to SA-CONTRIB-2023-030 to partially mitigate the risk. It was never intended to be a “works everywhere” class rather it was built as extra mitigation for when sites fail to disable REST.
The 2.x branch has had a significant change in architecture to allow us to support REST that can. If reasonable be backported to the 1.x API.
I’m inclined to won’t fix this on 1.x as it would add some extra complications for a feature that we can not properly support.
If we break still in 2.x there would be an issue for us to evaluate.
- 🇺🇸United States delzhand
I forgot an important aspect of this - what this MR allows is for users who DON'T have TFA enabled to log in via REST. My particular use case is that TFA is only required for certain privileged roles, while the vast majority of users do not have TFA enabled. The one place I'm using REST is exclusively for those less privileged users.
- 🇺🇸United States cmlara
The one place I'm using REST is exclusively for those less privileged users.
The problem is that our architecture is fundamentally flawed in the 1.x branch. We can not say that TFA will be applied on anything except the login form, and even that honestly is not guaranteed given issues we have found (2.x attempts to fix). My statement as a co-maintainer is "If you have REST enabled you should assume TFA is not enforced for any user, especially privileged users". You might not have any attack points in that regard however as a co-maintainer the only way I can even remotely stand behind the code for the 1.x branch is if REST is disabled.
https://project.pages.drupalcode.org/tfa/technical/set-user-protection/ discusses some of the technical points we needed to target in order to be able to properly secure down REST authentication in 2.x.
The code in TfaLoginController should NOT be called. It is a a partial (not a full) safety fence. Hence we really do not want to be doing much in the way of "improvements" to it.
As for the MR itself: A 10 second review has me questioning the lack of flood protection, and wondering what other risks might be applicable to trying to call authenticate() at that portion of the stack. As noted above we wouldn't want to expend effort here on 1.x given that we expect sites to disable rest and for this code to never be called.
- 🇺🇸United States cmlara
Closing as won't-fix per above notes that 8.x-1.x is not REST safe.