Skip to content

Commit ac7f429

Browse files
AndyScherzingerAndyScherzinger
authored
Merge pull request #60925 from nextcloud/backport/59813/stable33
[stable33] fix(sharing): set STATUS_ACCEPTED when creating USERGROUP subshare on…
2 parents 107f433 + 85202d7 commit ac7f429

9 files changed

Lines changed: 335 additions & 1 deletion

File tree

‎apps/files_sharing/appinfo/info.xml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ Turning the feature off removes shared files and folders on the server for all s
4949
<command>OCA\Files_Sharing\Command\CleanupRemoteStorages</command>
5050
<command>OCA\Files_Sharing\Command\ExiprationNotification</command>
5151
<command>OCA\Files_Sharing\Command\DeleteOrphanShares</command>
52+
<command>OCA\Files_Sharing\Command\FixOwncloudGroupSubshareStatus</command>
5253
<command>OCA\Files_Sharing\Command\FixShareOwners</command>
5354
<command>OCA\Files_Sharing\Command\ListShares</command>
5455
</commands>

‎apps/files_sharing/composer/composer/autoload_classmap.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
'OCA\\Files_Sharing\\Command\\CleanupRemoteStorages' => $baseDir . '/../lib/Command/CleanupRemoteStorages.php',
2828
'OCA\\Files_Sharing\\Command\\DeleteOrphanShares' => $baseDir . '/../lib/Command/DeleteOrphanShares.php',
2929
'OCA\\Files_Sharing\\Command\\ExiprationNotification' => $baseDir . '/../lib/Command/ExiprationNotification.php',
30+
'OCA\\Files_Sharing\\Command\\FixOwncloudGroupSubshareStatus' => $baseDir . '/../lib/Command/FixOwncloudGroupSubshareStatus.php',
3031
'OCA\\Files_Sharing\\Command\\FixShareOwners' => $baseDir . '/../lib/Command/FixShareOwners.php',
3132
'OCA\\Files_Sharing\\Command\\ListShares' => $baseDir . '/../lib/Command/ListShares.php',
3233
'OCA\\Files_Sharing\\Config\\ConfigLexicon' => $baseDir . '/../lib/Config/ConfigLexicon.php',

‎apps/files_sharing/composer/composer/autoload_static.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ class ComposerStaticInitFiles_Sharing
4242
'OCA\\Files_Sharing\\Command\\CleanupRemoteStorages' => __DIR__ . '/..' . '/../lib/Command/CleanupRemoteStorages.php',
4343
'OCA\\Files_Sharing\\Command\\DeleteOrphanShares' => __DIR__ . '/..' . '/../lib/Command/DeleteOrphanShares.php',
4444
'OCA\\Files_Sharing\\Command\\ExiprationNotification' => __DIR__ . '/..' . '/../lib/Command/ExiprationNotification.php',
45+
'OCA\\Files_Sharing\\Command\\FixOwncloudGroupSubshareStatus' => __DIR__ . '/..' . '/../lib/Command/FixOwncloudGroupSubshareStatus.php',
4546
'OCA\\Files_Sharing\\Command\\FixShareOwners' => __DIR__ . '/..' . '/../lib/Command/FixShareOwners.php',
4647
'OCA\\Files_Sharing\\Command\\ListShares' => __DIR__ . '/..' . '/../lib/Command/ListShares.php',
4748
'OCA\\Files_Sharing\\Config\\ConfigLexicon' => __DIR__ . '/..' . '/../lib/Config/ConfigLexicon.php',

