Use an EventSubscriber to protect one time login links

📌 Move TfaLoginContextTrait back to a dedicated Class and add factory. Active would make implementing this easier, primarily from a testing point of view less data would need to be mocked.

Opened MR,

This will use an event subscriber to protect the One Time Login route

Important notes:
I'm not 'burning' the One Time when they get redirected to the entry form, instead allowing the existing Core code to do so after login. I believe this is reasonable. Justification for this being the link isn't actually used just by having the link, you still need the second factor to prove your using it. This also helps out with avoiding 🐛 Bingpreview invalidates one time login links Active

There is no flood protection on the OTP links. I'll leave this one for discussion. We currently match core's lack of having a flood protection on the password reset link, however I wonder if we should go more in depth than core and protect these paths as well.

Protection was added in 🐛 Installing contrib modules can lead to TFA accidently being bypassed Fixed .

This issue only needs to allow access for users which do not require TFA and redirects users that require TFA to the EntryForm.

The new implementation should be "Fail Secure" as tampering with the new code or adding alternative routes will not allow access.

elaman → changed the visibility of the branch 3392427-use-an-eventsubscriber-spr to hidden.

@cmlara I've been working on checking if Simple Reset Password can be made to work with TFA. Created draft MR based on MR, please feel free to check it let me know what needs can be improved to include it in your MR. Thank you

@elaman My goal in referring you to this issue was to see if there are any changes that does not require implementing module specific workarounds.

If you can see any other changes to !57 based on your knowledge of Simple Password Resets order of operations that would allow it to function without specific knowledge I would be willing to look at those in this issue.

If there is no way to allow Simple Password Reset to work without module specific knowledge we will need to return back to ✨ Support Simple Password Reset module Postponed .

Will need to evaluate if this requires any changes to align with SA-CONTRIB-2024-043

Hey all,

Just wondering on the status of this ticket. It looks like there's been no movement on MR57 since early Feb. Should this be in a Needs Review state? That is something I can contribute to. :)

Was reviewing this a week ago, the main change I wanted was to make classes final. I have pushed those up today.

This could use additional eyes.

Nice. Functionally, this is doing the job. :)

I ran it through phpcs and it only raised one minor issue. I don't see that as being enough of a reason to prevent this from being marked RTBC though.

Thanks @cmlara for the effort on this.

Hey all,

Just back from DrupalSouth and checking up on issues. Just wondering if there is anything I can to do help push this one over the line?

Cheers,
Gold

Was primarily allowing a little more time for anyone who might want to provide feedback since this decently significant code.

This is one the areas that has had security vulnerabilities in the past, including 2 weeks ago (note: the report that triggered two weeks ago would not have been allowed either with existing 2.x branch or the 2.x branch and this code.

That said I believe this is ready to go in and anything else can be followups.

Letting final test run occur after removing unused code and will merge.

Regarding Simple Password Reset:
Recent data indicates Password Reset Landing Page (PRLP) may be better aligned to API and provides a similar feature set. Currently PRLP does not work with this code however PRLP may be in a better spot than Simple Password Reset to modify their process to work with TFA.

Automatically closed - issue fixed for 2 weeks with no activity.

cmlara → closed merge request !90