bitflicker64 commented on code in PR #1755:
URL: https://github.com/apache/casbin/pull/1755#discussion_r3983735105
##########
persist/string-adapter/adapter.go:
##########
@@ -49,7 +49,9 @@ func (a *Adapter) LoadPolicy(model model.Model) error {
if str == "" {
continue
}
- _ = persist.LoadPolicyLine(str, model)
+ if err := persist.LoadPolicyLine(str, model); err != nil {
+ return err
+ }
Review Comment:
Trim before the handler, the way `loadPolicyFile` does, so the two adapters
agree on what a line is and not just on error propagation.
Without it, `" # a note"` becomes `missing required section #` where it
used to be skipped. It also leaves `" "` panicking in `LoadPolicyArray` at
`key[:1]`, because `str == ""` does not catch a whitespace-only line. To be
clear, that panic is not introduced by this PR: I reproduced it on the
pre-change loop body too. The trim just closes it for free.
```suggestion
if line := strings.TrimSpace(str); line != "" {
if err := persist.LoadPolicyLine(line, model); err !=
nil {
return err
}
}
```
`strings` is already imported. With this applied, both inputs above load
cleanly, the malformed-quote case still errors, gofmt and vet are clean, and
`go test ./...` is green.
##########
persist/string-adapter/adapter_test.go:
##########
@@ -171,3 +171,16 @@ g, alice, data_group_admin
t.Error("unexpected enforce result")
}
}
+
+// Test_LoadPolicyMalformedLine verifies that a malformed policy line (here an
+// unterminated quoted field) makes LoadPolicy return an error instead of
+// silently producing a model that lacks the rule.
+func Test_LoadPolicyMalformedLine(t *testing.T) {
+ a := NewAdapter(`p, alice, data1, "read`)
+ m := model.NewModel()
+
+ err := a.LoadPolicy(m)
+ if err == nil {
+ t.Fatal("LoadPolicy() error = nil, want a parse error for the
unterminated quoted field")
+ }
+}
Review Comment:
This does fail before the change, so it earns its place. But it is close to
vacuous: `model.NewModel()` has no model text loaded, and against that model a
well-formed line errors too.
```
"p, alice, data1, read" -> missing required section p
"p, alice, data1, \"read" -> parse error ... extraneous or missing " in
quoted-field
```
The assertion cannot tell those apart, so it would keep passing if the csv
quoting behaviour regressed entirely. Loading a real model and pinning the
error message fixes both:
```suggestion
func Test_LoadPolicyMalformedLine(t *testing.T) {
conf := `
[request_definition]
r = sub, obj, act
[policy_definition]
p = sub, obj, act
[policy_effect]
e = some(where (p.eft == allow))
[matchers]
m = r.sub == p.sub && r.obj == p.obj && r.act == p.act
`
m := model.NewModel()
if err := m.LoadModelFromText(conf); err != nil {
t.Fatal(err)
}
// A well-formed line must still load against this model, so the failure
// below can only come from the unterminated quote.
if err := NewAdapter(`p, alice, data1, read`).LoadPolicy(m); err != nil
{
t.Fatalf("well-formed line should load, got %v", err)
}
err := NewAdapter(`p, alice, data1, "read`).LoadPolicy(m)
if err == nil {
t.Fatal("LoadPolicy() error = nil, want a parse error for the
unterminated quoted field")
}
if !strings.Contains(err.Error(), "quoted-field") {
t.Fatalf("want a csv quoting error, got %v", err)
}
}
```
`strings` is already imported in this file. I checked this passes on the
branch and still fails against the pre-change loop, on the same assertion as
before.
##########
persist/string-adapter/adapter.go:
##########
@@ -49,7 +49,9 @@ func (a *Adapter) LoadPolicy(model model.Model) error {
if str == "" {
continue
}
- _ = persist.LoadPolicyLine(str, model)
+ if err := persist.LoadPolicyLine(str, model); err != nil {
+ return err
Review Comment:
Not blocking, just so it is a deliberate choice: returning here leaves
`model` holding every rule parsed before the bad line. The file adapter does
the same, so this is consistent rather than new, and
`Enforcer.loadPolicyFromAdapter` loads into a scratch copy and swaps only on
success, so the partial model is not reachable through `NewEnforcer` or
`Enforcer.LoadPolicy`. It is visible only to code calling `adapter.LoadPolicy`
on a model it owns.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
