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..c3447aae5 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() @@ -104,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> $updates The per-recipient blobs; each row has @@ -137,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 @@ -149,7 +157,17 @@ 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); + if ($writesOn === self::GROUP_WRITE) { + [$writesOn, $allowedIds] = $this->assertGroupWriteGuards(source: $source, updates: $updates, userId: $userId); + } + + $updated = $this->writeCopyBlobs( + source: $source, + updates: $updates, + allowedIds: $allowedIds, + savedAt: $savedAt, + writerId: $userId + ); if ($updated > 0) { $this->recordNonOwnerSync(source: $source, userId: $userId, writesOn: $writesOn); } @@ -160,6 +178,107 @@ 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 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. 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> $updates The per-recipient blobs + * @param string $userId The requesting user + * + * @return array{0:string|null,1:array} 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, 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 + */ + private function assertGroupWriteGuards(Secret $source, array $updates, string $userId): array { + // 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]; + $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)); + $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; + $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) !== []) { + throw new InvalidArgumentException( + message: 'Name the source and every copy whose access has not ended, and nothing else' + ); + } + + 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 $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 @@ -380,19 +499,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> $updates The per-recipient blobs * @param array $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 +535,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 +555,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/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 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..1039bcaef 100644 --- a/tests/Unit/Service/GroupMemberWritesForEveryoneTest.php +++ b/tests/Unit/Service/GroupMemberWritesForEveryoneTest.php @@ -11,12 +11,15 @@ 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\Exception\ForbiddenException; 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; @@ -34,6 +37,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 */ private array $rows = []; @@ -52,9 +57,25 @@ class GroupMemberWritesForEveryoneTest extends TestCase { private ShareAuditTrail&MockObject $audit; + private SecretVersionService&MockObject $versions; + + /** @var array>|null The members of each group once the sync holds the source lock */ + private ?array $groupsUnderLock = null; + + /** @var array The rows the sync wrote, in order */ + private array $written = []; + + /** @var list 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; + 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 = [ @@ -101,7 +122,29 @@ 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; + } + + if ($this->srcUnderLock !== null) { + ($this->srcUnderLock)($this->rows['src']); + } + + 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); @@ -138,6 +181,9 @@ private function sync(): ShareSyncService { $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, @@ -154,6 +200,7 @@ private function sync(): ShareSyncService { groupManager: $groups, ), auditTrail: $this->audit, + versionService: $this->versions, ); } @@ -192,7 +239,131 @@ 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 { + $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(), self::BASE, '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 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 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); + + 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')); } public function testAMemberWhoLeftTheGroupKeepsTheirCopyButNoWriteGrade(): void { @@ -222,7 +393,19 @@ 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 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 { 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..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 Rows written by the copy fan-out */ private array $copyWrites = []; @@ -287,6 +293,7 @@ private function service(?ShareAuthorizationService $auth = null): ShareSaveServ suiteMapper: $this->suites, db: $this->db, auth: ($auth ?? $this->auth), + versionService: $this->versionHistory, ); return new ShareSaveService( @@ -373,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')); @@ -392,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. * @@ -850,18 +872,57 @@ public function testACopyEditorsAuditNamesTheOwnerWhenTheSourceIsLeftOut(): void $this->assertSame(['written' => 1, 'skipped' => 1], $result['copies']); } + /** + * Log the snapshots the copy fan-out takes, as `snapshot `. + * + * @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: []); @@ -885,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']); 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,