‎apps/files_sharing/lib/Command/FixOwncloudGroupSubshareStatus.php‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\Files_Sharing\Command;
11+
12+
use OC\Core\Command\Base;
13+
use OCP\DB\QueryBuilder\IQueryBuilder;
14+
use OCP\IDBConnection;
15+
use OCP\Share\IShare;
16+
use Symfony\Component\Console\Input\InputInterface;
17+
use Symfony\Component\Console\Input\InputOption;
18+
use Symfony\Component\Console\Output\OutputInterface;
19+
20+
/**
21+
* Fixes USERGROUP subshares that were created without `accepted = STATUS_ACCEPTED`
22+
* by a rename on an ownCloud-migrated instance.
23+
*
24+
* When an OC-migrated group share (which has no per-user USERGROUP subshare) is
25+
* renamed for the first time, DefaultShareProvider::move() inserted a new USERGROUP
26+
* row without setting `accepted`. The column defaulted to 0 (STATUS_PENDING), causing
27+
* MountProvider to skip the share on the next login — the file disappeared for the
28+
* recipient.
29+
*
30+
* USERGROUP subshares with permissions = 0 were explicitly declined by the user
31+
* and are left untouched.
32+
*/
33+
class FixOwncloudGroupSubshareStatus extends Base {
34+
35+
public function __construct(
36+
private IDBConnection $connection,
37+
) {
38+
parent::__construct();
39+
}
40+
41+
#[\Override]
42+
protected function configure(): void {
43+
$this
44+
->setName('sharing:fix-owncloud-group-shares')
45+
->setDescription('Fix group share subshares left pending after renaming on an ownCloud-migrated instance')
46+
->addOption(
47+
'dry-run',
48+
null,
49+
InputOption::VALUE_NONE,
50+
'Show how many shares would be fixed without making any changes',
51+
);
52+
}
53+
54+
#[\Override]
55+
public function execute(InputInterface $input, OutputInterface $output): int {
56+
$dryRun = $input->getOption('dry-run');
57+
58+
$qb = $this->connection->getQueryBuilder();
59+
$count = (int)$qb->select($qb->func()->count('id'))
60+
->from('share')
61+
->where($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_USERGROUP, IQueryBuilder::PARAM_INT)))
62+
->andWhere($qb->expr()->eq('accepted', $qb->createNamedParameter(IShare::STATUS_PENDING, IQueryBuilder::PARAM_INT)))
63+
->andWhere($qb->expr()->neq('permissions', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)))
64+
->executeQuery()
65+
->fetchOne();
66+
67+
if ($count === 0) {
68+
$output->writeln('No affected group share subshares found.');
69+
return self::SUCCESS;
70+
}
71+
72+
if ($dryRun) {
73+
$output->writeln("Would fix <info>$count</info> group share subshare(s) (dry-run, no changes made).");
74+
return self::SUCCESS;
75+
}
76+
77+
$qb = $this->connection->getQueryBuilder();
78+
$qb->update('share')
79+
->set('accepted', $qb->createNamedParameter(IShare::STATUS_ACCEPTED, IQueryBuilder::PARAM_INT))
80+
->where($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_USERGROUP, IQueryBuilder::PARAM_INT)))
81+
->andWhere($qb->expr()->eq('accepted', $qb->createNamedParameter(IShare::STATUS_PENDING, IQueryBuilder::PARAM_INT)))
82+
->andWhere($qb->expr()->neq('permissions', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)))
83+
->executeStatement();
84+
85+
$output->writeln("Fixed <info>$count</info> group share subshare(s).");
86+
return self::SUCCESS;
87+
}
88+
}

