From 41fb77b72c8a0998834a454c81b2716dc0d62a44 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 06:51:26 +0000 Subject: [PATCH] chore(auth): enumerate permission actions and resources, and check the 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 #166 Co-Authored-By: Claude Opus 5.5 --- cmd/gen-permissions/union_drift_test.go | 147 +++++++++++++++++++++++ internal/auth/permission_catalog.go | 59 +++++++++ internal/auth/permission_catalog_test.go | 142 ++++++++++++++++++++++ 3 files changed, 348 insertions(+) create mode 100644 cmd/gen-permissions/union_drift_test.go create mode 100644 internal/auth/permission_catalog.go create mode 100644 internal/auth/permission_catalog_test.go diff --git a/cmd/gen-permissions/union_drift_test.go b/cmd/gen-permissions/union_drift_test.go new file mode 100644 index 000000000..5f0d9e975 --- /dev/null +++ b/cmd/gen-permissions/union_drift_test.go @@ -0,0 +1,147 @@ +package main + +import ( + "os" + "path/filepath" + "regexp" + "slices" + "strings" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" +) + +// permissionsTSRelPath is the hand-written frontend file whose Action and +// Resource unions mirror auth.AllActions / auth.AllResources. The test only +// reads it. +var permissionsTSRelPath = filepath.Join("..", "..", "frontend", "src", "permissions.ts") + +var tsUnionMember = regexp.MustCompile(`[=|]\s*(?:'([^']*)'|"([^"]*)")`) + +// stripTSComments removes // and /* */ comments from src, leaving string +// literals (which may contain comment markers) intact. Template literals +// are not handled; permissions.ts has none in its unions. +func stripTSComments(src string) string { + var b strings.Builder + for i := 0; i < len(src); i++ { + switch { + case src[i] == '\'' || src[i] == '"': + end := i + 1 + for end < len(src) && src[end] != src[i] && src[end] != '\n' { + if src[end] == '\\' { + end++ + } + end++ + } + end = min(end, len(src)-1) + b.WriteString(src[i : end+1]) + i = end + case strings.HasPrefix(src[i:], "//"): + nl := strings.IndexByte(src[i:], '\n') + if nl < 0 { + return b.String() + } + i += nl - 1 + case strings.HasPrefix(src[i:], "/*"): + end := strings.Index(src[i+2:], "*/") + if end < 0 { + return b.String() + } + i += end + 3 + default: + b.WriteByte(src[i]) + } + } + return b.String() +} + +// tsUnionMembers returns the string-literal members of +// `export type = 'a' | 'b' ...;` in src, or nil if the declaration +// is absent. +func tsUnionMembers(src, name string) []string { + src = stripTSComments(src) + start := regexp.MustCompile(`export\s+type\s+` + regexp.QuoteMeta(name) + `\s*=`).FindStringIndex(src) + if start == nil { + return nil + } + body := src[start[1]-1:] // keep the '=' so the first member matches + if end := strings.IndexByte(body, ';'); end >= 0 { + body = body[:end] + } + var out []string + for _, m := range tsUnionMember.FindAllStringSubmatch(body, -1) { + out = append(out, m[1]+m[2]) + } + return out +} + +func diffSets(want, got []string) (missing, extra []string) { + for _, v := range want { + if !slices.Contains(got, v) { + missing = append(missing, v) + } + } + for _, v := range got { + if !slices.Contains(want, v) { + extra = append(extra, v) + } + } + return missing, extra +} + +// TestFrontendPermissionUnionsMatchGo fails when the frontend's Action or +// Resource union drifts from the Go constants (issue #166). The frontend's +// Record check keeps ALL_ACTIONS equal to the union; this +// keeps the union equal to Go, so drift fails CI instead of silently +// dropping or widening group permissions on save (cli#1629). +func TestFrontendPermissionUnionsMatchGo(t *testing.T) { + raw, err := os.ReadFile(permissionsTSRelPath) + if err != nil { + t.Fatalf("read %s: %v", permissionsTSRelPath, err) + } + src := string(raw) + for _, tc := range []struct { + union string + want []string + }{ + {"Action", auth.AllActions()}, + {"Resource", auth.AllResources()}, + } { + got := tsUnionMembers(src, tc.union) + if len(got) == 0 { + t.Fatalf("no `export type %s = ...` union found in %s", tc.union, permissionsTSRelPath) + } + missing, extra := diffSets(tc.want, got) + if len(missing) > 0 { + t.Errorf("%s union in %s lacks Go value(s) %q: add them to the union and its exhaustiveness record", + tc.union, permissionsTSRelPath, missing) + } + if len(extra) > 0 { + t.Errorf("%s union in %s has %q, which auth.All%ss() does not enumerate", + tc.union, permissionsTSRelPath, extra, tc.union) + } + } +} + +func TestTSUnionMembers(t *testing.T) { + src := ` +// export type Action = 'commented-out'; +export type Action = + | 'view' + // a comment with 'quoted' text | 'not-a-member' + | "double" + /* | 'blocked' */ + | 'admin'; +// a line comment with an opener /* that must not swallow the next union +export type Resource = '*' | 'plans' | '//not-a-comment'; +` + if got, want := tsUnionMembers(src, "Action"), []string{"view", "double", "admin"}; !slices.Equal(got, want) { + t.Errorf("Action = %q, want %q", got, want) + } + if got, want := tsUnionMembers(src, "Resource"), []string{"*", "plans", "//not-a-comment"}; !slices.Equal(got, want) { + t.Errorf("Resource = %q, want %q", got, want) + } + if got := tsUnionMembers(src, "Missing"); got != nil { + t.Errorf("Missing = %q, want nil", got) + } +} diff --git a/internal/auth/permission_catalog.go b/internal/auth/permission_catalog.go new file mode 100644 index 000000000..1eb17db59 --- /dev/null +++ b/internal/auth/permission_catalog.go @@ -0,0 +1,59 @@ +package auth + +// allActions and allResources enumerate the predefined Action* and +// Resource* constants (issue #166). Go cannot enumerate a const block, so +// these arrays are the enumerable form of it. They are not trusted to stay +// complete by hand: TestPermissionCatalogMatchesConstDecls parses this +// package's source and fails when an Action* or Resource* constant is +// declared without being listed here, or listed here without being +// declared. Add new verbs or resources to the const blocks in types.go AND +// here; the test names the one you missed. +var allActions = [...]string{ + ActionView, + ActionCreate, + ActionUpdate, + ActionDelete, + ActionExecute, + ActionApprove, + ActionAdmin, + ActionCancelOwn, + ActionCancelAny, + ActionRetryOwn, + ActionRetryAny, + ActionApproveOwn, + ActionApproveAny, + ActionExecuteOwn, + ActionExecuteAny, + ActionUpdateAny, + ActionRevokeOwn, + ActionRevokeAny, + ActionSellOwn, + ActionSellAny, +} + +var allResources = [...]string{ + ResourceRecommendations, + ResourcePlans, + ResourcePurchases, + ResourceHistory, + ResourceConfig, + ResourceAccounts, + ResourceUsers, + ResourceGroups, + ResourceAPIKeys, + ResourceRIExchange, + ResourceAll, +} + +// AllActions returns every predefined permission action, in declaration +// order. It includes the admin verb. The caller owns the returned slice. +func AllActions() []string { + return append([]string(nil), allActions[:]...) +} + +// AllResources returns every predefined permission resource, in +// declaration order. It includes the ResourceAll wildcard ("*"), which a +// stored permission may name. The caller owns the returned slice. +func AllResources() []string { + return append([]string(nil), allResources[:]...) +} diff --git a/internal/auth/permission_catalog_test.go b/internal/auth/permission_catalog_test.go new file mode 100644 index 000000000..24e742da4 --- /dev/null +++ b/internal/auth/permission_catalog_test.go @@ -0,0 +1,142 @@ +package auth + +import ( + "go/ast" + "go/parser" + "go/token" + "maps" + "path/filepath" + "slices" + "strconv" + "strings" + "testing" + "unicode" + "unicode/utf8" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// declaredPermissionConsts parses every non-test .go file in this package +// and returns the value of each package-level constant whose name is +// followed by an upper-case letter (ActionView, ResourceAll, ...), +// keyed by value and mapped to the constant's name. It reads the source +// rather than a hand-kept list so that a constant added anywhere in the +// package is seen without anyone remembering to register it (issue #166). +func declaredPermissionConsts(t *testing.T, prefix string) map[string]string { + t.Helper() + files, err := filepath.Glob("*.go") + require.NoError(t, err) + fset := token.NewFileSet() + out := map[string]string{} + for _, path := range files { + if strings.HasSuffix(path, "_test.go") { + continue + } + f, err := parser.ParseFile(fset, path, nil, parser.SkipObjectResolution) + require.NoError(t, err, "parse %s", path) + for _, decl := range f.Decls { + gd, ok := decl.(*ast.GenDecl) + if !ok || gd.Tok != token.CONST { + continue + } + collectConstSpecs(t, fset, gd, prefix, out) + } + } + return out +} + +// collectConstSpecs adds the matching specs of one const declaration to +// out. A spec with no value repeats the previous spec's expression, as the +// Go spec defines for parenthesized const blocks. +func collectConstSpecs(t *testing.T, fset *token.FileSet, gd *ast.GenDecl, prefix string, out map[string]string) { + t.Helper() + var prev []ast.Expr + for _, spec := range gd.Specs { + vs := spec.(*ast.ValueSpec) + if len(vs.Values) > 0 { + prev = vs.Values + } + for i, name := range vs.Names { + if !hasPermissionPrefix(name.Name, prefix) { + continue + } + pos := fset.Position(name.Pos()) + require.Less(t, i, len(prev), "%s: %s has no value", pos, name.Name) + lit, ok := prev[i].(*ast.BasicLit) + require.True(t, ok && lit.Kind == token.STRING, + "%s: %s must be a string literal so the permission catalog test can read it", pos, name.Name) + val, err := strconv.Unquote(lit.Value) + require.NoError(t, err) + if other, dup := out[val]; dup { + t.Fatalf("%s: %s and %s both have value %q", pos, other, name.Name, val) + } + out[val] = name.Name + } + } +} + +func hasPermissionPrefix(name, prefix string) bool { + rest, ok := strings.CutPrefix(name, prefix) + if !ok || rest == "" { + return false + } + r, _ := utf8.DecodeRuneInString(rest) + return unicode.IsUpper(r) +} + +func assertCatalogMatches(t *testing.T, kind string, declared map[string]string, listed []string) { + t.Helper() + require.NotEmpty(t, declared, "found no %s constants; did the naming convention change?", kind) + seen := map[string]bool{} + for _, v := range listed { + assert.False(t, seen[v], "%s %q is listed twice in the catalog", kind, v) + seen[v] = true + _, ok := declared[v] + assert.True(t, ok, "%s %q is in the catalog but no %s* constant declares it", kind, v, kind) + } + for _, v := range slices.Sorted(maps.Keys(declared)) { + assert.True(t, seen[v], + "%s = %q is declared but missing from the catalog in permission_catalog.go", declared[v], v) + } +} + +// TestPermissionCatalogMatchesConstDecls is the exhaustiveness guarantee +// for AllActions / AllResources (issue #166): adding an Action* or +// Resource* constant without listing it, or listing a value no constant +// declares, fails here. +func TestPermissionCatalogMatchesConstDecls(t *testing.T) { + assertCatalogMatches(t, "Action", declaredPermissionConsts(t, "Action"), AllActions()) + assertCatalogMatches(t, "Resource", declaredPermissionConsts(t, "Resource"), AllResources()) +} + +func TestPermissionCatalogReturnsCopies(t *testing.T) { + a := AllActions() + a[0] = "mutated" + assert.Equal(t, ActionView, AllActions()[0]) + r := AllResources() + r[0] = "mutated" + assert.Equal(t, ResourceRecommendations, AllResources()[0]) +} + +// TestPermissionCatalogCoversSeededPermissions checks that every permission +// the package itself grants or carves out uses the enumerated vocabulary, +// so a verb spelled as a bare string literal cannot bypass the catalog. +func TestPermissionCatalogCoversSeededPermissions(t *testing.T) { + actions, resources := AllActions(), AllResources() + sets := map[string][]Permission{ + "DefaultAdminPermissions": DefaultAdminPermissions(), + "DefaultUserPermissions": DefaultUserPermissions(), + "DefaultReadOnlyPermissions": DefaultReadOnlyPermissions(), + "DefaultPurchaserPermissions": DefaultPurchaserPermissions(), + } + for pair := range adminCarvedOuts { + sets["adminCarvedOuts"] = append(sets["adminCarvedOuts"], Permission{Action: pair[0], Resource: pair[1]}) + } + for name, perms := range sets { + for _, p := range perms { + assert.Contains(t, actions, p.Action, "%s grants unknown action %q", name, p.Action) + assert.Contains(t, resources, p.Resource, "%s grants unknown resource %q", name, p.Resource) + } + } +}