feat(fabric-admin): Fabric Admin section with API token generator#1533
feat(fabric-admin): Fabric Admin section with API token generator#1533DavidCockerill wants to merge 4 commits into
Conversation
New top-level "Fabric Admin" area, gated to fabric admins / super users, built as an extensible section (layout + sub-nav rail) so future admin endpoints slot in as sub-pages. First page generates a short-lived API token for programmatic access. - Navbar: fabric-admin-gated "Fabric Admin" item (isAdminMode). - routes: fabricAdminLayoutRoute (under dashboardLayout) + api-token index, wired into rootRouteTree keeping getParentRoute/addChildren in lockstep. - FabricAdminShell: SubNavRail shell, render-gated via useCloudAuth/isAdminMode (dashboard guard still handles the unauthenticated redirect). - ApiToken page: generate button -> POST /Admin/ApiToken, shows the token (read-only field + copy) with expiry and a "copy it now" note. - ApiTokenResult type in api.patch.d.ts; endpoint isn't in generated types so the mutation casts the URL + response (like getCurrentUser). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a new Fabric Admin section, allowing administrators to generate short-lived API tokens for programmatic access. It includes the addition of a 'Fabric Admin' link in the navigation bar, a shell layout with sub-navigation, and the API token generation interface and mutation. Feedback on the changes suggests removing the local 'onError' handler from the token generation mutation to prevent duplicate error notifications, and correcting the 'variant="submit"' prop on the generate button to 'type="button"'.
| const onGenerate = () => { | ||
| mutate(undefined, { | ||
| onSuccess: (result) => setToken(result), | ||
| onError: (error) => toast.error('Failed to generate API token', { description: error.message }), | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Avoid adding local 'onError' handlers to individual mutations if a global error handler is already configured to handle and display error messages, to prevent duplicate error notifications (double-toasting).
const onGenerate = () => {
mutate(undefined, {
onSuccess: (result) => setToken(result),
});
};
References
- Do not add local
onErrorhandlers to individual mutations if a global error handler (such as a globalMutationCache.onErrorin React Query) is already configured to handle and display error messages, to prevent duplicate error notifications (double-toasting).
There was a problem hiding this comment.
Applied in b0c9a6d — confirmed the global MutationCache.onError in src/react-query/queryClient.ts already toasts mutation failures (and other components like ResendInviteButton document relying on it), so the local handler was indeed a double-toast. Removed.
— Claude Opus 4.8 (on behalf of @DavidCockerill)
| <code className="rounded bg-muted px-1 py-0.5 text-xs">Authorization: Bearer <token></code> | ||
| </p> | ||
|
|
||
| <Button className="mt-4" variant="submit" onClick={onGenerate} disabled={isPending}> |
There was a problem hiding this comment.
The 'variant' prop on the 'Button' component is typically used for visual styles (e.g., 'default', 'outline', 'ghost'). Using variant='submit' is likely a typo or an invalid variant. Since this button is not inside a form and uses an explicit 'onClick' handler, using type='button' is the standard approach.
| <Button className="mt-4" variant="submit" onClick={onGenerate} disabled={isPending}> | |
| <Button className="mt-4" type="button" onClick={onGenerate} disabled={isPending}> |
There was a problem hiding this comment.
Declining this one: submit is a defined variant in this repo — see src/components/ui/buttonVariants.tsx:31. It's the primary-CTA style used across the app, and it's why tsc accepts the prop (the variants are typed via cva, so an invalid variant would fail the build). The generic shadcn variant list doesn't apply here.
— Claude Opus 4.8 (on behalf of @DavidCockerill)
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Review feedback (Gemini): the global MutationCache.onError in react-query/queryClient already toasts mutation failures, so the local onError produced a duplicate toast. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kriszyp
left a comment
There was a problem hiding this comment.
I see there are comments about super user; I don't know if a super user is really a valid concern, if you manage to login as the super user, I assume you don't need a token. So I think this looks good.
| const { isLoading, user } = useCloudAuth(); | ||
|
|
||
| if (isLoading) { return null; } | ||
| if (!isAdminMode(user)) { return <Navigate to="/" replace />; } |
There was a problem hiding this comment.
Could we align this guard with the token endpoint's session contract? isAdminMode includes super_user, and CM intentionally permits password login for that role, but POST /Admin/ApiToken requires the OAuth-only session.ssoAuthAt marker. A password-authenticated super user therefore sees this page and can only receive a 403. Please either restrict the token page to the intended SSO-only role, expose the session auth method and offer an SSO re-auth path/message, or adjust the endpoint if these sessions are intended to mint tokens.
There was a problem hiding this comment.
Applied in 1b21e2a, taking your first option (and matching your review note that a super user doesn't need a token): a new isFabricAdmin helper gates the shell and the navbar item, so only fabric_admin accounts — the ones whose sessions can actually satisfy the endpoint's ssoAuthAt contract — see the section. A password-authenticated super user no longer gets shown a page that can only 403. If the section later grows tools that don't need SSO, we can widen per-page.
— Claude Opus 4.8 (on behalf of @DavidCockerill)
| const { mutate: signOut } = useLogoutMutation(); | ||
| const navigate = useNavigate(); | ||
| const { user } = useOverallAuth(); | ||
| const isAdmin = useAdminMode(); |
There was a problem hiding this comment.
Navbar already has user from useOverallAuth() on the preceding line, while useAdminMode() calls useCloudAuth() -> useOverallAuth() again. That adds another state/effect/authStore listener for every Navbar (raising the Navbar tree's existing three subscriptions to four, including for non-admin/local users), and the extra listener runs on every auth update. Could we import the pure isAdminMode helper and derive this from the existing user instead, with a cloud/local type guard if needed?
There was a problem hiding this comment.
Applied in 1b21e2a — Navbar now derives admin visibility from its existing useOverallAuth() subscription via the pure helper (no fourth authStore listener). isFabricAdmin accepts the User | LocalUser union with an internal 'fabricRole' in user narrow, so no type-guard boilerplate at the call site. (useAdminMode() had been introduced here to dodge exactly that union mismatch — your version is better.)
— Claude Opus 4.8 (on behalf of @DavidCockerill)
…ription Review feedback (Kris): isAdminMode includes super_user, but the token endpoint requires an SSO session only fabric_admin accounts have — a password-authenticated super user would see the page and only ever get 403. New isFabricAdmin helper (accepts the cloud/local user union) gates the shell and navbar item. Navbar now derives it from its existing useOverallAuth() subscription instead of useAdminMode(), which added a second authStore listener per Navbar. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
I can't verify this locally since fabric admins must use Google auth, even locally... pushing to dev, though I don't think I have a user there either... how did you verify locally, David? |
dawsontoth
left a comment
There was a problem hiding this comment.
Let's just call it Admin, drop Fabric from the name. We're already on Fabric, that's understood. And there's no Admin for anyone except us, so it's safe ambiguation of the term.
Review feedback (Dawson): we're already on Fabric and there's no admin area for anyone but us, so "Admin" is unambiguous. Renames the route (/fabric-admin -> /admin), nav label, section heading, feature dir (features/fabricAdmin -> features/admin), and FabricAdminShell -> AdminShell. The Harper role identifier (fabric_admin / fabricRole / isFabricAdmin) is unchanged — that's the actual role name, not the section label — and it now aligns with CM's existing /Admin namespace. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Done in 7446b78 — renamed to Admin throughout: route On verifying locally — you're right that you can't, and neither could I do a true end-to-end run: real Google SSO won't complete against remote CM from localhost (the OAuth callback lands on the dev domain). What I checked locally was appearance only — I seeded a fake admin user and stubbed the generate-token mutation for a pure-visual pass, then reverted it. The happy path (real SSO → mint) was never exercised locally. The actual unblock for "fabric admins must use Google even locally" is the CM side: central-manager#459 adds — Claude Opus 4.8 (on behalf of @DavidCockerill) |
kriszyp
left a comment
There was a problem hiding this comment.
Looks like there is an edge case to cover, but looks good.
Proposed inline comments (anchors failed):
src/features/fabricAdmin/apiToken/mutations/useGenerateApiToken.ts:17: Could we keep this bearer token out of the global mutation cache once the page stops using it? Eachmutatestores the successfulApiTokenResult; generating again or unmounting only makes the old entry inactive, and logout currently clearsQueryCachebut notMutationCache, so the credential remains readable throughqueryClient.getMutationCache()until GC. SettinggcTime: 0here would discard each token as soon as its observer is removed. If we want the token to live only inApiTokenIndexstate even while mounted, we can also call the mutation'sreset()after copying the success result into state.src/hooks/useAuth.ts:54: Could we pin the role boundary introduced in the follow-up commit? A small table test forisFabricAdmin(fabric_admintrue;super_user,least_privileged, a local user, andnullfalse) would prevent the exact regression this re-review fixed. The current mutation test mocks only the transport and cannot catch the route being exposed to password-authenticatedsuper_userusers again.
| } | ||
|
|
||
| export function useGenerateApiTokenMutation() { | ||
| return useMutation<ApiTokenResult, Error, void>({ mutationFn: generateApiToken }); |
There was a problem hiding this comment.
Could we avoid returning the bearer credential through React Query mutation state? useMutation stores successful data in the global MutationCache; with the browser default it remains there for five minutes after this route unmounts, and logoutOnSuccess() clears only the QueryCache. That means a generated operationToken survives navigation/logout in memory (and is directly shown in the mutation inspector in development builds). Please keep the secret out of shared mutation state, or reset/remove this mutation with immediate GC after moving the value into local component state, and add a test asserting the cache no longer contains operationToken after success/unmount/logout.
What
Adds a Fabric Admin section to Studio — an extensible home for internal admin tooling — and ships its first tool: a short-lived API Token generator.
/fabric-adminarea (src/features/fabricAdmin/): a layout shell (FabricAdminShell) with a sub-nav rail and an<Outlet/>, so future admin tools drop in as sibling routes.apiToken/index.tsx): a Generate button that callsPOST /Admin/ApiTokenand shows the returned token (read-only field + copy) with its expiry and a short usage note. The token authenticates as the signed-in admin, with their permissions.useAdminMode()→fabricRoleoffabric_admin/super_user) and are hidden entirely in local Studio.Why
We're forcing internal
fabric_adminaccounts onto Google SSO (see the companion central-manager PR). SSO gives humans a browser session but no password for programmatic/API access. This page lets an admin mint a short-lived, SSO-derived token for CLI/API use without reintroducing a password. It also gives us a place to grow the rest of the admin surface.Depends on / related
fabric-admin-auth-hardening— provides thePOST /Admin/ApiTokenendpoint this page calls (plus the SSO-session hardening). That endpoint must ship first, or Generate returns 404.Testing
vitest run— full suite green (241 files, 1722 passing).tsc -b,oxlint,dprint check— all clean.getParentRoute/addChildrenlockstep rule (CLAUDE.md).Notes