From f2559e44808fc785fa7ef148d4e15713b411e605 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Mon, 19 Aug 2024 15:27:34 +0200 Subject: [PATCH] Revert "[stable28] Apply group limit on remove from group" --- .../files_sharing/limit_to_same_group.cy.ts | 107 ------------------ lib/private/Share20/DefaultShareProvider.php | 45 +------- lib/private/Share20/ProviderFactory.php | 1 - .../lib/Share20/DefaultShareProviderTest.php | 23 +--- 4 files changed, 8 insertions(+), 168 deletions(-) delete mode 100644 cypress/e2e/files_sharing/limit_to_same_group.cy.ts diff --git a/cypress/e2e/files_sharing/limit_to_same_group.cy.ts b/cypress/e2e/files_sharing/limit_to_same_group.cy.ts deleted file mode 100644 index c95efa089ff6f..0000000000000 --- a/cypress/e2e/files_sharing/limit_to_same_group.cy.ts +++ /dev/null @@ -1,107 +0,0 @@ -/** - * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors - * SPDX-License-Identifier: AGPL-3.0-or-later - */ - -import { User } from "@nextcloud/cypress" -import { createShare } from "./FilesSharingUtils.ts" - -describe('Limit to sharing to people in the same group', () => { - let alice: User - let bob: User - let randomFileName1 = '' - let randomFileName2 = '' - let randomGroupName = '' - let randomGroupName2 = '' - let randomGroupName3 = '' - - before(() => { - randomFileName1 = Math.random().toString(36).replace(/[^a-z]+/g, '').substring(0, 10) + '.txt' - randomFileName2 = Math.random().toString(36).replace(/[^a-z]+/g, '').substring(0, 10) + '.txt' - randomGroupName = Math.random().toString(36).replace(/[^a-z]+/g, '').substring(0, 10) - randomGroupName2 = Math.random().toString(36).replace(/[^a-z]+/g, '').substring(0, 10) - randomGroupName3 = Math.random().toString(36).replace(/[^a-z]+/g, '').substring(0, 10) - - cy.runOccCommand('config:app:set core shareapi_only_share_with_group_members --value yes') - - cy.createRandomUser() - .then(user => { - alice = user - cy.createRandomUser() - }) - .then(user => { - bob = user - - cy.runOccCommand(`group:add ${randomGroupName}`) - cy.runOccCommand(`group:add ${randomGroupName2}`) - cy.runOccCommand(`group:add ${randomGroupName3}`) - cy.runOccCommand(`group:adduser ${randomGroupName} ${alice.userId}`) - cy.runOccCommand(`group:adduser ${randomGroupName} ${bob.userId}`) - cy.runOccCommand(`group:adduser ${randomGroupName2} ${alice.userId}`) - cy.runOccCommand(`group:adduser ${randomGroupName2} ${bob.userId}`) - cy.runOccCommand(`group:adduser ${randomGroupName3} ${bob.userId}`) - - cy.uploadContent(alice, new Blob(['share to bob'], { type: 'text/plain' }), 'text/plain', `/${randomFileName1}`) - cy.uploadContent(bob, new Blob(['share by bob'], { type: 'text/plain' }), 'text/plain', `/${randomFileName2}`) - - cy.login(alice) - cy.visit('/apps/files') - createShare(randomFileName1, bob.userId) - cy.login(bob) - cy.visit('/apps/files') - createShare(randomFileName2, alice.userId) - }) - }) - - after(() => { - cy.runOccCommand('config:app:set core shareapi_only_share_with_group_members --value no') - }) - - it('Alice can see the shared file', () => { - cy.login(alice) - cy.visit('/apps/files') - cy.get(`[data-cy-files-list] [data-cy-files-list-row-name="${randomFileName2}"]`).should('exist') - }) - - it('Bob can see the shared file', () => { - cy.login(alice) - cy.visit('/apps/files') - cy.get(`[data-cy-files-list] [data-cy-files-list-row-name="${randomFileName1}"]`).should('exist') - }) - - context('Bob is removed from the first group', () => { - before(() => { - cy.runOccCommand(`group:removeuser ${randomGroupName} ${bob.userId}`) - }) - - it('Alice can see the shared file', () => { - cy.login(alice) - cy.visit('/apps/files') - cy.get(`[data-cy-files-list] [data-cy-files-list-row-name="${randomFileName2}"]`).should('exist') - }) - - it('Bob can see the shared file', () => { - cy.login(alice) - cy.visit('/apps/files') - cy.get(`[data-cy-files-list] [data-cy-files-list-row-name="${randomFileName1}"]`).should('exist') - }) - }) - - context('Bob is removed from the second group', () => { - before(() => { - cy.runOccCommand(`group:removeuser ${randomGroupName2} ${bob.userId}`) - }) - - it('Alice cannot see the shared file', () => { - cy.login(alice) - cy.visit('/apps/files') - cy.get(`[data-cy-files-list] [data-cy-files-list-row-name="${randomFileName2}"]`).should('not.exist') - }) - - it('Bob cannot see the shared file', () => { - cy.login(alice) - cy.visit('/apps/files') - cy.get(`[data-cy-files-list] [data-cy-files-list-row-name="${randomFileName1}"]`).should('not.exist') - }) - }) -}) diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index 75e853164b2ae..fd22095b420eb 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -53,7 +53,6 @@ use OCP\Mail\IMailer; use OCP\Share\Exceptions\ShareNotFound; use OCP\Share\IAttributes; -use OCP\Share\IManager; use OCP\Share\IShare; use OCP\Share\IShareProvider; use function str_starts_with; @@ -103,7 +102,6 @@ public function __construct( IFactory $l10nFactory, IURLGenerator $urlGenerator, ITimeFactory $timeFactory, - private IManager $shareManager, ) { $this->dbConn = $connection; $this->userManager = $userManager; @@ -1306,7 +1304,6 @@ public function groupDeleted($gid) { * * @param string $uid * @param string $gid - * @return void */ public function userDeletedFromGroup($uid, $gid) { /* @@ -1318,7 +1315,7 @@ public function userDeletedFromGroup($uid, $gid) { ->where($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_GROUP))) ->andWhere($qb->expr()->eq('share_with', $qb->createNamedParameter($gid))); - $cursor = $qb->executeQuery(); + $cursor = $qb->execute(); $ids = []; while ($row = $cursor->fetch()) { $ids[] = (int)$row['id']; @@ -1335,45 +1332,7 @@ public function userDeletedFromGroup($uid, $gid) { ->where($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_USERGROUP))) ->andWhere($qb->expr()->eq('share_with', $qb->createNamedParameter($uid))) ->andWhere($qb->expr()->in('parent', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_INT_ARRAY))); - $qb->executeStatement(); - } - } - - if ($this->shareManager->shareWithGroupMembersOnly()) { - $user = $this->userManager->get($uid); - if ($user === null) { - return; - } - $userGroups = $this->groupManager->getUserGroupIds($user); - - // Delete user shares received by the user from users in the group. - $userReceivedShares = $this->shareManager->getSharedWith($uid, IShare::TYPE_USER, null, -1); - foreach ($userReceivedShares as $share) { - $owner = $this->userManager->get($share->getSharedBy()); - if ($owner === null) { - continue; - } - $ownerGroups = $this->groupManager->getUserGroupIds($owner); - $mutualGroups = array_intersect($userGroups, $ownerGroups); - - if (count($mutualGroups) === 0) { - $this->shareManager->deleteShare($share); - } - } - - // Delete user shares from the user to users in the group. - $userEmittedShares = $this->shareManager->getSharesBy($uid, IShare::TYPE_USER, null, true, -1); - foreach ($userEmittedShares as $share) { - $recipient = $this->userManager->get($share->getSharedWith()); - if ($recipient === null) { - continue; - } - $recipientGroups = $this->groupManager->getUserGroupIds($recipient); - $mutualGroups = array_intersect($userGroups, $recipientGroups); - - if (count($mutualGroups) === 0) { - $this->shareManager->deleteShare($share); - } + $qb->execute(); } } } diff --git a/lib/private/Share20/ProviderFactory.php b/lib/private/Share20/ProviderFactory.php index 63c161d5beee7..dbf1b21dabe8b 100644 --- a/lib/private/Share20/ProviderFactory.php +++ b/lib/private/Share20/ProviderFactory.php @@ -106,7 +106,6 @@ protected function defaultShareProvider() { $this->serverContainer->getL10NFactory(), $this->serverContainer->getURLGenerator(), $this->serverContainer->query(ITimeFactory::class), - $this->serverContainer->get(IManager::class), ); } diff --git a/tests/lib/Share20/DefaultShareProviderTest.php b/tests/lib/Share20/DefaultShareProviderTest.php index 9fbcebb2d63af..f977619b7b246 100644 --- a/tests/lib/Share20/DefaultShareProviderTest.php +++ b/tests/lib/Share20/DefaultShareProviderTest.php @@ -39,7 +39,6 @@ use OCP\IUserManager; use OCP\L10N\IFactory; use OCP\Mail\IMailer; -use OCP\Share\IManager as IShareManager; use OCP\Share\IShare; use PHPUnit\Framework\MockObject\MockObject; @@ -83,9 +82,6 @@ class DefaultShareProviderTest extends \Test\TestCase { /** @var ITimeFactory|MockObject */ protected $timeFactory; - /** @var IShareManager&MockObject */ - protected $shareManager; - protected function setUp(): void { $this->dbConn = \OC::$server->getDatabaseConnection(); $this->userManager = $this->createMock(IUserManager::class); @@ -97,7 +93,6 @@ protected function setUp(): void { $this->defaults = $this->getMockBuilder(Defaults::class)->disableOriginalConstructor()->getMock(); $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->timeFactory = $this->createMock(ITimeFactory::class); - $this->shareManager = $this->createMock(IShareManager::class); $this->userManager->expects($this->any())->method('userExists')->willReturn(true); $this->timeFactory->expects($this->any())->method('now')->willReturn(new \DateTimeImmutable("2023-05-04 00:00 Europe/Berlin")); @@ -114,8 +109,7 @@ protected function setUp(): void { $this->defaults, $this->l10nFactory, $this->urlGenerator, - $this->timeFactory, - $this->shareManager, + $this->timeFactory ); } @@ -476,8 +470,7 @@ public function testDeleteSingleShare() { $this->defaults, $this->l10nFactory, $this->urlGenerator, - $this->timeFactory, - $this->shareManager, + $this->timeFactory ]) ->setMethods(['getShareById']) ->getMock(); @@ -572,8 +565,7 @@ public function testDeleteGroupShareWithUserGroupShares() { $this->defaults, $this->l10nFactory, $this->urlGenerator, - $this->timeFactory, - $this->shareManager, + $this->timeFactory ]) ->setMethods(['getShareById']) ->getMock(); @@ -2533,8 +2525,7 @@ public function testGetSharesInFolder() { $this->defaults, $this->l10nFactory, $this->urlGenerator, - $this->timeFactory, - $this->shareManager, + $this->timeFactory ); $password = md5(time()); @@ -2632,8 +2623,7 @@ public function testGetAccessListNoCurrentAccessRequired() { $this->defaults, $this->l10nFactory, $this->urlGenerator, - $this->timeFactory, - $this->shareManager, + $this->timeFactory ); $u1 = $userManager->createUser('testShare1', 'test'); @@ -2729,8 +2719,7 @@ public function testGetAccessListCurrentAccessRequired() { $this->defaults, $this->l10nFactory, $this->urlGenerator, - $this->timeFactory, - $this->shareManager, + $this->timeFactory ); $u1 = $userManager->createUser('testShare1', 'test');