From 966b2b200e0ade0ce600901a7853a4a94751df22 Mon Sep 17 00:00:00 2001 From: Oleksandr Andriienko Date: Tue, 4 Jun 2024 16:08:06 +0300 Subject: [PATCH] fix(rbac): fix handling condition action conflicts (#1781) * fix(rbac): fix handling condition action conflicts Signed-off-by: Oleksandr Andriienko * fix(rbac): refactor condition conflict check, add more unit test Signed-off-by: Oleksandr Andriienko --------- Signed-off-by: Oleksandr Andriienko --- .../src/database/conditional-storage.ts | 104 ++++----- .../src/database/conditional.storage.test.ts | 208 ++++++++++++++++-- .../src/service/permission-policy.test.ts | 3 +- .../src/service/policies-rest-api.test.ts | 2 +- 4 files changed, 242 insertions(+), 75 deletions(-) diff --git a/plugins/rbac-backend/src/database/conditional-storage.ts b/plugins/rbac-backend/src/database/conditional-storage.ts index e6aa30c9cc..6b860a6ec6 100644 --- a/plugins/rbac-backend/src/database/conditional-storage.ts +++ b/plugins/rbac-backend/src/database/conditional-storage.ts @@ -32,11 +32,13 @@ export interface ConditionalStorage { createCondition( conditionalDecision: RoleConditionalPolicyDecision, ): Promise; - findUniqueCondition( + checkConflictedConditions( roleEntityRef: string, resourceType: string, + pluginId: string, queryPermissionNames: string[], - ): Promise | undefined>; + idToExclude?: number, + ): Promise; getCondition( id: number, ): Promise | undefined>; @@ -57,7 +59,7 @@ export class DataBaseConditionalStorage implements ConditionalStorage { actions?: PermissionAction[], permissionNames?: string[], ): Promise[]> { - const daoRaws = await this.knex?.table(CONDITIONAL_TABLE).where(builder => { + const daoRaws = await this.knex.table(CONDITIONAL_TABLE).where(builder => { if (pluginId) { builder.where('pluginId', pluginId); } @@ -100,22 +102,16 @@ export class DataBaseConditionalStorage implements ConditionalStorage { async createCondition( conditionalDecision: RoleConditionalPolicyDecision, ): Promise { - const condition = await this.findUniqueCondition( + await this.checkConflictedConditions( conditionalDecision.roleEntityRef, conditionalDecision.resourceType, - conditionalDecision.permissionMapping.map(permInfo => permInfo.name), + conditionalDecision.pluginId, + conditionalDecision.permissionMapping.map(permInfo => permInfo.action), ); - if (condition) { - throw new ConflictError( - `A condition with resource type '${condition.resourceType}'` + - ` and permission '${JSON.stringify(condition.permissionMapping)}'` + - ` has already been stored for role '${conditionalDecision.roleEntityRef}'`, - ); - } const conditionRaw = this.toDAO(conditionalDecision); const result = await this.knex - ?.table(CONDITIONAL_TABLE) + .table(CONDITIONAL_TABLE) .insert(conditionRaw) .returning('id'); if (result && result?.length > 0) { @@ -125,42 +121,53 @@ export class DataBaseConditionalStorage implements ConditionalStorage { throw new Error(`Failed to create the condition.`); } - async findUniqueCondition( + async checkConflictedConditions( roleEntityRef: string, resourceType: string, - queryPermissionNames: string[], - ): Promise | undefined> { - const daoRaws = await this.knex - ?.table(CONDITIONAL_TABLE) - .where('roleEntityRef', roleEntityRef) - .where('resourceType', resourceType); + pluginId: string, + queryConditionActions: PermissionAction[], + idToExclude?: number, + ): Promise { + let conditionsForTheSameResource = await this.filterConditions( + roleEntityRef, + pluginId, + resourceType, + ); + conditionsForTheSameResource = conditionsForTheSameResource.filter( + c => c.id !== idToExclude, + ); - if (daoRaws) { - const conditions = daoRaws.map(daoRaw => - this.daoToConditionalDecision(daoRaw), + if (conditionsForTheSameResource) { + const conflictedCondition = conditionsForTheSameResource.find( + condition => { + const conditionActions = condition.permissionMapping.map( + permInfo => permInfo.action, + ); + return queryConditionActions.some(action => + conditionActions.includes(action), + ); + }, ); - return conditions.find(condition => { - const conditionPermissionNames = condition.permissionMapping.map( - permInfo => permInfo.name, + if (conflictedCondition) { + const conflictedActions = queryConditionActions.filter(action => + conflictedCondition.permissionMapping.some(p => p.action === action), ); - const isContainTheSamePermissions = queryPermissionNames.every(name => - conditionPermissionNames.includes(name), + throw new ConflictError( + `Found condition with conflicted permission action '${JSON.stringify( + conflictedActions, + )}'. Role could have multiple ` + + `conditions for the same resource type '${conflictedCondition.resourceType}', but with different permission action sets.`, ); - return ( - isContainTheSamePermissions && - conditionPermissionNames.length === queryPermissionNames.length - ); - }); + } } - return undefined; } async getCondition( id: number, ): Promise | undefined> { const daoRaw = await this.knex - ?.table(CONDITIONAL_TABLE) + .table(CONDITIONAL_TABLE) .where('id', id) .first(); @@ -187,35 +194,18 @@ export class DataBaseConditionalStorage implements ConditionalStorage { throw new NotFoundError(`Condition with id ${id} was not found`); } - const conditionsForTheSameResource = await this.filterConditions( + await this.checkConflictedConditions( conditionalDecision.roleEntityRef, - conditionalDecision.pluginId, conditionalDecision.resourceType, + conditionalDecision.pluginId, + conditionalDecision.permissionMapping.map(perm => perm.action), + id, ); - for (const permission of conditionalDecision.permissionMapping) { - for (const conditionToCompare of conditionsForTheSameResource) { - if (conditionToCompare.id === id) { - continue; - } - const conditionPermNames = conditionToCompare.permissionMapping.map( - perm => perm.name, - ); - if (conditionPermNames.includes(permission.name)) { - throw new ConflictError( - `Found condition with conflicted permission '${JSON.stringify( - permission, - )}'. Role could have multiple ` + - `conditions for the same resource type '${conditionalDecision.resourceType}', but with different permission name and action sets.`, - ); - } - } - } - const conditionRaw = this.toDAO(conditionalDecision); conditionRaw.id = id; const result = await this.knex - ?.table(CONDITIONAL_TABLE) + .table(CONDITIONAL_TABLE) .where('id', conditionRaw.id) .update(conditionRaw) .returning('id'); diff --git a/plugins/rbac-backend/src/database/conditional.storage.test.ts b/plugins/rbac-backend/src/database/conditional.storage.test.ts index 3a8779f3e1..c287d79750 100644 --- a/plugins/rbac-backend/src/database/conditional.storage.test.ts +++ b/plugins/rbac-backend/src/database/conditional.storage.test.ts @@ -249,7 +249,7 @@ describe('DataBaseConditionalStorage', () => { describe('createCondition', () => { it.each(databases.eachSupportedId())( - 'should successfully create new policy metadata', + 'should successfully create new conditional policy', async databasesId => { const { knex, db } = await createDatabase(databasesId); @@ -278,9 +278,7 @@ describe('DataBaseConditionalStorage', () => { await expect(async () => { await db.createCondition(condition1); }).rejects.toThrow( - `A condition with resource type 'catalog-entity'` + - ` and permission '[{"action":"read","name":"catalog.entity.read"}]'` + - ` has already been stored for role 'role:default/test'`, + `Found condition with conflicted permission action '["read"]'. Role could have multiple conditions for the same resource type 'catalog-entity', but with different permission action sets.`, ); }, ); @@ -299,37 +297,158 @@ describe('DataBaseConditionalStorage', () => { }); }); - describe('findUniqueCondition', () => { + describe('checkConflictedConditions', () => { it.each(databases.eachSupportedId())( - 'should find condition', + 'should check conflicted condition', async databasesId => { const { knex, db } = await createDatabase(databasesId); await knex(CONDITIONAL_TABLE).insert( conditionDao1, ); - const condition = await db.findUniqueCondition( + await expect(async () => { + await db.checkConflictedConditions( + 'role:default/test', + 'catalog-entity', + 'catalog', + ['read'], + ); + }).rejects.toThrow( + `Found condition with conflicted permission action '["read"]'. Role could have multiple conditions for the same resource type 'catalog-entity', but with different permission action sets.`, + ); + }, + ); + + it.each(databases.eachSupportedId())( + 'should fail check, when there is condition with one conflicted action "read"', + async databasesId => { + const { knex, db } = await createDatabase(databasesId); + const conditionDaoWithFewActions = { + ...conditionDao1, + permissions: + '[{"action":"read","name":"catalog.entity.read"}, {"action":"delete","name":"catalog.entity.delete"}]', + }; + await knex(CONDITIONAL_TABLE).insert( + conditionDaoWithFewActions, + ); + + await expect(async () => { + await db.checkConflictedConditions( + 'role:default/test', + 'catalog-entity', + 'catalog', + ['read'], + ); + }).rejects.toThrow( + `Found condition with conflicted permission action '["read"]'. Role could have multiple conditions for the same resource type 'catalog-entity', but with different permission action sets.`, + ); + }, + ); + + it.each(databases.eachSupportedId())( + 'should fail check, when there is one condition with two conflicted actions "read" and "update"', + async databasesId => { + const { knex, db } = await createDatabase(databasesId); + const conditionDaoWithFewActions = { + ...conditionDao1, + permissions: + '[{"action":"read","name":"catalog.entity.read"}, {"action":"delete","name":"catalog.entity.delete"}, {"action":"update","name":"catalog.entity.update"}]', + }; + await knex(CONDITIONAL_TABLE).insert( + conditionDaoWithFewActions, + ); + + await expect(async () => { + await db.checkConflictedConditions( + 'role:default/test', + 'catalog-entity', + 'catalog', + ['read', 'update'], + ); + }).rejects.toThrow( + `Found condition with conflicted permission action '["read","update"]'. Role could have multiple conditions for the same resource type 'catalog-entity', but with different permission action sets.`, + ); + }, + ); + + it.each(databases.eachSupportedId())( + 'should fail check, when there is condition with three conflicted actions "read", "update", "delete"', + async databasesId => { + const { knex, db } = await createDatabase(databasesId); + const conditionDaoWithFewActions = { + ...conditionDao1, + permissions: + '[{"action":"read","name":"catalog.entity.read"}, {"action":"delete","name":"catalog.entity.delete"}, {"action":"update","name":"catalog.entity.update"}]', + }; + await knex(CONDITIONAL_TABLE).insert( + conditionDaoWithFewActions, + ); + + await expect(async () => { + await db.checkConflictedConditions( + 'role:default/test', + 'catalog-entity', + 'catalog', + ['read', 'update', 'delete'], + ); + }).rejects.toThrow( + `Found condition with conflicted permission action '["read","update","delete"]'. Role could have multiple conditions for the same resource type 'catalog-entity', but with different permission action sets.`, + ); + }, + ); + + it.each(databases.eachSupportedId())( + 'should pass check, when there is one non conflicted condition', + async databasesId => { + const { knex, db } = await createDatabase(databasesId); + const filterConditionsSpy = jest.spyOn(db, 'filterConditions'); + + const conditionDaoWithFewActions = { + ...conditionDao1, + permissions: + '[{"action":"read","name":"catalog.entity.read"}, {"action":"update","name":"catalog.entity.update"}]', + }; + await knex(CONDITIONAL_TABLE).insert( + conditionDaoWithFewActions, + ); + + await db.checkConflictedConditions( 'role:default/test', 'catalog-entity', - ['catalog.entity.read'], + 'catalog', + ['delete'], ); - expect(condition).toEqual(condition1); + expect(filterConditionsSpy).toHaveBeenCalledTimes(1); + const result = await filterConditionsSpy.mock.results[0].value; + expect(result).toEqual([ + { + ...condition1, + permissionMapping: [ + { name: 'catalog.entity.read', action: 'read' }, + { name: 'catalog.entity.update', action: 'update' }, + ], + }, + ]); }, ); it.each(databases.eachSupportedId())( - 'should not find condition', + 'should pass check, when there are no conditions', async databasesId => { const { db } = await createDatabase(databasesId); + const filterConditionsSpy = jest.spyOn(db, 'filterConditions'); - const condition = await db.findUniqueCondition( + await db.checkConflictedConditions( 'role:default/test', 'catalog-entity', + 'catalog', ['read'], ); - expect(condition).toBeUndefined(); + expect(filterConditionsSpy).toHaveBeenCalledTimes(1); + const result = await filterConditionsSpy.mock.results[0].value; + expect(result).toEqual([]); }, ); }); @@ -393,7 +512,7 @@ describe('DataBaseConditionalStorage', () => { describe('updateCondition', () => { it.each(databases.eachSupportedId())( - 'should update condition', + 'should update condition with added new action', async databasesId => { const { knex, db } = await createDatabase(databasesId); await knex(CONDITIONAL_TABLE).insert( @@ -424,6 +543,36 @@ describe('DataBaseConditionalStorage', () => { }, ); + it.each(databases.eachSupportedId())( + 'should update condition with removed one action', + async databasesId => { + const { knex, db } = await createDatabase(databasesId); + await knex(CONDITIONAL_TABLE).insert({ + ...conditionDao1, + permissions: + '[{"action":"read","name":"catalog.entity.read"}, {"action":"delete","name":"catalog.entity.delete"}]', + }); + + const updateCondition: RoleConditionalPolicyDecision = { + ...condition1, + permissionMapping: [{ name: 'catalog.entity.read', action: 'read' }], + }; + await db.updateCondition(1, updateCondition); + + const condition = await knex + .table(CONDITIONAL_TABLE) + .select() + .where('id', 1); + expect(condition).toEqual([ + { + ...conditionDao1, + permissions: '[{"name":"catalog.entity.read","action":"read"}]', + id: 1, + }, + ]); + }, + ); + it.each(databases.eachSupportedId())( 'should fail to update condition, because condition not found', async databasesId => { @@ -466,8 +615,37 @@ describe('DataBaseConditionalStorage', () => { await expect(async () => { await db.updateCondition(1, updateCondition); }).rejects.toThrow( - `Found condition with conflicted permission '{"name":"catalog.entity.delete","action":"delete"}'. Role could have multiple ` + - `conditions for the same resource type 'catalog-entity', but with different permission name and action sets.`, + `Found condition with conflicted permission action '["delete"]'. Role could have multiple conditions for the same resource type 'catalog-entity', but with different permission action sets.`, + ); + }, + ); + + it.each(databases.eachSupportedId())( + 'should fail to update condition, because found condition with two conflicted actions', + async databasesId => { + const { knex, db } = await createDatabase(databasesId); + + await knex(CONDITIONAL_TABLE).insert( + conditionDao1, + ); + await knex(CONDITIONAL_TABLE).insert({ + ...conditionDao1, + permissions: + '[{"name": "catalog.entity.delete", "action": "delete"}, {"name": "catalog.entity.read", "action": "read"}]', + }); + + const updateCondition: RoleConditionalPolicyDecision = { + ...condition1, + permissionMapping: [ + { name: 'catalog.entity.read', action: 'read' }, + { name: 'catalog.entity.delete', action: 'delete' }, + ], + }; + await expect(async () => { + await db.updateCondition(1, updateCondition); + }).rejects.toThrow( + `Found condition with conflicted permission action '["read","delete"]'. Role could have multiple ` + + `conditions for the same resource type 'catalog-entity', but with different permission action sets.`, ); }, ); diff --git a/plugins/rbac-backend/src/service/permission-policy.test.ts b/plugins/rbac-backend/src/service/permission-policy.test.ts index 43bd023781..0404c33b61 100644 --- a/plugins/rbac-backend/src/service/permission-policy.test.ts +++ b/plugins/rbac-backend/src/service/permission-policy.test.ts @@ -66,7 +66,7 @@ const catalogApi = { const conditionalStorage: ConditionalStorage = { filterConditions: jest.fn().mockImplementation(() => []), createCondition: jest.fn().mockImplementation(), - findUniqueCondition: jest.fn().mockImplementation(), + checkConflictedConditions: jest.fn().mockImplementation(), getCondition: jest.fn().mockImplementation(), deleteCondition: jest.fn().mockImplementation(), updateCondition: jest.fn().mockImplementation(), @@ -1881,7 +1881,6 @@ describe('Policy checks for conditional policies', () => { ); catalogApi.getEntities.mockReset(); - (conditionalStorage.findUniqueCondition as jest.Mock).mockReset(); }); it('should execute condition policy', async () => { 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 ef0fb33ea9..ec27b0dfe5 100644 --- a/plugins/rbac-backend/src/service/policies-rest-api.test.ts +++ b/plugins/rbac-backend/src/service/policies-rest-api.test.ts @@ -141,7 +141,7 @@ const roleMetadataStorageMock: RoleMetadataStorage = { const conditionalStorage = { filterConditions: jest.fn().mockImplementation(), createCondition: jest.fn().mockImplementation(), - findUniqueCondition: jest.fn().mockImplementation(), + checkConflictedConditions: jest.fn().mockImplementation(), getCondition: jest.fn().mockImplementation(), deleteCondition: jest.fn().mockImplementation(), updateCondition: jest.fn().mockImplementation(),