diff --git a/.changeset/marketplace-detail-admin-guard-order-5583.md b/.changeset/marketplace-detail-admin-guard-order-5583.md new file mode 100644 index 000000000..b50d52248 --- /dev/null +++ b/.changeset/marketplace-detail-admin-guard-order-5583.md @@ -0,0 +1,39 @@ +--- +'@object-ui/app-shell': patch +--- + +The marketplace package detail page decides "you are not an admin" before it fetches, +instead of after the load has already failed. + +`MarketplacePackagePage` ordered its early returns with the `!isAdmin` guard *after* +both the loading branch and the `error || !data` branch, and gated its two fetch +effects on `features.marketplace` alone. On a runtime that mounts a marketplace, a +non-admin who opened a package URL was therefore walked through the fetch and the +skeleton, and — when the load failed — was handed the destructive "Failed to load +package" card carrying the server's own error message. Whether that viewer was +refused or handed a diagnosis about a surface they are not allowed to use came down +to whether an unrelated request happened to succeed. + +The guard now sits ahead of both branches, and `getMarketplacePackage` and +`getCloudInstallationInfo` are gated on `isAdmin` as well, so the page stops issuing +requests on behalf of a viewer it has already decided to turn away. That is the +discipline objectui#5533 established on this same page for `features.marketplace`, +applied to the other predicate that decides the same thing. It is also the ordering +`MarketplacePage` carries after objectui#5557, so the two sibling pages now answer one +runtime the same way for every viewer. The server remains the authority on what a +non-admin may fetch; this only stops the client doing work it would discard. + +Unchanged for an admin, deliberately and under test: a failing load still produces the +destructive card with the server's message intact, and a successful one still renders +the package. A "fix" that hoisted the refusal unconditionally, or that deleted the +failure branch, would satisfy every non-admin assertion and fail those two. + +`loading` stays seeded from `marketplaceEnabled` alone rather than from +`marketplaceEnabled && isAdmin`. `isAdmin` reads `activeMember`, which `AuthProvider` +resolves asynchronously *after* the session settles, so an admin whose role comes from +the org member row renders once as a non-admin before the flag flips. Seeding `false` +there would leave that first admin render with `loading: false` and no data — the +destructive card, painted for a frame before the effect could raise the flag again. +`MarketplacePackagePage.guardOrder.test.tsx` pins the flip case for that reason, along +with the ordering, the skipped requests, and the marketplace-off boundary the guard +must not jump above. diff --git a/packages/app-shell/src/console/marketplace/MarketplacePackagePage.tsx b/packages/app-shell/src/console/marketplace/MarketplacePackagePage.tsx index d02263420..132bf8f97 100644 --- a/packages/app-shell/src/console/marketplace/MarketplacePackagePage.tsx +++ b/packages/app-shell/src/console/marketplace/MarketplacePackagePage.tsx @@ -98,10 +98,21 @@ export function MarketplacePackagePage() { const marketplaceEnabled = isMarketplaceEnabled(); const [data, setData] = useState(null); - // Seeded from the flag rather than settled by the effect: a runtime with no - // marketplace is not "loading" a package, it is done. Keeps the state + // Seeded from the runtime flag rather than settled by the effect: a runtime + // with no marketplace is not "loading" a package, it is done. Keeps the state // truthful even if the early return below is ever reordered -- a seeded // `true` with the fetch skipped would spin forever. + // + // Deliberately NOT `&& isAdmin`, even though objectui#5583 gates both fetches + // on `isAdmin` as well. `isAdmin` reads `activeMember`, which AuthProvider + // resolves asynchronously AFTER the session settles (`refreshActiveMember`), + // so an admin whose adminship comes from the org member row renders once as a + // non-admin before flipping. Seeding `false` there would leave that first + // admin render with `loading: false` and no data -- i.e. the destructive + // "failed to load" card, painted for a frame before the effect could raise + // the flag again. `true` means "this runtime has a marketplace, so a package + // answer is expected and none has arrived yet"; whether THIS viewer may see + // it is the separate question answered by the guard above the branch. const [loading, setLoading] = useState(marketplaceEnabled); const [error, setError] = useState(null); @@ -162,6 +173,10 @@ export function MarketplacePackagePage() { // are absent on this runtime too -- the probe would be one more guaranteed // 404 in the operator's network log. if (!marketplaceEnabled) return; + // The same argument one step further (objectui#5583): this viewer is + // refused before any CTA renders, so seeding one is work fired on behalf of + // a page we have already decided not to draw. + if (!isAdmin) return; const currentEnvId = getRuntimeConfig().defaultEnvironmentId ?? ''; let cancelled = false; (async () => { @@ -171,7 +186,7 @@ export function MarketplacePackagePage() { setCloudInstalledVersion(info.version); })(); return () => { cancelled = true; }; - }, [packageId, marketplaceEnabled]); + }, [packageId, marketplaceEnabled, isAdmin]); useEffect(() => { let cancelled = false; @@ -182,6 +197,10 @@ export function MarketplacePackagePage() { // race the destructive card onto the screen before the disabled state // settles (objectui#5533). if (!marketplaceEnabled) return; + // Nor on behalf of a viewer this page refuses (objectui#5583). + // Authorization is not a function of whether the fetch succeeded, so it + // is settled before the request rather than after it. + if (!isAdmin) return; setLoading(true); setError(null); try { @@ -194,7 +213,7 @@ export function MarketplacePackagePage() { } })(); return () => { cancelled = true; }; - }, [packageId, marketplaceEnabled]); + }, [packageId, marketplaceEnabled, isAdmin]); const openInstall = async () => { setInstallOpen(true); @@ -518,6 +537,24 @@ export function MarketplacePackagePage() { // that exists for nobody is the same misdirection this fix removes. if (!marketplaceEnabled) return ; + // Ahead of BOTH the loading and the load-failure branches below + // (objectui#5583): authorization is not a function of whether the fetch + // succeeded. Sitting behind them, this guard handed a non-admin whose package + // failed to load the destructive "Failed to load package" card carrying the + // server's own error message -- a diagnosis about a surface they are not + // allowed to use -- and reached the refusal only on the paths where the load + // happened to work. Both fetch effects above are gated on the same predicate, + // so the refusal also stops the page requesting on behalf of a viewer it has + // already decided to turn away: the discipline objectui#5533 established on + // this page for `features.marketplace`, applied to the other predicate that + // decides the same thing. + // + // The server remains the authority on what a non-admin may fetch; this only + // stops the client doing work it would throw away. It is also the ordering + // `MarketplacePage` carries after objectui#5557, so the sibling pages now + // answer one runtime the same way for every viewer. + if (!isAdmin) return ; + if (loading) { return (
@@ -590,8 +627,6 @@ export function MarketplacePackagePage() { ? t(`marketplace.category.${pkg.category}` as any, { defaultValue: pkg.category }) : null; - if (!isAdmin) return ; - return (