From b25708a257466cfc272e98ca7553532b7a8060fc Mon Sep 17 00:00:00 2001
From: Robert Zondervan <robert@conduction.nl>
Date: Tue, 20 Oct 2026 10:16:20 +0200
Subject: [PATCH 1/6] =?UTF-8?q?fix(review):=20#1294=20f1=20=E2=80=94=20A?=
 =?UTF-8?q?=20group=20member's=20save=20leaves=20the=20owner=20no=20versio?=
 =?UTF-8?q?n=20to=20restore?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

https://github.com/ConductionNL/keepiq/pull/1294#discussion_r4322767959

ShareSyncService::writeCopyBlobs() now keeps the source's state before a
write by anyone but its owner as a version (SecretVersionService::snapshot,
actor `user:<writer>`), under the same rule SecretService applies to the
owner's own update: only when a value changes and the source held one.
SecretVersionService is an appended, nullable, DI-resolved constructor
parameter of ShareSyncService.

Only the source is snapshotted: secret-version-history keeps the canonical
history on the owner's copy and scopes a recipient's history to their own
copy, and the owner's own update snapshots no recipient copy either. The
caller's own copy is already snapshotted by SecretService::update().

Class sweep: the same snapshot covers every non-owner write of the source,
on share-save and on /sync: a group member, a team-folder writer and a
delegate. The owner's own /sync takes none.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 lib/Service/ShareSaveService.php              |  1 +
 lib/Service/ShareSyncService.php              | 63 ++++++++++++++++++-
 .../Controller/UseOnlyShareRefusalTest.php    |  2 +
 tests/Unit/Db/ShareSaveVersionLockTest.php    |  1 +
 .../Federation/ReadOnlyFederatedCopyTest.php  |  2 +
 .../GroupMemberWritesForEveryoneTest.php      | 27 ++++++++
 .../Service/ManagerWritesLikeAnEditorTest.php |  2 +
 tests/Unit/Service/ShareSaveServiceTest.php   | 42 ++++++++++++-
 tests/Unit/Service/ShareServiceTest.php       |  3 +
 .../Service/UserSharingIntegrationTest.php    |  2 +
 10 files changed, 141 insertions(+), 4 deletions(-)

