hsluoyz commented on PR #1739:
URL: https://github.com/apache/casbin/pull/1739#issuecomment-5175639168
Thanks for taking a look at this, but we're going to close this PR.
**It doesn't fix #1733.** A comma collision requires two rules with the same
field count
whose comma-joins are identical, e.g. `["alice","b,c","d"]` vs
`["alice,b","c","d"]`.
The rules in #1733 all differ in the resource field and cannot collide, and
a collision
would cause wrong deletions rather than missed ones. We also can't reproduce
#1733 on
master — batch-deleting rules with commas in the condition works correctly.
The reporter
is on v2.77.x and that issue still needs a fresh repro.
**It introduces a regression.** `policyKey()` was only applied in
`model/policy.go`.
`model/model.go:263` and `model/model.go:343` still build `PolicyMap` with
`strings.Join(policy, ",")`, and both run on every `LoadPolicy`. With
`examples/priority_model_explicit.conf`,
`RemovePolicy({"1","alice","data1","write","allow"})`
returns `true` but deletes `[10 data1_deny_group data1 write deny]` instead
— silent
authorization data corruption, correct on master.
**The tests don't cover the change.** All four new tests pass against
unmodified master,
assert via `t.Logf` instead of failing, and neither file is gofmt-clean.
The underlying key-collision concern is real, though narrow. If you'd like
to pursue it,
please open a separate issue and a new PR that routes every `PolicyMap`
write through one
helper (including the two `model.go` sites), and includes a test that fails
without the fix.
--
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]
