Repository navigation
fix(auth): useIsWorkspaceAdmin reads the session's published positions, restoring platform-admin detection - #5430
Merged
Conversation
…#5389) Leg 3 read `user.roles`, a key the protocol-17 session face no longer emits (framework ADR-0090 D3 renamed it to `positions`). A platform admin derived from the `admin_full_access` permission set has no member row (leg 1) and no admin `user.role` scalar (leg 2), so all three legs missed and Setup/Studio disappeared for a real administrator. Measured on a live single-tenant server at published 17.1.0 before fixing; the captured payload is the test fixture.
…t behind The `roles?: string[]` mirror key on the client `AuthUser` cannot be deleted yet — `AuthGuard`'s `requiredRoles` check still compiles against it (measured: TS2339 once the key is gone, because the index signature then types the read as `unknown`). So it keeps its declaration and gains a docblock that says what it actually is, pointing at the follow-up card filed as #5424 for the four remaining read sites. Also corrects two comments in app-shell's Home page that described `useIsWorkspaceAdmin` as reading ROLES. Comment-only; the EE workspace owner they describe holds `org_owner`, which is in `positions` too, so the ruling those comments record is unchanged. Adds the changeset.
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5389
Local gate union re-run on the final commit
8623e13ea— see "Verification" below. Nothing here waits on CI; the PM reads the real gate conclusions after this lands as a draft.The card said there was no runtime reproduction. There is one now.
The dispatch order made measuring the first step, so before touching anything I booted a real single-tenant server on the published
@objectstack/cli/@objectstack/plugin-auth17.1.0 (objectstack dev --seed-admin --fresh, banner reportsTenancy: single, no organizations plugin among the 30 loaded) and built the exact deployment shape the card names: a platform administrator whose adminship comes from theadmin_full_accesspermission set (asys_user_permission_setrow withorganization_id = NULL, whatbootstrapPlatformAdminseeds), with nosys_memberrow and no admin scalar on the user row.GET /api/v1/auth/get-sessionanswered:{ "user": { "id": "gwDZFnqdUQGFnOUh3IRWdWqykCw33eVl", "email": "psadmin@example.com", "role": "user", "positions": ["user", "platform_admin"], "isPlatformAdmin": true }, "session": { "activeOrganizationId": null } }Object.prototype.hasOwnProperty.call(user, 'roles')on that payload answers false, andorganization/get-active-memberanswers400 NO_ACTIVE_ORGANIZATION. So all three legs of the hook missed:activeMember.roleuser.role"user"user.rolesThe hook returned
falsefor a platform administrator: Setup and Studio vanish, the marketplace gates shut, the "Build an app" CTAs disappear. The premise holds and the defect is real. The captured payload is now the test fixture, verbatim.The producer half is confirmed at the artifact too, not just at the wire:
@objectstack/plugin-auth@17.1.0's bundledcustomSessionreturns exactlyuser: { ...user, positions, isPlatformAdmin: platformAdmin, ...authGate ? { authGate } : {} }, and noroleskey appears anywhere in its emission path.The fix
Leg 3 now reads
user.positions[]— the one spelling the session publishes, and one this repository does not hand-copy: it is inherited from the spec's ownAuthUserthroughAuthUser extends SpecAuthUser, so it cannot drift the way the mirror key did.Deliberately not done, per the card and AGENTS.md commandment #0.1:
roles ?? positionsdual spelling. ADR-0090 D3 renamed with no deprecation window and bans reviving the old name as a fallback. A negative case pins this: a session carrying only the retired spelling reads as not an administrator. That case failed before the fix (the old leg 3 honoured it) and passes now.isPlatformAdminis not read alongsidepositions. The server computes it as'platform_admin' in positionsin the same expression, so the two cannot usefully disagree — reading both would be two spellings for one fact, andpositionsadditionally covers theorg_owner/org_adminnames that the boolean does not.Nobody new becomes an administrator
This is an authorization-adjacent read, and the positive case alone cannot distinguish a restored detection from a widened one — "return
truemore often" also makes it green. Four negative cases exist to make that substitution fail:positions: ["user", "org_member"], member row with rolemember) reads as not admin through every leg;positionsarray admits nobody;positionskey at all admits nobody;rolesspelling alone confers nothing.And the two legs that were never broken get cases of their own, so a swap of one dead read for one live read cannot disturb them unnoticed: an active member row with an admin role still detects, a stored
user.roleadmin scalar still detects, and preview mode still seeds an admin.Worth stating because it is the reason nothing widened:
positionsat protocol 17 carries exactly whatrolescarried at 16 — the stored scalar split on commas, the active membership mapped to canonicalorg_owner/org_admin/org_member, andplatform_adminwhen the permission set is held. This is a rename, so restoring the read restores the pre-rename semantics rather than adding to them.Reverse verification (predicted before running, both directions)
Written test-first. Predicted 2 failures on the pre-fix tree: the platform-admin case (
trueexpected,falseobserved) and the dead-spelling pin (falseexpected,trueobserved, because leg 3 still honouredroles). Observed exactly those two, in exactly those directions, 7 others passing:After the fix:
Tests 9 passed (9).No build artifact sits between the edit and the thing under test, on any leg. The root
vitest.config.mtsaliases every@object-ui/*specifier to that package'ssrc/, andpackages/auth/srchas no runtime workspace import at all — its only cross-package dependency is a type-only import from@objectstack/spec, erased before execution. So the red and the green were both read off source, and no ablation needed a rebuild to be trustworthy.The one thing the card asked for that could not be done
The card proposed retiring the dead
roles?: string[]mirror key on the clientAuthUserin the same pass. It is not dead. Removing it fails compilation:AuthGuard'srequiredRolescheck still reads it, and once the declaration is goneAuthUser's index signature types the read asunknown. That is a live reader, not a leftover, so the key keeps its declaration and gains a docblock stating what it actually is — absent from the v17 session face, kept for one compile-time reader, and not to be paired withpositionsas a fallback.Chasing it further would have meant editing an authorization gate the card explicitly scoped out, and
requiredRolesis not a mechanical rename anyway (whether it should readpositions, whether it keeps the scalar fallback, and how it relates topackages/permissions' separaterolessurface are open questions). Filed separately as #5424 together with three other surviving read sites, which is where that decision belongs. Out of scope for this PR; #5424 stays open.Also in this PR, and why each is in scope rather than filed
Both are comment-only, in the same defect class (documentation naming the removed spelling), mechanically determined by the rename, and inside the same gate family — no new verification surface:
packages/app-shell/src/console/home/HomePage.tsx— the "useIsWorkspaceAdminreads ROLES" comment named in the card. The ruling it records is unchanged: the EE workspace owner it describes holdsorg_owner, which is inpositionstoo, so the sentence needed a spelling correction and not a semantic one.packages/app-shell/src/console/home/__tests__/HomePage.authoringCapabilityGate.test.tsx— carries the same sentence, one file over. Correcting one and leaving the other would have left the card's own stale claim standing in the file that pins the behaviour. Named here because a drive-by fix nobody points at is unreviewable scope creep; this one is pointed at.Attribution correction for the record: the card locates that comment at
apps/console/.../HomePage.tsx:82; it actually lives atpackages/app-shell/src/console/home/HomePage.tsx:82.Verification
All re-run against the final commit
8623e13eawith a clean tree, so these results describe the tree being reviewed:vitest run packages/auth/ packages/app-shell/src/console/home/__tests__/HomePage.authoringCapabilityGate.test.tsxpnpm --filter @object-ui/auth type-check(tsc --noEmit+tsc -p tsconfig.test.json)eslinton all 5 changed filesno-explicit-anywarnings on untouched lines)check-changeset-presence.mjscheck-changeset-no-major.mjspatchcheck-control-bytes.mjscheck-node-esm-load.mjs --specifiers-onlycheck-package-self-import.mjsTest invocation was from the repository root with repo-relative paths and no
--separator, per AGENTS.md. Heavy steps ran under the shared/tmp/os-heavy-verify.lock.The full lint/CI farm is left to CI, which runs it exactly once regardless.
Landing
Draft, and auto-merge is deliberately not enabled — the PM lands this.
target:v17, release-blocking.Generated by Claude Code