Skip to content

[stable33] fix: don't rely on share providers being avaiable in CleanupShareTarget #57927

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
icewind1991 wants to merge 1 commit into
base: stable33
Choose a base branch
from
+36 −23
Open

[stable33] fix: don't rely on share providers being avaiable in CleanupShareTarget #57927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 30 additions & 17 deletions apps/files_sharing/lib/Repair/CleanupShareTarget.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,17 @@
use OC\Files\SetupManager;
use OCA\Files_Sharing\ShareTargetValidator;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\Files\IRootFolder;
use OCP\Files\Mount\IMountManager;
use OCP\IDBConnection;
use OCP\IUserManager;
use OCP\Migration\IOutput;
use OCP\Migration\IRepairStep;
use OCP\Share\IManager;
use OCP\Share\IProviderFactory;
use OCP\Share\IShare;

/**
* @psalm-type ShareInfo = array{id: string|int, share_type: string, share_with: string, file_source: string, file_target: string}
*/
class CleanupShareTarget implements IRepairStep {
/** we only care about shares with a user target,
* since the underling group/deck/talk share doesn't get moved
Expand All @@ -33,12 +35,11 @@ class CleanupShareTarget implements IRepairStep {

public function __construct(
private readonly IDBConnection $connection,
private readonly IManager $shareManager,
private readonly IProviderFactory $shareProviderFactory,
private readonly ShareTargetValidator $shareTargetValidator,
private readonly IUserManager $userManager,
private readonly SetupManager $setupManager,
private readonly IMountManager $mountManager,
private readonly IRootFolder $rootFolder,
) {
}

Expand All @@ -57,12 +58,10 @@ public function run(IOutput $output) {

$lastUser = '';
$userMounts = [];
$userFolder = null;

foreach ($this->getProblemShares() as $shareInfo) {
$recipient = $this->userManager->getExistingUser($shareInfo['share_with']);
$share = $this->shareProviderFactory
->getProviderForType((int)$shareInfo['share_type'])
->getShareById($shareInfo['id'], $recipient->getUID());

// since we ordered the share by user, we can reuse the last data until we get to the next user
if ($lastUser !== $recipient->getUID()) {
Expand All @@ -71,19 +70,23 @@ public function run(IOutput $output) {
$this->setupManager->tearDown();
$this->setupManager->setupForUser($recipient);
$userMounts = $this->mountManager->getAll();
$userFolder = $this->rootFolder->getUserFolder($recipient->getUID());
}

$oldTarget = $share->getTarget();
$oldTarget = $shareInfo['file_target'];
$newTarget = $this->cleanTarget($oldTarget);
$share->setTarget($newTarget);
$this->shareManager->moveShare($share, $recipient->getUID());
$absoluteNewTarget = $userFolder->getFullPath($newTarget);
$targetParentNode = $this->rootFolder->get(dirname($absoluteNewTarget));

$this->shareTargetValidator->verifyMountPoint(
$recipient,
$share,
$absoluteNewTarget = $this->shareTargetValidator->generateUniqueTarget(
(int)$shareInfo['file_source'],
$absoluteNewTarget,
$targetParentNode->getMountPoint(),
$userMounts,
[$share],
);
$newTarget = $userFolder->getRelativePath($absoluteNewTarget);

$this->moveShare((string)$shareInfo['id'], $newTarget);

$oldMountPoint = "/{$recipient->getUID()}/files$oldTarget/";
$newMountPoint = "/{$recipient->getUID()}/files$newTarget/";
Expand All @@ -105,19 +108,29 @@ private function countProblemShares(): int {
return (int)$query->executeQuery()->fetchOne();
}

private function moveShare(string $id, string $target) {
Copy link
Member

@CarlSchwan CarlSchwan Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private function moveShare(string $id, string $target) {
private function moveShare(string $id, string $target): void {

// since we only process user-specific shares, we can just move them
// without having to check if we need to create a user-specific override
$query = $this->connection->getQueryBuilder();
$query->update('share')
->set('file_target', $query->createNamedParameter($target))
->where($query->expr()->eq('id', $query->createNamedParameter($id)))
->executeStatement();
}

/**
* @return \Traversable<array{id: string, share_type: string, share_with: string}>
* @return \Traversable<ShareInfo>
*/
private function getProblemShares(): \Traversable {
$query = $this->connection->getQueryBuilder();
$query->select('id', 'share_type', 'share_with')
$query->select('id', 'share_type', 'share_with', 'file_source', 'file_target')
->from('share')
->where($query->expr()->like('file_target', $query->createNamedParameter('% (_) (_)%')))
->andWhere($query->expr()->in('share_type', $query->createNamedParameter(self::USER_SHARE_TYPES, IQueryBuilder::PARAM_INT_ARRAY), IQueryBuilder::PARAM_INT_ARRAY))
->orderBy('share_with')
->addOrderBy('id');
$result = $query->executeQuery();
/** @var \Traversable<array{id: string, share_type: string, share_with: string}> $rows */
/** @var \Traversable<ShareInfo> $rows */
$rows = $result->iterateAssociative();
return $rows;
}
Expand Down
12 changes: 6 additions & 6 deletions apps/files_sharing/lib/ShareTargetValidator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ public function verifyMountPoint(
}

$newAbsoluteMountPoint = $this->generateUniqueTarget(
$share,
$share->getNodeId(),
Filesystem::normalizePath($absoluteParent . '/' . $mountPoint),
$parentMount,
$allCachedMounts,
Expand All @@ -107,8 +107,8 @@ public function verifyMountPoint(
/**
* @param IMountPoint[] $allCachedMounts
*/
private function generateUniqueTarget(
IShare $share,
public function generateUniqueTarget(
int $shareNodeId,
string $absolutePath,
IMountPoint $parentMount,
array $allCachedMounts,
Expand All @@ -121,7 +121,7 @@ private function generateUniqueTarget(
$i = 2;
$parentCache = $parentMount->getStorage()->getCache();
$internalPath = $parentMount->getInternalPath($absolutePath);
while ($parentCache->inCache($internalPath) || $this->hasConflictingMount($share, $allCachedMounts, $absolutePath)) {
while ($parentCache->inCache($internalPath) || $this->hasConflictingMount($shareNodeId, $allCachedMounts, $absolutePath)) {
$absolutePath = Filesystem::normalizePath($dir . '/' . $name . ' (' . $i . ')' . $ext);
$internalPath = $parentMount->getInternalPath($absolutePath);
$i++;
Expand All @@ -133,13 +133,13 @@ private function generateUniqueTarget(
/**
* @param IMountPoint[] $allCachedMounts
*/
private function hasConflictingMount(IShare $share, array $allCachedMounts, string $absolutePath): bool {
private function hasConflictingMount(int $shareNodeId, array $allCachedMounts, string $absolutePath): bool {
if (!isset($allCachedMounts[$absolutePath . '/'])) {
return false;
}

$mount = $allCachedMounts[$absolutePath . '/'];
if ($mount instanceof SharedMount && $mount->getShare()->getNodeId() === $share->getNodeId()) {
if ($mount instanceof SharedMount && $mount->getShare()->getNodeId() === $shareNodeId) {
// "conflicting" mount is a mount for the current share
return false;
}
Expand Down
Loading