Stateful 2fa providers #9632

ChristophWurst · 2018-05-28T12:09:48Z

This change makes two-factor providers and their association with users (whether the provider is enabled or not) stateful as in that we don't actually require the provider to be loaded when we want to check if a user uses 2FA. This greatly improves performance as we don't have to read from the file system and parse the info.xml file.

To do:

Basic implementation
Adapted/added tests
Squashed commits
Adapt 2fa apps to use the IRegistry to propagate their enabled/disabled state
- twofactor_u2f
- twofactor_totp
- twofactor_sms
- others?
~~Proper handling of the backup codes app (auto-enable when other providers get enabled? auto-disable when all other providers get disabled?)~~ won't change that for now and keep the existing logic (app has to be configured by user).

cc @rullzer

codecov · 2018-06-04T09:50:43Z

Codecov Report

Merging #9632 into master will decrease coverage by <.01%.
The diff coverage is 56.92%.

@@             Coverage Diff              @@
##             master    #9632      +/-   ##
============================================
- Coverage      52.1%   52.09%   -0.01%     
- Complexity    25910    25949      +39     
============================================
  Files          1642     1648       +6     
  Lines         95721    95862     +141     
  Branches       1289     1289              
============================================
+ Hits          49871    49943      +72     
- Misses        45850    45919      +69

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Server.php	`82.24% <ø> (-0.18%)`	`278 <0> (-1)`
core/templates/twofactorselectchallenge.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
core/Command/TwoFactorAuth/State.php	`0% <0%> (ø)`	`11 <11> (?)`
core/Migrations/Version14000Date20180522074438.php	`0% <0%> (ø)`	`4 <4> (?)`
core/register_command.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
version.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
core/Controller/LoginController.php	`79.02% <100%> (ø)`	`38 <0> (ø)`	⬇️
.../private/Authentication/TwoFactorAuth/Registry.php	`100% <100%> (ø)`	`4 <4> (?)`
...ivate/Authentication/TwoFactorAuth/ProviderSet.php	`100% <100%> (ø)`	`5 <5> (?)`
...b/private/Authentication/TwoFactorAuth/Manager.php	`80% <74%> (+1.95%)`	`36 <13> (+1)`	⬆️
... and 11 more

ChristophWurst · 2018-06-18T09:16:44Z

This is ready for review now.

Steps to test:

Enable a 2fa provider app
Configure the 2fa provider
Log in once
Log out
Disable the app again
Try to log in

Before:
You could still log in.

Now:
You see a warning that at least one provider failed to load and you cannot complete the login.

ChristophWurst · 2018-06-18T14:32:55Z

failure 1

There were 2 warnings:

Tests\Core\Controller\LoginControllerTest::testLoginWithOneTwoFactorProvider
Trying to configure method "getProviders" which cannot be configured because it does not exist, has not been specified, is final, or is static

Tests\Core\Controller\LoginControllerTest::testLoginWithMultpleTwoFactorProviders
Trying to configure method "getProviders" which cannot be configured because it does not exist, has not been specified, is final, or is static

failure 2

Test\Authentication\TwoFactorAuth\Db\ProviderUserAssignmentDaoTest::testPersist
Failed asserting that 0 is identical to 1.

/drone/src/github.com/nextcloud/server/tests/lib/Authentication/TwoFactorAuth/Db/ProviderUserAssignmentDaoTest.php:93

ChristophWurst · 2018-06-19T06:01:29Z

tests/lib/Authentication/TwoFactorAuth/Db/ProviderUserAssignmentDaoTest.php

+			->andWhere($qb->expr()->eq('enabled', $qb->createNamedParameter(0)));
+		$res = $q->execute();
+		$cnt = $res->rowCount();
+		$this->assertSame(1, $cnt);


For some weird reason the query returns 0 rows here. I cannot find out why that is the case. If I attach a debugger an step through the code I see that the ->persist call inserts one row.

@rullzer did I use the query builder incorrectly?

Rowcount is the number of rows affected by insert/update/delete

Either do 'fetchAll' and count on that. Or use the count function in the query. Both should work just fine here.

I should have read the method docs. Makes sense, updated my code and tests seem to pass locally. Thanks a lot!

MorrisJobke · 2018-06-19T08:20:27Z

version.php

@@ -29,7 +29,7 @@
 // between betas, final and RCs. This is _not_ the public version number. Reset minor/patchlevel
 // when updating major/minor version number.

-$OC_Version = array(14, 0, 0, 4);
+$OC_Version = array(14, 0, 0, 5);


Needs to be changed again, because another PR with the same change got merged.

Yep #9485 (comment)

