feat: app-scoped share listing and revoke surface (PUT-1670) - #3695
feat: app-scoped share listing and revoke surface (PUT-1670)#3695Salazareo wants to merge 2 commits into
Conversation
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
jfcastro92
left a comment
jfcastro92
left a comment
There was a problem hiding this comment.
Took a close look at this. Most of what I found traces back to one decision: data.issuedByApp is a mutable JSON field, and this PR makes it the authority for listing scope and DELETE authorization. The upsert dedupe paths clobber or freeze it, rows already on main store it under the old issuerAppUid key, and unshare doesn't honor the app scoping the new DELETE promises. Details inline — the last comment on ShareStore.js sketches the schema-level fix (a real indexed column + backfill) that would take out several of these at once.
| : { email: row.recipient_email }; | ||
| if (!recipient.username && !recipient.email) throw notFound(); | ||
|
|
||
| return this.unshare(actor, { uid: entry.uuid, recipient }); |
There was a problem hiding this comment.
The app-scope check above only gates which row we find — once we get here, unshare re-targets by (entry, recipient), and for an owner its isOwner branch collects every issuer's rows for that pair, with userRelatedActor stripping the app scope before #revokeFor runs. So an app token deleting its own row can also take out a grant a manage-delegate issued for the same file+recipient, and the response reports the inflated count. Same shape in #cancelInvite: an app-scoped delete of one pending invite removes every issuer's invite for that email+entry. That contradicts the scoping this surface documents. None of the new tests put two issuers on the same (file, recipient) pair, which is why it doesn't show up.
| * @param {string | null | undefined} appUid | ||
| * @param {string} dataColumn | ||
| */ | ||
| #appFilter(appUid, dataColumn) { |
There was a problem hiding this comment.
Now that this filter makes data.issuedByApp the thing deciding listing scope and DELETE authorization, the upserts become a problem: upsertActive dedupes on (holder, fsentry, issuer) and overwrites data wholesale on conflict. Share via app A, then re-share the same file to the same person from the browser, and the field is gone — app A's listing drops the grant and its DELETE starts 404ing on a grant it issued. In the other order, the manual grant gets stamped with app A, leaves the none group, gets counted under A in /shared-by-me/apps, and A's token can revoke it. The new tests give each app its own file, so neither direction is exercised.
| ...(issuerAppUid ? { issuerAppUid } : {}), | ||
| // Same key an active share records it under, so one filter | ||
| // covers an invite and the grant it becomes. | ||
| ...(issuerAppUid ? { issuedByApp: issuerAppUid } : {}), |
There was a problem hiding this comment.
This renames the key from issuerAppUid with no migration and no dual-read, and the old key already shipped on main (2c852bf, b795b21). Any app-issued invite already in the table gets read as "no app": the app's scoped listing won't show it and its DELETE 404s, so the app can never withdraw its own outstanding invite, while the user's none group claims it as self-made. applyPending copies data forward verbatim, so the misattribution survives into the active grant once the invite is claimed. Either read both keys or backfill.
| @@ -372,7 +451,9 @@ export class ShareStore extends PuterStore { | |||
| fsentryId, | |||
| mode, | |||
| JSON.stringify({ | |||
There was a problem hiding this comment.
Related: this only runs on the insert branch. The dedupe branch up top (keyed on email+fsentry+issuer, no app) updates mode and never touches data, so issuedByApp stays whatever the first invite wrote. App invites someone, user later re-invites the same address by hand — the row still says the app issued it, the user's invite is missing from the none group, and the app token can cancel it via DELETE. In the other order, the app never sees the invite it just created. upsertActive does refresh data on conflict, so the pending path is the odd one out.
| */ | ||
| #appUidFilter(query: Record<string, unknown>): string | null | undefined { | ||
| const value = query.appUid; | ||
| if (typeof value !== 'string' || value === '') return undefined; |
There was a problem hiding this comment.
This fails open: a duplicated query param (?appUid=X&appUid=X — Express hands you an array) or an empty string silently becomes undefined, i.e. the unscoped all-apps listing, while the caller believes it's filtered. normalizeLimit and decodeCursor both 400 on malformed input; this should too. Also, a client that round-trips group.appUid from the /apps summary gets null for the no-app group, and null isn't 'none', so it silently receives every share instead of the advertised count. (App tokens are clamped server-side, so this is about user sessions.)
| } | ||
| if (!(await this.#hasOwnReach(actor, entry, 'see'))) throw notFound(); | ||
|
|
||
| const holder = row.holder_user_id |
There was a problem hiding this comment.
If the invitee has signed up and confirmed by now but the claim never ran (claimPendingShares is fire-and-forget and swallows per-row errors, and an invite inserted after its snapshot is never claimed), this resolves the email to a real user — and unshare's user branch only deletes rows matching holder_user_id, never the pending row. The invite stays in the listing (pending rows skip the stillReaches filter) and DELETE returns 200 {revoked: 0} forever. The new test uses a never-registered address, so only the clean cancel-invite path is covered.
| ) { | ||
| throw notFound(); | ||
| } | ||
| if (!(await this.#hasOwnReach(actor, entry, 'see'))) throw notFound(); |
There was a problem hiding this comment.
Minor one: the docblock promises everything the caller may not act on 404s alike, but there are a couple of edge cases where the gates past this point answer differently — a delegate who issued a re-share and later got downgraded to read still passes this check (see) and then gets a 403 from unshare's #assertCanManage, a row the listing hides answers 200 {revoked: 0}, and the pending path can 400 (cannot_revoke_owner). Probably fine in practice — I'd just soften the docstring so it doesn't promise a uniformity the code doesn't quite hold.
|
|
||
| /** The app this credential acts as, or null for a plain user session. */ | ||
| #actingAppUid(actor: Actor): string | null { | ||
| return (actor.effectiveApp ?? actor.app)?.uid ?? null; |
There was a problem hiding this comment.
The ?? actor.app re-creates the derivation core/actor.ts says lives in one place — "a gate must not read the second as the first", with makeActor as the only spot that derives it. Every other consumer (KVStoreDriver, SubdomainDriver, SystemKVStore, FSController, gates.ts) reads effectiveApp bare, and this PR deleted the last actor literal the fallback would have defended against, so the fallback arm is dead code today — but it's a second derivation site someone will copy. actor.effectiveApp?.uid ?? null does the job. Also worth flagging that routing the write path at 868/2138 through this helper quietly changes what it records, from actor.app to the effective app.
| * whatever it was handed and legacy rows carry plain strings — extracting | ||
| * from one of those aborts the whole query, so there the read is guarded. | ||
| */ | ||
| #issuedByAppExpr(dataColumn) { |
There was a problem hiding this comment.
Bigger picture: filtering and grouping on a JSON extract means every one of these queries computes json_valid + json_extract per row with no possible index — listOutboundApps runs the UNION ALL scan with the extract as its GROUP/ORDER key, and countOutboundApps repeats the identical scan when includeTotal is set. Elsewhere we denormalize the app onto a real column (sessions.app_uid + index, app_feedback.app_uid, fsentries.associated_app_id). A real issuer_app_uid column with a backfill migration would also have forced the answer to the legacy-key problem above, and takes the upsert-clobbering issues with it. Not a bug — but the cost is O(user's outbound rows) per page rather than an index seek.
84783a7 to
a8fb64f
Compare
Coverage Report for puter.js SDK
File Coverage
|
||||||||||||||||||||||||||||||||||||||
- Scope the uid-addressed revoke to the named row: only that row's issuer's grant is withdrawn, and only that one invite cancelled — an app or owner addressing one row no longer takes another issuer's grant on the same (item, recipient) pair with it. - Delete a pending row directly on uid-addressed revoke, so an invite whose address registered but never claimed can still be withdrawn. - Read the legacy `issuerAppUid` data key in the SQL app filter and grouping, alongside the unified `issuedByApp`. - Refuse malformed `appUid` input (duplicated param, empty string) instead of silently listing everything, and refuse app-listing cursors that decode but name no appUid. - Derive the acting app from `effectiveApp` alone, per the actor contract; drop the second derivation site. - Pin the attribution semantics with tests: one row records one issuance, so re-sharing the same pair re-attributes it to whoever issued last, in both directions. - Soften the uniform-404 docblocks to what the gates actually answer.
e1cad19 to
7de955a
Compare
2 participants
Stacked on
DS/put-1664— review only this PR's diff; merges bottom-up.