Distinguish 'done' from 'configuring' in 2FA

[mrvahedi68]

• Resolves: Confusing variable name in 2FA #35554

Summary

When there is a token in the session for which the user is still setting up 2FA, setting self::SESSION_UID_DONE ("two_factor_auth_passed") is a misnomer.

AFAICT, everything works fine if you set nothing into the session and just return 'false' from this if-statement, but in case there is some code (now or in the future) that needs to know if the user is configuring 2FA, to play it safe I would suggest storing self::SESSION_UID_CONFIGURING ("two_factor_auth_configuring") into the session.

TODO

• add tests
• test manually
• consider removing this session variable once configuration is successfully completed

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[michielbdejong]

Can someone review this please?
@miaulalala you already gave this code a "looks good", this is the rebase you requested.

[mickenordin]

Is there anything blocking the merge of this? It would be great if this could be merged.

[ChristophWurst]

See the open todos in the PR description

[ChristophWurst]

For the tests: let's also add one that tests the change of session data IDs during an authentication. That can happen when an instance is upgraded in the middle of a user's authentication attempt.

[michielbdejong]

@MahdiBaghbani and I did some more manual testing of this PR, and found out that it's more complicated than we thought:

• It seems the needsSecondFactor function is only called during 2FA configuring, not during login/check where you would expect it to be called
• We cannot figure out what code comment "If the session is expired check if we are not logged in by a token that still needs 2FA auth" means
• If you log in through 2FA and then disconfigure the TOTP app, the current session is still 2FA done, and the code block where we wanted to add the 'SESSION_UID_CONFIGURING' isn't run. So it's not as clean-cut as we had previously thought.
• It's not clear to us where we can unset 'SESSION_UID_CONFIGURING' - we think it's in verifyChallenge but we're also not v ery confident that we understand when this function is called and when it's not.

We'll do some more manual testing and code digging to gain a better understanding.

[michielbdejong]

For the tests: let's also add one that tests the change of session data IDs during an authentication. That can happen when an instance is upgraded in the middle of a user's authentication attempt.

@ChristophWurst good point! If this change is applied on a server while someone is in the middle of configuring 2FA, they will still have the old behaviour at the start of the process, and they will have the new behaviour by the time they finish, meaning it will set _DONE at the beginning and then silently fail to remove _CONFIGURING at the end so that's fine.

The way the unit tests are organised, it only checks that the Mock receives an attempt to remove the _CONFIGURING session data item, so there's not really much more to test there.

I added the code that removes the _CONFIGURING key, and additional unit test code for it.

I'm now doing some more manual testing but other than that I think this is all done. I can't mark the checkboxes at the top because this PR is authored by @mrvahedi68 - maybe he can mark the checkboxes, or if needed we can create a follow-up PR with up-to-date task status info.