Merge pull request #21344 from nextcloud/fix/twofactor-cleanup-event

Emit an event for every disabled 2FA provider during cleanup

7 files changed, 118 insertions, 16 deletions

diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php
index d6c49d19b86..e063fe0b715 100644
--- a/lib/composer/composer/autoload_classmap.php
+++ b/lib/composer/composer/autoload_classmap.php
@@ -101,6 +101,7 @@ return array(
     'OCP\\Authentication\\TwoFactorAuth\\IRegistry' => $baseDir . '/lib/public/Authentication/TwoFactorAuth/IRegistry.php',
     'OCP\\Authentication\\TwoFactorAuth\\RegistryEvent' => $baseDir . '/lib/public/Authentication/TwoFactorAuth/RegistryEvent.php',
     'OCP\\Authentication\\TwoFactorAuth\\TwoFactorException' => $baseDir . '/lib/public/Authentication/TwoFactorAuth/TwoFactorException.php',
+    'OCP\\Authentication\\TwoFactorAuth\\TwoFactorProviderDisabled' => $baseDir . '/lib/public/Authentication/TwoFactorAuth/TwoFactorProviderDisabled.php',
     'OCP\\AutoloadNotAllowedException' => $baseDir . '/lib/public/AutoloadNotAllowedException.php',
     'OCP\\BackgroundJob' => $baseDir . '/lib/public/BackgroundJob.php',
     'OCP\\BackgroundJob\\IJob' => $baseDir . '/lib/public/BackgroundJob/IJob.php',
diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php
index 0edbf18c9a6..20394b4ae30 100644
--- a/lib/composer/composer/autoload_static.php
+++ b/lib/composer/composer/autoload_static.php
@@ -130,6 +130,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c
         'OCP\\Authentication\\TwoFactorAuth\\IRegistry' => __DIR__ . '/../../..' . '/lib/public/Authentication/TwoFactorAuth/IRegistry.php',
         'OCP\\Authentication\\TwoFactorAuth\\RegistryEvent' => __DIR__ . '/../../..' . '/lib/public/Authentication/TwoFactorAuth/RegistryEvent.php',
         'OCP\\Authentication\\TwoFactorAuth\\TwoFactorException' => __DIR__ . '/../../..' . '/lib/public/Authentication/TwoFactorAuth/TwoFactorException.php',
+        'OCP\\Authentication\\TwoFactorAuth\\TwoFactorProviderDisabled' => __DIR__ . '/../../..' . '/lib/public/Authentication/TwoFactorAuth/TwoFactorProviderDisabled.php',
         'OCP\\AutoloadNotAllowedException' => __DIR__ . '/../../..' . '/lib/public/AutoloadNotAllowedException.php',
         'OCP\\BackgroundJob' => __DIR__ . '/../../..' . '/lib/public/BackgroundJob.php',
         'OCP\\BackgroundJob\\IJob' => __DIR__ . '/../../..' . '/lib/public/BackgroundJob/IJob.php',
diff --git a/lib/private/Authentication/TwoFactorAuth/Db/ProviderUserAssignmentDao.php b/lib/private/Authentication/TwoFactorAuth/Db/ProviderUserAssignmentDao.php
index 02e6863d1c4..bd8ff0353ee 100644
--- a/lib/private/Authentication/TwoFactorAuth/Db/ProviderUserAssignmentDao.php
+++ b/lib/private/Authentication/TwoFactorAuth/Db/ProviderUserAssignmentDao.php
@@ -29,6 +29,7 @@ namespace OC\Authentication\TwoFactorAuth\Db;
 use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
 use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\IDBConnection;
+use function array_map;
 
 /**
  * Data access object to query and assign (provider_id, uid, enabled) tuples of
@@ -91,13 +92,35 @@ class ProviderUserAssignmentDao {
 		}
 	}
 
-	public function deleteByUser(string $uid) {
-		$qb = $this->conn->getQueryBuilder();
-
-		$deleteQuery = $qb->delete(self::TABLE_NAME)
-			->where($qb->expr()->eq('uid', $qb->createNamedParameter($uid)));
-
+	/**
+	 * Delete all provider states of a user and return the provider IDs
+	 *
+	 * @param string $uid
+	 *
+	 * @return int[]
+	 */
+	public function deleteByUser(string $uid): array {
+		$qb1 = $this->conn->getQueryBuilder();
+		$selectQuery = $qb1->select('*')
+			->from(self::TABLE_NAME)
+			->where($qb1->expr()->eq('uid', $qb1->createNamedParameter($uid)));
+		$selectResult = $selectQuery->execute();
+		$rows = $selectResult->fetchAll();
+		$selectResult->closeCursor();
+
+		$qb2 = $this->conn->getQueryBuilder();
+		$deleteQuery = $qb2
+			->delete(self::TABLE_NAME)
+			->where($qb2->expr()->eq('uid', $qb2->createNamedParameter($uid)));
 		$deleteQuery->execute();
+
+		return array_map(function (array $row) {
+			return [
+				'provider_id' => $row['provider_id'],
+				'uid' => $row['uid'],
+				'enabled' => 1 === (int) $row['enabled'],
+			];
+		}, $rows);
 	}
 
 	public function deleteAll(string $providerId) {
diff --git a/lib/private/Authentication/TwoFactorAuth/Registry.php b/lib/private/Authentication/TwoFactorAuth/Registry.php
index 97df2bd5311..2af8566d3e5 100644
--- a/lib/private/Authentication/TwoFactorAuth/Registry.php
+++ b/lib/private/Authentication/TwoFactorAuth/Registry.php
@@ -31,6 +31,7 @@ use OC\Authentication\TwoFactorAuth\Db\ProviderUserAssignmentDao;
 use OCP\Authentication\TwoFactorAuth\IProvider;
 use OCP\Authentication\TwoFactorAuth\IRegistry;
 use OCP\Authentication\TwoFactorAuth\RegistryEvent;
+use OCP\Authentication\TwoFactorAuth\TwoFactorProviderDisabled;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\IUser;
 
@@ -66,11 +67,11 @@ class Registry implements IRegistry {
 		$this->dispatcher->dispatch(self::EVENT_PROVIDER_DISABLED, $event);
 	}
 
-	/**
-	 * @todo evaluate if we should emit RegistryEvents for each of the deleted rows -> needs documentation
-	 */
 	public function deleteUserData(IUser $user): void {
-		$this->assignmentDao->deleteByUser($user->getUID());
+		foreach ($this->assignmentDao->deleteByUser($user->getUID()) as $provider) {
+			$event = new TwoFactorProviderDisabled($provider['provider_id']);
+			$this->dispatcher->dispatchTyped($event);
+		}
 	}
 
 	public function cleanUp(string $providerId) {
diff --git a/lib/public/Authentication/TwoFactorAuth/TwoFactorProviderDisabled.php b/lib/public/Authentication/TwoFactorAuth/TwoFactorProviderDisabled.php
new file mode 100644
index 00000000000..a0e111c59b3
--- /dev/null
+++ b/lib/public/Authentication/TwoFactorAuth/TwoFactorProviderDisabled.php
@@ -0,0 +1,52 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * @copyright 2020 Christoph Wurst <christoph@winzerhof-wurst.at>
+ *
+ * @author 2020 Christoph Wurst <christoph@winzerhof-wurst.at>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+namespace OCP\Authentication\TwoFactorAuth;
+
+use OCP\EventDispatcher\Event;
+
+/**
+ * @since 20.0.0
+ */
+final class TwoFactorProviderDisabled extends Event {
+
+	/** @var string */
+	private $providerId;
+
+	/**
+	 * @since 20.0.0
+	 */
+	public function __construct(string $providerId) {
+		parent::__construct();
+		$this->providerId = $providerId;
+	}
+
+	/**
+	 * @since 20.0.0
+	 */
+	public function getProviderId(): string {
+		return $this->providerId;
+	}
+}
diff --git a/tests/lib/Authentication/TwoFactorAuth/Db/ProviderUserAssignmentDaoTest.php b/tests/lib/Authentication/TwoFactorAuth/Db/ProviderUserAssignmentDaoTest.php
index 1a21ee047df..7975108c59b 100644
--- a/tests/lib/Authentication/TwoFactorAuth/Db/ProviderUserAssignmentDaoTest.php
+++ b/tests/lib/Authentication/TwoFactorAuth/Db/ProviderUserAssignmentDaoTest.php
@@ -136,14 +136,29 @@ class ProviderUserAssignmentDaoTest extends TestCase {
 		$this->dao->persist('twofactor_fail', 'user1', 1);
 		$this->dao->persist('twofactor_u2f', 'user1', 1);
 		$this->dao->persist('twofactor_fail', 'user2', 0);
-		$this->dao->persist('twofactor_u2f', 'user1', 0);
-
-		$this->dao->deleteByUser('user1');
-
+		$this->dao->persist('twofactor_u2f', 'user2', 0);
+
+		$deleted = $this->dao->deleteByUser('user1');
+
+		$this->assertEquals(
+			[
+				[
+					'uid' => 'user1',
+					'provider_id' => 'twofactor_fail',
+					'enabled' => true,
+				],
+				[
+					'uid' => 'user1',
+					'provider_id' => 'twofactor_u2f',
+					'enabled' => true,
+				],
+			],
+			$deleted
+		);
 		$statesUser1 = $this->dao->getState('user1');
 		$statesUser2 = $this->dao->getState('user2');
 		$this->assertCount(0, $statesUser1);
-		$this->assertCount(1, $statesUser2);
+		$this->assertCount(2, $statesUser2);
 	}
 
 	public function testDeleteAll() {
diff --git a/tests/lib/Authentication/TwoFactorAuth/RegistryTest.php b/tests/lib/Authentication/TwoFactorAuth/RegistryTest.php
index 49f4eaa7020..b0d0ef8efef 100644
--- a/tests/lib/Authentication/TwoFactorAuth/RegistryTest.php
+++ b/tests/lib/Authentication/TwoFactorAuth/RegistryTest.php
@@ -31,6 +31,7 @@ use OC\Authentication\TwoFactorAuth\Registry;
 use OCP\Authentication\TwoFactorAuth\IProvider;
 use OCP\Authentication\TwoFactorAuth\IRegistry;
 use OCP\Authentication\TwoFactorAuth\RegistryEvent;
+use OCP\Authentication\TwoFactorAuth\TwoFactorProviderDisabled;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\IUser;
 use PHPUnit\Framework\MockObject\MockObject;
@@ -115,7 +116,15 @@ class RegistryTest extends TestCase {
 		$user->expects($this->once())->method('getUID')->willReturn('user123');
 		$this->dao->expects($this->once())
 			->method('deleteByUser')
-			->with('user123');
+			->with('user123')
+			->willReturn([
+				[
+					'provider_id' => 'twofactor_u2f',
+				]
+			]);
+		$this->dispatcher->expects($this->once())
+			->method('dispatchTyped')
+			->with(new TwoFactorProviderDisabled('twofactor_u2f'));
 
 		$this->registry->deleteUserData($user);
 	}