Distinguish 'done' from 'configuring' in 2FA

[michielbdejong]

• 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)

[st3iny]

Test need to be fixed:

There was 1 failure:
--
264 |  
265 | 1) Test\Authentication\TwoFactorAuth\ManagerTest::testNeedsSecondFactorSessionAuthFailDBPass
266 | Expectation failed for method name is "set" when invoked 1 time(s)
267 | Parameter 0 for invocation OCP\ISession::set('two_factor_auth_configuring', 'user') does not match expected value.
268 | Failed asserting that two strings are equal.
269 | --- Expected
270 | +++ Actual
271 | @@ @@
272 | -'two_factor_auth_passed'
273 | +'two_factor_auth_configuring'
274 |  
275 | /drone/src/lib/private/Authentication/TwoFactorAuth/Manager.php:363
276 | /drone/src/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php:679

[michielbdejong]

• performance tests Errors during submodule fetch - will investigate
• Will sign off latest commit so DCO passes
• Cypress Default cloud background is not rendered seems unrelated?
• continuous-integration/drone/pr integration-contacts-menu test is failing, seems unrelated?

[st3iny]

• performance tests Errors during submodule fetch - will investigate
• Will sign off latest commit so DCO passes
• Cypress Default cloud background is not rendered seems unrelated?
• continuous-integration/drone/pr integration-contacts-menu test is failing, seems unrelated?

Failures seem to be unrelated. Please fix DCO and we should be good to go.

[michielbdejong]

DCO fixed

[miaulalala]

Drone failure is

--- Failed scenarios:
--
1366 |  
1367 | /drone/src/build/integration/features/contacts-menu.feature:27

[miaulalala]

Something is wrong with the submodule 3rd party reference hash. can you rebase on the current master?

[miaulalala]

/rebase

[mickenordin]

Any news on getting this merged?

[michielbdejong]

I think the rebase was successful and now all we're waiting for is for someone to actually click "merge", right?
CC @ChristophWurst @st3iny @miaulalala

[miaulalala]

Unfortunately the rebase wasn't successful. Can you do it @michielbdejong or do you need some help?

[mrvahedi68]

@miaulalala Tests passing now.

[michielbdejong]

@miaulalala what is the status of this PR, is it going to be merged now?

[miaulalala]

@miaulalala what is the status of this PR, is it going to be merged now?

Just checking if the changes to the .devcontainer can stay, otherwise looks good. in the meantime, can you update your branch to the latest master?

[miaulalala]

Unfortunately the .devconatiner changes have to go to keep everything tidy.

If you would like to keep the changes, you can open a second PR to update the devcontainer to 8.1 - would be nice to have that in, but not in this PR :)

[michielbdejong]

superseded by #39411