Skip to content

Commit 617e856

Browse files
blizzzblizzz
authored
Merge pull request #47227 from nextcloud/backport/47192/stable28
[stable28] Apply group limit on remove from group
2 parents e889f33 + aa25f60 commit 617e856

File tree

4 files changed

+168
-8
lines changed

4 files changed

+168
-8
lines changed

‎cypress/e2e/files_sharing/limit_to_same_group.cy.ts‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
/**
2+
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
3+
* SPDX-License-Identifier: AGPL-3.0-or-later
4+
*/
5+
6+
import { User } from "@nextcloud/cypress"
7+
import { createShare } from "./FilesSharingUtils.ts"
8+
9+
describe('Limit to sharing to people in the same group', () => {
10+
let alice: User
11+
let bob: User
12+
let randomFileName1 = ''
13+
let randomFileName2 = ''
14+
let randomGroupName = ''
15+
let randomGroupName2 = ''
16+
let randomGroupName3 = ''
17+
18+
before(() => {
19+
randomFileName1 = Math.random().toString(36).replace(/[^a-z]+/g, '').substring(0, 10) + '.txt'
20+
randomFileName2 = Math.random().toString(36).replace(/[^a-z]+/g, '').substring(0, 10) + '.txt'
21+
randomGroupName = Math.random().toString(36).replace(/[^a-z]+/g, '').substring(0, 10)
22+
randomGroupName2 = Math.random().toString(36).replace(/[^a-z]+/g, '').substring(0, 10)
23+
randomGroupName3 = Math.random().toString(36).replace(/[^a-z]+/g, '').substring(0, 10)
24+
25+
cy.runOccCommand('config:app:set core shareapi_only_share_with_group_members --value yes')
26+
27+
cy.createRandomUser()
28+
.then(user => {
29+
alice = user
30+
cy.createRandomUser()
31+
})
32+
.then(user => {
33+
bob = user
34+
35+
cy.runOccCommand(`group:add ${randomGroupName}`)
36+
cy.runOccCommand(`group:add ${randomGroupName2}`)
37+
cy.runOccCommand(`group:add ${randomGroupName3}`)
38+
cy.runOccCommand(`group:adduser ${randomGroupName} ${alice.userId}`)
39+
cy.runOccCommand(`group:adduser ${randomGroupName} ${bob.userId}`)
40+
cy.runOccCommand(`group:adduser ${randomGroupName2} ${alice.userId}`)
41+
cy.runOccCommand(`group:adduser ${randomGroupName2} ${bob.userId}`)
42+
cy.runOccCommand(`group:adduser ${randomGroupName3} ${bob.userId}`)
43+
44+
cy.uploadContent(alice, new Blob(['share to bob'], { type: 'text/plain' }), 'text/plain', `/${randomFileName1}`)
45+
cy.uploadContent(bob, new Blob(['share by bob'], { type: 'text/plain' }), 'text/plain', `/${randomFileName2}`)
46+
47+
cy.login(alice)
48+
cy.visit('/apps/files')
49+
createShare(randomFileName1, bob.userId)
50+
cy.login(bob)
51+
cy.visit('/apps/files')
52+
createShare(randomFileName2, alice.userId)
53+
})
54+
})
55+
56+
after(() => {
57+
cy.runOccCommand('config:app:set core shareapi_only_share_with_group_members --value no')
58+
})
59+
60+
it('Alice can see the shared file', () => {
61+
cy.login(alice)
62+
cy.visit('/apps/files')
63+
cy.get(`[data-cy-files-list] [data-cy-files-list-row-name="${randomFileName2}"]`).should('exist')
64+
})
65+
66+
it('Bob can see the shared file', () => {
67+
cy.login(alice)
68+
cy.visit('/apps/files')
69+
cy.get(`[data-cy-files-list] [data-cy-files-list-row-name="${randomFileName1}"]`).should('exist')
70+
})
71+
72+
context('Bob is removed from the first group', () => {
73+
before(() => {
74+
cy.runOccCommand(`group:removeuser ${randomGroupName} ${bob.userId}`)
75+
})
76+
77+
it('Alice can see the shared file', () => {
78+
cy.login(alice)
79+
cy.visit('/apps/files')
80+
cy.get(`[data-cy-files-list] [data-cy-files-list-row-name="${randomFileName2}"]`).should('exist')
81+
})
82+
83+
it('Bob can see the shared file', () => {
84+
cy.login(alice)
85+
cy.visit('/apps/files')
86+
cy.get(`[data-cy-files-list] [data-cy-files-list-row-name="${randomFileName1}"]`).should('exist')
87+
})
88+
})
89+
90+
context('Bob is removed from the second group', () => {
91+
before(() => {
92+
cy.runOccCommand(`group:removeuser ${randomGroupName2} ${bob.userId}`)
93+
})
94+
95+
it('Alice cannot see the shared file', () => {
96+
cy.login(alice)
97+
cy.visit('/apps/files')
98+
cy.get(`[data-cy-files-list] [data-cy-files-list-row-name="${randomFileName2}"]`).should('not.exist')
99+
})
100+
101+
it('Bob cannot see the shared file', () => {
102+
cy.login(alice)
103+
cy.visit('/apps/files')
104+
cy.get(`[data-cy-files-list] [data-cy-files-list-row-name="${randomFileName1}"]`).should('not.exist')
105+
})
106+
})
107+
})

