Add support for AddPoliciesEx

[nickzelei]

https://casbin.org/docs/management-api#addpoliciesex

AddPoliciesEx adds authorization rules to the current policy. If the rule already exists, the rule will not be added. But unlike AddPolicies, other non-existent rules are added instead of returning false directly

I tried using this method with this adapter and it results in duplicate rows being inserted.

My model file:

[request_definition]
r = sub, dom, obj, act

[policy_definition]
p = sub, dom, obj, act

[role_definition]
g = _, _, _

[policy_effect]
e = some(where (p.eft == allow))

[matchers]
m = g(r.sub, p.sub, r.dom) && r.dom == p.dom && r.obj == p.obj && r.act == p.act

Policies:

p, account_admin, account/123, *, *
p, account_admin, account/456, *, *

When calling this function it always seems to insert duplicates.

This method does fail if I add a unique constraint to my table.

Postgres v15

CREATE TABLE casbin_rule (
	p_type varchar(32) DEFAULT ''::character varying NOT NULL,
	v0 varchar(255) DEFAULT ''::character varying NOT NULL,
	v1 varchar(255) DEFAULT ''::character varying NOT NULL,
	v2 varchar(255) DEFAULT ''::character varying NOT NULL,
	v3 varchar(255) DEFAULT ''::character varying NOT NULL,
	v4 varchar(255) DEFAULT ''::character varying NOT NULL,
	v5 varchar(255) DEFAULT ''::character varying NOT NULL,
	created_at timestamptz DEFAULT CURRENT_TIMESTAMP NOT NULL,
	updated_at timestamptz DEFAULT CURRENT_TIMESTAMP NOT NULL
);
CREATE INDEX idx_casbin_rule ON casbin_rule USING btree (p_type, v0, v1);
CREATE UNIQUE INDEX idx_casbin_rule_unique_policy ON casbin_rule USING btree (p_type, v0, v1, v2, v3, v4, v5);

[Blank-Xu]

This package is to implement the interfaces for Casbin adapter like

	_ persist.Adapter          = new(Adapter)
	_ persist.FilteredAdapter  = new(Adapter)
	_ persist.BatchAdapter     = new(Adapter)
	_ persist.UpdatableAdapter = new(Adapter)

From your description, have you check the test cases like https://github.com/Blank-Xu/sql-adapter/blob/master/test/e2e/adapter_test.go#L118 and https://github.com/Blank-Xu/sql-adapter/blob/master/test/e2e/adapter_test.go#L152 ?

[nickzelei]

I did take a look at this. They don't seem to convey the use of the AddPoliciesEx function. But perhaps the notion here is that they shouldn't have to and that is the job of the Enforcer (forgive me if that's not accurate, I'm relatively new to casbin.

However, I ended up here after finding this issue in casbin main: casbin/casbin#1423 - they seem to think it's up to the adapter layer to handle this.
The casbin/gorm adapter fixed it (at least for postgres) by adding an on conflict clause in their inserts. That also assumes the table uses a unique index, however.
casbin/gorm-adapter#243

[Blank-Xu]

I got all your points now. Actually I had thought to add UNIQUE INDEX about this when I write the SQL like https://github.com/Blank-Xu/sql-adapter/blob/master/constants.go#L110 . Even the index could be changed to use all the fields like CREATE INDEX IF NOT EXISTS idx_%[1]s ON %[1]s (p_type,v0,v1,v2,v3,v4,v5);.

But I thought it may have issues like if AddPolicies have repeat rules and it will cause other problem. So I just added index for (p_type,v0,v1) and did not add UNIQUE INDEX.

For this simple solution maybe you could execute the SQL like CREATE UNIQUE INDEX idx_casbin_rule_unique_policy ON casbin_rule USING btree (p_type, v0, v1, v2, v3, v4, v5); after you created the casbin_rule table.

We need consider more about this and maybe Casbin could fix this. @hsluoyz

[nickzelei]

Good callouts. Adding the unique key and handling the duplicate check in the adapter would go a long way towards being safe as well as handling races.

I have added the unique index to my casbin_rule table for my use cases. However, I can't use the AddPoliciesEx due to the adapter not handling the duplicate key error (similar to how the gorm one is).

But it definitely does seem like this is a bug in casbin and shouldn't really be cause for concern at the adapter layer.
I'm able to get around it by essentially writing the code myself using the lower-level casbin primitives HasPolicy and AddPolicy, but that isn't ideal as those higher-level functions exist for a reason! 😄

I took a cursory look at the AddPolciesEx code and it seems like that is what it is doing, but there must be a bug in there somewhere.

Of course, having a unique index on the table seems like a sure-fire way to handle the duplicates regardless (to better handle races too). So I'm thinking there might be multiple improvements that could be made.

[Blank-Xu]

Not sure if it is a good to add UNIQUE INDEX. Please check #13.

[nickzelei]

I've seen the mention of adding that index in various places, don't have immediate refs on hand though.
Seems like a sensible thing to do, however casbin does throw a hard error as it's not necessarily expecting it, which places the burden on the adapter (which makes sense as it is adapter specific.)