Repository navigation
Conversation
…e TS unions against them Go cannot enumerate a const block, so the frontend's Action and Resource unions were a hand-maintained mirror with nothing detecting drift. Add auth.AllActions() / auth.AllResources(). Their completeness is not left to memory: a go/ast test parses every non-test file in internal/auth and fails when an Action*/Resource* constant is declared without being enumerated (or enumerated without being declared), when such a constant is not a string literal, or when two share a value. A second test checks that the default group permissions and admin carve-outs use only enumerated values. cmd/gen-permissions gains a read-only test that parses the Action and Resource unions in frontend/src/permissions.ts and fails CI on drift in either direction. No frontend file changes. Refs LeanerCloud#166 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD
added
impact/internal
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
This branch has not been deployed
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
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.
Root cause
internal/authdeclares its permission vocabulary as individual untypedconsts (ActionView,ResourcePlans, ...). Go can't enumerate a const block, so nothing in Go could list the full set. The frontend'sAction/Resourceunions infrontend/src/permissions.tswere therefore a hand-kept mirror, and nothing detected them drifting from Go. That drift is what caused cli#1629, where the group-edit form silently dropped permissions and widenedview:historytoview:*.Fix
internal/auth/permission_catalog.go(new file):auth.AllActions()andauth.AllResources()return the full sets in declaration order. They includeadminand the*wildcard, and each call returns a fresh copy. I put them in a new file becausetypes.gois already over 500 lines, and because this keeps the const blocks untouched.Completeness doesn't depend on anyone remembering to update the list, which is the failure mode the issue warns about.
TestPermissionCatalogMatchesConstDeclsusesgo/astto parse every non-test.gofile ininternal/auth. It collects each package-level const namedAction<Upper>…/Resource<Upper>…, including Go's implicit-repeat specs, and fails when:Each failure names the constant and its position.
TestPermissionCatalogCoversSeededPermissionschecks that everyDefault*Permissions()entry and everyadminCarvedOutspair uses only enumerated actions and resources.Go-to-TS seam:
cmd/gen-permissions/union_drift_test.goreadsfrontend/src/permissions.ts, parses theexport type Action = …/export type Resource = …string-literal unions (comment-aware), and fails CI if either side has a value the other lacks. The frontend's existingRecord<Action, true>check keepsALL_ACTIONSequal to the union, and this test keeps the union equal to Go. Drift now failsgo test ./...in CI. The test only reads the file. This PR changes nothing underfrontend/.I kept the enumeration as a listed array plus an AST exhaustiveness test. A
go:generatestep or restructuring the consts around a slice would have rewritten the heavily commented const blocks intypes.go, and an open branch is already editing those blocks. This approach leavestypes.gountouched.Refs #166
(This is
Refs, not a closing keyword, because the audit finding attached to the issue, A11-011 onpermissionMatrix.ts, and the TS generation step both need frontend changes. See below.)Verification
All evidence below is from unit tests on this branch. No DB, cloud or integration code is involved.
Fail-before / mutation probes. Each was applied temporarily and then reverted:
permission_catalog.goremoved): both test packages fail to build (undefined: AllResources/undefined: auth.AllResources). Go has no enumeration to test against.ActionFreeze = "freeze"added to the action block):TestPermissionCatalogMatchesConstDeclsFAILS withActionFreeze = "freeze" is declared but missing from the catalog in permission_catalog.go.allActions): the auth tests pass, andTestFrontendPermissionUnionsMatchGoFAILS withAction union in ../../frontend/src/permissions.ts lacks Go value(s) ["freeze"].ResourceRIExchangeremoved fromallResources): the const-decl test FAILS (ResourceRIExchange = "ri-exchange" is declared but missing…), the seeded-permissions test FAILS (adminCarvedOutsusesri-exchange), and the frontend drift test FAILS (Resource union … has ["ri-exchange"]).ResourceAllAliaswith no value afterResourceAll): FAILS withResourceAll and ResourceAllAlias both have value "*".ResourceComputed = ResourcePlans + "-x"): FAILS withResourceComputed must be a string literal….TestTSUnionMemberscovers the TS parser: line comments, block comments, a/*inside a line comment,//inside a string literal, double quotes, and a missing declaration.I also checked with
git merge-treethat the branch merges cleanly with our unopenedfix/issue-313branch, which removes theRole*constants. This change doesn't reference them.Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@936996ad= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-72andfix/issue-312conflict on one line ininternal/api/handler_dashboard.go(both change thecalculateCommitmentMetricscall). The tree used #72's side with #312's validatedprovidervariable; whichever PR merges second takes that same resolution.Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:936996ad:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without--build-tags integration.tests/e2eis a separate module, so it was vetted under its owne2etag.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout because the VM was shared):936996ad, these pass:internal/server,internal/credentials,internal/scheduler,internal/database/postgres/migrations,internal/purchaseandinternal/api.internal/configpasses exceptTestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).26795376, same branches at earlier heads):internal/analytics,internal/auth,internal/commitmentopts,internal/databaseandcmd/lambda/clear-rate-limitall pass. The later fixes don't touch those packages.httpToLambdaRequestsignature change;scripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Deliberately out of scope
cmd/gen-permissions: that would change the committedfrontend/src/permissions.generated.ts, and the UI owner ownsfrontend/. The drift test gets the same "CI fails on drift" result without editing the frontend. See the follow-up below.AllActions()/AllResources()would make one trivial if it's ever wanted.internal/auth: the AST scan covers this package only. Handlers that pass bare string literals to permission checks elsewhere aren't covered.Action/Resource+ upper-case prefix. A permission const named some other way wouldn't be seen, and the test docs say this.Frontend follow-up for the UI owner
cmd/gen-permissionsto emit, for example,export const ACTIONS = [...] as const; export type Action = (typeof ACTIONS)[number];(and the same for resources) fromauth.AllActions()/auth.AllResources()intopermissions.generated.ts. Then import those inpermissions.ts, regenerate withgo run ./cmd/gen-permissions, and commit the result. The existingpermissions-codegenpre-commit hook then catches staleness, andcmd/gen-permissions/union_drift_test.gocan be deleted or pointed at the generated file. Until that happens, a new verb needs updating in three places: the const ininternal/auth/types.go, the list ininternal/auth/permission_catalog.go, and the union plus*_EXHAUSTIVENESS_CHECKrecord infrontend/src/permissions.ts. The tests name whichever one was missed.frontend/src/users/permissionMatrix.ts:11still has its own seven-verbACTIONSlist, so the matrix shows no row forapprove-any,retry-any,cancel-any,update-any,execute-any,sell-any,revoke-anyor any-ownverb. Fix: derive the matrix rows fromALL_ACTIONS, asgroupModals.tsalready does.permissions.tsheader says the unions are hand-written mirrors. It could also mention thatcmd/gen-permissions/union_drift_test.gonow enforces them.ACTION_EXHAUSTIVENESS_CHECKholds within the frontend, but reads as repo-wide. The issue asks for it to be scoped.Maintainer: please mirror the issue's triage labels onto this PR:
impact/internal,type/chore,triaged,priority/p3,severity/low,urgency/eventually,effort/m.🤖 Generated with Claude Code