Skip to content

Commit 725fece

Browse files
MorrisJobkeMorrisJobke
authored
Merge pull request #21344 from nextcloud/fix/twofactor-cleanup-event
Emit an event for every disabled 2FA provider during cleanup
2 parents 3a39f2a + 68794eb commit 725fece

File tree

7 files changed

+118
-16
lines changed

7 files changed

+118
-16
lines changed

‎lib/composer/composer/autoload_classmap.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@
101101
'OCP\\Authentication\\TwoFactorAuth\\IRegistry' => $baseDir . '/lib/public/Authentication/TwoFactorAuth/IRegistry.php',
102102
'OCP\\Authentication\\TwoFactorAuth\\RegistryEvent' => $baseDir . '/lib/public/Authentication/TwoFactorAuth/RegistryEvent.php',
103103
'OCP\\Authentication\\TwoFactorAuth\\TwoFactorException' => $baseDir . '/lib/public/Authentication/TwoFactorAuth/TwoFactorException.php',
104+
'OCP\\Authentication\\TwoFactorAuth\\TwoFactorProviderDisabled' => $baseDir . '/lib/public/Authentication/TwoFactorAuth/TwoFactorProviderDisabled.php',
104105
'OCP\\AutoloadNotAllowedException' => $baseDir . '/lib/public/AutoloadNotAllowedException.php',
105106
'OCP\\BackgroundJob' => $baseDir . '/lib/public/BackgroundJob.php',
106107
'OCP\\BackgroundJob\\IJob' => $baseDir . '/lib/public/BackgroundJob/IJob.php',

‎lib/composer/composer/autoload_static.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c
130130
'OCP\\Authentication\\TwoFactorAuth\\IRegistry' => __DIR__ . '/../../..' . '/lib/public/Authentication/TwoFactorAuth/IRegistry.php',
131131
'OCP\\Authentication\\TwoFactorAuth\\RegistryEvent' => __DIR__ . '/../../..' . '/lib/public/Authentication/TwoFactorAuth/RegistryEvent.php',
132132
'OCP\\Authentication\\TwoFactorAuth\\TwoFactorException' => __DIR__ . '/../../..' . '/lib/public/Authentication/TwoFactorAuth/TwoFactorException.php',
133+
'OCP\\Authentication\\TwoFactorAuth\\TwoFactorProviderDisabled' => __DIR__ . '/../../..' . '/lib/public/Authentication/TwoFactorAuth/TwoFactorProviderDisabled.php',
133134
'OCP\\AutoloadNotAllowedException' => __DIR__ . '/../../..' . '/lib/public/AutoloadNotAllowedException.php',
134135
'OCP\\BackgroundJob' => __DIR__ . '/../../..' . '/lib/public/BackgroundJob.php',
135136
'OCP\\BackgroundJob\\IJob' => __DIR__ . '/../../..' . '/lib/public/BackgroundJob/IJob.php',

‎lib/private/Authentication/TwoFactorAuth/Db/ProviderUserAssignmentDao.php‎

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
3030
use OCP\DB\QueryBuilder\IQueryBuilder;
3131
use OCP\IDBConnection;
32+
use function array_map;
3233

3334
/**
3435
* Data access object to query and assign (provider_id, uid, enabled) tuples of
@@ -91,13 +92,35 @@ public function persist(string $providerId, string $uid, int $enabled) {
9192
}
9293
}
9394

94-
public function deleteByUser(string $uid) {
95-
$qb = $this->conn->getQueryBuilder();
96-
97-
$deleteQuery = $qb->delete(self::TABLE_NAME)
98-
->where($qb->expr()->eq('uid', $qb->createNamedParameter($uid)));
99-
95+
/**
96+
* Delete all provider states of a user and return the provider IDs
97+
*
98+
* @param string $uid
99+
*
100+
* @return int[]
101+
*/
102+
public function deleteByUser(string $uid): array {
103+
$qb1 = $this->conn->getQueryBuilder();
104+
$selectQuery = $qb1->select('*')
105+
->from(self::TABLE_NAME)
106+
->where($qb1->expr()->eq('uid', $qb1->createNamedParameter($uid)));
107+
$selectResult = $selectQuery->execute();
108+
$rows = $selectResult->fetchAll();
109+
$selectResult->closeCursor();
110+
111+
$qb2 = $this->conn->getQueryBuilder();
112+
$deleteQuery = $qb2
113+
->delete(self::TABLE_NAME)
114+
->where($qb2->expr()->eq('uid', $qb2->createNamedParameter($uid)));
100115
$deleteQuery->execute();
116+
117+
return array_map(function (array $row) {
118+
return [
119+
'provider_id' => $row['provider_id'],
120+
'uid' => $row['uid'],
121+
'enabled' => 1 === (int) $row['enabled'],
122+
];
123+
}, $rows);
101124
}
102125