‎apps/files_sharing/lib/SharedMount.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ public function __construct(
5858
* @param IShare $share
5959
* @return bool
6060
*/
61-
private function updateFileTarget($newPath, &$share) {
61+
protected function updateFileTarget($newPath, &$share) {
6262
$share->setTarget($newPath);
6363

6464
foreach ($this->groupedShares as $tmpShare) {
@@ -117,6 +117,7 @@ public function moveMount($target) {
117117
'exception' => $e,
118118
]
119119
);
120+
$result = false;
120121
}
121122

122123
return $result;

‎apps/files_sharing/tests/Command/FixOwncloudGroupSubshareStatusTest.php‎

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\Files_Sharing\Tests\Command;
11+
12+
use OCA\Files_Sharing\Command\FixOwncloudGroupSubshareStatus;
13+
use OCA\Files_Sharing\Tests\TestCase;
14+
use OCP\DB\QueryBuilder\IQueryBuilder;
15+
use OCP\IDBConnection;
16+
use OCP\Server;
17+
use OCP\Share\IShare;
18+
use Symfony\Component\Console\Tester\CommandTester;
19+
20+
#[\PHPUnit\Framework\Attributes\Group(name: 'DB')]
21+
class FixOwncloudGroupSubshareStatusTest extends TestCase {
22+
23+
private IDBConnection $connection;
24+
private CommandTester $commandTester;
25+
26+
protected function setUp(): void {
27+
parent::setUp();
28+
$this->connection = Server::get(IDBConnection::class);
29+
$this->commandTester = new CommandTester(new FixOwncloudGroupSubshareStatus($this->connection));
30+
$this->cleanDB();
31+
}
32+
33+
protected function tearDown(): void {
34+
$this->cleanDB();
35+
parent::tearDown();
36+
}
37+
38+
private function cleanDB(): void {
39+
$this->connection->getQueryBuilder()->delete('share')->executeStatement();
40+
}
41+
42+
private function insertShare(int $shareType, int $accepted, int $permissions): int {
43+
$qb = $this->connection->getQueryBuilder();
44+
$qb->insert('share')
45+
->values([
46+
'share_type' => $qb->createNamedParameter($shareType, IQueryBuilder::PARAM_INT),
47+
'share_with' => $qb->createNamedParameter('user1'),
48+
'uid_owner' => $qb->createNamedParameter('owner'),
49+
'uid_initiator' => $qb->createNamedParameter('owner'),
50+
'parent' => $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT),
51+
'item_type' => $qb->createNamedParameter('file'),
52+
'item_source' => $qb->createNamedParameter('42'),
53+
'item_target' => $qb->createNamedParameter('/file'),
54+
'file_source' => $qb->createNamedParameter(42, IQueryBuilder::PARAM_INT),
55+
'file_target' => $qb->createNamedParameter('/file'),
56+
'permissions' => $qb->createNamedParameter($permissions, IQueryBuilder::PARAM_INT),
57+
'stime' => $qb->createNamedParameter(time(), IQueryBuilder::PARAM_INT),
58+
'accepted' => $qb->createNamedParameter($accepted, IQueryBuilder::PARAM_INT),
59+
])
60+
->executeStatement();
61+
return (int)$this->connection->lastInsertId('*PREFIX*share');
62+
}
63+
64+
private function getAccepted(int $id): int {
65+
$qb = $this->connection->getQueryBuilder();
66+
return (int)$qb->select('accepted')
67+
->from('share')
68+
->where($qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT)))
69+
->executeQuery()
70+
->fetchOne();
71+
}
72+
73+
public function testFixesPendingSubshareWithPermissions(): void {
74+
$id = $this->insertShare(IShare::TYPE_USERGROUP, IShare::STATUS_PENDING, 31);
75+
76+
$this->commandTester->execute([]);
77+
78+
$this->assertSame(IShare::STATUS_ACCEPTED, $this->getAccepted($id));
79+
$this->assertStringContainsString('Fixed', $this->commandTester->getDisplay());
80+
}
81+
82+
public function testDryRunShowsCountWithoutChanging(): void {
83+
$id = $this->insertShare(IShare::TYPE_USERGROUP, IShare::STATUS_PENDING, 31);
84+
85+
$this->commandTester->execute(['--dry-run' => true]);
86+
87+
$this->assertSame(IShare::STATUS_PENDING, $this->getAccepted($id));
88+
$this->assertStringContainsString('dry-run', $this->commandTester->getDisplay());
89+
}
90+
91+
public function testDoesNotTouchDeclinedSubshare(): void {
92+
// permissions = 0 means the user explicitly declined the share
93+
$id = $this->insertShare(IShare::TYPE_USERGROUP, IShare::STATUS_PENDING, 0);
94+
95+
$this->commandTester->execute([]);
96+
97+
$this->assertSame(IShare::STATUS_PENDING, $this->getAccepted($id));
98+
$this->assertStringContainsString('No affected', $this->commandTester->getDisplay());
99+
}
100+
101+
public function testDoesNotTouchAlreadyAcceptedSubshare(): void {
102+
$id = $this->insertShare(IShare::TYPE_USERGROUP, IShare::STATUS_ACCEPTED, 31);
103+
104+
$this->commandTester->execute([]);
105+
106+
$this->assertSame(IShare::STATUS_ACCEPTED, $this->getAccepted($id));
107+
$this->assertStringContainsString('No affected', $this->commandTester->getDisplay());
108+
}
109+
110+
public function testDoesNotTouchNonUsergroupShares(): void {
111+
$id = $this->insertShare(IShare::TYPE_GROUP, IShare::STATUS_PENDING, 31);
112+
113+
$this->commandTester->execute([]);
114+
115+
$this->assertSame(IShare::STATUS_PENDING, $this->getAccepted($id));
116+
$this->assertStringContainsString('No affected', $this->commandTester->getDisplay());
117+
}
118+
119+
public function testFixesMultipleAffectedRows(): void {
120+
$id1 = $this->insertShare(IShare::TYPE_USERGROUP, IShare::STATUS_PENDING, 31);
121+
$id2 = $this->insertShare(IShare::TYPE_USERGROUP, IShare::STATUS_PENDING, 17);
122+
$idDeclined = $this->insertShare(IShare::TYPE_USERGROUP, IShare::STATUS_PENDING, 0);
123+
124+
$this->commandTester->execute([]);
125+
126+
$this->assertSame(IShare::STATUS_ACCEPTED, $this->getAccepted($id1));
127+
$this->assertSame(IShare::STATUS_ACCEPTED, $this->getAccepted($id2));
128+
$this->assertSame(IShare::STATUS_PENDING, $this->getAccepted($idDeclined));
129+
$this->assertStringContainsString('2', $this->commandTester->getDisplay());
130+
}
131+
}

