Skip to content

Commit cc6555e

Browse files
AndrienkoAleksandrAndrienkoAleksandr
authored
fix(rbac): remove admin metadata, when all admins removed from config (#1314)
* fix(rbac): remove admin metadata, when all admins removed from config Signed-off-by: Oleksandr Andriienko <[email protected]> * fix(rbac): count super users in warning message about no admins Signed-off-by: Oleksandr Andriienko <[email protected]> * fix(rbac): remove role, when it really unused Signed-off-by: Oleksandr Andriienko <[email protected]> --------- Signed-off-by: Oleksandr Andriienko <[email protected]>
1 parent c2c9e22 commit cc6555e

File tree

6 files changed

+81
-27
lines changed

6 files changed

+81
-27
lines changed

‎plugins/rbac-backend/src/helper.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ describe('helper.ts', () => {
9090
expect(mockEnforcerDelegate.removeGroupingPolicy).toHaveBeenCalledWith(
9191
['user:default/admin', roleName],
9292
source,
93-
true,
93+
false,
9494
);
9595
});
9696

‎plugins/rbac-backend/src/helper.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ export async function removeTheDifference(
3535

3636
for (const missingRole of missing) {
3737
const role = [missingRole, roleName];
38-
await enf.removeGroupingPolicy(role, source, true);
38+
await enf.removeGroupingPolicy(role, source, false);
3939
}
4040
}
4141

‎plugins/rbac-backend/src/service/enforcer-delegate.ts

Lines changed: 30 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -349,7 +349,21 @@ export class EnforcerDelegate {
349349
);
350350
await this.policyMetadataStorage.removePolicyMetadata(policy, trx);
351351
if (!isUpdate) {
352-
await this.roleMetadataStorage.removeRoleMetadata(roleEntity, trx);
352+
const roleMetadata = await this.roleMetadataStorage.findRoleMetadata(
353+
roleEntity,
354+
trx,
355+
);
356+
const groupPolicies = await this.enforcer.getFilteredGroupingPolicy(
357+
1,
358+
roleEntity,
359+
);
360+
if (
361+
roleMetadata &&
362+
groupPolicies.length === 0 &&
363+
roleEntity !== 'role:default/rbac_admin'
364+
) {
365+
await this.roleMetadataStorage.removeRoleMetadata(roleEntity, trx);
366+
}
353367
}
354368
const ok = await this.enforcer.removeGroupingPolicy(...policy);
355369
if (!ok) {
@@ -376,20 +390,12 @@ export class EnforcerDelegate {
376390
const trx = externalTrx ?? (await this.knex.transaction());
377391
try {
378392
for (const policy of policies) {
379-
const roleEntity = policy[1];
380393
await this.checkIfPolicyModifiable(
381394
policy,
382395
source,
383396
trx,
384397
allowToDeleteCSVFilePolicy,
385398
);
386-
if (!isUpdate) {
387-
if (
388-
await this.roleMetadataStorage.findRoleMetadata(roleEntity, trx)
389-
) {
390-
await this.roleMetadataStorage.removeRoleMetadata(roleEntity, trx);
391-
}
392-
}
393399
await this.policyMetadataStorage.removePolicyMetadata(policy, trx);
394400
}
395401

@@ -400,6 +406,21 @@ export class EnforcerDelegate {
400406
);
401407
}
402408

409+
if (!isUpdate) {
410+
const roleEntity = policies[0][1];
411+
const roleMetadata = await this.roleMetadataStorage.findRoleMetadata(
412+
roleEntity,
413+
trx,
414+
);
415+
const groupPolicies = await this.enforcer.getFilteredGroupingPolicy(
416+
1,
417+
roleEntity,
418+
);
419+
if (roleMetadata && groupPolicies.length === 0) {
420+
await this.roleMetadataStorage.removeRoleMetadata(roleEntity, trx);
421+
}
422+
}
423+
403424
if (!externalTrx) {
404425
await trx.commit();
405426
}

‎plugins/rbac-backend/src/service/permission-policy.test.ts

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -449,7 +449,12 @@ describe('RBACPermissionPolicy Tests', () => {
449449

450450
expect(await enf.getAllRoles()).toEqual(allEnfRoles);
451451

452-
expect(await enf.getPolicy()).toEqual(allEnfPolicies);
452+
const nonAdminPolicies = (await enf.getPolicy()).filter(
453+
(policy: string[]) => {
454+
return policy[0] !== 'role:default/rbac_admin';
455+
},
456+
);
457+
expect(nonAdminPolicies).toEqual(allEnfPolicies);
453458

454459
// policy metadata should to be removed
455460
expect(policyMetadataStorage.removePolicyMetadata).toHaveBeenCalledTimes(
@@ -513,7 +518,10 @@ describe('RBACPermissionPolicy Tests', () => {
513518

514519
expect(await enf.getGroupingPolicy()).toEqual(allEnfGroupPolicies);
515520

516-
expect(await enf.getPolicy()).toEqual(allEnfPolicies);
521+
const nonAdminPolicies = (await enf.getPolicy()).filter((p: string[]) => {
522+
return p[0] !== 'role:default/rbac_admin';
523+
});
524+
expect(nonAdminPolicies).toEqual(allEnfPolicies);
517525

518526
// policy metadata should to be removed
519527
expect(policyMetadataStorage.removePolicyMetadata).toHaveBeenCalledTimes(
@@ -604,7 +612,12 @@ describe('RBACPermissionPolicy Tests', () => {
604612

605613
expect(await enf.getGroupingPolicy()).toEqual(allEnfGroupPolicies);
606614

607-
expect(await enf.getPolicy()).toEqual(allEnfPolicies);
615+
const nonAdminPolicies = (await enf.getPolicy()).filter(
616+
(policy: string[]) => {
617+
return policy[0] !== 'role:default/rbac_admin';
618+
},
619+
);
620+
expect(nonAdminPolicies).toEqual(allEnfPolicies);
608621

609622
// policy metadata should to be removed
610623
expect(policyMetadataStorage.removePolicyMetadata).toHaveBeenCalledTimes(
@@ -685,7 +698,12 @@ describe('RBACPermissionPolicy Tests', () => {
685698

686699
expect(await enf.getGroupingPolicy()).toEqual(allEnfGroupPolicies);
687700

688-
expect(await enf.getPolicy()).toEqual(allEnfPolicies);
701+
const nonAdminPolicies = (await enf.getPolicy()).filter(
702+
(policy: string[]) => {
703+
return policy[0] !== 'role:default/rbac_admin';
704+
},
705+
);
706+
expect(nonAdminPolicies).toEqual(allEnfPolicies);
689707

690708
// policy metadata should to be removed
691709
expect(policyMetadataStorage.removePolicyMetadata).toHaveBeenCalledTimes(
@@ -749,7 +767,12 @@ describe('RBACPermissionPolicy Tests', () => {
749767

750768
expect(await enf.getGroupingPolicy()).toEqual(allEnfGroupPolicies);
751769

752-
expect(await enf.getPolicy()).toEqual(allEnfPolicies);
770+
const nonAdminPolicies = (await enf.getPolicy()).filter(
771+
(policy: string[]) => {
772+
return policy[0] !== 'role:default/rbac_admin';
773+
},
774+
);
775+
expect(nonAdminPolicies).toEqual(allEnfPolicies);
753776

754777
// policy metadata should to be removed
755778
expect(policyMetadataStorage.removePolicyMetadata).toHaveBeenCalledTimes(
@@ -828,7 +851,12 @@ describe('RBACPermissionPolicy Tests', () => {
828851

829852
expect(await enf.getGroupingPolicy()).toEqual(allEnfGroupPolicies);
830853

831-
expect(await enf.getPolicy()).toEqual(allEnfPolicies);
854+
const nonAdminPolicies = (await enf.getPolicy()).filter(
855+
(policy: string[]) => {
856+
return policy[0] !== 'role:default/rbac_admin';
857+
},
858+
);
859+
expect(nonAdminPolicies).toEqual(allEnfPolicies);
832860

833861
// policy metadata should to be removed
834862
expect(policyMetadataStorage.removePolicyMetadata).toHaveBeenCalledTimes(

‎plugins/rbac-backend/src/service/permission-policy.ts

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -177,17 +177,20 @@ export class RBACPermissionPolicy implements PermissionPolicy {
177177
}
178178
}
179179

180-
if (adminUsers && adminUsers.length > 0) {
181-
await useAdminsFromConfig(
182-
adminUsers,
183-
enforcerDelegate,
184-
roleMetadataStorage,
185-
knex,
186-
);
187-
await setAdminPermissions(enforcerDelegate);
188-
} else {
180+
await useAdminsFromConfig(
181+
adminUsers || [],
182+
enforcerDelegate,
183+
roleMetadataStorage,
184+
knex,
185+
);
186+
await setAdminPermissions(enforcerDelegate);
187+
188+
if (
189+
(!adminUsers || adminUsers.length === 0) &&
190+
(!superUsers || superUsers.length === 0)
191+
) {
189192
logger.warn(
190-
'There are no admins configured for the RBAC-backend plugin. The plugin may not work properly.',
193+
'There are no admins or super admins configured for the RBAC-backend plugin.',
191194
);
192195
}
193196

‎plugins/rbac-backend/src/service/policies-rest-api.test.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,8 @@ const mockEnforcer: Partial<EnforcerDelegate> = {
127127
updatePolicies: jest.fn().mockImplementation(),
128128

129129
updateGroupingPolicies: jest.fn().mockImplementation(),
130+
131+
addOrUpdateGroupingPolicies: jest.fn().mockImplementation(),
130132
};
131133

132134
const roleMetadataStorageMock: RoleMetadataStorage = {

0 commit comments

Comments
 (0)