-
-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Distinguish 'done' from 'configuring' in 2FA #39411
base: master
Are you sure you want to change the base?
Conversation
Can someone review this please? |
Is there anything blocking the merge of this? It would be great if this could be merged. |
See the open todos in the PR description |
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. |
@MahdiBaghbani and I did some more manual testing of this PR, and found out that it's more complicated than we thought:
We'll do some more manual testing and code digging to gain a better understanding. |
@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. |
Signed-off-by: MohammadReza vahedi <mr.vahedi68@gmail.com> Signed-off-by: Michiel de Jong <michiel@pondersource.com>
Signed-off-by: Michiel de Jong <michiel@pondersource.com>
6d79ece
to
d7cb86a
Compare
Hello @michielbdejong, I'm reopening your PR and requesting reviews! :) Is there anything left blocking you? We'd like to help you get this one in! |
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
Checklist