Distinguish 'done' from 'configuring' in 2FA #35555

michielbdejong · 2022-12-02T09:33:35Z

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 · 2022-12-02T11:04:28Z

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 · 2023-01-09T12:16:45Z

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 · 2023-01-09T12:30:59Z

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 · 2023-01-10T11:25:45Z

DCO fixed

miaulalala · 2023-01-16T13:59:08Z

Drone failure is

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

miaulalala · 2023-01-16T14:01:59Z

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

miaulalala · 2023-01-23T12:21:41Z

/rebase

mickenordin · 2023-04-03T11:39:27Z

Any news on getting this merged?

michielbdejong · 2023-04-05T06:45:26Z

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 · 2023-04-06T21:36:50Z

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

mrvahedi68 · 2023-04-17T07:52:49Z

@miaulalala Tests passing now.

…rrent implementation uses php 7.4, which is no longer compatible with the required PHP version of the server. I upped this to PHP 8.1 List of changes: - Upped PHP Version to 8.1 - Added Apache Webserver so the Container works "out of the box" after docker-compose up -d - Mounting whole project as volume to /var/www/html in docker-compose.yml (and set WORKDIR to /var/www/html) Tested in a Docker for Windows environment. Signed-off-by: MohammadReza vahedi <[email protected]>

* Autostart apache2 * Apply occ installation on start * Autostart Xdebug on request * Add DevContainer Xdebug profile Signed-off-by: GitHub <[email protected]> Signed-off-by: MohammadReza vahedi <[email protected]>

Signed-off-by: GitHub <[email protected]> Signed-off-by: MohammadReza vahedi <[email protected]>

* Add gnupg2 to be able to sign commits * Make sure /var/www/html always belongs to www-data * Add Git-History plugin * Introduce dedicated entrypoint script * Store Postgres database data in volume to be persistent * Cleaner check if NC is already installed in setup.sh * Add composer to DevContainer Signed-off-by: GitHub <[email protected]> Signed-off-by: MohammadReza vahedi <[email protected]>

* Use dedicated DevContainer user to run Apache (ensure file permissions) * Install NVM for node Signed-off-by: GitHub <[email protected]> Signed-off-by: MohammadReza vahedi <[email protected]>

Signed-off-by: GitHub <[email protected]> Signed-off-by: MohammadReza vahedi <[email protected]>

Signed-off-by: MohammadReza vahedi <[email protected]>

