NUAAwqy opened a new pull request, #1768:
URL: https://github.com/apache/casbin/pull/1768
## What
`Model.UpdatePolicies` can lose rules from the model's duplicate-detection
index.
The function looks each old rule up in `PolicyMap` inside the same loop that
overwrites
`PolicyMap`. When a batch's new rules are a permutation of its old rules -
"swap these two rows",
"shift these three" - a later lookup finds the index that was just assigned
to an earlier new
rule. Two rows then point at one index, the row that was just written is
overwritten again, and
the key that was just added is deleted. The call still returns `true`.
Reproduce with two policies:
```go
// old: [alice, data1, read] [bob, data2, write]
// new: [bob, data2, write] [alice, data1, read] // just swap them
ok, _ := e.UpdatePolicies(old, new)
```
Before:
```
policy after UpdatePolicies: [["alice" "data1" "read"] ["bob" "data2"
"write"]] // the swap never happened
PolicyMap has 1 entries but Policy has 2 rows
GetPolicy reports ["bob" "data2" "write"] but HasPolicy says the model does
not have it
```
so the rule is listed by `GetPolicy` yet `HasPolicy` and `RemovePolicy` can
no longer reach it, and
it cannot be deleted from the model.
After: the slice holds the swapped rules, `PolicyMap` has exactly one key
per row, and
`HasPolicy` / `RemovePolicy` / `UpdatePolicy` agree with `GetPolicy`.
## Fix
Resolve and de-duplicate every index for the whole batch first, then write.
Old keys are all
deleted before any new key is inserted, because in a permuted batch a new
rule is still keyed as an
old rule. An unknown or repeated old rule returns `false` before the first
write, so the model is
never left half-updated - which is what `89584f4` ("feat: implement atomic
UpdatePolicies() API")
intended and what the old `rollbackFlag`/`defer` bookkeeping was trying,
unsuccessfully, to do.
`UpdatePolicy` (singular) is untouched: it does a single lookup before
writing and is not affected
by this.
## Test
`model/policy_test.go` (new, table-driven) keeps `PolicyMap` and the policy
slice in step for a
two-rule swap, a three-rule rotation, an ordinary batch of unrelated rules,
an unknown old rule and
a repeated old rule. The first two fail on `master` and pass here; the last
two pin the
"nothing is modified when the batch is rejected" behaviour.
- `go test -race ./...`: 8 packages ok
- `gofmt -l .`: empty
- `golangci-lint run` (v2.15.0, as `.golangci.yml` requires): 0 issues
## Notes
- No existing test covered a permuted batch: `management_api_test.go:273` is
the only
`UpdatePolicies` call and its new rules share no key with the surviving
rows, so it passes
either way and is unchanged.
- I have not run this through an external `UpdatableAdapter`/`Watcher`, so
the storage side of
`UpdatePolicies` is untested here; the change is confined to the model.
--
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]
