Skip to content

Commit c08d28e

Browse files
PVince81PVince81
committed
Fix remote share deletion when deleting user
When deleting a user, we should only delete the direct remote user shares or the remote group based subshares. Signed-off-by: Vincent Petry <vincent@nextcloud.com>
1 parent 6307426 commit c08d28e

File tree

2 files changed

+201
-51
lines changed

2 files changed

+201
-51
lines changed

‎apps/files_sharing/lib/External/Manager.php‎

Lines changed: 54 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -675,38 +675,69 @@ public function removeUserShares($uid): bool {
675675
$getShare = $this->connection->prepare('
676676
SELECT `id`, `remote`, `share_type`, `share_token`, `remote_id`
677677
FROM `*PREFIX*share_external`
678-
WHERE `user` = ?');
679-
$result = $getShare->execute([$uid]);
678+
WHERE `user` = ?
679+
AND `share_type` = ?');
680+
$result = $getShare->execute([$uid, IShare::TYPE_USER]);
680681
$shares = $result->fetchAll();
681682
$result->closeCursor();
682-
$deletedGroupShares = [];
683+
683684
foreach ($shares as $share) {
684-
if ((int)$share['share_type'] === IShare::TYPE_GROUP) {
685-
$deletedGroupShares[] = $share['id'];
686-
} else {
687-
$this->sendFeedbackToRemote($share['remote'], $share['share_token'], $share['remote_id'], 'decline');
688-
}
685+
$this->sendFeedbackToRemote($share['remote'], $share['share_token'], $share['remote_id'], 'decline');
689686
}
690687

691-
$query = $this->connection->prepare('
692-
DELETE FROM `*PREFIX*share_external`
688+
$qb = $this->connection->getQueryBuilder();
689+
$qb->delete('share_external')
690+
// user field can specify a user or a group
691+
->where($qb->expr()->eq('user', $qb->createNamedParameter($uid)))
692+
->andWhere(
693+
$qb->expr()->orX(
694+
// delete direct shares
695+
$qb->expr()->eq('share_type', $qb->expr()->literal(IShare::TYPE_USER)),
696+
// delete sub-shares of group shares for that user
697+
$qb->expr()->andX(
698+
$qb->expr()->eq('share_type', $qb->expr()->literal(IShare::TYPE_GROUP)),
699+
$qb->expr()->neq('parent', $qb->expr()->literal(-1)),
700+
)
701+
)
702+
);
703+
$qb->execute();
704+
} catch (\Doctrine\DBAL\Exception $ex) {
705+
$this->logger->emergency('Could not delete user shares', ['exception' => $ex]);
706+
return false;
707+
}
708+
709+
return true;
710+
}
711+
712+
public function removeGroupShares($gid): bool {
713+
try {
714+
$getShare = $this->connection->prepare('
715+
SELECT `id`, `remote`, `share_type`, `share_token`, `remote_id`
716+
FROM `*PREFIX*share_external`
693717
WHERE `user` = ?
694-
');
695-
$deleteResult = $query->execute([$uid]);
696-
$deleteResult->closeCursor();
718+
AND `share_type` = ?');
719+
$result = $getShare->execute([$gid, IShare::TYPE_GROUP]);
720+
$shares = $result->fetchAll();
721+
$result->closeCursor();
697722

698-
// delete sub-entries from deleted parents
699-
foreach ($deletedGroupShares as $deletedId) {
700-
// TODO: batch this with query builder
701-
$query = $this->connection->prepare('
702-
DELETE FROM `*PREFIX*share_external`
703-
WHERE `parent` = ?
704-
');
705-
$deleteResult = $query->execute([$deletedId]);
706-
$deleteResult->closeCursor();
723+
$deletedGroupShares = [];
724+
$qb = $this->connection->getQueryBuilder();
725+
// delete group share entry and matching sub-entries
726+
$qb->delete('share_external')
727+
->where(
728+
$qb->expr()->orX(
729+
$qb->expr()->eq('id', $qb->createParameter('share_id')),
730+
$qb->expr()->eq('parent', $qb->createParameter('share_parent_id'))
731+
)
732+
);
733+
734+
foreach ($shares as $share) {
735+
$qb->setParameter('share_id', $share['id']);
736+
$qb->setParameter('share_parent_id', $share['id']);
737+
$qb->execute();
707738
}
708739
} catch (\Doctrine\DBAL\Exception $ex) {
709-
$this->logger->emergency('Could not get shares', ['exception' => $ex]);
740+
$this->logger->emergency('Could not delete user shares', ['exception' => $ex]);
710741
return false;
711742
}
712743

‎apps/files_sharing/tests/External/ManagerTest.php‎

Lines changed: 147 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,9 @@ class ManagerTest extends TestCase {
8282
/** @var \PHPUnit\Framework\MockObject\MockObject|IUserManager */
8383
private $userManager;
8484

85+
/** @var LoggerInterface */
86+
private $logger;
87+
8588
private $uid;
8689

8790
/**
@@ -113,27 +116,10 @@ protected function setUp(): void {
113116
->method('search')
114117
->willReturn([]);
115118

116-
$logger = $this->createMock(LoggerInterface::class);
117-
$logger->expects($this->never())->method('emergency');
119+
$this->logger = $this->createMock(LoggerInterface::class);
120+
$this->logger->expects($this->never())->method('emergency');
118121

119-
$this->manager = $this->getMockBuilder(Manager::class)
120-
->setConstructorArgs(
121-
[
122-
\OC::$server->getDatabaseConnection(),
123-
$this->mountManager,
124-
new StorageFactory(),
125-
$this->clientService,
126-
\OC::$server->getNotificationManager(),
127-
\OC::$server->query(\OCP\OCS\IDiscoveryService::class),
128-
$this->cloudFederationProviderManager,
129-
$this->cloudFederationFactory,
130-
$this->groupManager,
131-
$this->userManager,
132-
$this->uid,
133-
$this->eventDispatcher,
134-
$logger,
135-
]
136-
)->setMethods(['tryOCMEndPoint'])->getMock();
122+
$this->manager = $this->createManagerForUser($this->uid);
137123

138124
$this->testMountProvider = new MountProvider(\OC::$server->getDatabaseConnection(), function () {
139125
return $this->manager;
@@ -165,6 +151,27 @@ protected function tearDown(): void {
165151
parent::tearDown();
166152
}
167153

154+
private function createManagerForUser($userId) {
155+
return $this->getMockBuilder(Manager::class)
156+
->setConstructorArgs(
157+
[
158+
\OC::$server->getDatabaseConnection(),
159+
$this->mountManager,
160+
new StorageFactory(),
161+
$this->clientService,
162+
\OC::$server->getNotificationManager(),
163+
\OC::$server->query(\OCP\OCS\IDiscoveryService::class),
164+
$this->cloudFederationProviderManager,
165+
$this->cloudFederationFactory,
166+
$this->groupManager,
167+
$this->userManager,
168+
$userId,
169+
$this->eventDispatcher,
170+
$this->logger,
171+
]
172+
)->setMethods(['tryOCMEndPoint'])->getMock();
173+
}
174+
168175
private function setupMounts() {
169176
$this->mountManager->clear();
170177
$mounts = $this->testMountProvider->getMountsForUser($this->user, new StorageFactory());
@@ -348,7 +355,7 @@ public function doTestAddShare($shareData1, $isGroup = false) {
348355

349356
if ($isGroup) {
350357
// no http requests here
351-
$this->manager->removeUserShares('group1');
358+
$this->manager->removeGroupShares('group1');
352359
} else {
353360
$client1 = $this->getMockBuilder('OCP\Http\Client\IClient')
354361
->disableOriginalConstructor()->getMock();
@@ -416,7 +423,24 @@ private function verifyDeclinedGroupShare($shareData, $tempMount = null) {
416423
$this->assertNotMount($tempMount);
417424
}
418425

419-
private function createTestGroupShare() {
426+
private function createTestUserShare($userId = 'user1') {
427+
$shareData = [
428+
'remote' => 'http://localhost',
429+
'token' => 'token1',
430+
'password' => '',
431+
'name' => '/SharedFolder',
432+
'owner' => 'foobar',
433+
'shareType' => IShare::TYPE_USER,
434+
'accepted' => false,
435+
'user' => $userId,
436+
'remoteId' => '2342'
437+
];
438+
439+
$this->assertSame(null, call_user_func_array([$this->manager, 'addShare'], $shareData));
440+
441+
return $shareData;
442+
}
443+
private function createTestGroupShare($groupId = 'group1') {
420444
$shareData = [
421445
'remote' => 'http://localhost',
422446
'token' => 'token1',
@@ -425,17 +449,20 @@ private function createTestGroupShare() {
425449
'owner' => 'foobar',
426450
'shareType' => IShare::TYPE_GROUP,
427451
'accepted' => false,
428-
'user' => 'group1',
452+
'user' => $groupId,
429453
'remoteId' => '2342'
430454
];
431455

432456
$this->assertSame(null, call_user_func_array([$this->manager, 'addShare'], $shareData));
433457

434458
$allShares = self::invokePrivate($this->manager, 'getShares', [null]);
435-
$this->assertCount(1, $allShares);
436-
437-
// this will hold the main group entry
438-
$groupShare = $allShares[0];
459+
foreach ($allShares as $share) {
460+
if ($share['user'] === $groupId) {
461+
// this will hold the main group entry
462+
$groupShare = $share;
463+
break;
464+
}
465+
}
439466

440467
return [$shareData, $groupShare];
441468
}
@@ -461,8 +488,9 @@ public function testAcceptGroupShareAgainThroughGroupShare() {
461488

462489
// this will return sub-entries
463490
$openShares = $this->manager->getOpenShares();
491+
$this->assertCount(1, $openShares);
464492

465-
// accept through sub-share
493+
// accept through group share
466494
$this->assertTrue($this->manager->acceptShare($groupShare['id']));
467495
$this->verifyAcceptedGroupShare($shareData, '/SharedFolder');
468496

@@ -482,6 +510,7 @@ public function testAcceptGroupShareAgainThroughSubShare() {
482510

483511
// this will return sub-entries
484512
$openShares = $this->manager->getOpenShares();
513+
$this->assertCount(1, $openShares);
485514

486515
// accept through sub-share
487516
$this->assertTrue($this->manager->acceptShare($openShares[0]['id']));
@@ -583,6 +612,96 @@ public function testDeclineThenAcceptGroupShareAgainThroughSubShare() {
583612
$this->verifyAcceptedGroupShare($shareData);
584613
}
585614

615+
public function testDeleteUserShares() {
616+
// user 1 shares
617+
618+
$shareData = $this->createTestUserShare($this->uid);
619+
620+
[$shareData, $groupShare] = $this->createTestGroupShare();
621+
622+
$shares = $this->manager->getOpenShares();
623+
$this->assertCount(2, $shares);
624+
625+
$this->assertTrue($this->manager->acceptShare($groupShare['id']));
626+
627+
// user 2 shares
628+
$manager2 = $this->createManagerForUser('user2');
629+
$shareData2 = [
630+
'remote' => 'http://localhost',
631+
'token' => 'token1',
632+
'password' => '',
633+
'name' => '/SharedFolder',
634+
'owner' => 'foobar',
635+
'shareType' => IShare::TYPE_USER,
636+
'accepted' => false,
637+
'user' => 'user2',
638+
'remoteId' => '2342'
639+
];
640+
$this->assertSame(null, call_user_func_array([$manager2, 'addShare'], $shareData2));
641+
642+
$user2Shares = $manager2->getOpenShares();
643+
$this->assertCount(2, $user2Shares);
644+
645+
$this->manager->expects($this->at(0))->method('tryOCMEndPoint')->with('http://localhost', 'token1', '2342', 'decline')->willReturn([]);
646+
$this->manager->removeUserShares($this->uid);
647+
648+
$user1Shares = $this->manager->getOpenShares();
649+
// user share is gone, group is still there
650+
$this->assertCount(1, $user1Shares);
651+
$this->assertEquals($user1Shares[0]['share_type'], IShare::TYPE_GROUP);
652+
653+
// user 2 shares untouched
654+
$user2Shares = $manager2->getOpenShares();
655+
$this->assertCount(2, $user2Shares);
656+
$this->assertEquals($user2Shares[0]['share_type'], IShare::TYPE_GROUP);
657+
$this->assertEquals($user2Shares[0]['user'], 'group1');
658+
$this->assertEquals($user2Shares[1]['share_type'], IShare::TYPE_USER);
659+
$this->assertEquals($user2Shares[1]['user'], 'user2');
660+
}
661+
662+
public function testDeleteGroupShares() {
663+
$shareData = $this->createTestUserShare($this->uid);
664+
665+
[$shareData, $groupShare] = $this->createTestGroupShare();
666+
667+
$shares = $this->manager->getOpenShares();
668+
$this->assertCount(2, $shares);
669+
670+
$this->assertTrue($this->manager->acceptShare($groupShare['id']));
671+
672+
// user 2 shares
673+
$manager2 = $this->createManagerForUser('user2');
674+
$shareData2 = [
675+
'remote' => 'http://localhost',
676+
'token' => 'token1',
677+
'password' => '',
678+
'name' => '/SharedFolder',
679+
'owner' => 'foobar',
680+
'shareType' => IShare::TYPE_USER,
681+
'accepted' => false,
682+
'user' => 'user2',
683+
'remoteId' => '2342'
684+
];
685+
$this->assertSame(null, call_user_func_array([$manager2, 'addShare'], $shareData2));
686+
687+
$user2Shares = $manager2->getOpenShares();
688+
$this->assertCount(2, $user2Shares);
689+
690+
$this->manager->expects($this->never())->method('tryOCMEndPoint');
691+
$this->manager->removeGroupShares('group1');
692+
693+
$user1Shares = $this->manager->getOpenShares();
694+
// user share is gone, group is still there
695+
$this->assertCount(1, $user1Shares);
696+
$this->assertEquals($user1Shares[0]['share_type'], IShare::TYPE_USER);
697+
698+
// user 2 shares untouched
699+
$user2Shares = $manager2->getOpenShares();
700+
$this->assertCount(1, $user2Shares);
701+
$this->assertEquals($user2Shares[0]['share_type'], IShare::TYPE_USER);
702+
$this->assertEquals($user2Shares[0]['user'], 'user2');
703+
}
704+
586705
/**
587706
* @param array $expected
588707
* @param array $actual

0 commit comments

Comments
 (0)