Skip to content

Commit 0a8629a

Browse files
hsluoyzhsluoyz
committed
fix: re-sort policies by subject hierarchy on policy change
1 parent 59f2353 commit 0a8629a

4 files changed

Lines changed: 106 additions & 41 deletions

File tree

‎examples/subject_priority_policy_simple.csv‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
p, group, data1, read, deny
2+
p, user, data1, read, allow
3+
4+
g, user, group

‎src/internalEnforcer.ts‎

Lines changed: 26 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,18 @@ import { PolicyOp } from './model';
2121
* InternalEnforcer = CoreEnforcer + Internal API.
2222
*/
2323
export class InternalEnforcer extends CoreEnforcer {
24+
/**
25+
* The subjectPriority effect resolves conflicts by the order of the "p" rules, so that order has
26+
* to be rebuilt whenever a "p" or "g" rule changes, not only when the policy is loaded from an
27+
* adapter. It is a no-op for models that do not use the effect.
28+
*/
29+
private resortPoliciesBySubjectHierarchy(ok: boolean): boolean {
30+
if (ok) {
31+
this.model.sortPoliciesBySubjectHierarchy();
32+
}
33+
return ok;
34+
}
35+
2436
/**
2537
* addPolicyInternal adds a rule to the current policy.
2638
*/
@@ -56,7 +68,7 @@ export class InternalEnforcer extends CoreEnforcer {
5668
if (sec === 'g' && ok) {
5769
await this.buildIncrementalRoleLinks(PolicyOp.PolicyAdd, ptype, [rule]);
5870
}
59-
return ok;
71+
return this.resortPoliciesBySubjectHierarchy(ok);
6072
}
6173

6274
// addPolicies adds rules to the current policy.
@@ -98,7 +110,7 @@ export class InternalEnforcer extends CoreEnforcer {
98110
if (sec === 'g' && ok && effects?.length) {
99111
await this.buildIncrementalRoleLinks(PolicyOp.PolicyAdd, ptype, effects);
100112
}
101-
return ok;
113+
return this.resortPoliciesBySubjectHierarchy(ok);
102114
}
103115

104116
/**
@@ -145,7 +157,7 @@ export class InternalEnforcer extends CoreEnforcer {
145157
if (sec === 'g' && ok && effects?.length) {
146158
await this.buildIncrementalRoleLinks(PolicyOp.PolicyAdd, ptype, effects);
147159
}
148-
return ok;
160+
return this.resortPoliciesBySubjectHierarchy(ok);
149161
}
150162

151163
/**
@@ -192,7 +204,7 @@ export class InternalEnforcer extends CoreEnforcer {
192204
await this.buildIncrementalRoleLinks(PolicyOp.PolicyAdd, ptype, [newRule]);
193205
}
194206

195-
return ok;
207+
return this.resortPoliciesBySubjectHierarchy(ok);
196208
}
197209

198210
/**
@@ -229,7 +241,7 @@ export class InternalEnforcer extends CoreEnforcer {
229241
if (sec === 'g' && ok) {
230242
await this.buildIncrementalRoleLinks(PolicyOp.PolicyRemove, ptype, [rule]);
231243
}
232-
return ok;
244+
return this.resortPoliciesBySubjectHierarchy(ok);
233245
}
234246

235247
// removePolicies removes rules from the current policy.
@@ -270,7 +282,7 @@ export class InternalEnforcer extends CoreEnforcer {
270282
if (sec === 'g' && ok && effects?.length) {
271283
await this.buildIncrementalRoleLinks(PolicyOp.PolicyRemove, ptype, effects);
272284
}
273-
return ok;
285+
return this.resortPoliciesBySubjectHierarchy(ok);
274286
}
275287

276288
/**
@@ -309,7 +321,7 @@ export class InternalEnforcer extends CoreEnforcer {
309321
if (sec === 'g' && ok && effects?.length) {
310322
await this.buildIncrementalRoleLinks(PolicyOp.PolicyRemove, ptype, effects);
311323
}
312-
return ok;
324+
return this.resortPoliciesBySubjectHierarchy(ok);
313325
}
314326

315327
/**
@@ -336,7 +348,7 @@ export class InternalEnforcer extends CoreEnforcer {
336348
if (sec === 'g' && ok) {
337349
await this.buildIncrementalRoleLinks(PolicyOp.PolicyAdd, ptype, [rule]);
338350
}
339-
return ok;
351+
return this.resortPoliciesBySubjectHierarchy(ok);
340352
}
341353

342354
protected async addPoliciesWithoutNotify(sec: string, ptype: string, rules: string[][]): Promise<boolean> {
@@ -350,7 +362,7 @@ export class InternalEnforcer extends CoreEnforcer {
350362
if (sec === 'g' && ok && effects?.length) {
351363
await this.buildIncrementalRoleLinks(PolicyOp.PolicyAdd, ptype, effects);
352364
}
353-
return ok;
365+
return this.resortPoliciesBySubjectHierarchy(ok);
354366
}
355367

356368
protected async addPoliciesWithoutNotifyEx(sec: string, ptype: string, rules: string[][]): Promise<boolean> {
@@ -363,7 +375,7 @@ export class InternalEnforcer extends CoreEnforcer {
363375
if (sec === 'g' && ok && effects?.length) {
364376
await this.buildIncrementalRoleLinks(PolicyOp.PolicyAdd, ptype, effects);
365377
}
366-
return ok;
378+
return this.resortPoliciesBySubjectHierarchy(ok);
367379
}
368380

369381
protected async updatePolicyWithoutNotify(sec: string, ptype: string, oldRule: string[], newRule: string[]): Promise<boolean> {
@@ -376,7 +388,7 @@ export class InternalEnforcer extends CoreEnforcer {
376388
await this.buildIncrementalRoleLinks(PolicyOp.PolicyRemove, ptype, [oldRule]);
377389
await this.buildIncrementalRoleLinks(PolicyOp.PolicyAdd, ptype, [newRule]);
378390
}
379-
return ok;
391+
return this.resortPoliciesBySubjectHierarchy(ok);
380392
}
381393

382394
protected async removePolicyWithoutNotify(sec: string, ptype: string, rule: string[]): Promise<boolean> {
@@ -388,7 +400,7 @@ export class InternalEnforcer extends CoreEnforcer {
388400
if (sec === 'g' && ok) {
389401
await this.buildIncrementalRoleLinks(PolicyOp.PolicyRemove, ptype, [rule]);
390402
}
391-
return ok;
403+
return this.resortPoliciesBySubjectHierarchy(ok);
392404
}
393405

394406
protected async removePoliciesWithoutNotify(sec: string, ptype: string, rules: string[][]): Promise<boolean> {
@@ -402,7 +414,7 @@ export class InternalEnforcer extends CoreEnforcer {
402414
if (sec === 'g' && ok && effects?.length) {
403415
await this.buildIncrementalRoleLinks(PolicyOp.PolicyRemove, ptype, effects);
404416
}
405-
return ok;
417+
return this.resortPoliciesBySubjectHierarchy(ok);
406418
}
407419

408420
protected async removeFilteredPolicyWithoutNotify(
@@ -415,7 +427,7 @@ export class InternalEnforcer extends CoreEnforcer {
415427
if (sec === 'g' && ok && effects?.length) {
416428
await this.buildIncrementalRoleLinks(PolicyOp.PolicyRemove, ptype, effects);
417429
}
418-
return ok;
430+
return this.resortPoliciesBySubjectHierarchy(ok);
419431
}
420432

421433
protected async updatePoliciesWithoutNotify(sec: string, ptype: string, oldRules: string[][], newRules: string[][]): Promise<boolean> {

‎src/model/model.ts‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -512,19 +512,23 @@ export class Model {
512512
return;
513513
}
514514

515+
const groupPolicies = this.model.get('g')?.get('g')?.policy;
516+
if (!groupPolicies) {
517+
return;
518+
}
519+
515520
this.model.get('p')?.forEach((assertion, ptype) => {
516521
const domainIndex = this.getFieldIndex(ptype, FieldIndex.Domain);
517522
const subIndex = this.getFieldIndex(ptype, FieldIndex.Subject);
518-
// eslint-disable-next-line
519-
const subjectHierarchyMap = this.getSubjectHierarchyMap(this.model.get('g')!.get('g')!.policy);
523+
const subjectHierarchyMap = this.getSubjectHierarchyMap(groupPolicies);
520524

521525
assertion.policy.sort((policyA, policyB) => {
522526
const domainA = domainIndex === -1 ? defaultDomain : policyA[domainIndex];
523527
const domainB = domainIndex === -1 ? defaultDomain : policyB[domainIndex];
524-
// eslint-disable-next-line
525-
const priorityA = subjectHierarchyMap.get(this.getNameWithDomain(domainA, policyA[subIndex]))!;
526-
// eslint-disable-next-line
527-
const priorityB = subjectHierarchyMap.get(this.getNameWithDomain(domainB, policyB[subIndex]))!;
528+
// A subject that takes part in no "g" rule has no depth, so it sorts as a root would.
529+
// Without the fallback the subtraction below yields NaN and scrambles the whole array.
530+
const priorityA = subjectHierarchyMap.get(this.getNameWithDomain(domainA, policyA[subIndex])) ?? 0;
531+
const priorityB = subjectHierarchyMap.get(this.getNameWithDomain(domainB, policyB[subIndex])) ?? 0;
528532
return priorityB - priorityA;
529533
});
530534
});

‎test/enforcer.test.ts‎

Lines changed: 66 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -674,8 +674,8 @@ test('TestEnforceEx', async () => {
674674

675675
await e.addPermissionForUser('alice', 'data1', 'invalid');
676676

677-
testEnforceEx(e, 'alice', 'data1', 'read', [false, []]);
678-
testEnforceEx(e, 'alice', 'data1', 'invalid', [true, ['alice', 'data1', 'invalid']]);
677+
await testEnforceEx(e, 'alice', 'data1', 'read', [false, []]);
678+
await testEnforceEx(e, 'alice', 'data1', 'invalid', [true, ['alice', 'data1', 'invalid']]);
679679
});
680680

681681
test('TestSyncEnforceEx', async () => {
@@ -718,45 +718,90 @@ test('TestKeyGet2', async () => {
718718

719719
test('TestEnforceExWithRBACDenyModel', async () => {
720720
const e = await newEnforcer('examples/rbac_with_deny_model.conf', 'examples/rbac_with_deny_policy.csv');
721-
testEnforceEx(e, 'alice', 'data1', 'read', [true, ['alice', 'data1', 'read', 'allow']]);
722-
testEnforceEx(e, 'bob', 'data2', 'write', [true, ['bob', 'data2', 'write', 'allow']]);
723-
testEnforceEx(e, 'alice', 'data2', 'write', [false, ['alice', 'data2', 'write', 'deny']]);
724-
testEnforceEx(e, 'data2_admin', 'data2', 'read', [true, ['data2_admin', 'data2', 'read', 'allow']]);
725-
testEnforceEx(e, 'data2_admin', 'data2', 'write', [true, ['data2_admin', 'data2', 'write', 'allow']]);
726-
testEnforceEx(e, 'data2_admin', 'data1', 'read', [false, []]);
727-
testEnforceEx(e, 'data2_admin', 'data1', 'write', [false, []]);
721+
await testEnforceEx(e, 'alice', 'data1', 'read', [true, ['alice', 'data1', 'read', 'allow']]);
722+
await testEnforceEx(e, 'bob', 'data2', 'write', [true, ['bob', 'data2', 'write', 'allow']]);
723+
await testEnforceEx(e, 'alice', 'data2', 'write', [false, ['alice', 'data2', 'write', 'deny']]);
724+
await testEnforceEx(e, 'data2_admin', 'data2', 'read', [true, ['data2_admin', 'data2', 'read', 'allow']]);
725+
await testEnforceEx(e, 'data2_admin', 'data2', 'write', [true, ['data2_admin', 'data2', 'write', 'allow']]);
726+
await testEnforceEx(e, 'data2_admin', 'data1', 'read', [false, []]);
727+
await testEnforceEx(e, 'data2_admin', 'data1', 'write', [false, []]);
728728
});
729729

730730
test('TestEnforceExWithGlobModel', async () => {
731731
const e = await newEnforcer('examples/glob_model.conf', 'examples/glob_policy.csv');
732-
testEnforceEx(e, 'u1', '/foo/1', 'read', [true, ['u1', '/foo/*', 'read']]);
733-
testEnforceEx(e, 'u2', '/foo1', 'read', [true, ['u2', '/foo*', 'read']]);
734-
testEnforceEx(e, 'u3', '/foo/2', 'read', [false, []]);
732+
await testEnforceEx(e, 'u1', '/foo/1', 'read', [true, ['u1', '/foo/*', 'read']]);
733+
await testEnforceEx(e, 'u2', '/foo1', 'read', [true, ['u2', '/foo*', 'read']]);
734+
await testEnforceEx(e, 'u3', '/foo/2', 'read', [false, []]);
735735
});
736736

737737
test('TestEnforceExWithKeyMatchModel', async () => {
738738
const e = await newEnforcer('examples/keymatch_model.conf', 'examples/keymatch_policy.csv');
739-
testEnforceEx(e, 'alice', '/alice_data/1', 'GET', [true, ['alice', '/alice_data/*', 'GET']]);
740-
testEnforceEx(e, 'bob', '/alice_data/1', 'POST', [false, []]);
739+
await testEnforceEx(e, 'alice', '/alice_data/1', 'GET', [true, ['alice', '/alice_data/*', 'GET']]);
740+
await testEnforceEx(e, 'bob', '/alice_data/1', 'POST', [false, []]);
741741
});
742742

743743
test('TestEnforceExWithPriorityModel', async () => {
744744
const e = await newEnforcer('examples/priority_model.conf', 'examples/priority_policy.csv');
745-
testEnforceEx(e, 'alice', 'data1', 'read', [true, ['alice', 'data1', 'read', 'allow']]);
746-
testEnforceEx(e, 'bob', 'data2', 'read', [true, ['data2_allow_group', 'data2', 'read', 'allow']]);
747-
testEnforceEx(e, 'alice', 'data2', 'read', [false, []]);
745+
await testEnforceEx(e, 'alice', 'data1', 'read', [true, ['alice', 'data1', 'read', 'allow']]);
746+
await testEnforceEx(e, 'bob', 'data2', 'read', [true, ['data2_allow_group', 'data2', 'read', 'allow']]);
747+
await testEnforceEx(e, 'alice', 'data2', 'read', [false, []]);
748748
});
749749

750750
test('TestSubjectPriority', async () => {
751751
const e = await newEnforcer('examples/subject_priority_model.conf', 'examples/subject_priority_policy.csv');
752-
testEnforceEx(e, 'jane', 'data1', 'read', [true, ['jane', 'data1', 'read', 'allow']]);
753-
testEnforceEx(e, 'alice', 'data1', 'read', [true, ['alice', 'data1', 'read', 'allow']]);
752+
await testEnforceEx(e, 'jane', 'data1', 'read', [true, ['jane', 'data1', 'read', 'allow']]);
753+
await testEnforceEx(e, 'alice', 'data1', 'read', [true, ['alice', 'data1', 'read', 'allow']]);
754+
});
755+
756+
test('TestSubjectPriority with CSV converted to addPolicy/addGroupingPolicy', async () => {
757+
const e = await newEnforcer('examples/subject_priority_model.conf');
758+
759+
await e.addPolicy('root', 'data1', 'read', 'deny');
760+
await e.addPolicy('admin', 'data1', 'read', 'deny');
761+
await e.addPolicy('editor', 'data1', 'read', 'deny');
762+
await e.addPolicy('subscriber', 'data1', 'read', 'deny');
763+
await e.addPolicy('jane', 'data1', 'read', 'allow');
764+
await e.addPolicy('alice', 'data1', 'read', 'allow');
765+
766+
await e.addGroupingPolicy('admin', 'root');
767+
await e.addGroupingPolicy('editor', 'admin');
768+
await e.addGroupingPolicy('subscriber', 'admin');
769+
await e.addGroupingPolicy('jane', 'editor');
770+
await e.addGroupingPolicy('alice', 'subscriber');
771+
772+
await testEnforceEx(e, 'jane', 'data1', 'read', [true, ['jane', 'data1', 'read', 'allow']]);
773+
await testEnforceEx(e, 'alice', 'data1', 'read', [true, ['alice', 'data1', 'read', 'allow']]);
774+
});
775+
776+
test('TestSubjectPriority simpler with CSV', async () => {
777+
const e = await newEnforcer('examples/subject_priority_model.conf', 'examples/subject_priority_policy_simple.csv');
778+
779+
await testEnforceEx(e, 'user', 'data1', 'read', [true, ['user', 'data1', 'read', 'allow']]);
780+
});
781+
782+
test('TestSubjectPriority simpler with addPolicy', async () => {
783+
const e = await newEnforcer('examples/subject_priority_model.conf');
784+
785+
await e.addPolicy('group', 'data1', 'read', 'deny');
786+
await e.addPolicy('user', 'data1', 'read', 'allow');
787+
await e.addGroupingPolicy('user', 'group');
788+
789+
await testEnforceEx(e, 'user', 'data1', 'read', [true, ['user', 'data1', 'read', 'allow']]);
790+
});
791+
792+
test('TestSubjectPriority is kept after removing a grouping policy', async () => {
793+
const e = await newEnforcer('examples/subject_priority_model.conf', 'examples/subject_priority_policy.csv');
794+
795+
// jane loses her role, so only the "jane" rule itself is left to match her.
796+
await e.removeGroupingPolicy('jane', 'editor');
797+
await testEnforceEx(e, 'jane', 'data1', 'read', [true, ['jane', 'data1', 'read', 'allow']]);
798+
await testEnforceEx(e, 'alice', 'data1', 'read', [true, ['alice', 'data1', 'read', 'allow']]);
754799
});
755800

756801
test('TestSubjectPriorityWithDomain', async () => {
757802
const e = await newEnforcer('examples/subject_priority_model_with_domain.conf', 'examples/subject_priority_policy_with_domain.csv');
758-
testEnforceEx(e, 'alice', 'data1', 'write', [true, ['alice', 'data1', 'domain1', 'write', 'allow']], 'domain1');
759-
testEnforceEx(e, 'bob', 'data2', 'write', [true, ['bob', 'data2', 'domain2', 'write', 'allow']], 'domain2');
803+
await testEnforceEx(e, 'alice', 'data1', 'write', [true, ['alice', 'data1', 'domain1', 'write', 'allow']], 'domain1');
804+
await testEnforceEx(e, 'bob', 'data2', 'write', [true, ['bob', 'data2', 'domain2', 'write', 'allow']], 'domain2');
760805
});
761806

762807
test('TestEnforcerWithScopeFileSystem', async () => {

0 commit comments

Comments
 (0)