From 3601eb8d0c19e0aad27031ab61f1afa0edc78945 Mon Sep 17 00:00:00 2001 From: Oleksandr Andriienko Date: Tue, 23 Jan 2024 14:25:57 +0200 Subject: [PATCH] fix(rbac): fix work resource permission specified by name (#940) * fix(rbac): fix work resource permission specified by name Signed-off-by: Oleksandr Andriienko * fix(rbac): add more tests Signed-off-by: Oleksandr Andriienko * fix(rbac): handle code review feedback Signed-off-by: Oleksandr Andriienko * fix(rbac): fix rbac-backend doc Signed-off-by: Oleksandr Andriienko --------- Signed-off-by: Oleksandr Andriienko --- plugins/rbac-backend/docs/permissions.md | 12 +- .../src/service/permission-policy.test.ts | 198 +++++++++++++++++- .../src/service/permission-policy.ts | 73 ++++--- .../src/service/role-manager.test.ts | 75 ++++++- .../rbac-backend/src/service/role-manager.ts | 60 +++++- 5 files changed, 369 insertions(+), 49 deletions(-) diff --git a/plugins/rbac-backend/docs/permissions.md b/plugins/rbac-backend/docs/permissions.md index 3ee9456361..95ca856a5f 100644 --- a/plugins/rbac-backend/docs/permissions.md +++ b/plugins/rbac-backend/docs/permissions.md @@ -4,10 +4,16 @@ Note: The requirements section primarily pertains to the frontend and may not be When defining a permission for the RBAC Backend plugin to consume, follow these guidelines: -- If the permission has a Resource Type, it must be used. Otherwise, use the name of the permission. +Permission policies defined using the name of the permission will have higher priority over permission policies that are defined using the resource type. +Example: - - Example: `p, role:default/test, catalog.entity.read, read, allow` will not be properly picked up. - - Instead, use: `p, role:default/test, catalog-entity, read, allow` +``` +p, role:default/myrole, catalog-entity, read, allow +p, role:default/myrole, catalog.entity.read, read, deny +g, user:default/myuser, role:default/myrole +``` + +Where 'myuser' will have a deny for reading catalog entities, because the permission name takes priority over the permission resource type. - If the permission does not have a policy associated with it, use the keyword `use` in its place. - Example: `p, role:default/test, kubernetes.proxy, use, allow` diff --git a/plugins/rbac-backend/src/service/permission-policy.test.ts b/plugins/rbac-backend/src/service/permission-policy.test.ts index a8bbf6a0c9..44b746dbde 100644 --- a/plugins/rbac-backend/src/service/permission-policy.test.ts +++ b/plugins/rbac-backend/src/service/permission-policy.test.ts @@ -197,16 +197,24 @@ describe('RBACPermissionPolicy Tests', () => { beforeEach(async () => { const adapter = new StringAdapter( ` - # basic type permission policies + # ========== basic type permission policies ========== # + # case 1 p, user:default/known_user, test.resource.deny, use, deny + # case 2 is about user without listed permissions + # case 3 p, user:default/duplicated, test.resource, use, allow p, user:default/duplicated, test.resource, use, deny + # case 4 p, user:default/known_user, test.resource, use, allow - # resource type permission policies - p, user:default/known_user, test-resource-deny, update, deny + # ========== resource type permission policies ========== # + # case 1 + p, user:default/known_user, test-resource-deny, update, deny + # case 2 is about user without listed permissions + # case 3 p, user:default/duplicated, test-resource, update, allow p, user:default/duplicated, test-resource, update, deny + # case 4 p, user:default/known_user, test-resource, update, allow `, ); @@ -229,13 +237,13 @@ describe('RBACPermissionPolicy Tests', () => { catalogApi.getEntities.mockReturnValue({ items: [] }); }); - // +-------+------+------------------------+ - // | allow | deny | result | - // +-------+------+------------------------+ - // | N | Y | deny | 1 - // | N | N | deny (user not listed) | 2 - // | Y | Y | deny (user:default/duplicated) | 3 - // | Y | N | allow | 4 + // +-------+------+------------------------------------+ + // | allow | deny | result | | + // +-------+------+--------------------------------+---| + // | N | Y | deny | 1 | + // | N | N | deny (user not listed) | 2 | + // | Y | Y | deny (user:default/duplicated) | 3 | + // | Y | N | allow | 4 | // Tests for Resource basic type permission @@ -351,6 +359,176 @@ describe('RBACPermissionPolicy Tests', () => { }); }); +// Notice: There is corner case, when "resourced" permission policy can be defined not by resource type, but by name. +describe('Policy checks for resourced permissions defined by name', () => { + async function createRBACPolicy( + policyContent: string, + ): Promise { + const adapter = new StringAdapter(policyContent); + const config = new ConfigReader({}); + const theModel = newModelFromString(MODEL); + const logger = getVoidLogger(); + const enf = await createEnforcer( + theModel, + adapter, + logger, + tokenManagerMock, + ); + + return await RBACPermissionPolicy.build( + logger, + config, + conditionalStorage, + enf, + ); + } + it('should allow access to resourced permission assigned by name', async () => { + catalogApi.getEntities.mockReturnValue({ items: [] }); + + const policy = await createRBACPolicy(` + p, role:default/catalog_reader, catalog.entity.read, read, allow + + g, user:default/tor, role:default/catalog_reader + `); + + const decision = await policy.handle( + newPolicyQueryWithResourcePermission( + 'catalog.entity.read', + 'catalog-entity', + 'read', + ), + newIdentityResponse('user:default/tor'), + ); + expect(decision.result).toBe(AuthorizeResult.ALLOW); + }); + + it('should allow access to resourced permission assigned by name, because it has higher priority then permission for the same resource assigned by resource type', async () => { + catalogApi.getEntities.mockReturnValue({ items: [] }); + + const policy = await createRBACPolicy(` + p, role:default/catalog_reader, catalog.entity.read, read, allow + p, role:default/catalog_reader, catalog-entity, read, deny + + g, user:default/tor, role:default/catalog_reader + `); + + const decision = await policy.handle( + newPolicyQueryWithResourcePermission( + 'catalog.entity.read', + 'catalog-entity', + 'read', + ), + newIdentityResponse('user:default/tor'), + ); + expect(decision.result).toBe(AuthorizeResult.ALLOW); + }); + + it('should deny access to resourced permission assigned by name, because it has higher priority then permission for the same resource assigned by resource type', async () => { + catalogApi.getEntities.mockReturnValue({ items: [] }); + + const policy = await createRBACPolicy(` + p, role:default/catalog_reader, catalog.entity.read, read, deny + p, role:default/catalog_reader, catalog-entity, read, allow + + g, user:default/tor, role:default/catalog_reader + `); + + const decision = await policy.handle( + newPolicyQueryWithResourcePermission( + 'catalog.entity.read', + 'catalog-entity', + 'read', + ), + newIdentityResponse('user:default/tor'), + ); + expect(decision.result).toBe(AuthorizeResult.DENY); + }); + + it('should allow access to resourced permission assigned by name, but user inherits policy from his group', async () => { + const groupEntityMock: Entity = { + apiVersion: 'v1', + kind: 'Group', + metadata: { + name: 'team-a', + namespace: 'default', + }, + }; + catalogApi.getEntities.mockImplementation(arg => { + const hasMember = arg.filter['relations.hasMember']; + if (hasMember && hasMember[0] === 'user:default/tor') { + return { items: [groupEntityMock] }; + } + return { items: [] }; + }); + + const policy = await createRBACPolicy(` + p, role:default/catalog_user, catalog.entity.read, read, allow + + g, group:default/team-a, role:default/catalog_user + `); + + const decision = await policy.handle( + newPolicyQueryWithResourcePermission( + 'catalog.entity.read', + 'catalog-entity', + 'read', + ), + newIdentityResponse('user:default/tor'), + ); + expect(decision.result).toBe(AuthorizeResult.ALLOW); + }); + + it('should allow access to resourced permission assigned by name, but user inherits policy from few groups', async () => { + const groupEntityMock: Entity = { + apiVersion: 'v1', + kind: 'Group', + metadata: { + name: 'team-a', + namespace: 'default', + }, + spec: { + parent: 'team-b', + }, + }; + const groupParentMock: Entity = { + apiVersion: 'v1', + kind: 'Group', + metadata: { + name: 'team-b', + namespace: 'default', + }, + }; + catalogApi.getEntities.mockImplementation(arg => { + const hasMember = arg.filter['relations.hasMember']; + if (hasMember && hasMember[0] === 'user:default/tor') { + return { items: [groupEntityMock] }; + } + const hasParent = arg.filter['relations.parentOf']; + if (hasParent && hasParent[0] === 'group:default/team-a') { + return { items: [groupParentMock] }; + } + return { items: [] }; + }); + + const policy = await createRBACPolicy(` + p, role:default/catalog_user, catalog.entity.read, read, allow + + g, group:default/team-a, group:default/team-b + g, group:default/team-b, role:default/catalog_user + `); + + const decision = await policy.handle( + newPolicyQueryWithResourcePermission( + 'catalog.entity.read', + 'catalog-entity', + 'read', + ), + newIdentityResponse('user:default/tor'), + ); + expect(decision.result).toBe(AuthorizeResult.ALLOW); + }); +}); + describe('Policy checks for users and groups', () => { let policy: RBACPermissionPolicy; diff --git a/plugins/rbac-backend/src/service/permission-policy.ts b/plugins/rbac-backend/src/service/permission-policy.ts index 32d3588a4d..765ac3ef27 100644 --- a/plugins/rbac-backend/src/service/permission-policy.ts +++ b/plugins/rbac-backend/src/service/permission-policy.ts @@ -1,9 +1,6 @@ import { Config } from '@backstage/config'; import { ConfigApi } from '@backstage/core-plugin-api'; -import { - BackstageIdentityResponse, - BackstageUserIdentity, -} from '@backstage/plugin-auth-node'; +import { BackstageIdentityResponse } from '@backstage/plugin-auth-node'; import { AuthorizeResult, isResourcePermission, @@ -160,32 +157,53 @@ export class RBACPermissionPolicy implements PermissionPolicy { // a more complicated model with multiple policy and request shapes. const action = request.permission.attributes.action ?? 'use'; + if (!identityResp?.identity) { + return { result: AuthorizeResult.DENY }; + } + + const userEntityRef = identityResp.identity.userEntityRef; + const permissionName = request.permission.name; + if (isResourcePermission(request.permission)) { - status = await this.isAuthorized( - identityResp?.identity, - request.permission.resourceType, - action, - ); + const resourceType = request.permission.resourceType; + const hasNamedPermission = + await this.hasImplicitPermissionSpecifiedByName( + userEntityRef, + permissionName, + action, + ); + // Let's set up higher priority for permission specified by name, than by resource type + const obj = hasNamedPermission ? permissionName : resourceType; + + status = await this.enforcer.enforce(userEntityRef, obj, action); if (status && identityResp) { - const conditionalDecision = await this.conditionStorage.findCondition( - request.permission.resourceType, - ); + const conditionalDecision = + await this.conditionStorage.findCondition(resourceType); if (conditionalDecision) { + this.logger.info( + `${identityResp?.identity.userEntityRef} executed condition for permission ${request.permission.name}, resource type ${resourceType} and action ${action}`, + ); return conditionalDecision; } } } else { - status = await this.isAuthorized( - identityResp?.identity, - request.permission.name, + status = await this.enforcer.enforce( + userEntityRef, + permissionName, action, ); } const result = status ? AuthorizeResult.ALLOW : AuthorizeResult.DENY; this.logger.info( - `${identityResp?.identity.userEntityRef} is ${result} for permission ${request.permission.name} and action ${action}`, + `${userEntityRef} is ${result} for permission '${ + request.permission.name + }'${ + isResourcePermission(request.permission) + ? `, resource type '${request.permission.resourceType}'` + : '' + } and action ${action}`, ); return Promise.resolve({ result: result, @@ -198,17 +216,18 @@ export class RBACPermissionPolicy implements PermissionPolicy { } } - private isAuthorized = async ( - identity: BackstageUserIdentity | undefined, - resourceType: string, + private async hasImplicitPermissionSpecifiedByName( + userEntityRef: string, + permissionName: string, action: string, - ): Promise => { - if (!identity) { - return false; + ): Promise { + const userPerms = + await this.enforcer.getImplicitPermissionsForUser(userEntityRef); + for (const perm of userPerms) { + if (permissionName === perm[1] && action === perm[2]) { + return true; + } } - - const entityRef = identity.userEntityRef; - - return await this.enforcer.enforce(entityRef, resourceType, action); - }; + return false; + } } diff --git a/plugins/rbac-backend/src/service/role-manager.test.ts b/plugins/rbac-backend/src/service/role-manager.test.ts index 8a16d77b94..1221c42cf2 100644 --- a/plugins/rbac-backend/src/service/role-manager.test.ts +++ b/plugins/rbac-backend/src/service/role-manager.test.ts @@ -41,12 +41,6 @@ describe('BackstageRoleManager', () => { ).toThrow('Method "syncedHasLink" not implemented.'); }); - it('should throw an error for getRoles', async () => { - await expect(roleManager.getRoles('name')).rejects.toThrow( - 'Method "getRoles" not implemented.', - ); - }); - it('should throw an error for getUsers', async () => { await expect(roleManager.getUsers('name')).rejects.toThrow( 'Method "getUsers" not implemented.', @@ -1121,6 +1115,75 @@ describe('BackstageRoleManager', () => { expect(result).toBeTruthy(); }); }); + + describe('getRoles returns roles per user', () => { + it('should returns role per user', async () => { + roleManager.addLink('user:default/test', 'role:default/rbac_admin'); + roleManager.addLink('user:default/test-two', 'role:default/rbac_admin'); + roleManager.addLink( + 'user:default/test-three', + 'role:default/rbac_admin_test', + ); + + catalogApiMock.getEntities.mockImplementation((arg: any) => { + const hasMember = arg.filter['relations.hasMember']; + + if (hasMember && hasMember[0] === 'user:default/test') { + return { items: [] }; + } + if (hasMember && hasMember[0] === 'user:default/test-two') { + return { items: [] }; + } + if (hasMember && hasMember[0] === 'user:default/test-three') { + return { items: [] }; + } + return { items: [] }; + }); + + let roles = await roleManager.getRoles('user:default/test'); + expect(roles.length).toBe(1); + expect(roles[0]).toEqual('role:default/rbac_admin'); + + roles = await roleManager.getRoles('user:default/test-two'); + expect(roles.length).toBe(1); + expect(roles[0]).toEqual('role:default/rbac_admin'); + + roles = await roleManager.getRoles('user:default/test-three'); + expect(roles.length).toBe(1); + expect(roles[0]).toEqual('role:default/rbac_admin_test'); + }); + + it('getRoles returns role for user inherited from group', async () => { + const teamAGroup = createGroupEntity('team-a', undefined, [ + 'user:default/test', + ]); + + roleManager.addLink('group:default/team-a', 'role:default/rbac_admin'); + + catalogApiMock.getEntities.mockImplementation((arg: any) => { + const hasMember = arg.filter['relations.hasMember']; + + if (hasMember && hasMember[0] === 'user:default/test') { + return { items: [teamAGroup] }; + } + + return { items: [] }; + }); + + let roles = await roleManager.getRoles('user:default/test'); + expect(roles.length).toBe(1); + expect(roles[0]).toEqual('role:default/rbac_admin'); + + // should return empty array for group + roles = await roleManager.getRoles('group:default/team-a'); + expect(roles.length).toBe(0); + + // should return empty array for role + roles = await roleManager.getRoles('role:default/rbac_admin'); + expect(roles.length).toBe(0); + }); + }); + function createGroupEntity( name: string, parent?: string, diff --git a/plugins/rbac-backend/src/service/role-manager.ts b/plugins/rbac-backend/src/service/role-manager.ts index 84620415f3..652ae2704a 100644 --- a/plugins/rbac-backend/src/service/role-manager.ts +++ b/plugins/rbac-backend/src/service/role-manager.ts @@ -44,6 +44,10 @@ class Role { return false; } + + getRoles(): Role[] { + return this.roles; + } } // AncestorSearchMemo - should be used to build group hierarchy graph for User entity reference. @@ -108,6 +112,10 @@ class AncestorSearchMemo { `SubGraph nodes: ${JSON.stringify(this.graph.nodes())} for ${userEntity}`, ); } + + getNodes(): string[] { + return this.graph.nodes(); + } } export class BackstageRoleManager implements RoleManager { @@ -237,10 +245,56 @@ export class BackstageRoleManager implements RoleManager { /** * getRoles gets the roles that a subject inherits. - * domain is a prefix to the roles. + * + * name - is a string entity reference, for example: user:default/tom, role:default/dev, + * so format is :/. + * GetRoles method supports only two kind values: 'user' and 'role'. + * + * domain - is a prefix to the roles, unused parameter. + * + * If name's kind === 'user' we return all inherited roles from groups and roles directly assigned to the user. + * if name's kind === 'role' we return empty array, because we don't support role inheritance. + * Case kind === 'group' - should not happen, because: + * 1) Method getRoles returns only role entity references, so casbin engine doesn't call this + * method again to ask about name with kind "group". + * 2) We implemented getRoles method only to use: + * 'await enforcer.getImplicitPermissionsForUser(userEntityRef)', + * so name argument can be only with kind 'user' or 'role'. + * + * Info: when we call 'await enforcer.getImplicitPermissionsForUser(userEntityRef)', + * then casbin engine executes 'getRoles' method few times. + * Firstly casbin asks about roles for 'userEntityRef'. + * Let's imagine, that 'getRoles' returned two roles for userEntityRef. + * Then casbin calls 'getRoles' two more times to + * find parent roles. But we return empty array for each such call, + * because we don't support role inheritance and we notify casbin about end of the role sub-tree. */ - async getRoles(_name: string, ..._domain: string[]): Promise { - throw new Error('Method "getRoles" not implemented.'); + async getRoles(name: string, ..._domain: string[]): Promise { + const { kind } = parseEntityRef(name); + if (kind === 'user') { + const memo = new AncestorSearchMemo(name); + await this.findAncestorGroups(memo); + memo.debugNodesAndEdges(this.log, name); + const userAndParentGroups = memo.getNodes(); + userAndParentGroups.push(name); + + const allRoles: string[] = []; + for (const userOrParentGroup of userAndParentGroups) { + const r = this.allRoles.get(userOrParentGroup); + if (r) { + const rolesEntityRefs = [...r.getRoles()] + .filter(role => { + return role.name.startsWith('role:default'); + }) + .map(role => role.name); + + allRoles.push(...rolesEntityRefs); + } + } + return Promise.resolve(allRoles); + } + + return []; } /**