search

casbin / xorm-adapter

Xorm adapter for Casbin
https://github.com/casbin/casbin
Apache License 2.0
384 stars 58 forks source link

UpdateFilteredPolicies returns the oldPolicy containing ptype, Is this a bug? #49

Closed kkbblzq closed 2 years ago

kkbblzq commented 2 years ago

https://github.com/casbin/xorm-adapter/blob/eadfe045b52f90eb635d0a097dafdb2ef53e75ed/adapter.go#L552 https://github.com/casbin/xorm-adapter/blob/eadfe045b52f90eb635d0a097dafdb2ef53e75ed/adapter.go#L560-L562

will returns the oldPolicy containing ptype

but in usage: https://github.com/casbin/casbin/blob/c4cf679dfe87889c4228be7172a346282c2efdd4/internal_api.go#L339 https://github.com/casbin/casbin/blob/c4cf679dfe87889c4228be7172a346282c2efdd4/model/policy.go#L271

model[sec][ptype].PolicyMap key does not contain ptype so, it cannot delete policy correctly

I'm not sure if this is an adapter bug or a casbin bug

casbin-bot commented 2 years ago

@tangyang9464 @closetool @sagilio

hsluoyz commented 2 years ago

@tangyang9464

tangyang9464 commented 2 years ago

@kkbblzq I think UpdateFilteredPolicies shouldn't return oldpolicy containing ptype. But almost all adapter implementations follow xorm-adapter. So it seems easier to modify casbin to be compatible.

huijiezheng commented 2 years ago

https://github.com/casbin/casbin/blob/9b434269935411da709f6c85a6b59e9ce2401e4c/internal_api.go#L337-L342 There will panic when the length of oldRules and newRules is not equal, is it better to constraint Adapter return the same format oldRules? I think 337 ~342 code should be remove because the newRules may be empty, or when newRules is empty should return error? @tangyang9464

tangyang9464 commented 2 years ago

@JalinWang

JalinWang commented 2 years ago

@huijiezheng Thanks. I think you are right and proposed a PR: https://github.com/casbin/casbin/pull/1064