amadan
svcrole: Allow panics on unparseable roles, add panic + empty-form tests
Changes:
- Allow() now panics on unknown roles instead of silently dropping them,
ensuring typos fire at server start in CI rather than as 403s in
production. Silent drop would create a valid-looking Set with the
mistyped role quietly missing (e.g., Allow(Publish, Role('oprate'))
would return {admin, publish} with no indication of the typo).
- Removed dead code guard (len(s.roles)==0) after the loop; Allow() now
only returns Set{} as the explicit no-arguments form.
- Added strconv import for Quote in the panic message.
- Updated Allow's doc comment to explain the panic rationale.
- Added TestAllowPanicsOnAnUnknownRole: verifies panic on unknown roles
both in isolation and mixed with valid roles.
- Added TestEmptyDistinguishesTheTwoForms: distinguishes Allow() returning
an empty Set (deliberate closed door) from Allow(Publish) returning a
non-empty Set, and asserts Empty() can be both true and false.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016UimU7fAfeAoBMm6HKCPwQ
2 files changed,
+54
−8
internal/svcrole/svcrole.go+21 −8internal/svcrole/svcrole_test.go+33 −0
diff --git a/internal/svcrole/svcrole.go b/internal/svcrole/svcrole.go| index ca0da7c81..80f9805e9 100644 |
| --- a/internal/svcrole/svcrole.go |
| +++ b/internal/svcrole/svcrole.go |
| @@ -16,7 +16,10 @@ |
| // Spec: docs/superpowers/specs/2026-08-18-service-principals-design.md §3. |
| package svcrole |
| -import "sort" |
| +import ( |
| + "sort" |
| + "strconv" |
| +) |
| // Role is one name from the closed catalogue. A person's cli_tokens row |
| // carries the empty string, which is deliberately NOT a Role: authority for a |
| @@ -76,6 +79,14 @@ type Set struct { |
| // that Admin is the union — so ~50 registrations cannot disagree with the |
| // catalogue about what Admin is. A door reachable ONLY by an administrator |
| // says Allow(Admin) explicitly and gets exactly that. |
| +// |
| +// Allow called with no arguments returns an empty, closed Set — a legitimate |
| +// form distinct from a typo. Calling Allow with an unparseable role panics. |
| +// A dropped role would be a door silently narrowed: Allow(Publish, Role("oprate")) |
| +// would return a plausible {admin, publish} with the typo gone and nothing to |
| +// catch it. Registration runs once at server start, in NewServer and CI, so a |
| +// panic is the cheapest place for a typo to surface. Reaching this at all requires |
| +// an explicit Role("...") conversion — a mistyped CONSTANT is a compile error. |
| func Allow(roles ...Role) Set { |
| if len(roles) == 0 { |
| return Set{} |
| @@ -83,16 +94,18 @@ func Allow(roles ...Role) Set { |
| s := Set{roles: make(map[Role]bool, len(roles)+1)} |
| for _, r := range roles { |
| if _, ok := Parse(string(r)); !ok { |
| - // A typo'd constant is a programming error, and the safe |
| - // direction is to drop it: the door ends up MORE closed, not |
| - // silently open to a name nothing can hold anyway. |
| - continue |
| + // Panic, not skip. A dropped role is a door silently narrowed: |
| + // Allow(Publish, Role("oprate")) would return a perfectly |
| + // valid-looking {admin, publish} with the typo gone and nothing |
| + // to catch it. Registration runs once, in NewServer, so this |
| + // fires at server start and in CI rather than as a 403 someone |
| + // debugs in production weeks later. Reaching it at all takes an |
| + // explicit Role("...") conversion — a mistyped CONSTANT is a |
| + // compile error, which is why this costs nothing in practice. |
| + panic("svcrole: Allow called with an unknown role " + strconv.Quote(string(r))) |
| } |
| s.roles[r] = true |
| } |
| - if len(s.roles) == 0 { |
| - return Set{} |
| - } |
| s.roles[Admin] = true |
| return s |
| } |
diff --git a/internal/svcrole/svcrole_test.go b/internal/svcrole/svcrole_test.go| index 3dde663d4..09c5ab0a0 100644 |
| --- a/internal/svcrole/svcrole_test.go |
| +++ b/internal/svcrole/svcrole_test.go |
| @@ -80,3 +80,36 @@ func TestRolesIsSortedAndDeduplicated(t *testing.T) { |
| t.Errorf("Roles() = %v; want %v", got, want) |
| } |
| } |
| + |
| +func TestAllowPanicsOnAnUnknownRole(t *testing.T) { |
| + // A dropped role would be a door silently narrowed — see Allow's comment. |
| + // Registration runs once at server start, so this is the cheapest place |
| + // for a typo to surface. |
| + for _, bad := range []Role{"", "oprate", "Publish", "root"} { |
| + func() { |
| + defer func() { |
| + if recover() == nil { |
| + t.Errorf("Allow(%q) did not panic", bad) |
| + } |
| + }() |
| + Allow(bad) |
| + }() |
| + } |
| + // A typo MIXED WITH valid roles is the dangerous case: it would otherwise |
| + // produce a plausible Set with the mistyped door quietly missing. |
| + defer func() { |
| + if recover() == nil { |
| + t.Error("Allow(Publish, invalid) did not panic") |
| + } |
| + }() |
| + Allow(Publish, Role("oprate")) |
| +} |
| + |
| +func TestEmptyDistinguishesTheTwoForms(t *testing.T) { |
| + if s := Allow(); !s.Empty() { |
| + t.Error("Allow() should be the empty Set — the deliberate closed-door form") |
| + } |
| + if s := Allow(Publish); s.Empty() { |
| + t.Error("Allow(Publish).Empty() = true; Empty must not be unconditionally true") |
| + } |
| +} |