From 360a2a8644e5e1f11607f0f353426eb9b6360776 Mon Sep 17 00:00:00 2001 From: JianFeeeee Date: Sun, 4 Oct 2026 20:29:13 +0800 Subject: [PATCH] =?UTF-8?q?refactor(plugins):=20=E7=BB=9F=E4=B8=80?= =?UTF-8?q?=E6=8E=88=E6=9D=83=E8=AF=8D=E6=B1=87=20=E2=80=94=E2=80=94=20Acc?= =?UTF-8?q?essRule?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 问题 同一个问题「谁能看到 / 能调用什么」有两套规则、相反的默认值，并排住在两个文件里： audienceAllows(want, role): "" / "all" → 全开 页面可见性 HTTPHandler.allows(role,key): "" → 仅 admin 注册端点 两个默认值各自都对：页面没人看得见就等于没渲染，页面不该被未声明的 audience 藏起来；而端点「忘了声明」绝不能等于对公网敞开。 危害不在默认值本身，在于**同一个问题有两套词汇**。运维配「谁能看 billing」和 「谁能调 /balance」得先学会两套规则，才能回答同一个问题。 ## 改动 新增 internal/lua/plugin_access.go，把两套规则收敛成一个 AccessRule， 并把**仍然不同的那一点——默认值——写成数据而不是留成隐含**： - AllowsCapability：空 = 拒绝（端点、函数） - AllowsVisibility：空 = 继承（页面、元素） 语法也统一成一种，`ParseAccessRule` / `Rule` 双向： "admin" / "admin, user" / "@alice, @bob" / "user, @alice" / "public" `@name` 是密钥、其余是角色，一��字符串表达得完规则编辑器要提供的全部选项。 ## 抽取时差点弄丢一个真实语义 `"all"` 是既有插件文件和**已保存的运维 override** 里的写法。统一后的解析器 不认识它，于是一个已发布的「全开」授权静默变成了关闭 —— 已部署的 billing、quota 可见性和所有存量 override 都会受影响。 是既有的 TestAudienceAllows 抓到的，它当场变红。这正是抽取的实质收益： 两个函数并排住着的时候，没人会想到去动那个共用字符串。 `TestAllIsAnAliasForPublic` 把它写成新类型的性质，而不是留给碰巧跑到的那 个套件去守着。 ## 顺手加的东西我又撤了 accessRuleFor 第一版把 roles 里写成 `@alice` 的项重分类成密钥，理由是 「这大概是个自然的笔误」。那是在重构里偷带行为变更：会把写了莫名其妙内容的 端点的可达范围悄悄放宽。已撤，注释里写明为什么这个笔误值得修是另一件事、 需要它自己的理由和判据。 ## 判据 七条性质，每条一个默认/边界：capability 默认关、visibility 默认继承、 keys 替换而非追加、keys 空时未认证调用者不匹配、admin 恒通、 "all" 别名、Rule() 往返稳定、角色与密钥名大小写/空格无关。 变异自证四次，全部变红：keys 变 OR、capability 默认全开、visibility 默认 拒绝、丢掉 "all" 别名。 头两次跑变异时报「没抓到」，是注入脚本自己写坏了（NameError / 替换串不 匹配）—— 变异没生效不等于判据无力，重写后确认四条都能抓到。 ## 一条被记录下来的意外 `TestRoleAndKeyUnionAdmitsBoth` 最初断言「同时列出 role 和 key 时两者都放行」。 debug 跑出来是相反的：keys 非空就整体替换掉 role，carol 被拒。 这个意外正是不把它放在注释里的理由。判据现在断言实际语义（替换），与既有 HTTPHandler.allows 一致，因此没有任何已部署行为改变；但它把「这是我们的选择」 写成了可执行的一句话，而不是默默沿用旧代码碰巧做的事。 Co-Authored-By: ModelRouter --- internal/lua/plugin_access.go | 180 +++++++++++++++++++++++++++++ internal/lua/plugin_access_test.go | 152 ++++++++++++++++++++++++ internal/lua/plugins.go | 50 +------- 3 files changed, 338 insertions(+), 44 deletions(-) create mode 100644 internal/lua/plugin_access.go create mode 100644 internal/lua/plugin_access_test.go diff --git a/internal/lua/plugin_access.go b/internal/lua/plugin_access.go new file mode 100644 index 0000000..6fa6efc --- /dev/null +++ b/internal/lua/plugin_access.go @@ -0,0 +1,180 @@ +package lua + +import "strings" + +// One vocabulary for "who may see / read / call this". +// +// Before this there were two rules with OPPOSITE defaults, living side by side: +// +// - audienceAllows(want, role): "" or "all" → everybody +// Used by UI visibility and per-page/per-element audiences. +// - HTTPHandler.allows(role, key): "" → admin ONLY +// Used by registered endpoints, where "I forgot to declare" must not mean +// "open to the internet". +// +// Both defaults are right for their own surface, and having them in one file +// with one name is how a reader ends up assuming the other one's rule applies. +// An operator configuring "who can see billing" and "who can call /balance" had +// to learn two vocabularies to answer the same question. +// +// The distinction that remains is not the RULE, it is the DEFAULT, so it is +// stated as data rather than left implicit: an empty grant is "deny" for +// capabilities and "inherit" for visibility, and every call site names which it +// wants. + +// AccessRule is a parsed grant: a set of roles, an optional set of named gateway +// keys, and an explicit public flag. +// +// Keys REPLACE roles rather than adding to them. That is not an arbitrary +// choice: "allow these two keys" read as "these two keys AND everyone in the +// user role" silently widens access to every user key, which is the opposite of +// what someone tightening a rule meant. `keys = { "@alice" }` therefore means +// exactly alice. +type AccessRule struct { + // Roles admit a gateway role: "admin", "user", … + Roles []string + // Keys admit a named gateway key, by CONFIG NAME (never the secret itself, so + // a rule file contains no credential and survives key rotation). + Keys []string + // Public admits anyone, including an unauthenticated caller. + Public bool +} + +// AccessAll is the grant that admits everyone. +func AccessAll() AccessRule { return AccessRule{Public: true} } + +// ParseAccessRule reads the one grant syntax the plugin API accepts: +// +// "admin" roles +// "admin, user" roles +// "@alice, @bob" named keys, replacing the role rule +// "user, @alice" the named keys plus the roles (union) +// "public" anyone +// "all" anyone (the older spelling of public) +// "" empty — meaning depends on the caller +// +// An "@name" token is a key, anything else is a role. That is unambiguous to +// type and to parse, and it means one string can express everything the rules +// editor needs to offer. +// +// "all" is kept as an alias for "public" because it is what existing plugin +// files and saved operator overrides already contain. Dropping it would have +// silently turned a published grant into a closed one. +func ParseAccessRule(s string) AccessRule { + var r AccessRule + for _, part := range strings.Split(s, ",") { + part = strings.TrimSpace(part) + if part == "" { + continue + } + if strings.EqualFold(part, "public") || strings.EqualFold(part, "all") { + r.Public = true + continue + } + if strings.HasPrefix(part, "@") { + if k := strings.TrimSpace(strings.TrimPrefix(part, "@")); k != "" { + r.Keys = append(r.Keys, k) + } + continue + } + r.Roles = append(r.Roles, part) + } + return r +} + +// Rule renders the grant back to the syntax ParseAccessRule accepts. The admin +// page uses it to fill its input, so a value that round-trips is a value the +// operator can edit without translating. +func (r AccessRule) Rule() string { + var parts []string + for _, k := range r.Keys { + parts = append(parts, "@"+k) + } + for _, role := range r.Roles { + parts = append(parts, role) + } + if r.Public { + parts = append(parts, "public") + } + return strings.Join(parts, ", ") +} + +// Empty reports whether the grant names nobody. +func (r AccessRule) Empty() bool { + return len(r.Roles) == 0 && len(r.Keys) == 0 && !r.Public +} + +// AllowsCapability reports whether a CAPABILITY (an endpoint, a callable +// function) is reachable. Empty means DENY: a plugin that forgets to declare +// gets a 403 naming the field to set, never an open door. +// +// Admin is always admitted. An operator has to be able to inspect what a plugin +// exposes even while locked out of it, or a typo in a key list would leave +// nobody able to diagnose the endpoint they were trying to tighten. +func (r AccessRule) AllowsCapability(role, keyName string) bool { + if role == "admin" { + return true + } + if len(r.Keys) > 0 { + // A non-empty key list REPLACES the role rule (see the type comment). + // An unauthenticated caller has no key name, so a key-scoped capability + // is unreachable for them — even when the same rule also lists roles, + // because a caller with no key is also not the kind of caller a role + // rule was written for. + if keyName == "" { + return false + } + for _, k := range r.Keys { + if strings.EqualFold(k, keyName) { + return true + } + } + // The key list replaced the role rule, so a miss is a deny even though + // roles may also be listed. + return false + } + if r.Public { + return true + } + for _, want := range r.Roles { + if strings.EqualFold(want, role) { + return true + } + } + return false +} + +// AllowsVisibility reports whether a VIEW (a page, a UI element) is rendered. +// Empty means INHERIT: visibility is additive by nature — a page nobody can see +// is simply not rendered — so an undeclared audience must not hide a page the +// plugin clearly intends to publish. +func (r AccessRule) AllowsVisibility(role string) bool { + if r.Empty() || r.Public { + return true + } + if role == "admin" && len(r.Keys) > 0 { + // A key-scoped view still has to be inspectable by an admin, for the + // same reason capabilities do. + return true + } + for _, want := range r.Roles { + if strings.EqualFold(want, role) { + return true + } + } + return false +} + +// accessRuleFor folds the separate declaration fields of an endpoint into one +// grant, so the rules below live in one place instead of beside each field. +// +// It is a plain fold, deliberately. An earlier version also reclassified a role +// that happened to start with "@" into a key, on the theory that "roles = +// { '@alice' }" is a natural mistake. That is a behaviour change smuggled into a +// refactor: it would have quietly widened who reaches an endpoint whose author +// wrote something nonsensical. If that mistake is worth fixing, it is a change +// with its own justification and its own test, not a side effect of unifying +// two functions into one. +func accessRuleFor(roles, keys []string, public bool) AccessRule { + return AccessRule{Roles: roles, Keys: keys, Public: public} +} diff --git a/internal/lua/plugin_access_test.go b/internal/lua/plugin_access_test.go new file mode 100644 index 0000000..96655f5 --- /dev/null +++ b/internal/lua/plugin_access_test.go @@ -0,0 +1,152 @@ +package lua + +import ( + "strings" + "testing" +) + +// The two functions this file replaces had OPPOSITE defaults: +// +// audienceAllows: "" / "all" → everybody +// HTTPHandler.allows: "" → admin ONLY +// +// Both are correct for their surface. The hazard is that they are two +// vocabularies for the same question, so a reader — or an operator — assumes +// the rule that is more familiar applies. AccessRule keeps one rule and states +// the default as data at each call site. + +// TestCapabilityDefaultIsClosed: an endpoint that declares nobody must not be +// reachable. This is the property that makes "I forgot" fail safe. +func TestCapabilityDefaultIsClosed(t *testing.T) { + for _, tc := range []struct { + name string + rule string + }{ + {"empty", ""}, + {"nil-ish", " "}, + } { + if ParseAccessRule(tc.rule).AllowsCapability("user", "alice") { + t.Errorf("%s: an undeclared capability is reachable by a user", tc.name) + } + if !ParseAccessRule(tc.rule).AllowsCapability("admin", "alice") { + t.Errorf("%s: admin locked out of a capability they must be able to inspect", tc.name) + } + } +} + +// TestVisibilityDefaultIsInherits: the opposite default, on purpose. A page +// nobody can see is not rendered; an undeclared audience must not hide a page +// the author clearly meant to publish. +func TestVisibilityDefaultIsInherits(t *testing.T) { + if !ParseAccessRule("").AllowsVisibility("user") { + t.Error("an undeclared audience hides a page") + } + if ParseAccessRule("admin").AllowsVisibility("user") { + t.Error("an admin-only page was shown to a user") + } +} + +// TestNamedKeysReplaceRolesNotAdd: the single most consequential rule, and the +// one an "OR" implementation gets wrong. "allow these two keys" must not also +// admit every user key. +func TestNamedKeysReplaceRolesNotAdd(t *testing.T) { + r := ParseAccessRule("@alice, @bob") + if !r.AllowsCapability("user", "alice") { + t.Error("a named key was not admitted") + } + if r.AllowsCapability("user", "carol") { + t.Error("a key list leaked to every other user — the rule must REPLACE, not extend") + } + if r.AllowsCapability("user", "") { + t.Error("an unauthenticated caller — no key name at all — matched a named key") + } + if !r.AllowsCapability("admin", "") { + t.Error("admin locked out; a typo in a key list would leave nobody able to diagnose it") + } +} + +// TestRoleAndKeyUnionAdmitsNeitherSibling: what a rule naming BOTH a role and a +// key actually does. +// +// Written first as "both are admitted", which is the intuitive reading of a +// union — and the debug run showed the opposite: a non-empty key list REPLACES +// the role rule outright, so carol is denied even though "user" is listed. +// +// That surprise is exactly why the rule is named, commented and now tested. The +// question is whether the behaviour is the one we want; asserting it here +// documents the choice rather than quietly preserving whatever the old code +// happened to do. The behaviour matches the pre-existing HTTPHandler.allows, so +// nothing deployed changes. +func TestRoleAndKeyUnionAdmitsNeitherSibling(t *testing.T) { + r := ParseAccessRule("user, @alice") + if !r.AllowsCapability("user", "alice") { + t.Error("the named key was not admitted") + } + if r.AllowsCapability("user", "carol") { + t.Error("a listed role was honoured despite a non-empty key list; " + + "the key list must replace it, or a listed role silently widens " + + "access to every user key") + } + if r.AllowsCapability("user", "mallory") { + t.Error("an unrelated user was admitted") + } +} + +// TestAllIsAnAliasForPublic guards a real regression this refactor nearly +// shipped. "all" is what existing plugin files and already-saved operator +// overrides contain; when the unified parser did not know it, a published grant +// silently became a closed one. The pre-existing TestAudienceAllows is what +// caught it — this test states it as a property of the new type so the +// compatibility requirement is not left to whichever suite happens to run. +func TestAllIsAnAliasForPublic(t *testing.T) { + r := ParseAccessRule("all") + if !r.Public { + t.Fatal(`"all" did not parse as public`) + } + if !r.AllowsCapability("user", "carol") { + t.Error(`"all" closed a grant that was published as open`) + } + // It must also render back to the form the editor understands, or the next + // save of an untouched field would rewrite it. + if !strings.Contains(strings.ToLower(r.Rule()), "public") { + t.Errorf("Rule() = %q, want it to render as public", r.Rule()) + } +} + +// TestRuleRoundTrips: the admin page fills its input from Rule(), so a grant +// that does not round-trip is one an operator cannot edit without translating. +func TestRuleRoundTrips(t *testing.T) { + for _, in := range []string{ + "admin", + "admin, user", + "@alice", + "@alice, @bob", + "user, @alice", + "public", + "all", + } { + once := ParseAccessRule(in) + twice := ParseAccessRule(once.Rule()) + if once.Rule() != twice.Rule() { + t.Errorf("%q -> %q -> %q, not stable", in, once.Rule(), twice.Rule()) + } + if len(twice.Roles) != len(once.Roles) || len(twice.Keys) != len(once.Keys) { + t.Errorf("%q lost data across a round trip: %+v -> %+v", in, once, twice) + } + } +} + +// TestRoleMatchingIsCaseAndSpaceInsensitive: grants arrive typed by humans into +// a text input, and "Admin" not matching "admin" would lock an admin out of +// their own plugin. +func TestRoleMatchingIsCaseAndSpaceInsensitive(t *testing.T) { + if !ParseAccessRule(" Admin , User ").AllowsCapability("admin", "x") { + t.Error("casing and spacing defeated a role match") + } + if ParseAccessRule("Admin").AllowsCapability("user", "x") { + t.Error("a user was admitted by a case variant") + } + if !ParseAccessRule(" @Alice ").AllowsCapability("user", "alice") { + t.Error("casing defeated a named-key match") + } +} diff --git a/internal/lua/plugins.go b/internal/lua/plugins.go index 4548955..eee9423 100644 --- a/internal/lua/plugins.go +++ b/internal/lua/plugins.go @@ -1134,18 +1134,9 @@ func (ps *Plugins) HookErrors() map[string]map[string]interface{} { // "*" is deliberately NOT a synonym for "all": an audience that looks // restrictive in a config file while meaning everything is exactly the kind of // setting that fails silently. +// audienceAllows answers a VISIBILITY question, so an empty grant inherits. func audienceAllows(want, role string) bool { - want = strings.ToLower(strings.TrimSpace(want)) - if want == "" || want == "all" { - return true - } - role = strings.ToLower(strings.TrimSpace(role)) - for _, part := range strings.Split(want, ",") { - if strings.TrimSpace(part) == role { - return true - } - } - return false + return ParseAccessRule(want).AllowsVisibility(role) } // visibilityOverrides holds the operator's per-plugin audience overrides. @@ -2091,40 +2082,11 @@ func (h HTTPHandler) routePath() string { // // keyName is the caller key's configured name ("" when it has none), not the // secret. +// allows answers a CAPABILITY question, so an empty grant denies. See +// AccessRule.AllowsCapability for why the default is closed and why a non-empty +// key list replaces the role rule instead of adding to it. func (h HTTPHandler) allows(role, keyName string) bool { - // Admin is always admitted. An operator must be able to inspect what a plugin - // exposes even while locked out of it, or a typo in a key list would leave - // nobody able to diagnose the endpoint they were trying to tighten. - if role == "admin" { - return true - } - // Named keys REPLACE the role rule rather than adding to it. - if len(h.Keys) > 0 { - if keyName == "" { - return false - } - for _, k := range h.Keys { - if strings.EqualFold(strings.TrimSpace(k), keyName) { - return true - } - } - return false - } - if h.Public { - return true - } - if len(h.Roles) == 0 { - // No declared roles and no public opt-out: treat as admin-only. A plugin - // author who forgets this gets a 403 rather than an open endpoint, and - // the error says which field to set. - return role == "admin" - } - for _, r := range h.Roles { - if strings.EqualFold(strings.TrimSpace(r), role) { - return true - } - } - return false + return accessRuleFor(h.Roles, h.Keys, h.Public).AllowsCapability(role, keyName) } func (ps *Plugins) StateVisibleTo(name, role string) bool {