diff --git a/lib/Service/ShareSaveService.php b/lib/Service/ShareSaveService.php
index be8176c9d..71ebb8cd3 100644
--- a/lib/Service/ShareSaveService.php
+++ b/lib/Service/ShareSaveService.php
@@ -295,6 +295,7 @@ private function write(
 			updates: $entries->copies,
 			allowedIds: array_fill_keys(array_keys($required['copies']), true),
 			savedAt: $savedAt,
+			writerId: $userId,
 		);
 		$federatedRows = $this->writePartners(
 			path: $path,
diff --git a/lib/Service/ShareSyncService.php b/lib/Service/ShareSyncService.php
index 26cca1923..31f8391a1 100644
--- a/lib/Service/ShareSyncService.php
+++ b/lib/Service/ShareSyncService.php
@@ -78,6 +78,7 @@ class ShareSyncService {
 	 * @param IDBConnection $db The DB connection (sync transaction)
 	 * @param ShareAuthorizationService $auth The share authorization service
 	 * @param ShareAuditTrail|null $auditTrail The share audit trail
+	 * @param SecretVersionService|null $versionService The version history (snapshot of a source written by anyone but its owner)
 	 *
 	 * @return void
 	 *
@@ -90,6 +91,7 @@ public function __construct(
 		private IDBConnection $db,
 		private ShareAuthorizationService $auth,
 		?ShareAuditTrail $auditTrail = null,
+		private ?SecretVersionService $versionService = null,
 	) {
 		$this->auditTrail = ($auditTrail ?? new ShareAuditTrail());
 	}//end __construct()
@@ -149,7 +151,13 @@ public function syncUpdate(
 		return $this->atomic(
 			fn: function () use ($source, $updates, $allowedIds, $base, $savedAt, $writesOn, $userId): int {
 				$this->lockSource(source: $source, base: $base, savedAt: $savedAt);
-				$updated = $this->writeCopyBlobs(source: $source, updates: $updates, allowedIds: $allowedIds, savedAt: $savedAt);
+				$updated = $this->writeCopyBlobs(
+					source: $source,
+					updates: $updates,
+					allowedIds: $allowedIds,
+					savedAt: $savedAt,
+					writerId: $userId
+				);
 				if ($updated > 0) {
 					$this->recordNonOwnerSync(source: $source, userId: $userId, writesOn: $writesOn);
 				}
@@ -380,19 +388,29 @@ private function collectSyncTargets(Secret $source): array {
 
 	/**
 	 * Write every permitted recipient blob. Opens no transaction: the caller
-	 * runs it inside its own, so it commits with the rest of the save.
+	 * runs it inside its own, so it commits with the rest of the save. When
+	 * anyone but the owner writes the source, its state before the write is
+	 * kept as a version, as the owner's own update keeps it.
 	 *
 	 * @param Secret $source The source secret being synced
 	 * @param array<int,array<string,string|null>> $updates The per-recipient blobs
 	 * @param array<string,bool> $allowedIds The secret ids this sync may write
 	 * @param DateTime $savedAt The version every written row gets
+	 * @param string|null $writerId The caller; null takes no snapshot
 	 *
 	 * @return int Number of recipient copies updated.
 	 *
 	 * @spec openspec/specs/user-sharing/spec.md#requirement-sync-on-update
 	 * @spec openspec/changes/sharing-atomic-share-save/specs/user-sharing/spec.md#requirement-a-shared-secret-and-every-copy-save-in-one-transaction
+	 * @spec openspec/specs/secret-version-history/spec.md#requirement-prior-encrypted-values-are-retained-on-update
 	 */
-	public function writeCopyBlobs(Secret $source, array $updates, array $allowedIds, DateTime $savedAt): int {
+	public function writeCopyBlobs(
+		Secret $source,
+		array $updates,
+		array $allowedIds,
+		DateTime $savedAt,
+		?string $writerId = null,
+	): int {
 		$updated = 0;
 		foreach ($updates as $update) {
 			$recipientSecretId = (string)($update['secretId'] ?? '');
@@ -406,7 +424,11 @@ public function writeCopyBlobs(Secret $source, array $updates, array $allowedIds
 				continue;
 			}
 
+			$preUpdate = clone $copy;
 			$this->applyRecipientBlob(copy: $copy, update: $update, savedAt: $savedAt);
+			if ($recipientSecretId === $source->getId()) {
+				$this->snapshotSource(source: $source, preUpdate: $preUpdate, after: $copy, writerId: $writerId);
+			}
 
 			// A key rewrite of the SOURCE row is a real rotation —
 			// advance keyUpdatedAt so rotation proofs stay honest
@@ -422,6 +444,41 @@ public function writeCopyBlobs(Secret $source, array $updates, array $allowedIds
 		return $updated;
 	}//end writeCopyBlobs()
 
+	/**
+	 * Keep the source's state before a write by anyone but its owner (a
+	 * delegate, a team-folder writer or a member of a group it is shared with)
+	 * as a version, with the writer as its actor: only when a value changes
+	 * and the source held a value, as SecretService does on the owner's own
+	 * update. The other copies are not kept: a recipient's history is its own
+	 * copy's, and the owner's update keeps none of them either.
+	 *
+	 * @param Secret $source The source secret being synced
+	 * @param Secret $preUpdate The source row as read under the lock
+	 * @param Secret $after The source row with the write applied
+	 * @param string|null $writerId The caller; null takes no snapshot
+	 *
+	 * @return void
+	 *
+	 * @spec openspec/specs/secret-version-history/spec.md#requirement-prior-encrypted-values-are-retained-on-update
+	 */
+	private function snapshotSource(Secret $source, Secret $preUpdate, Secret $after, ?string $writerId): void {
+		if ($this->versionService === null || $writerId === null
+			|| ($source->getOwnerType() === 'user' && $source->getOwnerId() === $writerId)
+		) {
+			return;
+		}
+
+		$hadValues = (string)$preUpdate->getKey() !== ''
+			|| (string)$preUpdate->getLogin() !== ''
+			|| (string)$preUpdate->getAdditionalFields() !== '';
+		$changed = $preUpdate->getKey() !== $after->getKey()
+			|| $preUpdate->getLogin() !== $after->getLogin()
+			|| $preUpdate->getAdditionalFields() !== $after->getAdditionalFields();
+		if ($hadValues === true && $changed === true) {
+			$this->versionService->snapshot(preUpdate: $preUpdate, actorType: 'user', actorId: $writerId);
+		}
+	}//end snapshotSource()
+
 	/**
 	 * Copy one re-encrypted blob onto a recipient's Secret row.
 	 *
diff --git a/tests/Unit/Controller/UseOnlyShareRefusalTest.php b/tests/Unit/Controller/UseOnlyShareRefusalTest.php
index 20b25ad00..85533dbb3 100644
--- a/tests/Unit/Controller/UseOnlyShareRefusalTest.php
+++ b/tests/Unit/Controller/UseOnlyShareRefusalTest.php
@@ -23,6 +23,7 @@
 use OCA\Keepiq\Service\ShareRestrictionResolver;
 use OCA\Keepiq\Service\ShareRevocationService;
 use OCA\Keepiq\Service\ShareService;
+use OCA\Keepiq\Service\SecretVersionService;
 use OCA\Keepiq\Service\ShareSyncService;
 use OCA\Keepiq\Service\TeamFolderQueryService;
 use OCA\Keepiq\Service\UseOnlyUseRecorder;
@@ -152,6 +153,7 @@ private function shareController(string $uid): ShareController {
 				suiteMapper: $this->suites,
 				db: $db,
 				auth: $auth,
+				versionService: $this->createMock(SecretVersionService::class),
 			),
 			revocationService: new ShareRevocationService(
 				mapper: $this->targets,
diff --git a/tests/Unit/Db/ShareSaveVersionLockTest.php b/tests/Unit/Db/ShareSaveVersionLockTest.php
index 5360023c4..20e44a92d 100644
--- a/tests/Unit/Db/ShareSaveVersionLockTest.php
+++ b/tests/Unit/Db/ShareSaveVersionLockTest.php
@@ -138,6 +138,7 @@ function () use ($mapper, $source): bool {
 			suiteMapper: new \OCA\Keepiq\Db\EncryptionSuiteMapper(db: $this->db),
 			db: $this->db,
 			auth: $auth,
+			versionService: $this->createMock(\OCA\Keepiq\Service\SecretVersionService::class),
 		);
 		$expected = $mapper->findById($source->getId())->getUpdatedAt()?->format(DateTime::ATOM);
 
diff --git a/tests/Unit/Federation/ReadOnlyFederatedCopyTest.php b/tests/Unit/Federation/ReadOnlyFederatedCopyTest.php
index 7512e6c34..785c7b3ff 100644
--- a/tests/Unit/Federation/ReadOnlyFederatedCopyTest.php
+++ b/tests/Unit/Federation/ReadOnlyFederatedCopyTest.php
@@ -50,6 +50,7 @@
 use OCA\Keepiq\Service\ShareRestrictionResolver;
 use OCA\Keepiq\Service\ShareRevocationService;
 use OCA\Keepiq\Service\ShareService;
+use OCA\Keepiq\Service\SecretVersionService;
 use OCA\Keepiq\Service\ShareSyncService;
 use OCA\Keepiq\Service\TeamFolderQueryService;
 use OCA\Keepiq\Service\WriteLockService;
@@ -170,6 +171,7 @@ private function shareController(): ShareController {
 				suiteMapper: $this->suites,
 				db: $db,
 				auth: $auth,
+				versionService: $this->createMock(SecretVersionService::class),
 			),
 			revocationService: new ShareRevocationService(
 				mapper: $this->targets,
diff --git a/tests/Unit/Service/GroupMemberWritesForEveryoneTest.php b/tests/Unit/Service/GroupMemberWritesForEveryoneTest.php
index f9d967c00..c16ea36e8 100644
--- a/tests/Unit/Service/GroupMemberWritesForEveryoneTest.php
+++ b/tests/Unit/Service/GroupMemberWritesForEveryoneTest.php
@@ -11,12 +11,14 @@
 use OCA\Keepiq\Db\GroupShare;
 use OCA\Keepiq\Db\GroupShareMapper;
 use OCA\Keepiq\Db\Secret;
+use OCA\Keepiq\Db\SecretVersion;
 use OCA\Keepiq\Db\SecretDelegationMapper;
 use OCA\Keepiq\Db\SecretMapper;
 use OCA\Keepiq\Db\ShareTarget;
 use OCA\Keepiq\Db\ShareTargetMapper;
 use OCA\Keepiq\Service\ShareAuditTrail;
 use OCA\Keepiq\Service\ShareAuthorizationService;
+use OCA\Keepiq\Service\SecretVersionService;
 use OCA\Keepiq\Service\ShareSyncService;
 use OCA\Keepiq\Service\TeamFolderService;
 use OCP\AppFramework\Db\DoesNotExistException;
@@ -52,8 +54,11 @@ class GroupMemberWritesForEveryoneTest extends TestCase {
 
 	private ShareAuditTrail&MockObject $audit;
 
+	private SecretVersionService&MockObject $versions;
+
 	protected function setUp(): void {
 		$this->audit = $this->createMock(ShareAuditTrail::class);
+		$this->versions = $this->createMock(SecretVersionService::class);
 		$this->addRow(id: 'src', owner: 'alice');
 		$this->addRow(id: 'c-bob', owner: 'bob');
 		$this->addRow(id: 'c-dan', owner: 'dan');
@@ -154,6 +159,7 @@ private function sync(): ShareSyncService {
 				groupManager: $groups,
 			),
 			auditTrail: $this->audit,
+			versionService: $this->versions,
 		);
 	}
 
@@ -195,6 +201,27 @@ public function testAGroupMemberWritesTheSourceAndEveryCopyAndIsAuditedAsAGroupW
 		$this->assertSame(3, $this->sync()->syncUpdate('src', $this->everyRow(), '', 'bob'));
 	}
 
+	public function testAGroupMembersSyncKeepsTheSourcesPreviousValueAsAVersion(): void {
+		$this->rows['src']->setKey('OLD');
+		$this->versions->expects($this->once())->method('snapshot')
+			->with(
+				$this->callback(static fn (Secret $preUpdate): bool => $preUpdate->getId() === 'src' && $preUpdate->getKey() === 'OLD'),
+				'user',
+				'bob'
+			)
+			->willReturn(new SecretVersion());
+
+		$this->sync()->syncUpdate('src', $this->everyRow(), '', 'bob');
+	}
+
+	public function testTheOwnersSyncTakesNoSnapshotOfTheSource(): void {
+		// The owner's own update through PUT /secrets/{id} keeps her version.
+		$this->rows['src']->setKey('OLD');
+		$this->versions->expects($this->never())->method('snapshot');
+
+		$this->sync()->syncUpdate('src', $this->everyRow(), '', 'alice');
+	}
+
 	public function testAMemberWhoLeftTheGroupKeepsTheirCopyButNoWriteGrade(): void {
 		// The leave cleanup failed: Bob's copy and its share row are still there.
 		$this->groups['admin'] = ['mallory'];
diff --git a/tests/Unit/Service/ManagerWritesLikeAnEditorTest.php b/tests/Unit/Service/ManagerWritesLikeAnEditorTest.php
index ac3a18632..53cc32ed1 100644
--- a/tests/Unit/Service/ManagerWritesLikeAnEditorTest.php
+++ b/tests/Unit/Service/ManagerWritesLikeAnEditorTest.php
@@ -12,6 +12,7 @@
 use OCA\Keepiq\Db\SecretMapper;
 use OCA\Keepiq\Db\ShareTargetMapper;
 use OCA\Keepiq\Service\ShareAuthorizationService;
+use OCA\Keepiq\Service\SecretVersionService;
 use OCA\Keepiq\Service\ShareSyncService;
 use OCA\Keepiq\Service\TeamFolderService;
 use OCP\AppFramework\Db\DoesNotExistException;
@@ -56,6 +57,7 @@ private function sync(string $grade): ShareSyncService {
 				suiteMapper: $this->createMock(EncryptionSuiteMapper::class),
 				teamFolderService: $teamFolders,
 			),
+			versionService: $this->createMock(SecretVersionService::class),
 		);
 	}
 
diff --git a/tests/Unit/Service/ShareSaveServiceTest.php b/tests/Unit/Service/ShareSaveServiceTest.php
index bba0766dd..c53162a7a 100644
--- a/tests/Unit/Service/ShareSaveServiceTest.php
+++ b/tests/Unit/Service/ShareSaveServiceTest.php
@@ -287,6 +287,7 @@ private function service(?ShareAuthorizationService $auth = null): ShareSaveServ
 			suiteMapper: $this->suites,
 			db: $this->db,
 			auth: ($auth ?? $this->auth),
+			versionService: $this->versionHistory,
 		);
 
 		return new ShareSaveService(
@@ -850,18 +851,57 @@ public function testACopyEditorsAuditNamesTheOwnerWhenTheSourceIsLeftOut(): void
 		$this->assertSame(['written' => 1, 'skipped' => 1], $result['copies']);
 	}
 
+	/**
+	 * Log the snapshots the copy fan-out takes, as `snapshot <id> <key> <actor>`.
+	 *
+	 * @return void
+	 */
+	private function logSnapshots(): void {
+		$this->versionHistory->method('snapshot')->willReturnCallback(
+			function (Secret $preUpdate, string $actorType, string $actorId): SecretVersion {
+				$this->log[] = 'snapshot '.$preUpdate->getId().' '.$preUpdate->getKey().' '.$actorType.':'.$actorId;
+				return new SecretVersion();
+			}
+		);
+	}
+
 	public function testAGroupMemberWritesTheSourceAndTheOtherCopiesAndIsAuditedAsAGroupWrite(): void {
 		$auth = $this->groupAuth(members: ['bob']);
+		$this->logSnapshots();
 		$this->audit->expects($this->once())->method('recordTeamWriteSync')->with('bob', 'src', 'Router', 'group-write sync');
 		$this->federated->expects($this->never())->method('writeCiphertext');
 
 		$result = $this->service($auth)->save(id: 'c-bob', request: $this->bobRequest(), userId: 'bob');
 
-		$this->assertSame(['begin', 'lock src', 'lock c-bob', 'copy src', 'copy c-carol', 'row c-bob', 'commit'], $this->log);
+		// Alice's value before Bob's save is kept as a version of her source, Bob as its actor.
+		$this->assertSame(
+			['begin', 'lock src', 'lock c-bob', 'snapshot src OLD-src user:bob', 'copy src', 'copy c-carol', 'row c-bob', 'commit'],
+			$this->log
+		);
 		$this->assertSame(['written' => 2, 'skipped' => 0], $result['copies']);
 		$this->assertTrue($result['federated']['ownerOnly']);
 	}
 
+	public function testAWriteGradeMembersSaveKeepsTheSourcesPreviousValueAsAVersion(): void {
+		$this->bobWritesOnAGrade();
+		$this->logSnapshots();
+
+		$this->service()->save(id: 'c-bob', request: $this->bobRequest(), userId: 'bob');
+
+		$this->assertSame(
+			['begin', 'lock src', 'lock c-bob', 'snapshot src OLD-src user:bob', 'copy src', 'copy c-carol', 'row c-bob', 'commit'],
+			$this->log
+		);
+	}
+
+	public function testTheOwnersSaveTakesNoSnapshotInTheCopyFanOut(): void {
+		// The owner's row is snapshotted by SecretService::update(); the copies keep no version.
+		$this->delivery->method('deliver')->willReturn(true);
+		$this->versionHistory->expects($this->never())->method('snapshot');
+
+		$this->service()->save(id: 'src', request: $this->ownerRequest(), userId: 'alice');
+	}
+
 	public function testAMemberWhoLeftTheGroupIsRefusedThoughTheirCopyRemains(): void {
 		// The leave cleanup failed: Bob's copy and its share row are still there.
 		$auth = $this->groupAuth(members: []);
diff --git a/tests/Unit/Service/ShareServiceTest.php b/tests/Unit/Service/ShareServiceTest.php
index de435b040..535ce9651 100644
--- a/tests/Unit/Service/ShareServiceTest.php
+++ b/tests/Unit/Service/ShareServiceTest.php
@@ -38,6 +38,7 @@
 use OCA\Keepiq\Service\ShareAuthorizationService;
 use OCA\Keepiq\Service\ShareRevocationService;
 use OCA\Keepiq\Service\ShareService;
+use OCA\Keepiq\Service\SecretVersionService;
 use OCA\Keepiq\Service\ShareSyncService;
 use OCA\Keepiq\Service\TeamFolderService;
 use OCP\AppFramework\Db\DoesNotExistException;
@@ -176,6 +177,7 @@ private function wireService(
 				db: $this->db,
 				auth: $auth,
 				auditTrail: $auditTrail,
+				versionService: $this->createMock(SecretVersionService::class),
 			),
 			revocationService: new ShareRevocationService(
 				mapper: $this->mapper,
@@ -686,6 +688,7 @@ public function testWriteCopyBlobsOpensNoTransactionAndStampsTheSaveVersion(): v
 			suiteMapper: $this->suiteMapper,
 			db: $this->db,
 			auth: $this->createMock(ShareAuthorizationService::class),
+			versionService: $this->createMock(SecretVersionService::class),
 		);
 		$at = new DateTime('2026-10-13T12:00:00+00:00');
 
diff --git a/tests/Unit/Service/UserSharingIntegrationTest.php b/tests/Unit/Service/UserSharingIntegrationTest.php
index fe0a4fac8..a25b48530 100644
--- a/tests/Unit/Service/UserSharingIntegrationTest.php
+++ b/tests/Unit/Service/UserSharingIntegrationTest.php
@@ -62,6 +62,7 @@
 use OCA\Keepiq\Service\ShareAuthorizationService;
 use OCA\Keepiq\Service\ShareRevocationService;
 use OCA\Keepiq\Service\ShareService;
+use OCA\Keepiq\Service\SecretVersionService;
 use OCA\Keepiq\Service\ShareSyncService;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\IDBConnection;
@@ -173,6 +174,7 @@ private function buildShareService(): array {
 				suiteMapper: $suiteMapper,
 				db: $db,
 				auth: $auth,
+				versionService: $this->createMock(SecretVersionService::class),
 			),
 			revocationService: new ShareRevocationService(
 				mapper: $shareTargetMapper,

From aceeb9d00413bdd7635201149516a73f6b10fa34 Mon Sep 17 00:00:00 2001
From: Robert Zondervan <robert@conduction.nl>
Date: Tue, 20 Oct 2026 10:28:00 +0200
Subject: [PATCH 2/6] =?UTF-8?q?fix(review):=20#1294=20f2=20=E2=80=94=20/sy?=
 =?UTF-8?q?nc=20opened=20to=20group=20members=20without=20share-save's=20g?=
 =?UTF-8?q?uards?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

https://github.com/ConductionNL/keepiq/pull/1294#discussion_r4322767967

A member of a group the secret is shared with (GROUP_WRITE) now gets
share-save's three guards on PUT /secrets/{id}/sync:
- expectedUpdatedAt '' is refused, so no blind overwrite;
- the writer is checked again inside atomic(), after the source lock, so a
  leave or a group-share revoke that committed meanwhile refuses the sync;
- the updates must name the source and every copy whose access has not
  ended (an ended or gone copy may be named and is not written), as
  ShareSaveScope::assertComplete() requires; ShareSaveScope depends on
  ShareSyncService, so the check is mirrored, not called.

The owner's, a delegate's and a team-folder writer's /sync are unchanged.
GroupMemberWritesForEveryoneTest's successful group-member syncs now pass
a real base.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 lib/Service/ShareSyncService.php              | 70 ++++++++++++++-
 .../GroupMemberWritesForEveryoneTest.php      | 90 ++++++++++++++++++-
 2 files changed, 155 insertions(+), 5 deletions(-)

diff --git a/lib/Service/ShareSyncService.php b/lib/Service/ShareSyncService.php
index 31f8391a1..fbc1d7df2 100644
--- a/lib/Service/ShareSyncService.php
+++ b/lib/Service/ShareSyncService.php
@@ -106,7 +106,8 @@ public function __construct(
 	 * `updatedAt`, moves that version and writes every recipient copy in a
 	 * single transaction, and clears `possiblyCompromisedAt` from any copy
 	 * where it was set. A sync by anyone but the owner is audited on the
-	 * source in that transaction.
+	 * source in that transaction. A member of a group the secret is shared
+	 * with gets share-save's guards (assertGroupWriteGuards()).
 	 *
 	 * @param string $secretId The source secret ID
 	 * @param array<int,array<string,string|null>> $updates The per-recipient blobs; each row has
@@ -139,6 +140,11 @@ public function syncUpdate(
 		}
 
 		$writesOn = $this->resolveSyncWriter(source: $source, userId: $userId);
+		if ($writesOn === self::GROUP_WRITE && $expectedUpdatedAt === '') {
+			// A member never overwrites a value they have not seen.
+			throw new InvalidArgumentException(message: 'expectedUpdatedAt is required when saving as a group member');
+		}
+
 		$this->assertSyncSourceUnchanged(source: $source, expectedUpdatedAt: $expectedUpdatedAt);
 
 		// The source is locked against the version just read, which is the
@@ -151,6 +157,10 @@ public function syncUpdate(
 		return $this->atomic(
 			fn: function () use ($source, $updates, $allowedIds, $base, $savedAt, $writesOn, $userId): int {
 				$this->lockSource(source: $source, base: $base, savedAt: $savedAt);
+				if ($writesOn === self::GROUP_WRITE) {
+					[$writesOn, $allowedIds] = $this->assertGroupWriteGuards(source: $source, updates: $updates, userId: $userId);
+				}
+
 				$updated = $this->writeCopyBlobs(
 					source: $source,
 					updates: $updates,
@@ -168,6 +178,64 @@ public function syncUpdate(
 		);
 	}//end syncUpdate()
 
+	/**
+	 * Share-save's guards for a member of a group the secret is shared with,
+	 * run under the source lock: the caller is checked again, so a leave or a
+	 * revoke that committed after the first check refuses the sync, and the
+	 * updates must name exactly the source and every copy whose access has
+	 * not ended, so no holder is left on the old value. Only those rows are
+	 * written.
+	 *
+	 * @param Secret $source The source secret being synced
+	 * @param array<int,array<string,string|null>> $updates The per-recipient blobs
+	 * @param string $userId The requesting user
+	 *
+	 * @return array{0:string|null,1:array<string,bool>} How the audit names the write now, and the ids to write
+	 *
+	 * @throws InvalidArgumentException When the caller lost the right to write, or a row is missing or unexpected
+	 *
+	 * @spec openspec/specs/sharing-group/spec.md#requirement-a-group-member-saves-for-everyone-it-is-shared-with
+	 * @spec openspec/changes/sharing-atomic-share-save/specs/user-sharing/spec.md#requirement-a-share-save-names-every-copy-or-is-refused
+	 */
+	private function assertGroupWriteGuards(Secret $source, array $updates, string $userId): array {
+		$writesOn = $this->resolveSyncWriter(source: $this->auth->loadSecret(secretId: $source->getId()), userId: $userId);
+
+		$required = [$source->getId() => true];
+		$ignored = [];
+		$targets = $this->mapper->findBySourceSecret($source->getId());
+		$rows = $this->secretMapper->findByIds(ids: array_map(static fn ($target): string => $target->getSecretId(), $targets));
+		$now = time();
+		foreach ($targets as $target) {
+			// A copy whose access ended, or whose row is gone, may be named but is not written.
+			$copyId = $target->getSecretId();
+			if (isset($rows[$copyId]) === false) {
+				$ignored[$copyId] = true;
+				continue;
+			}
+
+			$ends = $rows[$copyId]->getAccessExpiresAt();
+			if ($ends !== null && $ends->getTimestamp() <= $now) {
+				$ignored[$copyId] = true;
+				continue;
+			}
+
+			$required[$copyId] = true;
+		}
+
+		$named = [];
+		foreach ($updates as $update) {
+			$named[(string)($update['secretId'] ?? '')] = true;
+		}
+
+		if (array_diff_key($required, $named) !== [] || array_diff_key($named, $required, $ignored) !== []) {
+			throw new InvalidArgumentException(
+				message: 'Name the source and every copy whose access has not ended, and nothing else'
+			);
+		}
+
+		return [$writesOn, $required];
+	}//end assertGroupWriteGuards()
+
 	/**
 	 * Attribute a sync by anyone but the owner to the writer on the source
 	 * (§3.3), as a share-save through a copy is: a team writer as
diff --git a/tests/Unit/Service/GroupMemberWritesForEveryoneTest.php b/tests/Unit/Service/GroupMemberWritesForEveryoneTest.php
index c16ea36e8..91c90620a 100644
--- a/tests/Unit/Service/GroupMemberWritesForEveryoneTest.php
+++ b/tests/Unit/Service/GroupMemberWritesForEveryoneTest.php
@@ -36,6 +36,8 @@
  * @spec openspec/specs/sharing-group/spec.md#requirement-a-group-member-saves-for-everyone-it-is-shared-with
  */
 class GroupMemberWritesForEveryoneTest extends TestCase {
+	/** The version of `src` a sync is made from */
+	private const BASE = '2026-10-01T10:00:00+00:00';
 
 	/** @var array<string,Secret> */
 	private array $rows = [];
@@ -56,10 +58,17 @@ class GroupMemberWritesForEveryoneTest extends TestCase {
 
 	private SecretVersionService&MockObject $versions;
 
+	/** @var array<string,list<string>>|null The members of each group once the sync holds the source lock */
+	private ?array $groupsUnderLock = null;
+
+	/** @var array<int,string> The rows the sync wrote, in order */
+	private array $written = [];
+
 	protected function setUp(): void {
 		$this->audit = $this->createMock(ShareAuditTrail::class);
 		$this->versions = $this->createMock(SecretVersionService::class);
 		$this->addRow(id: 'src', owner: 'alice');
+		$this->rows['src']->setUpdatedAt(new DateTime(self::BASE));
 		$this->addRow(id: 'c-bob', owner: 'bob');
 		$this->addRow(id: 'c-dan', owner: 'dan');
 		$this->targets = [
@@ -106,7 +115,25 @@ private function sync(): ShareSyncService {
 		$secrets->method('findById')->willReturnCallback(
 			fn (string $id): Secret => $this->rows[$id] ?? throw new DoesNotExistException('none')
 		);
-		$secrets->method('update')->willReturnArgument(0);
+		$secrets->method('findByIds')->willReturnCallback(
+			fn (array $ids): array => array_intersect_key($this->rows, array_flip($ids))
+		);
+		$secrets->method('advanceVersion')->willReturnCallback(
+			function (): int {
+				// A leave that commits after the first check, before the lock.
+				if ($this->groupsUnderLock !== null) {
+					$this->groups = $this->groupsUnderLock;
+				}
+
+				return 1;
+			}
+		);
+		$secrets->method('update')->willReturnCallback(
+			function (Secret $row): Secret {
+				$this->written[] = $row->getId();
+				return $row;
+			}
+		);
 
 		$targets = $this->createMock(ShareTargetMapper::class);
 		$targets->method('findBySourceSecret')->willReturnCallback(fn (): array => $this->targets);
@@ -198,7 +225,7 @@ public function testAGroupMemberWritesTheSourceAndEveryCopyAndIsAuditedAsAGroupW
 		$this->audit->expects($this->once())->method('recordTeamWriteSync')
 			->with('bob', 'src', 'Router', 'group-write sync');
 
-		$this->assertSame(3, $this->sync()->syncUpdate('src', $this->everyRow(), '', 'bob'));
+		$this->assertSame(3, $this->sync()->syncUpdate('src', $this->everyRow(), self::BASE, 'bob'));
 	}
 
 	public function testAGroupMembersSyncKeepsTheSourcesPreviousValueAsAVersion(): void {
@@ -211,7 +238,7 @@ public function testAGroupMembersSyncKeepsTheSourcesPreviousValueAsAVersion(): v
 			)
 			->willReturn(new SecretVersion());
 
-		$this->sync()->syncUpdate('src', $this->everyRow(), '', 'bob');
+		$this->sync()->syncUpdate('src', $this->everyRow(), self::BASE, 'bob');
 	}
 
 	public function testTheOwnersSyncTakesNoSnapshotOfTheSource(): void {
@@ -222,6 +249,61 @@ public function testTheOwnersSyncTakesNoSnapshotOfTheSource(): void {
 		$this->sync()->syncUpdate('src', $this->everyRow(), '', 'alice');
 	}
 
+	public function testAGroupMembersSyncWithoutABaseIsRefused(): void {
+		try {
+			$this->sync()->syncUpdate('src', $this->everyRow(), '', 'bob');
+			$this->fail('A group member overwrote a value they had not seen');
+		} catch (InvalidArgumentException $e) {
+			$this->assertStringContainsString('expectedUpdatedAt is required', $e->getMessage());
+		}
+
+		$this->assertSame([], $this->written);
+	}
+
+	public function testAGroupMembersSyncLeavingOutACopyIsRefused(): void {
+		// Dan's copy is left on the old value.
+		$updates = [['secretId' => 'src', 'key' => 'NEW'], ['secretId' => 'c-bob', 'key' => 'NEW']];
+
+		try {
+			$this->sync()->syncUpdate('src', $updates, self::BASE, 'bob');
+			$this->fail('A group member changed the source and left a copy behind');
+		} catch (InvalidArgumentException $e) {
+			$this->assertStringContainsString('every copy', $e->getMessage());
+		}
+
+		$this->assertSame([], $this->written);
+	}
+
+	public function testAGroupMembersSyncNamingARowThatIsNotACopyIsRefused(): void {
+		$this->expectException(InvalidArgumentException::class);
+		$this->sync()->syncUpdate('src', [...$this->everyRow(), ['secretId' => 'other', 'key' => 'NEW']], self::BASE, 'bob');
+	}
+
+	public function testAGroupMembersSyncSkipsACopyWhoseAccessEnded(): void {
+		$this->rows['c-dan']->setAccessExpiresAt(new DateTime('-1 minute'));
+
+		$this->assertSame(2, $this->sync()->syncUpdate('src', $this->everyRow(), self::BASE, 'bob'));
+		$this->assertSame(['src', 'c-bob'], $this->written);
+	}
+
+	public function testAMemberWhoLeftTheGroupBeforeTheSyncsLockIsRefused(): void {
+		$this->groupsUnderLock = ['admin' => ['mallory']];
+		$this->audit->expects($this->never())->method('recordTeamWriteSync');
+
+		try {
+			$this->sync()->syncUpdate('src', $this->everyRow(), self::BASE, 'bob');
+			$this->fail('A member who left the group wrote every copy');
+		} catch (InvalidArgumentException $e) {
+			$this->assertSame('Not authorized to manage shares of this secret', $e->getMessage());
+		}
+
+		$this->assertSame([], $this->written);
+	}
+
+	public function testTheOwnersSyncKeepsWorkingWithoutABase(): void {
+		$this->assertSame(1, $this->sync()->syncUpdate('src', [['secretId' => 'src', 'key' => 'NEW']], '', 'alice'));
+	}
+
 	public function testAMemberWhoLeftTheGroupKeepsTheirCopyButNoWriteGrade(): void {
 		// The leave cleanup failed: Bob's copy and its share row are still there.
 		$this->groups['admin'] = ['mallory'];
@@ -249,7 +331,7 @@ public function testAMemberWhoseCopyCameThroughADirectShareGetsTheWriteGrade():
 			->with('dan', 'src', 'Router', 'group-write sync');
 
 		$this->assertSame('write', $this->sync()->writeContext('c-dan', 'dan')['effectiveGrade']);
-		$this->assertSame(3, $this->sync()->syncUpdate('src', $this->everyRow(), '', 'dan'));
+		$this->assertSame(3, $this->sync()->syncUpdate('src', $this->everyRow(), self::BASE, 'dan'));
 	}
 
 	public function testAMemberWhoLeftOneGroupButIsStillInAnotherGetsTheWriteGrade(): void {

From d0ee2e51be11254d20d83f73f2b56cccfabbd17e Mon Sep 17 00:00:00 2001
From: Robert Zondervan <robert@conduction.nl>
Date: Tue, 20 Oct 2026 10:28:25 +0200
Subject: [PATCH 3/6] =?UTF-8?q?docs(review):=20#1294=20f4=20=E2=80=94=20us?=
 =?UTF-8?q?er-sharing=20spec=20still=20says=20either=20party's=20update=20?=
 =?UTF-8?q?propagates?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

https://github.com/ConductionNL/keepiq/pull/1294#discussion_r4322768008

user-sharing/spec.md: the Sync on Update requirement (L194) takes the
review's suggestion and names delegates; the Purpose (L12) and the
acceptance criterion (L440) say the same instead of "either party".

Class sweep: no other live copy of the sentence in docs/, README or
openspec/changes/ (only archived changes, which stay as recorded).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 openspec/specs/user-sharing/spec.md | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/openspec/specs/user-sharing/spec.md b/openspec/specs/user-sharing/spec.md
index fb45a3895..74126331b 100644
--- a/openspec/specs/user-sharing/spec.md
+++ b/openspec/specs/user-sharing/spec.md
@@ -9,7 +9,7 @@
 
 @e2e exclude No user-sharing UI is built in v0.1; all sharing scenarios require the encrypted secret CRUD surface (itself unbuilt) — covered by integration tests, not Playwright UI flows.
 
-A user can share a secret with another Nextcloud user or with a Nextcloud user group. Because encryption is asymmetric, sharing works by creating an encrypted copy of the secret using the recipient's public certificate. Both the original and all shared copies stay in sync: when either party updates the secret, the change is written back to all copies.
+A user can share a secret with another Nextcloud user or with a Nextcloud user group. Because encryption is asymmetric, sharing works by creating an encrypted copy of the secret using the recipient's public certificate. Both the original and all shared copies stay in sync: when the owner, a delegate or a recipient who holds a write grade on it updates the secret, the change is written back to all copies.
 
 Group sharing expands statically to individual shares at share time. When new members join a group, the owner is notified and asked to approve adding them. When members leave a group, their group-derived shares are automatically revoked. Direct shares are always independent of group membership.
 
@@ -191,7 +191,7 @@ The marks MUST come from an endpoint that takes the search term and the user ids
 - WHEN it reaches the endpoint
 - THEN the system MUST refuse it with 400
 ### Requirement: Sync on Update
-When the owner, or a recipient who holds a write grade on it, updates a shared secret, the change MUST be propagated to all copies.
+When the owner, a delegate, or a recipient who holds a write grade on it updates a shared secret, the change MUST be propagated to all copies.
 
 #### Scenario: Owner updates shared secret
 - GIVEN a secret is shared with one or more users
@@ -437,7 +437,7 @@ The system MUST allow the original owner to revoke a share, removing the recipie
 - [ ] The recipient receives an encrypted copy in their own vault
 - [ ] The copy is encrypted with the recipient's public certificate — the sender cannot read it after sharing
 - [ ] The recipient receives a Nextcloud notification when a secret is shared with them
-- [ ] Updates by either party propagate to all copies of the shared secret
+- [ ] Updates by the owner, a delegate or a recipient who holds a write grade on it propagate to all copies of the shared secret
 - [ ] The original owner can revoke a share at any time
 - [ ] Revoking a share deletes the recipient's copy
 - [ ] Sharing fails with a clear error if the recipient has no EncryptionSuite

From 47a16bedcd55bffe65fa0be31277add94b587f7c Mon Sep 17 00:00:00 2001
From: Robert Zondervan <robert@conduction.nl>
Date: Tue, 20 Oct 2026 10:29:43 +0200
Subject: [PATCH 4/6] =?UTF-8?q?test(review):=20#1294=20f6=20=E2=80=94=20Tw?=
 =?UTF-8?q?o=20body=20claims=20without=20a=20test?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

https://github.com/ConductionNL/keepiq/pull/1294#discussion_r4322768086

- GroupMemberWritesForEveryoneTest: a member whose copy came through a
  team folder (with a `read` grade there) and who is in a group the secret
  is shared with gets `write` and saves for everyone.
- ShareSaveServiceTest: a member who left the group, or whose group share
  was revoked, between resolve() and the lock is refused under the lock
  and the save rolls back. groupAuth() answers isInGroup / findBySecret
  differently once the save's transaction began, as goneUnderLock does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 .../GroupMemberWritesForEveryoneTest.php      | 12 +++++
 tests/Unit/Service/ShareSaveServiceTest.php   | 49 ++++++++++++++++++-
 2 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/tests/Unit/Service/GroupMemberWritesForEveryoneTest.php b/tests/Unit/Service/GroupMemberWritesForEveryoneTest.php
index 91c90620a..2168910e1 100644
--- a/tests/Unit/Service/GroupMemberWritesForEveryoneTest.php
+++ b/tests/Unit/Service/GroupMemberWritesForEveryoneTest.php
@@ -334,6 +334,18 @@ public function testAMemberWhoseCopyCameThroughADirectShareGetsTheWriteGrade():
 		$this->assertSame(3, $this->sync()->syncUpdate('src', $this->everyRow(), self::BASE, 'dan'));
 	}
 
+	public function testAMemberWhoseCopyCameThroughATeamFolderGetsTheWriteGrade(): void {
+		// Dan's copy came with his `read` membership of a team folder; he is in `admin` too.
+		$this->targets[1]->setTeamFolderId('tf-1');
+		$this->teamGrades['dan'] = 'read';
+		$this->groups['admin'][] = 'dan';
+		$this->audit->expects($this->once())->method('recordTeamWriteSync')
+			->with('dan', 'src', 'Router', 'group-write sync');
+
+		$this->assertSame('write', $this->sync()->writeContext('c-dan', 'dan')['effectiveGrade']);
+		$this->assertSame(3, $this->sync()->syncUpdate('src', $this->everyRow(), self::BASE, 'dan'));
+	}
+
 	public function testAMemberWhoLeftOneGroupButIsStillInAnotherGetsTheWriteGrade(): void {
 		// Bob's copy is tied to the admin share; the ops share reaches him too.
 		$this->groupShares[] = $this->groupShare(id: 'gs-ops', secretId: 'src', groupId: 'ops');
diff --git a/tests/Unit/Service/ShareSaveServiceTest.php b/tests/Unit/Service/ShareSaveServiceTest.php
index c53162a7a..e42b09af9 100644
--- a/tests/Unit/Service/ShareSaveServiceTest.php
+++ b/tests/Unit/Service/ShareSaveServiceTest.php
@@ -97,6 +97,12 @@ class ShareSaveServiceTest extends TestCase {
 	/** The holder whose share row is gone when read again under the lock */
 	private ?string $goneUnderLock = null;
 
+	/** Whether the caller left the `admin` group before the lock */
+	private bool $leftGroupUnderLock = false;
+
+	/** Whether the share with the `admin` group was revoked before the lock */
+	private bool $groupShareRevokedUnderLock = false;
+
 	/** @var array<int,array{id:string,key:?string,updatedAt:?DateTime}> Rows written by the copy fan-out */
 	private array $copyWrites = [];
 
@@ -374,11 +380,14 @@ private function groupAuth(array $members): ShareAuthorizationService {
 		$groupShare->setGroupId('admin');
 		$groupShares = $this->createMock(GroupShareMapper::class);
 		$groupShares->method('findBySecret')->willReturnCallback(
-			static fn (string $secretId): array => ($secretId === 'src') ? [$groupShare] : []
+			fn (string $secretId): array => ($secretId === 'src' && $this->underLock($this->groupShareRevokedUnderLock) === false)
+				? [$groupShare]
+				: []
 		);
 		$groups = $this->createMock(IGroupManager::class);
 		$groups->method('isInGroup')->willReturnCallback(
-			static fn (string $userId, string $groupId): bool => $groupId === 'admin' && in_array($userId, $members, true)
+			fn (string $userId, string $groupId): bool => $groupId === 'admin' && in_array($userId, $members, true)
+				&& $this->underLock($this->leftGroupUnderLock) === false
 		);
 		$delegations = $this->createMock(SecretDelegationMapper::class);
 		$delegations->method('findActiveBySecretAndUser')->willThrowException(new DoesNotExistException('none'));
@@ -393,6 +402,18 @@ private function groupAuth(array $members): ShareAuthorizationService {
 		);
 	}
 
+	/**
+	 * Whether a change set to happen before the lock has happened: the save's
+	 * transaction began.
+	 *
+	 * @param bool $happens Whether the change happens at all
+	 *
+	 * @return bool
+	 */
+	private function underLock(bool $happens): bool {
+		return $happens === true && in_array('begin', $this->log, true) === true;
+	}
+
 	/**
 	 * Run a save that must be refused, and return the refusal.
 	 *
@@ -925,6 +946,30 @@ public function testAShareRowRemovedBeforeTheLockRefusesTheSave(): void {
 		$this->assertSame(['begin', 'lock src', 'lock c-bob', 'rollback'], $this->log);
 	}
 
+	public function testAMemberWhoLeftTheGroupBeforeTheLockIsRefused(): void {
+		// Bob was in `admin` at resolve(); his leave commits before the lock.
+		$auth = $this->groupAuth(members: ['bob']);
+		$this->leftGroupUnderLock = true;
+		$this->audit->expects($this->never())->method('recordTeamWriteSync');
+
+		$refusal = $this->refusal(fn () => $this->service($auth)->save(id: 'c-bob', request: $this->bobRequest(), userId: 'bob'));
+
+		$this->assertSame(['not_allowed', 403], [$refusal->getError(), $refusal->getStatus()]);
+		$this->assertSame(['begin', 'lock src', 'lock c-bob', 'rollback'], $this->log);
+	}
+
+	public function testAGroupShareRevokedBeforeTheLockRefusesTheSave(): void {
+		// Alice revokes the `admin` share after resolve() read it; Bob's copy is still there.
+		$auth = $this->groupAuth(members: ['bob']);
+		$this->groupShareRevokedUnderLock = true;
+		$this->audit->expects($this->never())->method('recordTeamWriteSync');
+
+		$refusal = $this->refusal(fn () => $this->service($auth)->save(id: 'c-bob', request: $this->bobRequest(), userId: 'bob'));
+
+		$this->assertSame(['not_allowed', 403], [$refusal->getError(), $refusal->getStatus()]);
+		$this->assertSame(['begin', 'lock src', 'lock c-bob', 'rollback'], $this->log);
+	}
+
 	public function testADirectRecipientIsRefused(): void {
 		// Carol's copy is a direct share, and she is in no group it is shared with.
 		$auth = $this->groupAuth(members: ['bob']);

From b6c149366981f88378e86cb44cdfc1e1201a5100 Mon Sep 17 00:00:00 2001
From: Robert Zondervan <robert@conduction.nl>
Date: Tue, 20 Oct 2026 10:54:02 +0200
Subject: [PATCH 5/6] =?UTF-8?q?fix(review):=20#1294=20f2=20=E2=80=94=20pre?=
 =?UTF-8?q?-push:=20refuse=20a=20source=20made=20read-only=20or=20use-only?=
 =?UTF-8?q?=20before=20the=20sync's=20lock?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

https://github.com/ConductionNL/keepiq/pull/1294#discussion_r4322767967

assertGroupWriteGuards() checked the writer again on the freshly loaded
source but not, as share-save's assertStillWriter() does through
assertWritable(), whether that source became a read-only or use-only copy
meanwhile: neither moves the version the lock checks. It now refuses both,
with the same refusals syncUpdate() gives before the lock
(ForbiddenException for read-only, 'A use-only copy cannot be changed').
assertWritable() is private to ShareSaveScope, which depends on
ShareSyncService, so the two checks are mirrored, not called.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 lib/Service/ShareSyncService.php              | 16 +++++++--
 .../GroupMemberWritesForEveryoneTest.php      | 33 +++++++++++++++++++
 2 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/lib/Service/ShareSyncService.php b/lib/Service/ShareSyncService.php
index fbc1d7df2..d3cffe2fb 100644
--- a/lib/Service/ShareSyncService.php
+++ b/lib/Service/ShareSyncService.php
@@ -180,8 +180,10 @@ public function syncUpdate(
 
 	/**
 	 * Share-save's guards for a member of a group the secret is shared with,
-	 * run under the source lock: the caller is checked again, so a leave or a
-	 * revoke that committed after the first check refuses the sync, and the
+	 * run under the source lock: the source is read again and refused when it
+	 * became a read-only or use-only copy, the caller is checked again, so a
+	 * leave or a revoke that committed after the first check refuses the sync,
+	 * and the
 	 * updates must name exactly the source and every copy whose access has
 	 * not ended, so no holder is left on the old value. Only those rows are
 	 * written.
@@ -198,7 +200,15 @@ public function syncUpdate(
 	 * @spec openspec/changes/sharing-atomic-share-save/specs/user-sharing/spec.md#requirement-a-share-save-names-every-copy-or-is-refused
 	 */
 	private function assertGroupWriteGuards(Secret $source, array $updates, string $userId): array {
-		$writesOn = $this->resolveSyncWriter(source: $this->auth->loadSecret(secretId: $source->getId()), userId: $userId);
+		// Read again, as share-save's assertStillWriter() does: becoming a
+		// read-only or use-only copy does not move the version the lock checks.
+		$current = $this->auth->loadSecret(secretId: $source->getId());
+		$current->assertNotReadOnly();
+		if ($current->getUseOnly() === true) {
+			throw new InvalidArgumentException(message: 'A use-only copy cannot be changed');
+		}
+
+		$writesOn = $this->resolveSyncWriter(source: $current, userId: $userId);
 
 		$required = [$source->getId() => true];
 		$ignored = [];
diff --git a/tests/Unit/Service/GroupMemberWritesForEveryoneTest.php b/tests/Unit/Service/GroupMemberWritesForEveryoneTest.php
index 2168910e1..910ef0e65 100644
--- a/tests/Unit/Service/GroupMemberWritesForEveryoneTest.php
+++ b/tests/Unit/Service/GroupMemberWritesForEveryoneTest.php
@@ -16,6 +16,7 @@
 use OCA\Keepiq\Db\SecretMapper;
 use OCA\Keepiq\Db\ShareTarget;
 use OCA\Keepiq\Db\ShareTargetMapper;
+use OCA\Keepiq\Exception\ForbiddenException;
 use OCA\Keepiq\Service\ShareAuditTrail;
 use OCA\Keepiq\Service\ShareAuthorizationService;
 use OCA\Keepiq\Service\SecretVersionService;
@@ -64,6 +65,9 @@ class GroupMemberWritesForEveryoneTest extends TestCase {
 	/** @var array<int,string> The rows the sync wrote, in order */
 	private array $written = [];
 
+	/** @var (callable(Secret):void)|null What happens to `src` once the sync holds its lock */
+	private $srcUnderLock = null;
+
 	protected function setUp(): void {
 		$this->audit = $this->createMock(ShareAuditTrail::class);
 		$this->versions = $this->createMock(SecretVersionService::class);
@@ -125,6 +129,10 @@ function (): int {
 					$this->groups = $this->groupsUnderLock;
 				}
 
+				if ($this->srcUnderLock !== null) {
+					($this->srcUnderLock)($this->rows['src']);
+				}
+
 				return 1;
 			}
 		);
@@ -300,6 +308,31 @@ public function testAMemberWhoLeftTheGroupBeforeTheSyncsLockIsRefused(): void {
 		$this->assertSame([], $this->written);
 	}
 
+	public function testASourceMadeReadOnlyBeforeTheSyncsLockRefusesAGroupMember(): void {
+		$this->srcUnderLock = static fn (Secret $src) => $src->setReadOnly(true);
+
+		try {
+			$this->sync()->syncUpdate('src', $this->everyRow(), self::BASE, 'bob');
+			$this->fail('A group member wrote a source that became read-only');
+		} catch (ForbiddenException) {
+		}
+
+		$this->assertSame([], $this->written);
+	}
+
+	public function testASourceMadeUseOnlyBeforeTheSyncsLockRefusesAGroupMember(): void {
+		$this->srcUnderLock = static fn (Secret $src) => $src->setUseOnly(true);
+
+		try {
+			$this->sync()->syncUpdate('src', $this->everyRow(), self::BASE, 'bob');
+			$this->fail('A group member wrote a source that became use-only');
+		} catch (InvalidArgumentException $e) {
+			$this->assertSame('A use-only copy cannot be changed', $e->getMessage());
+		}
+
+		$this->assertSame([], $this->written);
+	}
+
 	public function testTheOwnersSyncKeepsWorkingWithoutABase(): void {
 		$this->assertSame(1, $this->sync()->syncUpdate('src', [['secretId' => 'src', 'key' => 'NEW']], '', 'alice'));
 	}

From 2d90c2201cf6279da9c383880a60aca361d23135 Mon Sep 17 00:00:00 2001
From: Robert Zondervan <robert@conduction.nl>
Date: Tue, 20 Oct 2026 10:54:17 +0200
Subject: [PATCH 6/6] =?UTF-8?q?fix(review):=20#1294=20f2=20=E2=80=94=20pre?=
 =?UTF-8?q?-push:=20leave=20a=20valueless=20copy=20untouched=20only=20for?=
 =?UTF-8?q?=20a=20holder=20without=20a=20vault=20key?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

https://github.com/ConductionNL/keepiq/pull/1294#discussion_r4322767967

assertGroupWriteGuards() required every live copy to be named but not to
carry a value, so a group member's /sync could name a copy with no blob:
it got only the new updatedAt and kept the old ciphertext, and its holder
read a stale value under a fresh version.

Share-save's rule is followed: a copy may be left without the new value
only when its holder has no vault key (ShareSaveScope::refusedSkips()),
and then its row is not written at all. A named row with no non-empty
key, login or additionalFields is now refused when its holder (the
owner for the source) has an active key pair, and otherwise dropped from
the rows to write, so its version does not move.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 lib/Service/ShareSyncService.php              | 43 ++++++++++++++++---
 .../GroupMemberWritesForEveryoneTest.php      | 29 +++++++++++++
 2 files changed, 67 insertions(+), 5 deletions(-)

diff --git a/lib/Service/ShareSyncService.php b/lib/Service/ShareSyncService.php
index d3cffe2fb..c3447aae5 100644
--- a/lib/Service/ShareSyncService.php
+++ b/lib/Service/ShareSyncService.php
@@ -183,10 +183,11 @@ public function syncUpdate(
 	 * run under the source lock: the source is read again and refused when it
 	 * became a read-only or use-only copy, the caller is checked again, so a
 	 * leave or a revoke that committed after the first check refuses the sync,
-	 * and the
-	 * updates must name exactly the source and every copy whose access has
-	 * not ended, so no holder is left on the old value. Only those rows are
-	 * written.
+	 * and the updates must name exactly the source and every copy whose access
+	 * has not ended, so no holder is left on the old value. Only those rows are
+	 * written. A named row that carries no value is left out, untouched, as
+	 * share-save's `skipped` list leaves it, and only when its holder has no
+	 * vault key: every other holder must get the new value.
 	 *
 	 * @param Secret $source The source secret being synced
 	 * @param array<int,array<string,string|null>> $updates The per-recipient blobs
@@ -194,7 +195,7 @@ public function syncUpdate(
 	 *
 	 * @return array{0:string|null,1:array<string,bool>} How the audit names the write now, and the ids to write
 	 *
-	 * @throws InvalidArgumentException When the caller lost the right to write, or a row is missing or unexpected
+	 * @throws InvalidArgumentException When the caller lost the right to write, or a row is missing, unexpected or valueless
 	 *
 	 * @spec openspec/specs/sharing-group/spec.md#requirement-a-group-member-saves-for-everyone-it-is-shared-with
 	 * @spec openspec/changes/sharing-atomic-share-save/specs/user-sharing/spec.md#requirement-a-share-save-names-every-copy-or-is-refused
@@ -211,6 +212,7 @@ private function assertGroupWriteGuards(Secret $source, array $updates, string $
 		$writesOn = $this->resolveSyncWriter(source: $current, userId: $userId);
 
 		$required = [$source->getId() => true];
+		$holders = [$source->getId() => [$source->getOwnerType(), $source->getOwnerId()]];
 		$ignored = [];
 		$targets = $this->mapper->findBySourceSecret($source->getId());
 		$rows = $this->secretMapper->findByIds(ids: array_map(static fn ($target): string => $target->getSecretId(), $targets));
@@ -230,11 +232,16 @@ private function assertGroupWriteGuards(Secret $source, array $updates, string $
 			}
 
 			$required[$copyId] = true;
+			$holders[$copyId] = ['user', $target->getTargetUserId()];
 		}
 
 		$named = [];
+		$valueless = [];
 		foreach ($updates as $update) {
 			$named[(string)($update['secretId'] ?? '')] = true;
+			if ($this->carriesValue(update: $update) === false) {
+				$valueless[(string)($update['secretId'] ?? '')] = true;
+			}
 		}
 
 		if (array_diff_key($required, $named) !== [] || array_diff_key($named, $required, $ignored) !== []) {
@@ -243,9 +250,35 @@ private function assertGroupWriteGuards(Secret $source, array $updates, string $
 			);
 		}
 
+		foreach (array_keys(array_intersect_key($valueless, $required)) as $copyId) {
+			[$ownerType, $ownerId] = $holders[$copyId];
+			if ($this->suiteMapper->countActiveByOwner(ownerType: $ownerType, ownerId: $ownerId) > 0) {
+				throw new InvalidArgumentException(
+					message: 'Every holder with a vault key must get the new value'
+				);
+			}
+
+			// No vault key to encrypt for: the row keeps its value and its version.
+			unset($required[$copyId]);
+		}
+
 		return [$writesOn, $required];
 	}//end assertGroupWriteGuards()
 
+	/**
+	 * Whether an update row carries ciphertext for its copy: a non-empty key,
+	 * or a login or additional fields.
+	 *
+	 * @param array<string,string|null> $update The blob row
+	 *
+	 * @return bool
+	 */
+	private function carriesValue(array $update): bool {
+		return (isset($update['key']) === true && $update['key'] !== '')
+			|| array_key_exists('login', $update) === true
+			|| array_key_exists('additionalFields', $update) === true;
+	}//end carriesValue()
+
 	/**
 	 * Attribute a sync by anyone but the owner to the writer on the source
 	 * (§3.3), as a share-save through a copy is: a team writer as
diff --git a/tests/Unit/Service/GroupMemberWritesForEveryoneTest.php b/tests/Unit/Service/GroupMemberWritesForEveryoneTest.php
index 910ef0e65..1039bcaef 100644
--- a/tests/Unit/Service/GroupMemberWritesForEveryoneTest.php
+++ b/tests/Unit/Service/GroupMemberWritesForEveryoneTest.php
@@ -65,6 +65,9 @@ class GroupMemberWritesForEveryoneTest extends TestCase {
 	/** @var array<int,string> The rows the sync wrote, in order */
 	private array $written = [];
 
+	/** @var list<string> The users without a vault key */
+	private array $keyless = [];
+
 	/** @var (callable(Secret):void)|null What happens to `src` once the sync holds its lock */
 	private $srcUnderLock = null;
 
@@ -178,6 +181,9 @@ function (Secret $row): Secret {
 		$suite->setCertificate('OWNER-CERT');
 		$suites = $this->createMock(EncryptionSuiteMapper::class);
 		$suites->method('findActiveByOwner')->willReturn($suite);
+		$suites->method('countActiveByOwner')->willReturnCallback(
+			fn (string $ownerType, string $ownerId): int => in_array($ownerId, $this->keyless, true) ? 0 : 1
+		);
 
 		return new ShareSyncService(
 			mapper: $targets,
@@ -308,6 +314,29 @@ public function testAMemberWhoLeftTheGroupBeforeTheSyncsLockIsRefused(): void {
 		$this->assertSame([], $this->written);
 	}
 
+	public function testAGroupMembersSyncNamingACopyWithoutAValueIsRefused(): void {
+		// Dan has a vault key: his copy would keep the old value under a new version.
+		$updates = [['secretId' => 'src', 'key' => 'NEW'], ['secretId' => 'c-bob', 'key' => 'NEW'], ['secretId' => 'c-dan']];
+
+		try {
+			$this->sync()->syncUpdate('src', $updates, self::BASE, 'bob');
+			$this->fail('A copy was named without a value and its holder has a vault key');
+		} catch (InvalidArgumentException $e) {
+			$this->assertStringContainsString('must get the new value', $e->getMessage());
+		}
+
+		$this->assertSame([], $this->written);
+	}
+
+	public function testAGroupMembersSyncLeavesTheCopyOfAHolderWithoutAVaultKeyUntouched(): void {
+		$this->keyless = ['dan'];
+		$updates = [['secretId' => 'src', 'key' => 'NEW'], ['secretId' => 'c-bob', 'key' => 'NEW'], ['secretId' => 'c-dan']];
+
+		$this->assertSame(2, $this->sync()->syncUpdate('src', $updates, self::BASE, 'bob'));
+		$this->assertSame(['src', 'c-bob'], $this->written);
+		$this->assertNull($this->rows['c-dan']->getUpdatedAt());
+	}
+
 	public function testASourceMadeReadOnlyBeforeTheSyncsLockRefusesAGroupMember(): void {
 		$this->srcUnderLock = static fn (Secret $src) => $src->setReadOnly(true);