diff --git a/README.md b/README.md index 5b27942..79b6e9d 100644 --- a/README.md +++ b/README.md @@ -240,6 +240,39 @@ also a construction error, never a panic (`providers.go`, `mcp_a2a.go`, ### `GroupConfig` +A user can belong to more than one group (`UserConfig.group`/`groups`) +and can additionally carry a personal `providers`/`models` grant of their +own — see [`UserConfig`](#userconfig) for the full contract. In short: +a request is authorized when it matches some **single** member group's +own `providers` **and** `models` (or the personal grant's own), never a +provider from one group combined with a model only a *different* group +or the personal grant allows — providers/models from separate grants +never merge. Each member group's own `limits` apply as its own, +independent limit scope; `mcpServers`/`agents` are unioned across member +groups (any one member's empty list makes the union allow-all too); the +effective `cache` setting is `false` if any member sets it `false`, else +`true` if any sets it `true`, else inherits; `cacheTTL` is the smallest +positive `cacheTTL` configured among member groups. A user belonging to +exactly one group with no personal grant is completely unaffected by any +of this — it behaves exactly as before multi-group support existed. + +`passthroughPaths` is the one exception to the union rule above: it is +**coupled to the provider AND the model**, not unioned across member +groups. A native passthrough request to provider `P`, path `X`, and body +model `M` is authorized when some **single** grant — a member group's +own, or the personal grant, which has none of its own `passthroughPaths` +and so borrows the user's *primary* group's (`group`, or `groups[0]` when +`group` is empty) — allows `P` **and** `X` **and** (its own `models` list +is empty, or `M` matches it). Unioning `passthroughPaths` the same way as +the other lists, or checking the model against a *different* matching +grant than the one that authorized the path, would both let joining +another group (or adding a personal grant) authorize a combination no +single grant ever granted — e.g. group `a` (`providers: [beta], models: +[m1]`, no path restriction) joined with group `b` (`providers: [beta], +passthroughPaths: [v1/chat/completions]`, no model restriction) must +still deny `/beta/v1/files` with body `{"model":"m2"}`, even though `a` +alone allows the path and `b` alone allows the model. + | Field | Type | Default | Semantics | |---|---|---|---| | `limits` | `*LimitsConfig` | `nil` (unlimited) | | @@ -288,10 +321,17 @@ own is governed purely by their group's — see | Field | Type | Default | Semantics | |---|---|---|---| | `name` | `string` | — | Required. | -| `group` | `string` | — | Required; must reference a configured group, or construction fails. | +| `group` | `string` | — | The user's primary group. May be left empty when `groups` below names at least one group instead — see `groups` for the "at least one" rule. Any non-empty value here or in `groups` must reference a configured group, or construction fails. | +| `groups` | `[]string` | `[]` | Further member groups, alongside `group` — see [`GroupConfig`](#groupconfig) for the full multi-group authorization/limits/cache contract. Effective membership is `group` first (when non-empty), then `groups`, de-duplicated, order preserved. At least one group, from either field, is required — a user naming none at all fails construction. | | `apiKey` | `string` | — | Secret form (see [Secret forms](#secret-forms)); must resolve non-empty and unique across every inline **and** file-sourced user. | -| `limits` | `*LimitsConfig` | `nil` (governed by group's limits alone) | When set, enforced alongside — not instead of — the group's own limits; both scopes are checked, and the request is refused by whichever is breached first. | -| `admin` | `bool` | `false` | Grants access to the read-only admin dashboard — see [Admin](#admin). Works the same for an inline or a file-sourced user. An admin user is otherwise ordinary: their own keys, group, and limits still apply, including to the admin routes themselves. | +| `providers` | `[]string` | `[]` | This user's own personal grant — access beyond whatever their group(s) already allow, glob-matched exactly like a group's own `providers`. Set alone (with `models` left empty), it grants **every** model on those providers — the same empty-`models`-means-all-models contract a group's own `models` carries. The personal grant exists at all only when `providers` is non-empty — leaving it empty/omitted never means "allow everything" on its own, regardless of `models`. | +| `models` | `[]string` | `[]` | The personal grant's own model glob, narrowing `providers` above — same semantics as a group's own `models`. Requires `providers` to be non-empty too: a `models`-only personal grant (no `providers`) would reach every configured provider, not just the ones already covered — a construction error naming the user. | +| `limits` | `*LimitsConfig` | `nil` (governed by group's limits alone) | When set, enforced alongside — not instead of — every member group's own limits (one independent scope per member group); the request is refused by whichever scope is breached first. The personal grant above carries no limits of its own. | +| `admin` | `bool` | `false` | Grants access to the read-only admin dashboard — see [Admin](#admin). Works the same for an inline or a file-sourced user. An admin user is otherwise ordinary: their own keys, group(s), and limits still apply, including to the admin routes themselves. | + +A user belonging to exactly one group (`group` alone, `groups` empty) +with no personal grant (`providers`/`models` both empty) behaves +byte-identically to before multi-group support existed. ### `RedisConfig` @@ -416,11 +456,25 @@ Secret (`users_file.go`): { "users": [ {"name": "alice", "group": "engineering", "apiKey": "sk-llmgw-..."}, - {"name": "bob", "group": "general", "apiKey": "env:LLMGW_KEY_BOB", "limits": {"requestsPerDay": 100}} + {"name": "bob", "group": "general", "apiKey": "env:LLMGW_KEY_BOB", "limits": {"requestsPerDay": 100}}, + { + "name": "carol", + "group": "engineering", + "groups": ["friends"], + "apiKey": "env:LLMGW_KEY_CAROL", + "providers": ["gx10"], + "models": ["gx10/GLM-5.3-Flash-EXL3"] + } ] } ``` +Carol belongs to both `engineering` and `friends` (each group's own +grant authorizes her independently — see [`GroupConfig`](#groupconfig)), +**plus** a personal grant adding exactly one `gx10` model — an addition +on top of whatever `engineering`/`friends` themselves already allow, not +a cap on her overall access. + A file user with the same `name` as an inline user overrides it (the inline entry is dropped from the merged set). Hot reload polls the file's mtime, throttled to once per 5 seconds on the request path, and is diff --git a/admin.go b/admin.go index bda3379..b846c59 100644 --- a/admin.go +++ b/admin.go @@ -732,23 +732,28 @@ func (g *Gateway) serveAdminOverview(w http.ResponseWriter) { // dashboard can tell "explicitly restricted to these N" apart from // "unrestricted" without the payload growing with catalog size. type adminUsageEntryView struct { - Limits *LimitsConfig `json:"limits,omitempty"` - Kind string `json:"kind"` - ID string `json:"id"` - GroupName string `json:"groupName,omitempty"` - Providers []string `json:"providers,omitempty"` - Models []string `json:"models,omitempty"` - MCPServers []string `json:"mcpServers,omitempty"` - Agents []string `json:"agents,omitempty"` - RequestsPerMinute int64 `json:"requestsPerMinute"` - RequestsPerDay int64 `json:"requestsPerDay"` - TokensInPerDay int64 `json:"tokensInPerDay"` - TokensOutPerDay int64 `json:"tokensOutPerDay"` - TokensInPerMonth int64 `json:"tokensInPerMonth"` - TokensOutPerMonth int64 `json:"tokensOutPerMonth"` - CostPerDayMicroUSD int64 `json:"costPerDayMicroUsd"` - CostPerMonthMicroUSD int64 `json:"costPerMonthMicroUsd"` - StoreDown bool `json:"storeDown,omitempty"` + Limits *LimitsConfig `json:"limits,omitempty"` + Kind string `json:"kind"` + ID string `json:"id"` + GroupName string `json:"groupName,omitempty"` + // Groups is the user's full member group list (multi-group support) + // — set only for a user entry, same "user-only" convention GroupName + // above already follows. GroupName keeps the first group for + // back-compat; Groups is the complete membership. + Groups []string `json:"groups,omitempty"` + Providers []string `json:"providers,omitempty"` + Models []string `json:"models,omitempty"` + MCPServers []string `json:"mcpServers,omitempty"` + Agents []string `json:"agents,omitempty"` + RequestsPerMinute int64 `json:"requestsPerMinute"` + RequestsPerDay int64 `json:"requestsPerDay"` + TokensInPerDay int64 `json:"tokensInPerDay"` + TokensOutPerDay int64 `json:"tokensOutPerDay"` + TokensInPerMonth int64 `json:"tokensInPerMonth"` + TokensOutPerMonth int64 `json:"tokensOutPerMonth"` + CostPerDayMicroUSD int64 `json:"costPerDayMicroUsd"` + CostPerMonthMicroUSD int64 `json:"costPerMonthMicroUsd"` + StoreDown bool `json:"storeDown,omitempty"` } // adminUsageResponse is the full body of GET /admin/api/usage: every @@ -863,6 +868,7 @@ func (g *Gateway) buildAdminUsage() adminUsageResponse { for i, su := range userUsage { users[i] = usageEntryView(su, userSummaries[i].limits) users[i].GroupName = userSummaries[i].groupName + users[i].Groups = userSummaries[i].groups } groups := make([]adminUsageEntryView, len(groupUsage)) for i, su := range groupUsage { diff --git a/admin_test.go b/admin_test.go index 6bbb511..9d637ac 100644 --- a/admin_test.go +++ b/admin_test.go @@ -1080,6 +1080,42 @@ func TestAdminUsage_GroupAccessLists(t *testing.T) { } } +// TestAdminUsage_MultiGroupUser_ListsAllGroupsInJSON proves the admin +// usage JSON view (adminUsageEntryView.Groups) lists every member group +// for a multi-group user, not just the first — GroupName (singular) +// keeps the first group for back-compat. +func TestAdminUsage_MultiGroupUser_ListsAllGroupsInJSON(t *testing.T) { + t.Parallel() + cfg := CreateConfig() + cfg.Admin = &AdminConfig{Enabled: true} + cfg.Providers = map[string]*ProviderConfig{ + "alpha": {Type: "openai", BaseURL: "http://alpha.invalid", APIKey: "sk-alpha"}, + } + cfg.Groups = map[string]*GroupConfig{"eng": {}, "ops": {}} + cfg.Users = &UsersConfig{Inline: []*UserConfig{ + {Name: "carol", Group: "eng", Groups: []string{"ops"}, APIKey: "sk-carol"}, + {Name: "admin1", Group: "eng", APIKey: "sk-admin1", Admin: true}, + }} + h, _ := newAdminGatewayHandle(t, cfg) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, adminRequest(http.MethodGet, adminUsagePath, "sk-admin1")) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + var got adminUsageResponse + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("decode: %v", err) + } + carol := findUsageEntry(t, got.Users, "carol") + if carol.GroupName != "eng" { + t.Errorf("carol.GroupName = %q, want %q (first group, back-compat)", carol.GroupName, "eng") + } + if !slices.Equal(carol.Groups, []string{"eng", "ops"}) { + t.Errorf("carol.Groups = %v, want [eng ops]", carol.Groups) + } +} + // TestLimiterCurrentUsage_StoreDown drives limiter.currentUsage's // fail-closed path directly: a configured store whose every operation // errors, with failOpen false, must report storeDown and zero values diff --git a/auth.go b/auth.go index c128088..bd02a31 100644 --- a/auth.go +++ b/auth.go @@ -9,6 +9,7 @@ import ( "net/url" "path" "sort" + "strconv" "strings" "sync" "time" @@ -18,6 +19,13 @@ import ( type user struct { limits *LimitsConfig name, groupName string + // groupNames is this user's full, de-duplicated, order-preserving + // member group list (effectiveGroupNames, below) — groupName above + // keeps only the first, for back-compat display; groupNames is the + // complete membership, read by snapshot (below) for the admin + // dashboard's per-user "groups" listing and for counting this user + // once in EACH member group's own memberCount. + groupNames []string // admin grants access to the read-only admin dashboard (spec §4, // v0.2) — an admin user is otherwise ordinary: their own limits and // group authorization still apply, including to the admin routes. @@ -31,16 +39,43 @@ type group struct { // cache is GroupConfig.Cache carried through unchanged: nil inherits // the global cache.enabled setting, non-nil overrides it for this // group's requests. Resolved by groupCacheEnabled (cache.go). - cache *bool - name string - providers []string - models []string - mcpServers []string - agents []string + cache *bool + // personalGrant is the same grant as grants' own LAST element, + // whenever a personal grant is present (effectiveGroup, below) — kept + // as its own field, distinct from indexing into grants, so + // allowsPassthroughPathForProvider (below, HIGH-1 fix, review round + // 2) can tell "the personal grant" apart from "the last member + // group's grant" directly, rather than inferring it from + // len(grants) vs len(memberGroups). nil for an ordinary group and for + // a multi-group user with no personal grant alike. + personalGrant *grant + name string + providers []string + models []string + mcpServers []string + agents []string // passthroughPaths is GroupConfig.PassthroughPaths carried through // unchanged (security+performance audit, 2026-08-22) — see // allowsPassthroughPath below. passthroughPaths []string + // grants is the ordered list of (providers, models) authorization + // pairs this principal grants THROUGH — one per member group plus an + // optional personal grant appended last (effectiveGroup, below). nil + // for an ordinary, single-membership group with no personal grant + // (the overwhelmingly common case, including every group literal + // this package's own tests construct directly): allowsProviderModel + // (below) then falls back to treating grp itself — via its own + // providers/models fields above — as the sole implicit grant, so + // every existing group keeps behaving exactly as before this field + // existed. + grants []grant + // memberGroups is the list of actual, named *group values a + // synthetic multi-membership/personal-grant principal + // (effectiveGroup) was built from — nil for an ordinary group, which + // is its own sole member (memberScopeGroups, below, used by + // buildLimitScopes, routes_unified.go, for one limit scope per + // member group). + memberGroups []*group // cacheTTL is GroupConfig.CacheTTL parsed and validated at construction // (newAuthStore below): 0 inherits the global responseCache's TTL // (effectiveTTL, cache.go); a positive value sets the TTL written @@ -54,6 +89,18 @@ type group struct { cacheTTL time.Duration } +// grant is one (providers, models) authorization pair a principal (a +// *group, possibly synthetic — effectiveGroup, below) grants access +// through. A principal's authorization is the union of one grant per +// member group plus an optional personal grant, but a request must match +// a SINGLE grant's providers AND models — never a provider one grant +// allows combined with a model only some OTHER grant allows +// (allowsProviderModel, below). +type grant struct { + providers []string + models []string +} + // allowsProvider reports whether name matches one of the group's provider // glob patterns. An empty pattern list allows every provider. func (grp *group) allowsProvider(name string) bool { @@ -90,6 +137,158 @@ func (grp *group) hasModelRestriction() bool { return len(grp.models) > 0 } +// grantCount reports how many grants grp carries — len(grp.grants) for a +// synthetic multi-membership/personal-grant principal, or 1 for grp's +// own single-implicit-grant case (grp.grants nil) — WITHOUT allocating +// (review round 2, NIT fix): callers that only need to know "single +// grant or not" to pick a fast/slow path (bareEntryWinner, registry.go; +// failoverCandidates, failover.go) used to call +// len(grp.effectiveGrants()), which built and threw away a fresh +// []grant{...} on every call, including every ordinary single-group +// request — the overwhelmingly common case, on the request-path hot +// loop. +func (grp *group) grantCount() int { + if grp.grants != nil { + return len(grp.grants) + } + return 1 +} + +// allowsProviderModel reports whether SOME SINGLE grant of grp's own +// allows provider AND at least one of ids — the multi-group/personal- +// grant feature's "no cross-grant leak" rule: a provider one grant +// allows combined with a model only a DIFFERENT grant allows must never +// authorize a request neither grant alone would. For an ordinary group +// with exactly one implicit grant (the overwhelmingly common case, +// grp.grants nil), this is exactly the pre-existing combined "model glob +// matches AND provider glob matches" check every caller below switched +// to this method FROM — registry.go's resolveAgainst/listFor, +// routes_passthrough.go's allowsPassthroughModel, and failover.go's +// failoverCandidates for a multi-grant principal — specifically so +// single-grant behavior is preserved byte-for-byte while the multi-grant +// case gets this stricter, per-grant rule. +// +// The single-grant case is inlined directly against grp's own +// providers/models fields (review round 2, NIT fix), rather than +// wrapping them in a one-element []grant and looping that: this method +// runs at least once per request through every metered route +// (resolveAgainst, allowsPassthroughModel) and the wrapping slice was a +// real per-request heap allocation for every ordinary, single-group +// caller — the multi-grant loop below is unchanged and still used for +// grp.grants != nil. +func (grp *group) allowsProviderModel(provider string, ids ...string) bool { + if grp.grants == nil { + if !matchesGlob(grp.providers, provider) { + return false + } + for _, id := range ids { + if matchesGlob(grp.models, id) { + return true + } + } + return false + } + for _, g := range grp.grants { + if !matchesGlob(g.providers, provider) { + continue + } + for _, id := range ids { + if matchesGlob(g.models, id) { + return true + } + } + } + return false +} + +// memberScopeGroups returns the *group values buildLimitScopes (routes_ +// unified.go) builds one "group" limit scope per: grp.memberGroups for a +// synthetic multi-membership/personal-grant principal (effectiveGroup, +// below), or grp itself — its own sole member — for the overwhelmingly +// common ordinary-group case (memberGroups nil), preserving today's +// single "group" scope exactly. +func (grp *group) memberScopeGroups() []*group { + if grp.memberGroups != nil { + return grp.memberGroups + } + return []*group{grp} +} + +// writeLengthPrefixed appends s to b as ":" (a +// netstring — a well-known unambiguous, prefix-free encoding): a reader +// that always consumes digits up to the first ':' as a byte count, then +// consumes exactly that many further bytes as the value, can never +// misinterpret where one component ends and the next begins, regardless +// of what bytes s itself contains (including further digits or ':' +// characters) — unlike a fixed separator byte/string, which a value +// containing that exact separator can spoof. Used by modelsCacheKey, +// below (NIT fix, review round 3): a plain "\x00"-joined encoding with +// bare "p:"/"m:" marker literals let a group literally named "p:" (group +// names carry no character-set validation — effectiveGroup's own doc +// comment, above) produce a signature byte-for-byte identical to an +// unrelated (membership, personal grant) combination. +func writeLengthPrefixed(b *strings.Builder, s string) { + b.WriteString(strconv.Itoa(len(s))) + b.WriteByte(':') + b.WriteString(s) +} + +// modelsCacheKey returns grp's stable STRING key for modelRegistry's +// modelsCache (MEDIUM-3 fix, review round 2) — see modelsJSON's own doc +// comment (registry.go) for why a *group pointer is unsafe to key that +// cache on. A real, named group (grp.memberGroups nil) is keyed on its +// own name, netstring-encoded (writeLengthPrefixed, above) behind a "g" +// discriminator byte that can never collide with a synthetic key's own +// "s" discriminator, regardless of what an operator names a group. A +// synthetic principal (effectiveGroup) is keyed on its member group +// names, in EFFECTIVE ORDER — each length-prefixed, preceded by their own +// count, so the reader never needs a marker byte to know when the member +// list ends — followed by a single 'Y'/'N' byte for "personal grant +// present", and, when present, its own providers then models, each list +// as a count followed by that many length-prefixed entries. Every +// component of every list is length-prefixed (review round 3, NIT fix — +// the pre-fix version separated components with a bare "\x00" byte and +// bare "p:"/"m:" marker strings, both of which a sufficiently-adversarial +// group/provider/model NAME could itself contain, producing a genuine +// collision between two different, unrelated principals — see this +// function's own regression test, registry_test.go, for the exact +// reviewer-reported pair). Two DIFFERENT users with IDENTICAL membership +// therefore still share one cache entry — including the SAME principal +// rebuilt, as a brand-new *group, across a users-file hot reload +// (effectiveGroup allocates fresh every call) — closing the reload-driven +// unbounded growth MEDIUM-3 (review round 2) fixed, without reintroducing +// this round's own ambiguity. +func (grp *group) modelsCacheKey() string { + var b strings.Builder + if grp.memberGroups == nil { + b.WriteString("g") + writeLengthPrefixed(&b, grp.name) + return b.String() + } + b.WriteString("s") + b.WriteString(strconv.Itoa(len(grp.memberGroups))) + b.WriteByte(':') + for _, m := range grp.memberGroups { + writeLengthPrefixed(&b, m.name) + } + if grp.personalGrant == nil { + b.WriteByte('N') + return b.String() + } + b.WriteByte('Y') + b.WriteString(strconv.Itoa(len(grp.personalGrant.providers))) + b.WriteByte(':') + for _, p := range grp.personalGrant.providers { + writeLengthPrefixed(&b, p) + } + b.WriteString(strconv.Itoa(len(grp.personalGrant.models))) + b.WriteByte(':') + for _, mo := range grp.personalGrant.models { + writeLengthPrefixed(&b, mo) + } + return b.String() +} + // allowsMCP reports whether name matches one of the group's MCP-server glob // patterns. An empty pattern list allows every server. func (grp *group) allowsMCP(name string) bool { @@ -147,6 +346,135 @@ func (grp *group) allowsPassthroughPath(rest string) bool { return matchesGlob(grp.passthroughPaths, decoded) } +// personalGrantAllowsPath reports whether grp's personal grant (if any) +// authorizes path — the "personal grant borrows the primary group's own +// paths" rule (HIGH-1 fix, review round 2; extracted into its own, +// independently testable function in review round 3 so the rule cannot +// be silently collapsed to "always allow" without a dedicated test +// catching it — see auth_test.go's own mutation-checked test). The +// personal grant carries no passthroughPaths of its own (UserConfig has +// no such field on Providers/Models), so it borrows +// grp.memberGroups[0] — the PRIMARY member group, "first in effective +// order" (UserConfig.Group before Groups, effectiveGroupNames) — the +// same one that already governs every other provider this principal can +// reach without a more specific member group's own restriction. false +// when grp has no personal grant at all, or (defensively) no member +// groups to borrow from — every real caller with a non-nil +// personalGrant also has a non-empty memberGroups (effectiveGroup, below, +// never builds one without the other). +func (grp *group) personalGrantAllowsPath(path string) bool { + if grp.personalGrant == nil || len(grp.memberGroups) == 0 { + return false + } + return grp.memberGroups[0].allowsPassthroughPath(path) +} + +// allowsPassthroughPathForProvider reports whether grp authorizes a +// native passthrough request to provider AND path TOGETHER (HIGH-1 fix, +// review round 2). passthroughPaths is deliberately NOT unioned across +// member groups the way providers/models/mcpServers/agents are +// (unionOrAll, effectiveGroup below): unioning it let joining ANY +// further group silently strip a DIFFERENT member group's own path +// restriction — group "a" {providers:[alpha]}, no path restriction, +// joined with group "b" {providers:[beta], passthroughPaths:[v1/chat/ +// completions]}, would otherwise let "a"'s own unrestricted paths +// authorize "/beta/v1/files" even though NEITHER group alone ever +// granted that combination. +// +// The rule: some MEMBER group g has g.allowsProvider(provider) AND +// g.allowsPassthroughPath(path) — its OWN, un-merged path list — OR the +// personal grant (if any) allows provider (matchesGlob against its own +// providers; MEDIUM-5's constructor validation, buildEntry below, +// guarantees a personal grant's providers is never empty when the grant +// exists at all) AND personalGrantAllowsPath (above). +// +// This method alone does NOT couple the MODEL check to the same grant — +// see hasModelRestrictionForProviderPath/allowsPassthroughModelForPath +// below (HIGH fix, review round 3) for that: this function answers +// "is provider+path authorized at all", used both by a single-grant +// principal's own path gate (independent of its own, separately-checked +// model gate — safe with only one grant to combine from) and by a +// multi-grant principal's own path gate (handlePassthrough, +// routes_passthrough.go), which then re-derives which SPECIFIC grants +// matched provider+path before ever consulting a model. +// +// For an ordinary, single-grant group (grp.memberGroups nil), this +// collapses to grp itself as its own sole member (memberScopeGroups): +// grp.allowsProvider(provider) && grp.allowsPassthroughPath(path) — +// byte-identical to the two independent checks handlePassthrough +// (routes_passthrough.go) ran before this method existed. +func (grp *group) allowsPassthroughPathForProvider(provider, path string) bool { + for _, m := range grp.memberScopeGroups() { + if m.allowsProvider(provider) && m.allowsPassthroughPath(path) { + return true + } + } + if grp.personalGrant != nil && matchesGlob(grp.personalGrant.providers, provider) && grp.personalGrantAllowsPath(path) { + return true + } + return false +} + +// hasModelRestrictionForProviderPath reports whether EVERY grant that +// allows BOTH provider AND path also restricts models (a non-empty +// Models glob) — the HIGH fix's (review round 3) multi-grant, path- +// coupled counterpart to the single-grant hasModelRestriction() above: +// peeking the body is pointless once some grant that ALREADY covers this +// EXACT provider+path combination also grants blanket model access on +// it. Mirrors allowsPassthroughPathForProvider's own member-then-personal +// structure exactly (down to reusing personalGrantAllowsPath), so the +// two can never disagree about which grants "match" provider+path. +// +// Used only for a multi-grant principal (handlePassthrough, +// routes_passthrough.go, gates on grp.grantCount() > 1 itself) — a +// single-grant principal keeps using hasModelRestriction() directly, +// unchanged, since a lone grant can never combine with itself to produce +// the cross-grant leak this exists to prevent. +func (grp *group) hasModelRestrictionForProviderPath(provider, path string) bool { + for _, m := range grp.memberScopeGroups() { + if m.allowsProvider(provider) && m.allowsPassthroughPath(path) && len(m.models) == 0 { + return false + } + } + if grp.personalGrant != nil && matchesGlob(grp.personalGrant.providers, provider) && grp.personalGrantAllowsPath(path) && len(grp.personalGrant.models) == 0 { + return false + } + return true +} + +// allowsPassthroughModelForPath reports whether SOME SINGLE grant — a +// member group's own, or the personal grant's own (borrowing the +// primary's own paths via personalGrantAllowsPath) — authorizes provider +// AND path AND at least one of ids, ALL from that SAME grant (HIGH fix, +// review round 3): the defect P1/P2 reproduce is exactly a provider+path +// match from one grant combined with a model match from a DIFFERENT +// one — round 2's fix coupled provider+model per grant +// (allowsProviderModel) and provider+path per grant +// (allowsPassthroughPathForProvider) SEPARATELY, which still let the two +// checks succeed via two DIFFERENT grants for the same request. Used +// only for a multi-grant principal, mirroring +// hasModelRestrictionForProviderPath's own gating. +func (grp *group) allowsPassthroughModelForPath(provider, path string, ids ...string) bool { + for _, m := range grp.memberScopeGroups() { + if !m.allowsProvider(provider) || !m.allowsPassthroughPath(path) { + continue + } + for _, id := range ids { + if matchesGlob(m.models, id) { + return true + } + } + } + if grp.personalGrant != nil && matchesGlob(grp.personalGrant.providers, provider) && grp.personalGrantAllowsPath(path) { + for _, id := range ids { + if matchesGlob(grp.personalGrant.models, id) { + return true + } + } + } + return false +} + // cloneStringSlice returns an independent copy of s, preserving nil (a nil // s returns nil, never an empty non-nil slice — groupSummary's own // providers/models/mcpServers/agents fields rely on that nil-ness to stay @@ -470,9 +798,154 @@ func newAuthStore(cfg *Config) (*authStore, error) { return a, nil } -// buildEntry resolves uc's group and API key into an authEntry. The API key -// goes through resolveSecret before digesting, so env:/file: references -// work the same as for provider keys. +// unionOrAll returns the union of every list in lists' own glob patterns, +// EXCEPT when some list is itself empty: matchesGlob's own contract (this +// file) already treats an empty pattern list as "matches anything," so +// one empty list among several already allows everything on its own — +// the union must too, returning nil (matchesGlob's own empty fast path) +// rather than a literal concatenation of the OTHER lists, which would +// lose that all-allowing meaning entirely. Used by effectiveGroup, below, +// for a synthetic principal's providers/models/mcpServers/agents fields: +// "if ANY member's list is empty, the effective list is empty" is the +// identical rule for every one of them. NOT used for passthroughPaths +// (HIGH-1 fix, review round 2) — see allowsPassthroughPathForProvider's +// own doc comment above for why a union is actively wrong for that one +// field specifically: unlike providers/models/mcpServers/agents, +// passthroughPaths is scoped to a SPECIFIC provider, so merging it +// across member groups can strip a narrower member's own restriction the +// instant a second, broader member group is joined. +func unionOrAll(lists ...[]string) []string { + for _, l := range lists { + if len(l) == 0 { + return nil + } + } + var out []string + for _, l := range lists { + out = append(out, l...) + } + return out +} + +// effectiveGroupNames returns uc's de-duplicated, order-preserving member +// group name list: primary (UserConfig.Group) first when non-empty, then +// extra (UserConfig.Groups) in order, skipping any name already seen — +// UserConfig.Groups' own doc comment (llmgateway.go) has the full +// contract this implements. +func effectiveGroupNames(primary string, extra []string) []string { + seen := make(map[string]bool, len(extra)+1) + var names []string + if primary != "" { + seen[primary] = true + names = append(names, primary) + } + for _, name := range extra { + if name == "" || seen[name] { + continue + } + seen[name] = true + names = append(names, name) + } + return names +} + +// effectiveGroup returns the *group a user belonging to every group in +// members, plus an optional personal grant, is authorized through — +// UserConfig.Groups' own doc comment (llmgateway.go) has the full +// authorization/limits/cache contract this builds. +// +// Identity is preserved for the overwhelmingly common case — exactly one +// member group and no personal grant — by returning that member's own +// *group pointer completely unchanged: name, limit-scope id, every +// field, all identical to before this feature existed (multi-group +// back-compat rule). Every other case builds a fresh, synthetic *group: +// its name joins every member group's own name with "+", for display/log +// use only — this synthetic group is never looked up by name (grp.name's +// only reader outside tests, registry.go's modelsJSON, uses it purely to +// label a log line), and Config.Groups keys go through no character-set +// validation the way provider/mcpServers/agents names do +// (validateConfigName, providers.go), so a real, operator-chosen group +// name COULD textually collide with a synthetic one; that collision is +// harmless (an ambiguous-looking log line, never a lookup) precisely +// because nothing resolves a *group by this name. grants holds one +// (providers, models) pair per member plus the personal grant (if any) +// appended last (personalGrant, below, keeps that same last grant under +// its own name too, for allowsPassthroughPathForProvider's own use), and +// memberGroups keeps the actual member *group pointers, in EFFECTIVE +// ORDER (primary first), for memberScopeGroups' own per-member-group +// limit scope and for allowsPassthroughPathForProvider's own "primary +// group" rule. +// +// passthroughPaths is deliberately left at its zero value (nil) here, +// UNLIKE providers/models/mcpServers/agents: it is not a simple union +// (HIGH-1 fix, review round 2) — allowsPassthroughPathForProvider reads +// memberGroups/personalGrant directly instead, so a synthetic +// principal's own .passthroughPaths field would be dead, and +// potentially misleading, state if this function still populated it. +func effectiveGroup(members []*group, personal *grant) *group { + if len(members) == 1 && personal == nil { + return members[0] + } + + names := make([]string, len(members)) + grants := make([]grant, 0, len(members)+1) + var providerLists, modelLists, mcpLists, agentLists [][]string + var sawCacheFalse, sawCacheTrue bool + var minTTL time.Duration + for i, m := range members { + names[i] = m.name + grants = append(grants, grant{providers: m.providers, models: m.models}) + providerLists = append(providerLists, m.providers) + modelLists = append(modelLists, m.models) + mcpLists = append(mcpLists, m.mcpServers) + agentLists = append(agentLists, m.agents) + if m.cache != nil { + if *m.cache { + sawCacheTrue = true + } else { + sawCacheFalse = true + } + } + if m.cacheTTL > 0 && (minTTL == 0 || m.cacheTTL < minTTL) { + minTTL = m.cacheTTL + } + } + var personalGrant *grant + if personal != nil { + grants = append(grants, *personal) + providerLists = append(providerLists, personal.providers) + modelLists = append(modelLists, personal.models) + personalGrant = personal + } + + var cache *bool + switch { + case sawCacheFalse: + f := false + cache = &f + case sawCacheTrue: + t := true + cache = &t + } + + return &group{ + name: strings.Join(names, "+"), + providers: unionOrAll(providerLists...), + models: unionOrAll(modelLists...), + mcpServers: unionOrAll(mcpLists...), + agents: unionOrAll(agentLists...), + cache: cache, + cacheTTL: minTTL, + grants: grants, + memberGroups: members, + personalGrant: personalGrant, + } +} + +// buildEntry resolves uc's groups (Group/Groups, effectiveGroupNames), +// its optional personal grant (Providers/Models), and its API key into an +// authEntry. The API key goes through resolveSecret before digesting, so +// env:/file: references work the same as for provider keys. // // uc.Name must be non-empty (security audit finding 5, 2026-08-22): // counters are keyed on a user's name (windowKey's "kind:id:..." shape, @@ -500,10 +973,41 @@ func (a *authStore) buildEntry(uc *UserConfig) (*authEntry, error) { if uc.Name == "" { return nil, fmt.Errorf("llmgateway: user config entry must have a non-empty name") } - grp, ok := a.groups[uc.Group] - if !ok { - return nil, fmt.Errorf("llmgateway: user %q references unknown group %q", uc.Name, uc.Group) + groupNames := effectiveGroupNames(uc.Group, uc.Groups) + if len(groupNames) == 0 { + return nil, fmt.Errorf("llmgateway: user %q must belong to at least one group", uc.Name) } + members := make([]*group, len(groupNames)) + for i, name := range groupNames { + grp, ok := a.groups[name] + if !ok { + return nil, fmt.Errorf("llmgateway: user %q references unknown group %q", uc.Name, name) + } + members[i] = grp + } + // personal is uc's own Providers/Models grant — present only when at + // least one of the two lists is non-empty (UserConfig.Providers/ + // Models' own doc comment, llmgateway.go): an empty personal grant + // can never mean "allow everything" on its own. + // + // A MODELS-ONLY personal grant (Models set, Providers empty) is + // rejected here (MEDIUM-5 fix, review round 2): empty Providers means + // "any provider" (matchesGlob's own empty-means-all contract), so a + // models-only grant would silently reach every configured provider — + // including ones no member group of this user covers at all — and + // would pass allowsPassthroughPathForProvider's own provider gate for + // any provider too. A Providers-only grant (Models empty) is valid: + // it means "every model on those providers," the same empty-means-all + // contract applied the other way round. + var personal *grant + if len(uc.Providers) > 0 || len(uc.Models) > 0 { + if len(uc.Providers) == 0 { + return nil, fmt.Errorf("llmgateway: user %q: personal \"models\" requires personal \"providers\"", uc.Name) + } + personal = &grant{providers: uc.Providers, models: uc.Models} + } + effective := effectiveGroup(members, personal) + if err := uc.Limits.validate(); err != nil { return nil, fmt.Errorf("llmgateway: user %q: %w", uc.Name, err) } @@ -517,8 +1021,14 @@ func (a *authStore) buildEntry(uc *UserConfig) (*authEntry, error) { digest := sha256.Sum256([]byte(key)) return &authEntry{ digest: digest, - user: &user{limits: uc.Limits, name: uc.Name, groupName: uc.Group, admin: uc.Admin}, - group: grp, + user: &user{ + limits: uc.Limits, + groupNames: groupNames, + name: uc.Name, + groupName: groupNames[0], + admin: uc.Admin, + }, + group: effective, }, nil } @@ -733,6 +1243,10 @@ type userSummary struct { limits *LimitsConfig name string groupName string + // groups is the user's full, de-duplicated member group list + // (user.groupNames) — groupName above keeps only the first, for + // back-compat display (admin.go's adminUsageEntryView.GroupName). + groups []string } // groupSummary mirrors userSummary for one configured group, plus its @@ -770,8 +1284,21 @@ func (a *authStore) snapshot() ([]userSummary, []groupSummary) { users := make([]userSummary, 0, len(a.byDigest)) memberCounts := make(map[string]int, len(a.groups)) for _, entry := range a.byDigest { - users = append(users, userSummary{limits: entry.user.limits, name: entry.user.name, groupName: entry.user.groupName}) - memberCounts[entry.user.groupName]++ + users = append(users, userSummary{ + limits: entry.user.limits, + name: entry.user.name, + groupName: entry.user.groupName, + groups: cloneStringSlice(entry.user.groupNames), + }) + // A multi-group user counts once in EACH member group's own + // memberCount (UserConfig.Groups' own doc comment, llmgateway.go) + // — entry.user.groupNames is always non-empty (buildEntry rejects + // a user with none), so this is exactly one increment per member + // group, matching the single-group case's own one-increment + // behavior before this field existed. + for _, name := range entry.user.groupNames { + memberCounts[name]++ + } } a.mu.RUnlock() sort.Slice(users, func(i, j int) bool { return users[i].name < users[j].name }) diff --git a/auth_test.go b/auth_test.go index 39dba7d..6d38c91 100644 --- a/auth_test.go +++ b/auth_test.go @@ -136,6 +136,292 @@ func TestNewAuthStore_UnknownGroup_ReturnsError(t *testing.T) { } } +// TestNewAuthStore_UnknownGroupInGroupsList_ReturnsError proves an +// unknown group named in UserConfig.Groups (not just Group) is a +// constructor error naming the user and the unknown group, the same +// style as the pre-existing Group-alone check above. +func TestNewAuthStore_UnknownGroupInGroupsList_ReturnsError(t *testing.T) { + cfg := &Config{ + Providers: map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}}, + Groups: map[string]*GroupConfig{"eng": {}}, + Users: &UsersConfig{Inline: []*UserConfig{ + {Name: "a", Group: "eng", Groups: []string{"nonexistent"}, APIKey: "sk-secret"}, + }}, + } + _, err := newAuthStore(cfg) + if err == nil { + t.Fatal("want error for a user referencing an unknown group in Groups") + } + if !strings.Contains(err.Error(), `user "a"`) || !strings.Contains(err.Error(), `unknown group "nonexistent"`) { + t.Errorf("err = %v, want it to name the user and the unknown group", err) + } +} + +// TestNewAuthStore_UserWithNoGroupAtAll_ReturnsError proves a user naming +// neither Group nor Groups is a constructor error: at least one group is +// required. +func TestNewAuthStore_UserWithNoGroupAtAll_ReturnsError(t *testing.T) { + cfg := &Config{ + Providers: map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}}, + Groups: map[string]*GroupConfig{"eng": {}}, + Users: &UsersConfig{Inline: []*UserConfig{{Name: "a", APIKey: "sk-secret"}}}, + } + if _, err := newAuthStore(cfg); err == nil { + t.Fatal("want error for a user with no group at all") + } +} + +// TestBuildEntry_SingleGroupNoPersonalGrant_ReusesSharedGroupPointer is +// the multi-group feature's back-compat rule: a user belonging to exactly +// one group, with no personal Providers/Models grant, resolves to the +// EXACT SAME *group pointer newAuthStore already built for that group — +// identity preserved, not merely equal values. +func TestBuildEntry_SingleGroupNoPersonalGrant_ReusesSharedGroupPointer(t *testing.T) { + cfg := &Config{ + Providers: map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}}, + Groups: map[string]*GroupConfig{"eng": {Providers: []string{"openai"}}}, + Users: &UsersConfig{Inline: []*UserConfig{{Name: "a", Group: "eng", APIKey: "sk-secret"}}}, + } + a, err := newAuthStore(cfg) + if err != nil { + t.Fatal(err) + } + _, grp, ok := identifyWithKey(a, "sk-secret") + if !ok { + t.Fatal("identify failed") + } + if grp != a.groups["eng"] { + t.Error("want the exact shared *group pointer for a single-group user with no personal grant") + } +} + +// --- LOW (review round 3): personalGrantAllowsPath's own "borrow the +// primary group's paths" rule, isolated and mutation-checked --- + +// TestGroup_PersonalGrantAllowsPath_BorrowsPrimaryGroupPaths proves the +// personal grant's own path authorization borrows the PRIMARY member +// group's (memberGroups[0]) own passthroughPaths — never an unconditional +// allow, and never a DIFFERENT member group's list. +func TestGroup_PersonalGrantAllowsPath_BorrowsPrimaryGroupPaths(t *testing.T) { + primary := &group{name: "home", passthroughPaths: []string{"v1/chat/completions"}} + other := &group{name: "friends"} + grp := effectiveGroup([]*group{primary, other}, &grant{providers: []string{"beta"}}) + + if !grp.personalGrantAllowsPath("v1/chat/completions") { + t.Error("want the personal grant to allow the PRIMARY group's own allowed path") + } + if grp.personalGrantAllowsPath("v1/files") { + t.Error("want the personal grant denied a path the PRIMARY group does not allow — this must not silently become \"always allow\"") + } +} + +// TestGroup_PersonalGrantAllowsPath_NoPersonalGrant_ReturnsFalse proves +// the method is not vacuously true for a principal with no personal +// grant at all. +func TestGroup_PersonalGrantAllowsPath_NoPersonalGrant_ReturnsFalse(t *testing.T) { + grp := effectiveGroup([]*group{{name: "a"}, {name: "b"}}, nil) + if grp.personalGrantAllowsPath("anything") { + t.Error("want false: no personal grant exists to borrow a path for") + } +} + +// --- LOW-7 (review round 2): test-gap coverage for rules the suite +// stayed green about even after they were reverted --- + +// TestUnionOrAll_AnyEmptyList_AllowsAll pins unionOrAll's own contract: +// ONE empty list among several already means "matches anything" +// (matchesGlob's own empty-means-all rule), so the union of everything +// must too, regardless of what the OTHER lists contain. +func TestUnionOrAll_AnyEmptyList_AllowsAll(t *testing.T) { + if got := unionOrAll([]string{"a", "b"}, nil); got != nil { + t.Errorf("unionOrAll([a b], nil) = %v, want nil (one empty list allows everything)", got) + } + if got := unionOrAll([]string{"a"}, []string{}, []string{"c"}); got != nil { + t.Errorf("unionOrAll = %v, want nil (an empty list ANYWHERE in the input allows everything)", got) + } +} + +// TestUnionOrAll_NoEmptyLists_ConcatenatesEveryPattern proves the +// positive case: with no empty list among the inputs, the union is +// every pattern from every list, in order. +func TestUnionOrAll_NoEmptyLists_ConcatenatesEveryPattern(t *testing.T) { + got := unionOrAll([]string{"a", "b"}, []string{"c"}) + want := []string{"a", "b", "c"} + if len(got) != len(want) { + t.Fatalf("unionOrAll = %v, want %v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Errorf("unionOrAll = %v, want %v", got, want) + } + } +} + +// TestBuildLimitScopes_DuplicateGroupInGroupsList_CountedOnce proves +// effectiveGroupNames' de-duplication (auth.go) actually matters end to +// end: a user naming the SAME group via both Group and Groups collapses +// to a single-member principal — reusing the shared *group pointer +// (back-compat identity rule) — and buildLimitScopes/snapshot must count +// it exactly once, never twice. +func TestBuildLimitScopes_DuplicateGroupInGroupsList_CountedOnce(t *testing.T) { + cfg := &Config{ + Providers: map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}}, + Groups: map[string]*GroupConfig{"eng": {}}, + Users: &UsersConfig{Inline: []*UserConfig{ + {Name: "a", Group: "eng", Groups: []string{"eng"}, APIKey: "sk-secret"}, + }}, + } + a, err := newAuthStore(cfg) + if err != nil { + t.Fatal(err) + } + u, grp, ok := identifyWithKey(a, "sk-secret") + if !ok { + t.Fatal("identify failed") + } + if grp != a.groups["eng"] { + t.Error("want the exact shared *group pointer — a duplicate group name must collapse to a single-member principal") + } + + scopes := buildLimitScopes(u, grp) + if len(scopes) != 2 { + t.Fatalf("len(scopes) = %d, want 2 (1 user + 1 group — a group listed twice must not produce two scopes)", len(scopes)) + } + + _, groups := a.snapshot() + for _, g := range groups { + if g.name == "eng" && g.memberCount != 1 { + t.Errorf("eng memberCount = %d, want 1 (listing the same group twice must not double-count the user)", g.memberCount) + } + } +} + +// TestEffectiveGroup_MCPServersUnion_AnyEmptyMemberAllowsAll and +// TestEffectiveGroup_MCPServersUnion_BothRestricted_ConcatenatesPatterns +// (plus their Agents counterparts, below) pin mcpServers/agents' own +// union rule (effectiveGroup, auth.go) — LOW-7 test-gap coverage, +// mirroring providers/models' own already-covered union tests +// (TestModelRegistry_Resolve_MultiGroup_UnionAcrossMemberGroups) for the +// two fields that stay a plain union (unlike passthroughPaths, HIGH-1). +func TestEffectiveGroup_MCPServersUnion_AnyEmptyMemberAllowsAll(t *testing.T) { + a := &group{name: "a", mcpServers: []string{"search-*"}} + b := &group{name: "b"} // empty: allow-all + grp := effectiveGroup([]*group{a, b}, nil) + if !grp.allowsMCP("anything-at-all") { + t.Errorf("mcpServers = %v, want allow-all (member b's own empty list)", grp.mcpServers) + } +} + +func TestEffectiveGroup_MCPServersUnion_BothRestricted_ConcatenatesPatterns(t *testing.T) { + a := &group{name: "a", mcpServers: []string{"search-*"}} + b := &group{name: "b", mcpServers: []string{"db-admin"}} + grp := effectiveGroup([]*group{a, b}, nil) + if !grp.allowsMCP("search-web") || !grp.allowsMCP("db-admin") { + t.Errorf("want both search-web and db-admin allowed, mcpServers=%v", grp.mcpServers) + } + if grp.allowsMCP("other") { + t.Errorf("want \"other\" denied, mcpServers=%v", grp.mcpServers) + } +} + +func TestEffectiveGroup_AgentsUnion_AnyEmptyMemberAllowsAll(t *testing.T) { + a := &group{name: "a", agents: []string{"triage"}} + b := &group{name: "b"} // empty: allow-all + grp := effectiveGroup([]*group{a, b}, nil) + if !grp.allowsAgent("anything-at-all") { + t.Errorf("agents = %v, want allow-all (member b's own empty list)", grp.agents) + } +} + +func TestEffectiveGroup_AgentsUnion_BothRestricted_ConcatenatesPatterns(t *testing.T) { + a := &group{name: "a", agents: []string{"triage"}} + b := &group{name: "b", agents: []string{"support"}} + grp := effectiveGroup([]*group{a, b}, nil) + if !grp.allowsAgent("triage") || !grp.allowsAgent("support") { + t.Errorf("want both triage and support allowed, agents=%v", grp.agents) + } + if grp.allowsAgent("other") { + t.Errorf("want \"other\" denied, agents=%v", grp.agents) + } +} + +// TestNewAuthStore_ModelsOnlyPersonalGrant_ReturnsConstructorError is +// MEDIUM-5's regression (review round 2): a personal grant with Models +// set but Providers left empty means "any provider" (matchesGlob's own +// empty-means-all contract) combined with a model restriction — which +// silently reaches EVERY configured provider, not just the ones the user's +// group(s) already cover, and passes the passthrough provider gate for +// any provider too. This must be rejected at construction, naming the +// user, rather than silently granted. +func TestNewAuthStore_ModelsOnlyPersonalGrant_ReturnsConstructorError(t *testing.T) { + cfg := &Config{ + Providers: map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}}, + Groups: map[string]*GroupConfig{"eng": {}}, + Users: &UsersConfig{Inline: []*UserConfig{ + {Name: "a", Group: "eng", APIKey: "sk-secret", Models: []string{"gpt-x"}}, + }}, + } + _, err := newAuthStore(cfg) + if err == nil { + t.Fatal("want a constructor error for a models-only personal grant (no personal providers)") + } + if !strings.Contains(err.Error(), `user "a"`) { + t.Errorf("err = %v, want it to name the user", err) + } +} + +// TestNewAuthStore_ProvidersOnlyPersonalGrant_OK is the positive control: +// a personal grant with Providers set and Models left empty is valid — +// it means "every model on those providers," never a construction error. +func TestNewAuthStore_ProvidersOnlyPersonalGrant_OK(t *testing.T) { + cfg := &Config{ + Providers: map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}}, + Groups: map[string]*GroupConfig{"eng": {}}, + Users: &UsersConfig{Inline: []*UserConfig{ + {Name: "a", Group: "eng", APIKey: "sk-secret", Providers: []string{"gx10"}}, + }}, + } + if _, err := newAuthStore(cfg); err != nil { + t.Fatalf("newAuthStore: %v, want no error for a providers-only personal grant", err) + } +} + +// TestAuthStore_Snapshot_MultiGroupUser_ListsAllGroupsAndCountsEachMember +// proves the admin-snapshot side of multi-group support: a user's own +// summary lists every member group (not just the first, which stays for +// back-compat), and each member group's memberCount includes that user +// once — see UserConfig.Groups' own doc comment (llmgateway.go). +func TestAuthStore_Snapshot_MultiGroupUser_ListsAllGroupsAndCountsEachMember(t *testing.T) { + cfg := &Config{ + Providers: map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}}, + Groups: map[string]*GroupConfig{"eng": {}, "ops": {}}, + Users: &UsersConfig{Inline: []*UserConfig{ + {Name: "carol", Group: "eng", Groups: []string{"ops"}, APIKey: "sk-carol"}, + }}, + } + a, err := newAuthStore(cfg) + if err != nil { + t.Fatal(err) + } + users, groups := a.snapshot() + if len(users) != 1 { + t.Fatalf("users = %v, want 1", users) + } + if users[0].groupName != "eng" { + t.Errorf("groupName = %q, want %q (first group, back-compat)", users[0].groupName, "eng") + } + if len(users[0].groups) != 2 || users[0].groups[0] != "eng" || users[0].groups[1] != "ops" { + t.Errorf("groups = %v, want [eng ops]", users[0].groups) + } + counts := map[string]int{} + for _, g := range groups { + counts[g.name] = g.memberCount + } + if counts["eng"] != 1 || counts["ops"] != 1 { + t.Errorf("memberCounts = %v, want eng=1 ops=1", counts) + } +} + func TestNewAuthStore_DuplicateAPIKey_ReturnsError(t *testing.T) { cfg := &Config{ Providers: map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}}, diff --git a/cache_test.go b/cache_test.go index 01fc530..b454a6c 100644 --- a/cache_test.go +++ b/cache_test.go @@ -656,6 +656,60 @@ func TestEffectiveTTL_NilInheritsGlobal(t *testing.T) { // TestEffectiveTTL_GroupOverrideHonored proves a group's own resolved // cacheTTL wins over the cache's global TTL. +// --- effectiveGroup: multi-group cache merge rule --- + +// TestEffectiveGroup_CacheMerge_FalseWinsOverTrue proves the multi-group +// cache-merge rule (UserConfig.Groups' own doc comment, llmgateway.go): +// if ANY member group sets cache false, the synthetic principal's own +// effective cache setting is false, even when another member sets it +// true. +func TestEffectiveGroup_CacheMerge_FalseWinsOverTrue(t *testing.T) { + no, yes := false, true + a := &group{name: "a", cache: &yes} + b := &group{name: "b", cache: &no} + grp := effectiveGroup([]*group{a, b}, nil) + if groupCacheEnabled(grp) { + t.Error("groupCacheEnabled = true, want false (one member group opts out)") + } +} + +// TestEffectiveGroup_CacheMerge_TrueWinsOverNil proves true wins when no +// member group sets false but at least one sets true. +func TestEffectiveGroup_CacheMerge_TrueWinsOverNil(t *testing.T) { + yes := true + a := &group{name: "a", cache: &yes} + b := &group{name: "b"} // nil: inherit + grp := effectiveGroup([]*group{a, b}, nil) + if !groupCacheEnabled(grp) { + t.Error("groupCacheEnabled = false, want true (one member group opts in, none opts out)") + } +} + +// TestEffectiveGroup_CacheMerge_AllNilInherits proves nil-inherit stays +// nil-inherit when no member group sets Cache at all. +func TestEffectiveGroup_CacheMerge_AllNilInherits(t *testing.T) { + a := &group{name: "a"} + b := &group{name: "b"} + grp := effectiveGroup([]*group{a, b}, nil) + if grp.cache != nil { + t.Errorf("grp.cache = %v, want nil (no member group sets it)", grp.cache) + } +} + +// TestEffectiveGroup_CacheTTLMerge_SmallestPositiveWins proves the +// multi-group cacheTTL-merge rule: the SMALLEST positive cacheTTL among +// member groups wins; a member that leaves cacheTTL unset (0) must never +// win as "smallest". +func TestEffectiveGroup_CacheTTLMerge_SmallestPositiveWins(t *testing.T) { + a := &group{name: "a", cacheTTL: 30 * time.Second} + b := &group{name: "b", cacheTTL: 5 * time.Minute} + c := &group{name: "c"} // 0: unset + grp := effectiveGroup([]*group{a, b, c}, nil) + if grp.cacheTTL != 30*time.Second { + t.Errorf("grp.cacheTTL = %v, want 30s (the smallest positive among member groups)", grp.cacheTTL) + } +} + func TestEffectiveTTL_GroupOverrideHonored(t *testing.T) { c := &responseCache{ttl: 5 * time.Minute} grp := &group{name: "g", cacheTTL: 30 * time.Second} diff --git a/failover.go b/failover.go index 08c9498..c04daa7 100644 --- a/failover.go +++ b/failover.go @@ -156,16 +156,28 @@ func (m *modelRegistry) resolveWithCandidates(id string, grp *group) (resolveCan primary := resolveCandidate{adapter: adapter, providerName: adapter.name(), upstreamModel: upstreamModel, canonical: canonical} bareID := id + // extraID is id's own alias name, when id resolved via one (LOW-6 + // fix, review round 2) — empty otherwise. Passed through to + // failoverCandidates below so a multi-grant principal's per-candidate + // check sees the SAME extra id candidate resolveAgainst itself + // checked to resolve the primary (resolveAliasTarget's own + // extraModelName): without it, a candidate provider reachable ONLY + // via a grant that names the alias itself — not the bare target id, + // nor "provider/bareID" — was wrongly excluded from the pool even + // though the primary resolution just succeeded through that exact + // same grant. + extraID := "" if target, isAlias := m.aliases[id]; isAlias { if _, _, ok := m.splitConfiguredProvider(target); ok { return primary, nil, nil } bareID = target + extraID = id } else if _, _, ok := m.splitConfiguredProvider(id); ok { return primary, nil, nil } - return primary, m.failoverCandidates(bareID, primary.providerName, grp), nil + return primary, m.failoverCandidates(bareID, extraID, primary.providerName, grp), nil } // resolvePrimaryOnly resolves id exactly like resolveWithCandidates' @@ -191,20 +203,49 @@ func (m *modelRegistry) resolvePrimaryOnly(id string, grp *group) (resolveCandid // raw candidate pool orderedFailoverCandidates (below) narrows and // reorders further by health. // -// Only grp.allowsProvider is re-checked per candidate: grp.allowsModel is -// already known true from the primary winner's own successful resolve -// call, and — for the identical bare id string every candidate here -// shares by construction — allowsModel's result does not vary by -// provider (auth.go's allowsModel takes no provider argument), so -// re-checking it per candidate would be redundant work, not a +// Only grp.allowsProvider is re-checked per candidate FOR A SINGLE-GRANT +// PRINCIPAL (grp.grantCount() == 1 — the overwhelmingly common case, +// unchanged from before the multi-group/personal-grant feature existed): +// grp.allowsModel is already known true from the primary winner's own +// successful resolve call, and — for the identical bare id string every +// candidate here shares by construction — allowsModel's result does not +// vary by provider (auth.go's allowsModel takes no provider argument), +// so re-checking it per candidate would be redundant work, not a // correctness gap. -func (m *modelRegistry) failoverCandidates(bareID, primaryProvider string, grp *group) []resolveCandidate { +// +// A MULTI-GRANT principal (more than one member group and/or a personal +// grant) cannot use that shortcut: grp.allowsProvider(name) is a UNION +// across every grant (auth.go's effectiveGroup), so it can report true +// for a candidate whose OWN grant does not authorize this bareID at +// all — the exact no-cross-grant-leak rule allowsProviderModel (auth.go) +// exists to enforce. For that case, every candidate is checked via +// allowsProviderModel(name, bareID, name+"/"+bareID, extraID) instead — +// the same candidate shape resolveAgainst (registry.go) checks for a +// direct request. extraID is resolveWithCandidates' own alias name (LOW-6 +// fix, review round 2) — empty for a non-alias resolution — passed +// through unconditionally rather than only in the multi-grant branch, +// so a future single-grant caller that starts needing it never has to +// remember to add it; matchesGlob(nil-or-non-nil, "") is simply never +// true against a real glob pattern, so an empty extraID here is a no-op. +func (m *modelRegistry) failoverCandidates(bareID, extraID, primaryProvider string, grp *group) []resolveCandidate { var extra []resolveCandidate + multiGrant := grp.grantCount() > 1 for _, name := range m.providerNames { if name == primaryProvider { continue } - if !m.states[name].hasModel(bareID) || !grp.allowsProvider(name) { + if !m.states[name].hasModel(bareID) { + continue + } + if multiGrant { + ids := []string{bareID, name + "/" + bareID} + if extraID != "" { + ids = append(ids, extraID) + } + if !grp.allowsProviderModel(name, ids...) { + continue + } + } else if !grp.allowsProvider(name) { continue } // canonical assigned to a local before the struct literal, matching diff --git a/failover_test.go b/failover_test.go index f893d19..db2ed26 100644 --- a/failover_test.go +++ b/failover_test.go @@ -160,6 +160,105 @@ func TestModelRegistry_ResolveWithCandidates_ResolveError_PropagatesUnchanged(t assert.Nil(t, extra) } +// TestModelRegistry_FailoverCandidates_MultiGrant_CrossGrantLeakDenied +// proves feat/failover's own per-candidate check for a multi-grant +// principal (multi-group and/or personal grant): a candidate provider +// reachable ONLY through a grant that does not itself authorize the +// requested bare model must never be offered as a failover candidate, +// even though a DIFFERENT grant of the same principal allows some OTHER +// provider entirely — a naive provider-only check (grp.allowsProvider +// would see "beta" as true via the union) must not be used for a +// multi-grant principal. +func TestModelRegistry_FailoverCandidates_MultiGrant_CrossGrantLeakDenied(t *testing.T) { + t.Parallel() + adapters := map[string]providerAdapter{ + "alpha": newFakeAdapter("alpha"), + "beta": newFakeAdapter("beta"), + } + cfg := &Config{Providers: map[string]*ProviderConfig{ + "alpha": {Models: []string{"shared"}}, + "beta": {Models: []string{"shared"}}, + }} + reg, err := newModelRegistry(adapters, cfg, func(string, ...any) {}) + require.NoError(t, err) + + member := &group{name: "alpha-friends", providers: []string{"alpha"}} + grp := effectiveGroup([]*group{member}, &grant{providers: []string{"beta"}, models: []string{"completely-different"}}) + + primary, extra, err := reg.resolveWithCandidates("shared", grp) + require.NoError(t, err) + assert.Equal(t, "alpha", primary.providerName) + for _, c := range extra { + assert.NotEqual(t, "beta", c.providerName, "beta is reachable only through a grant whose own models list denies \"shared\"; it must never be offered as a failover candidate despite the principal's OTHER grant allowing a different provider") + } +} + +// TestModelRegistry_FailoverCandidates_MultiGrant_IncludesAliasName is +// LOW-6's regression (review round 2): resolveWithCandidates resolved +// "aliased" (-> bare target "shared") via member group "eng"'s own +// direct match on "shared" — but the personal grant's own reach to +// "beta" is authorized ONLY via the alias's own name ("aliased" itself, +// not "shared" or "beta/shared"), exactly the extra candidate id +// resolveAgainst itself would check for an alias target +// (resolveAliasTarget's own extraModelName). Without passing that same +// id through to failoverCandidates, beta is wrongly excluded from the +// candidate pool even though the principal's own personal grant +// genuinely authorizes it via the alias. +func TestModelRegistry_FailoverCandidates_MultiGrant_IncludesAliasName(t *testing.T) { + t.Parallel() + adapters := map[string]providerAdapter{ + "alpha": newFakeAdapter("alpha"), + "beta": newFakeAdapter("beta"), + } + cfg := &Config{ + Providers: map[string]*ProviderConfig{ + "alpha": {Models: []string{"shared"}}, + "beta": {Models: []string{"shared"}}, + }, + ModelAliases: map[string]string{"aliased": "shared"}, + } + reg, err := newModelRegistry(adapters, cfg, func(string, ...any) {}) + require.NoError(t, err) + + eng := &group{name: "eng", providers: []string{"alpha"}, models: []string{"shared"}} + grp := effectiveGroup([]*group{eng}, &grant{providers: []string{"beta"}, models: []string{"aliased"}}) + + primary, extra, err := reg.resolveWithCandidates("aliased", grp) + require.NoError(t, err) + assert.Equal(t, "alpha", primary.providerName, "primary must resolve via eng's own direct match on the bare target") + + names := make([]string, len(extra)) + for i, c := range extra { + names[i] = c.providerName + } + assert.Contains(t, names, "beta", "beta is reachable via the personal grant's own alias-name authorization and must be offered as a failover candidate") +} + +// TestModelRegistry_FailoverCandidates_MultiGrant_MatchingGrantAllowed is +// the positive control for the test above: a candidate provider whose +// OWN grant allows both it and the model IS offered. +func TestModelRegistry_FailoverCandidates_MultiGrant_MatchingGrantAllowed(t *testing.T) { + t.Parallel() + adapters := map[string]providerAdapter{ + "alpha": newFakeAdapter("alpha"), + "beta": newFakeAdapter("beta"), + } + cfg := &Config{Providers: map[string]*ProviderConfig{ + "alpha": {Models: []string{"shared"}}, + "beta": {Models: []string{"shared"}}, + }} + reg, err := newModelRegistry(adapters, cfg, func(string, ...any) {}) + require.NoError(t, err) + + member := &group{name: "alpha-friends", providers: []string{"alpha"}} + grp := effectiveGroup([]*group{member}, &grant{providers: []string{"beta"}, models: []string{"beta/shared"}}) + + _, extra, err := reg.resolveWithCandidates("shared", grp) + require.NoError(t, err) + require.Len(t, extra, 1) + assert.Equal(t, "beta", extra[0].providerName) +} + // --- requestHealthTracker --- func TestRequestHealthTracker_HealthyByDefault(t *testing.T) { diff --git a/llmgateway.go b/llmgateway.go index 939c566..4c8b290 100644 --- a/llmgateway.go +++ b/llmgateway.go @@ -267,7 +267,70 @@ type UserConfig struct { Limits *LimitsConfig `json:"limits,omitempty"` Name string `json:"name"` Group string `json:"group"` - APIKey string `json:"apiKey"` + // Groups lists further groups this user belongs to, alongside Group + // (multi-group support). Effective membership is Group first (when + // non-empty), then Groups, de-duplicated, order preserved + // (effectiveGroupNames, auth.go) — at least one group, from either + // field, is required: a user naming none at all is a constructor + // error. Every named group, whether from Group or Groups, must + // reference a configured group — an unknown one is a constructor + // error naming the user and the unknown group, the same + // "llmgateway: user %q references unknown group %q" shape Group + // alone always used. + // + // A multi-group user's effective authorization is the UNION of every + // member group's own grant (effectiveGroup, auth.go): a request is + // allowed when it matches some SINGLE member group's providers AND + // models — providers from one member group never combine with models + // only some OTHER member group allows to authorize a request neither + // alone would (no cross-grant leak; see Providers/Models below, which + // carry the identical rule for the personal grant). Each member + // group's own limits apply as ITS OWN limit scope (kind "group", id + // that group's own name) — never merged into one combined limit. + // mcpServers/agents are unioned across every member group, with the + // rule that any ONE member group's empty (allow-all) list makes the + // union allow-all too. passthroughPaths is NOT unioned this way — a + // native passthrough request is authorized only when ONE grant (a + // member group's own, or the personal grant, which has none of its + // own and so borrows the PRIMARY group's — Group, or Groups[0] when + // Group is empty) covers the provider AND the path AND (when + // restricted) the model TOGETHER; unioning it the same way as the + // other lists would let joining a further, less-restricted group + // silently strip a more specific member group's own path restriction + // on a provider that other group does not even reach. The effective + // cache setting is false if any member group sets cache false, else + // true if any sets true, else nil (inherit); cacheTTL is the smallest + // positive cacheTTL configured among member groups. + // + // A user belonging to exactly one group (Group alone, Groups empty) + // with no personal grant below behaves byte-identically to before + // this field existed, down to *group pointer identity + // (effectiveGroup's own back-compat rule, auth.go). + Groups []string `json:"groups,omitempty"` + APIKey string `json:"apiKey"` + // Providers and Models together describe this user's own PERSONAL + // grant — access beyond whatever their member group(s) already allow, + // with the identical glob semantics a group's own Providers/Models + // carry (matchesGlob, auth.go: an empty list matches anything). The + // personal grant exists at all ONLY when Providers is non-empty: + // Models alone, with Providers left empty, is a constructor error + // naming the user (`personal "models" requires personal "providers"`) + // — an empty Providers list means "any provider" (matchesGlob's own + // empty-means-all contract), so a Models-only grant would silently + // reach every configured provider, including ones no member group of + // this user covers at all. Providers alone (Models left empty) is + // valid and means "every model on those providers." Leaving both + // empty/omitted (the default) means this user has no personal grant + // beyond their member group(s)'. Like a member group's own grant, the + // personal grant participates in the "some SINGLE grant must allow + // provider AND model" rule Groups' own doc comment above states — and, + // for native passthrough specifically, the identical rule extended to + // a provider+path+model triple, borrowing the PRIMARY group's own + // passthroughPaths (Groups' own doc comment above): it has no limits + // of its own, and contributes to a request's authorization only + // through those combined checks, never as a separate limit scope. + Providers []string `json:"providers,omitempty"` + Models []string `json:"models,omitempty"` // Admin grants this user access to the read-only admin dashboard // (spec §4, v0.2), whether the user is inline or file-sourced — // file-users granting admin is operator-controlled via the Secret diff --git a/registry.go b/registry.go index bc92208..c94b8fc 100644 --- a/registry.go +++ b/registry.go @@ -582,9 +582,11 @@ type modelRegistry struct { // modelsCache holds one encoded GET /v1/models response body per // group, tagged with the modelsGen it was built from (perf finding 3, // 2026-08-2x audit: 15.9ms/21.3MB/552,884 allocs per uncached call at - // the live 1,033-model catalog) — see modelsJSON's own doc comment. - // Guarded by modelsCacheMu. - modelsCache map[*group]modelsCacheEntry + // the live 1,033-model catalog) — see modelsJSON's own doc comment, + // including MEDIUM-3's (review round 2) own key-shape correction: + // keyed by group.modelsCacheKey()'s stable STRING signature, not a + // *group pointer. Guarded by modelsCacheMu. + modelsCache map[string]modelsCacheEntry providerNames []string // modelsGen counts how many times finishRefresh (below) has recorded a // discovery attempt for ANY provider — bumped unconditionally, success @@ -691,7 +693,7 @@ func newModelRegistry(adapters map[string]providerAdapter, cfg *Config, log func nowFn: time.Now, warned: make(map[string]bool), providerNames: make([]string, 0, len(adapters)), - modelsCache: make(map[*group]modelsCacheEntry), + modelsCache: make(map[string]modelsCacheEntry), } for name := range adapters { m.providerNames = append(m.providerNames, name) @@ -1087,6 +1089,48 @@ func (m *modelRegistry) splitConfiguredProvider(id string) (providerName, rest s return "", "", false } +// bareEntryWinner returns the first provider in owners (already sorted +// and already PROVIDER-authorized for grp — bareWinner's own "visible", +// or listFor's identical "visible") that grp additionally authorizes id +// (plus any further candidate resolveAgainst would also check for this +// specific call site, e.g. resolveAliasTarget's own alias name) ON — +// HIGH-2 fix, review round 2. Shared by bareWinner (below) and listFor's +// own bare-entry selection, so the two can never drift into picking a +// different winner for the identical scenario. +// +// For a single-grant grp (grp.grantCount() <= 1 — the overwhelmingly +// common case), this is exactly owners[0]: a single grant's own provider +// authorization and model authorization are independent axes, already +// checked SEPARATELY by every caller (resolveAgainst's own model check; +// listFor's own grp.allowsProviderModel check on the returned winner), so +// this never needs to consult id/extraIDs for that case at all — the +// identical, unconditional "first provider-visible owner" bareWinner +// always returned before this fix existed. +// +// For a MULTI-grant grp, owners[0] alone is not good enough: it might be +// authorized for PROVIDER ONLY by one grant while a LATER owner in the +// same list is authorized for BOTH provider and model by a DIFFERENT +// grant. Before this fix, both bareWinner and listFor picked owners[0] +// unconditionally and let the caller's own separate model check deny it +// — silently taking away a model the principal could actually reach via +// a later, still-visible owner. ok is false only when NO owner's own +// grant covers both provider and (any of) id/extraIDs. +func bareEntryWinner(owners []string, grp *group, id string, extraIDs ...string) (string, bool) { + if len(owners) == 0 { + return "", false + } + if grp.grantCount() <= 1 { + return owners[0], true + } + ids := append([]string{id}, extraIDs...) + for _, name := range owners { + if grp.allowsProviderModel(name, ids...) { + return name, true + } + } + return "", false +} + // bareWinner returns the provider that owns bare id when more than one // provider's known model set contains it: the first, in sorted // provider-name order, among providers grp is authorized to use (ruling @@ -1094,34 +1138,59 @@ func (m *modelRegistry) splitConfiguredProvider(id string) (providerName, rest s // always return the GLOBAL sorted-first owning provider, so a caller was // denied a model they were actually entitled to whenever some OTHER, // group-invisible provider happened to sort earlier and also served that -// id). grp.allowsModel is NOT consulted here — only allowsProvider; model- -// level authorization still happens once, in resolveAgainst, against -// whichever provider this returns. +// id). // -// When grp allows none of id's owning providers, this still returns the -// GLOBAL sorted-first owner with ok true, exactly as the pre-fix version -// always did, rather than reporting "not found": id is still a genuinely -// known model, just not one this group may reach, and the caller +// MULTI-GRANT (HIGH-2 fix, review round 2): for a principal with more +// than one grant, "authorized to use" additionally requires the SAME +// grant to also cover id (plus extraIDs, the caller's own further +// resolveAgainst candidate — e.g. resolveAliasTarget passes its alias +// name), via the shared bareEntryWinner helper above — not merely the +// coarser grp.allowsProvider union check a single-grant principal still +// uses. Before this fix, a multi-grant principal's bareWinner could pick +// a provider visible only through a DIFFERENT grant than the one that +// actually covers this model, which resolveAgainst then denied — even +// when a LATER owning provider, further down the sorted list, was fully +// authorized by one of the principal's own grants. Consulting the +// eventual model check up front avoids that false deny. Single-grant +// principals are unaffected: bareEntryWinner's own fast path keeps this +// byte-identical to the original, provider-only preference. +// +// When grp allows none of id's owning providers at all (no grant even +// matches the PROVIDER for any owner), this still returns the GLOBAL +// sorted-first owner with ok true, exactly as the pre-fix version always +// did, rather than reporting "not found": id is still a genuinely known +// model, just not one this group may reach, and the caller // (resolveAgainst) must see it as known-but-denied (errModelDenied) — // the same outcome grp got before this fix — not the wrong -// errModelUnknown a "no visible owner" result would produce. -// -// Single pass, same O(providerNames) bound as before: the fallback and the -// grp-preferred search share one loop instead of two. -func (m *modelRegistry) bareWinner(id string, grp *group) (string, bool) { - fallback, fallbackOK := "", false +// errModelUnknown a "no visible owner" result would produce. The same +// fallback covers the multi-grant case where grp allows the PROVIDER for +// one or more owners but no single grant covers both that provider and +// this model: bareEntryWinner reports no winner, so this falls back to +// the first PROVIDER-visible owner instead (visible[0]) — +// resolveAgainst will independently re-derive errModelDenied against it, +// so the eventual error is unaffected, only which provider it is denied +// against. +func (m *modelRegistry) bareWinner(id string, grp *group, extraIDs ...string) (string, bool) { + var owners, visible []string for _, name := range m.providerNames { if !m.states[name].hasModel(id) { continue } - if !fallbackOK { - fallback, fallbackOK = name, true - } + owners = append(owners, name) if grp.allowsProvider(name) { - return name, true + visible = append(visible, name) } } - return fallback, fallbackOK + if len(owners) == 0 { + return "", false + } + if len(visible) == 0 { + return owners[0], true + } + if winner, ok := bareEntryWinner(visible, grp, id, extraIDs...); ok { + return winner, true + } + return visible[0], true } // resolve maps a client-requested model id to the adapter that serves it. @@ -1224,7 +1293,14 @@ func (m *modelRegistry) resolveAliasTarget(alias, target string, grp *group) (pr if providerName, rest, ok := m.splitConfiguredProvider(target); ok { return m.resolveAgainst(providerName, rest, target, alias, grp, notFound) } - providerName, ok := m.bareWinner(target, grp) + // alias passed through as bareWinner's own extraIDs (HIGH-2 fix, + // review round 2): the SAME extraModelName candidate resolveAgainst + // itself checks two lines below — a multi-grant principal whose only + // grant reaching some owning provider does so via the alias's own + // name (not target's bare form) must still be preferred over a + // grant-mismatched, merely PROVIDER-visible owner earlier in sorted + // order. + providerName, ok := m.bareWinner(target, grp, alias) if !ok { return nil, "", "", notFound } @@ -1307,11 +1383,17 @@ func (m *modelRegistry) resolveAgainst(providerName, upstreamModel, requestedID, if !m.states[providerName].hasModel(upstreamModel) { return nil, "", "", notFoundErr } - allowed := grp.allowsModel(requestedID) || grp.allowsModel(upstreamModel) + // allowsProviderModel (multi-group/personal-grant feature, auth.go) + // requires provider AND model to both come from the SAME grant — the + // no-cross-grant-leak rule. For an ordinary single-grant group this + // is byte-identical to the combined "model matches (requestedID OR + // upstreamModel OR extraModelName) AND provider matches" check this + // replaced. + ids := []string{requestedID, upstreamModel} if extraModelName != "" { - allowed = allowed || grp.allowsModel(extraModelName) + ids = append(ids, extraModelName) } - if !allowed || !grp.allowsProvider(providerName) { + if !grp.allowsProviderModel(providerName, ids...) { return nil, "", "", errModelDenied } // canonical is assigned to a local before the return, not inlined into @@ -1385,18 +1467,31 @@ func (m *modelRegistry) warnf(msg string) { // the identical cached bytes without re-walking every provider's model // set or re-marshaling. // -// Cached per *group pointer, not by name or any other derived key: -// authStore builds every group once, at construction (auth.go's -// newAuthStore), and never rebuilds or replaces that map afterward — a -// config reload constructs an entirely new Gateway (and so a new -// modelRegistry with its own empty modelsCache), so a *group pointer here -// can never alias a different group's catalog, either within one -// registry's lifetime or across a reload. +// Cached by grp.modelsCacheKey()'s stable STRING signature (MEDIUM-3 +// fix, review round 2), NOT by *group pointer identity: that assumption +// held for every ordinary, named group — authStore builds those once, at +// construction (auth.go's newAuthStore), and never rebuilds or replaces +// them afterward, so a config reload (which constructs an entirely new +// Gateway, and so a new modelRegistry with its own empty modelsCache) was +// never actually the risk. It broke for a SYNTHETIC multi-group/ +// personal-grant principal (effectiveGroup, auth.go): that function +// allocates a FRESH *group on every call, so authStore.replaceFileUsers +// (users_file.go's hot-reload path) rebuilds a semantically identical +// principal as a brand-new pointer on every reload — a *group-pointer +// key left the OLD pointer's entry permanently unreachable, growing this +// cache without bound across repeated reloads even when nothing about +// the user's membership changed (reviewer-reproduced: 5 identical +// reloads of one two-group user left 6 entries). group.modelsCacheKey() +// derives a signature from what actually determines this cache's VALUE +// (member group names plus the personal grant's own providers/models), +// so two principals with identical membership — including the SAME +// principal rebuilt across reloads — always share one entry. func (m *modelRegistry) modelsJSON(grp *group) []byte { gen := atomic.LoadInt64(&m.modelsGen) + key := grp.modelsCacheKey() m.modelsCacheMu.Lock() - if entry, ok := m.modelsCache[grp]; ok && entry.gen == gen { + if entry, ok := m.modelsCache[key]; ok && entry.gen == gen { m.modelsCacheMu.Unlock() return entry.body } @@ -1423,7 +1518,7 @@ func (m *modelRegistry) modelsJSON(grp *group) []byte { body = append(body, '\n') m.modelsCacheMu.Lock() - m.modelsCache[grp] = modelsCacheEntry{gen: gen, body: body} + m.modelsCache[key] = modelsCacheEntry{gen: gen, body: body} m.modelsCacheMu.Unlock() return body } @@ -1494,8 +1589,24 @@ func (m *modelRegistry) listFor(grp *group) []map[string]any { // address it once the alias owns the bare id. _, shadowed := aliasEntries[id] - groupWinner := visible[0] - if grp.allowsModel(id) && !shadowed { + // bareEntryWinner (HIGH-2 fix, review round 2), the same shared + // helper bareWinner uses: for a single-grant grp this is exactly + // visible[0] (today's pre-fix behavior); for a multi-grant grp it + // skips a merely PROVIDER-visible owner whose OWN grant does not + // also cover id, in favor of a later visible owner whose grant + // covers both — never silently dropping a model the principal can + // actually reach through some other visible provider. + groupWinner, gwOK := bareEntryWinner(visible, grp, id) + // allowsProviderModel (multi-group/personal-grant feature, + // auth.go), checked against groupWinner specifically — the + // no-cross-grant-leak rule: a grant that allows some OTHER + // visible provider must never authorize THIS entry, which is + // attributed to groupWinner alone. Redundant with + // bareEntryWinner's own multi-grant check when gwOK came from + // that branch (harmless — matchesGlob is cheap), but still + // REQUIRED for the single-grant fast path, which never consults + // id at all. + if gwOK && grp.allowsProviderModel(groupWinner, id) && !shadowed { out = append(out, modelObject(id, groupWinner, m.resolveMetaFor(groupWinner, id))) } if len(visible) < 2 && !shadowed { @@ -1505,10 +1616,10 @@ func (m *modelRegistry) listFor(grp *group) []map[string]any { pid := p + "/" + id // allowsModel does no prefix-stripping (auth.go): check both the // prefixed form and its bare suffix, same as resolveAgainst does - // for the equivalent client request. allowsProvider(p) is already - // established by p's membership in visible, so it is not - // re-checked per candidate here. - if grp.allowsModel(pid) || grp.allowsModel(id) { + // for the equivalent client request — allowsProviderModel(p, ...) + // requires BOTH candidates come from a grant that also allows p + // itself (no cross-grant leak). + if grp.allowsProviderModel(p, pid, id) { out = append(out, modelObject(pid, p, m.resolveMetaFor(p, id))) } } diff --git a/registry_test.go b/registry_test.go index 6eb8799..3ca8b30 100644 --- a/registry_test.go +++ b/registry_test.go @@ -295,6 +295,221 @@ func TestModelRegistry_Resolve_ProviderPrefixed_ModelGlobMatchesViaBareCandidate } } +// --- resolve: multi-group / personal-grant authorization --- + +// TestModelRegistry_Resolve_MultiGroup_UnionAcrossMemberGroups proves a +// multi-group principal (effectiveGroup, auth.go) is authorized through +// EITHER member group's own grant — the union rule. +func TestModelRegistry_Resolve_MultiGroup_UnionAcrossMemberGroups(t *testing.T) { + t.Parallel() + adapters := map[string]providerAdapter{ + "minimax": newFakeAdapter("minimax"), + "gx10": newFakeAdapter("gx10"), + } + cfg := &Config{Providers: map[string]*ProviderConfig{ + "minimax": {Models: []string{"m1"}}, + "gx10": {Models: []string{"only"}}, + }} + reg, err := newModelRegistry(adapters, cfg, func(string, ...any) {}) + if err != nil { + t.Fatalf("newModelRegistry: %v", err) + } + + a := &group{name: "minimax-friends", providers: []string{"minimax"}} + b := &group{name: "gx10-only", providers: []string{"gx10"}, models: []string{"gx10/only"}} + grp := effectiveGroup([]*group{a, b}, nil) + + if _, _, _, err := reg.resolve("minimax/m1", grp); err != nil { + t.Errorf("resolve minimax/m1: %v, want success via member group a's unrestricted-model grant", err) + } + if _, _, _, err := reg.resolve("gx10/only", grp); err != nil { + t.Errorf("resolve gx10/only: %v, want success via member group b's own grant", err) + } +} + +// TestModelRegistry_Resolve_MultiGroup_CrossGrantLeakDenied is the +// feature's core safety rule: grant A {providers:[minimax], models:[]} +// (models unrestricted, but only within minimax) must never combine with +// grant B {providers:[gx10], models:[gx10/only]} to authorize +// "gx10/other" — a naive "any grant allows the provider AND any grant +// allows the model" check would wrongly allow it, since A's own empty +// Models list matches anything on its own. +func TestModelRegistry_Resolve_MultiGroup_CrossGrantLeakDenied(t *testing.T) { + t.Parallel() + adapters := map[string]providerAdapter{ + "minimax": newFakeAdapter("minimax"), + "gx10": newFakeAdapter("gx10"), + } + cfg := &Config{Providers: map[string]*ProviderConfig{ + "minimax": {Models: []string{"m1"}}, + "gx10": {Models: []string{"only", "other"}}, + }} + reg, err := newModelRegistry(adapters, cfg, func(string, ...any) {}) + if err != nil { + t.Fatalf("newModelRegistry: %v", err) + } + + a := &group{name: "minimax-unrestricted", providers: []string{"minimax"}} + b := &group{name: "gx10-only", providers: []string{"gx10"}, models: []string{"gx10/only"}} + grp := effectiveGroup([]*group{a, b}, nil) + + if _, _, _, err := reg.resolve("gx10/only", grp); err != nil { + t.Errorf("resolve gx10/only: %v, want success", err) + } + if _, _, _, err := reg.resolve("gx10/other", grp); err != errModelDenied { + t.Errorf("resolve gx10/other: err = %v, want errModelDenied (no cross-grant leak from A's unrestricted models)", err) + } +} + +// TestModelRegistry_Resolve_PersonalProvidersGrant_AddsProviderKeepsGroupIntact +// covers a personal Providers-only grant layered on top of a group: group +// friends{providers:[minimax,uni]} plus a personal grant of +// providers:[gx10] must allow gx10 in addition to minimax/uni, unchanged, +// and deny every other provider. +func TestModelRegistry_Resolve_PersonalProvidersGrant_AddsProviderKeepsGroupIntact(t *testing.T) { + t.Parallel() + adapters := map[string]providerAdapter{ + "minimax": newFakeAdapter("minimax"), + "uni": newFakeAdapter("uni"), + "gx10": newFakeAdapter("gx10"), + "other": newFakeAdapter("other"), + } + cfg := &Config{Providers: map[string]*ProviderConfig{ + "minimax": {Models: []string{"m1"}}, + "uni": {Models: []string{"u1"}}, + "gx10": {Models: []string{"g1"}}, + "other": {Models: []string{"o1"}}, + }} + reg, err := newModelRegistry(adapters, cfg, func(string, ...any) {}) + if err != nil { + t.Fatalf("newModelRegistry: %v", err) + } + + friends := &group{name: "friends", providers: []string{"minimax", "uni"}} + grp := effectiveGroup([]*group{friends}, &grant{providers: []string{"gx10"}}) + + for _, id := range []string{"minimax/m1", "uni/u1", "gx10/g1"} { + if _, _, _, err := reg.resolve(id, grp); err != nil { + t.Errorf("resolve %s: %v, want success", id, err) + } + } + if _, _, _, err := reg.resolve("other/o1", grp); err != errModelDenied { + t.Errorf("resolve other/o1: err = %v, want errModelDenied (personal grant only adds gx10)", err) + } +} + +// TestModelRegistry_Resolve_PersonalModelsOnlyGrant_ExactModelAllowedOthersDenied +// covers a personal Models-only grant: models:["gx10/GLM-5.3-Flash-EXL3"] +// allows exactly that id and denies "gx10/other", independent of the +// user's own group's (unrelated) model restriction. +func TestModelRegistry_Resolve_PersonalModelsOnlyGrant_ExactModelAllowedOthersDenied(t *testing.T) { + t.Parallel() + adapters := map[string]providerAdapter{"gx10": newFakeAdapter("gx10")} + cfg := &Config{Providers: map[string]*ProviderConfig{ + "gx10": {Models: []string{"GLM-5.3-Flash-EXL3", "other"}}, + }} + reg, err := newModelRegistry(adapters, cfg, func(string, ...any) {}) + if err != nil { + t.Fatalf("newModelRegistry: %v", err) + } + + base := &group{name: "base", providers: []string{"gx10"}, models: []string{"nothing-else"}} + grp := effectiveGroup([]*group{base}, &grant{models: []string{"gx10/GLM-5.3-Flash-EXL3"}}) + + if _, _, _, err := reg.resolve("gx10/GLM-5.3-Flash-EXL3", grp); err != nil { + t.Errorf("resolve gx10/GLM-5.3-Flash-EXL3: %v, want success via personal grant", err) + } + if _, _, _, err := reg.resolve("gx10/other", grp); err != errModelDenied { + t.Errorf("resolve gx10/other: err = %v, want errModelDenied", err) + } +} + +// TestModelRegistry_ListFor_MultiGroup_ShowsUnionOfMemberGroupAndPersonalGrant +// proves GET /v1/models (listFor) shows exactly the union: a member +// group's own unrestricted-model provider, and the personal grant's own +// restricted model, with everything the personal grant does not list +// absent. +func TestModelRegistry_ListFor_MultiGroup_ShowsUnionOfMemberGroupAndPersonalGrant(t *testing.T) { + t.Parallel() + adapters := map[string]providerAdapter{ + "minimax": newFakeAdapter("minimax"), + "gx10": newFakeAdapter("gx10"), + } + cfg := &Config{Providers: map[string]*ProviderConfig{ + "minimax": {Models: []string{"m1"}}, + "gx10": {Models: []string{"only", "other"}}, + }} + reg, err := newModelRegistry(adapters, cfg, func(string, ...any) {}) + if err != nil { + t.Fatalf("newModelRegistry: %v", err) + } + + member := &group{name: "minimax-friends", providers: []string{"minimax"}} + grp := effectiveGroup([]*group{member}, &grant{providers: []string{"gx10"}, models: []string{"only"}}) + + ids := map[string]bool{} + for _, entry := range reg.listFor(grp) { + ids[entry["id"].(string)] = true //nolint:forcetypeassert // modelObject always sets id to a string + } + if !ids["m1"] { + t.Errorf("listFor ids = %v, want \"m1\" present (member group's unrestricted grant on minimax)", ids) + } + if !ids["only"] { + t.Errorf("listFor ids = %v, want \"only\" present (personal grant)", ids) + } + if ids["other"] { + t.Errorf("listFor ids = %v, want \"other\" absent (personal grant restricts gx10 to \"only\")", ids) + } +} + +// TestModelRegistry_Resolve_MultiGrant_BareWinnerPrefersGrantThatActuallyAuthorizes +// is HIGH-2's regression (review round 2): eng{providers:[beta]} alone +// already fully authorizes bare "llama" via beta (unrestricted models). +// Adding a personal grant restricted to alpha/special must NEVER take +// that away — a union-based bareWinner picks alpha first (visible via +// the personal grant's own provider entry) and then denies it (alpha's +// grant doesn't cover "llama"), even though beta, right behind it in +// sorted order, is fully authorized by eng's own grant. bareWinner (and +// listFor's identical bare-entry selection) must skip alpha and land on +// beta instead. +func TestModelRegistry_Resolve_MultiGrant_BareWinnerPrefersGrantThatActuallyAuthorizes(t *testing.T) { + t.Parallel() + adapters := map[string]providerAdapter{ + "alpha": newFakeAdapter("alpha"), + "beta": newFakeAdapter("beta"), + } + cfg := &Config{Providers: map[string]*ProviderConfig{ + "alpha": {Models: []string{"llama", "special"}}, + "beta": {Models: []string{"llama"}}, + }} + reg, err := newModelRegistry(adapters, cfg, func(string, ...any) {}) + if err != nil { + t.Fatalf("newModelRegistry: %v", err) + } + + eng := &group{name: "eng", providers: []string{"beta"}} + grp := effectiveGroup([]*group{eng}, &grant{providers: []string{"alpha"}, models: []string{"alpha/special"}}) + + adapter, upstreamModel, canonical, err := reg.resolve("llama", grp) + if err != nil { + t.Fatalf("resolve(llama): %v, want success via eng's own unrestricted-model grant on beta", err) + } + if adapter != adapters["beta"] { + t.Errorf("adapter = %v, want beta's adapter", adapter) + } + if upstreamModel != "llama" || canonical != "beta/llama" { + t.Errorf("upstreamModel/canonical = %q/%q, want \"llama\"/\"beta/llama\"", upstreamModel, canonical) + } + + ids := map[string]bool{} + for _, e := range reg.listFor(grp) { + ids[e["id"].(string)] = true + } + if !ids["llama"] { + t.Errorf("listFor ids = %v, want bare \"llama\" still listed (via eng's own grant on beta)", ids) + } +} + // --- resolve: collision precedence (ruling g) --- func TestModelRegistry_Resolve_BareIDCollision_PicksSortedFirstProvider(t *testing.T) { @@ -1167,6 +1382,123 @@ func TestModelRegistry_ModelsJSON_PerGroupIsolation(t *testing.T) { } } +// TestGroup_ModelsCacheKey_NoAmbiguityBetweenMemberNamesAndPersonalGrantMarkers +// is the round-3 NIT regression: the cache key's own "p:"/"m:" markers +// used to be bare string literals inside the joined \x00-separated +// signature, so a member group literally NAMED "p:" or "m:" (group names +// carry no character-set validation — effectiveGroup's own doc comment, +// auth.go) could produce a signature IDENTICAL to an entirely different +// (membership, personal grant) combination: members=[A,"p:","x","m:"] +// (no personal grant) collided byte-for-byte with members=[A] plus a +// personal grant of providers:["x"] — both joined to +// "s:A\x00p:\x00x\x00m:". Both must now get distinct keys. +func TestGroup_ModelsCacheKey_NoAmbiguityBetweenMemberNamesAndPersonalGrantMarkers(t *testing.T) { + t.Parallel() + a := &group{name: "A"} + pColon := &group{name: "p:"} + x := &group{name: "x"} + mColon := &group{name: "m:"} + + ambiguousMembers := effectiveGroup([]*group{a, pColon, x, mColon}, nil) + ambiguousPersonal := effectiveGroup([]*group{a}, &grant{providers: []string{"x"}}) + + keyMembers := ambiguousMembers.modelsCacheKey() + keyPersonal := ambiguousPersonal.modelsCacheKey() + if keyMembers == keyPersonal { + t.Errorf("modelsCacheKey collision: members=[A,p:,x,m:] (no personal) and members=[A]+personal{providers:[x]} both produced %q", keyMembers) + } +} + +// TestGroup_ModelsCacheKey_LengthPrefixPreventsSplitPointAmbiguity is the +// round-4 regression: the test above already distinguishes its two +// signatures by member COUNT alone (4 vs 1), so it stays green even if +// writeLengthPrefixed's own length prefix were removed entirely — the +// count prefix ahead of the member list would still differ. This test +// isolates the length prefix's OWN job with two adversarial pairs: +// +// - members=["ab","c"] vs members=["a","bc"]: both concatenate to the +// IDENTICAL "abc" once a split point moves, so with NO separator at +// all (raw concatenation), both would produce "s2:abcN". +// - members=["a:b","c"] vs members=["a","b:c"] (round-6 regression): +// both concatenate to "a:b:c" — this pair specifically catches a +// WEAKER mutation than "no separator at all": writeLengthPrefixed +// keeping ONLY a bare ":" separator, with the length DIGITS dropped, +// would still produce ":a:b" + ":c" = ":a:b:c" for the first and +// ":a" + ":b:c" = ":a:b:c" for the second — identical +// ("s2::a:b:cN" either way) — because a bare ":" is not +// self-delimiting the way ":" is; it does not tell a reader +// where the CURRENT component ends if the component's own value can +// itself contain ":". +// +// Only writeLengthPrefixed's own length-then-colon-then-value encoding +// (a netstring) keeps every pair apart: "2:ab1:c" is never confusable +// with "1:a2:bc", and "3:a:b1:c" is never confusable with "1:a3:b:c", +// regardless of what either component contains. +func TestGroup_ModelsCacheKey_LengthPrefixPreventsSplitPointAmbiguity(t *testing.T) { + t.Parallel() + splitAbC := effectiveGroup([]*group{{name: "ab"}, {name: "c"}}, nil) + splitABc := effectiveGroup([]*group{{name: "a"}, {name: "bc"}}, nil) + + keyAbC := splitAbC.modelsCacheKey() + keyABc := splitABc.modelsCacheKey() + if keyAbC == keyABc { + t.Errorf("modelsCacheKey collision: members=[ab,c] and members=[a,bc] (same count, concatenation \"abc\" either way) both produced %q", keyAbC) + } + + splitAColonBC := effectiveGroup([]*group{{name: "a:b"}, {name: "c"}}, nil) + splitABColonC := effectiveGroup([]*group{{name: "a"}, {name: "b:c"}}, nil) + + keyAColonBC := splitAColonBC.modelsCacheKey() + keyABColonC := splitABColonC.modelsCacheKey() + if keyAColonBC == keyABColonC { + t.Errorf("modelsCacheKey collision: members=[a:b,c] and members=[a,b:c] (same count, concatenation \"a:b:c\" either way) both produced %q", keyAColonBC) + } +} + +// TestModelRegistry_ModelsJSON_MultiGroupReload_CacheStaysBounded is +// MEDIUM-3's regression (review round 2): effectiveGroup (auth.go) +// allocates a FRESH *group for every multi-group/personal-grant +// principal, so a *group-pointer-keyed modelsCache grows without bound +// across repeated, semantically IDENTICAL users-file reloads (each +// reload rebuilds every user's principal from scratch, even when nothing +// changed). Five identical reloads of the same two-member-group user +// must leave the cache no larger than the number of DISTINCT principals +// actually queried (here: one multi-group user + one single-group user = +// 2 stable cache keys), not one entry per reload. +func TestModelRegistry_ModelsJSON_MultiGroupReload_CacheStaysBounded(t *testing.T) { + a, err := newAuthStore(&Config{Groups: map[string]*GroupConfig{"eng": {}, "ops": {}}}) + if err != nil { + t.Fatal(err) + } + reg, err := newModelRegistry(map[string]providerAdapter{"alpha": newFakeAdapter("alpha")}, + &Config{Providers: map[string]*ProviderConfig{"alpha": {Models: []string{"m1"}}}}, func(string, ...any) {}) + if err != nil { + t.Fatalf("newModelRegistry: %v", err) + } + + for i := 0; i < 5; i++ { + if err := a.replaceFileUsers([]*UserConfig{ + {Name: "carol", Group: "eng", Groups: []string{"ops"}, APIKey: "sk-carol"}, + {Name: "dave", Group: "eng", APIKey: "sk-dave"}, + }); err != nil { + t.Fatalf("replaceFileUsers (reload %d): %v", i, err) + } + for _, key := range []string{"sk-carol", "sk-dave"} { + _, grp, ok := identifyWithKey(a, key) + if !ok { + t.Fatalf("identify failed for %q on reload %d", key, i) + } + if body := reg.modelsJSON(grp); len(body) == 0 { + t.Fatalf("modelsJSON returned an empty body on reload %d", i) + } + } + } + + if got := len(reg.modelsCache); got > 2 { + t.Errorf("modelsCache entries after 5 identical reloads = %d, want at most 2 (one per DISTINCT principal — carol's multi-group principal must share one cache entry across reloads, not grow one per reload)", got) + } +} + // TestModelRegistry_ModelsJSON_MatchesUncachedListFor proves modelsJSON's // output is byte-identical to the OLD, pre-cache call site // (json.NewEncoder(w).Encode(...), routes_unified.go before perf finding diff --git a/routes_passthrough.go b/routes_passthrough.go index dd99abb..28ca634 100644 --- a/routes_passthrough.go +++ b/routes_passthrough.go @@ -488,9 +488,11 @@ func extractPassthroughUsage(typeName, providerName string, body []byte) (usage, // body, up to 32MiB, into memory before forwarding it). // // Callers only invoke this once they have already decided enforcement is -// required (handlePassthrough checks group.hasModelRestriction() first) — -// this function itself has no fail-open/fail-closed opinion; hasModel -// simply reports whether a non-empty top-level "model" STRING was found +// required (handlePassthrough checks group.hasModelRestriction — +// single-grant — or group.hasModelRestrictionForProviderPath — +// multi-grant — first) — this function itself has no fail-open/ +// fail-closed opinion; hasModel simply reports whether a non-empty +// top-level "model" STRING was found // within the peek window. It is skipped, at no cost — no read at all — // for a Content-Type isPassthroughBinaryContentType recognizes as // genuinely binary (multipart uploads, audio/image/video, octet-stream): @@ -713,7 +715,61 @@ func (g *Gateway) allowsPassthroughModel(grp *group, providerName, model string) if _, rest, ok := g.registry.splitConfiguredProvider(model); ok { bareModel = rest } - return grp.allowsModel(model) || grp.allowsModel(bareModel) || grp.allowsModel(providerName+"/"+bareModel) + // allowsProviderModel (multi-group/personal-grant feature, auth.go) + // requires providerName AND one of the three model candidates to come + // from the SAME grant — the no-cross-grant-leak rule. For an ordinary + // single-grant group this is byte-identical to the combined + // provider-then-model check this replaced (handlePassthrough already + // gated on grp.allowsProvider(providerName) before ever calling + // here). + return grp.allowsProviderModel(providerName, model, bareModel, providerName+"/"+bareModel) +} + +// allowsPassthroughModelForPath is allowsPassthroughModel's multi-grant, +// path-coupled counterpart (HIGH fix, review round 3): the SAME three +// model candidates, but checked via group.allowsPassthroughModelForPath +// — which requires provider, path, AND the model to all come from ONE +// grant — rather than group.allowsProviderModel, which knows nothing +// about path at all. Used only when grp.grantCount() > 1 +// (handlePassthrough, below); a single-grant principal keeps calling +// allowsPassthroughModel, unchanged. +func (g *Gateway) allowsPassthroughModelForPath(grp *group, providerName, path, model string) bool { + bareModel := model + if _, rest, ok := g.registry.splitConfiguredProvider(model); ok { + bareModel = rest + } + return grp.allowsPassthroughModelForPath(providerName, path, model, bareModel, providerName+"/"+bareModel) +} + +// peekAndValidateModel runs peekPassthroughModel and its two fail-closed +// checks (truncated, no model found), writing the matching 400/403 error +// itself on failure — shared by handlePassthrough's single-grant and +// multi-grant (HIGH fix, review round 3) branches so the two can never +// drift into different error bodies for the identical failure. ok is +// false whenever an error was already written and the caller must return +// immediately without doing anything else. +func (g *Gateway) peekAndValidateModel(w http.ResponseWriter, r *http.Request) (model string, ok bool) { + model, hasModel, truncated, err := peekPassthroughModel(r) + if err != nil { + writeOAIError(w, http.StatusBadRequest, "invalid_request_error", "cannot read request body") + return "", false + } + // Checked BEFORE hasModel (round-3 coordinator ruling, 2026-08-22): + // a truncated peek means a "model" found within the window cannot + // be trusted — a duplicate top-level "model" beyond + // maxModelPeekBytes may still exist and would win upstream + // (last-wins parsing) — so this must fail closed regardless of + // whatever hasModel/model peekPassthroughModel also returned. See + // peekPassthroughModel's own doc comment for the full mechanism. + if truncated { + writeOAIError(w, http.StatusForbidden, "invalid_request_error", "request body exceeds the model-enforcement window; a top-level model field beyond it cannot be safely authorized") + return "", false + } + if !hasModel { + writeOAIError(w, http.StatusForbidden, "invalid_request_error", "model could not be determined") + return "", false + } + return model, true } // handlePassthrough implements the native provider passthrough route: @@ -725,24 +781,47 @@ func (g *Gateway) allowsPassthroughModel(grp *group, providerName, model string) // in the gateway's own error envelope the way the unified routes wrap a // providerHTTPError. // -// Checks run in this order: group authorization — provider (403), model -// when the group restricts models at all (403), path allowlist (403) — -// before capability checks (Upgrade→501, path validity→400) before rate -// limits (429/503) — a caller who cannot use providerName/model/path at -// all learns that first, rather than learning something about how they -// tried to use it. The provider-level Passthrough toggle (ProviderConfig, -// llmgateway.go) is checked earlier still, by ServeHTTP's own route gate -// — a disabled provider never reaches this function at all, reported as -// the ordinary unknown-route 404 instead. +// Checks run in this order: group authorization — provider (403), then +// EITHER model-then-path OR path-then-model (see below), each check +// itself a 403 — before capability checks (Upgrade→501, path +// validity→400) before rate limits (429/503) — a caller who cannot use +// providerName/model/path at all learns that first, rather than learning +// something about how they tried to use it. The provider-level +// Passthrough toggle (ProviderConfig, llmgateway.go) is checked earlier +// still, by ServeHTTP's own route gate — a disabled provider never +// reaches this function at all, reported as the ordinary unknown-route +// 404 instead. +// +// ORDER DIFFERS BY GRANT COUNT (HIGH fix, review round 3): a +// SINGLE-grant principal (grp.grantCount() == 1, the overwhelmingly +// common case) keeps this feature's original order — model (when +// restricted at all) THEN path, checked INDEPENDENTLY of each other, +// exactly as before multi-group support existed; with only one grant to +// combine from, independent checks can never disagree about which grant +// authorized what. A MULTI-grant principal checks path FIRST, then +// model, and BOTH must be satisfied by the SAME grant — see the +// MODEL+PATH FROM ONE GRANT paragraph below for why, and why the order +// flips for that case specifically (cheaper to fail on path before ever +// peeking a body). // // MODEL ENFORCEMENT (security review, 2026-08-22, round 2 — closes a -// round-1 bypass): enforcement runs ONLY for a group with a non-empty -// Models list (grp.hasModelRestriction) — a group that has not opted into -// model restriction has nothing allowsModel could reject, so the request -// body is never even read, exactly as before this feature existed at all. -// For a restricted group, peekPassthroughModel reads (bounded, +// round-1 bypass): for a SINGLE-grant principal, enforcement runs ONLY +// when that one grant restricts models (grp.hasModelRestriction, auth.go +// — for an ordinary single-grant group this is exactly "a non-empty +// Models list"). For a MULTI-grant principal, enforcement runs +// ONLY when EVERY grant that allows THIS EXACT provider+path combination +// also restricts models (grp.hasModelRestrictionForProviderPath, auth.go +// — the HIGH fix's own per-provider-AND-path scoping, review round 3; +// see the MODEL+PATH FROM ONE GRANT paragraph below for why provider +// alone is no longer the right scope once a principal can carry more +// than one grant). Either way, a grant with nothing allowsProviderModel/ +// allowsPassthroughModelForPath could ever reject means the request body +// is never even read, exactly as before this feature, and before the +// multi-group/personal-grant feature, existed at all. For a restricted +// principal, peekPassthroughModel reads (bounded, // maxModelPeekBytes) for a top-level "model" field and this function -// checks it via allowsPassthroughModel — the round-1 version instead +// checks it via allowsPassthroughModel (single-grant) or +// allowsPassthroughModelForPath (multi-grant) — the round-1 version instead // gated the READ ITSELF on Content-Type containing "application/json", // which a client fully controls: sending "text/plain", an unexpected // value, or no Content-Type at all skipped the check outright and let @@ -779,11 +858,40 @@ func (g *Gateway) allowsPassthroughModel(grp *group, providerName, model string) // // PATH ALLOWLIST (same review): GroupConfig.PassthroughPaths, when // non-empty, additionally restricts which rest path this group's -// passthrough requests may address (grp.allowsPassthroughPath). Empty -// (the default, matchesGlob's own empty-means-all contract) allows every -// path, exactly as before this field existed — see its own doc comment -// for a path.Match footgun this allow-list inherits (a literal "*" -// pattern does NOT mean "allow everything"). +// passthrough requests may address +// (grp.allowsPassthroughPathForProvider, which couples the path check to +// the SAME grant that authorizes providerName — HIGH-1 fix, review round +// 2: passthroughPaths is not simply unioned across a multi-group +// principal's member groups, so joining a further, less-restricted group +// can never strip a more specific one's own path restriction on a +// DIFFERENT provider). Empty (the default, matchesGlob's own +// empty-means-all contract) allows every path, exactly as before this +// field existed — see its own doc comment for a path.Match footgun this +// allow-list inherits (a literal "*" pattern does NOT mean "allow +// everything"). +// +// MODEL+PATH FROM ONE GRANT (HIGH fix, review round 3 — closes a round-2 +// residual gap): round 2 coupled provider+model per grant +// (group.allowsProviderModel) and, separately, provider+path per grant +// (group.allowsPassthroughPathForProvider) — but nothing required BOTH +// checks to succeed via the SAME grant. A multi-grant principal with one +// grant that restricts models but not paths, and another that restricts +// paths but not models, could satisfy the path check via the second +// grant's own unrestricted models and the (irrelevant) model check via +// the first grant's own unrestricted path — authorizing a provider+path+ +// model combination NEITHER grant alone ever granted. Fixed by branching +// on grp.grantCount(): a MULTI-grant principal (> 1) checks path FIRST +// (group.allowsPassthroughPathForProvider, unchanged), then — only when +// EVERY grant that allows that exact provider+path also restricts +// models (group.hasModelRestrictionForProviderPath) — peeks the body and +// checks the model via group.allowsPassthroughModelForPath, which +// re-derives the SAME provider+path-matching grant set and requires one +// of THOSE to also allow the model. A SINGLE-grant principal (the +// overwhelmingly common case, grp.grantCount() == 1) keeps the exact +// pre-round-3 code, order, and error messages: model (if grp. +// hasModelRestriction) then path — with only one grant to combine +// from, the two checks can never disagree about which grant authorized +// what, so there is nothing to couple. // // For a Gemini provider, rest is appended to base() exactly as the client // sent it: there is no model extraction or URL rewriting here, so a @@ -797,36 +905,46 @@ func (g *Gateway) handlePassthrough(w http.ResponseWriter, r *http.Request, u *u return } - if grp.hasModelRestriction() { - model, hasModel, truncated, err := peekPassthroughModel(r) - if err != nil { - writeOAIError(w, http.StatusBadRequest, "invalid_request_error", "cannot read request body") + if grp.grantCount() > 1 { + // Multi-grant principal (HIGH fix, review round 3): path checked + // BEFORE model — cheaper than peeking a body, and it lets + // hasModelRestrictionForProviderPath/allowsPassthroughModelForPath + // below scope themselves to exactly the grants that already + // matched provider+path, never a grant that only matched provider. + if !grp.allowsPassthroughPathForProvider(providerName, rest) { + writeOAIError(w, http.StatusForbidden, "invalid_request_error", "path access denied") return } - // Checked BEFORE hasModel (round-3 coordinator ruling, 2026-08-22): - // a truncated peek means a "model" found within the window cannot - // be trusted — a duplicate top-level "model" beyond - // maxModelPeekBytes may still exist and would win upstream - // (last-wins parsing) — so this must fail closed regardless of - // whatever hasModel/model peekPassthroughModel also returned. See - // peekPassthroughModel's own doc comment for the full mechanism. - if truncated { - writeOAIError(w, http.StatusForbidden, "invalid_request_error", "request body exceeds the model-enforcement window; a top-level model field beyond it cannot be safely authorized") - return + if grp.hasModelRestrictionForProviderPath(providerName, rest) { + model, ok := g.peekAndValidateModel(w, r) + if !ok { + return + } + if !g.allowsPassthroughModelForPath(grp, providerName, rest, model) { + writeOAIError(w, http.StatusForbidden, "invalid_request_error", "model access denied") + return + } } - if !hasModel { - writeOAIError(w, http.StatusForbidden, "invalid_request_error", "model could not be determined") - return + } else { + // Single-grant principal: unchanged pre-round-3 order (model, then + // path), checked INDEPENDENTLY of each other — with only one grant + // to combine from, this can never produce the cross-grant leak the + // branch above exists to prevent. + if grp.hasModelRestriction() { + model, ok := g.peekAndValidateModel(w, r) + if !ok { + return + } + if !g.allowsPassthroughModel(grp, providerName, model) { + writeOAIError(w, http.StatusForbidden, "invalid_request_error", "model access denied") + return + } } - if !g.allowsPassthroughModel(grp, providerName, model) { - writeOAIError(w, http.StatusForbidden, "invalid_request_error", "model access denied") - return - } - } - if !grp.allowsPassthroughPath(rest) { - writeOAIError(w, http.StatusForbidden, "invalid_request_error", "path access denied") - return + if !grp.allowsPassthroughPathForProvider(providerName, rest) { + writeOAIError(w, http.StatusForbidden, "invalid_request_error", "path access denied") + return + } } if r.Header.Get("Upgrade") != "" { diff --git a/routes_passthrough_test.go b/routes_passthrough_test.go index 07d99a5..f7bd7ef 100644 --- a/routes_passthrough_test.go +++ b/routes_passthrough_test.go @@ -1356,6 +1356,344 @@ func TestHandlePassthrough_UnrestrictedGroup_SkipsModelPeekEntirely(t *testing.T } } +// TestHandlePassthrough_MultiGrant_UnrestrictedGrantProvider_SkipsModelPeek +// proves handlePassthrough's hasModelRestrictionForProviderPath gate for +// a multi-grant (group + personal grant) principal: a provider reachable +// only through a grant that carries NO model restriction never even +// reads the request body — mirrors TestHandlePassthrough_ +// UnrestrictedGroup_SkipsModelPeekEntirely above, but for a principal +// that also carries a personal grant restricting a DIFFERENT provider +// (proved not to interfere, by the companion test right below). +func TestHandlePassthrough_MultiGrant_UnrestrictedGrantProvider_SkipsModelPeek(t *testing.T) { + var upstreamCalled bool + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + upstreamCalled = true + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + cfg := CreateConfig() + cfg.Providers = map[string]*ProviderConfig{"open": {Type: "openai", BaseURL: srv.URL, APIKey: "sk-up"}} + cfg.Groups = map[string]*GroupConfig{"base": {Providers: []string{"open"}}} // no Models restriction + cfg.Users = &UsersConfig{Inline: []*UserConfig{ + {Name: "alice", Group: "base", APIKey: "sk-alice", Providers: []string{"closed"}, Models: []string{"closed/allowed"}}, + }} + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + if err != nil { + t.Fatalf("New: %v", err) + } + + req := httptest.NewRequest(http.MethodPost, "/open/v1/audio/transcriptions", strings.NewReader("--boundary\r\nfake multipart body\r\n--boundary--")) + req.Header.Set("Authorization", "Bearer sk-alice") + req.Header.Set("Content-Type", "multipart/form-data; boundary=boundary") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (base group's own grant is unrestricted on \"open\"), body=%s", rec.Code, rec.Body.String()) + } + if !upstreamCalled { + t.Error("upstream must be called: the grant that allows \"open\" carries no model restriction") + } +} + +// TestHandlePassthrough_MultiGrant_RestrictedPersonalGrantProvider_EnforcesModel +// is the opposite case for the SAME principal as the test above: the +// personal grant restricts "closed" to exactly one model, so a +// passthrough request against "closed" must peek and enforce it. +func TestHandlePassthrough_MultiGrant_RestrictedPersonalGrantProvider_EnforcesModel(t *testing.T) { + var upstreamCalled bool + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + upstreamCalled = true + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + cfg := CreateConfig() + cfg.Providers = map[string]*ProviderConfig{ + "open": {Type: "openai", BaseURL: srv.URL, APIKey: "sk-up"}, + "closed": {Type: "openai", BaseURL: srv.URL, APIKey: "sk-up"}, + } + cfg.Groups = map[string]*GroupConfig{"base": {Providers: []string{"open"}}} + cfg.Users = &UsersConfig{Inline: []*UserConfig{ + {Name: "alice", Group: "base", APIKey: "sk-alice", Providers: []string{"closed"}, Models: []string{"closed/allowed"}}, + }} + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + if err != nil { + t.Fatalf("New: %v", err) + } + + deniedReq := httptest.NewRequest(http.MethodPost, "/closed/v1/native-endpoint", strings.NewReader(`{"model":"closed/denied"}`)) + deniedReq.Header.Set("Authorization", "Bearer sk-alice") + deniedReq.Header.Set("Content-Type", "application/json") + deniedRec := httptest.NewRecorder() + h.ServeHTTP(deniedRec, deniedReq) + if deniedRec.Code != http.StatusForbidden { + t.Fatalf("denied model: status = %d, want 403, body=%s", deniedRec.Code, deniedRec.Body.String()) + } + if upstreamCalled { + t.Error("upstream must never be called for a model the personal grant does not list") + } + + allowedReq := httptest.NewRequest(http.MethodPost, "/closed/v1/native-endpoint", strings.NewReader(`{"model":"closed/allowed"}`)) + allowedReq.Header.Set("Authorization", "Bearer sk-alice") + allowedReq.Header.Set("Content-Type", "application/json") + allowedRec := httptest.NewRecorder() + h.ServeHTTP(allowedRec, allowedReq) + if allowedRec.Code != http.StatusOK { + t.Fatalf("allowed model: status = %d, want 200, body=%s", allowedRec.Code, allowedRec.Body.String()) + } + if !upstreamCalled { + t.Error("upstream must be called for the personal grant's own allowed model") + } +} + +// TestHandlePassthrough_MultiGroup_PassthroughPathsNeverUnionAcrossProviders +// is HIGH-1's regression (review round 2): passthroughPaths must NOT be +// unioned across member groups the way providers/models/mcpServers/agents +// are — group "a" {providers:[alpha]} carries no path restriction at all, +// group "b" {providers:[beta], passthroughPaths:[v1/chat/completions]} +// restricts beta narrowly. A user in BOTH groups must still be denied +// "/beta/v1/files": pairing "a"'s own unrestricted paths with "b"'s own +// provider would let joining group "a" silently strip group "b"'s own +// path restriction, even though NEITHER group alone ever authorized that +// combination. "/beta/v1/chat/completions" (b's own allowed path) and +// "/alpha/v1/files" (a's own unrestricted provider) must both still work. +func TestHandlePassthrough_MultiGroup_PassthroughPathsNeverUnionAcrossProviders(t *testing.T) { + var lastPath string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + lastPath = r.URL.Path + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + cfg := CreateConfig() + cfg.Providers = map[string]*ProviderConfig{ + "alpha": {Type: "openai", BaseURL: srv.URL, APIKey: "sk-up"}, + "beta": {Type: "openai", BaseURL: srv.URL, APIKey: "sk-up"}, + } + cfg.Groups = map[string]*GroupConfig{ + "a": {Providers: []string{"alpha"}}, + "b": {Providers: []string{"beta"}, PassthroughPaths: []string{"v1/chat/completions"}}, + } + cfg.Users = &UsersConfig{Inline: []*UserConfig{ + {Name: "multi", Group: "a", Groups: []string{"b"}, APIKey: "sk-multi"}, + }} + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + if err != nil { + t.Fatalf("New: %v", err) + } + + post := func(path string) int { + lastPath = "" + req := httptest.NewRequest(http.MethodPost, path, strings.NewReader(`{}`)) + req.Header.Set("Authorization", "Bearer sk-multi") + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + return rec.Code + } + + if code := post("/beta/v1/files"); code != http.StatusForbidden { + t.Errorf("POST /beta/v1/files: status = %d, want 403 (group b's own passthroughPaths restriction must not be stripped by joining group a)", code) + } + if lastPath != "" { + t.Error("upstream must never be called for a path denied by the coupled provider+path check") + } + if code := post("/beta/v1/chat/completions"); code != http.StatusOK { + t.Errorf("POST /beta/v1/chat/completions: status = %d, want 200 (group b's own allowed path)", code) + } + if code := post("/alpha/v1/files"); code != http.StatusOK { + t.Errorf("POST /alpha/v1/files: status = %d, want 200 (group a's own unrestricted provider)", code) + } +} + +// TestHandlePassthrough_MultiGroup_ModelAndPathMustComeFromSameGrant is +// the round-3 HIGH regression (review round 3, repro P1): group +// a{providers:[beta], models:[m1]} restricts models but not paths; +// group b{providers:[beta], passthroughPaths:[v1/chat/completions]} +// restricts paths but not models. Round 2's own fix coupled provider+ +// path per grant and provider+model per grant SEPARATELY — but nothing +// required BOTH to come from the SAME grant, so a multi-grant user could +// satisfy the path check via b's own unrestricted-model grant and the +// (irrelevant) model check via a's own unrestricted-path grant, +// authorizing a combination NEITHER grant alone ever granted. Every +// single-grant user (a-only, b-only) must still be denied on their own +// terms too (sanity controls, unaffected by this fix). +func TestHandlePassthrough_MultiGroup_ModelAndPathMustComeFromSameGrant(t *testing.T) { + var upstreamCalled bool + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + upstreamCalled = true + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + cfg := CreateConfig() + cfg.Providers = map[string]*ProviderConfig{"beta": {Type: "openai", BaseURL: srv.URL, APIKey: "sk-up"}} + cfg.Groups = map[string]*GroupConfig{ + "a": {Providers: []string{"beta"}, Models: []string{"m1"}}, + "b": {Providers: []string{"beta"}, PassthroughPaths: []string{"v1/chat/completions"}}, + } + cfg.Users = &UsersConfig{Inline: []*UserConfig{ + {Name: "ua", Group: "a", APIKey: "sk-a"}, + {Name: "ub", Group: "b", APIKey: "sk-b"}, + {Name: "uab", Group: "a", Groups: []string{"b"}, APIKey: "sk-ab"}, + }} + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + if err != nil { + t.Fatalf("New: %v", err) + } + + post := func(key string) int { + upstreamCalled = false + req := httptest.NewRequest(http.MethodPost, "/beta/v1/files", strings.NewReader(`{"model":"m2"}`)) + req.Header.Set("Authorization", "Bearer "+key) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if upstreamCalled { + t.Errorf("key %q: upstream must never be called for a denied combination", key) + } + return rec.Code + } + + if code := post("sk-a"); code != http.StatusForbidden { + t.Errorf("a-only: status = %d, want 403 (a has no path restriction, but model m2 is not m1)", code) + } + if code := post("sk-b"); code != http.StatusForbidden { + t.Errorf("b-only: status = %d, want 403 (b has no model restriction, but v1/files is not v1/chat/completions)", code) + } + if code := post("sk-ab"); code != http.StatusForbidden { + t.Errorf("a+b: status = %d, want 403 — model m2 must not be authorized by combining a's own unrestricted path with b's own unrestricted model", code) + } +} + +// TestHandlePassthrough_MultiGroup_PersonalGrantModelAndPathFromSameGrant +// is the round-3 HIGH regression's personal-grant half (review round 3, +// repro P2): primary "home"{providers:[alpha]} carries no path +// restriction; member "friends"{providers:[beta], passthroughPaths: +// [v1/chat/completions]} restricts beta's own paths; carol's personal +// grant adds providers:[beta],models:[beta/m1]. Her request must be +// checked as ONE unit — the path it borrows is home's (the primary +// group's) own list, and the model it allows is its own — home's own +// unrestricted paths must never combine with friends' own unrestricted +// models to authorize a request the personal grant itself denies. +func TestHandlePassthrough_MultiGroup_PersonalGrantModelAndPathFromSameGrant(t *testing.T) { + var upstreamCalled bool + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + upstreamCalled = true + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + cfg := CreateConfig() + cfg.Providers = map[string]*ProviderConfig{ + "alpha": {Type: "openai", BaseURL: srv.URL, APIKey: "sk-up"}, + "beta": {Type: "openai", BaseURL: srv.URL, APIKey: "sk-up"}, + } + cfg.Groups = map[string]*GroupConfig{ + "home": {Providers: []string{"alpha"}}, + "friends": {Providers: []string{"beta"}, PassthroughPaths: []string{"v1/chat/completions"}}, + } + cfg.Users = &UsersConfig{Inline: []*UserConfig{ + {Name: "carol", Group: "home", Groups: []string{"friends"}, APIKey: "sk-carol", Providers: []string{"beta"}, Models: []string{"beta/m1"}}, + }} + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + if err != nil { + t.Fatalf("New: %v", err) + } + + req := httptest.NewRequest(http.MethodPost, "/beta/v1/files", strings.NewReader(`{"model":"beta/other"}`)) + req.Header.Set("Authorization", "Bearer sk-carol") + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403 (the personal grant's own Models list denies %q; home's own unrestricted paths and friends' own unrestricted models must not combine to authorize it)", rec.Code, "beta/other") + } + if upstreamCalled { + t.Error("upstream must never be called for a model the personal grant denies") + } +} + +// TestHandlePassthrough_MultiGroup_ModelAndPathFromSameGrant_PositiveCases +// are the round-3 HIGH fix's own positive controls, using the SAME +// fixtures as the two regression tests above: a request whose model AND +// path both come from ONE grant must still succeed. +func TestHandlePassthrough_MultiGroup_ModelAndPathFromSameGrant_PositiveCases(t *testing.T) { + var upstreamCalled bool + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + upstreamCalled = true + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + cfg := CreateConfig() + cfg.Providers = map[string]*ProviderConfig{ + "alpha": {Type: "openai", BaseURL: srv.URL, APIKey: "sk-up"}, + "beta": {Type: "openai", BaseURL: srv.URL, APIKey: "sk-up"}, + } + cfg.Groups = map[string]*GroupConfig{ + "a": {Providers: []string{"beta"}, Models: []string{"m1"}}, + "b": {Providers: []string{"beta"}, PassthroughPaths: []string{"v1/chat/completions"}}, + "home": {Providers: []string{"alpha"}}, + "friends": {Providers: []string{"beta"}, PassthroughPaths: []string{"v1/chat/completions"}}, + } + cfg.Users = &UsersConfig{Inline: []*UserConfig{ + {Name: "ub", Group: "b", APIKey: "sk-b"}, + {Name: "uab", Group: "a", Groups: []string{"b"}, APIKey: "sk-ab"}, + {Name: "carol", Group: "home", Groups: []string{"friends"}, APIKey: "sk-carol", Providers: []string{"beta"}, Models: []string{"beta/m1"}}, + }} + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + if err != nil { + t.Fatalf("New: %v", err) + } + + post := func(key, path, body string) int { + upstreamCalled = false + req := httptest.NewRequest(http.MethodPost, path, strings.NewReader(body)) + req.Header.Set("Authorization", "Bearer "+key) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + return rec.Code + } + + // b-only: b's own grant allows beta with no model restriction, and + // b's own allowed path — must succeed regardless of model. + if code := post("sk-b", "/beta/v1/chat/completions", `{"model":"anything"}`); code != http.StatusOK { + t.Errorf("b-only on v1/chat/completions: status = %d, want 200", code) + } + if !upstreamCalled { + t.Error("b-only: upstream must be called") + } + + // a+b: model m1 on a path a allows (a has no path restriction) — a's + // OWN grant covers both model and path together. + if code := post("sk-ab", "/beta/v1/files", `{"model":"m1"}`); code != http.StatusOK { + t.Errorf("a+b, model m1 on a's own unrestricted path: status = %d, want 200", code) + } + if !upstreamCalled { + t.Error("a+b: upstream must be called") + } + + // carol: personal grant's own model (beta/m1) on a path the PRIMARY + // group (home) allows (home has no path restriction). + if code := post("sk-carol", "/beta/v1/anything-home-allows", `{"model":"beta/m1"}`); code != http.StatusOK { + t.Errorf("personal grant, model beta/m1 on home's own unrestricted path: status = %d, want 200", code) + } + if !upstreamCalled { + t.Error("carol: upstream must be called") + } +} + // TestHandlePassthrough_RestrictedGroup_BinaryContentType_Returns403 is // ruling item 4's fail-closed case for a genuinely binary body: a // RESTRICTED group's multipart/audio/image/video upload carries no diff --git a/routes_unified.go b/routes_unified.go index 77eed6d..fb540e6 100644 --- a/routes_unified.go +++ b/routes_unified.go @@ -641,15 +641,20 @@ func (g *Gateway) runMeteredCall(sw *statusTrackingWriter, r *http.Request, scop } // buildLimitScopes returns the limitScope slice runUnified passes to the -// limiter: a user scope and a group scope, ALWAYS both, unconditionally -// (v0.21 fix — see this function's doc comment history below for the bug -// this closes). The user scope is listed first, so checkAndCount reports a -// user's own violation ahead of their group's when both are breached by the -// same request. +// limiter: a user scope and one group scope PER MEMBER GROUP (grp. +// memberScopeGroups, auth.go — one for an ordinary, single-membership +// group, more for a multi-group principal; UserConfig.Groups' own "each +// member group's own limits apply as its own limit scope" rule, +// llmgateway.go), ALWAYS all of them, unconditionally (v0.21 fix — see +// this function's doc comment history below for the bug this closes). +// The user scope is listed first, so checkAndCount reports a user's own +// violation ahead of their group's when both are breached by the same +// request. // -// u.limits or grp.limits may be nil — that scope simply carries no limit for -// checkAndCount (limits.go) to enforce, and checkAndCount's own nil-limits -// check skips it during evaluation. It is NOT omitted from the slice: usage +// u.limits or a member group's own limits may be nil — that scope simply +// carries no limit for checkAndCount (limits.go) to enforce, and +// checkAndCount's own nil-limits check skips it during evaluation. It is +// NOT omitted from the slice: usage // accounting (checkAndCount's own counting half, and account) is // unconditional and must never be coupled to whether a limit happens to be // configured. Production bug (root-caused live, 2026-08-21): the previous @@ -675,10 +680,13 @@ func (g *Gateway) runMeteredCall(sw *statusTrackingWriter, r *http.Request, scop // handleAdminAPI "calls this directly, without withTotalScope" — it never // called this function at all, on any version of this file. func buildLimitScopes(u *user, grp *group) []limitScope { - return []limitScope{ - {limits: u.limits, kind: "user", id: u.name}, - {limits: grp.limits, kind: "group", id: grp.name}, + members := grp.memberScopeGroups() + scopes := make([]limitScope, 0, 1+len(members)) + scopes = append(scopes, limitScope{limits: u.limits, kind: "user", id: u.name}) + for _, mg := range members { + scopes = append(scopes, limitScope{limits: mg.limits, kind: "group", id: mg.name}) } + return scopes } // withTotalScope returns scopes with the synthetic total scope diff --git a/routes_unified_test.go b/routes_unified_test.go index 5c90a5d..4f7d44c 100644 --- a/routes_unified_test.go +++ b/routes_unified_test.go @@ -2094,6 +2094,37 @@ func TestBuildLimitScopes_AlwaysBuildsBothScopes(t *testing.T) { } } +// TestBuildLimitScopes_MultiGroup_OneScopePerMemberGroup proves a +// multi-group principal's own limit scopes are one PER MEMBER GROUP, +// each under its own name and its own limits — never merged into one +// combined scope, and never carrying the synthetic principal's own "+" +// -joined name. +func TestBuildLimitScopes_MultiGroup_OneScopePerMemberGroup(t *testing.T) { + a := &group{name: "eng", limits: &LimitsConfig{RequestsPerMinute: 5}} + b := &group{name: "ops", limits: &LimitsConfig{RequestsPerMinute: 9}} + grp := effectiveGroup([]*group{a, b}, nil) + + u := &user{name: "u1"} + scopes := buildLimitScopes(u, grp) + + if len(scopes) != 3 { + t.Fatalf("len(scopes) = %d, want 3 (1 user + 2 member groups)", len(scopes)) + } + if scopes[0].kind != "user" || scopes[0].id != "u1" { + t.Errorf("scopes[0] = %+v, want the user scope first", scopes[0]) + } + got := map[string]*LimitsConfig{} + for _, s := range scopes[1:] { + if s.kind != "group" { + t.Errorf("scope kind = %q, want %q", s.kind, "group") + } + got[s.id] = s.limits + } + if got["eng"] != a.limits || got["ops"] != b.limits { + t.Errorf("group scopes = %v, want each member group's own limits under its own name", got) + } +} + func TestUnifiedCostMicros(t *testing.T) { u := usage{prompt: 1_000_000, completion: 1_000_000} diff --git a/tools/yaegi-check/main.go b/tools/yaegi-check/main.go index 50bb09b..2e60cb3 100644 --- a/tools/yaegi-check/main.go +++ b/tools/yaegi-check/main.go @@ -543,6 +543,101 @@ func run() error { })) defer bareWinnerBUpstream.Close() + // multiGroupBUpstream backs exerciseMultiGroupPersonalGrant (multi- + // group/personal-grant feature, UserConfig.Groups/Providers/Models, + // llmgateway.go): the ONLY grant that reaches "multi-group-b" at all + // is the harness user's own PERSONAL grant (run()'s "users" + // override), restricted to multiGroupAllowedModelID alone — neither + // of the user's two member groups (multiGroupEngGroupName/ + // multiGroupOpsGroupName) names "multi-group-b" in their own + // providers list. A CORRECT per-grant check must still deny + // multiGroupDeniedModelID here; a naive "any grant allows the + // provider (union) AND any grant allows the model (union)" check + // would instead WRONGLY ALLOW it — neither member group restricts its + // own models at all, so the models-side union collapses to "allow + // anything," and pairing that with the personal grant's own + // providers-side reach to "multi-group-b" authorizes a combination + // neither grant alone ever granted — see this const block's own doc + // comments below for the full shape. + multiGroupBHits := new(int64) + multiGroupBUpstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt64(multiGroupBHits, 1) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"id":"c-multi-group","object":"chat.completion","model":"` + multiGroupAllowedModelID + `","choices":[{"index":0,"message":{"role":"assistant","content":"served by multi-group-b"},"finish_reason":"stop"}],"usage":{"prompt_tokens":1,"completion_tokens":1}}`)) + })) + defer multiGroupBUpstream.Close() + + // multiGroupCUpstream/multiGroupSharedXUpstream back + // exerciseMultiGroupBareWinnerFailoverAndPassthroughPaths (LOW-8, + // review round 2 — extends the multi-group/personal-grant probe + // above to also cover HIGH-1's provider-coupled passthroughPaths fix + // and HIGH-2's per-grant bareWinner/failover fix under the REAL + // interpreter). Both serve multiGroupSharedModelID as a BARE id + // alongside "multi-group-b" (all three configured below), the exact + // three-owner collision shape HIGH-2's own compiled regression test + // (TestModelRegistry_Resolve_MultiGrant_BareWinnerPrefersGrantThatActuallyAuthorizes) + // drives, here reachable only through the harness user's THREE + // distinct grants (multiGroupEngGroupName, multiGroupOpsGroupName, + // and the personal grant) — sorted provider-name order is + // "multi-group-b" < "multi-group-c" < "multi-group-shared-x", and + // only the personal grant (restricted to multiGroupAllowedModelID + // alone) reaches "multi-group-b", the sorted-first owner, so a + // pre-fix bareWinner picks it and wrongly denies the whole request + // even though multiGroupOpsGroupName's own unrestricted grant on + // "multi-group-c" would happily serve it. + // + // multiGroupCUpstream also DOUBLES as the primary-failure trigger for + // the failover half of that same assertion: it always 500s a chat- + // completions call (path-detected below) specifically so + // runMeteredCall's own failover loop falls through past the correctly + // -chosen primary ("multi-group-c") to the next candidate + // ("multi-group-shared-x", reachable via multiGroupEngGroupName's own + // unrestricted grant) — while any OTHER path it receives (a native + // passthrough request) succeeds normally, backing the HIGH-1 + // passthroughPaths assertion multiGroupOpsGroupName's own + // "passthroughPaths" override (below) restricts. + multiGroupCChatHits := new(int64) + multiGroupCPassthroughHits := new(int64) + multiGroupCUpstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/v1/chat/completions" { + atomic.AddInt64(multiGroupCChatHits, 1) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"error":{"message":"multi-group-c chat completions always fails, forcing failover","type":"server_error"}}`)) + return + } + atomic.AddInt64(multiGroupCPassthroughHits, 1) + w.WriteHeader(http.StatusOK) + })) + defer multiGroupCUpstream.Close() + multiGroupSharedXHits := new(int64) + multiGroupSharedXUpstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/v1/chat/completions" { + atomic.AddInt64(multiGroupSharedXHits, 1) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"id":"c-multi-group-shared-x","object":"chat.completion","model":"` + multiGroupSharedModelID + `","choices":[{"index":0,"message":{"role":"assistant","content":"served by multi-group-shared-x"},"finish_reason":"stop"}],"usage":{"prompt_tokens":1,"completion_tokens":1}}`)) + return + } + w.WriteHeader(http.StatusOK) + })) + defer multiGroupSharedXUpstream.Close() + + // round3SharedUpstream backs exerciseRound3ModelAndPathFromSameGrant + // (HIGH fix, review round 3): a native passthrough request never + // carries a translated "/v1/chat/completions" path the way the + // chat-completions route does (handlePassthrough forwards rest + // verbatim), so this upstream only ever needs to record that it was + // reached at all — both P1 and P2 (this probe's own doc comment) must + // deny before ever calling it. + round3PassthroughHits := new(int64) + round3PassthroughUpstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt64(round3PassthroughHits, 1) + w.WriteHeader(http.StatusOK) + })) + defer round3PassthroughUpstream.Close() + // failover.enabled must be set explicitly here: FailoverConfig.Enabled // defaults to false (coordinator ruling — a version upgrade with no // config change must preserve prior behavior), so without this the @@ -591,13 +686,52 @@ func run() error { // failover-a/failover-b: same collision shape, different fix under // test. `"bare-winner-a":{"type":"openai","baseUrl":"` + bareWinnerAUpstream.URL + `","apiKey":"sk-up","models":["` + bareWinnerModelID + `"]},` + - `"bare-winner-b":{"type":"openai","baseUrl":"` + bareWinnerBUpstream.URL + `","apiKey":"sk-up","models":["` + bareWinnerModelID + `"]}},` + - // groups: ADDS bareWinnerGroupName to the map Unmarshal already - // populated from .traefik.yml's own testData.groups.default — map - // keys merge (run()'s own doc comment, above, on why "brk" and the - // other new provider keys above are additive rather than - // replacing), so "default" (providers: [], allow-all) is untouched. - `"groups":{"` + bareWinnerGroupName + `":{"providers":["bare-winner-b"]}},` + + `"bare-winner-b":{"type":"openai","baseUrl":"` + bareWinnerBUpstream.URL + `","apiKey":"sk-up","models":["` + bareWinnerModelID + `"]},` + + // "multi-group-b" backs exerciseMultiGroupPersonalGrant — see + // multiGroupBUpstream's own doc comment above. Its models list + // ALSO includes multiGroupSharedModelID (LOW-8 extension) so it + // participates as the (wrongly-preferred, pre-fix) sorted-first + // owner in the bareWinner/failover collision + // exerciseMultiGroupBareWinnerFailoverAndPassthroughPaths drives + // — see multiGroupCUpstream's own doc comment above for the full + // three-provider shape. + `"multi-group-b":{"type":"openai","baseUrl":"` + multiGroupBUpstream.URL + `","apiKey":"sk-up","models":["only","other","` + multiGroupSharedModelID + `"]},` + + `"multi-group-c":{"type":"openai","baseUrl":"` + multiGroupCUpstream.URL + `","apiKey":"sk-up","models":["` + multiGroupSharedModelID + `"]},` + + `"multi-group-shared-x":{"type":"openai","baseUrl":"` + multiGroupSharedXUpstream.URL + `","apiKey":"sk-up","models":["` + multiGroupSharedModelID + `"]},` + + // "round3-shared" backs exerciseRound3ModelAndPathFromSameGrant + // (HIGH fix, review round 3) — a single real provider both P1's + // and P2's own groups reach, native passthrough only (no Models + // list needed: passthrough authorization is purely group-driven, + // never consults a provider's own configured Models). + `"round3-shared":{"type":"openai","baseUrl":"` + round3PassthroughUpstream.URL + `","apiKey":"sk-up"}},` + + // groups: ADDS bareWinnerGroupName and the two exerciseMultiGroupPersonalGrant + // member groups to the map Unmarshal already populated from + // .traefik.yml's own testData.groups.default — map keys merge + // (run()'s own doc comment, above, on why "brk" and the other new + // provider keys above are additive rather than replacing), so + // "default" (providers: [], allow-all) is untouched. Neither + // multiGroupEngGroupName nor multiGroupOpsGroupName names + // "multi-group-b" — only the harness user's own PERSONAL grant + // (below) does — so a correct implementation reaches "multi-group-b" + // through that personal grant alone, not through either group. + // multiGroupOpsGroupName's own "passthroughPaths" restricts ITS + // OWN provider ("multi-group-c") to multiGroupAllowedPathID alone + // (LOW-8/HIGH-1 extension) — multiGroupEngGroupName carries no + // such restriction on its own provider ("multi-group-shared-x"), + // and the fix under test (auth.go's + // allowsPassthroughPathForProvider) must keep the two from + // leaking into each other. + // round3P1AGroupName/round3P1BGroupName (P1) and + // round3P2HomeGroupName/round3P2FriendsGroupName (P2) — see + // exerciseRound3ModelAndPathFromSameGrant's own doc comment for the + // full shape each pair reproduces. + `"groups":{"` + bareWinnerGroupName + `":{"providers":["bare-winner-b"]},` + + `"` + multiGroupEngGroupName + `":{"providers":["multi-group-shared-x"]},` + + `"` + multiGroupOpsGroupName + `":{"providers":["multi-group-c"],"passthroughPaths":["` + multiGroupAllowedPathID + `"]},` + + `"` + round3P1AGroupName + `":{"providers":["round3-shared"],"models":["` + round3ModelID + `"]},` + + `"` + round3P1BGroupName + `":{"providers":["round3-shared"],"passthroughPaths":["v1/chat/completions"]},` + + `"` + round3P2HomeGroupName + `":{"providers":["round3-alpha"]},` + + `"` + round3P2FriendsGroupName + `":{"providers":["round3-shared"],"passthroughPaths":["v1/chat/completions"]}},` + // modelMeta (feature v0.23): a config-override entry for // testDataWantModel, so exerciseHandler's GET /v1/models // assertion below proves resolveModelMeta's config-override @@ -645,7 +779,25 @@ func run() error { // exerciseBareWinnerGroupAware's own — group bareWinnerGroupName, // authorized for "bare-winner-b" only (the "groups" override // above). - `"admin":{"enabled":true},"users":{"inline":[{"name":"tester","group":"default","apiKey":"` + testDataUserAPIKey + `"},{"name":"admin1","group":"default","apiKey":"` + attemptAccountingAdminAPIKey + `","admin":true},{"name":` + string(trickyNameJSON) + `,"group":"default","apiKey":"sk-tricky"},{"name":"bare-winner-friend","group":"` + bareWinnerGroupName + `","apiKey":"` + bareWinnerFriendAPIKey + `"}]}}` + // + // The fifth inline user (multiGroupUserAPIKey) is + // exerciseMultiGroupPersonalGrant's own — member of BOTH + // multiGroupEngGroupName and multiGroupOpsGroupName (group + + // groups, multi-group support), plus a personal grant + // (providers/models) restricting "multi-group-b" to + // multiGroupAllowedModelID alone. + // + // The sixth inline user (round3P1UserAPIKey) is + // exerciseRound3ModelAndPathFromSameGrant's own P1 principal — + // member of BOTH round3P1AGroupName (models-restricted) and + // round3P1BGroupName (paths-restricted), no personal grant. + // + // The seventh inline user (round3P2UserAPIKey) is that same + // probe's own P2 principal — member of round3P2HomeGroupName + // (primary, unrestricted) and round3P2FriendsGroupName + // (paths-restricted), plus a personal grant restricting + // "round3-shared" to round3PersonalModelID alone. + `"admin":{"enabled":true},"users":{"inline":[{"name":"tester","group":"default","apiKey":"` + testDataUserAPIKey + `"},{"name":"admin1","group":"default","apiKey":"` + attemptAccountingAdminAPIKey + `","admin":true},{"name":` + string(trickyNameJSON) + `,"group":"default","apiKey":"sk-tricky"},{"name":"bare-winner-friend","group":"` + bareWinnerGroupName + `","apiKey":"` + bareWinnerFriendAPIKey + `"},{"name":"multi-group-user","group":"` + multiGroupEngGroupName + `","groups":["` + multiGroupOpsGroupName + `"],"apiKey":"` + multiGroupUserAPIKey + `","providers":["multi-group-b"],"models":["` + multiGroupAllowedModelID + `"]},{"name":"round3-p1-user","group":"` + round3P1AGroupName + `","groups":["` + round3P1BGroupName + `"],"apiKey":"` + round3P1UserAPIKey + `"},{"name":"round3-p2-user","group":"` + round3P2HomeGroupName + `","groups":["` + round3P2FriendsGroupName + `"],"apiKey":"` + round3P2UserAPIKey + `","providers":["round3-shared"],"models":["` + round3PersonalModelID + `"]}]}}` if err = json.Unmarshal([]byte(attemptAccountingOverride), cfgVal.Interface()); err != nil { return fmt.Errorf("decode attempt-accounting harness override into the interpreted Config: %w", err) } @@ -713,6 +865,30 @@ func run() error { if err := exerciseBareWinnerGroupAware(handler, bareWinnerAHits); err != nil { return err } + // exerciseMultiGroupPersonalGrant (multi-group/personal-grant feature) + // runs right after exerciseBareWinnerGroupAware, for the identical + // reason: it drives its own, freshly named provider/groups/user, so it + // cannot perturb exerciseBreaker's "brk"-specific hit counting or any + // later probe's /metrics assertions, which are all keyed to OTHER + // provider/model names. + if err := exerciseMultiGroupPersonalGrant(handler, multiGroupBHits); err != nil { + return err + } + // exerciseMultiGroupBareWinnerFailoverAndPassthroughPaths (LOW-8, + // review round 2) runs right after exerciseMultiGroupPersonalGrant, + // against the SAME multi-group-user principal but its own, freshly + // named providers/models/paths, so it cannot perturb any earlier or + // later probe's own hit counters or /metrics assertions. + if err := exerciseMultiGroupBareWinnerFailoverAndPassthroughPaths(handler, multiGroupBHits, multiGroupCChatHits, multiGroupCPassthroughHits, multiGroupSharedXHits); err != nil { + return err + } + // exerciseRound3ModelAndPathFromSameGrant (HIGH fix, review round 3) + // runs right after, against its own, freshly named P1/P2 providers/ + // groups/users, so it cannot perturb any earlier or later probe's own + // hit counters or /metrics assertions. + if err := exerciseRound3ModelAndPathFromSameGrant(handler, round3PassthroughHits); err != nil { + return err + } // exerciseBreaker (feat/provider-health, adversarial-review round 2): // runs after exerciseHandler, against the SAME interpreted handler — // proves the discovery circuit breaker's open/half-open/closed state @@ -800,6 +976,100 @@ const bareWinnerGroupName = "bare-winner-friends" // user (run()'s "users" override). const bareWinnerFriendAPIKey = "sk-bare-winner-friend" // #nosec G101 -- test fixture literal, not a real credential +// multiGroupEngGroupName/multiGroupOpsGroupName are the two member groups +// exerciseMultiGroupPersonalGrant's own inline user belongs to +// (UserConfig.Group/Groups, llmgateway.go, run()'s "users" override) — +// neither names "multi-group-b" in its own "groups" override providers +// list, so the only grant reaching it at all is the user's own personal +// one (multiGroupUserAPIKey's own doc comment, below). +const ( + multiGroupEngGroupName = "multi-group-eng" + multiGroupOpsGroupName = "multi-group-ops" +) + +// multiGroupUserAPIKey authenticates exerciseMultiGroupPersonalGrant's +// own inline user: member of BOTH multiGroupEngGroupName and +// multiGroupOpsGroupName (run()'s "users" override), plus a personal +// grant (Providers/Models) restricting "multi-group-b" to +// multiGroupAllowedModelID alone. +const multiGroupUserAPIKey = "sk-multi-group-user" // #nosec G101 -- test fixture literal, not a real credential + +// multiGroupAllowedModelID/multiGroupDeniedModelID are two ids served by +// the SAME real provider ("multi-group-b", run()'s "providers" override): +// the personal grant's own Models list (run()'s "users" override) names +// only the "only" form, so the "other" form must resolve as +// errModelDenied — proving the multi-group/personal-grant feature's +// per-grant (not per-union) model check runs correctly under Yaegi, not +// merely compiled (registry_test.go's own +// TestModelRegistry_Resolve_MultiGroup_CrossGrantLeakDenied covers the +// identical rule compiled). +const ( + multiGroupAllowedModelID = "multi-group-b/only" + multiGroupDeniedModelID = "multi-group-b/other" +) + +// multiGroupSharedModelID is the BARE id (no provider prefix) all three +// of "multi-group-b", "multi-group-c", and "multi-group-shared-x" serve +// (run()'s "providers" override) — LOW-8's extension of this probe +// (review round 2), exercising HIGH-2's bareWinner/listFor/failover fix +// under the REAL interpreter: sorted provider-name order is +// "multi-group-b" < "multi-group-c" < "multi-group-shared-x", and only +// the harness user's PERSONAL grant reaches "multi-group-b" — restricted +// to multiGroupAllowedModelID alone, which does NOT cover this bare id — +// so a pre-fix, union-based bareWinner picks "multi-group-b" first and +// wrongly denies the whole request, even though multiGroupOpsGroupName's +// own unrestricted grant on "multi-group-c" (sorted next) would happily +// serve it. multiGroupCUpstream's own doc comment (above) has the full +// three-provider shape, including how it doubles as the failover +// trigger. +const multiGroupSharedModelID = "multi-group-shared" + +// multiGroupAllowedPathID is the one native-passthrough REST path +// multiGroupOpsGroupName's own "passthroughPaths" override (run()'s +// "groups" override) allows on ITS OWN provider ("multi-group-c") — +// LOW-8's HIGH-1 extension (review round 2): multiGroupEngGroupName +// carries no such restriction on its own, DIFFERENT provider +// ("multi-group-shared-x"), and +// exerciseMultiGroupBareWinnerFailoverAndPassthroughPaths (below) proves +// the two never leak into each other under the REAL interpreter — the +// identical rule routes_passthrough_test.go's own +// TestHandlePassthrough_MultiGroup_PassthroughPathsNeverUnionAcrossProviders +// covers compiled. +const multiGroupAllowedPathID = "v1/allowed-path" + +// multiGroupDeniedPathID is any path NOT in multiGroupOpsGroupName's own +// "passthroughPaths" list — must be denied on "multi-group-c" regardless +// of multiGroupEngGroupName's own unrestricted paths on its DIFFERENT +// provider. +const multiGroupDeniedPathID = "v1/other-path" + +// round3P1AGroupName/round3P1BGroupName back +// exerciseRound3ModelAndPathFromSameGrant's own P1 reproduction (HIGH +// fix, review round 3): group A restricts models (round3ModelID) but not +// paths; group B restricts paths ("v1/chat/completions") but not models +// — both cover the SAME provider ("round3-shared"). round3P1UserAPIKey's +// own principal belongs to BOTH, no personal grant. +const ( + round3P1AGroupName = "round3-p1-a" + round3P1BGroupName = "round3-p1-b" + round3P1UserAPIKey = "sk-round3-p1-user" // #nosec G101 -- test fixture literal, not a real credential + round3ModelID = "round3-m1" +) + +// round3P2HomeGroupName/round3P2FriendsGroupName back +// exerciseRound3ModelAndPathFromSameGrant's own P2 reproduction: "home" +// is the PRIMARY group (no path restriction of its own); "friends" +// restricts paths on "round3-shared" but not models. +// round3P2UserAPIKey's own principal belongs to both, PLUS a personal +// grant restricting "round3-shared" to round3PersonalModelID alone — +// borrowing home's own unrestricted paths. +const ( + round3P2HomeGroupName = "round3-p2-home" + round3P2FriendsGroupName = "round3-p2-friends" + round3P2UserAPIKey = "sk-round3-p2-user" // #nosec G101 -- test fixture literal, not a real credential + round3PersonalModelID = "round3-shared/m1" +) + // mcpProbeServerName is the federated MCP server run() configures against // mcpProbeUpstream, and mcpProbeToolName is a tool id carrying its // "_" prefix so resolveFederatedTool routes POST /mcp's @@ -2210,6 +2480,253 @@ func exerciseBareWinnerGroupAware(handler http.Handler, aHits *int64) error { return nil } +// exerciseMultiGroupPersonalGrant proves the multi-group/personal-grant +// feature (UserConfig.Groups/Providers/Models, llmgateway.go) resolves +// correctly under the REAL interpreter: multiGroupUserAPIKey's own +// principal belongs to TWO groups (multiGroupEngGroupName, +// multiGroupOpsGroupName — neither reaching "multi-group-b" at all) +// plus a personal grant restricting "multi-group-b" to +// multiGroupAllowedModelID alone. A request for the personal grant's own +// allowed model must succeed; a request for a DIFFERENT model on the +// SAME provider must be denied — the no-cross-grant-leak rule +// (group.allowsProviderModel, auth.go) a naive union-based check would +// get wrong. +func exerciseMultiGroupPersonalGrant(handler http.Handler, bHits *int64) error { + allowedBody := `{"model":"` + multiGroupAllowedModelID + `","messages":[{"role":"user","content":"hi"}]}` + allowedReq := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(allowedBody)) + allowedReq.Header.Set("Authorization", "Bearer "+multiGroupUserAPIKey) + allowedReq.Header.Set("Content-Type", "application/json") + allowedRec := httptest.NewRecorder() + handler.ServeHTTP(allowedRec, allowedReq) + if allowedRec.Code != http.StatusOK { + return fmt.Errorf("POST /v1/chat/completions (multi-group/personal-grant harness, allowed model): status = %d, want 200, body=%s", allowedRec.Code, allowedRec.Body.String()) + } + if !strings.Contains(allowedRec.Body.String(), "served by multi-group-b") { + return fmt.Errorf("POST /v1/chat/completions (multi-group/personal-grant harness, allowed model): body does not contain multi-group-b's own content: %s", allowedRec.Body.String()) + } + if got := atomic.LoadInt64(bHits); got != 1 { + return fmt.Errorf("multi-group/personal-grant harness: multi-group-b upstream hit count = %d, want exactly 1 after the allowed request", got) + } + + deniedBody := `{"model":"` + multiGroupDeniedModelID + `","messages":[{"role":"user","content":"hi"}]}` + deniedReq := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(deniedBody)) + deniedReq.Header.Set("Authorization", "Bearer "+multiGroupUserAPIKey) + deniedReq.Header.Set("Content-Type", "application/json") + deniedRec := httptest.NewRecorder() + handler.ServeHTTP(deniedRec, deniedReq) + if deniedRec.Code != http.StatusForbidden { + return fmt.Errorf("POST /v1/chat/completions (multi-group/personal-grant harness, cross-grant model): status = %d, want 403 (the personal grant's own Models list denies %q; neither member group's grant may leak into authorizing it), body=%s", deniedRec.Code, multiGroupDeniedModelID, deniedRec.Body.String()) + } + if got := atomic.LoadInt64(bHits); got != 1 { + return fmt.Errorf("multi-group/personal-grant harness: multi-group-b upstream hit count = %d, want still exactly 1 (the denied request must never reach the upstream)", got) + } + + fmt.Println("yaegi-check: multi-group user with a personal grant resolved the personal-grant provider's allowed model and was denied a cross-grant model") + return nil +} + +// exerciseMultiGroupBareWinnerFailoverAndPassthroughPaths extends the +// multi-group/personal-grant probe above (LOW-8, review round 2) to +// additionally assert, under the REAL interpreter: +// +// - HIGH-2: bareWinner/listFor prefer a grant that actually authorizes +// a bare-id collision, not merely the sorted-first PROVIDER-visible +// owner — resolving multiGroupSharedModelID (a bare, three-provider +// collision: "multi-group-b" < "multi-group-c" < +// "multi-group-shared-x") must land on "multi-group-c" +// (multiGroupOpsGroupName's own unrestricted grant), never get +// denied via "multi-group-b" (the personal grant's own, narrower +// one, which sorts first but does not cover this id). +// - The SAME multi-grant principal's failover pool correctly includes +// "multi-group-shared-x" (multiGroupEngGroupName's own grant) and +// correctly excludes "multi-group-b" — multiGroupCUpstream always +// 500s its chat-completions call, forcing the fall-through past the +// correctly-chosen primary. +// - GET /v1/models shows exactly the union this principal is entitled +// to for multiGroupSharedModelID: the bare id (owned by +// "multi-group-c"), "multi-group-c/"+id, and +// "multi-group-shared-x/"+id — never "multi-group-b/"+id. +// - HIGH-1: a native passthrough request to "multi-group-c" is denied +// on multiGroupDeniedPathID and allowed on multiGroupAllowedPathID +// (multiGroupOpsGroupName's own "passthroughPaths"), while +// "multi-group-shared-x" (multiGroupEngGroupName's own, unrestricted +// provider) stays reachable on an arbitrary path either way — +// proving passthroughPaths stays coupled to the SAME grant that +// authorizes the provider, never unioned across this principal's +// member groups. +func exerciseMultiGroupBareWinnerFailoverAndPassthroughPaths(handler http.Handler, bHits, cChatHits, cPassthroughHits, sharedXHits *int64) error { + bHitsBefore := atomic.LoadInt64(bHits) // exerciseMultiGroupPersonalGrant already drove one hit + + // --- HIGH-2 + failover --- + body := `{"model":"` + multiGroupSharedModelID + `","messages":[{"role":"user","content":"hi"}]}` + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(body)) + req.Header.Set("Authorization", "Bearer "+multiGroupUserAPIKey) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + return fmt.Errorf("POST /v1/chat/completions (multi-group bareWinner/failover harness): status = %d, want 200 (bareWinner must land on \"multi-group-c\" via multiGroupOpsGroupName's own grant, then fail over to \"multi-group-shared-x\" once multi-group-c's own upstream 500s), body=%s", rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), "served by multi-group-shared-x") { + return fmt.Errorf("POST /v1/chat/completions (multi-group bareWinner/failover harness): body does not contain multi-group-shared-x's own content, want the failover candidate's response: %s", rec.Body.String()) + } + if got := atomic.LoadInt64(cChatHits); got != 1 { + return fmt.Errorf("multi-group bareWinner/failover harness: multi-group-c chat-completions hit count = %d, want exactly 1 (bareWinner must choose multi-group-c as primary — a pre-fix, union-based bareWinner would pick \"multi-group-b\" first and never reach multi-group-c at all)", got) + } + if got := atomic.LoadInt64(sharedXHits); got != 1 { + return fmt.Errorf("multi-group bareWinner/failover harness: multi-group-shared-x hit count = %d, want exactly 1 (the correctly-included failover candidate)", got) + } + if got := atomic.LoadInt64(bHits) - bHitsBefore; got != 0 { + return fmt.Errorf("multi-group bareWinner/failover harness: multi-group-b hit count delta = %d, want exactly 0 (the personal grant's own Models list does not cover %q; multi-group-b must never be offered as a failover candidate for it)", got, multiGroupSharedModelID) + } + + // --- GET /v1/models: the union this principal is entitled to --- + listReq := httptest.NewRequest(http.MethodGet, "/v1/models", nil) + listReq.Header.Set("Authorization", "Bearer "+multiGroupUserAPIKey) + listRec := httptest.NewRecorder() + handler.ServeHTTP(listRec, listReq) + if listRec.Code != http.StatusOK { + return fmt.Errorf("GET /v1/models (multi-group bareWinner harness): status = %d, want 200, body=%s", listRec.Code, listRec.Body.String()) + } + var listBody struct { + Data []map[string]any `json:"data"` + } + if err := json.Unmarshal(listRec.Body.Bytes(), &listBody); err != nil { + return fmt.Errorf("decode GET /v1/models body (multi-group bareWinner harness): %w (body=%s)", err, listRec.Body.String()) + } + ownedBy := map[string]string{} + for _, entry := range listBody.Data { + id, idOK := entry["id"].(string) + owner, ownerOK := entry["owned_by"].(string) + if idOK && ownerOK { + ownedBy[id] = owner + } + } + if ownedBy[multiGroupSharedModelID] != "multi-group-c" { + return fmt.Errorf("GET /v1/models (multi-group bareWinner harness): bare %q owned_by = %q, want \"multi-group-c\"", multiGroupSharedModelID, ownedBy[multiGroupSharedModelID]) + } + if _, ok := ownedBy["multi-group-c/"+multiGroupSharedModelID]; !ok { + return fmt.Errorf("GET /v1/models (multi-group bareWinner harness): missing \"multi-group-c/%s\"", multiGroupSharedModelID) + } + if _, ok := ownedBy["multi-group-shared-x/"+multiGroupSharedModelID]; !ok { + return fmt.Errorf("GET /v1/models (multi-group bareWinner harness): missing \"multi-group-shared-x/%s\"", multiGroupSharedModelID) + } + if _, ok := ownedBy["multi-group-b/"+multiGroupSharedModelID]; ok { + return fmt.Errorf("GET /v1/models (multi-group bareWinner harness): \"multi-group-b/%s\" must be absent (the personal grant's own Models list does not cover it)", multiGroupSharedModelID) + } + + // --- HIGH-1: passthroughPaths coupled to the provider --- + deniedPathReq := httptest.NewRequest(http.MethodPost, "/multi-group-c/"+multiGroupDeniedPathID, strings.NewReader(`{}`)) + deniedPathReq.Header.Set("Authorization", "Bearer "+multiGroupUserAPIKey) + deniedPathReq.Header.Set("Content-Type", "application/json") + deniedPathRec := httptest.NewRecorder() + handler.ServeHTTP(deniedPathRec, deniedPathReq) + if deniedPathRec.Code != http.StatusForbidden { + return fmt.Errorf("POST /multi-group-c/%s (multi-group passthroughPaths harness): status = %d, want 403 (multiGroupOpsGroupName's own path restriction on its OWN provider must survive, never stripped by multiGroupEngGroupName's own unrestricted paths on a DIFFERENT provider), body=%s", multiGroupDeniedPathID, deniedPathRec.Code, deniedPathRec.Body.String()) + } + if got := atomic.LoadInt64(cPassthroughHits); got != 0 { + return fmt.Errorf("multi-group passthroughPaths harness: multi-group-c passthrough hit count = %d, want exactly 0 (the denied-path request must never reach the upstream)", got) + } + + allowedPathReq := httptest.NewRequest(http.MethodPost, "/multi-group-c/"+multiGroupAllowedPathID, strings.NewReader(`{}`)) + allowedPathReq.Header.Set("Authorization", "Bearer "+multiGroupUserAPIKey) + allowedPathReq.Header.Set("Content-Type", "application/json") + allowedPathRec := httptest.NewRecorder() + handler.ServeHTTP(allowedPathRec, allowedPathReq) + if allowedPathRec.Code != http.StatusOK { + return fmt.Errorf("POST /multi-group-c/%s (multi-group passthroughPaths harness): status = %d, want 200 (multiGroupOpsGroupName's own allowed path), body=%s", multiGroupAllowedPathID, allowedPathRec.Code, allowedPathRec.Body.String()) + } + if got := atomic.LoadInt64(cPassthroughHits); got != 1 { + return fmt.Errorf("multi-group passthroughPaths harness: multi-group-c passthrough hit count = %d, want exactly 1 after the allowed-path request", got) + } + + sharedXPathReq := httptest.NewRequest(http.MethodPost, "/multi-group-shared-x/"+multiGroupDeniedPathID, strings.NewReader(`{}`)) + sharedXPathReq.Header.Set("Authorization", "Bearer "+multiGroupUserAPIKey) + sharedXPathReq.Header.Set("Content-Type", "application/json") + sharedXPathRec := httptest.NewRecorder() + handler.ServeHTTP(sharedXPathRec, sharedXPathReq) + if sharedXPathRec.Code != http.StatusOK { + return fmt.Errorf("POST /multi-group-shared-x/%s (multi-group passthroughPaths harness): status = %d, want 200 (multiGroupEngGroupName's own provider carries no path restriction at all, regardless of multiGroupOpsGroupName's own restriction on a DIFFERENT provider), body=%s", multiGroupDeniedPathID, sharedXPathRec.Code, sharedXPathRec.Body.String()) + } + + fmt.Println("yaegi-check: multi-group principal's bareWinner/listFor/failover chose the grant-authorized owner, GET /v1/models showed exactly the union, and passthroughPaths stayed coupled to its own provider") + return nil +} + +// exerciseRound3ModelAndPathFromSameGrant proves the HIGH fix (review +// round 3) under the REAL interpreter: model and path authorization for +// native passthrough must come from ONE grant, never two different ones. +// +// - P1: round3P1AGroupName restricts models (round3ModelID) but not +// paths; round3P1BGroupName restricts paths ("v1/chat/completions") +// but not models — both cover "round3-shared". round3-p1-user +// (member of both, no personal grant) POSTs a model +// round3P1AGroupName denies, on a path only round3P1BGroupName would +// allow: round3P1AGroupName's own unrestricted path must never +// combine with round3P1BGroupName's own unrestricted model to +// authorize it. +// - P2: round3P2HomeGroupName (primary) carries no path restriction; +// round3P2FriendsGroupName restricts paths on "round3-shared" but not +// models; round3-p2-user's own personal grant restricts +// "round3-shared" to round3PersonalModelID alone. The personal grant +// borrows home's own unrestricted paths, but its OWN model +// restriction must still apply — friends' own unrestricted models +// must never leak into it via a shared provider. +// +// Both must deny (403) and never reach round3PassthroughUpstream. +func exerciseRound3ModelAndPathFromSameGrant(handler http.Handler, hits *int64) error { + hitsBefore := atomic.LoadInt64(hits) + + p1Body := `{"model":"round3-m2"}` + p1Req := httptest.NewRequest(http.MethodPost, "/round3-shared/v1/files", strings.NewReader(p1Body)) + p1Req.Header.Set("Authorization", "Bearer "+round3P1UserAPIKey) + p1Req.Header.Set("Content-Type", "application/json") + p1Rec := httptest.NewRecorder() + handler.ServeHTTP(p1Rec, p1Req) + if p1Rec.Code != http.StatusForbidden { + return fmt.Errorf("P1 POST /round3-shared/v1/files (round-3 model+path-from-same-grant harness): status = %d, want 403 — round3P1AGroupName's own unrestricted path must never combine with round3P1BGroupName's own unrestricted model to authorize %q, body=%s", p1Rec.Code, round3ModelID, p1Rec.Body.String()) + } + + p2Body := `{"model":"round3-shared/other"}` + p2Req := httptest.NewRequest(http.MethodPost, "/round3-shared/v1/files", strings.NewReader(p2Body)) + p2Req.Header.Set("Authorization", "Bearer "+round3P2UserAPIKey) + p2Req.Header.Set("Content-Type", "application/json") + p2Rec := httptest.NewRecorder() + handler.ServeHTTP(p2Rec, p2Req) + if p2Rec.Code != http.StatusForbidden { + return fmt.Errorf("P2 POST /round3-shared/v1/files (round-3 model+path-from-same-grant harness): status = %d, want 403 — home's own unrestricted paths and friends' own unrestricted models must not combine to authorize a model the personal grant itself denies, body=%s", p2Rec.Code, p2Rec.Body.String()) + } + + if got := atomic.LoadInt64(hits) - hitsBefore; got != 0 { + return fmt.Errorf("round-3 model+path-from-same-grant harness: round3-shared upstream hit count delta = %d, want exactly 0 (neither P1's nor P2's denied request may ever reach the upstream)", got) + } + + // P2+ (positive control, review round 4): the personal grant's own + // ALLOWED model (round3PersonalModelID), on a path the PRIMARY group + // (round3P2HomeGroupName) allows — home carries no path restriction + // at all, so ANY path is allowed via the "personal grant borrows the + // primary group's own paths" rule (auth.go's personalGrantAllowsPath). + // Proves the round-3 coupling fix denies the WRONG combination (P2 + // above, a different model on the same provider+path) without also + // wrongly denying the RIGHT one — the body IS peeked (the personal + // grant restricts models) and the model IS matched. + p2PositiveBody := `{"model":"` + round3PersonalModelID + `"}` + p2PositiveReq := httptest.NewRequest(http.MethodPost, "/round3-shared/v1/files", strings.NewReader(p2PositiveBody)) + p2PositiveReq.Header.Set("Authorization", "Bearer "+round3P2UserAPIKey) + p2PositiveReq.Header.Set("Content-Type", "application/json") + p2PositiveRec := httptest.NewRecorder() + handler.ServeHTTP(p2PositiveRec, p2PositiveReq) + if p2PositiveRec.Code != http.StatusOK { + return fmt.Errorf("P2+ POST /round3-shared/v1/files (round-3 model+path-from-same-grant harness): status = %d, want 200 — the personal grant's own model (%q) on a path the PRIMARY group (home) allows must succeed, body=%s", p2PositiveRec.Code, round3PersonalModelID, p2PositiveRec.Body.String()) + } + if got := atomic.LoadInt64(hits) - hitsBefore; got != 1 { + return fmt.Errorf("round-3 model+path-from-same-grant harness: round3-shared upstream hit count delta after the P2+ positive case = %d, want exactly 1", got) + } + + fmt.Println("yaegi-check: native passthrough denied a model and a path that could only be authorized by combining two DIFFERENT grants (P1: two member groups; P2: primary group's paths + a personal grant's own model restriction), and allowed the personal grant's own model on the primary group's own path (P2+)") + return nil +} + // exerciseRequestTimeout proves the request-timeout feature aborts a // hung provider under the REAL interpreter, not merely in compiled // tests: timeoutProviderName's own ProviderConfig.RequestTimeout ("80ms") diff --git a/users_file_test.go b/users_file_test.go index 831d34c..68b3a44 100644 --- a/users_file_test.go +++ b/users_file_test.go @@ -6,6 +6,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "slices" "sync" "testing" "time" @@ -169,6 +170,46 @@ func TestUsersFile_Load_ValidFile(t *testing.T) { } } +// TestUsersFile_Load_GroupsProvidersModels proves the multi-group/ +// personal-grant fields (UserConfig.Groups/Providers/Models, +// llmgateway.go) round-trip through the users file's JSON shape exactly +// like every other UserConfig field. +func TestUsersFile_Load_GroupsProvidersModels(t *testing.T) { + dir := t.TempDir() + fp := writeUsersFile(t, dir, "users.json", []*UserConfig{ + { + Name: "carol", + Group: "friends", + Groups: []string{"home"}, + APIKey: "sk-carol", + Providers: []string{"gx10"}, + Models: []string{"gx10/GLM-5.3-Flash-EXL3"}, + }, + }) + + uf := newUsersFile(fp) + users, err := uf.load() + if err != nil { + t.Fatalf("load: %v", err) + } + if len(users) != 1 { + t.Fatalf("want 1 user, got %d", len(users)) + } + got := users[0] + if got.Group != "friends" { + t.Errorf("Group = %q, want %q", got.Group, "friends") + } + if !slices.Equal(got.Groups, []string{"home"}) { + t.Errorf("Groups = %v, want [home]", got.Groups) + } + if !slices.Equal(got.Providers, []string{"gx10"}) { + t.Errorf("Providers = %v, want [gx10]", got.Providers) + } + if !slices.Equal(got.Models, []string{"gx10/GLM-5.3-Flash-EXL3"}) { + t.Errorf("Models = %v, want [gx10/GLM-5.3-Flash-EXL3]", got.Models) + } +} + func TestUsersFile_Load_MalformedJSON_ReturnsError(t *testing.T) { dir := t.TempDir() fp := filepath.Join(dir, "bad.json") @@ -330,3 +371,79 @@ func TestAuthStore_MaybeReload_FileUserSameNameAsInline_FileWins(t *testing.T) { t.Fatalf("want file alice identifiable, got %v ok=%v", u, ok) } } + +// TestAuthStore_MaybeReload_MultiGroupUser_RebuiltPrincipalStillAuthorizesCorrectly +// is LOW-7's regression coverage (review round 2): a users-file hot +// reload rebuilds EVERY principal from scratch (replaceFileUsers calls +// buildEntry fresh for each user) — a multi-group user's rebuilt +// principal must still resolve exactly the same union of member-group +// authorization after a reload as before it. This is functional +// correctness, distinct from MEDIUM-3's own modelsCache-growth-bound +// test: that test proves the CACHE stays bounded across reloads; this +// one proves the REBUILT principal itself still authorizes correctly. +func TestAuthStore_MaybeReload_MultiGroupUser_RebuiltPrincipalStillAuthorizesCorrectly(t *testing.T) { + dir := t.TempDir() + fp := writeUsersFile(t, dir, "users.json", []*UserConfig{ + {Name: "carol", Group: "eng", Groups: []string{"ops"}, APIKey: "sk-carol"}, + }) + + cfg := &Config{ + Providers: map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}}, + Groups: map[string]*GroupConfig{ + "eng": {Providers: []string{"alpha"}}, + "ops": {Providers: []string{"beta"}}, + }, + } + a, err := newAuthStore(cfg) + if err != nil { + t.Fatalf("newAuthStore: %v", err) + } + uf := newUsersFile(fp) + initial, err := uf.load() + if err != nil { + t.Fatalf("initial load: %v", err) + } + if err := a.replaceFileUsers(initial); err != nil { + t.Fatalf("initial replaceFileUsers: %v", err) + } + + clock := &fakeClock{now: time.Unix(1_700_000_000, 0)} + a.usersFile = uf + a.log = &stubLogger{} + a.nowFn = clock.Now + if info, statErr := os.Stat(fp); statErr == nil { + a.lastModTime = info.ModTime() + } + a.lastCheck = clock.Now() + + _, grpBefore, ok := identifyWithKey(a, "sk-carol") + if !ok { + t.Fatal("identify failed before reload") + } + if !grpBefore.allowsProvider("alpha") || !grpBefore.allowsProvider("beta") { + t.Fatalf("sanity: want both alpha and beta reachable before reload, providers=%v", grpBefore.providers) + } + + // Force a real reload of the IDENTICAL content — buildEntry rebuilds + // carol's principal as a brand-new *group either way (effectiveGroup + // never caches or reuses a synthetic principal across calls). + writeUsersDoc(t, fp, []*UserConfig{ + {Name: "carol", Group: "eng", Groups: []string{"ops"}, APIKey: "sk-carol"}, + }, time.Now().Add(time.Hour)) + clock.Advance(reloadEvery + time.Second) + a.maybeReload() + + _, grpAfter, ok := identifyWithKey(a, "sk-carol") + if !ok { + t.Fatal("identify failed after reload") + } + if grpAfter == grpBefore { + t.Fatal("sanity: want a NEW *group pointer after reload (buildEntry rebuilds every principal from scratch) — otherwise this test never exercises the rebuild path at all") + } + if !grpAfter.allowsProvider("alpha") || !grpAfter.allowsProvider("beta") { + t.Errorf("want both alpha and beta still reachable after reload, providers=%v", grpAfter.providers) + } + if grpAfter.allowsProvider("gamma") { + t.Error("want gamma still denied after reload") + } +}