From c9b90b9e0bee91f773d66883de75b42493c0baa6 Mon Sep 17 00:00:00 2001 From: Louis Chemineau Date: Wed, 22 May 2024 11:55:47 +0200 Subject: fix(files_sharing): Delete user shares if needed when user is removed from a group Signed-off-by: Louis Chemineau --- lib/private/Share20/DefaultShareProvider.php | 48 ++++++++++++++++++++++++++-- lib/private/Share20/ProviderFactory.php | 1 + 2 files changed, 47 insertions(+), 2 deletions(-) (limited to 'lib') diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index f38dcf05298..883e1172bfe 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -53,6 +53,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\IShareProvider; use Psr\Log\LoggerInterface; @@ -103,6 +104,7 @@ class DefaultShareProvider implements IShareProvider { IFactory $l10nFactory, IURLGenerator $urlGenerator, ITimeFactory $timeFactory, + private IManager $shareManager, ) { $this->dbConn = $connection; $this->userManager = $userManager; @@ -1304,7 +1306,7 @@ class DefaultShareProvider implements IShareProvider { ->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']; @@ -1321,9 +1323,51 @@ class DefaultShareProvider implements IShareProvider { ->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()) { + $deleteQuery = $this->dbConn->getQueryBuilder(); + $deleteQuery->delete('share') + ->where($deleteQuery->expr()->in('id', $deleteQuery->createParameter('id'))); + + // Delete direct shares received by the user from users in the group. + $selectInboundShares = $this->dbConn->getQueryBuilder(); + $selectInboundShares->select('id') + ->from('share', 's') + ->join('s', 'group_user', 'g', 's.uid_initiator = g.uid') + ->where($selectInboundShares->expr()->eq('share_type', $selectInboundShares->createNamedParameter(IShare::TYPE_USER))) + ->andWhere($selectInboundShares->expr()->eq('share_with', $selectInboundShares->createNamedParameter($uid))) + ->andWhere($selectInboundShares->expr()->eq('gid', $selectInboundShares->createNamedParameter($gid))) + ->setMaxResults(1000) + ->executeQuery(); + + do { + $rows = $selectInboundShares->executeQuery(); + $ids = $rows->fetchAll(); + $deleteQuery->setParameter('id', array_column($ids, 'id'), IQueryBuilder::PARAM_INT_ARRAY); + $deleteQuery->executeStatement(); + } while (count($ids) > 0); + + // Delete direct shares from the user to users in the group. + $selectOutboundShares = $this->dbConn->getQueryBuilder(); + $selectOutboundShares->select('id') + ->from('share', 's') + ->join('s', 'group_user', 'g', 's.share_with = g.uid') + ->where($selectOutboundShares->expr()->eq('share_type', $selectOutboundShares->createNamedParameter(IShare::TYPE_USER))) + ->andWhere($selectOutboundShares->expr()->eq('uid_initiator', $selectOutboundShares->createNamedParameter($uid))) + ->andWhere($selectOutboundShares->expr()->eq('gid', $selectOutboundShares->createNamedParameter($gid))) + ->setMaxResults(1000) + ->executeQuery(); + + do { + $rows = $selectOutboundShares->executeQuery(); + $ids = $rows->fetchAll(); + $deleteQuery->setParameter('id', array_column($ids, 'id'), IQueryBuilder::PARAM_INT_ARRAY); + $deleteQuery->executeStatement(); + } while (count($ids) > 0); + } } /** diff --git a/lib/private/Share20/ProviderFactory.php b/lib/private/Share20/ProviderFactory.php index cac25226f1a..49307e219c7 100644 --- a/lib/private/Share20/ProviderFactory.php +++ b/lib/private/Share20/ProviderFactory.php @@ -109,6 +109,7 @@ class ProviderFactory implements IProviderFactory { $this->serverContainer->getL10NFactory(), $this->serverContainer->getURLGenerator(), $this->serverContainer->query(ITimeFactory::class), + $this->serverContainer->get(IManager::class), ); } -- cgit v1.2.3 From 21e47d4ff9ce3e85080750b7a104278418dfc52c Mon Sep 17 00:00:00 2001 From: Côme Chilliet Date: Thu, 13 Jun 2024 17:05:29 +0200 Subject: fix: Remove shares only if there are no more common groups between users MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- .../e2e/files_sharing/limit_to_same_group.cy.ts | 29 ++++++++- lib/private/Share20/DefaultShareProvider.php | 74 +++++++++++----------- 2 files changed, 64 insertions(+), 39 deletions(-) (limited to 'lib') diff --git a/cypress/e2e/files_sharing/limit_to_same_group.cy.ts b/cypress/e2e/files_sharing/limit_to_same_group.cy.ts index 6d9a4170cba..0c771c931f7 100644 --- a/cypress/e2e/files_sharing/limit_to_same_group.cy.ts +++ b/cypress/e2e/files_sharing/limit_to_same_group.cy.ts @@ -29,11 +29,15 @@ describe('Limit to sharing to people in the same group', () => { 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') @@ -46,8 +50,13 @@ describe('Limit to sharing to people in the same group', () => { 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}`) @@ -77,11 +86,29 @@ describe('Limit to sharing to people in the same group', () => { cy.get(`[data-cy-files-list] [data-cy-files-list-row-name="${randomFileName1}"]`).should('exist') }) - context('Bob is removed from the group', () => { + 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') diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index 883e1172bfe..fe13e336712 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -1295,6 +1295,7 @@ class DefaultShareProvider implements IShareProvider { * * @param string $uid * @param string $gid + * @return void */ public function userDeletedFromGroup($uid, $gid) { /* @@ -1328,45 +1329,42 @@ class DefaultShareProvider implements IShareProvider { } if ($this->shareManager->shareWithGroupMembersOnly()) { - $deleteQuery = $this->dbConn->getQueryBuilder(); - $deleteQuery->delete('share') - ->where($deleteQuery->expr()->in('id', $deleteQuery->createParameter('id'))); + $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); - // Delete direct shares received by the user from users in the group. - $selectInboundShares = $this->dbConn->getQueryBuilder(); - $selectInboundShares->select('id') - ->from('share', 's') - ->join('s', 'group_user', 'g', 's.uid_initiator = g.uid') - ->where($selectInboundShares->expr()->eq('share_type', $selectInboundShares->createNamedParameter(IShare::TYPE_USER))) - ->andWhere($selectInboundShares->expr()->eq('share_with', $selectInboundShares->createNamedParameter($uid))) - ->andWhere($selectInboundShares->expr()->eq('gid', $selectInboundShares->createNamedParameter($gid))) - ->setMaxResults(1000) - ->executeQuery(); - - do { - $rows = $selectInboundShares->executeQuery(); - $ids = $rows->fetchAll(); - $deleteQuery->setParameter('id', array_column($ids, 'id'), IQueryBuilder::PARAM_INT_ARRAY); - $deleteQuery->executeStatement(); - } while (count($ids) > 0); - - // Delete direct shares from the user to users in the group. - $selectOutboundShares = $this->dbConn->getQueryBuilder(); - $selectOutboundShares->select('id') - ->from('share', 's') - ->join('s', 'group_user', 'g', 's.share_with = g.uid') - ->where($selectOutboundShares->expr()->eq('share_type', $selectOutboundShares->createNamedParameter(IShare::TYPE_USER))) - ->andWhere($selectOutboundShares->expr()->eq('uid_initiator', $selectOutboundShares->createNamedParameter($uid))) - ->andWhere($selectOutboundShares->expr()->eq('gid', $selectOutboundShares->createNamedParameter($gid))) - ->setMaxResults(1000) - ->executeQuery(); - - do { - $rows = $selectOutboundShares->executeQuery(); - $ids = $rows->fetchAll(); - $deleteQuery->setParameter('id', array_column($ids, 'id'), IQueryBuilder::PARAM_INT_ARRAY); - $deleteQuery->executeStatement(); - } while (count($ids) > 0); + 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); + } + } } } -- cgit v1.2.3