‎lib/private/Share20/DefaultShareProvider.php‎

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@
5353
use OCP\Mail\IMailer;
5454
use OCP\Share\Exceptions\ShareNotFound;
5555
use OCP\Share\IAttributes;
56+
use OCP\Share\IManager;
5657
use OCP\Share\IShare;
5758
use OCP\Share\IShareProvider;
5859
use function str_starts_with;
@@ -102,6 +103,7 @@ public function __construct(
102103
IFactory $l10nFactory,
103104
IURLGenerator $urlGenerator,
104105
ITimeFactory $timeFactory,
106+
private IManager $shareManager,
105107
) {
106108
$this->dbConn = $connection;
107109
$this->userManager = $userManager;
@@ -1304,6 +1306,7 @@ public function groupDeleted($gid) {
13041306
*
13051307
* @param string $uid
13061308
* @param string $gid
1309+
* @return void
13071310
*/
13081311
public function userDeletedFromGroup($uid, $gid) {
13091312
/*
@@ -1315,7 +1318,7 @@ public function userDeletedFromGroup($uid, $gid) {
13151318
->where($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_GROUP)))
13161319
->andWhere($qb->expr()->eq('share_with', $qb->createNamedParameter($gid)));
13171320

1318-
$cursor = $qb->execute();
1321+
$cursor = $qb->executeQuery();
13191322
$ids = [];
13201323
while ($row = $cursor->fetch()) {
13211324
$ids[] = (int)$row['id'];
@@ -1332,7 +1335,45 @@ public function userDeletedFromGroup($uid, $gid) {
13321335
->where($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_USERGROUP)))
13331336
->andWhere($qb->expr()->eq('share_with', $qb->createNamedParameter($uid)))
13341337
->andWhere($qb->expr()->in('parent', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_INT_ARRAY)));
1335-
$qb->execute();
1338+
$qb->executeStatement();
1339+
}
1340+
}
1341+
1342+
if ($this->shareManager->shareWithGroupMembersOnly()) {
1343+
$user = $this->userManager->get($uid);
1344+
if ($user === null) {
1345+
return;
1346+
}
1347+
$userGroups = $this->groupManager->getUserGroupIds($user);
1348+
1349+
// Delete user shares received by the user from users in the group.
1350+
$userReceivedShares = $this->shareManager->getSharedWith($uid, IShare::TYPE_USER, null, -1);
1351+
foreach ($userReceivedShares as $share) {
1352+
$owner = $this->userManager->get($share->getSharedBy());
1353+
if ($owner === null) {
1354+
continue;
1355+
}
1356+
$ownerGroups = $this->groupManager->getUserGroupIds($owner);
1357+
$mutualGroups = array_intersect($userGroups, $ownerGroups);
1358+
1359+
if (count($mutualGroups) === 0) {
1360+
$this->shareManager->deleteShare($share);
1361+
}
1362+
}
1363+
1364+
// Delete user shares from the user to users in the group.
1365+
$userEmittedShares = $this->shareManager->getSharesBy($uid, IShare::TYPE_USER, null, true, -1);
1366+
foreach ($userEmittedShares as $share) {
1367+
$recipient = $this->userManager->get($share->getSharedWith());
1368+
if ($recipient === null) {
1369+
continue;
1370+
}
1371+
$recipientGroups = $this->groupManager->getUserGroupIds($recipient);
1372+
$mutualGroups = array_intersect($userGroups, $recipientGroups);
1373+
1374+
if (count($mutualGroups) === 0) {
1375+
$this->shareManager->deleteShare($share);
1376+
}
13361377
}
13371378
}
13381379
}

‎lib/private/Share20/ProviderFactory.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ protected function defaultShareProvider() {
106106
$this->serverContainer->getL10NFactory(),
107107
$this->serverContainer->getURLGenerator(),
108108
$this->serverContainer->query(ITimeFactory::class),
109+
$this->serverContainer->get(IManager::class),
109110
);
110111
}
111112

‎tests/lib/Share20/DefaultShareProviderTest.php‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
use OCP\IUserManager;
4040
use OCP\L10N\IFactory;
4141
use OCP\Mail\IMailer;
42+
use OCP\Share\IManager as IShareManager;
4243
use OCP\Share\IShare;
4344
use PHPUnit\Framework\MockObject\MockObject;
4445

@@ -82,6 +83,9 @@ class DefaultShareProviderTest extends \Test\TestCase {
8283
/** @var ITimeFactory|MockObject */
8384
protected $timeFactory;
8485

86+
/** @var IShareManager&MockObject */
87+
protected $shareManager;
88+
8589
protected function setUp(): void {
8690
$this->dbConn = \OC::$server->getDatabaseConnection();
8791
$this->userManager = $this->createMock(IUserManager::class);
@@ -93,6 +97,7 @@ protected function setUp(): void {
9397
$this->defaults = $this->getMockBuilder(Defaults::class)->disableOriginalConstructor()->getMock();
9498
$this->urlGenerator = $this->createMock(IURLGenerator::class);
9599
$this->timeFactory = $this->createMock(ITimeFactory::class);
100+
$this->shareManager = $this->createMock(IShareManager::class);
96101

97102
$this->userManager->expects($this->any())->method('userExists')->willReturn(true);
98103
$this->timeFactory->expects($this->any())->method('now')->willReturn(new \DateTimeImmutable("2023-05-04 00:00 Europe/Berlin"));
@@ -109,7 +114,8 @@ protected function setUp(): void {
109114
$this->defaults,
110115
$this->l10nFactory,
111116
$this->urlGenerator,
112-
$this->timeFactory
117+
$this->timeFactory,
118+
$this->shareManager,
113119
);
114120
}
115121

@@ -470,7 +476,8 @@ public function testDeleteSingleShare() {
470476
$this->defaults,
471477
$this->l10nFactory,
472478
$this->urlGenerator,
473-
$this->timeFactory
479+
$this->timeFactory,
480+
$this->shareManager,
474481
])
475482
->setMethods(['getShareById'])
476483
->getMock();
@@ -565,7 +572,8 @@ public function testDeleteGroupShareWithUserGroupShares() {
565572
$this->defaults,
566573
$this->l10nFactory,
567574
$this->urlGenerator,
568-
$this->timeFactory
575+
$this->timeFactory,
576+
$this->shareManager,
569577
])
570578
->setMethods(['getShareById'])
571579
->getMock();
@@ -2525,7 +2533,8 @@ public function testGetSharesInFolder() {
25252533
$this->defaults,
25262534
$this->l10nFactory,
25272535
$this->urlGenerator,
2528-
$this->timeFactory
2536+
$this->timeFactory,
2537+
$this->shareManager,
25292538
);
25302539

25312540
$password = md5(time());
@@ -2623,7 +2632,8 @@ public function testGetAccessListNoCurrentAccessRequired() {
26232632
$this->defaults,
26242633
$this->l10nFactory,
26252634
$this->urlGenerator,
2626-
$this->timeFactory
2635+
$this->timeFactory,
2636+
$this->shareManager,
26272637
);
26282638

26292639
$u1 = $userManager->createUser('testShare1', 'test');
@@ -2719,7 +2729,8 @@ public function testGetAccessListCurrentAccessRequired() {
27192729
$this->defaults,
27202730
$this->l10nFactory,
27212731
$this->urlGenerator,
2722-
$this->timeFactory
2732+
$this->timeFactory,
2733+
$this->shareManager,
27232734
);
27242735

27252736
$u1 = $userManager->createUser('testShare1', 'test');

0 commit comments

Comments
 (0)