Merge pull request #47180 from nextcloud/fix/apply-group-limit-on-remove-from-group

Apply group limit on remove from group

cypress/e2e/files_sharing/limit_to_same_group.cy.ts

@@ -0,0 +1,107 @@
+/**
+ * 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')
+		})
+	})
+})

lib/private/Share20/DefaultShareProvider.php

@@ -28,6 +28,7 @@ use OCP\L10N\IFactory;
 use OCP\Mail\IMailer;
 use OCP\Share\Exceptions\ShareNotFound;
 use OCP\Share\IAttributes;
+use OCP\Share\IManager;
 use OCP\Share\IShare;
 use OCP\Share\IShareProviderSupportsAccept;
 use OCP\Share\IShareProviderWithNotification;
@@ -54,6 +55,7 @@ class DefaultShareProvider implements IShareProviderWithNotification, IShareProv
 		private IURLGenerator $urlGenerator,
 		private ITimeFactory $timeFactory,
 		private LoggerInterface $logger,
+		private IManager $shareManager,
 	) {
 	}
 
@@ -1223,6 +1225,7 @@ class DefaultShareProvider implements IShareProviderWithNotification, IShareProv
 	 *
 	 * @param string $uid
 	 * @param string $gid
+	 * @return void
 	 */
 	public function userDeletedFromGroup($uid, $gid) {
 		/*
@@ -1234,7 +1237,7 @@ class DefaultShareProvider implements IShareProviderWithNotification, IShareProv
 			->where($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_GROUP)))
 			->andWhere($qb->expr()->eq('share_with', $qb->createNamedParameter($gid)));
 
-		$cursor = $qb->execute();
+		$cursor = $qb->executeQuery();
 		$ids = [];
 		while ($row = $cursor->fetch()) {
 			$ids[] = (int)$row['id'];
@@ -1251,7 +1254,46 @@ class DefaultShareProvider implements IShareProviderWithNotification, IShareProv
 					->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->execute();
+				$qb->executeStatement();
+			}
+		}
+
+		if ($this->shareManager->shareWithGroupMembersOnly()) {
+			$user = $this->userManager->get($uid);
+			if ($user === null) {
+				return;
+			}
+			$userGroups = $this->groupManager->getUserGroupIds($user);
+			$userGroups = array_diff($userGroups, $this->shareManager->shareWithGroupMembersOnlyExcludeGroupsList());
+
+			// 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);
+				}
 			}
 		}
 	}

lib/private/Share20/ProviderFactory.php

@@ -88,6 +88,7 @@ class ProviderFactory implements IProviderFactory {
 				$this->serverContainer->getURLGenerator(),
 				$this->serverContainer->query(ITimeFactory::class),
 				$this->serverContainer->get(LoggerInterface::class),
+				$this->serverContainer->get(IManager::class),
 			);
 		}
 

tests/lib/Share20/DefaultShareProviderTest.php

@@ -72,6 +72,8 @@ class DefaultShareProviderTest extends \Test\TestCase {
 	/** @var LoggerInterface|MockObject */
 	protected $logger;
 
+	protected IShareManager&MockObject $shareManager;
+
 	protected function setUp(): void {
 		$this->dbConn = \OC::$server->getDatabaseConnection();
 		$this->userManager = $this->createMock(IUserManager::class);
@@ -84,6 +86,7 @@ class DefaultShareProviderTest extends \Test\TestCase {
 		$this->urlGenerator = $this->createMock(IURLGenerator::class);
 		$this->timeFactory = $this->createMock(ITimeFactory::class);
 		$this->logger = $this->createMock(LoggerInterface::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"));
@@ -101,7 +104,8 @@ class DefaultShareProviderTest extends \Test\TestCase {
 			$this->l10nFactory,
 			$this->urlGenerator,
 			$this->timeFactory,
-			$this->logger
+			$this->logger,
+			$this->shareManager,
 		);
 	}
 
@@ -464,7 +468,8 @@ class DefaultShareProviderTest extends \Test\TestCase {
 				$this->l10nFactory,
 				$this->urlGenerator,
 				$this->timeFactory,
-				$this->logger
+				$this->logger,
+				$this->shareManager,
 			])
 			->setMethods(['getShareById'])
 			->getMock();
@@ -560,7 +565,8 @@ class DefaultShareProviderTest extends \Test\TestCase {
 				$this->l10nFactory,
 				$this->urlGenerator,
 				$this->timeFactory,
-				$this->logger
+				$this->logger,
+				$this->shareManager,
 			])
 			->setMethods(['getShareById'])
 			->getMock();
@@ -2529,7 +2535,8 @@ class DefaultShareProviderTest extends \Test\TestCase {
 			$this->l10nFactory,
 			$this->urlGenerator,
 			$this->timeFactory,
-			$this->logger
+			$this->logger,
+			$this->shareManager,
 		);
 
 		$password = md5(time());
@@ -2628,7 +2635,8 @@ class DefaultShareProviderTest extends \Test\TestCase {
 			$this->l10nFactory,
 			$this->urlGenerator,
 			$this->timeFactory,
-			$this->logger
+			$this->logger,
+			$this->shareManager,
 		);
 
 		$u1 = $userManager->createUser('testShare1', 'test');
@@ -2725,7 +2733,8 @@ class DefaultShareProviderTest extends \Test\TestCase {
 			$this->l10nFactory,
 			$this->urlGenerator,
 			$this->timeFactory,
-			$this->logger
+			$this->logger,
+			$this->shareManager,
 		);
 
 		$u1 = $userManager->createUser('testShare1', 'test');