Santoshkumarpuppala commented on PR #1758:
URL: https://github.com/apache/casbin/pull/1758#issuecomment-5555259073
Ran `AssertDeny` against the repo's own `keymatch2` fixtures at `ee5784e`,
and the diagnostic contradicts itself on line 2:
```
authztest: expected DENY, got ALLOW for (alice, /alice_data/123, GET)
no policy matched; closest candidates:
p, alice, /alice_data/:resource, GET
sub: ok ('alice')
act: ok ('GET')
obj: '/alice_data/123' does not match '/alice_data/:resource' (not a
wildcard)
```
The header says `got ALLOW`; the next line says `no policy matched`. Both
come from the same call. And the per-position note blames `obj` — which is the
position that actually *did* match. `EnforceEx` on the identical request
returns `[alice /alice_data/:resource GET]`: exactly the rule the diagnostic
says did not match, matched at exactly the position it says failed.
**Why the two disagree.** `AssertDeny` already has the real verdict at `:81`
(`ok, err := e.Enforce(rvals...)`) and drops it at `:89` (`d := Diagnose(e,
rvals...)`). `Diagnose` then recomputes the verdict structurally at `:123`:
```go
allowed := misses[0].FullyMatched()
```
`FullyMatched()` is `len(n.Failed) == 0` (`:105`), and the structural pass
can't see `keyMatch2`, so it records a failure at `obj` and picks the "no
policy matched" branch — while `Enforce` said allow. The header is
authoritative in tone and computed from a weaker mechanism than the decision it
describes.
**The documented safeguard for this isn't implemented.** `:110-115` says
matchers using built-in functions "may legitimately match in ways the
structural analysis cannot see; **the output is labelled accordingly**."
`keyMatch`, `regexMatch` and "structural" appear nowhere in the package outside
that comment — the file has 14 output strings and none carries such a label. So
the one thing that would tell a reader not to trust the header is the thing
that's missing, which is why the contradiction reads as authoritative rather
than as a caveat.
**The fix I'd suggest keeps the signature.** `Diagnose` already takes the
`IEnforcer`, so it can ask rather than infer:
```go
allowed, explain, err := e.EnforceEx(rvals...)
```
`EnforceEx` is on the `IEnforcer` interface (`enforcer_interface.go:63`) and
returns the genuinely matched rule (`enforcer.go:912-914`). Two things fall
out: the header always agrees with the enforcer because it *is* the enforcer's
answer, and on the allow path you can print the real matched rule instead of
near-misses — which is the more useful output anyway when someone is debugging
an unexpected ALLOW.
I'd avoid threading the verdict in as a parameter (`Diagnose(e, allowed
bool, rvals...)`). That lets a caller pass a verdict that disagrees with the
enforcer, which is the same failure this report is about, just relocated to the
call site. Asking the enforcer inside `Diagnose` can't drift.
**Test.** `examples/keymatch2_model.conf` and
`examples/keymatch2_policy.csv` already ship, so it's a fixture load plus one
assertion, and it would be the package's first test to reach the documented
limitation. Worth noting `go test ./authztest/` is green at `ee5784e` with this
present — no current test exercises a matcher function, so nothing fails today.
Method note: measured by executing the package at `ee5784e` (output above is
verbatim, `EnforceEx` result likewise). I haven't tried to judge whether the
near-miss ranking is right in general — only that the header and the enforcer
can disagree, and that here they do.
--
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]