‎apps/files_sharing/tests/SharedMountTest.php‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,9 @@
99

1010
use OC\Files\Filesystem;
1111
use OCA\Files_Sharing\SharedMount;
12+
use OCA\Files_Sharing\SharedStorage;
1213
use OCP\Constants;
14+
use OCP\DB\QueryBuilder\IQueryBuilder;
1315
use OCP\IDBConnection;
1416
use OCP\IGroupManager;
1517
use OCP\IUserManager;
@@ -171,6 +173,93 @@ public static function dataProviderTestStripUserFilesPath() {
171173
];
172174
}
173175

176+
/**
177+
* Simulate the ownCloud migration scenario: a TYPE_GROUP share whose `accepted`
178+
* column was set to STATUS_ACCEPTED by the SetAcceptedStatus repair step, but
179+
* which has no per-user USERGROUP subshare (OC never created them).
180+
*
181+
* On first rename, DefaultShareProvider::move() must create the USERGROUP subshare
182+
* with accepted = STATUS_ACCEPTED so MountProvider does not skip it on the next
183+
* login, causing the file to appear to vanish.
184+
*/
185+
public function testMoveOwncloudMigratedGroupShareRemainsVisibleAfterRename(): void {
186+
$testGroup = $this->groupManager->createGroup('testGroupOC');
187+
$user1 = $this->userManager->get(self::TEST_FILES_SHARING_API_USER1);
188+
$user2 = $this->userManager->get(self::TEST_FILES_SHARING_API_USER2);
189+
$testGroup->addUser($user1);
190+
$testGroup->addUser($user2);
191+
192+
// Create a group share without going through the NC acceptance flow,
193+
// then directly set accepted = STATUS_ACCEPTED on the GROUP row to mimic
194+
// the state left by SetAcceptedStatus after an OC→NC migration.
195+
$share = $this->share(
196+
IShare::TYPE_GROUP,
197+
$this->filename,
198+
self::TEST_FILES_SHARING_API_USER1,
199+
'testGroupOC',
200+
Constants::PERMISSION_READ | Constants::PERMISSION_UPDATE | Constants::PERMISSION_SHARE
201+
);
202+
203+
$db = Server::get(IDBConnection::class);
204+
205+
// Remove any USERGROUP subshares the acceptance flow may have created,
206+
// then stamp the GROUP row as STATUS_ACCEPTED — this is the OC migration state.
207+
$qb = $db->getQueryBuilder();
208+
$qb->delete('share')
209+
->where($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_USERGROUP, IQueryBuilder::PARAM_INT)))
210+
->andWhere($qb->expr()->eq('parent', $qb->createNamedParameter((int)$share->getId(), IQueryBuilder::PARAM_INT)))
211+
->executeStatement();
212+
213+
$qb = $db->getQueryBuilder();
214+
$qb->update('share')
215+
->set('accepted', $qb->createNamedParameter(IShare::STATUS_ACCEPTED, IQueryBuilder::PARAM_INT))
216+
->where($qb->expr()->eq('id', $qb->createNamedParameter((int)$share->getId(), IQueryBuilder::PARAM_INT)))
217+
->executeStatement();
218+
219+
// Log in as the recipient; the share should be visible via the GROUP row.
220+
self::loginHelper(self::TEST_FILES_SHARING_API_USER2);
221+
$this->assertTrue(Filesystem::file_exists($this->filename), 'Share should be visible before rename');
222+
223+
// Rename — this triggers move() which must create the USERGROUP subshare
224+
// with accepted = STATUS_ACCEPTED (not the default STATUS_PENDING).
225+
$renamed = Filesystem::rename($this->filename, $this->filename . '_oc_renamed');
226+
$this->assertTrue($renamed, 'Rename should succeed');
227+
$this->assertTrue(Filesystem::file_exists($this->filename . '_oc_renamed'));
228+
229+
// Re-login to flush the mount cache and force MountProvider to rebuild from DB.
230+
// If the USERGROUP subshare was created with accepted = STATUS_PENDING, the
231+
// share would be skipped here and the file would appear to vanish.
232+
self::loginHelper(self::TEST_FILES_SHARING_API_USER2);
233+
$this->assertTrue(
234+
Filesystem::file_exists($this->filename . '_oc_renamed'),
235+
'Share must still be visible after re-login — USERGROUP subshare must have accepted = STATUS_ACCEPTED'
236+
);
237+
238+
// Cleanup
239+
self::loginHelper(self::TEST_FILES_SHARING_API_USER1);
240+
$this->shareManager->deleteShare($share);
241+
$testGroup->removeUser($user1);
242+
$testGroup->removeUser($user2);
243+
$this->groupManager->get('testGroupOC')?->delete();
244+
}
245+
246+
/**
247+
* When updateFileTarget() throws — e.g. Manager::moveShare() rejects an
248+
* OC-migrated group share because the original group no longer exists —
249+
* moveMount() must return false so View::rename() propagates the failure
250+
* cleanly rather than silently corrupting the virtual filesystem state.
251+
*/
252+
public function testMoveMountReturnsFalseWhenUpdateFileTargetThrows(): void {
253+
$storage = $this->createMock(SharedStorage::class);
254+
$storage->method('getShare')->willReturn($this->createMock(IShare::class));
255+
256+
$mount = new SharedMountWithFailingUpdate($storage);
257+
258+
$result = $mount->moveMount('/' . self::TEST_FILES_SHARING_API_USER2 . '/files/newname');
259+
260+
$this->assertFalse($result);
261+
}
262+
174263
/**
175264
* If the permissions on a group share are upgraded be sure to still respect
176265
* removed shares by a member of that group
@@ -244,3 +333,18 @@ public function stripUserFilesPathDummy($path) {
244333
return $this->stripUserFilesPath($path);
245334
}
246335
}
336+
337+
/**
338+
* SharedMount subclass whose updateFileTarget() always throws, used to verify
339+
* that moveMount() returns false and does not silently corrupt VFS state.
340+
*/
341+
class SharedMountWithFailingUpdate extends SharedMount {
342+
public function __construct(SharedStorage $storage) {
343+
$this->storage = $storage;
344+
$this->mountPoint = '/testuser/files/testfile';
345+
}
346+
347+
protected function updateFileTarget($newPath, &$share): void {
348+
throw new \Exception('Simulated failure: group no longer exists');
349+
}
350+
}

‎lib/private/Share20/DefaultShareProvider.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -593,6 +593,7 @@ public function move(\OCP\Share\IShare $share, $recipient) {
593593
'permissions' => $qb->createNamedParameter($share->getPermissions()),
594594
'attributes' => $qb->createNamedParameter($shareAttributes),
595595
'stime' => $qb->createNamedParameter($share->getShareTime()->getTimestamp()),
596+
'accepted' => $qb->createNamedParameter(IShare::STATUS_ACCEPTED),
596597
])->executeStatement();
597598
} else {
598599
// Already a usergroup share. Update it.

0 commit comments

Comments
 (0)