Skip to content

Fix revoked sharing reuse#3034

Merged
shepilov merged 1 commit into
masterfrom
fix/remove-shareing-after-last-recipient-removed
Jun 16, 2026
Merged

Fix revoked sharing reuse#3034
shepilov merged 1 commit into
masterfrom
fix/remove-shareing-after-last-recipient-removed

Conversation

@shepilov

@shepilov shepilov commented Jun 15, 2026

Copy link
Copy Markdown
Member

Summary

Fixes cozy-sharing state after the last recipient is revoked from a non-shared-drive sharing.

The reducer already removed the document index for fully revoked non-drive sharings, but it kept the stale sharing object in state.sharings. That could let later share flows reuse the deleted sharing and call POST /sharings/{old_id}/recipients, which fails when adding the same recipient again.

Changes

  • Remove fully revoked non-drive sharings from state.sharings on revoke/update.
  • Keep existing shared-drive behavior unchanged.
  • Add a regression test for revoking the last recipient.

Validation

  • corepack yarn workspace cozy-sharing test --runTestsByPath src/state.spec.js --moduleNameMapper '{"^cozy-flags$":"<rootDir>/../cozy-flags/src/index.js"}'
  • corepack yarn workspace cozy-sharing lint passed with existing unrelated warnings.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed sharing cleanup logic to properly handle revoked recipients. When all recipients of a sharing are revoked, the sharing record is now removed from the system, maintaining data consistency.
  • Tests

    • Added test coverage for sharing removal when the last recipient is revoked.

@shepilov shepilov changed the title [codex] Fix revoked sharing reuse Fix revoked sharing reuse Jun 15, 2026
@shepilov shepilov marked this pull request as ready for review June 15, 2026 15:01
@coderabbitai

coderabbitai Bot commented Jun 15, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

In packages/cozy-sharing/src/state.js, the sharings reducer gains a conditional early return for the UPDATE_SHARING, REVOKE_GROUP, and REVOKE_RECIPIENT action types. When the targeted sharing is not a drive and all its recipients have been revoked, the sharing is removed from the state array by filtering on its id rather than updated in place. A corresponding unit test in state.spec.js dispatches revokeRecipient for the last remaining recipient of a sharing and asserts that both sharings and byDocId no longer contain that sharing, while the other sharing in state remains unaffected.

Suggested reviewers

  • JF-Cozy
  • doubleface
  • rezk2ll
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title directly addresses the main fix: removing stale sharing objects that could be reused after revocation, which is the core issue resolved in this PR.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/remove-shareing-after-last-recipient-removed

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
packages/cozy-sharing/src/state.spec.js (1)

346-360: ⚡ Quick win

Consider verifying sharedPaths cleanup for complete coverage.

The test verifies removal from byDocId and sharings, but the sharedPaths reducer (lines 290-295 in state.js) also removes paths for fully-revoked non-drive sharings. For complete coverage, consider adding the path via addSharing and verifying it's removed from state.sharedPaths, similar to the shared drive test on lines 196-263.

✅ Proposed enhancement for complete test coverage
  it('should forget a sharing when revoking the last recipient', () => {
+   const sharingPath = '/folder_2'
    const state = reducer(
-     reducer(
+     reducer(
+       reducer(
-         undefined,
+           undefined,
-         receiveSharings({
+           receiveSharings({
-           sharings: [SHARING_1, SHARING_2]
+             sharings: [SHARING_1, SHARING_2]
-         })
+           })
+         ),
+       addSharing(SHARING_2, sharingPath)
      ),
      revokeRecipient(SHARING_2, 1)
    )
    expect(state.byDocId).toEqual({
      folder_1: { sharings: [SHARING_1.id], permissions: [] }
    })
    expect(state.sharings).toEqual([SHARING_1])
+   expect(state.sharedPaths).not.toContain(sharingPath)
  })
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/cozy-sharing/src/state.spec.js` around lines 346 - 360, The test for
the revokeRecipient action on the last recipient only verifies that byDocId and
sharings are properly cleaned up, but does not verify that sharedPaths are also
cleaned up when a non-drive sharing is fully revoked. Enhance the test by first
adding a path to the sharing using addSharing (similar to the pattern shown in
the shared drive test at lines 196-263), then after revoking the last recipient
with revokeRecipient, add an assertion to verify that state.sharedPaths no
longer contains the path that was previously added. This ensures complete
coverage of the cleanup behavior defined in the sharedPaths reducer at lines
290-295 in state.js.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@packages/cozy-sharing/src/state.spec.js`:
- Around line 346-360: The test for the revokeRecipient action on the last
recipient only verifies that byDocId and sharings are properly cleaned up, but
does not verify that sharedPaths are also cleaned up when a non-drive sharing is
fully revoked. Enhance the test by first adding a path to the sharing using
addSharing (similar to the pattern shown in the shared drive test at lines
196-263), then after revoking the last recipient with revokeRecipient, add an
assertion to verify that state.sharedPaths no longer contains the path that was
previously added. This ensures complete coverage of the cleanup behavior defined
in the sharedPaths reducer at lines 290-295 in state.js.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5f42e4ed-4889-4f6b-9863-f57c954037f1

📥 Commits

Reviewing files that changed from the base of the PR and between a2e29d6 and 7cfa3a9.

📒 Files selected for processing (2)
  • packages/cozy-sharing/src/state.js
  • packages/cozy-sharing/src/state.spec.js

@shepilov shepilov merged commit e0f486c into master Jun 16, 2026
3 checks passed
@shepilov shepilov deleted the fix/remove-shareing-after-last-recipient-removed branch June 16, 2026 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants