Duplicate entries

[killua-eu]

Hi,

I noticed that

$e = new Enforcer('casbin_model.conf', $adapter);
$e->addPermissionForUser('eve', 'data3', 'read');

run more then once will add duplicate database entries. I don't think this should be possible, shouldn't the function skip adding the permission in case it already exists? Same for addPolicy and possibly other functions.

[hsluoyz]

Hi @killua-eu ,

I have tried this example:

https://github.com/php-casbin/database-adapter/blob/abac889f57c8a179c035a494b20ad4cfa8371caa/tests/AdapterTest.php#L47-L55

I changed it to this:

public function testLoadPolicy()
    {
        $e = $this->getEnforcer();
        $this->assertFalse($e->addPermissionForUser('alice', 'data1', 'read'));
        $this->assertFalse($e->addPermissionForUser('alice', 'data1', 'read'));
        $this->assertTrue($e->enforce('alice', 'data1', 'read'));
        $this->assertFalse($e->enforce('bob', 'data1', 'read'));
        $this->assertTrue($e->enforce('bob', 'data2', 'write'));
        $this->assertTrue($e->enforce('alice', 'data2', 'read'));
        $this->assertTrue($e->enforce('alice', 'data2', 'write'));
    }

After running this test, the DB table is still like this:

No duplicated entries.

You can also check the return value of addPermissionForUser(), false means "not affected, already existed".

[leeqvip]

Hi，@killua-eu Which database do you use? Mysql? Show the contents of the casbin_model.conf file.

[killua-eu]

Hey @leeqvip , @hsluoyz

my model is

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

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

[role_definition]
g = _, _, _
g2 = _, _

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

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

When I add the first entry, i.e.

$e->addNamedGroupingPolicy('g2', ['system', '*']);
$b = $e->savePolicy();

I immediately get repetitive lines upon consecutive runs of the script (when using mysql). This doesn't apply for the file adapter that I was using instead. I hoped just adding a virtual column would work

DROP TABLE IF EXISTS casbin_rule;
CREATE TABLE `casbin_rule` (
  `id` int NOT NULL AUTO_INCREMENT,
  `ptype` varchar(255) NOT NULL,
  `v0` varchar(255) DEFAULT NULL,
  `v1` varchar(255) DEFAULT NULL,
  `v2` varchar(255) DEFAULT NULL,
  `v3` varchar(255) DEFAULT NULL,
  `v4` varchar(255) DEFAULT NULL,
  `v5` varchar(255) DEFAULT NULL,
  `hash` VARCHAR(32) GENERATED ALWAYS AS (MD5(CONCAT_WS('.', `ptype`, `v0`, `v1`, `v2`, `v3`, `v4`, `v5`))) STORED,
  PRIMARY KEY (`id`),
  UNIQUE KEY `unique_rule` (`hash`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci;

but the unique constraint would require INSERT IGNORE to be used instead of INSERT.

[killua-eu]

Tested the above, the major pain is, that the AUTO_INCREMENT on id will create a set of ids. There are workarounds, such as https://www.percona.com/blog/avoiding-auto-increment-holes-on-innodb-with-insert-ignore/ , but I'd probably use uuid columns as they are safe in distributed environment ... so, modifying the above to

`id` binary(16) NOT NULL DEFAULT (uuid_to_bin(uuid(),true)) 

imho solves this elegantly with insert ignore. I'm not sure if you don't rely on the numeric id though.

Alternatively, the hash column could be used as well as it is the unique constraint.