Skip to content

Commit

Permalink
fix(rbac): fix role validation (#1020)
Browse files Browse the repository at this point in the history
  • Loading branch information
@PatAKnight
PatAKnight authored Jan 8, 2024
1 parent a3d6d86 commit 49c7975
Show file tree
Hide file tree
Showing 4 changed files with 149 additions and 11 deletions.
2 changes: 1 addition & 1 deletion plugins/rbac-backend/src/service/permission-policy.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}`,
Expand Down
130 changes: 127 additions & 3 deletions plugins/rbac-backend/src/service/policies-rest-api.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
policyEntityDeletePermission,
policyEntityReadPermission,
policyEntityUpdatePermission,
Role,
} from '@janus-idp/backstage-plugin-rbac-common';

import { RBACPermissionPolicy } from './permission-policy';
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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<boolean> => {
Expand Down Expand Up @@ -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<boolean> => {
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()
Expand Down Expand Up @@ -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"`,
});
});

Expand Down Expand Up @@ -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')
Expand Down Expand Up @@ -1639,7 +1704,7 @@ describe('REST policies api', () => {
},
newRole: {
memberReferences: ['user:default/test'],
name: 'role/default/rbac_admin',
name: 'role:default/rbac_admin',
},
});

Expand Down Expand Up @@ -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 [<kind>:][<namespace>/]<name>`,
});
});

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', () => {
Expand Down Expand Up @@ -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();
Expand Down
10 changes: 5 additions & 5 deletions plugins/rbac-backend/src/service/policies-rest-api.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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);
}
Expand Down
18 changes: 16 additions & 2 deletions plugins/rbac-backend/src/service/policies-validation.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -56,7 +61,10 @@ function isValidEffectValue(effect: string): boolean {
}

// We supports only full form entity reference: [<kind>:][<namespace>/]<name>
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`);
}
Expand All @@ -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' &&
Expand Down

0 comments on commit 49c7975

Please sign in to comment.