fix(sharing): let group members save a shared secret for everyone by SudoThijn · Pull Request #1294 · ConductionNL/keepiq

SudoThijn · 2026-10-19T10:21:48Z

Closes #1285
Closes #1266

Summary

A member of a group a secret is shared with can now change it for everyone it is shared with. Their edit used to reach only their own copy, so the owner and the other recipients kept the old value. The write context now also answers 404 to a caller with no access to the secret (#1266).

Changes

Who saves for everyone

A caller gets the write grade, with ownerCertificate, when they hold a live copy of the source that isn't use-only and are in the group of a non-expired, non-use-only group share of that source. ShareAuthorizationService::copyGrade and isInSharedGroup decide this:

The check is about the caller's own copy, found by source and user, never about the secret id that was passed in.
How the copy got there doesn't matter. A member whose copy came first through a direct share, a team folder or another group share still counts. The copy's groupShareId is ignored, so a crafted one gives nothing.
Membership is checked live with IGroupManager::isInGroup, so a member whose copy survived a failed leave cleanup is refused. Share-save re-reads the share row, the copy and the group shares under its lock.
A use-only group share never counts, even when another grant makes the copy full. Otherwise a full direct share and a use-only group share would add up to write access for everyone.
Direct-share recipients in no such group keep none and edit only their own copy.

writeContext and resolveSyncWriter share standingOn(), so the grade the client sees and the check on the server can't drift apart. resolveSyncWriter now returns the audit label instead of a bool, so a group member's save is audited on the source as group-write sync, not as a team-folder write. Partner shares stay owner-only, as for a team-folder editor. The clients need no change, because COPY_WRITER_GRADES already includes write.

A copy holder who joins a shared group later

A user who already holds a copy, for example through a direct share, and later joins a group the secret is shared with in full, gets the right at once, with nothing for the owner to approve. @SudoThijn decided this; a link to the decision on #1285 follows. The owner gets a new group_member_gained_write notification without Approve or Deny: "X joined the group G and can now change S for everyone it is shared with. Revoke their share to stop this." It follows the owner's notify_security setting, not notify_group_shares, because turning it off would hide a grant of write rather than just skip an approval. When joining changes nothing, for example because the group share or the copy is use-only, the owner isn't notified. A new member without a copy still gets the ordinary approve/deny notification.

Versions kept before a non-owner's save

A save by a delegate, a team-folder writer or a group member through their copy now keeps the owner's previous value as a version of the owner's secret, on share-save and on /sync, so the owner can restore it. Only the writer's own copy used to keep a version, and /sync kept none.

A group member's `/sync`

A group member's PUT /api/v1/secrets/{id}/sync gets share-save's guards (ShareSyncGuards): it needs a base version, must name the source and every live copy exactly once, may not clear a copy's key, and is checked again under the source's lock. Otherwise it answers 409, or 403 when the source is a read-only copy from another organisation.

The write context answers 404 (#1266)

writeContext answers only the owner, a delegate, a holder of a copy or a team-folder member with a grade. Anyone else, including a user who only knows someone else's copy id, gets the same 404 as for an unknown id. A caller who asks about a live row of their own whose share rows lead to no source, such as a copy with two share rows, gets effectiveGrade: 'none' with no source, so they can still save their own row. When that row is itself the source of share rows, they get 404 instead, so a save of it never skips its copies.

Specs and changelog

sharing-group gains the requirement and the late-joiner scenario.
user-sharing "Recipient updates shared secret", the "New Group Member — Owner Notification" requirement, its sharing-atomic-share-save delta and the folder-permission-grades read-grade scenario are reconciled with it.
user-settings and docs/FEATURES.md list the new notification under notify_security.
The CHANGELOG [Unreleased] section has entries under Fixed and Security.

Follow-ups (out of scope)

The sidebar badge "Editable — changes sync to the whole team" says "team", and group members now see it too.
GroupShareService::handleMemberLeave deletes a copy even when the user is still in another group the secret is shared with.
Leaving a group never recalculates the use-only flag and expiry of the copies a user keeps, so a copy can stay full after its only full grant is gone.
POST /shares doesn't check that recipientSecretId is a fresh copy owned by the recipient.
share_targets has no unique index on (source, user).
/sync doesn't re-check a team-folder writer or a delegate under the lock, and doesn't apply the group member's other guards to them either.
A joiner whose copy's access ended still gets group_member_added, and Approve does nothing. A joiner with two share rows gets it too, and Approve throws an uncaught MultipleObjectsReturnedException.
A use-only or read-only copy holder who joins a full group share never gets the full copy a new member would get.
The group_member_gained_write strings, like group_member_added's, aren't in the l10n catalogues yet.
The ExcessiveClassComplexity suppression comment on KeepiqNotifier gives an outdated measured value.

Test plan

PHPUnit, full tests/Unit in the container: 2402 tests, 3 skipped. GroupMemberWritesForEveryoneTest has 47 tests, and ShareSaveServiceTest, KeepiqNotifierTest and NotificationServiceTest cover the share-save path and the new notification.
phpcs over lib, phpstan and phpmd pass. psalm crashes on the container's PHP 8.4, so it ran in php:8.3-cli with ext-zip, where it finds no errors.
Reviewed in two rounds on this PR, with the round 1 fixes in fix(review): #1294 review findings, round 1 #1296.
Live e2e on the dev instance before the review rounds, tests/e2e/workflows/share-save.spec.ts (11 passed, 1 skipped: the staging test):
- a group member edits a shared login, and the owner and another member see the change, with the write context and the audit entry checked;
- a member whose copy came through a direct share saves for everyone, and is refused after leaving the group;
- a direct recipient beside a group share gets none and changes only their own copy;
- a team-folder read member gets read and is refused.
Gate-19 finds a test for every scenario this PR adds; its remaining findings all fail on development too.

A member of a group a secret is shared with in full now gets the write grade from the write context and passes share-save's copy-editor path, so their change reaches the source and every other copy. Membership is checked live against the source's group shares; use-only group shares and direct shares don't give the grade. The write context now answers 404, the same as for an unknown id, to a caller who doesn't own the secret, isn't its delegate, holds no copy of it and has no team-folder grade on it. Closes #1285 Closes #1266

github-actions · 2026-10-19T10:29:45Z

Quality Report — ConductionNL/keepiq @ `e6a208e`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
format		✅
check-l10n-js		✅
check-schema-l10n		✅
composer			✅	✅ 114/114
npm			✅	✅ 660/660
app:check-code	⏭️
info.xml	✅
REUSE	✅
lockfile sync		✅
PHPUnit					✅ 2345/2345 · 3 skipped
Newman					✅ 177/177 · 20 skipped
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets `development`, so the suite is asked once per promotion into `beta` and `main` rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into `development`.
Hydra gates					✅

Quality workflow — 2026-10-19 10:29 UTC

Download the full PDF report from the workflow artifacts.

Four live scenarios in the share-save e2e: a group member's edit reaching the owner and the other members, a member whose copy came through a direct share, a direct recipient beside a group share, and a team-folder read member. The new sharing-group scenarios point at them, so Gate-19 finds a test for each.

github-actions · 2026-10-19T11:15:47Z

Quality Report — ConductionNL/keepiq @ `7b42554`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
format		✅
check-l10n-js		✅
check-schema-l10n		✅
composer			✅	✅ 114/114
npm			✅	✅ 660/660
app:check-code	⏭️
info.xml	✅
REUSE	✅
lockfile sync		✅
PHPUnit					✅ 2345/2345 · 3 skipped
Newman					✅ 177/177 · 20 skipped
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets `development`, so the suite is asked once per promotion into `beta` and `main` rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into `development`.
Hydra gates					✅

Quality workflow — 2026-10-19 11:15 UTC

Download the full PDF report from the workflow artifacts.

rjzondervan · 2026-10-20T08:03:33Z

+				continue;
+			}
+
+			if ($this->groupManager->isInGroup($userId, $groupShare->getGroupId()) === true) {


🔴 Blocker — Group membership changes grant write-for-everyone without the owner

Risk: privilege escalation by whoever controls a group's membership. Membership is read live here. When an existing copy holder (for example a direct-share recipient) is added to a group the secret is shared with in full, they get write at once. handleNewGroupMember() only notifies the owner (GroupShareService.php:325-358), and approveGroupMemberShare() returns early because a (source, user) row already exists (GroupShareService.php:395-401). So the owner has nothing to approve or deny. Anyone who can change membership grants it: an instance admin, a group subadmin, an LDAP or OIDC group sync, or the re-creation of a deleted group under the same id. A brand-new member, by contrast, gets nothing until the owner approves.

This contradicts the spec that stays live after the PR:

openspec/specs/user-sharing/spec.md:14: "Direct shares are always independent of group membership."

openspec/specs/user-sharing/spec.md:220 and :428: the owner approves a joining member "so that I can decide whether to extend access".

#1285's refinement covers a member who held a direct copy when the owner shared with the group. It doesn't cover a holder who joins later. The PR body itself defers that case to the PO ("worth confirming with the PO"). Under Strict, an open authorization question blocks.

Impact: A Nextcloud admin or subadmin, or the IdP, upgrades any read recipient to "overwrite the owner and everyone". With f1 that write can't be undone.

Suggested fix: Get the PO decision on #1285 first. Then do one of these:

(a) Accept it: rewrite user-sharing/spec.md:14 and the "New Group Member — Owner Notification" requirement to say an existing holder gains write on joining. Add a sharing-group scenario "A copy holder who joins a shared group later" and a test.

(b) Gate it: an existing holder who joins counts only after the owner approves, so approveGroupMemberShare() records that approval instead of returning early, and isInSharedGroup() requires it. Test seam: GroupMemberWritesForEveryoneTest ($this->groups, $this->targets).

Done when:

Members of a group a secret is shared with can't save changes for everyone #1285 links the PO decision, and git grep -n "always independent of group membership" -- openspec/specs returns nothing, or the code gates the grant

a test named for the late-join case pins the decided outcome and goes red when it is flipped

The spec now encodes option (a) consistently (user-sharing/spec.md:14, :231, :428; a sharing-group scenario and testACopyHolderWhoJoinsASharedGroupLaterSavesForEveryone), so the contradiction is gone. What is still missing is the decision itself: nothing on #1285 or here records that the PO chose it, and the PR body still says "worth confirming with the PO". Leaving this open until that confirmation is on #1285 or this PR; the notification side of it is g1.

rjzondervan

Verdict: REQUEST_CHANGES (Strict)

Group members can now save for everyone, and the authorization holds where the PR says it does: the grade comes from the caller's own copy, a crafted groupShareId gives nothing, use-only and expired group shares never count, and share-save re-checks the writer under its lock. Two blockers come from what the new grade reaches: a member's save leaves the owner no version to restore, and a membership change grants write-for-everyone without the owner. The branch also conflicts with development in CHANGELOG.md.

🔧 Fix PR: #1296 — 4 of 6 findings, one commit each (plus two pre-push commits for f2)

Merge it into fix/1285-group-member-share-save, then re-request the review. f3 (the PO decision) and f5 stay on their threads.

🔴 Blockers (2)

A group member's save leaves the owner no version to restore — lib/Service/ShareSyncService.php:280. (fix proposed in #1296) The copy-editor path writes the owner's source and the other copies without a snapshot, against secret-version-history/spec.md:15; only the caller's own row is snapshotted.
Group membership changes grant write-for-everyone without the owner — lib/Service/ShareAuthorizationService.php:287. Adding an existing copy holder to the group gives them write at once; the approval path returns early because their row exists, contradicting user-sharing/spec.md:14, :220 and :428. A product decision the PR body leaves to the PO; it blocks until it is made.

🟡 Concerns (2)

/sync opened to group members without share-save's guards — lib/Service/ShareSyncService.php:139. (fix proposed in #1296) No re-check under the lock, '' skips the base check, and a partial set of copies is accepted.
user-sharing spec still says either party's update propagates — openspec/specs/user-sharing/spec.md:194. (fix proposed in #1296) Two live copies (:12, :440) contradict the new scenario, and :194 drops delegates.

🟢 Minor (2)

write-context 404 locks some holders out of their own row — lib/Service/ShareAuthorizationService.php:245.
Contract table: two body claims without a test — tests/Unit/Service/GroupMemberWritesForEveryoneTest.php:219. (fix proposed in #1296)

✅ Verified clean

copyGrade() keys on (source, caller) and never reads groupShareId; a use-only, expired or revoked group share never counts; a copy whose access ended answers 404.
Share-save re-checks the writer after lockVersions; partner shares stay owner-only on the copy-editor path.
writeContext and resolveSyncWriter both use standingOn(); every resolveSyncWriter caller handles the new audit label, and the InvalidArgumentException still maps to 409/403.
#1266: a stranger and a caller who knows only someone else's copy id get 404; two share rows give 404 instead of a 500.
The constructor changes are optional, appended parameters; ShareSavePath has only named callers.

Persistence audit

Nine vectors covered (removal from the group on share-save, a failed leave cleanup, expired/use-only/revoked group shares, crafted groupShareId, duplicate rows, partner shares, the audit label). Three come from this PR: the missing version history (f1), /sync (f2) and the membership-driven grant (f3). Pre-existing and only marginally widened: a crafted recipientSecretId, which only the owner or a delegate can plant.

Connected issues

Issue	State	This review
#1285	open	done for share-save; f1 and f3 stand between it and closing; closes on merge
#1266	open	done: the write context answers 404 as asked; closes on merge

CI: every check green on a90001d, including quality / Hydra Gates. Merge: conflicts with development in CHANGELOG.md (only that file).

🤖 Generated with Claude Code

…sion to restore #1294 (comment) 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>

…ave's guards #1294 (comment) 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>

…update propagates #1294 (comment) 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>

#1294 (comment) - 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>

…se-only before the sync's lock #1294 (comment) 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>

…ly for a holder without a vault key #1294 (comment) 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>

fix(review): #1294 review findings, round 1

…-member-share-save # Conflicts: # CHANGELOG.md

…d to no source A caller who owns a live row whose share rows are ambiguous, or keyed to someone else, got 404 from the write context, so their client could not save that row at all. They now get effectiveGrade none with no source, so the client saves their own row alone. Anyone else still gets the 404 of an unknown id.

…saves for everyone The holder gets none before joining and write with the owner certificate once they are in the group, before the owner answers the new-member notification; approving it creates no second share row.

… with the lock

…py write ShareSyncService takes SecretVersionService as a required constructor argument and writeCopyBlobs() a required writerId, so no caller can skip the source's snapshot by leaving either out.

… lock A non-owner's sync that changes no value, or writes over an empty source, keeps no version; a delegate's and a team-folder writer's sync keep the source's value with them as actor. Reads under the lock return a new object, so the read-only and use-only checks must use the row read again. The refusal tests name a base, so they reach the writer check.

…ber's sync A group member's /sync now runs share-save's body checks: a row whose key is empty would wipe that holder's value, and a row named twice would make the outcome depend on the order of the rows. Both refuse the sync with 409, as its other guards do.

Share-save keeps the caller's row through SecretService::update(), but /sync wrote the caller's own copy with no version. It now keeps one too, under the same rule: only when a value changes and the row held one.

… everyone Record the decision on the late joiner in user-sharing and sharing-group, document the group member's /sync guards, the versions kept before a non-owner's save and the write context's answer to a holder whose share rows lead to no source.

…between share-save and sync

…ncService

github-actions · 2026-10-20T10:58:45Z

Quality Report — ConductionNL/keepiq @ `72e1377`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	❌
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
format		✅
check-l10n-js		✅
check-schema-l10n		✅
composer			✅	✅ 114/114
npm			✅	✅ 660/660
app:check-code	⏭️
info.xml	✅
REUSE	✅
lockfile sync		✅
PHPUnit					✅ 2393/2393 · 3 skipped
Newman					✅ 177/177 · 20 skipped
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets `development`, so the suite is asked once per promotion into `beta` and `main` rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into `development`.
Hydra gates					✅

Quality workflow — 2026-10-20 10:58 UTC

Download the full PDF report from the workflow artifacts.

github-actions · 2026-10-20T11:13:47Z

Quality Report — ConductionNL/keepiq @ `8a4405b`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
format		✅
check-l10n-js		✅
check-schema-l10n		✅
composer			✅	✅ 114/114
npm			✅	✅ 660/660
app:check-code	⏭️
info.xml	✅
REUSE	✅
lockfile sync		✅
PHPUnit					✅ 2393/2393 · 3 skipped
Newman					✅ 177/177 · 20 skipped
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets `development`, so the suite is asked once per promotion into `beta` and `main` rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into `development`.
Hydra gates					✅

Quality workflow — 2026-10-20 11:13 UTC

Download the full PDF report from the workflow artifacts.

… right to save for everyone

github-actions · 2026-10-20T12:40:51Z

Quality Report — ConductionNL/keepiq @ `d74068c`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
format		✅
check-l10n-js		✅
check-schema-l10n		✅
composer			✅	✅ 114/114
npm			✅	✅ 660/660
app:check-code	⏭️
info.xml	✅
REUSE	✅
lockfile sync		✅
PHPUnit					✅ 2393/2393 · 3 skipped
Newman					✅ 177/177 · 20 skipped
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets `development`, so the suite is asked once per promotion into `beta` and `main` rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into `development`.
Hydra gates					✅

Quality workflow — 2026-10-20 12:40 UTC

Download the full PDF report from the workflow artifacts.

…s 403 The re-check under the lock refuses a read-only copy from another organisation with 403, not 409; the requirement and the changelog now say so.

…an now change the secret When a member who already holds a live copy joined, the owner got the new-member notice, whose Approve and Deny do nothing. The owner now gets group_member_gained_write when the group share lets that copy save for everyone, without actions and under the security toggle, and no notice when joining changes nothing.

… row keyed to someone else A share row that names the owner's source as someone else's copy gave her none, so her client saved the source alone and left every copy behind without an error. The own-row answer now goes only to a row with no copies of its own.

…hared group can now change the secret Record the group_member_gained_write notice under notify_security, that a use-only joiner tells the owner nothing, and that a shared source behind a stray share row answers 404 in the write context.

github-actions · 2026-10-20T14:26:55Z

Quality Report — ConductionNL/keepiq @ `0360c2c`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
format		✅
check-l10n-js		✅
check-schema-l10n		✅
composer			✅	✅ 114/114
npm			✅	✅ 660/660
app:check-code	⏭️
info.xml	✅
REUSE	✅
lockfile sync		✅
PHPUnit					✅ 2399/2399 · 3 skipped
Newman					✅ 177/177 · 20 skipped
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets `development`, so the suite is asked once per promotion into `beta` and `main` rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into `development`.
Hydra gates					✅

Quality workflow — 2026-10-20 14:26 UTC

Download the full PDF report from the workflow artifacts.

rjzondervan · 2026-10-20T14:29:53Z

+					continue;
+				}
+
+				$subject = 'group_member_gained_write';


🟢 Minor — group_member_gained_write also fires for a member who could already write through another group

copyGrade() answers write when the joiner is in any full, live group share of the source (isInSharedGroup), not because of the group just joined. A copy holder who already saves for everyone through group B and then joins group A (also shared in full) makes the owner get a notify_security alert saying they "can now change" the secret, which is not new. The other half of the condition, $groupShare->isFullAndLive() === false, only matters in that same case (write through another group, joining a use-only or expired share); in every existing test copyGrade is already none there, so deleting that half keeps the suite green.

Suggested fix: notify only when the joined group share is what grants the write: compare copyGrade with this group share left out (or check that no other full, live group share already covers the user) before choosing group_member_gained_write, and add a test where the joiner already writes through another group (no notification), which also pins the isFullAndLive() half.

rjzondervan

Verdict: REQUEST_CHANGES (Strict, round 2)

1 blocker still open — f3, until the PO's decision is recorded — and one new 🟢; everything else from both rounds is fixed and resolved. Before a merge: f3, one approval, and the 2 open threads (f3 and h1) resolved.

🔧 Fix PR: #1296 — merged, 4 of 4 verified (and kept through the refactor)

Round	Found	Status
1 (`a90001d`)	🔴 2 · 🟡 2 · 🟢 2	f1, f2, f4, f6 fixed in #1296; f5 in `54606078`; f3 encoded in the spec, PO decision not yet recorded
2 (`65b78a0` → `d93ee9c`) — this review	🟡 3 · 🟢 2	g1–g4 fixed within the round (`4cb87dfc`, `ce2158ae`, `16360e34`, `d93ee9c2`, body updated); h1 open

Finding	Was	Now	Where
🔴 A group member's save leaves the owner no version to restore	🔴 round 1	resolved in #1296, now `ShareSyncService::keepVersion`	thread
🟡 `/sync` opened to group members without share-save's guards	🟡 round 1	resolved in #1296, now `ShareSyncGuards`	thread
🟡 user-sharing spec still says either party's update propagates	🟡 round 1	resolved in #1296	thread
🟢 write-context 404 locks some holders out of their own row	🟢 round 1	resolved in `54606078`	thread
🟢 Contract table: two body claims without a test	🟢 round 1	resolved in #1296	thread
🟡 The owner is asked to approve a write that already took effect	🟡 this round	resolved in `4cb87dfc`, `d93ee9c2`	thread
🟡 A read-only source on `/sync` answers 403, not the promised 409	🟡 this round	resolved in `ce2158ae`	thread
🟡 The PR body still describes round 1	🟡 this round	resolved (body updated; the #1285 link belongs to f3)	thread
🟢 Own source behind a planted share row gets a silent own-row save	🟢 this round	resolved in `16360e34`	thread
🔴 Group membership changes grant write-for-everyone without the owner	🔴 round 1	still open: the spec, a test and the new `group_member_gained_write` notification encode option (a), but no PO decision is recorded (the body says "@SudoThijn decided this; a link to the decision on #1285 follows", and #1285 has no such comment yet)	thread
🟢 `group_member_gained_write` also fires for a member who could already write through another group	new	new — 🟢	comment

Details of this round

Scope: 34 commits since a90001d: #1296 (merged), development merged in (270dd203, a CHANGELOG.md-only conflict resolved keeping both sides), and 16 commits of the author — 11 before this round's comments (270dd203..65b78a0) and 5 that answered them (65b78a0..d93ee9c), both analysed at Strict depth.
Verified clean: the refactor into ShareSyncGuards and ShareWriteContext keeps every guard and its order on share-save and /sync; writeContext and resolveSyncWriter still share standingOn; the new own-copy version on /sync does not double up with share-save; SecretService's snapshot rules are unchanged; every changed constructor's call sites are updated; group_member_gained_write is rendered without actions, follows notify_security (default on), and is not sent for a use-only copy or group share; a direct-share holder who leaves a group loses the right unless another full group share still covers them.
CI: green on 65b78a0; d93ee9c is docs, the notifier and tests on top of it.

Before the next round: fix rules on (fix-induced 2 + missed-sibling 0 ≥ new 1; copy-missed 1): the next fixes go through the class sweep first.

🤖 Generated with Claude Code

rjzondervan reviewed Oct 20, 2026

View reviewed changes

rjzondervan requested changes Oct 20, 2026

View reviewed changes

rjzondervan and others added 6 commits October 20, 2026 10:16

rjzondervan mentioned this pull request Oct 20, 2026

fix(review): #1294 review findings, round 1 #1296

Merged

SudoThijn and others added 12 commits October 20, 2026 11:58

Merge pull request #1296 from ConductionNL/review/pr-1294-fixes

af89599

fix(review): #1294 review findings, round 1

Merge remote-tracking branch 'origin/development' into fix/1285-group…

270dd20

…-member-share-save # Conflicts: # CHANGELOG.md

fix(sharing): say that a valueless source row's version already moved…

a798f34

… with the lock

refactor(sharing): require the version history and the writer of a co…

261d117

…py write ShareSyncService takes SecretVersionService as a required constructor argument and writeCopyBlobs() a required writerId, so no caller can skip the source's snapshot by leaving either out.

fix(sharing): keep a version of the writer's own copy on /sync

be907eb

Share-save keeps the caller's row through SecretService::update(), but /sync wrote the caller's own copy with no version. It now keeps one too, under the same rule: only when a value changes and the row held one.

refactor(sharing): share the copy-write checks and the snapshot rule …

ef826d0

…between share-save and sync

refactor(sharing): move the group member's sync guards out of ShareSy…

1fb0221

…ncService

refactor(sharing): move the write context out of ShareSyncService

65b78a0

docs(sharing): say a direct-share holder who leaves a group loses the…

4e8da82

… right to save for everyone

rjzondervan reviewed Oct 20, 2026

View reviewed changes

Comment thread openspec/specs/user-sharing/spec.md Outdated

Comment thread openspec/specs/sharing-group/spec.md Outdated

Comment thread CHANGELOG.md Outdated

Comment thread lib/Service/ShareSync/ShareWriteContext.php

SudoThijn added 4 commits October 20, 2026 16:04

docs(sharing): say a group member's sync of a read-only source answer…

ce2158a

…s 403 The re-check under the lock refuses a read-only copy from another organisation with 403, not 409; the requirement and the changelog now say so.

rjzondervan reviewed Oct 20, 2026

View reviewed changes

rjzondervan requested changes Oct 20, 2026

View reviewed changes

rjzondervan mentioned this pull request Oct 20, 2026

fix(sharing): keep a recipient without write permission from editing their copy #1302

Open

Conversation

SudoThijn commented Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Who saves for everyone

A copy holder who joins a shared group later

Versions kept before a non-owner's save

A group member's /sync

The write context answers 404 (#1266)

Specs and changelog

Follow-ups (out of scope)

Test plan

Uh oh!

github-actions Bot commented Oct 19, 2026

Quality Report — ConductionNL/keepiq @ e6a208e

Uh oh!

github-actions Bot commented Oct 19, 2026

Quality Report — ConductionNL/keepiq @ 7b42554

Uh oh!

Uh oh!

Uh oh!

rjzondervan Oct 20, 2026

Choose a reason for hiding this comment

🔴 Blocker — Group membership changes grant write-for-everyone without the owner

Uh oh!

rjzondervan Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rjzondervan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Verdict: REQUEST_CHANGES (Strict)

🔧 Fix PR: #1296 — 4 of 6 findings, one commit each (plus two pre-push commits for f2)

🔴 Blockers (2)

🟡 Concerns (2)

🟢 Minor (2)

✅ Verified clean

Persistence audit

Connected issues

Uh oh!

github-actions Bot commented Oct 20, 2026

Quality Report — ConductionNL/keepiq @ 72e1377

Uh oh!

github-actions Bot commented Oct 20, 2026

Quality Report — ConductionNL/keepiq @ 8a4405b

Uh oh!

github-actions Bot commented Oct 20, 2026

Quality Report — ConductionNL/keepiq @ d74068c

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Oct 20, 2026

Quality Report — ConductionNL/keepiq @ 0360c2c

Uh oh!

rjzondervan Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

rjzondervan left a comment

Choose a reason for hiding this comment

Verdict: REQUEST_CHANGES (Strict, round 2)

🔧 Fix PR: #1296 — merged, 4 of 4 verified (and kept through the refactor)

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SudoThijn commented Oct 19, 2026 •

edited

Loading

A group member's `/sync`

Quality Report — ConductionNL/keepiq @ `e6a208e`

Quality Report — ConductionNL/keepiq @ `7b42554`

rjzondervan left a comment •

edited

Loading

Quality Report — ConductionNL/keepiq @ `72e1377`

Quality Report — ConductionNL/keepiq @ `8a4405b`

Quality Report — ConductionNL/keepiq @ `d74068c`

Quality Report — ConductionNL/keepiq @ `0360c2c`