rullzer · 2018-06-09T17:25:55Z

core/Migrations/Version14000Date20180522074438.php

+	 * @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper`
+	 * @param array $options
+	 */
+	public function preSchemaChange(IOutput $output, Closure $schemaClosure,


rullzer · 2018-06-09T17:26:02Z

core/Migrations/Version14000Date20180522074438.php

+	 * @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper`
+	 * @param array $options
+	 */
+	public function postSchemaChange(IOutput $output, Closure $schemaClosure,


rullzer · 2018-06-19T13:45:43Z

tests/lib/Authentication/TwoFactorAuth/Db/ProviderUserAssignmentDaoTest.php

+			->andWhere($qb->expr()->eq('enabled', $qb->createNamedParameter(0)));
+		$res = $q->execute();
+		$cnt = $res->rowCount();
+		$this->assertSame(1, $cnt);


Rowcount is the number of rows affected by insert/update/delete

Either do 'fetchAll' and count on that. Or use the count function in the query. Both should work just fine here.

rullzer · 2018-06-19T13:47:51Z

Very cool stuff. I need to test it properly but code makes a lot of sense

ChristophWurst · 2018-06-20T06:09:28Z

Let's see if CI likes the current state and if so, I'll squash my commits one more time.

This adds persistence to the Nextcloud server 2FA logic so that the server knows which 2FA providers are enabled for a specific user at any time, even when the provider is not available. The `IStatefulProvider` interface was added as tagging interface for providers that are compatible with this new API. Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

MorrisJobke · 2018-06-20T13:02:21Z

Let's see if CI likes the current state and if so, I'll squash my commits one more time.

And now? xD

ChristophWurst · 2018-06-20T13:46:53Z

And now? xD

Squashed, rebased and CI is happy. Time to review :)

Will fix the 2fa providers soonish so that they propagate their state automatically.

rullzer

Cool stuff!!!

rullzer · 2018-06-21T12:24:22Z

lib/private/Authentication/TwoFactorAuth/ProviderLoader.php

@@ -0,0 +1,88 @@
+<?php
+


rullzer · 2018-06-21T12:25:38Z

lib/private/Authentication/TwoFactorAuth/ProviderLoader.php

