Skip to content

Members of a group a secret is shared with can't save changes for everyone #1285

Open

Description

@SudoThijn

Problem

A secret shared with a group is meant to be editable by every member of that group, for everyone it is shared with. Today a member can only change their own copy:

  • ShareSyncService::writeContext gives a group member effectiveGrade: 'none'. A grade only comes from team-folder membership (ShareAuthorizationService::resolveGrade → TeamFolderQueryService::resolveGrade), and only the owner or a delegate counts as owner.
  • ShareSyncService::resolveSyncWriter refuses anyone who isn't the owner, a delegate or a write/manage team-folder member. share-save's copy-editor path goes through it (ShareSaveScope::writerOn, again under the lock in assertStillWriter), so a group member's share-save is refused with not_allowed.
  • The clients therefore save a group member's edit to their own copy only, with a plain PUT /api/v1/secrets/{id}. The web app does that without saying so (saveShared in src/services/shareSave.js); the browser extension says "Only your copy changed. The owner and the others it is shared with keep the old value."

The result: the member holds a password nobody else has, and the owner's next save overwrites only the fields the owner changed, so the copy can end up with a mix of both.

Found while testing the browser extension's move to share-save (#1249): Alice shared a login with the admin group, a member changed its username and password, and Alice's secret kept the old values. The project lead and the backend lead confirmed group members should be able to edit it.

Proposed solution

  • A user who holds a copy of the source through a group share (ShareTarget.groupShareId set) may save for every copy, on share-save's copy-editor path.
  • writeContext answers effectiveGrade: 'write' for them, and returns ownerCertificate as it does for a write grade. Both clients take the copy-editor path for owner, write and manage only (COPY_WRITER_GRADES in src/services/shareSave.js, COPY_EDITOR_GRADES in browser-extension/src/vault/share-save.ts), so neither client needs a change if the grade name stays write.
  • resolveSyncWriter accepts them, so share-save (and the old /sync) let them write the source and every other copy. Partner shares stay with the owner's next save, as for a team-folder editor (ownerOnly).
  • Their saves are audited on the source as non-owner writes, as folder-permission-grades requires for a team-folder editor.
  • Specs:
    • openspec/specs/sharing-group gains the requirement.
    • openspec/specs/user-sharing "Recipient updates shared secret" and its share-save delta (openspec/changes/sharing-atomic-share-save/specs/user-sharing/spec.md, "who holds a write grade") are reconciled with it.
  • Tests: a group member's share-save reaching the source and the other copies, writeContext answering write, and a member removed from the group being refused again.

Open questions

1. Should a direct-share recipient also save for everyone?

This decides how far this fix reaches. A direct-share recipient has the same problem as a group member: their edit only changes their own copy. The user-sharing scenario "Recipient updates shared secret" says any recipient's edit reaches everyone; the share-save delta narrowed that to a recipient "who holds a write grade".

Suggested: A. It does what was confirmed, and widening write access later is easier than taking it back.

2. Must a save re-check that the user is still in the group?

This decides whether an ex-member can keep editing. Correction to the earlier text: a member who leaves does not keep their copy. UserRemovedFromGroupListener calls GroupShareService::handleMemberLeave, which deletes the copy and its ShareTarget. The listener does catch and only log a failure, though, and then the copy stays.

  • A. Trust the copy. resolveSyncWriter accepts a caller who holds a copy with groupShareId set. No extra lookup, but when the leave cleanup failed, the ex-member keeps saving for everyone until the copy is removed by hand.
  • B. Also check the group. resolveSyncWriter also checks that the caller is still in the group share's group (IGroupManager::isInGroup). One extra lookup per save, and a failed cleanup no longer grants write access, though the ex-member can still read their copy.

Suggested: B. It is cheap, and the planned test "a member removed from the group is refused again" then holds even when the listener fails.

3. Fix #1266 in the same change?

This decides the scope of the change. #1266: writeContext answers any logged-in user for any secret id. This issue changes the same method. Whichever option is picked, the new check must ask whether the caller holds a group-derived copy of the source, not whether the secret id passed in is one. Otherwise writeContext answers write to anyone who knows a copy's id.

Suggested: A.

Anything else?

The browser extension (#1249) needs no change for this: once writeContext answers write for a group member and share-save accepts them, their edit takes the copy-editor path and reaches the owner and every other copy.

Activity

  1. github-actions commented on Oct 16, 2026

    @github-actions
    Contributor

    Thank you for opening this issue! It has been assigned to @rubenvdlinde for review.

    What happens next:

    1. A maintainer will review this issue and determine the appropriate action
    2. If accepted, it may be converted into an OpenSpec change for structured development
    3. You will be notified of any status changes

    Triage labels:

    • triage:accept-feature — accepted as a new feature
    • triage:accept-bugfix — accepted as a bug fix
    • triage:needs-info — more information needed
    • triage:wontfix — will not be addressed
    • triage:duplicate — duplicate of another issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bugSomething isn't workinggroup-sharingGroup-based secret sharingtriageAwaiting triage

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions