refactor(admin): operator-only admin + change-management framework#35
Merged
Conversation
Add canUseFinanceFeatures, canUseAdminFeatures, and getLandingPath as the single source of frontend role truth. Pure, synchronous, framework-free helpers typed against the core User type, re-exported from the core barrel.
Assert the full two-role x three-helper truth table for the role helpers using a local User fixture (core is dependency-free and cannot import the shared test-utils factory without a circular dependency).
- add change-management/ with .validation/, .tools/, and 000-template/assets/ - keep the otherwise-empty dirs tracked via .gitkeep files - establish the foundation for the validator, log generator, and 001 item
- carry the fixed Change Event / Impact-Risk / Worst Case / Rollback headings - match section 6 exactly so items pass the validator delivered in #10 - guide authors with per-prompt angle-bracket placeholders
- demonstrate the paired # Activity N / # Validation N structure - give every block a Description line, Checklist, and Rollback Plan section - ship two matched pairs each to show the contiguous indexing convention
Add the 001 change-management item's dry-run and cleanup SQL: - dry-run.sql: read-only per-table count of admin-owned rows - cleanup.sql: single transaction deleting admin-owned rows from four finance.* tables and datarights.export_jobs, scoped and idempotent Keeps auth.users admin rows and datarights.deletion_jobs audit data.
- replace the em-dash in each 000-template H1 with a colon - comply with the repo prohibition on em-dashes in tracked files - no validator impact: title lines are not part of the enforced structure
…tion ensureSchema opens a plain lib/pq connection to CREATE SCHEMA before golang-migrate runs. It stripped search_path but left golang-migrate driver params (the x-* family, e.g. x-migrations-table), which lib/pq forwards to Postgres as runtime GUCs, causing 'unrecognized configuration parameter' errors. Extract rawConnectionURL to strip search_path and all x-* params, and unit-test it.
Add a temporary, TEST_DATABASE_URL-gated integration test that proves the shipping cleanup.sql is correctly scoped and idempotent before it runs on prod. Runs auth/finance/datarights migrations (per-service search_path + distinct x-migrations-table), seeds admin + regular-user finance rows, executes the exact assets/cleanup.sql, and asserts admin rows deleted, regular-user rows intact, auth.users admin + datarights.deletion_jobs audit data preserved, and a second run is a no-op. Removed by the 001 completion PR.
Replace both hardcoded /dashboard navigation targets in useLoginForm with getLandingPath(user) from @gofin/core: the already-authenticated redirect effect and the login success handler's final else branch. Admins land on /admin; regular-user flow (onboarding, returnTo, /dashboard) is unchanged.
Add /admin route to the shared login render helper and cover admin -> /admin in both the success handler and the already-authenticated redirect, alongside the existing user -> /dashboard and valid-returnTo cases.
Lift the tab-to-section mapping into a role-keyed settingsTabs.tsx so SettingsFeature is a thin renderer. Admins get [Profile, Password] only; the finance-only sections (DefaultBudget, Tags, ExportData) are referenced solely by the user tab definitions. The default tab is now tabList[0].id (Profile for admins) instead of a hardcoded "budget".
Enumerate every known gateway route class and assert each resolves to an explicit, rule-classified access level rather than the fail-safe Default. Distinguishes rule-classified from default fall-through via a twin-policy technique (resolve under two policies differing only in Default), so Authenticated routes in the mixed /api/auth group are guarded too, not just non-default levels. Covers US-GW-09.
Add settingsTabs unit tests (exact admin/user tab lists, role selection, default tab) and admin render tests asserting Profile+Password only, the Profile default tab, absence of Budget/Tags/Export sections, and that no finance API calls fire on the admin render path.
Define the canonical TokenValidator interface and TokenValidationResult struct in the access package, their consumer being access.AccessControl. middleware keeps compiling via type aliases (removed with auth.go in the router-cutover ticket). The concrete gRPC client satisfies access.TokenValidator structurally, since middleware.TokenValidationResult is now an alias.
Add a direct-admin redirect guard to auth-layout that precedes the onboarding guard: an operator (canUseAdminFeatures && !isAssuming) on any FINANCE_ROUTES path is redirected to /admin. Derive navLinks from role (Admin+Settings for a direct admin, full finance nav otherwise), gate the Log Expense FAB off for direct admins, and target the logo at getLandingPath(user). Role decisions read through the core helpers, not bare role checks.
…session Replace the old admin-nav-at-/dashboard test (behavior changed: admins are now redirected off finance routes) with coverage for the direct-admin guard redirect, its precedence over the onboarding guard, guard skip while assuming, operator-only nav (Admin+Settings, no FAB, logo->/admin), and the assumed-session nav (full finance nav + Return to Admin, logo->/dashboard).
Matrix over {Public, Authenticated, Personal, Admin} x {no-token, user, admin,
assumed-user} with a fake TokenValidator, plus focused tests for header
stripping, Public validation skip, downstream identity injection, X-Assumed-By
forwarding, assumed-session reachability, and 401/403 warn logging.
- add validate_change_management.py enforcing folder-name, required files, description headings, activity/validation pairing, and completion-evidence rules per framework section 6 - validate one item (--item) or all items (default); exit non-zero with item/file/rule on the first violation per item
- add unittest suite building fixtures in temp dirs (no malformed items committed) for every check and CLI exit code - assert the shipped 000-template and all four status suffixes pass
- Add hasPathPrefix so a prefix rule matches only on a path-segment boundary (next char is '/' or end), not as a bare substring - Prevents a future sibling like /api/datarights/exports-admin from matching the Personal prefix /api/datarights/exports and being under-restricted; this resolver is the single authz gate - Extend resolver tests with boundary cases and a focused segment-boundary test; bare-group paths like /api/finance still match
- Reword rejectForbidden comment: the FORBIDDEN code is preserved but the human-facing message text was consolidated to a generic 'Access denied' - Prevents the comment from overstating that the full 403 contract is unchanged
- Rename the login mutation onSuccess parameter from user to loggedInUser - The param is the fresh awaited login result; naming it user shadowed the store user used in the adjacent redirect effect
- Assert an admin with a valid non-finance returnTo lands there, not on the /admin landing path - Locks the onSuccess-wins timing that this refactor touched (previously uncovered)
- Resolve activeDefinition to tabList[0] when a persisted activeTab falls outside the role-derived tab list, instead of rendering nothing - Guards the role-flip-without-remount case (auth store re-renders with a new user via onUserUpdated); removes the silent optional-chaining no-render - Add a discriminating test that flips role on the same instance
thompsnt
reviewed
Jul 4, 2026
- Introduce services/access, a gin-free module owning the Access enum, a single Registry of all 41 gateway-facing routes with mandatory access levels, and a gin-priority path resolver - Resolve matches concrete request paths against Registry patterns and breaks overlaps (e.g. /api/expenses/suggestions vs /:id) the way gin dispatches, carrying forward ca37e4c's segment-precise intent - Register ./access in go.work so the CI module loop runs its tests
- Depend on services/access (require + replace) and inject GatewayResolve into AccessControl instead of a local Policy; /health and /metrics stay gateway-classified Public, everything else delegates to access.Resolve - Delete the moved/superseded access.go, policy.go, resolver.go and their duplicated resolver_test.go and hand-maintained coverage_test.go - Rewrite the fail-safe test around an injected out-of-enum resolver and add a GatewayResolve test; add the access module COPY lines to the Dockerfile - Point the direct-admin rejection test at the real GET /api/expenses route (the precise resolver sends the non-real trailing-slash path to the Authenticated default, which 404s downstream)
- BindRoutes iterates RoutesFor(service) and binds each route's handler by ID, panicking when a classified route has no handler so the gap is caught at startup and in the per-service registration test - Generic over the handler type to keep the module web-framework-free; services pass gin.HandlerFunc via an engine.Handle adapter
- VerifyRegistration compares a service's registered routes against the Registry in both directions and returns a route-naming, Registry-pointing error, so each service's coverage test is a thin wrapper over shared logic - Compares method+path byte-for-byte (params included), pinning Registry patterns to gin's real registration and ignoring health/metrics
- Convert auth, finance, expense and datarights RegisterRoutes to bind handlers by ID via access.BindRoutes over RoutesFor(service); datarights gets a package-level RegisterRoutes merging its export + deletion handlers - Add a per-service registration coverage test that builds the real engine with nil deps and asserts engine.Routes() matches the Registry both ways, so an unclassified route fails that service's own suite in CI - Add the access module COPY lines to all four service Dockerfiles
- Add app/lib/route-access.ts (RouteAccess + canAccess) and a
useAuthLayoutGuards hook that reads the deepest matched route's
handle.access via useMatches; each route module now exports its access
("personal"/"admin"/"authenticated") and home redirects role-aware
- Delete the FINANCE_ROUTES array and the hasCompletedOnboarding redirect
hack: a direct admin on a personal route (or a regular user on an admin
route) now renders a 403 page, and onboarding is role-driven so admins are
never sent to /onboarding
- Decompose auth-layout into Navbar/DesktopNav/MobileNav/ReturnToAdminButton/
LogExpenseFab/Forbidden + the guards hook under features/shell-layout;
auth-layout is a thin orchestrator (<300 LOC), one component per file
- Scope a shell ESLint override so route modules may export handle
- Rewrite auth-layout tests for the 403 model: attach handle.access to test route defs and assert direct-admin-on-personal and user-on-admin render the Forbidden page, assumed sessions pass, and admins are never onboarding-routed - Migrate login (incl. the returnTo regression) and register interaction tests to userEvent.setup(); fix the stale FINANCE_ROUTE comment - Keep fireEvent.submit only where a case exercises the submit handler's own validation that native required/type=email constraints would otherwise shadow
- data-model: drop the temporal one-time-cleanup clauses and the change-management/001 link from the Datarights and Finance notes, keeping only the durable invariant (tables hold role=user data only) - development: state the seed-admin flag is set for data consistency but is not the exemption mechanism (admins are structurally exempt via role/route metadata); reflect the 403-page behavior instead of a redirect - testing: describe the per-service registry-driven coverage tests (a new unclassified route fails that service's suite in CI) and the 403 guard - architecture/api: reference services/access instead of the deleted policy.go and drop the stale exact/longest-prefix matching description
- Replace the exact/prefix annotations in the assumed-user matrix test with plain Personal labels (the resolver is registry-based, not prefix-based) - Reword the auth ListUsers/AssumeIdentity doc comments to reference the shared services/access registry classification instead of an /api/admin prefix or a "policy" table
Flip the /api resolver fail-safe from Authenticated to a new Deny level so an unclassified route (or a whole unclassified prefix) is refused with 403 instead of silently proxying through as any-authenticated. This extends the "can't ship an unclassified route" guarantee to new services by construction. - add Access.Deny; Resolve returns it when no Registry entry matches - gateway AccessControl short-circuits Deny to 403 before reading the cookie (an unclassified path carries no identity); the post-validation switch default stays as defense in depth - real routes keep their exact outcome; only previously-unmatched /api paths change (now 403 instead of 401/proxy-through); /health and /metrics stay Public
Add access.ProxyPrefixes (an ordered {Prefix, Service} inventory) as the single
source of truth for which downstream prefixes the gateway proxies and to which
service, and data-drive router.New from it: build one proxy handler per service
and loop the inventory registering each group. Fail fast (panic) if a prefix
names a service with no proxy handler.
A services/access cross-check test pins ProxyPrefixes to the Registry in both
directions: every classified route sits under a proxied prefix (and the right
service), and every proxied prefix has at least one classified route. Combined
with deny-by-default, onboarding a service now fails closed unless its routes
are classified and its prefix is inventoried.
services/access stays gin-free; the gateway imports it, not the services.
Update the access-control docs to reflect the hardening changes: - the resolver fail-safe is now deny (403) for unclassified /api paths, not the Authenticated default (architecture.md, api.md, auth.md, testing.md) - note that the gateway derives its proxy wiring from access.ProxyPrefixes and that a cross-check test pins it to the route registry
- add accessHandle(access) so each route's handle.access is checked against RouteAccess at the call site; a typo now fails to compile instead of silently resolving to a 403 - convert all route modules to accessHandle and remove the vestigial admin-page useEffect/isAdmin guard, since the auth-layout guard renders <Forbidden/> for non-admins before the page mounts - clarify in useAuthLayoutGuards why access (route metadata) and the current path (useLocation) are read from two distinct sources
- add a canAccess truth table across public/authenticated/personal/admin against direct-admin, regular-user, and assumed-session identities - add navLinksFor cases pinning the operator vs finance nav surfaces and asserting every link has an icon
thompsnt
approved these changes
Jul 6, 2026
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.
Summary
Converts the GoFin
adminaccount from a superset-of-user into an operator-only identity, makes gateway access control a single source of truth shared by every service, and introduces the reusable change-management framework plus the one-time data cleanup this refactor requires.Delivers the "Admin Operator Refactor" epic (tickets #1–#15) together with the PR review remediation (#17). The terminal operational step (running the
001cleanup on prod) is intentionally not in this PR: it is a change-managed manual procedure performed after this PR is merged and deployed, finalized by a separate completion PR.What changed
Backend: a shared access module is the single source of truth (
services/access)Registryof every concrete route (41 today) with a mandatory access level, a gin-priority pathResolve, a generic bind-by-IDBindRoutes, a bidirectionalVerifyRegistration, and aProxyPrefixesinventory.AccessControlresolves each request against the Registry (via an injectedGatewayResolvethat classifies gateway-native/health+/metricsas Public), and the proxy groups are generated fromProxyPrefixes. Onboarding a service is a single edit to the shared module.Denylevel and is refused with 403 before any token is read, replacing the earlierAuthenticatedfail-safe default. A cross-check test proves every proxied prefix has at least one classified route and vice versa, so no real route is silently denied. Access outcomes for every existing real route are preserved (the gatewayAccessControlrole matrix is unchanged).middleware/auth.go,middleware/admin.go) and their tests are deleted; a singleAccessControlmiddleware replaces them andTokenValidatorlives in theaccesspackage. Direct admin (role=admin) gets 403 on Personal routes; assumed sessions (role=user+assumedBy) pass unchanged.Frontend: route-metadata guards + operator-only role model
core/roles.ts(canUseFinanceFeatures,canUseAdminFeatures,getLandingPath).handlemetadata (accessHandle("personal" | "admin" | ...), checked againstRouteAccessat the call site); the auth-layout guard derives behavior from the deepest matchedhandle.accessthrough one symmetric predicate,canAccess. This deletes the ad-hocFINANCE_ROUTESarray and thehasCompletedOnboardingredirect hack.<Forbidden/>page (no silent redirect, no flash of the wrong UI). Onboarding is role-driven, so admins are never routed through it; assumed sessions behave as users.auth-layout.tsxdecomposed intofeatures/shell-layout/(Navbar, DesktopNav, MobileNav, ReturnToAdminButton, LogExpenseFab, Forbidden, theuseAuthLayoutGuardshook, andnavLinksFor), leaving a thin sub-300-LOC orchestrator; one component per file, named exports.userEventand rewritten for the 403 guard model; new unit coverage forcanAccessandnavLinksFor.Change-management framework (
change-management/)000-template, a Python validator (validate_change_management.py), an execution-log generator (generate_execution_log.py), avalidate-change-managementCI gate, and a project skill.Admin finance cleanup item
001001_admin-finance-cleanup(description.md/preflight.md/steps.md+dry-run.sql/cleanup.sql) and a temporary safety test (services/dbmigrate/admin_finance_cleanup_test.go) proving the SQL is scoped + idempotent. The safety test is removed by the post-run completion PR.Documentation
auth.md,data-model.md,api.md,architecture.md,README.md,development.md,testing.mdaligned with the operator-only model, the shared registry, and deny-by-default.Sequencing (why merge + deploy gate the cleanup)
Once this PR is merged and deployed, a direct admin can no longer create finance rows. Only then is the
001prod runbook run (dry-run → backup →cleanup.sqlin a transaction → post-check), followed by the completion PR that removes the safety test, adds the filledexecution-log.md, and renames the item folder to001_admin-finance-cleanup_completed.Test / validation
services/accessregistry/resolver/coverage suites plus the preserved gatewayAccessControlrole matrix; each module builds underGOWORK=off, and all 5 consumer Dockerfiles copy the shared module (exercised by the CI e2edocker compose up -d --build).tscat the pre-existing ~24-error baseline in@gofin/finance/@gofin/admin(no regression).python3 change-management/.validation/validate_change_management.py→OK: 2 change-management item(s) conform.