ποΈ PUT-1726 + PUT-1727 + PUT-1729: share with a workspace - #3729
Conversation
Coverage Report
File Coverage |
21438c3 to
2f5069d
Compare
2f5069d to
c939ee3
Compare
c939ee3 to
c50c04a
Compare
c50c04a to
d87ff02
Compare
d87ff02 to
c5e7af1
Compare
c5e7af1 to
6ddbe61
Compare
|
Warning Superseded β do not rely on the results below. Captured at Kept for history rather than deleted, so the comments that reference it still make sense. A fresh pass will be posted against the merged code. Local validationEngine: mysql 8 Β· real file, real share, real read β end to end over HTTP. Setup. Owner writes an actual file: Before sharing, a seat cannot see it: Share with the workspace by uid: A seat then reads the actual content: A non-member still cannot: The share row uses the Revoke takes effect immediately, no restart: One small thing for review: the response labels the recipient as an empty const label = recipient.email ?? recipient.username ?? '';with no branch for |
64a185b to
f2f661a
Compare
Fixed: team shares reported an empty recipientFollowing up on the observation in the validation comment above.
const label = recipient.email ?? recipient.username ?? '';Both now go through one helper: /** Echoes back the identifier the caller named, so results are matchable. */
#recipientLabel(recipient: ShareRecipient): string {
return (
recipient.email ??
recipient.username ??
recipient.teamHandle ??
recipient.team ??
''
);
}Echoing the identifier the caller supplied rather than resolving a display name:
Two tests, both falsified. Added to Reverting the helper to the old expression fails exactly those two: Full suite 7525 passed | 26 skipped, typecheck clean. One related thing left alone deliberately. I did not do that here because it adds a field to the share response shape, which |
f2f661a to
1146079
Compare
1146079 to
deba782
Compare
Review fixesA code review across the three phase 4 PRs raised 13 issues. All 13 are fixed; eight land here. The root cause behind four of themThe codebase had an invariant β
Each now checks both columns. Three further store predicates use Sharing with two workspaces shared with one
β Note for review: asserting on the response cannot catch this. Unsharing aborted when a delegate had lost authority
Now mirrors the user-to-user path: authority is the caller's throughout, with Members got no live events, and grants outlived deleted files
Also
Tests β all falsifiedEach fix was reverted, the test confirmed failing, then restored: β Two of these were vacuous on the first attempt and are worth knowing about, because the same trap applies to anything testing this area:
Full suite 7732 passed | 26 skipped, typecheck clean. Rebased onto current |
deba782 to
fd23663
Compare
Local validation (post-review)Engine: mysql 8 Β· Fixture: one workspace, three provisioned seats, all activated and email-confirmed, plus a non-member. Three is the minimum that distinguishes "told the members" from "told a member". The migrations replayed from a pre-teams schemaThe snapshot predates The listing union β every member sees itThis is what the earlier pass never covered: it tested Before sharing, The owner's outbound listing (findings 3 and 12)
The share dialog (finding 4) β and a gap the live run caught
Before the review it showed a wrong row; after, it showed none. Both are wrong β the item's manager could not see it was shared with a workspace, so could not revoke it there. That is also PUT-1762's "Done when". Added Test This was only found by listing live. No unit test covered the dialog for a workspace share. Sharing names the workspace, not an empty stringBoth the recipient label and Revocation clears every member's listing, no restartSame process, immediately after. A stale scan cache or a surviving index row would both show here. Full suite 7733 passed | 26 skipped, typecheck clean, on a |
fd23663 to
bb313c0
Compare
SDK bridge for workspace sharesFollow-up: the backend accepted Outbound β the recipient was silently dropped
...(record.team ? { team: String(record.team) } : {}),
...(record.teamHandle ? { teamHandle: String(record.teamHandle) } : {}),Object form only β no bare-string spelling. A Inbound β the workspace had no name
...(row.holder_team || row.holderTeam
? { holderTeam: row.holder_team ?? row.holderTeam }
: {}),Published types
Tests β falsified in both directionsDropping the outbound mapping fails the first two; dropping the inbound one fails the third; neither affects the other. Backend suite 7733 passed | 26 skipped, typecheck clean. This unblocks PUT-1762 (share with a workspace from the share dialog), which is written against |
Coverage Report for puter.js SDK
File Coverage
|
||||||||||||||||||||||||||||||||||||||
bb313c0 to
4792bb4
Compare
4792bb4 to
da78107
Compare
da78107 to
425f166
Compare
|
Rebased onto One collision worth naming: those commits took migration Verified per commit rather than at the tip β each of the 9 typechecks on its own, so no intermediate commit is broken. Full suite 7883 passed | 26 skipped. |
425f166 to
54628c2
Compare
Salazareo
left a comment
There was a problem hiding this comment.
code looks good but is this broadcasting to whole team?
54628c2 to
91f57d0
Compare
Terminology: "workspace" is now "team", everywhereThe feature said Applied to every commit rather than added on top, so each PR shows its own code already named correctly and no intermediate commit references a name a later one changes. All 13 commits still typecheck individually. The contract-bearing renamesThese were free to change now and expensive after release, which is why they went in this pass:
Plus Scoped, and checked"workspace" means other things in this repo. These were deliberately left alone, and verified untouched:
Audited three ways: every changed line mentions team or workspace (no collateral edits), no stutters or mangled words ( One thing not renamedThe git branch names still say |
91f57d0 to
000a4bd
Compare
Yes β a share to a team notifies every member, and that is the intent: the grant reaches all of them, so telling only some would misrepresent what happened. It is bounded rather than unbounded. Two things keep the volume down in practice:
β Worth flagging while you are looking at it: the cap is silent. A team of 600 gets 500 notifications and no signal that 100 were skipped. That is the behaviour today; if you would rather it logged or paged, say so and I will add it. |
000a4bd to
d814a18
Compare
Covers PUT-1726, PUT-1727 and PUT-1729. Together because a recipient without the listing work produces a share that is created correctly, resolves correctly, and never appears in "Shared with me" β a state nobody would ship. The recipient. `ShareRecipient` gains `team` (uid) and `teamHandle`, resolved before the email and username branches and never falling through to them. Passing both is an error rather than a precedence rule, so a call site always shows which was chosen: handles are released on soft delete and can be reclaimed by an unrelated team, and a scripted share to a handle would silently retarget. There is no bare-string spelling, since that would change how existing strings are interpreted. ACLService.setUserGroup mirrors setUserUser: same read-modify-write, same one-mode-per-node rule, under a node lock keyed on the group. One grant against the team, not one per member, so membership changes apply without touching the grant and a share spends one unit of the daily quota however many members there are. A member who joins afterwards gets access, which is asserted. The index row carries `holder_group_id` and leaves `holder_user_id` NULL, so `0077`'s group index constrains it rather than the user-holder one. Listing. `listByHolder` and `countByHolder` union the caller's teams into the same keyset page β `ORDER BY id` still holds β and `#grantEvidence` gains group grants as a third source. Without that third source the share is filtered out of every listing as dead: nothing errors, the share simply is not there, which is the one place a user would look for it. Unsharing revokes the grant as well as deleting the index row. Deleting the row alone would hide the share while leaving every member holding real access. Closes PUT-1726, PUT-1727 and PUT-1729.
d814a18 to
6dd8ac1
Compare
Covers PUT-1726, PUT-1727 and PUT-1729. Together because a recipient without the listing work produces a share that is created correctly, resolves correctly, and never appears in "Shared with me" β a state nobody would ship, and therefore not one worth reviewing on its own.
This is the PR that makes
puter.fs.share('/Docs', { team: uid }, 'write')actually work end to end.The recipient
ShareRecipientgainsteam(uid) andteamHandle, resolved before the email and username branches and never falling through to them.Passing both is a 400 rather than a precedence rule, so a call site always shows which was used. That is the whole reason they are separate fields: a handle is released on soft delete and can be reclaimed by an unrelated workspace, so a scripted share to a handle keeps working and silently starts sharing with strangers.
getByHandlefilters soft-deleted, so a released handle 404s until someone reclaims it.There is no bare-string spelling.
toShareRecipientsreads a bare string as an email when it contains@and a username otherwise, so any prefix convention would change how an existing string is interpreted β a breaking change to a released SDK. There is a test that a workspace handle passed as ausernamedoes not resolve to the workspace.One grant, not one per member
ACLService.setUserGroupmirrorssetUserUser: same read-modify-write, same one-mode-per-node rule, under a node lock keyed on the group.The grant is a single row against the workspace, so:
isNew: false.The index row carries
holder_group_idand leavesholder_user_idNULL, so0078's group index constrains it rather than the user-holder one.β The listing is the silent failure
#grantEvidencegains group grants as a third source alongside the linked and flat reads. Without it, a team share is created correctly, resolves correctly for every member, and is then filtered out of every listing as dead β nothing errors, the share is simply absent from the one place a user would look.listByHolderandcountByHolderunion the caller's workspaces into the same keyset page.ORDER BY idstill holds, so pagination is unaffected by which kind of holder a row names. The count matters too, orincludeTotalundercounts a member's workspace shares.Unsharing revokes the grant, not just the row
Deleting the index row alone would hide the share while leaving every member holding real access β worse than a listing bug, because it looks fixed. An owner may clear any issuer's grant of their node; anyone else only their own, as for user shares.
Verification
Per-site falsification
unshare revokes the grantinitially reported 0 failed, and the test was at fault: it asserted only that the share left the listing, which also happens when the index row is deleted. So a version that dropped the row without revoking the permission passed β the exact silent-retained-access bug worth catching. It now asserts the grant itself is gone.Not here
Member notifications (PUT-1761). As things stand, sharing with a workspace tells nobody:
notifySharedkeys offResolvedShare.holderId, a single user id, and a team share has none.holderGroupIdandholderTeamare populated on the resolved share so that ticket has what it needs.The GUI (PUT-1762) is what makes this reachable without writing code.
Closes PUT-1726, PUT-1727 and PUT-1729.