Unified Sharing Legacy Sync by provokateurin · Pull Request #65972 · nextcloud/server

provokateurin · 2026-10-27T15:35:06Z

There are some TODOs left in the code, but I just need to get this merged, so future changes can be done in smaller PRs.

The syncing logic is not nice to understand, because it's an encoding of all the quirks and limitations the legacy sharing system has. I tried my best to leave useful comments whenever possible, but I'm happy to add more if there are questions during the review.

… the controller Signed-off-by: provokateurin <kate@provokateurin.de>

Signed-off-by: provokateurin <kate@provokateurin.de>

… a hash already Signed-off-by: provokateurin <kate@provokateurin.de>

…ead of load Signed-off-by: provokateurin <kate@provokateurin.de>

Signed-off-by: provokateurin <kate@provokateurin.de>

…ailable Signed-off-by: provokateurin <kate@provokateurin.de>

… unit tests Signed-off-by: provokateurin <kate@provokateurin.de>

…licit recipients Signed-off-by: provokateurin <kate@provokateurin.de>

Signed-off-by: provokateurin <kate@provokateurin.de>

icewind1991 · 2026-10-27T16:40:23Z

-				$share->getShareType(),
-				$share->getPermissions()
-			);
+			if (defined('PHPUNIT_RUN')) {


imo it's better to have a sendNotifications or something property and have the tests set that to false.

Or, maybe better, mock the $this->notifications

icewind1991 · 2026-10-27T16:43:59Z

+
+		$sourceNodeTargetTable = $schema->createTable('sharing_source_node_target');
+		$sourceNodeTargetTable->addColumn('user_id', Types::TEXT, ['length' => 64]);
+		$sourceNodeTargetTable->addColumn('source_instance', Types::STRING, ['length' => 128, 'notnull' => false]);


this and any code using it will need to be adjusted once #65512 is in

icewind1991 · 2026-10-27T16:47:20Z

+
+	#[Override]
+	public function handle(Event $event): void {
+		foreach ($event->shareIds as $shareId) {


can we send the share objects with the event to prevent having to re-fetch them here.

icewind1991 · 2026-10-27T16:56:07Z

+		$ownerUserFolder = $this->rootFolder->getUserFolder($owner->userId);
+		$node = $ownerUserFolder->getFirstNodeById($sourceNodeId);


Since this function is called in a loop for the same share, the caller should get this once and pass it

icewind1991 · 2026-10-27T16:58:21Z

+				foreach (array_keys($userIds) as $userId) {
+					try {
+						if ($this->sourceNodeTargetManager->getTarget($userId, $share->owner, (int)$source->value) === null) {
+							$this->sourceNodeTargetManager->createDefaultTarget($userId, $share->owner, (int)$source->value);
+						}
+					} catch (NotFoundException) {
+						// The same exception will happen for the other users, because the source node is the same, so we can just stop here.
+						break;
+					}
+				}


this should use getTargets, which both fetches all existing targets at once instead of one by one, and has built-in "create default" handling.

icewind1991 · 2026-10-27T17:13:27Z

+	/**
+	 * Get user ids for a recipient value.
+	 *
+	 * @return list<string>


I would prefer list<IUser>

icewind1991 · 2026-10-27T17:16:07Z

 		if (!$share->getNoExpirationDate() || $isEnforced) {
 			if ($expirationDate !== null) {
 				$expirationDate->setTimezone($this->dateTimeZone->getTimeZone());
-				$expirationDate->setTime(23, 59, 59);


is this behavior change intended?

icewind1991 · 2026-10-27T17:25:24Z

+
+		$legacyMappingTable = $schema->createTable('sharing_share_legacy_mapping');
+		$legacyMappingTable->addColumn('id', Types::BIGINT);
+		$legacyMappingTable->addColumn('legacy_provider', Types::STRING, ['length' => 128]);


Maybe use class names and the class-mapper so we can store this as ints?

icewind1991 · 2026-10-27T17:26:17Z

+		$legacyMappingTable->addColumn('recipient_secrets', Types::TEXT);
+		$legacyMappingTable->addColumn('recipient_value', Types::STRING, ['length' => 255]);
+		$legacyMappingTable->setPrimaryKey(['legacy_provider', 'legacy_id']);
+		$legacyMappingTable->addIndex(['id']);


is id + recipient_value unique? if so, we should probably have an index for it

icewind1991 · 2026-10-27T17:45:14Z

+	 * @experimental 35.0.0
+	 */
+	public function getCreatedAt(): DateTimeImmutable {
+		return $this->createdAt ??= Server::get(ISnowflakeDecoder::class)->decode($this->id)->getCreatedAt();


inject the decoder in the constructor instead?

icewind1991 · 2026-10-27T17:51:18Z

+	}
+
+	// TODO: Test
+	public function mapLegacyShares(?IUser $user, ?int $limit = null): void {


mapUnmappedLegacyShares to make it more clear that it doesn't update existing mappings

icewind1991 · 2026-10-27T17:55:03Z

+			$legacyMapping = $this->legacyMapper->createLegacyMapping(
+				$id,
+				$provider,
+				$row['id'],
+				new DateTimeImmutable(),
+				[$this->getSharingManager()->generateSecret()],
+				$this->getSharingManager()->generateSecret(),
+			);
+
+			$this->syncLegacySharesToShare($id, [$legacyMapping]);


these two should be in a transaction to prevent an error halfway from causing a desync

icewind1991 · 2026-10-27T18:14:43Z

+		if ($event instanceof SharesUpdatedEvent) {
+			foreach ($event->shareIds as $id) {
+				$this->wrapSync($id, function () use ($id): void {
+					$share = $this->getSharingManager()->getShare(new ShareAccessContext(overrideChecks: true), $id);


another place where having the share in the event would save work

icewind1991 · 2026-10-27T18:41:12Z

+					new ShareUser(
+						$shareInitiator['value'],
+						$shareInitiator['instance'],
+					),


minor nitpick: creating the ShareUser can be de-duplicated

icewind1991 · 2026-10-27T19:11:48Z

+				$this->ignoreNewLegacyShare = false;
+			} catch (Exception $exception) {
+				$this->ignoreNewLegacyShare = false;
+				throw $exception;
+			}


} finally { $this->ignoreNewLegacyShare = false; }

provokateurin added 22 commits October 27, 2026 14:35

refactor(files_sharing): Set expiration date time to 23:59:59 only in…

8d291d7

… the controller Signed-off-by: provokateurin <kate@provokateurin.de>

feat(IShare): Add getProviderId()

1d635ea

Signed-off-by: provokateurin <kate@provokateurin.de>

feat(Share20): Dispatch ShareUpdatedEvent and ShareRestoredEvent

6bd5479

Signed-off-by: provokateurin <kate@provokateurin.de>

feat(OCP\Share\Events): Add setters for the share property

8c623ef

Signed-off-by: provokateurin <kate@provokateurin.de>

feat(SharingManager): Dispatch SharesUpdatedEvent and SharesDeletedEvent

b47f7a8

Signed-off-by: provokateurin <kate@provokateurin.de>

feat(DefaultShareProvider): Allow fetching shares of TYPE_USERGROUP

41d9b26

Signed-off-by: provokateurin <kate@provokateurin.de>

feat(ITeamManager): Allow calling getMembersOfTeam() without a user

71e492a

Signed-off-by: provokateurin <kate@provokateurin.de>

feat(IShareRecipientType): Add getUsers()

21af94c

Signed-off-by: provokateurin <kate@provokateurin.de>

feat(Sharing): Add node source target management

7968c10

Signed-off-by: provokateurin <kate@provokateurin.de>

feat(APasswordSharePropertyType): Only hash the new value if it's not…

91c5008

… a hash already Signed-off-by: provokateurin <kate@provokateurin.de>

refactor(ISharePropertyTypeModifyValue): Modify values on format inst…

839e375

…ead of load Signed-off-by: provokateurin <kate@provokateurin.de>

feat(Share): Add getCreatedAt() method

1f70c33

Signed-off-by: provokateurin <kate@provokateurin.de>

feat(ISharingBackend): Add getUserStatuses() method

417a326

Signed-off-by: provokateurin <kate@provokateurin.de>

feat(files): Add NodeNicknameSharePropertyType

5893022

Signed-off-by: provokateurin <kate@provokateurin.de>

fix(ShareByMailProvider): Set share status from DB

3ebaecf

Signed-off-by: provokateurin <kate@provokateurin.de>

fix(Share20): Fix restoring target after share was created

3b0344c

Signed-off-by: provokateurin <kate@provokateurin.de>

fix(FederatedShareProvider): Use existing share time and target if av…

78e001a

…ailable Signed-off-by: provokateurin <kate@provokateurin.de>

test(FederatedShareProvider): Don't send remote share notification in…

e024b2d

… unit tests Signed-off-by: provokateurin <kate@provokateurin.de>

refactor(Share): Allow checking effective enabled permissions for exp…

ca742e8

…licit recipients Signed-off-by: provokateurin <kate@provokateurin.de>

feat(Sharing): Implement bi-directional syncing with legacy sharing

84efc96

Signed-off-by: provokateurin <kate@provokateurin.de>

test(integration): Enable Unified Sharing legacy sync validation

6f9a5ca

Signed-off-by: provokateurin <kate@provokateurin.de>

chore(Sharing): Remove outdated TODOs

2279e86

provokateurin added this to the Nextcloud 36 milestone Oct 27, 2026

provokateurin requested review from icewind1991 and susnux October 27, 2026 15:35

provokateurin added enhancement 2. developing Work in progress labels Oct 27, 2026

provokateurin self-assigned this Oct 27, 2026

icewind1991 reviewed Oct 27, 2026

View reviewed changes

		$ownerUserFolder = $this->rootFolder->getUserFolder($owner->userId);
		$node = $ownerUserFolder->getFirstNodeById($sourceNodeId);

Conversation

provokateurin commented Oct 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants