From 49c7975f74a1791e205fe3a322f1efe6504212ed Mon Sep 17 00:00:00 2001 From: Patrick Knight Date: Mon, 8 Jan 2024 08:30:05 -0500 Subject: [PATCH] fix(rbac): fix role validation (#1020) --- .../src/service/permission-policy.ts | 2 +- .../src/service/policies-rest-api.test.ts | 130 +++++++++++++++++- .../src/service/policies-rest-api.ts | 10 +- .../src/service/policies-validation.ts | 18 ++- 4 files changed, 149 insertions(+), 11 deletions(-) diff --git a/plugins/rbac-backend/src/service/permission-policy.ts b/plugins/rbac-backend/src/service/permission-policy.ts index 3de5a67c86..32d3588a4d 100644 --- a/plugins/rbac-backend/src/service/permission-policy.ts +++ b/plugins/rbac-backend/src/service/permission-policy.ts @@ -94,7 +94,7 @@ const addPredefinedPoliciesAndGroupPolicies = async ( `Failed to validate group policy from file ${preDefinedPoliciesFile}. Cause: ${err.message}`, ); } - err = validateEntityReference(groupPolicy[1]); + err = validateEntityReference(groupPolicy[1], true); if (err) { throw new Error( `Failed to validate group policy from file ${preDefinedPoliciesFile}. Cause: ${err.message}`, diff --git a/plugins/rbac-backend/src/service/policies-rest-api.test.ts b/plugins/rbac-backend/src/service/policies-rest-api.test.ts index 05e0bf993d..4af3d6def9 100644 --- a/plugins/rbac-backend/src/service/policies-rest-api.test.ts +++ b/plugins/rbac-backend/src/service/policies-rest-api.test.ts @@ -16,6 +16,7 @@ import { policyEntityDeletePermission, policyEntityReadPermission, policyEntityUpdatePermission, + Role, } from '@janus-idp/backstage-plugin-rbac-common'; import { RBACPermissionPolicy } from './permission-policy'; @@ -249,6 +250,17 @@ describe('REST policies api', () => { }); }); + it('should return a status of Unauthorized - no user', async () => { + mockIdentityClient.getIdentity.mockImplementationOnce(() => undefined); + const result = await request(app).post('/policies').send(); + + expect(result.statusCode).toBe(403); + expect(result.body.error).toEqual({ + name: 'NotAllowedError', + message: 'User not found', + }); + }); + it('should not be created permission policy - req body is an empty', async () => { const result = await request(app).post('/policies').send(); @@ -1006,7 +1018,7 @@ describe('REST policies api', () => { expect(result.statusCode).toEqual(204); }); - it('should nothing to update - permissions in a different order', async () => { + it('should nothing to update - same permissions with different policy in a different order', async () => { mockEnforcer.hasPolicy = jest .fn() .mockImplementation(async (..._param: string[]): Promise => { @@ -1044,6 +1056,44 @@ describe('REST policies api', () => { expect(result.statusCode).toEqual(204); }); + it('should nothing to update - same permissions with different permission type in a different order', async () => { + mockEnforcer.hasPolicy = jest + .fn() + .mockImplementation(async (..._param: string[]): Promise => { + return true; + }); + const result = await request(app) + .put('/policies/user/default/permission_admin') + .send({ + oldPolicy: [ + { + permission: 'policy-entity', + policy: 'read', + effect: 'allow', + }, + { + permission: 'catalog-entity', + policy: 'read', + effect: 'allow', + }, + ], + newPolicy: [ + { + permission: 'catalog-entity', + policy: 'read', + effect: 'allow', + }, + { + permission: 'policy-entity', + policy: 'read', + effect: 'allow', + }, + ], + }); + + expect(result.statusCode).toEqual(204); + }); + it('should fail to update policy - unable to remove oldPolicy', async () => { mockEnforcer.hasPolicy = jest .fn() @@ -1372,7 +1422,7 @@ describe('REST policies api', () => { expect(result.statusCode).toBe(400); expect(result.body.error).toEqual({ name: 'InputError', - message: `Unsupported kind test. List supported values ["user", "group", "role"]`, + message: `Unsupported kind test. Supported value should be "role"`, }); }); @@ -1495,6 +1545,21 @@ describe('REST policies api', () => { }); }); + it('should not create a role - name is invalid', async () => { + const result = await request(app) + .post('/roles') + .send({ + memberReferences: ['user:default/permission_admin'], + name: 'x:default/rbac_admin', + }); + + expect(result.statusCode).toBe(400); + expect(result.body.error).toEqual({ + name: 'InputError', + message: `Invalid role definition. Cause: Unsupported kind x. Supported value should be "role"`, + }); + }); + it('should be created role', async () => { const result = await request(app) .post('/roles') @@ -1639,7 +1704,7 @@ describe('REST policies api', () => { }, newRole: { memberReferences: ['user:default/test'], - name: 'role/default/rbac_admin', + name: 'role:default/rbac_admin', }, }); @@ -1966,6 +2031,46 @@ describe('REST policies api', () => { message: `Duplicate role members found; user:default/test, role:default/rbac_admin is a duplicate`, }); }); + + it('should fail to update role name when role name is invalid', async () => { + const result = await request(app) + .put('/roles/role/default/rbac_admin') + .send({ + oldRole: { + memberReferences: ['user:default/permission_admin'], + }, + newRole: { + memberReferences: ['user:default/test'], + name: 'role:default/', + }, + }); + + expect(result.statusCode).toBe(400); + expect(result.body.error).toEqual({ + name: 'InputError', + message: `Invalid new role object. Cause: Entity reference "role:default/" was not on the form [:][/]`, + }); + }); + + it('should fail to update - oldRole name is invalid', async () => { + const result = await request(app) + .put('/roles/x/default/rbac_admin') + .send({ + oldRole: { + memberReferences: ['user:default/permission_admin'], + }, + newRole: { + memberReferences: ['user:default/test'], + name: 'role:default/', + }, + }); + + expect(result.statusCode).toBe(400); + expect(result.body.error).toEqual({ + name: 'InputError', + message: `Unsupported kind x. Supported value should be "role"`, + }); + }); }); describe('DELETE /roles/:kind/:namespace/:name', () => { @@ -2109,6 +2214,25 @@ describe('REST policies api', () => { }); }); + describe('transformRoleArray', () => { + it('should combine two roles together that are similar', () => { + const roles = [ + ['group:default/test', 'role:default/test'], + ['user:default/test', 'role:default/test'], + ]; + + const expectedResult: Role[] = [ + { + memberReferences: ['group:default/test', 'user:default/test'], + name: 'role:default/test', + }, + ]; + + const transformedRoles = server.transformRoleArray(...roles); + expect(transformedRoles).toStrictEqual(expectedResult); + }); + }); + describe('Conditions REST api', () => { beforeEach(() => { conditionalStorage.getCondition = jest.fn().mockImplementation(); diff --git a/plugins/rbac-backend/src/service/policies-rest-api.ts b/plugins/rbac-backend/src/service/policies-rest-api.ts index 1405b1dec8..26b004d014 100644 --- a/plugins/rbac-backend/src/service/policies-rest-api.ts +++ b/plugins/rbac-backend/src/service/policies-rest-api.ts @@ -318,7 +318,7 @@ export class PolicesServer { if (decision.result === AuthorizeResult.DENY) { throw new NotAllowedError(); // 403 } - const roleEntityRef = this.getEntityReference(request); + const roleEntityRef = this.getEntityReference(request, true); const role = await this.enforcer.getFilteredGroupingPolicy( 1, @@ -385,7 +385,7 @@ export class PolicesServer { if (decision.result === AuthorizeResult.DENY) { throw new NotAllowedError(); // 403 } - const roleEntityRef = this.getEntityReference(request); + const roleEntityRef = this.getEntityReference(request, true); const oldRoleRaw = request.body.oldRole; @@ -488,7 +488,7 @@ export class PolicesServer { throw new NotAllowedError(); // 403 } - const roleEntityRef = this.getEntityReference(request); + const roleEntityRef = this.getEntityReference(request, true); if (request.query.memberReferences) { const memberReferences = this.getFirstQuery( @@ -641,13 +641,13 @@ export class PolicesServer { return router; } - getEntityReference(request: Request): string { + getEntityReference(request: Request, role?: boolean): string { const kind = request.params.kind; const namespace = request.params.namespace; const name = request.params.name; const entityRef = `${kind}:${namespace}/${name}`; - const err = validateEntityReference(entityRef); + const err = validateEntityReference(entityRef, role); if (err) { throw new InputError(err.message); } diff --git a/plugins/rbac-backend/src/service/policies-validation.ts b/plugins/rbac-backend/src/service/policies-validation.ts index db28a12a4b..5cd3864f9e 100644 --- a/plugins/rbac-backend/src/service/policies-validation.ts +++ b/plugins/rbac-backend/src/service/policies-validation.ts @@ -35,12 +35,17 @@ export function validateRole(role: Role): Error | undefined { return new Error(`'name' field must not be empty`); } + let err = validateEntityReference(role.name, true); + if (err) { + return err; + } + if (!role.memberReferences || role.memberReferences.length === 0) { return new Error(`'memberReferences' field must not be empty`); } for (const member of role.memberReferences) { - const err = validateEntityReference(member); + err = validateEntityReference(member); if (err) { return err; } @@ -56,7 +61,10 @@ function isValidEffectValue(effect: string): boolean { } // We supports only full form entity reference: [:][/] -export function validateEntityReference(entityRef?: string): Error | undefined { +export function validateEntityReference( + entityRef?: string, + role?: boolean, +): Error | undefined { if (!entityRef) { return new Error(`'entityReference' must not be empty`); } @@ -75,6 +83,12 @@ export function validateEntityReference(entityRef?: string): Error | undefined { ); } + if (role && entityRefCompound.kind !== 'role') { + return new Error( + `Unsupported kind ${entityRefCompound.kind}. Supported value should be "role"`, + ); + } + if ( entityRefCompound.kind !== 'user' && entityRefCompound.kind !== 'group' &&