From 029fcd9a2c61bffbc0992f5ebf95a03c0bcb4684 Mon Sep 17 00:00:00 2001 From: Oleksandr Andriienko Date: Mon, 9 Sep 2024 13:56:14 +0300 Subject: [PATCH 1/4] feat(rbac): allow to disable group inheritance Signed-off-by: Oleksandr Andriienko --- .../src/role-manager/ancestor-search-memo.ts | 18 ++++++++++++------ .../src/role-manager/role-manager.ts | 4 ++-- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/plugins/rbac-backend/src/role-manager/ancestor-search-memo.ts b/plugins/rbac-backend/src/role-manager/ancestor-search-memo.ts index 71c8aa2697..3223ca607e 100644 --- a/plugins/rbac-backend/src/role-manager/ancestor-search-memo.ts +++ b/plugins/rbac-backend/src/role-manager/ancestor-search-memo.ts @@ -141,11 +141,6 @@ export class AncestorSearchMemo { allGroups: Entity[], current_depth: number, ) { - const depth = current_depth + 1; - if (this.maxDepth && current_depth >= this.maxDepth) { - return; - } - const groupName = `group:${group.metadata.namespace?.toLocaleLowerCase( 'en-US', )}/${group.metadata.name.toLocaleLowerCase('en-US')}`; @@ -153,6 +148,14 @@ export class AncestorSearchMemo { memo.setNode(groupName); } + if (this.maxDepth !== undefined && current_depth >= this.maxDepth) { + console.log( + `==== EXIT! ${current_depth} ${group.metadata.name}. Memo edges ${JSON.stringify(memo.graph.edges())} and nodes: ${JSON.stringify(memo.graph.nodes())}`, + ); + return; + } + const depth = current_depth + 1; + const parent = group.spec?.parent as string; const parentGroup = allGroups.find(g => g.metadata.name === parent); @@ -175,7 +178,10 @@ export class AncestorSearchMemo { current_depth: number, ) { // We add one to the maxDepth here because the user is considered the starting node - if (this.maxDepth && current_depth >= this.maxDepth + 1) { + if (this.maxDepth !== undefined && current_depth >= this.maxDepth + 1) { + console.log( + `==== EXIT! ${current_depth} ${relation.source_entity_ref}. Memo is ${JSON.stringify(memo.graph.edges())}`, + ); return; } const depth = current_depth + 1; diff --git a/plugins/rbac-backend/src/role-manager/role-manager.ts b/plugins/rbac-backend/src/role-manager/role-manager.ts index 1f7a15ca15..a5bb239a65 100644 --- a/plugins/rbac-backend/src/role-manager/role-manager.ts +++ b/plugins/rbac-backend/src/role-manager/role-manager.ts @@ -23,9 +23,9 @@ export class BackstageRoleManager implements RoleManager { this.allRoles = new Map(); const rbacConfig = this.config.getOptionalConfig('permission.rbac'); this.maxDepth = rbacConfig?.getOptionalNumber('maxDepth'); - if (this.maxDepth !== undefined && this.maxDepth! <= 0) { + if (this.maxDepth !== undefined && this.maxDepth! < 0) { throw new Error( - 'Max Depth for RBAC group hierarchy must be greater than zero', + 'Max Depth for RBAC group hierarchy must be greater than or equal to zero', ); } } From b6cb64f30146b8cde9d592b80a2c63efad0a74a0 Mon Sep 17 00:00:00 2001 From: Oleksandr Andriienko Date: Mon, 9 Sep 2024 13:58:20 +0300 Subject: [PATCH 2/4] feat(rbac): clean up Signed-off-by: Oleksandr Andriienko --- .../rbac-backend/src/role-manager/ancestor-search-memo.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/plugins/rbac-backend/src/role-manager/ancestor-search-memo.ts b/plugins/rbac-backend/src/role-manager/ancestor-search-memo.ts index 3223ca607e..990ecde44d 100644 --- a/plugins/rbac-backend/src/role-manager/ancestor-search-memo.ts +++ b/plugins/rbac-backend/src/role-manager/ancestor-search-memo.ts @@ -149,9 +149,6 @@ export class AncestorSearchMemo { } if (this.maxDepth !== undefined && current_depth >= this.maxDepth) { - console.log( - `==== EXIT! ${current_depth} ${group.metadata.name}. Memo edges ${JSON.stringify(memo.graph.edges())} and nodes: ${JSON.stringify(memo.graph.nodes())}`, - ); return; } const depth = current_depth + 1; @@ -179,9 +176,6 @@ export class AncestorSearchMemo { ) { // We add one to the maxDepth here because the user is considered the starting node if (this.maxDepth !== undefined && current_depth >= this.maxDepth + 1) { - console.log( - `==== EXIT! ${current_depth} ${relation.source_entity_ref}. Memo is ${JSON.stringify(memo.graph.edges())}`, - ); return; } const depth = current_depth + 1; From ab1a1167663fc52b641581d9542e9fbe7469baae Mon Sep 17 00:00:00 2001 From: Oleksandr Andriienko Date: Mon, 9 Sep 2024 15:02:09 +0300 Subject: [PATCH 3/4] feat(rbac): fix unit tests Signed-off-by: Oleksandr Andriienko --- .../src/role-manager/role-manager.test.ts | 50 ++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/plugins/rbac-backend/src/role-manager/role-manager.test.ts b/plugins/rbac-backend/src/role-manager/role-manager.test.ts index dd4b2840f8..f7c8916f81 100644 --- a/plugins/rbac-backend/src/role-manager/role-manager.test.ts +++ b/plugins/rbac-backend/src/role-manager/role-manager.test.ts @@ -66,7 +66,8 @@ describe('BackstageRoleManager', () => { expect(errorRoleManager).toBeUndefined(); expect(expectedError).toMatchObject({ - message: 'Max Depth for RBAC group hierarchy must be greater than zero', + message: + 'Max Depth for RBAC group hierarchy must be greater than or equal to zero', }); }); }); @@ -328,6 +329,53 @@ describe('BackstageRoleManager', () => { expect(result).toBeTruthy(); }); + // user:default/mike should inherits from group:default/team-a + // + // Hierarchy: + // + // group:default/team-a + // | + // group:default/team-b + // | + // user:default/mike + // + it('should disable group inheritance when max-depth=0', async () => { + // max-depth=0 + const config = newConfigReader(0); + const roleManager = new BackstageRoleManager( + catalogApiMock as CatalogApi, + loggerMock as LoggerService, + catalogDBClient, + rbacDBClient, + config, + mockAuthService, + ); + const groupMock = createGroupEntity('team-b', 'team-a', [], ['mike']); + const groupParentMock = createGroupEntity('team-a', undefined, [ + 'team-b', + ]); + + catalogApiMock.getEntities.mockImplementation((arg: any) => { + const hasMember = arg.filter['relations.hasMember']; + if (hasMember && hasMember === 'user:default/mike') { + return { items: [groupMock] }; + } + return { items: [groupMock, groupParentMock] }; + }); + + let result = await roleManager.hasLink( + 'user:default/mike', + 'group:default/team-b', + ); + expect(result).toBeTruthy(); + + result = await roleManager.hasLink( + 'user:default/mike', + 'group:default/team-a', + ); + expect(result).toBeFalsy(); + }); + // user:default/mike should inherits role:default/team-a from group:default/team-a // // Hierarchy: From 96077eb9ce83aff4dada280a8c62bee0ff57db85 Mon Sep 17 00:00:00 2001 From: Oleksandr Andriienko Date: Mon, 9 Sep 2024 15:40:09 +0300 Subject: [PATCH 4/4] feat(rbac): fix eslint Signed-off-by: Oleksandr Andriienko --- .../rbac-backend/src/role-manager/role-manager.test.ts | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/plugins/rbac-backend/src/role-manager/role-manager.test.ts b/plugins/rbac-backend/src/role-manager/role-manager.test.ts index f7c8916f81..05fba0c48c 100644 --- a/plugins/rbac-backend/src/role-manager/role-manager.test.ts +++ b/plugins/rbac-backend/src/role-manager/role-manager.test.ts @@ -342,7 +342,7 @@ describe('BackstageRoleManager', () => { it('should disable group inheritance when max-depth=0', async () => { // max-depth=0 const config = newConfigReader(0); - const roleManager = new BackstageRoleManager( + const rm = new BackstageRoleManager( catalogApiMock as CatalogApi, loggerMock as LoggerService, catalogDBClient, @@ -363,16 +363,13 @@ describe('BackstageRoleManager', () => { return { items: [groupMock, groupParentMock] }; }); - let result = await roleManager.hasLink( + let result = await rm.hasLink( 'user:default/mike', 'group:default/team-b', ); expect(result).toBeTruthy(); - result = await roleManager.hasLink( - 'user:default/mike', - 'group:default/team-a', - ); + result = await rm.hasLink('user:default/mike', 'group:default/team-a'); expect(result).toBeFalsy(); });