When there is a token in the session for which the user is still [setting up 2FA](https://raw.githubusercontent.com/nextcloud/twofactor_totp/master/screenshots/settings.png), 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. Signed-off-by: Michiel de Jong <[email protected]> Signed-off-by: MohammadReza vahedi <[email protected]>

Signed-off-by: MohammadReza vahedi <[email protected]>

michielbdejong · 2023-06-12T08:35:13Z

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

miaulalala · 2023-06-12T09:17:10Z

@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 · 2023-07-17T06:59:17Z

superseded by #39411

szaimen requested a review from ChristophWurst December 2, 2022 10:41

szaimen added the 3. to review Waiting for reviews label Dec 2, 2022

szaimen added this to the Nextcloud 26 milestone Dec 2, 2022

szaimen requested review from st3iny and miaulalala December 2, 2022 10:42

szaimen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Dec 3, 2022

This was referenced Jan 9, 2023

Add "MFA Verified" check to workflowengine #36045

Closed

Milestone 4 wrap-up pondersource/nextcloud-mfa-awareness#36

Closed

michielbdejong force-pushed the patch-1 branch from 8bec328 to 681fae0 Compare January 9, 2023 12:52

michielbdejong force-pushed the patch-1 branch from 681fae0 to 0b507ae Compare January 16, 2023 14:26

blizzz mentioned this pull request Feb 1, 2023

26.0.0 beta 2 #36484

Merged

skjnldsv mentioned this pull request Feb 23, 2023

26.0.0 beta 5 #36834

Merged

blizzz mentioned this pull request Mar 7, 2023

26.0.0 RC2 #37078

Merged

blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023

This was referenced Mar 9, 2023

Wrap up milestone 5 pondersource/nextcloud-mfa-awareness#38

Closed

Add "MFA Verified" check to workflowengine #37195

Closed

michielbdejong force-pushed the patch-1 branch 2 times, most recently from fff6ec5 to 81ba258 Compare April 10, 2023 14:45

Stephan and others added 12 commits April 19, 2023 16:03

Minor improvements for devcontainer

0046cd8

* Autostart apache2 * Apply occ installation on start * Autostart Xdebug on request * Add DevContainer Xdebug profile Signed-off-by: GitHub <[email protected]> Signed-off-by: MohammadReza vahedi <[email protected]>

Use ubuntu jammy and cleanup Dockerfile

df0ba7d

Signed-off-by: GitHub <[email protected]> Signed-off-by: MohammadReza vahedi <[email protected]>

Dedicated DevContainer user + NVM

6677e22

* Use dedicated DevContainer user to run Apache (ensure file permissions) * Install NVM for node Signed-off-by: GitHub <[email protected]> Signed-off-by: MohammadReza vahedi <[email protected]>

Configure line ending to let shell scripts work in Windows ENV

f3046ed

Signed-off-by: GitHub <[email protected]> Signed-off-by: MohammadReza vahedi <[email protected]>

Move git safe.directory to earlier stage

ec4f3ff

Signed-off-by: GitHub <[email protected]> Signed-off-by: MohammadReza vahedi <[email protected]>

Add .vscode to expected files list

b4372f6

Signed-off-by: MohammadReza vahedi <[email protected]>

Enbale apcu in devcontainer CLI

b35db66

Signed-off-by: MohammadReza vahedi <[email protected]>

Move launch.json to .devcontainer and copy on start

0867c66

Signed-off-by: MohammadReza vahedi <[email protected]>

fix test failing in drone

9bad67e

Signed-off-by: MohammadReza vahedi <[email protected]>

mrvahedi68 force-pushed the patch-1 branch from 195f35a to 9bad67e Compare April 19, 2023 16:04

This was referenced Apr 25, 2023

Distinguish 'done' from 'configuring' in 2FA #37913

Closed

Add "MFA Verified" check to workflowengine #37914

Closed

This was referenced May 3, 2023

27.0.0 beta 1 #38038

Merged

27.0.0 beta 2 #38137

Closed

27.0.0 beta 2 #38139

Merged

mickenordin mentioned this pull request May 14, 2023

Provide a tar ball pondersource/nextcloud-mfa-awareness#55

Closed

blizzz mentioned this pull request May 17, 2023

27.0.0 RC1 #38341

Merged

blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023

miaulalala requested changes Jun 12, 2023

View reviewed changes

thepeak99 mentioned this pull request Jun 15, 2023

Shepherd NC PR #39411 pondersource/nextcloud-mfa-awareness#56

Closed

michielbdejong closed this Jul 17, 2023

michielbdejong mentioned this pull request Aug 31, 2023

Distinguish 'done' from 'configuring' in 2FA #39411

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Distinguish 'done' from 'configuring' in 2FA #35555

Distinguish 'done' from 'configuring' in 2FA #35555

michielbdejong commented Dec 2, 2022

st3iny commented Dec 2, 2022

michielbdejong commented Jan 9, 2023

st3iny commented Jan 9, 2023

michielbdejong commented Jan 10, 2023

miaulalala commented Jan 16, 2023

miaulalala commented Jan 16, 2023

miaulalala commented Jan 23, 2023

mickenordin commented Apr 3, 2023

michielbdejong commented Apr 5, 2023

miaulalala commented Apr 6, 2023

mrvahedi68 commented Apr 17, 2023

michielbdejong commented Jun 12, 2023

miaulalala commented Jun 12, 2023

miaulalala left a comment

michielbdejong commented Jul 17, 2023

Distinguish 'done' from 'configuring' in 2FA #35555

Distinguish 'done' from 'configuring' in 2FA #35555

Conversation

michielbdejong commented Dec 2, 2022

Summary

TODO

Checklist

st3iny commented Dec 2, 2022

michielbdejong commented Jan 9, 2023

st3iny commented Jan 9, 2023

michielbdejong commented Jan 10, 2023

miaulalala commented Jan 16, 2023

miaulalala commented Jan 16, 2023

miaulalala commented Jan 23, 2023

mickenordin commented Apr 3, 2023

michielbdejong commented Apr 5, 2023

miaulalala commented Apr 6, 2023

mrvahedi68 commented Apr 17, 2023

michielbdejong commented Jun 12, 2023

miaulalala commented Jun 12, 2023

miaulalala left a comment

Choose a reason for hiding this comment

michielbdejong commented Jul 17, 2023