+	 * @param string $appId
+	 */
+	protected function loadTwoFactorApp(string $appId) {
+		if (!OC_App::isAppLoaded($appId)) {


Can't we use the appmanager here?

Neither the interface, nor the implementation provide methods to load an app.

rullzer · 2018-06-21T12:25:45Z

lib/private/Authentication/TwoFactorAuth/ProviderSet.php

@@ -0,0 +1,71 @@
+<?php
+


Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

MorrisJobke · 2018-06-25T09:44:50Z

core/templates/twofactorselectchallenge.php

@@ -1,6 +1,11 @@
 <div class="warning">
 	<h2 class="two-factor-header"><?php p($l->t('Two-factor authentication')) ?></h2>
 	<p><?php p($l->t('Enhanced security is enabled for your account. Please authenticate using a second factor.')) ?></p>
+	<?php if ($_['providerMissing']): ?>
+	<p>
+		<strong><?php p($l->t('Could not load at least one of your enabled two-factor auth methods. Please contact your admin.')) ?></strong>


Do we want to add an setup check for this as well? Just iterate over the table and list them for the admin?

Like querying a set of generally enabled providers and checking if they can be loaded? Sounds reasonable.

Edit: I'll create a ticket for that.

MorrisJobke

👍

MorrisJobke · 2018-06-25T09:46:41Z

CI is here: https://drone.nextcloud.com/nextcloud/server/8327

ChristophWurst · 2018-06-25T10:03:07Z

The one failing acceptance tests looks unrelated to this change.

nickvergessen · 2018-06-27T12:12:49Z

core/Command/TwoFactorAuth/State.php

+
+namespace OC\Core\Command\TwoFactorAuth;
+
+use OC\Core\Command\Base;


PHP Fatal error: Cannot use OC\Core\Command\Base as Base because the name is already in use in /drone/src/github.com/nextcloud/server/core/Command/TwoFactorAuth/State.php on line 29

- Admins can enable or disable 2FA for all users, this change give the possibility to be "statefull" in other word we have to register enable/disable state for all users in IRegistry during plugin configuration (all user IRegistry will be populated at first config) - Add type to IProvider implemented classes

ChristophWurst added enhancement 2. developing Work in progress feature: authentication labels May 28, 2018

ChristophWurst added this to the Nextcloud 14 milestone May 28, 2018

ChristophWurst self-assigned this May 28, 2018

ChristophWurst force-pushed the enhancement/stateful-2fa-providers branch from a3bd376 to 7c27db9 Compare June 18, 2018 08:42

ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 18, 2018

ChristophWurst changed the title ~~[WIP] Stateful 2fa providers~~ Stateful 2fa providers Jun 18, 2018

ChristophWurst force-pushed the enhancement/stateful-2fa-providers branch from 7c27db9 to 35b8532 Compare June 18, 2018 09:15

ChristophWurst requested review from rullzer, MorrisJobke and juliusknorr June 18, 2018 09:17

ChristophWurst commented Jun 19, 2018

View reviewed changes

MorrisJobke reviewed Jun 19, 2018

View reviewed changes

rullzer reviewed Jun 19, 2018

View reviewed changes

ChristophWurst force-pushed the enhancement/stateful-2fa-providers branch from f341134 to 13d93f5 Compare June 20, 2018 06:30

rullzer approved these changes Jun 21, 2018

View reviewed changes

Make new classes strict and fix license header

7be465f

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

MorrisJobke reviewed Jun 25, 2018

View reviewed changes

MorrisJobke approved these changes Jun 25, 2018

View reviewed changes

MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jun 25, 2018

ChristophWurst mentioned this pull request Jun 25, 2018

Add a setup check for 2FA providers that fail to load #9985

Open

MorrisJobke merged commit 9444a3f into master Jun 25, 2018

MorrisJobke deleted the enhancement/stateful-2fa-providers branch June 25, 2018 13:50

nickvergessen reviewed Jun 27, 2018

View reviewed changes

This was referenced Jul 31, 2018

Fix duplicate inserts in the 2fa provider registry DAO #10462

Merged

Nextcloud 14 compatibility RCDevs-Security-Organization/nextcloud_openotp_auth#3

Closed

Nextcloud 14 compatibility jaark/twofactor_yubikey#12

Closed

ChristophWurst mentioned this pull request Sep 3, 2018

[Mandatory 2FA] Spec: Revamp 2FA occ interface #11019

Closed

rcdevs mentioned this pull request May 15, 2024

Changelog RCDevs-Security-Organization/nextcloud_openotp_auth#14

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stateful 2fa providers #9632

Stateful 2fa providers #9632

ChristophWurst commented May 28, 2018 •

edited

Loading

codecov bot commented Jun 4, 2018 •

edited

Loading

ChristophWurst commented Jun 18, 2018

ChristophWurst commented Jun 18, 2018 •

edited

Loading

ChristophWurst Jun 19, 2018

rullzer Jun 19, 2018

ChristophWurst Jun 20, 2018

MorrisJobke Jun 19, 2018

ChristophWurst Jun 19, 2018

rullzer Jun 9, 2018

ChristophWurst Jun 20, 2018

rullzer Jun 9, 2018

ChristophWurst Jun 20, 2018

rullzer Jun 19, 2018

rullzer commented Jun 19, 2018

ChristophWurst commented Jun 20, 2018

MorrisJobke commented Jun 20, 2018

ChristophWurst commented Jun 20, 2018

rullzer left a comment

rullzer Jun 21, 2018

rullzer Jun 21, 2018

ChristophWurst Jun 25, 2018

MorrisJobke Jun 25, 2018

rullzer Jun 21, 2018

MorrisJobke Jun 25, 2018

ChristophWurst Jun 25, 2018 •

edited

Loading

ChristophWurst Jun 25, 2018

MorrisJobke left a comment

MorrisJobke commented Jun 25, 2018

ChristophWurst commented Jun 25, 2018

nickvergessen Jun 27, 2018

nickvergessen Jun 27, 2018


		namespace OC\Core\Command\TwoFactorAuth;

		use OC\Core\Command\Base;

Stateful 2fa providers #9632

Stateful 2fa providers #9632

Conversation

ChristophWurst commented May 28, 2018 • edited Loading

codecov bot commented Jun 4, 2018 • edited Loading

Codecov Report

ChristophWurst commented Jun 18, 2018

ChristophWurst commented Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rullzer commented Jun 19, 2018

ChristophWurst commented Jun 20, 2018

MorrisJobke commented Jun 20, 2018

ChristophWurst commented Jun 20, 2018

rullzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristophWurst Jun 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MorrisJobke left a comment

Choose a reason for hiding this comment

MorrisJobke commented Jun 25, 2018

ChristophWurst commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristophWurst commented May 28, 2018 •

edited

Loading

codecov bot commented Jun 4, 2018 •

edited

Loading

ChristophWurst commented Jun 18, 2018 •

edited

Loading

ChristophWurst Jun 25, 2018 •

edited

Loading