casbin / jcasbin

An authorization library that supports access control models like ACL, RBAC, ABAC in Java
https://casbin.org
Apache License 2.0
2.4k stars 464 forks source link

Delete the group API interface bug #277

Closed dhshenc closed 2 years ago

dhshenc commented 2 years ago

When using this interface enforcer. RemoveNameGroupingPolicy (" g ", "role1") to delete group, database data deleted successfully, but cached data is still there.

POM

<dependency>  
      <groupId>org.casbin</groupId>
      <artifactId>jcasbin</artifactId>
      <version>1.21.0</version>
</dependency>
<dependency>
      <groupId>org.casbin</groupId>
      <artifactId>jdbc-adapter</artifactId>
      <version>2.2.1</version>
</dependency>

Execute Code

image

Adapter

image

Datebase & Cache

image
hsluoyz commented 2 years ago

@tangyang9464

imp2002 commented 2 years ago

remove rules in the database: https://github.com/jcasbin/jdbc-adapter/blob/98a9aa8e48ea747f4b5128d74a1a2b9072ab562d/src/main/java/org/casbin/adapter/JDBCBaseAdapter.java#L411-L435

remove rules in the Redis: https://github.com/jcasbin/redis-adapter/blob/686bc3e769459ba1f4f98c4ffa519655450010b3/src/main/java/org/casbin/adapter/RedisAdapter.java#L91-L97

When rule incomplete, like above input role1, it will delete success in database, but fail in Redis. it matches the data in database (role1, role2) and (role1, role3), but didn't in Redis, In Redis if you want to remove (role1, role2), the input must be (role1, role2) too.

hsluoyz commented 2 years ago

@Memory63 which adapter do you use?

dhshenc commented 2 years ago

@Memory63 which adapter do you use?

JDBC. It's in the screenshot.

dhshenc commented 2 years ago

It feels strange when a delete succeeds in the database while the cache is still in place. Using full rule deletion is fine, but that approach should succeed and fail. Otherwise there is a sense of consistency.

hsluoyz commented 2 years ago

@Memory63 if you want to delete policy with a partial condition, you should use the "Filtered" API: RemoveFilteredNamedGroupingPolicy(): https://casbin.org/docs/en/management-api#removefilterednamedgroupingpolicy . RemoveNameGroupingPolicy() is only for matching all fields for a policy.

dhshenc commented 2 years ago

@hsluoyz RemoveFilteredNamedGroupingPolicy, the use of this API is no problem. RemoveNameGroupingPolicy is a full field match and is fine. However, the problem I feedback is that since the API parameters do not match, the data is also deleted, and only the database data is deleted. The cached data is not deleted, which is problematic, causing data consistency problems.

hsluoyz commented 2 years ago

@tangyang9464 @imp2002

imp2002 commented 2 years ago

Yes, it's a bug, in jdbc-adapter the removeNamedGroupingPolicy() implements with removePolicy() and removePolicy() implements with removeFilteredPolicy() directly, it leads to the wrong result.

code ref: https://github.com/jcasbin/jdbc-adapter/blob/98a9aa8e48ea747f4b5128d74a1a2b9072ab562d/src/main/java/org/casbin/adapter/JDBCBaseAdapter.java#L373-L378 image

hsluoyz commented 2 years ago

@imp2002 I don't think your pasted code has bug. It means to match all the fields in the filter, it is what removePolicy() should do: match all fields.

@tangyang9464 you wrote this part of code, what do you say?

tangyang9464 commented 2 years ago

@Memory63 First of all your usage is wrong, and at the same time the implementation of jdbc-adapter is wrong. removeNamedGroupingPolicy only matches a complete policy record. (params should berole1, role2) You should use removeFilteredPolicy to filter policy by some conditions. (params should be role1)

dhshenc commented 2 years ago

@tangyang9464 Yes, I did use it wrong. But the deletion did succeed, and even weirder, only the database was deleted, not the cache. I feel that this logic is wrong, you will either succeed in deleting or you will fail, regardless of the usage and input. Besides, my English is poor, so I don't know whether the translation software I use can correctly convey my meaning. I hope I can understand the meaning of my expression.

tangyang9464 commented 2 years ago

@Memory63 As I said above, the implementation of jdbc-adapter is wrong. We will fix it later. Could you @imp2002 fix it?

imp2002 commented 2 years ago

OK, i will fix it.