feat: global outbound share listing (PUT-1664) - #3694
Conversation
Coverage Report
File Coverage |
There was a problem hiding this comment.
Read through this carefully. Shape of the endpoint is good and the keyset merge in listOutbound is correct as far as I can tell. Comments inline — the first is the one I would fix before shipping; a few further down are pre-existing things this endpoint just surfaces more widely, and I say so where that applies.
Two things I checked and am not flagging: the total from countOutbound is fine (doc/pagination.md already defines totals this way, and the inbound listing does the same), and app credentials get no more here than from /share/shares — #assertCanManage applies the same see check.
| const entry = entries.get(Number(row.fsentry_id)); | ||
| if (!entry) continue; | ||
| if (!reachable.has(entry.uuid)) continue; | ||
| const pending = !row.holder_user_id; |
There was a problem hiding this comment.
I would hold the PR on this one. Pending rows skip the stillReaches check by construction, and nothing else catches them: #revokeDownstream goes via listByFsentrySubtree (filters holder_user_id IS NOT NULL) and deleteActive is holder-keyed, so revoking a manage delegate never deletes the invites they sent. #reachableBy is a no-op for plain sessions, so there is no caller-side gate left.
So after the owner revokes D, D can still call this forever and read the file's current name, size, mtime and a fresh signed thumbnail URL. listSharesOf and listSharedWithMe both gate the caller; this path does not. Cleanest fix is probably deleting pending invites when their issuer's grant goes away, since those orphans are a problem either way.
| const pending = !row.holder_user_id; | ||
| if ( | ||
| !pending && | ||
| !stillReaches.has(`${Number(row.holder_user_id)}:${entry.id}`) |
There was a problem hiding this comment.
stillReaches is keyed holder:entry, so it answers "can H still reach N through anyone", not "is my grant to H still alive". If O and D both shared N with H and O's permission row goes without the share row being deleted (onGrantRevoked bails early when issuerId is not a number), D's grant keeps the pair live and O sees a dead share they cannot clear.
#liveGrants already has issuer_user_id on hand (#hasGrantFrom uses it). listSharesOf is equally coarse, so I would fix it inside #reachingHolders rather than here.
| } | ||
| : {}), | ||
| createdAt: row.created_at, | ||
| issuedByApp: issuedByApp(row), |
There was a problem hiding this comment.
Always null for pending rows: upsertPending writes { issuerAppUid } into data (ShareStore.js:375), but this reads data.issuedByApp, the key upsertActive uses. So an app-sent invite looks self-issued, which is the one case the field exists for.
Pre-existing (listSharesOf:1595 too), but this is where someone would audit what an app did in their name.
| // still be there, and an app sees only the part of it the credential | ||
| // reaches in its own right. | ||
| const [stillReaches, reachable] = await Promise.all([ | ||
| this.#reachingHolders( |
There was a problem hiding this comment.
Fan-out worth a look: #reachingHolders does one #liveGrants per distinct holder in an unbounded Promise.all, ~2 reads each. Inbound only ever had one holder, this can be 200 a page, and 600 req/min on share:list makes it cheap to trigger. Cold cache that is ~200 SELECTs queued through the batcher per request.
The flat-perm keys already embed the holder id, so one mget plus a holder_user_id IN (...) read would do a whole page in ~2.
| * which cannot answer it at all for a caller who doesn't know what to ask | ||
| * about. | ||
| */ | ||
| @Get('/shared-by-me', { |
There was a problem hiding this comment.
AGENTS.md sends anything apps can call to doc/contributing-apis.md, which says the PR is not done until all seven steps are, including the puter.js method, docs page and SDK suite. Nothing under src/puter-js or src/docs here.
Not backend-only by precedent either: /shared-with-me ships as puter.fs.listShared() with docs. As-is no app can reach this listing.
| @Get('/shared-by-me', { | ||
| subdomain: 'api', | ||
| requireVerified: true, | ||
| rateLimit: SHARE_LIST_LIMIT, |
There was a problem hiding this comment.
This joins the shared 600/min share:list bucket, but rate-limits-and-quotas.md:138 still lists only getShares/listShared, and AGENTS.md says a limit is not done until the docs are. Since the bucket is shared, polling this quietly eats the budget other share reads need.
| * which is the only place those are visible, since the permission tables | ||
| * are keyed issuer to holder. | ||
| * | ||
| * Two indexed reads rather than one `OR` spanning the join: the optimizer |
There was a problem hiding this comment.
The comment promises a plan the migrations do not back. SQLite has no issuer index at all (0014 creates none; 0067 adds holder, fsentry and the unique triple), so the issued half walks the PK there.
Bigger one: the delegated half joins fsentries.user_id then orders by share.id, which no index gives us in any dialect, so each page re-sorts every share on every node the user owns. idx_share_fsentry only helps the join probe. Either add the composite issuer index or reword this.
| ) { | ||
| continue; | ||
| } | ||
| items.push({ |
There was a problem hiding this comment.
Fifth hand-built ResolvedShare in this file (inbound at 1307, three in listSharesOf), and they have drifted: inbound has name/type/thumbnail/owner but no issuedByApp/inheritedFrom, listSharesOf the reverse, this one the union. Same object, different shape per endpoint.
Something like #resolveShareRows(rows, { trash, grantCheck, pending }) would make the differences arguments instead of three near-identical loops.
| requireVerified: true, | ||
| rateLimit: SHARE_LIST_LIMIT, | ||
| }) | ||
| async listSharedByMe(req: Request, res: Response): Promise<void> { |
There was a problem hiding this comment.
listSharedWithMe above with one line changed. You added a test to keep the decorator options in sync, so the drift risk is clearly real, but nothing guards the parsing or response shaping. A small #listSharePage(req, res, list) used by both would cover all of it.
| } | ||
|
|
||
| /** The id a keyset page resumes after; 0 for the first page. */ | ||
| #afterId(cursor) { |
There was a problem hiding this comment.
Minor and pre-existing, but it now backs two endpoints. decodeCursor only 400s on unparseable input, so {}, {"id":"abc"} or another endpoint's cursor all decode and land here as 0, silently restarting from page 1 — a client that walked N pages re-ingests everything.
Same pattern is in AppDriver/FSEntryStore etc, so not a blocker; just cheaper to validate once here than in four places later.
Coverage Report for puter.js SDK
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
- Check share-row liveness per (holder, entry, issuer) so a grant withdrawn outside unshare doesn't stay listed while another issuer still reaches the same holder; batch the permission reads across the whole page instead of per holder. - Retire a revoked issuer's unclaimed invites in the revoke cascade, and hide invites whose issuer lost their authority at read time. - Unify the pending/active app-attribution key on `issuedByApp` and dual-read the legacy `issuerAppUid` spelling. - Add the missing share issuer index (sqlite, postgres) and correct the listOutbound plan comment. - Refuse cursors that decode but name no id instead of silently restarting from page one. - Consolidate the five hand-built ResolvedShare literals and the two listing endpoints' parse/shape code. - Ship the SDK surface: puter.fs.listSharedByMe() with docs, types, suite coverage, and the rate-limit page entry.
a63d92a to
bd2380d
Compare
2 participants
Bottom of a stacked-PR chain (targets
main).