103126
public function deleteAll(string $providerId) {

‎lib/private/Authentication/TwoFactorAuth/Registry.php‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
use OCP\Authentication\TwoFactorAuth\IProvider;
3232
use OCP\Authentication\TwoFactorAuth\IRegistry;
3333
use OCP\Authentication\TwoFactorAuth\RegistryEvent;
34+
use OCP\Authentication\TwoFactorAuth\TwoFactorProviderDisabled;
3435
use OCP\EventDispatcher\IEventDispatcher;
3536
use OCP\IUser;
3637

@@ -66,11 +67,11 @@ public function disableProviderFor(IProvider $provider, IUser $user) {
6667
$this->dispatcher->dispatch(self::EVENT_PROVIDER_DISABLED, $event);
6768
}
6869

69-
/**
70-
* @todo evaluate if we should emit RegistryEvents for each of the deleted rows -> needs documentation
71-
*/
7270
public function deleteUserData(IUser $user): void {
73-
$this->assignmentDao->deleteByUser($user->getUID());
71+
foreach ($this->assignmentDao->deleteByUser($user->getUID()) as $provider) {
72+
$event = new TwoFactorProviderDisabled($provider['provider_id']);
73+
$this->dispatcher->dispatchTyped($event);
74+
}
7475
}
7576

7677
public function cleanUp(string $providerId) {

‎lib/public/Authentication/TwoFactorAuth/TwoFactorProviderDisabled.php‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* @copyright 2020 Christoph Wurst <christoph@winzerhof-wurst.at>
7+
*
8+
* @author 2020 Christoph Wurst <christoph@winzerhof-wurst.at>
9+
*
10+
* @license GNU AGPL version 3 or any later version
11+
*
12+
* This program is free software: you can redistribute it and/or modify
13+
* it under the terms of the GNU Affero General Public License as
14+
* published by the Free Software Foundation, either version 3 of the
15+
* License, or (at your option) any later version.
16+
*
17+
* This program is distributed in the hope that it will be useful,
18+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
19+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
20+
* GNU Affero General Public License for more details.
21+
*
22+
* You should have received a copy of the GNU Affero General Public License
23+
* along with this program. If not, see <http://www.gnu.org/licenses/>.
24+
*/
25+
26+
namespace OCP\Authentication\TwoFactorAuth;
27+
28+
use OCP\EventDispatcher\Event;
29+
30+
/**
31+
* @since 20.0.0
32+
*/
33+
final class TwoFactorProviderDisabled extends Event {
34+
35+
/** @var string */
36+
private $providerId;
37+
38+
/**
39+
* @since 20.0.0
40+
*/
41+
public function __construct(string $providerId) {
42+
parent::__construct();
43+
$this->providerId = $providerId;
44+
}
45+
46+
/**
47+
* @since 20.0.0
48+
*/
49+
public function getProviderId(): string {
50+
return $this->providerId;
51+
}
52+
}

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

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -136,14 +136,29 @@ public function testDeleteByUser() {
136136
$this->dao->persist('twofactor_fail', 'user1', 1);
137137
$this->dao->persist('twofactor_u2f', 'user1', 1);
138138
$this->dao->persist('twofactor_fail', 'user2', 0);
139-
$this->dao->persist('twofactor_u2f', 'user1', 0);
140-
141-
$this->dao->deleteByUser('user1');
142-
139+
$this->dao->persist('twofactor_u2f', 'user2', 0);
140+
141+
$deleted = $this->dao->deleteByUser('user1');
142+
143+
$this->assertEquals(
144+
[
145+
[
146+
'uid' => 'user1',
147+
'provider_id' => 'twofactor_fail',
148+
'enabled' => true,
149+
],
150+
[
151+
'uid' => 'user1',
152+
'provider_id' => 'twofactor_u2f',
153+
'enabled' => true,
154+
],
155+
],
156+
$deleted
157+
);
143158
$statesUser1 = $this->dao->getState('user1');
144159
$statesUser2 = $this->dao->getState('user2');
145160
$this->assertCount(0, $statesUser1);
146-
$this->assertCount(1, $statesUser2);
161+
$this->assertCount(2, $statesUser2);
147162
}
148163

149164
public function testDeleteAll() {

‎tests/lib/Authentication/TwoFactorAuth/RegistryTest.php‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
use OCP\Authentication\TwoFactorAuth\IProvider;
3232
use OCP\Authentication\TwoFactorAuth\IRegistry;
3333
use OCP\Authentication\TwoFactorAuth\RegistryEvent;
34+
use OCP\Authentication\TwoFactorAuth\TwoFactorProviderDisabled;
3435
use OCP\EventDispatcher\IEventDispatcher;
3536
use OCP\IUser;
3637
use PHPUnit\Framework\MockObject\MockObject;
@@ -115,7 +116,15 @@ public function testDeleteUserData() {
115116
$user->expects($this->once())->method('getUID')->willReturn('user123');
116117
$this->dao->expects($this->once())
117118
->method('deleteByUser')
118-
->with('user123');
119+
->with('user123')
120+
->willReturn([
121+
[
122+
'provider_id' => 'twofactor_u2f',
123+
]
124+
]);
125+
$this->dispatcher->expects($this->once())
126+
->method('dispatchTyped')
127+
->with(new TwoFactorProviderDisabled('twofactor_u2f'));
119128

120129
$this->registry->deleteUserData($user);
121130
}

0 commit comments

Comments
 (0)