Skip to content

Commit 50b3f0a

Browse files
authored
feat(platform): support multiple policy rules per plugin (#1182)
* feat(platform): support multiple policy rules per plugin Extend the command policy framework from single-Rule to multi-Rule semantics. A plugin (or policy.yml) may now contribute several scoped Rules; the engine combines them with OR -- a command is allowed when it satisfies every axis of at least one rule. This lets one integration apply different risk ceilings and identity restrictions to different command groups. The cross-plugin fail-closed boundary is preserved: two distinct plugins both calling Restrict still aborts startup (multiple_restrict_plugins). Single-Rule behaviour is fully backward compatible -- the rejection reason_code / rule_name / envelope shape are byte-for-byte unchanged; multi-rule rejection surfaces the aggregate reason_code no_matching_rule. - engine: New keeps single-rule compat, add NewSet for OR over rules - resolver: dedupe by owner (one plugin may contribute many rules), return []*Rule; yaml gains a top-level rules: list - registrar/builder/staging: Restrict may be called more than once; retire the double_restrict error - config policy show / config plugins show: emit a rules array - inventory: PluginEntry.Rules is now a slice (fixes last-rule-wins overwrite when a plugin contributes multiple rules) * fix(platform): clone rules in Builder.Restrict and inventory snapshot Address review feedback. Builder.Restrict stored the caller's *Rule directly, so reusing and mutating one Rule object across multiple Restrict calls collapsed entries to the last mutation; clone the rule and its slices on append, mirroring the staging registrar. BuildInventory likewise reused the source Allow/Deny/Identities slices; copy them when building the RuleView snapshot instead of relying on cloneInventory downstream. Add a regression test: reusing and mutating one Rule across two Restrict calls now yields two independent rules. * fix(platform): skip yaml when a plugin owns policy; reject empty rules list Two policy-config robustness fixes from review: - A malformed ~/.lark-cli/policy.yml could abort a plugin-governed binary. applyUserPolicyPruning read yaml before resolving, and build.go fail-closes on any policy error when a plugin is present. Plugin rules shadow yaml anyway, so skip reading yaml entirely when a plugin contributed rules -- an unrelated broken file on the user's machine can no longer lock the CLI. - A present-but-empty "rules: []" collapsed to a single all-zero Rule that allows every annotated command ("looks like policy, enforces almost nothing"). yaml.Parse now distinguishes absent from present-but-empty (Rules is a pointer) and rejects the empty list. Add regression tests for both.
1 parent b1ecf2d commit 50b3f0a

22 files changed

Lines changed: 756 additions & 208 deletions

cmd/config/plugins.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,8 @@ func runConfigPluginsShow(f *cmdutil.Factory) error {
8282
"version": p.Version,
8383
"capabilities": p.Capabilities,
8484
}
85-
if p.Rule != nil {
86-
entry["rule"] = p.Rule
85+
if len(p.Rules) > 0 {
86+
entry["rules"] = p.Rules
8787
}
8888
entry["hooks"] = map[string]any{
8989
"observers": p.Observers,

cmd/config/policy.go

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -59,16 +59,20 @@ func runConfigPolicyShow(f *cmdutil.Factory) error {
5959
"source_name": sourceName,
6060
"denied_paths": active.DeniedPaths,
6161
}
62-
if active.Rule != nil {
63-
out["rule"] = map[string]any{
64-
"name": active.Rule.Name,
65-
"description": active.Rule.Description,
66-
"allow": active.Rule.Allow,
67-
"deny": active.Rule.Deny,
68-
"max_risk": active.Rule.MaxRisk,
69-
"identities": active.Rule.Identities,
70-
"allow_unannotated": active.Rule.AllowUnannotated,
62+
if len(active.Rules) > 0 {
63+
rules := make([]map[string]any, 0, len(active.Rules))
64+
for _, r := range active.Rules {
65+
rules = append(rules, map[string]any{
66+
"name": r.Name,
67+
"description": r.Description,
68+
"allow": r.Allow,
69+
"deny": r.Deny,
70+
"max_risk": r.MaxRisk,
71+
"identities": r.Identities,
72+
"allow_unannotated": r.AllowUnannotated,
73+
})
7174
}
75+
out["rules"] = rules
7276
}
7377
output.PrintJson(f.IOStreams.Out, out)
7478
return nil

cmd/config/policy_test.go

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ func TestConfigPolicyShow_PluginActive(t *testing.T) {
5757
MaxRisk: "read",
5858
}
5959
cmdpolicy.SetActive(&cmdpolicy.ActivePolicy{
60-
Rule: rule,
60+
Rules: []*platform.Rule{rule},
6161
Source: cmdpolicy.ResolveSource{
6262
Kind: cmdpolicy.SourcePlugin,
6363
Name: "secaudit",
@@ -83,12 +83,16 @@ func TestConfigPolicyShow_PluginActive(t *testing.T) {
8383
if got["denied_paths"] != float64(42) {
8484
t.Errorf("denied_paths = %v, want 42", got["denied_paths"])
8585
}
86-
ruleMap, ok := got["rule"].(map[string]any)
86+
rulesAny, ok := got["rules"].([]any)
87+
if !ok || len(rulesAny) != 1 {
88+
t.Fatalf("rules field missing or wrong shape: %v", got["rules"])
89+
}
90+
ruleMap, ok := rulesAny[0].(map[string]any)
8791
if !ok {
88-
t.Fatalf("rule field missing or wrong type")
92+
t.Fatalf("rules[0] wrong type")
8993
}
9094
if ruleMap["name"] != "secaudit" {
91-
t.Errorf("rule.name = %v", ruleMap["name"])
95+
t.Errorf("rules[0].name = %v", ruleMap["name"])
9296
}
9397
}
9498

@@ -101,7 +105,7 @@ func TestConfigPolicyShow_YamlSourceNameIsEmpty(t *testing.T) {
101105
t.Cleanup(cmdpolicy.ResetActiveForTesting)
102106

103107
cmdpolicy.SetActive(&cmdpolicy.ActivePolicy{
104-
Rule: &platform.Rule{Name: "my-yaml-rule"},
108+
Rules: []*platform.Rule{{Name: "my-yaml-rule"}},
105109
Source: cmdpolicy.ResolveSource{
106110
Kind: cmdpolicy.SourceYAML,
107111
Name: "/Users/alice/.lark-cli/policy.yml",

cmd/platform_bootstrap.go

Lines changed: 48 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -36,47 +36,71 @@ const userPolicyFileName = "policy.yml"
3636
// pluginRules carries Plugin.Restrict() contributions collected from
3737
// the InstallAll phase; nil/empty is fine.
3838
func applyUserPolicyPruning(rootCmd *cobra.Command, pluginRules []cmdpolicy.PluginRule) error {
39-
yamlPath, err := userPolicyPath()
40-
if err != nil {
41-
// No user home dir means we cannot locate the policy. Treat
42-
// the same as "file missing": no pruning, no error. This keeps
43-
// non-interactive CI environments (no HOME set) running.
44-
yamlPath = ""
45-
}
46-
47-
yamlRule, err := cmdpolicy.LoadYAMLPolicy(yamlPath)
48-
if err != nil {
49-
// Yaml-only failures are fail-OPEN at the caller (warn and
50-
// continue), but the active-policy snapshot is process-global
51-
// and may still carry data from a previous build in long-lived
52-
// embedders / tests. Clear it explicitly so `config policy
53-
// show` reports "no policy" instead of a stale rule that
54-
// doesn't reflect the current command tree.
55-
cmdpolicy.SetActive(nil)
56-
return err
39+
// Plugin rules shadow the yaml source entirely (Resolve: plugin >
40+
// yaml). When a plugin contributed rules we therefore do NOT even
41+
// read ~/.lark-cli/policy.yml: build.go fail-CLOSES on any policy
42+
// error once a plugin is present, so reading a malformed yaml here
43+
// would let an unrelated broken file on the user's machine abort a
44+
// plugin-governed binary -- exactly the file the plugin is supposed
45+
// to shadow. Skipping the read keeps the shadow contract honest.
46+
var (
47+
yamlRules []*platform.Rule
48+
yamlPath string
49+
)
50+
if len(pluginRules) == 0 {
51+
p, perr := userPolicyPath()
52+
if perr != nil {
53+
// No user home dir means we cannot locate the policy. Treat
54+
// the same as "file missing": no pruning, no error. This keeps
55+
// non-interactive CI environments (no HOME set) running.
56+
p = ""
57+
}
58+
yamlPath = p
59+
loaded, lerr := cmdpolicy.LoadYAMLPolicy(yamlPath)
60+
if lerr != nil {
61+
// Yaml-only failures are fail-OPEN at the caller (warn and
62+
// continue), but the active-policy snapshot is process-global
63+
// and may still carry data from a previous build in long-lived
64+
// embedders / tests. Clear it explicitly so `config policy
65+
// show` reports "no policy" instead of a stale rule that
66+
// doesn't reflect the current command tree.
67+
cmdpolicy.SetActive(nil)
68+
return lerr
69+
}
70+
yamlRules = loaded
5771
}
5872

59-
rule, source, err := cmdpolicy.Resolve(cmdpolicy.Sources{
73+
rules, source, err := cmdpolicy.Resolve(cmdpolicy.Sources{
6074
PluginRules: pluginRules,
61-
YAMLRule: yamlRule,
75+
YAMLRules: yamlRules,
6276
YAMLPath: yamlPath,
6377
})
6478
if err != nil {
6579
cmdpolicy.SetActive(nil)
6680
return err
6781
}
68-
if rule == nil {
82+
if len(rules) == 0 {
6983
cmdpolicy.SetActive(&cmdpolicy.ActivePolicy{Source: source})
7084
return nil
7185
}
7286

73-
engine := cmdpolicy.New(rule)
87+
// RuleName attributes a denial to a specific rule in the envelope.
88+
// With a single rule that is unambiguous and preserves the legacy
89+
// envelope verbatim; with several rules a denial means "no rule
90+
// granted it", which has no single owner, so the field is left empty
91+
// and reason_code=no_matching_rule carries the meaning instead.
92+
ruleName := ""
93+
if len(rules) == 1 {
94+
ruleName = rules[0].Name
95+
}
96+
97+
engine := cmdpolicy.NewSet(rules)
7498
decisions := engine.EvaluateAll(rootCmd)
75-
denied := cmdpolicy.BuildDeniedByPath(rootCmd, decisions, source, rule.Name)
99+
denied := cmdpolicy.BuildDeniedByPath(rootCmd, decisions, source, ruleName)
76100
cmdpolicy.Apply(rootCmd, denied)
77101

78102
cmdpolicy.SetActive(&cmdpolicy.ActivePolicy{
79-
Rule: rule,
103+
Rules: rules,
80104
Source: source,
81105
DeniedPaths: len(denied),
82106
})

cmd/platform_bootstrap_test.go

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@ import (
1313

1414
"github.com/spf13/cobra"
1515

16+
"github.com/larksuite/cli/extension/platform"
17+
"github.com/larksuite/cli/internal/cmdpolicy"
1618
"github.com/larksuite/cli/internal/cmdutil"
1719
"github.com/larksuite/cli/internal/output"
1820
)
@@ -184,6 +186,39 @@ func TestApplyUserPolicyPruning_malformedYamlReturnsError(t *testing.T) {
184186
}
185187
}
186188

189+
// When a plugin contributed rules, a malformed user policy.yml must NOT
190+
// abort: plugin rules shadow yaml entirely, so the broken file is never
191+
// read. Regression -- previously LoadYAMLPolicy ran first and an
192+
// unrelated broken yaml on the user's machine could fatal a
193+
// plugin-governed binary (build.go fail-CLOSES on policy errors when a
194+
// plugin is present).
195+
func TestApplyUserPolicyPruning_pluginRulesSkipBrokenYaml(t *testing.T) {
196+
cfgDir := tmpHome(t)
197+
t.Cleanup(cmdpolicy.ResetActiveForTesting)
198+
writePolicy(t, cfgDir, "::: not yaml :::") // broken on purpose
199+
200+
pluginRules := []cmdpolicy.PluginRule{
201+
{PluginName: "secaudit", Rule: &platform.Rule{
202+
Name: "docs-only",
203+
Allow: []string{"docs/**"},
204+
MaxRisk: "write",
205+
}},
206+
}
207+
root := fakeTree(t)
208+
if err := applyUserPolicyPruning(root, pluginRules); err != nil {
209+
t.Fatalf("plugin rules must shadow (and skip reading) yaml; broken yaml should not error, got %v", err)
210+
}
211+
212+
// Plugin rule actually applied: im/+send is outside docs/** -> hidden.
213+
if send := findLeaf(t, root, "im", "+send"); !send.Hidden {
214+
t.Errorf("im/+send should be hidden by plugin rule (not in docs/** allow)")
215+
}
216+
// docs/+update is within allow and at/below max_risk -> stays visible.
217+
if update := findLeaf(t, root, "docs", "+update"); update.Hidden {
218+
t.Errorf("docs/+update should remain visible under plugin rule")
219+
}
220+
}
221+
187222
// Semantically-invalid Rule (bad MaxRisk) reaches ValidateRule inside
188223
// Resolve and produces an error. This is the safety contract: a typo in
189224
// the rule must not silently lower the pruning bar.

extension/platform/README.md

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ You should see `audit` in the plugin list.
5959
| `Observer` | Before / After each command | No (fire-and-forget audit) |
6060
| `Wrap` | Around each command's RunE | Yes (return `*AbortError`) |
6161
| `On(Startup/Shutdown)` | Process lifecycle | N/A |
62-
| `Restrict(Rule)` | Bootstrap-time, single per binary | Denies whole subtrees |
62+
| `Restrict(Rule)` | Bootstrap-time, ≥1 per plugin | Denies whole subtrees |
6363

6464
### Plugin lifecycle
6565

@@ -102,10 +102,17 @@ the rejected dispatch.
102102
- A plugin calling `Restrict()` MUST declare `FailClosed`. The Builder
103103
flips it automatically; the lower-level `Plugin` interface rejects
104104
the mismatch with `restricts_mismatch`.
105-
- Only ONE plugin per binary can call `Restrict()`. Multi-plugin
106-
Restrict is a deliberate `plugin_conflict` error (single-rule
107-
ecosystem assumption). YAML policy at `~/.lark-cli/policy.yml` is
108-
shadowed by any plugin Restrict.
105+
- A plugin may call `Restrict()` more than once; each call adds one
106+
scoped Rule and the engine combines them with **OR** — a command is
107+
allowed when it satisfies every axis (allow / deny / max_risk /
108+
identities) of at least one rule. Note a rule's `deny` is scoped to
109+
that rule only and cannot veto another rule's allow. Only ONE plugin
110+
per binary may contribute rules, though: two DISTINCT plugins each
111+
calling `Restrict()` is a deliberate `multiple_restrict_plugins` error
112+
(single-owner assumption — an independent plugin must not be able to
113+
widen another's policy). YAML policy at `~/.lark-cli/policy.yml` (which
114+
may itself list several rules under `rules:`) is shadowed by any plugin
115+
Restrict.
109116
- The `Wrap` factory runs **once per command dispatch**, not at
110117
install time. Long-lived state (clients, caches, metrics counters)
111118
must live on the Plugin struct or in package-level variables.
@@ -115,7 +122,8 @@ the rejected dispatch.
115122
- Commands missing a `risk_level` annotation are denied by default
116123
when a Rule is active. Set `Rule.AllowUnannotated = true` (or
117124
`allow_unannotated: true` in yaml) to opt out during gradual
118-
adoption.
125+
adoption. With several rules this is per-rule: an unannotated command
126+
is allowed as long as one rule that opts in also grants it.
119127
- Risk annotation typos (e.g. `"wrtie"`) are always denied with
120128
`risk_invalid` plus a "did you mean" suggestion. `AllowUnannotated`
121129
does NOT bypass this — typo is a code bug, not a missing
@@ -144,8 +152,7 @@ messages are localised and may change between releases.
144152
| `duplicate_hook_name` | Same hook name registered twice within a plugin | Yes |
145153
| `invalid_hook_registration` | Hook factory returns nil / Wrap chain re-entry / etc. | Yes |
146154
| `invalid_rule` | Rule fails ValidateRule (malformed glob, bad MaxRisk, unknown Identity) | Yes |
147-
| `double_restrict` | Plugin called `r.Restrict()` more than once in one Install | Yes |
148-
| `multiple_restrict_plugins` | Two or more plugins each contributed Restrict | Yes |
155+
| `multiple_restrict_plugins` | Two or more DISTINCT plugins each contributed Restrict (one plugin may contribute several rules) | Yes |
149156
| `install_failed` | `Plugin.Install` returned a non-nil error | Yes |
150157
| `install_panic` | `Plugin.Install` panicked | Yes |
151158

@@ -165,6 +172,7 @@ might also be lying about being `FailOpen`).
165172
| `write_not_allowed` | Command risk is `write` / `high-risk-write` and exceeds Rule `max_risk` |
166173
| `risk_too_high` | Command risk exceeds Rule `max_risk` but is not a write (reserved for future risk levels) |
167174
| `identity_mismatch` | Command's `supportedIdentities` does not intersect Rule `identities` |
175+
| `no_matching_rule` | Several rules are active and the command satisfied none of them (the message summarises each rule's own rejection). Single-rule policies keep their specific reason_code instead |
168176
| `aggregate_all_denied` | Aggregate stub installed on a parent group because every live child was denied |
169177

170178
The `detail.layer` field distinguishes who rejected the call:

extension/platform/builder.go

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ type Builder struct {
3737
caps Capabilities
3838

3939
actions []func(Registrar)
40-
rule *Rule
40+
rules []*Rule
4141

4242
hookNames map[string]bool
4343
errs []error
@@ -125,15 +125,23 @@ func (b *Builder) On(event LifecycleEvent, hookName string, fn LifecycleHandler)
125125
// sets Restricts=true and FailurePolicy=FailClosed (the framework
126126
// requires both to coexist; the builder enforces the pairing so the
127127
// plugin author cannot accidentally ship a policy plugin under
128-
// FailOpen).
128+
// FailOpen). It may be called more than once; each call adds one scoped
129+
// Rule and the engine OR-combines them.
129130
func (b *Builder) Restrict(rule *Rule) *Builder {
130131
if rule == nil {
131132
b.errs = append(b.errs, errors.New("Restrict(nil): rule must not be nil"))
132133
return b
133134
}
134135
b.caps.Restricts = true
135136
b.caps.FailurePolicy = FailClosed
136-
b.rule = rule
137+
// Defensive clone: capture an independent snapshot so a caller that
138+
// reuses and mutates the same *Rule across multiple Restrict calls
139+
// gets distinct entries (mirrors the staging registrar's clone).
140+
cp := *rule
141+
cp.Allow = append([]string(nil), rule.Allow...)
142+
cp.Deny = append([]string(nil), rule.Deny...)
143+
cp.Identities = append([]Identity(nil), rule.Identities...)
144+
b.rules = append(b.rules, &cp)
137145
return b
138146
}
139147

@@ -143,7 +151,7 @@ func (b *Builder) Restrict(rule *Rule) *Builder {
143151
// The Restrict + FailOpen mismatch is checked here, not in the chained
144152
// setters, because the two methods may be called in either order.
145153
func (b *Builder) Build() (Plugin, error) {
146-
if b.rule != nil && b.caps.FailurePolicy == FailOpen {
154+
if len(b.rules) > 0 && b.caps.FailurePolicy == FailOpen {
147155
b.errs = append(b.errs, errors.New(
148156
"Restrict() requires FailClosed; do not call FailOpen() after Restrict()"))
149157
}
@@ -155,7 +163,7 @@ func (b *Builder) Build() (Plugin, error) {
155163
version: b.version,
156164
caps: b.caps,
157165
actions: b.actions,
158-
rule: b.rule,
166+
rules: b.rules,
159167
}, nil
160168
}
161169

@@ -198,15 +206,15 @@ type builtPlugin struct {
198206
version string
199207
caps Capabilities
200208
actions []func(Registrar)
201-
rule *Rule
209+
rules []*Rule
202210
}
203211

204212
func (p *builtPlugin) Name() string { return p.name }
205213
func (p *builtPlugin) Version() string { return p.version }
206214
func (p *builtPlugin) Capabilities() Capabilities { return p.caps }
207215
func (p *builtPlugin) Install(r Registrar) error {
208-
if p.rule != nil {
209-
r.Restrict(p.rule)
216+
for _, rule := range p.rules {
217+
r.Restrict(rule)
210218
}
211219
for _, action := range p.actions {
212220
action(r)

0 commit comments

Comments
 (0)