diff --git a/packages/backend/package.json b/packages/backend/package.json index 6c0834a15c..63f0ec3122 100644 --- a/packages/backend/package.json +++ b/packages/backend/package.json @@ -17,6 +17,7 @@ }, "dependencies": { "@backstage/backend-defaults": "^0.2.17", + "@backstage/backend-plugin-api": "^0.6.17", "@backstage/plugin-app-backend": "^0.3.65", "@backstage/plugin-auth-backend": "^0.22.4", "@backstage/plugin-auth-backend-module-guest-provider": "^0.1.3", @@ -28,6 +29,12 @@ "@backstage/plugin-search-backend-module-catalog": "^0.1.23", "@backstage/plugin-search-backend-module-techdocs": "^0.1.22", "@backstage/plugin-techdocs-backend": "^1.10.4", + "@manypkg/get-packages": "^1.1.3", + "@backstage/config-loader": "^1.8.0", + "winston": "^3.11.0", + "@backstage/backend-app-api": "^0.7.2", + "@backstage/backend-dynamic-feature-service": "^0.2.9", + "@backstage/cli-node": "^0.2.5", "@janus-idp/backstage-plugin-rbac-backend": "*", "app": "*" }, diff --git a/packages/backend/src/index.ts b/packages/backend/src/index.ts index 7cdb3713ff..476f3f33ef 100644 --- a/packages/backend/src/index.ts +++ b/packages/backend/src/index.ts @@ -1,7 +1,28 @@ import { createBackend } from '@backstage/backend-defaults'; +import { dynamicPluginsSchemasServiceFactory } from '@backstage/backend-dynamic-feature-service'; +import { PackageRoles } from '@backstage/cli-node'; + +import * as path from 'path'; + +import { customLogger } from './logger/customLogger'; const backend = createBackend(); +backend.add( + dynamicPluginsSchemasServiceFactory({ + schemaLocator(pluginPackage) { + const platform = PackageRoles.getRoleInfo( + pluginPackage.manifest.backstage.role, + ).platform; + return path.join( + platform === 'node' ? 'dist' : 'dist-scalprum', + 'configSchema.json', + ); + }, + }), +); +backend.add(customLogger()); + backend.add(import('@backstage/plugin-app-backend/alpha')); backend.add(import('@backstage/plugin-proxy-backend/alpha')); backend.add(import('@backstage/plugin-scaffolder-backend/alpha')); diff --git a/packages/backend/src/logger/customLogger.ts b/packages/backend/src/logger/customLogger.ts new file mode 100644 index 0000000000..b7f0eb75bb --- /dev/null +++ b/packages/backend/src/logger/customLogger.ts @@ -0,0 +1,94 @@ +import { + createConfigSecretEnumerator, + WinstonLogger, +} from '@backstage/backend-app-api'; +import { DynamicPluginsSchemasService } from '@backstage/backend-dynamic-feature-service'; +import { + coreServices, + createServiceFactory, + createServiceRef, +} from '@backstage/backend-plugin-api'; +import { loadConfigSchema } from '@backstage/config-loader'; + +import { getPackages } from '@manypkg/get-packages'; +import * as winston from 'winston'; + +const defaultFormat = winston.format.combine( + winston.format.timestamp({ + format: 'YYYY-MM-DD HH:mm:ss', + }), + winston.format.errors({ stack: true }), + winston.format.splat(), +); + +const auditLogFormat = winston.format((info, opts) => { + const { isAuditLog, ...newInfo } = info; + + if (isAuditLog) { + // keep `isAuditLog` field + return opts.isAuditLog ? info : false; + } + + // remove `isAuditLog` field from non audit log events + return !opts.isAuditLog ? newInfo : false; +}); + +const transports = { + log: [ + new winston.transports.Console({ + format: winston.format.combine( + auditLogFormat({ isAuditLog: false }), + defaultFormat, + winston.format.json(), + ), + }), + ], + auditLog: [ + new winston.transports.Console({ + format: winston.format.combine( + auditLogFormat({ isAuditLog: true }), + defaultFormat, + winston.format.json(), + ), + }), + ], +}; + +const dynamicPluginsSchemasServiceRef = + createServiceRef({ + id: 'core.dynamicplugins.schemas', + scope: 'root', + }); + +export const customLogger = createServiceFactory({ + service: coreServices.rootLogger, + deps: { + config: coreServices.rootConfig, + schemas: dynamicPluginsSchemasServiceRef, + }, + async factory({ config, schemas }) { + const logger = WinstonLogger.create({ + meta: { + service: 'backstage', + }, + level: process.env.LOG_LEVEL ?? 'info', + format: winston.format.combine(defaultFormat, winston.format.json()), + transports: [...transports.log, ...transports.auditLog], + }); + + const configSchema = await loadConfigSchema({ + dependencies: (await getPackages(process.cwd())).packages.map( + p => p.packageJson.name, + ), + }); + + const secretEnumerator = await createConfigSecretEnumerator({ + logger, + schema: (await schemas.addDynamicPluginsSchemas(configSchema)).schema, + }); + logger.addRedactions(secretEnumerator(config)); + config.subscribe?.(() => logger.addRedactions(secretEnumerator(config))); + + return logger; + }, +}); diff --git a/packages/backend/src/logger/index.ts b/packages/backend/src/logger/index.ts new file mode 100644 index 0000000000..cf697dbf15 --- /dev/null +++ b/packages/backend/src/logger/index.ts @@ -0,0 +1 @@ +export * from './customLogger'; diff --git a/plugins/rbac-backend/docs/audit-log.md b/plugins/rbac-backend/docs/audit-log.md new file mode 100644 index 0000000000..896aaad6b7 --- /dev/null +++ b/plugins/rbac-backend/docs/audit-log.md @@ -0,0 +1,49 @@ +# Audit logging + +The RBAC backend plugin supports audit logging with the help of the @janus-idp/backstage-plugin-audit-log-node library. Audit logging helps to track the latest changes and events from the RBAC plugin: + +- RBAC role changes; +- RBAC permissions changes; +- RBAC conditions changes; +- Changes causing modification of application configuration; +- Changes causing modification of the permission policy file; +- GET requests for RBAC permission information; +- User authorization results to RBAC resources. + +The RBAC backend plugin logging doesn't provide information about the actual state of the permissions. The actual state of RBAC permissions can be found in the RBAC UI. Audit logging provides information about the event name, event message, RBAC permission changes, the actor who made these changes, time, log level, stage, status, some part of the request, response, and so on. You can use this information like a history of the RBAC permission hierarchy. + +Notice: RBAC permissions and conditions are bound to RBAC roles. However, the RBAC backend plugin logs information about permissions and conditions with the help of separated log messages. That's because for now, the RBAC plugin has a separated API for RBAC roles, RBAC permissions, and RBAC conditions. + +## Audit log actor + +The audit log actor can be a real REST API user or the RBAC plugin itself. When the actor is a REST API user, then the RBAC plugin logs the user's IP, browser agent, and hostname. The RBAC plugin can also be the actor of the events. In this case, the actor has a name: "rbac-backend". In this case, the plugin typically applies changes from the configuration or permission policy file. Application configuration and permission policy files usually mount to the application deployment with the help of config maps. Unfortunately, the RBAC plugin cannot track who originally made modifications to these resources. But you can enable Kubernetes API audit log: https://kubernetes.io/docs/tasks/debug/debug-cluster/audit. Then you can match RBAC plugin audit log events to the events from Kubernetes logs by time. + +## Audit log format + +The RBAC plugin prints information to the backend log in JSON format. The format of these messages is defined in the @janus-idp/backstage-plugin-audit-log-node library. Each audit log line contains the key "isAuditLog". + +You can change the log level with the help of the environment variable: LOG_LEVEL. + +Example logged RBAC events: + +a) RBAC role created with corresponding basic permissions and conditional permission: + +```json +backend:start: {"actor":{"actorId":"user:default/andrienkoaleksandr","hostname":"localhost","ip":"::1","userAgent":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/125.0.0.0 Safari/537.36"},"eventName":"CreateRole","isAuditLog":true,"level":"info","message":"Created role:default/test","meta":{"author":"user:default/andrienkoaleksandr","createdAt":"Tue, 04 Jun 2024 13:51:45 GMT","description":"some test role","lastModified":"Tue, 04 Jun 2024 13:51:45 GMT","members":["user:default/logarifm","group:default/team-a"],"modifiedBy":"user:default/andrienkoaleksandr","roleEntityRef":"role:default/test","source":"rest"},"plugin":"permission","request":{"body":{"memberReferences":["user:default/logarifm","group:default/team-a"],"metadata":{"description":"some test role"},"name":"role:default/test"},"method":"POST","params":{},"query":{},"url":"/api/permission/roles"},"response":{"status":201},"service":"backstage","stage":"sendResponse","status":"succeeded","timestamp":"2024-06-04 16:51:45"} + +backend:start: {"actor":{"actorId":"user:default/andrienkoaleksandr","hostname":"localhost","ip":"::1","userAgent":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/125.0.0.0 Safari/537.36"},"eventName":"CreatePolicy","isAuditLog":true,"level":"info","message":"Created permission policies","meta":{"policies":[["role:default/test","scaffolder-template","read","allow"]],"source":"rest"},"plugin":"permission","request":{"body":[{"effect":"allow","entityReference":"role:default/test","permission":"scaffolder-template","policy":"read"}],"method":"POST","params":{},"query":{},"url":"/api/permission/policies"},"response":{"status":201},"service":"backstage","stage":"sendResponse","status":"succeeded","timestamp":"2024-06-04 16:51:45"} + +backend:start: {"actor":{"actorId":"user:default/andrienkoaleksandr","hostname":"localhost","ip":"::1","userAgent":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/125.0.0.0 Safari/537.36"},"eventName":"CreateCondition","isAuditLog":true,"level":"info","message":"Created conditional permission policy","meta":{"condition":{"conditions":{"params":{"claims":["group:default/team-a"]},"resourceType":"catalog-entity","rule":"IS_ENTITY_OWNER"},"permissionMapping":[{"action":"read","name":"catalog.entity.read"},{"action":"delete","name":"catalog.entity.delete"},{"action":"update","name":"catalog.entity.refresh"}],"pluginId":"catalog","resourceType":"catalog-entity","result":"CONDITIONAL","roleEntityRef":"role:default/test"}},"plugin":"permission","request":{"body":{"conditions":{"params":{"claims":["group:default/team-a"]},"resourceType":"catalog-entity","rule":"IS_ENTITY_OWNER"},"permissionMapping":["read","delete","update"],"pluginId":"catalog","resourceType":"catalog-entity","result":"CONDITIONAL","roleEntityRef":"role:default/test"},"method":"POST","params":{},"query":{},"url":"/api/permission/roles/conditions"},"response":{"body":{"id":9},"status":201},"service":"backstage","stage":"sendResponse","status":"succeeded","timestamp":"2024-06-04 16:51:45"} +``` + +b) Check access user to application resource: + +```json +backend:start: {"actor":{"actorId":"user:default/andrienkoaleksandr"},"eventName":"PermissionEvaluationStarted","isAuditLog":true,"level":"info","message":"Policy check for user:default/andrienkoaleksandr","meta":{"action":"create","permissionName":"policy.entity.create","resourceType":"policy-entity","userEntityRef":"user:default/andrienkoaleksandr"},"plugin":"permission","service":"backstage","stage":"evaluatePermissionAccess","status":"succeeded","timestamp":"2024-06-04 16:51:45"} + +backend:start: {"actor":{"actorId":"user:default/andrienkoaleksandr"},"eventName":"PermissionEvaluationCompleted","isAuditLog":true,"level":"info","message":"user:default/andrienkoaleksandr is ALLOW for permission 'policy.entity.create', resource type 'policy-entity' and action 'create'","meta":{"action":"create","decision":{"result":"ALLOW"},"permissionName":"policy.entity.create","resourceType":"policy-entity","userEntityRef":"user:default/andrienkoaleksandr"},"plugin":"permission","service":"backstage","stage":"evaluatePermissionAccess","status":"succeeded","timestamp":"2024-06-04 16:51:45"} +``` + +Most audit log lines contain a metadata object. The RBAC plugin includes information about RBAC roles, permissions, conditions, and authorization results in this metadata. Metadata types can be found in the RBAC plugin file audit-log/audit-logger.ts. + +Notice: You need to properly configure the logger to see nested JSON objects in the audit log lines. diff --git a/plugins/rbac-backend/package.json b/plugins/rbac-backend/package.json index e99f8265e4..d3147ae933 100644 --- a/plugins/rbac-backend/package.json +++ b/plugins/rbac-backend/package.json @@ -38,6 +38,7 @@ "@dagrejs/graphlib": "^2.1.13", "@janus-idp/backstage-plugin-rbac-common": "1.4.2", "@janus-idp/backstage-plugin-rbac-node": "1.1.1", + "@janus-idp/backstage-plugin-audit-log-node": "1.0.2", "casbin": "^5.27.1", "chokidar": "^3.6.0", "csv-parse": "^5.5.5", diff --git a/plugins/rbac-backend/src/audit-log/audit-logger.ts b/plugins/rbac-backend/src/audit-log/audit-logger.ts new file mode 100644 index 0000000000..ccc7be79fb --- /dev/null +++ b/plugins/rbac-backend/src/audit-log/audit-logger.ts @@ -0,0 +1,153 @@ +import { + AuthorizeResult, + PolicyDecision, + ResourcePermission, +} from '@backstage/plugin-permission-common'; +import { PolicyQuery } from '@backstage/plugin-permission-node'; + +import { AuditLogOptions } from '@janus-idp/backstage-plugin-audit-log-node'; +import { + PermissionAction, + PermissionInfo, + RoleConditionalPolicyDecision, + Source, + toPermissionAction, +} from '@janus-idp/backstage-plugin-rbac-common'; + +export const RoleEvents = { + CREATE_ROLE: 'CreateRole', + UPDATE_ROLE: 'UpdateRole', + DELETE_ROLE: 'DeleteRole', + CREATE_OR_UPDATE_ROLE: 'CreateOrUpdateRole', + GET_ROLE: 'GetRole', + + CREATE_ROLE_ERROR: 'CreateRoleError', + UPDATE_ROLE_ERROR: 'UpdateRoleError', + DELETE_ROLE_ERROR: 'DeleteRoleError', + GET_ROLE_ERROR: 'GetRoleError', +} as const; + +export const PermissionEvents = { + CREATE_POLICY: 'CreatePolicy', + CREATE_OR_UPDATE_POLICY: 'CreateOrUpdatePolicy', + UPDATE_POLICY: 'UpdatePolicy', + DELETE_POLICY: 'DeletePolicy', + GET_POLICY: 'GetPolicy', + + CREATE_POLICY_ERROR: 'CreatePolicyError', + UPDATE_POLICY_ERROR: 'UpdatePolicyError', + DELETE_POLICY_ERROR: 'DeletePolicyError', + GET_POLICY_ERROR: 'GetPolicyError', +} as const; + +export type RoleAuditInfo = { + roleEntityRef: string; + description?: string; + source: Source; + + members: string[]; +}; + +export type PermissionAuditInfo = { + policies: string[][]; + source: Source; +}; + +export const EvaluationEvents = { + PERMISSION_EVALUATION_STARTED: 'PermissionEvaluationStarted', + PERMISSION_EVALUATION_COMPLETED: 'PermissionEvaluationCompleted', + CONDITION_EVALUATION_COMPLETED: 'ConditionEvaluationCompleted', + PERMISSION_EVALUATION_FAILED: 'PermissionEvaluationFailed', +} as const; + +export const ListPluginPoliciesEvents = { + GET_PLUGINS_POLICIES: 'GetPluginsPolicies', + GET_PLUGINS_POLICIES_ERROR: 'GetPluginsPoliciesError', +}; + +export const ListConditionEvents = { + GET_CONDITION_RULES: 'GetConditionRules', + GET_CONDITION_RULES_ERROR: 'GetConditionRulesError', +}; + +export type EvaluationAuditInfo = { + userEntityRef: string; + permissionName: string; + action: PermissionAction; + resourceType?: string; + decision?: PolicyDecision; +}; + +export const ConditionEvents = { + CREATE_CONDITION: 'CreateCondition', + UPDATE_CONDITION: 'UpdateCondition', + DELETE_CONDITION: 'DeleteCondition', + GET_CONDITION: 'GetCondition', + + CREATE_CONDITION_ERROR: 'CreateConditionError', + UPDATE_CONDITION_ERROR: 'UpdateConditionError', + DELETE_CONDITION_ERROR: 'DeleteConditionError', + GET_CONDITION_ERROR: 'GetConditionError', +}; + +export type ConditionAuditInfo = { + condition: RoleConditionalPolicyDecision; +}; + +export const RBAC_BACKEND = 'rbac-backend'; + +// Audit log stage for processing Role-Based Access Control (RBAC) data +export const HANDLE_RBAC_DATA_STAGE = 'handleRBACData'; + +// Audit log stage for determining access rights based on user permissions and resource information +export const EVALUATE_PERMISSION_ACCESS_STAGE = 'evaluatePermissionAccess'; + +// Audit log stage for sending the response to the client about handled permission policies, roles, and condition policies +export const SEND_RESPONSE_STAGE = 'sendResponse'; +export const RESPONSE_ERROR = 'responseError'; + +export function createPermissionEvaluationOptions( + message: string, + userEntityRef: string, + request: PolicyQuery, + policyDecision?: PolicyDecision, +): AuditLogOptions { + const auditInfo: EvaluationAuditInfo = { + userEntityRef, + permissionName: request.permission.name, + action: toPermissionAction(request.permission.attributes), + }; + + const resourceType = (request.permission as ResourcePermission).resourceType; + if (resourceType) { + auditInfo.resourceType = resourceType; + } + + let eventName; + if (!policyDecision) { + eventName = EvaluationEvents.PERMISSION_EVALUATION_STARTED; + } else { + auditInfo.decision = policyDecision; + + switch (policyDecision.result) { + case AuthorizeResult.DENY: + case AuthorizeResult.ALLOW: + eventName = EvaluationEvents.PERMISSION_EVALUATION_COMPLETED; + break; + case AuthorizeResult.CONDITIONAL: + eventName = EvaluationEvents.CONDITION_EVALUATION_COMPLETED; + break; + default: + throw new Error('Unknown policy decision result'); + } + } + + return { + actorId: userEntityRef, + message, + eventName, + metadata: auditInfo, + stage: EVALUATE_PERMISSION_ACCESS_STAGE, + status: 'succeeded', + }; +} diff --git a/plugins/rbac-backend/src/audit-log/rest-errors-interceptor.ts b/plugins/rbac-backend/src/audit-log/rest-errors-interceptor.ts new file mode 100644 index 0000000000..8d651519c1 --- /dev/null +++ b/plugins/rbac-backend/src/audit-log/rest-errors-interceptor.ts @@ -0,0 +1,116 @@ +import { ErrorRequestHandler, NextFunction, Request, Response } from 'express'; + +import { AuditLogger } from '@janus-idp/backstage-plugin-audit-log-node'; + +import { + ConditionEvents, + ListConditionEvents, + ListPluginPoliciesEvents, + PermissionEvents, + RESPONSE_ERROR, + RoleEvents, +} from './audit-logger'; + +// Mapping paths and methods to corresponding events and messages +const eventMap: { + [key: string]: { [key: string]: { event: string; message: string } }; +} = { + '/policies': { + POST: { + event: PermissionEvents.CREATE_POLICY_ERROR, + message: 'Failed to create permission policies', + }, + PUT: { + event: PermissionEvents.UPDATE_POLICY_ERROR, + message: 'Failed to update permission policies', + }, + DELETE: { + event: PermissionEvents.DELETE_POLICY_ERROR, + message: 'Failed to delete permission policies', + }, + GET: { + event: PermissionEvents.GET_POLICY_ERROR, + message: 'Failed to get permission policies', + }, + }, + '/roles': { + POST: { + event: RoleEvents.CREATE_ROLE_ERROR, + message: 'Failed to create role', + }, + PUT: { + event: RoleEvents.UPDATE_ROLE_ERROR, + message: 'Failed to update role', + }, + DELETE: { + event: RoleEvents.DELETE_ROLE_ERROR, + message: 'Failed to delete role', + }, + GET: { event: RoleEvents.GET_ROLE_ERROR, message: 'Failed to get role' }, + }, + '/roles/conditions': { + POST: { + event: ConditionEvents.CREATE_CONDITION_ERROR, + message: 'Failed to create condition', + }, + PUT: { + event: ConditionEvents.UPDATE_CONDITION_ERROR, + message: 'Failed to update condition', + }, + DELETE: { + event: ConditionEvents.DELETE_CONDITION_ERROR, + message: 'Failed to delete condition', + }, + GET: { + event: ConditionEvents.GET_CONDITION_ERROR, + message: 'Failed to get condition', + }, + }, + '/plugins/policies': { + GET: { + event: ListPluginPoliciesEvents.GET_PLUGINS_POLICIES_ERROR, + message: 'Failed to get list permission policies', + }, + }, + '/plugins/condition-rules': { + GET: { + event: ListConditionEvents.GET_CONDITION_RULES_ERROR, + message: 'Failed to get list condition rules and schemas', + }, + }, +}; + +// Audit log REST api errors interceptor. +export function auditError(auditLogger: AuditLogger): ErrorRequestHandler { + return async ( + err: Error, + req: Request, + _resp: Response, + next: NextFunction, + ) => { + const matchedPath = Object.keys(eventMap).find(path => + req.path.startsWith(path), + ); + if (matchedPath) { + const methodEvents = eventMap[matchedPath][req.method]; + if (methodEvents) { + const { event, message } = methodEvents; + try { + await auditLogger.auditLog({ + message, + eventName: event, + stage: RESPONSE_ERROR, + status: 'failed', + request: req, + errors: [err], + }); + next(err); + } catch (auditLogError) { + next(auditLogError); + } + return; + } + } + next(err); + }; +} diff --git a/plugins/rbac-backend/src/database/role-metadata.test.ts b/plugins/rbac-backend/src/database/role-metadata.test.ts index 681d44ad96..d448aa6b11 100644 --- a/plugins/rbac-backend/src/database/role-metadata.test.ts +++ b/plugins/rbac-backend/src/database/role-metadata.test.ts @@ -17,6 +17,7 @@ describe('role-metadata-db-table', () => { const databases = TestDatabases.create({ ids: ['POSTGRES_13', 'SQLITE_3'], }); + const modifiedBy = 'user:default/some-user'; async function createDatabase(databaseId: TestDatabaseId) { const knex = await databases.init(databaseId); @@ -60,6 +61,7 @@ describe('role-metadata-db-table', () => { await knex(ROLE_METADATA_TABLE).insert({ roleEntityRef: 'role:default/some-super-important-role', source: 'rest', + modifiedBy, }); const trx = await knex.transaction(); @@ -75,7 +77,7 @@ describe('role-metadata-db-table', () => { description: null, id: 1, lastModified: null, - modifiedBy: null, + modifiedBy, roleEntityRef: 'role:default/some-super-important-role', source: 'rest', }); @@ -100,6 +102,7 @@ describe('role-metadata-db-table', () => { { source: 'configuration', roleEntityRef: 'role:default/some-super-important-role', + modifiedBy, }, trx, ); @@ -121,7 +124,7 @@ describe('role-metadata-db-table', () => { description: null, id: 1, lastModified: null, - modifiedBy: null, + modifiedBy, source: 'configuration', }); }, @@ -144,6 +147,7 @@ describe('role-metadata-db-table', () => { { source: 'configuration', roleEntityRef: 'role:default/some-super-important-role', + modifiedBy, }, trx, @@ -173,11 +177,12 @@ describe('role-metadata-db-table', () => { { source: 'configuration', roleEntityRef: 'role:default/some-super-important-role', + modifiedBy, }, trx, ), ).rejects.toThrow( - `Failed to create the role metadata: '{"source":"configuration","roleEntityRef":"role:default/some-super-important-role"}'.`, + `Failed to create the role metadata: '{"source":"configuration","roleEntityRef":"role:default/some-super-important-role","modifiedBy":"user:default/some-user"}'.`, ); }); @@ -196,6 +201,7 @@ describe('role-metadata-db-table', () => { { source: 'configuration', roleEntityRef: 'role:default/some-super-important-role', + modifiedBy, }, trx, ); @@ -205,7 +211,7 @@ describe('role-metadata-db-table', () => { throw err; } }).rejects.toThrow( - `Failed to create the role metadata: '{"source":"configuration","roleEntityRef":"role:default/some-super-important-role"}'.`, + `Failed to create the role metadata: '{"source":"configuration","roleEntityRef":"role:default/some-super-important-role","modifiedBy":"user:default/some-user"}'.`, ); }); @@ -226,6 +232,7 @@ describe('role-metadata-db-table', () => { { source: 'configuration', roleEntityRef: 'role:default/some-super-important-role', + modifiedBy, }, trx, ); @@ -255,6 +262,7 @@ describe('role-metadata-db-table', () => { { roleEntityRef: 'role:default/some-super-important-role', source: 'rest', + modifiedBy, }, 'role:default/some-super-important-role', trx, @@ -278,7 +286,7 @@ describe('role-metadata-db-table', () => { roleEntityRef: 'role:default/some-super-important-role', id: 1, lastModified: null, - modifiedBy: null, + modifiedBy, }); }, ); @@ -300,6 +308,7 @@ describe('role-metadata-db-table', () => { { roleEntityRef: 'role:default/some-super-important-role', source: 'configuration', + modifiedBy, }, 'role:default/some-super-important-role', trx, @@ -329,6 +338,7 @@ describe('role-metadata-db-table', () => { { roleEntityRef: 'role:default/important-role', source: 'configuration', + modifiedBy, }, 'role:default/some-super-important-role', trx, @@ -352,7 +362,7 @@ describe('role-metadata-db-table', () => { roleEntityRef: 'role:default/important-role', id: 1, lastModified: null, - modifiedBy: null, + modifiedBy, }); }, ); @@ -369,6 +379,7 @@ describe('role-metadata-db-table', () => { { roleEntityRef: 'role:default/important-role', source: 'configuration', + modifiedBy, }, 'role:default/some-super-important-role', trx, @@ -403,6 +414,7 @@ describe('role-metadata-db-table', () => { { roleEntityRef: 'role:default/important-role', source: 'configuration', + modifiedBy, }, 'role:default/some-super-important-role', trx, @@ -413,7 +425,7 @@ describe('role-metadata-db-table', () => { throw err; } }).rejects.toThrow( - `Failed to update the role metadata '{"roleEntityRef":"role:default/some-super-important-role","source":"configuration","id":1}' with new value: '{"roleEntityRef":"role:default/important-role","source":"configuration"}'.`, + `Failed to update the role metadata '{"roleEntityRef":"role:default/some-super-important-role","source":"configuration","id":1}' with new value: '{"roleEntityRef":"role:default/important-role","source":"configuration","modifiedBy":"user:default/some-user"}'.`, ); }); @@ -436,6 +448,7 @@ describe('role-metadata-db-table', () => { { roleEntityRef: 'role:default/important-role', source: 'configuration', + modifiedBy, }, 'role:default/some-super-important-role', trx, @@ -446,7 +459,7 @@ describe('role-metadata-db-table', () => { throw err; } }).rejects.toThrow( - `Failed to update the role metadata '{"roleEntityRef":"role:default/some-super-important-role","source":"configuration","id":1}' with new value: '{"roleEntityRef":"role:default/important-role","source":"configuration"}'.`, + `Failed to update the role metadata '{"roleEntityRef":"role:default/some-super-important-role","source":"configuration","id":1}' with new value: '{"roleEntityRef":"role:default/important-role","source":"configuration","modifiedBy":"user:default/some-user"}'.`, ); }); @@ -471,6 +484,7 @@ describe('role-metadata-db-table', () => { { roleEntityRef: 'role:default/important-role', source: 'configuration', + modifiedBy, }, 'role:default/some-super-important-role', trx, diff --git a/plugins/rbac-backend/src/database/role-metadata.ts b/plugins/rbac-backend/src/database/role-metadata.ts index cb58930151..abdec7af85 100644 --- a/plugins/rbac-backend/src/database/role-metadata.ts +++ b/plugins/rbac-backend/src/database/role-metadata.ts @@ -12,6 +12,7 @@ export interface RoleMetadataDao extends RoleMetadata { id?: number; roleEntityRef: string; source: Source; + modifiedBy: string; } export interface RoleMetadataStorage { @@ -139,14 +140,3 @@ export function daoToMetadata(dao: RoleMetadataDao): RoleMetadata { lastModified: dao.lastModified, }; } - -export function metadataToDao( - roleMetadata: RoleMetadataDao, - roleEntityRef: string, -): RoleMetadataDao { - return { - roleEntityRef, - source: roleMetadata.source, - description: roleMetadata.description, - }; -} diff --git a/plugins/rbac-backend/src/file-permissions/csv-file-watcher.test.ts b/plugins/rbac-backend/src/file-permissions/csv-file-watcher.test.ts index aac3e9e159..02d1787852 100644 --- a/plugins/rbac-backend/src/file-permissions/csv-file-watcher.test.ts +++ b/plugins/rbac-backend/src/file-permissions/csv-file-watcher.test.ts @@ -32,7 +32,11 @@ import { policyToString } from '../helper'; import { BackstageRoleManager } from '../role-manager/role-manager'; import { EnforcerDelegate } from '../service/enforcer-delegate'; import { MODEL } from '../service/permission-model'; -import { CSVFileWatcher } from './csv-file-watcher'; +import { ADMIN_ROLE_AUTHOR } from '../service/permission-policy'; +import { + CSV_PERMISSION_POLICY_FILE_AUTHOR, + CSVFileWatcher, +} from './csv-file-watcher'; const legacyPermission = [ 'role:default/legacy', @@ -81,8 +85,11 @@ const catalogApi = { const loggerMock: any = { warn: jest.fn().mockImplementation(), debug: jest.fn().mockImplementation(), + info: jest.fn().mockImplementation(), }; +const modifiedBy = 'user:default/some-admin'; + const roleMetadataStorageMock: RoleMetadataStorage = { findRoleMetadata: jest .fn() @@ -92,17 +99,26 @@ const roleMetadataStorageMock: RoleMetadataStorage = { _trx: Knex.Knex.Transaction, ): Promise => { if (roleEntityRef === legacyPermission[0]) { - return { roleEntityRef: legacyPermission[0], source: 'legacy' }; + return { + roleEntityRef: legacyPermission[0], + source: 'legacy', + modifiedBy, + }; } else if (roleEntityRef === restPermission[0]) { - return { roleEntityRef: restPermission[0], source: 'rest' }; + return { + roleEntityRef: restPermission[0], + source: 'rest', + modifiedBy, + }; } if (roleEntityRef === configPermission[0]) { return { roleEntityRef: configPermission[0], source: 'configuration', + modifiedBy, }; } - return { roleEntityRef: '', source: 'csv-file' }; + return { roleEntityRef: '', source: 'csv-file', modifiedBy }; }, ), createRoleMetadata: jest.fn().mockImplementation(), @@ -178,6 +194,12 @@ const dbManagerMock = Knex.knex({ client: MockClient }); const mockAuthService = mockServices.auth(); +const auditLoggerMock = { + getActorId: jest.fn().mockImplementation(), + createAuditLogDetails: jest.fn().mockImplementation(), + auditLog: jest.fn().mockImplementation(), +}; + const currentPermissionPolicies = [ ['role:default/catalog-writer', 'catalog-entity', 'update', 'allow'], ['role:default/legacy', 'catalog-entity', 'update', 'allow'], @@ -239,7 +261,9 @@ describe('CSVFileWatcher', () => { enforcerDelegate, loggerMock, roleMetadataStorageMock, + auditLoggerMock, ); + auditLoggerMock.auditLog.mockReset(); }); afterEach(() => { @@ -303,6 +327,7 @@ describe('CSVFileWatcher', () => { await enforcerDelegate.addGroupingPolicy(legacyRole, { roleEntityRef: legacyRole[1], source: 'legacy', + modifiedBy: CSV_PERMISSION_POLICY_FILE_AUTHOR, }); await csvFileWatcher.initialize(csvFileName, false); @@ -559,10 +584,12 @@ describe('CSVFileWatcher', () => { await enforcerDelegate.addGroupingPolicy(configRole, { roleEntityRef: configRole[1], source: 'configuration', + modifiedBy: ADMIN_ROLE_AUTHOR, }); await enforcerDelegate.addGroupingPolicy(restRole, { roleEntityRef: restRole[1], source: 'rest', + modifiedBy, }); csvFileWatcher.parse = jest.fn().mockImplementation(() => { diff --git a/plugins/rbac-backend/src/file-permissions/csv-file-watcher.ts b/plugins/rbac-backend/src/file-permissions/csv-file-watcher.ts index 9d3b281d1b..5a12b47887 100644 --- a/plugins/rbac-backend/src/file-permissions/csv-file-watcher.ts +++ b/plugins/rbac-backend/src/file-permissions/csv-file-watcher.ts @@ -4,9 +4,20 @@ import { parse } from 'csv-parse/sync'; import { difference } from 'lodash'; import { Logger } from 'winston'; +import { AuditLogger } from '@janus-idp/backstage-plugin-audit-log-node'; + import fs from 'fs'; -import { RoleMetadataStorage } from '../database/role-metadata'; +import { + HANDLE_RBAC_DATA_STAGE, + PermissionEvents, + RBAC_BACKEND, + RoleEvents, +} from '../audit-log/audit-logger'; +import { + RoleMetadataDao, + RoleMetadataStorage, +} from '../database/role-metadata'; import { metadataStringToPolicy, policyToString, @@ -39,6 +50,7 @@ export class CSVFileWatcher { private readonly enforcer: EnforcerDelegate, private readonly logger: Logger, private readonly roleMetadataStorage: RoleMetadataStorage, + private readonly auditLogger: AuditLogger, ) { this.csvFileName = ''; this.currentContent = []; @@ -295,6 +307,15 @@ export class CSVFileWatcher { } try { await this.enforcer.addOrUpdatePolicy(policy, 'csv-file'); + + await this.auditLogger.auditLog({ + actorId: RBAC_BACKEND, + message: `Created or updated policy`, + eventName: PermissionEvents.CREATE_OR_UPDATE_POLICY, + metadata: { policies: [policy], source: 'csv-file' }, + stage: HANDLE_RBAC_DATA_STAGE, + status: 'succeeded', + }); } catch (e) { this.logger.warn( `Failed to add or update policy ${policy} after modification ${this.csvFileName}. Cause: ${e}`, @@ -311,6 +332,18 @@ export class CSVFileWatcher { async removePermissionPolicies(): Promise { try { await this.enforcer.removePolicies(this.csvFilePolicies.removedPolicies); + + await this.auditLogger.auditLog({ + actorId: RBAC_BACKEND, + message: `Deleted policies`, + eventName: PermissionEvents.DELETE_POLICY, + metadata: { + policies: this.csvFilePolicies.removedPolicies, + source: 'csv-file', + }, + stage: HANDLE_RBAC_DATA_STAGE, + status: 'succeeded', + }); } catch (e) { this.logger.warn( `Failed to remove policies ${JSON.stringify( @@ -352,11 +385,33 @@ export class CSVFileWatcher { } try { - await this.enforcer.addOrUpdateGroupingPolicy(groupPolicy, { + const roleMetadata: RoleMetadataDao = { source: 'csv-file', roleEntityRef: groupPolicy[1], author: CSV_PERMISSION_POLICY_FILE_AUTHOR, modifiedBy: CSV_PERMISSION_POLICY_FILE_AUTHOR, + }; + + const currentMetadata = await this.roleMetadataStorage.findRoleMetadata( + roleMetadata.roleEntityRef, + ); + + await this.enforcer.addOrUpdateGroupingPolicy( + groupPolicy, + roleMetadata, + ); + + const eventName = currentMetadata + ? RoleEvents.UPDATE_ROLE + : RoleEvents.CREATE_ROLE; + const message = currentMetadata ? 'Updated role' : 'Created role'; + await this.auditLogger.auditLog({ + actorId: RBAC_BACKEND, + message, + eventName, + metadata: { ...roleMetadata, members: [groupPolicy[0]] }, + stage: HANDLE_RBAC_DATA_STAGE, + status: 'succeeded', }); } catch (e) { this.logger.warn( @@ -374,24 +429,44 @@ export class CSVFileWatcher { */ async removeRoles(): Promise { for (const groupPolicy of this.csvFilePolicies.removedGroupPolicies) { + const roleEntityRef = groupPolicy[1]; // this requires knowledge of whether or not it is an update const isUpdate = await this.enforcer.getFilteredGroupingPolicy( 1, - groupPolicy[1], + roleEntityRef, ); // Need to update the time try { + const metadata: RoleMetadataDao = { + source: 'csv-file', + roleEntityRef, + author: CSV_PERMISSION_POLICY_FILE_AUTHOR, + modifiedBy: CSV_PERMISSION_POLICY_FILE_AUTHOR, + }; + await this.enforcer.removeGroupingPolicy( groupPolicy, - { - source: 'csv-file', - roleEntityRef: groupPolicy[1], - author: CSV_PERMISSION_POLICY_FILE_AUTHOR, - modifiedBy: CSV_PERMISSION_POLICY_FILE_AUTHOR, - }, + metadata, isUpdate.length > 1, ); + + const isRolePresent = + await this.roleMetadataStorage.findRoleMetadata(roleEntityRef); + const eventName = isRolePresent + ? RoleEvents.UPDATE_ROLE + : RoleEvents.DELETE_ROLE; + const message = isRolePresent + ? 'Updated role: deleted members' + : 'Deleted role'; + await this.auditLogger.auditLog({ + actorId: RBAC_BACKEND, + message, + eventName, + metadata: { ...metadata, members: [groupPolicy[0]] }, + stage: HANDLE_RBAC_DATA_STAGE, + status: 'succeeded', + }); } catch (e) { this.logger.warn( `Failed to remove group policy ${groupPolicy} after modification ${this.csvFileName}. Cause: ${e}`, diff --git a/plugins/rbac-backend/src/helper.test.ts b/plugins/rbac-backend/src/helper.test.ts index 0aafaf0a2f..fd8ea11cde 100644 --- a/plugins/rbac-backend/src/helper.test.ts +++ b/plugins/rbac-backend/src/helper.test.ts @@ -12,6 +12,14 @@ import { import { EnforcerDelegate } from './service/enforcer-delegate'; import { ADMIN_ROLE_AUTHOR } from './service/permission-policy'; +const modifiedBy = 'user:default/some-user'; + +const auditLoggerMock = { + getActorId: jest.fn().mockImplementation(), + createAuditLogDetails: jest.fn().mockImplementation(), + auditLog: jest.fn().mockImplementation(), +}; + describe('helper.ts', () => { describe('policyToString', () => { it('should convert permission policy to string', () => { @@ -65,11 +73,13 @@ describe('helper.ts', () => { describe('removeTheDifference', () => { const mockEnforcerDelegate: Partial = { - removeGroupingPolicy: jest.fn().mockImplementation(), + removeGroupingPolicies: jest.fn().mockImplementation(), + getFilteredGroupingPolicy: jest.fn().mockReturnValue([]), }; beforeEach(() => { - (mockEnforcerDelegate.removeGroupingPolicy as jest.Mock).mockClear(); + (mockEnforcerDelegate.removeGroupingPolicies as jest.Mock).mockClear(); + auditLoggerMock.auditLog.mockReset(); }); it('removes the difference between originalGroup and addedGroup', async () => { @@ -88,11 +98,12 @@ describe('helper.ts', () => { source, roleName, mockEnforcerDelegate as EnforcerDelegate, + auditLoggerMock, ADMIN_ROLE_AUTHOR, ); - expect(mockEnforcerDelegate.removeGroupingPolicy).toHaveBeenCalledWith( - ['user:default/admin', roleName], + expect(mockEnforcerDelegate.removeGroupingPolicies).toHaveBeenCalledWith( + [['user:default/admin', roleName]], { modifiedBy: ADMIN_ROLE_AUTHOR, roleEntityRef: 'role:default/admin', @@ -114,10 +125,13 @@ describe('helper.ts', () => { source, roleName, mockEnforcerDelegate as EnforcerDelegate, + auditLoggerMock, ADMIN_ROLE_AUTHOR, ); - expect(mockEnforcerDelegate.removeGroupingPolicy).not.toHaveBeenCalled(); + expect( + mockEnforcerDelegate.removeGroupingPolicies, + ).not.toHaveBeenCalled(); }); it('does nothing when originalGroup is empty', async () => { @@ -132,10 +146,13 @@ describe('helper.ts', () => { source, roleName, mockEnforcerDelegate as EnforcerDelegate, + auditLoggerMock, ADMIN_ROLE_AUTHOR, ); - expect(mockEnforcerDelegate.removeGroupingPolicy).not.toHaveBeenCalled(); + expect( + mockEnforcerDelegate.removeGroupingPolicies, + ).not.toHaveBeenCalled(); }); }); @@ -167,12 +184,14 @@ describe('helper.ts', () => { id: 1, roleEntityRef: 'role:default/qa', source: 'rest', + modifiedBy, }; const obj2: RoleMetadataDao = { roleEntityRef: 'role:default/qa', description: 'qa team', id: 1, source: 'rest', + modifiedBy, }; expect(deepSortedEqual(obj1, obj2)).toBe(true); }); @@ -183,12 +202,14 @@ describe('helper.ts', () => { id: 1, roleEntityRef: 'role:default/qa', source: 'rest', + modifiedBy, }; const obj2: RoleMetadataDao = { id: 1, description: 'qa team', source: 'rest', roleEntityRef: 'role:default/qa', + modifiedBy, }; expect(deepSortedEqual(obj1, obj2)).toBe(true); }); @@ -210,6 +231,7 @@ describe('helper.ts', () => { description: 'qa team', source: 'rest', roleEntityRef: 'role:default/qa', + modifiedBy, }; expect( deepSortedEqual(obj1, obj2, [ @@ -227,12 +249,14 @@ describe('helper.ts', () => { id: 1, roleEntityRef: 'role:default/qa', source: 'rest', + modifiedBy, }; const obj2: RoleMetadataDao = { description: 'great qa', id: 1, roleEntityRef: 'role:default/qa', source: 'rest', + modifiedBy, }; expect(deepSortedEqual(obj1, obj2)).toBe(false); }); @@ -243,12 +267,14 @@ describe('helper.ts', () => { id: 1, roleEntityRef: 'role:default/qa', source: 'rest', + modifiedBy, }; const obj2: RoleMetadataDao = { description: 'qa teams', id: 1, roleEntityRef: 'role:default/qa', source: 'configuration', + modifiedBy, }; expect(deepSortedEqual(obj1, obj2)).toBe(false); }); @@ -259,12 +285,14 @@ describe('helper.ts', () => { id: 1, roleEntityRef: 'role:default/qa', source: 'rest', + modifiedBy, }; const obj2: RoleMetadataDao = { description: 'qa teams', id: 2, roleEntityRef: 'role:default/qa', source: 'rest', + modifiedBy, }; expect(deepSortedEqual(obj1, obj2)).toBe(false); }); @@ -275,12 +303,14 @@ describe('helper.ts', () => { id: 1, roleEntityRef: 'role:default/qa', source: 'rest', + modifiedBy, }; const obj2: RoleMetadataDao = { description: 'qa teams', id: 1, roleEntityRef: 'role:default/dev', source: 'rest', + modifiedBy, }; expect(deepSortedEqual(obj1, obj2)).toBe(false); }); diff --git a/plugins/rbac-backend/src/helper.ts b/plugins/rbac-backend/src/helper.ts index fb08566ad1..c3bff21911 100644 --- a/plugins/rbac-backend/src/helper.ts +++ b/plugins/rbac-backend/src/helper.ts @@ -7,12 +7,18 @@ import { ValueKeyIteratee, } from 'lodash'; +import { AuditLogger } from '@janus-idp/backstage-plugin-audit-log-node'; import { PermissionAction, RoleBasedPolicy, Source, } from '@janus-idp/backstage-plugin-rbac-common'; +import { + HANDLE_RBAC_DATA_STAGE, + RBAC_BACKEND, + RoleEvents, +} from './audit-log/audit-logger'; import { EnforcerDelegate } from './service/enforcer-delegate'; export function policyToString(policy: string[]): string { @@ -36,20 +42,48 @@ export async function removeTheDifference( source: Source, roleEntityRef: string, enf: EnforcerDelegate, + auditLogger: AuditLogger, modifiedBy: string, ): Promise { originalGroup.sort((a, b) => a.localeCompare(b)); addedGroup.sort((a, b) => a.localeCompare(b)); const missing = difference(originalGroup, addedGroup); + const groupPolicies: string[][] = []; for (const missingRole of missing) { - const role = [missingRole, roleEntityRef]; - await enf.removeGroupingPolicy( - role, - { source, modifiedBy, roleEntityRef }, - false, - ); + groupPolicies.push([missingRole, roleEntityRef]); + } + + if (groupPolicies.length === 0) { + return; } + + const roleMetadata = { source, modifiedBy, roleEntityRef }; + await enf.removeGroupingPolicies(groupPolicies, roleMetadata, false); + + const remainingMembers = await enf.getFilteredGroupingPolicy( + 1, + roleEntityRef, + ); + const message = + remainingMembers.length > 0 + ? 'Updated role: deleted members' + : 'Deleted role'; + const eventName = + remainingMembers.length > 0 + ? RoleEvents.UPDATE_ROLE + : RoleEvents.DELETE_ROLE; + await auditLogger.auditLog({ + actorId: RBAC_BACKEND, + message, + eventName, + metadata: { + ...roleMetadata, + members: groupPolicies.map(gp => gp[0]), + }, + stage: HANDLE_RBAC_DATA_STAGE, + status: 'succeeded', + }); } export function transformArrayToPolicy(policyArray: string[]): RoleBasedPolicy { diff --git a/plugins/rbac-backend/src/service/enforcer-delegate.test.ts b/plugins/rbac-backend/src/service/enforcer-delegate.test.ts index 0b1b1f3c74..ece81fa9ab 100644 --- a/plugins/rbac-backend/src/service/enforcer-delegate.test.ts +++ b/plugins/rbac-backend/src/service/enforcer-delegate.test.ts @@ -126,6 +126,8 @@ describe('EnforcerDelegate', () => { any >; + const modifiedBy = 'user:default/some-admin'; + beforeEach(() => { (policyMetadataStorageMock.createPolicyMetadata as jest.Mock).mockReset(); (roleMetadataStorageMock.createRoleMetadata as jest.Mock).mockReset(); @@ -136,6 +138,8 @@ describe('EnforcerDelegate', () => { (policyMetadataStorageMock.findPolicyMetadata as jest.Mock).mockReset(); }); + const knex = Knex.knex({ client: MockClient }); + async function createEnfDelegate( policies?: string[][], groupingPolicies?: string[][], @@ -180,8 +184,6 @@ describe('EnforcerDelegate', () => { await enf.addGroupingPolicies(groupingPolicies); } - const knex = Knex.knex({ client: MockClient }); - return new EnforcerDelegate( enf, policyMetadataStorageMock, @@ -414,9 +416,12 @@ describe('EnforcerDelegate', () => { const enfDelegate = await createEnfDelegate(); + const roleEntityRef = 'role:default/dev-team'; await enfDelegate.addGroupingPolicy(groupingPolicy, { source: 'rest', - roleEntityRef: 'role:default/dev-team', + roleEntityRef: roleEntityRef, + author: modifiedBy, + modifiedBy, }); expect(enfUpdateGroupingPolicySpy).toHaveBeenCalledWith( @@ -454,6 +459,8 @@ describe('EnforcerDelegate', () => { enfDelegate.addGroupingPolicy(groupingPolicy, { source: 'rest', roleEntityRef: 'role:default/dev-team', + author: 'user:default/some-user', + modifiedBy: 'user:default/some-user', }), ).rejects.toThrow('some unexpected error'); }); @@ -471,11 +478,13 @@ describe('EnforcerDelegate', () => { enfDelegate.addGroupingPolicy(groupingPolicy, { source: 'rest', roleEntityRef: 'role:default/dev-team', + author: modifiedBy, + modifiedBy, }), ).rejects.toThrow('some unexpected error'); }); - it('should update role metadata, because metadata has been created', async () => { + it('should update role metadata on addGroupingPolicy, because metadata has been created', async () => { roleMetadataStorageMock.findRoleMetadata = jest .fn() .mockImplementation( @@ -487,15 +496,20 @@ describe('EnforcerDelegate', () => { source: 'csv-file', roleEntityRef: 'role:default/dev-team', createdAt: '2024-03-01 00:23:41+00', + author: modifiedBy, + modifiedBy, }; }, ); const enfDelegate = await createEnfDelegate(); + const roleEntityRef = 'role:default/dev-team'; await enfDelegate.addGroupingPolicy(groupingPolicy, { source: 'rest', - roleEntityRef: 'role:default/dev-team', + roleEntityRef, + author: modifiedBy, + modifiedBy, }); expect(enfUpdateGroupingPolicySpy).toHaveBeenCalledWith( @@ -525,7 +539,8 @@ describe('EnforcerDelegate', () => { const roleMetadataDao: RoleMetadataDao = { roleEntityRef: 'role:default/security', source: 'rest', - author: 'user:default/some-user', + author: modifiedBy, + modifiedBy, }; await enfDelegate.addGroupingPolicies( [groupingPolicy, secondGroupingPolicy], @@ -553,7 +568,7 @@ describe('EnforcerDelegate', () => { const createdAtData = new Date(`${metadata.createdAt}`); const lastModified = new Date(`${metadata.lastModified}`); expect(lastModified).toEqual(createdAtData); - expect(metadata.author).toEqual('user:default/some-user'); + expect(metadata.author).toEqual(modifiedBy); expect(metadata.roleEntityRef).toEqual('role:default/security'); expect(metadata.source).toEqual('rest'); expect(metadata.description).toBeUndefined(); @@ -562,10 +577,13 @@ describe('EnforcerDelegate', () => { it('should add grouping policies and create role metadata with description', async () => { const enfDelegate = await createEnfDelegate(); + const description = 'Role for security engineers'; const roleMetadataDao: RoleMetadataDao = { roleEntityRef: 'role:default/security', source: 'rest', - description: 'Role for security engineers', + description, + author: modifiedBy, + modifiedBy, }; await enfDelegate.addGroupingPolicies( [groupingPolicy, secondGroupingPolicy], @@ -610,6 +628,8 @@ describe('EnforcerDelegate', () => { const roleMetadataDao: RoleMetadataDao = { roleEntityRef: 'role:default/security', source: 'rest', + author: 'user:default/some-user', + modifiedBy: 'user:default/some-user', }; await expect( enfDelegate.addGroupingPolicies( @@ -639,7 +659,8 @@ describe('EnforcerDelegate', () => { const roleMetadataDao: RoleMetadataDao = { roleEntityRef: 'role:default/dev-team', source: 'rest', - modifiedBy: 'user:default/system-admin', + author: 'user:default/some-user', + modifiedBy, }; await enfDelegate.addGroupingPolicies( [ @@ -671,6 +692,77 @@ describe('EnforcerDelegate', () => { expect(roleMetadataStorageMock.createRoleMetadata).not.toHaveBeenCalled(); + const metadata = (roleMetadataStorageMock.updateRoleMetadata as jest.Mock) + .mock.calls[0][0]; + + const createdAtData = new Date(`${metadata.createdAt}`); + const lastModified = new Date(`${metadata.lastModified}`); + expect(lastModified > createdAtData).toBeTruthy(); + expect(metadata.author).toEqual('user:default/some-user'); + expect(metadata.description).toEqual('Role for dev engineers'); + expect(metadata.modifiedBy).toEqual(modifiedBy); + expect(metadata.roleEntityRef).toEqual('role:default/dev-team'); + expect(metadata.source).toEqual('rest'); + }); + + it('should update role metadata, because metadata has been created, but skip audit logging, because external transaction', async () => { + (roleMetadataStorageMock.findRoleMetadata as jest.Mock) = jest + .fn() + .mockReturnValueOnce({ + source: 'csv-file', + roleEntityRef: 'role:default/dev-team', + author: 'user:default/some-user', + description: 'Role for dev engineers', + createdAt: '2024-03-01 00:23:41+00', + }); + + const enfDelegate = await createEnfDelegate(); + + const roleMetadataDao: RoleMetadataDao = { + roleEntityRef: 'role:default/dev-team', + source: 'rest', + author: 'user:default/some-user', + modifiedBy: 'user:default/system-admin', + }; + + const trx = await knex.transaction(); + try { + await enfDelegate.addGroupingPolicies( + [ + ['user:default/tom', 'role:default/dev-team'], + ['user:default/tim', 'role:default/dev-team'], + ], + roleMetadataDao, + trx, + ); + trx.commit(); + } catch (err) { + trx.rollback(err); + } + + const storedPolicies = await enfDelegate.getGroupingPolicy(); + + expect(storedPolicies).toEqual([ + ['user:default/tom', 'role:default/dev-team'], + ['user:default/tim', 'role:default/dev-team'], + ]); + expect( + policyMetadataStorageMock.createPolicyMetadata, + ).toHaveBeenCalledWith( + 'rest', + ['user:default/tom', 'role:default/dev-team'], + expect.anything(), + ); + expect( + policyMetadataStorageMock.createPolicyMetadata, + ).toHaveBeenCalledWith( + 'rest', + ['user:default/tim', 'role:default/dev-team'], + expect.anything(), + ); + + expect(roleMetadataStorageMock.createRoleMetadata).not.toHaveBeenCalled(); + const metadata = (roleMetadataStorageMock.updateRoleMetadata as jest.Mock) .mock.calls[0][0]; @@ -694,6 +786,7 @@ describe('EnforcerDelegate', () => { source: 'rest', roleEntityRef: 'role:default/dev-team', author: 'user:default/tom', + modifiedBy: 'user:default/tom', description: 'Role for dev engineers', createdAt: '2024-03-01 00:23:41+00', }; @@ -709,6 +802,7 @@ describe('EnforcerDelegate', () => { const roleMetadataDao: RoleMetadataDao = { roleEntityRef: 'role:default/dev-team', source: 'rest', + author: modifiedBy, modifiedBy: 'user:default/system-admin', }; @@ -740,7 +834,62 @@ describe('EnforcerDelegate', () => { expect(metadata.source).toEqual('rest'); }); - it('should update grouping policies: one policy should be removed', async () => { + it('should update grouping policies: one policy should be removed for updateGroupingPolicies', async () => { + roleMetadataStorageMock.findRoleMetadata = jest + .fn() + .mockImplementation(async (): Promise => { + return { + source: 'rest', + roleEntityRef: 'role:default/dev-team', + author: modifiedBy, + modifiedBy, + description: 'Role for dev engineers', + createdAt: '2024-03-01 00:23:41+00', + }; + }); + policyMetadataStorageMock.findPolicyMetadata = jest + .fn() + .mockImplementation(async (): Promise => { + return { source: 'rest' }; + }); + + const enfDelegate = await createEnfDelegate( + [], + [groupingPolicy, secondGroupingPolicy], + ); + + const roleMetadataDao: RoleMetadataDao = { + roleEntityRef: 'role:default/dev-team', + source: 'rest', + author: modifiedBy, + modifiedBy: 'user:default/system-admin', + }; + await enfDelegate.updateGroupingPolicies( + [groupingPolicy, secondGroupingPolicy], + [groupingPolicy], + roleMetadataDao, + ); + + const storedPolicies = await enfDelegate.getGroupingPolicy(); + expect(storedPolicies.length).toEqual(1); + expect( + policyMetadataStorageMock.createPolicyMetadata, + ).toHaveBeenCalledWith('rest', groupingPolicy, expect.anything()); + + const metadata = (roleMetadataStorageMock.updateRoleMetadata as jest.Mock) + .mock.calls[0][0]; + + const createdAtData = new Date(`${metadata.createdAt}`); + const lastModified = new Date(`${metadata.lastModified}`); + expect(lastModified > createdAtData).toBeTruthy(); + expect(metadata.author).toEqual(modifiedBy); + expect(metadata.description).toEqual('Role for dev engineers'); + expect(metadata.modifiedBy).toEqual('user:default/system-admin'); + expect(metadata.roleEntityRef).toEqual('role:default/dev-team'); + expect(metadata.source).toEqual('rest'); + }); + + it('should update grouping policies: one policy should be removed and description updated', async () => { roleMetadataStorageMock.findRoleMetadata = jest .fn() .mockImplementation(async (): Promise => { @@ -748,6 +897,7 @@ describe('EnforcerDelegate', () => { source: 'rest', roleEntityRef: 'role:default/dev-team', author: 'user:default/some-user', + modifiedBy: 'user:default/some-user', description: 'Role for dev engineers', createdAt: '2024-03-01 00:23:41+00', }; @@ -766,7 +916,9 @@ describe('EnforcerDelegate', () => { const roleMetadataDao: RoleMetadataDao = { roleEntityRef: 'role:default/dev-team', source: 'rest', + author: modifiedBy, modifiedBy: 'user:default/system-admin', + description: 'updated description', }; await enfDelegate.updateGroupingPolicies( [groupingPolicy, secondGroupingPolicy], @@ -787,7 +939,7 @@ describe('EnforcerDelegate', () => { const lastModified = new Date(`${metadata.lastModified}`); expect(lastModified > createdAtData).toBeTruthy(); expect(metadata.author).toEqual('user:default/some-user'); - expect(metadata.description).toEqual('Role for dev engineers'); + expect(metadata.description).toEqual('updated description'); expect(metadata.modifiedBy).toEqual('user:default/system-admin'); expect(metadata.roleEntityRef).toEqual('role:default/dev-team'); expect(metadata.source).toEqual('rest'); @@ -801,7 +953,8 @@ describe('EnforcerDelegate', () => { return { source: 'rest', roleEntityRef: oldRoleName, - author: 'user:default/some-user', + author: modifiedBy, + modifiedBy, description: 'Role for dev engineers', createdAt: '2024-03-01 00:23:41+00', }; @@ -812,25 +965,31 @@ describe('EnforcerDelegate', () => { return { source: 'rest' }; }); + const enfDelegate = await createEnfDelegate( + [], + [ + ['user:default/tom', 'role:default/dev-team'], + ['user:default/tim', 'role:default/dev-team'], + ], + ); + const newRoleName = 'role:default/new-team-name'; - const groupingPolicyWithRenamedRole = [groupingPolicy[0], newRoleName]; + const groupingPolicyWithRenamedRole = ['user:default/tom', newRoleName]; const secondGroupingPolicyWithRenamedRole = [ - secondGroupingPolicy[1], + 'user:default/tim', newRoleName, ]; - const enfDelegate = await createEnfDelegate( - [], - [groupingPolicy, secondGroupingPolicy], - ); - const roleMetadataDao: RoleMetadataDao = { roleEntityRef: newRoleName, source: 'rest', - modifiedBy: 'user:default/system-admin', + modifiedBy, }; await enfDelegate.updateGroupingPolicies( - [groupingPolicy, secondGroupingPolicy], + [ + ['user:default/tom', 'role:default/dev-team'], + ['user:default/tim', 'role:default/dev-team'], + ], [groupingPolicyWithRenamedRole, secondGroupingPolicyWithRenamedRole], roleMetadataDao, ); @@ -860,9 +1019,9 @@ describe('EnforcerDelegate', () => { const createdAtData = new Date(`${metadata.createdAt}`); const lastModified = new Date(`${metadata.lastModified}`); expect(lastModified > createdAtData).toBeTruthy(); - expect(metadata.author).toEqual('user:default/some-user'); + expect(metadata.author).toEqual(modifiedBy); expect(metadata.description).toEqual('Role for dev engineers'); - expect(metadata.modifiedBy).toEqual('user:default/system-admin'); + expect(metadata.modifiedBy).toEqual(modifiedBy); expect(metadata.roleEntityRef).toEqual(newRoleName); expect(metadata.source).toEqual('rest'); }); @@ -874,9 +1033,10 @@ describe('EnforcerDelegate', () => { return { source: 'legacy', roleEntityRef: 'role:default/dev-team', - author: 'user:default/some-user', + author: modifiedBy, description: 'Role for dev engineers', createdAt: '2024-03-01 00:23:41+00', + modifiedBy, }; }); policyMetadataStorageMock.findPolicyMetadata = jest @@ -890,7 +1050,7 @@ describe('EnforcerDelegate', () => { const roleMetadataDao: RoleMetadataDao = { roleEntityRef: 'role:default/dev-team', source: 'rest', - modifiedBy: 'user:default/system-admin', + modifiedBy, description: 'some-new-description', }; await enfDelegate.updateGroupingPolicies( @@ -909,9 +1069,9 @@ describe('EnforcerDelegate', () => { const createdAtData = new Date(`${metadata.createdAt}`); const lastModified = new Date(`${metadata.lastModified}`); expect(lastModified > createdAtData).toBeTruthy(); - expect(metadata.author).toEqual('user:default/some-user'); + expect(metadata.author).toEqual(modifiedBy); expect(metadata.description).toEqual('some-new-description'); - expect(metadata.modifiedBy).toEqual('user:default/system-admin'); + expect(metadata.modifiedBy).toEqual(modifiedBy); expect(metadata.roleEntityRef).toEqual('role:default/dev-team'); expect(metadata.source).toEqual('rest'); }); @@ -1077,27 +1237,31 @@ describe('EnforcerDelegate', () => { 'role:default/team-dev', ]; - it('should remove grouping policy and remove role metadata', async () => { - policyMetadataStorageMock.findPolicyMetadata = jest + beforeEach(() => { + roleMetadataStorageMock.findRoleMetadata = jest .fn() .mockImplementation(() => { return { source: 'rest', + roleEntityRef: 'role:default/team-dev', + createdAt: '2024-03-01 00:23:41+00', }; }); - roleMetadataStorageMock.findRoleMetadata = jest + }); + + it('should remove grouping policy and remove role metadata', async () => { + policyMetadataStorageMock.findPolicyMetadata = jest .fn() .mockImplementation(() => { return { source: 'rest', - roleEntityRef: 'role:default/team-dev', }; }); const enfDelegate = await createEnfDelegate([], [groupingPolicyToDelete]); await enfDelegate.removeGroupingPolicy( groupingPolicyToDelete, - { source: 'rest', roleEntityRef: 'role:default/team-dev' }, + { source: 'rest', roleEntityRef: 'role:default/team-dev', modifiedBy }, false, ); @@ -1128,15 +1292,6 @@ describe('EnforcerDelegate', () => { source: 'rest', }; }); - roleMetadataStorageMock.findRoleMetadata = jest - .fn() - .mockImplementation(() => { - return { - source: 'rest', - roleEntityRef: 'role:default/team-dev', - createdAt: '2024-03-01 00:23:41+00', - }; - }); enfFilterGroupingPolicySpy.mockReset(); const enfDelegate = await createEnfDelegate( @@ -1148,7 +1303,7 @@ describe('EnforcerDelegate', () => { ); await enfDelegate.removeGroupingPolicy( groupingPolicyToDelete, - { source: 'rest', roleEntityRef: 'role:default/team-dev' }, + { source: 'rest', roleEntityRef: 'role:default/team-dev', modifiedBy }, false, ); @@ -1184,20 +1339,16 @@ describe('EnforcerDelegate', () => { source: 'rest', }; }); - roleMetadataStorageMock.findRoleMetadata = jest - .fn() - .mockImplementation(() => { - return { - source: 'rest', - roleEntityRef: 'role:default/team-dev', - }; - }); enfFilterGroupingPolicySpy.mockReset(); const enfDelegate = await createEnfDelegate([], [groupingPolicyToDelete]); await enfDelegate.removeGroupingPolicy( groupingPolicyToDelete, - { source: 'rest', roleEntityRef: 'role:default/team-dev' }, + { + source: 'rest', + roleEntityRef: 'role:default/dev-team', + modifiedBy: 'user:default/some-user', + }, true, ); @@ -1226,7 +1377,11 @@ describe('EnforcerDelegate', () => { await expect( enfDelegate.removeGroupingPolicy( groupingPolicyToDelete, - { source: 'rest', roleEntityRef: 'role:default/team-dev' }, + { + source: 'rest', + roleEntityRef: 'role:default/team-dev', + modifiedBy: 'user:default/some-user', + }, false, ), ).rejects.toThrow( @@ -1265,7 +1420,11 @@ describe('EnforcerDelegate', () => { const enfDelegate = await createEnfDelegate([], groupingPoliciesToDelete); await enfDelegate.removeGroupingPolicies( groupingPoliciesToDelete, - 'user:default/test-user', + { + roleEntityRef: 'role:default/team-dev', + source: 'rest', + modifiedBy, + }, false, ); @@ -1299,7 +1458,7 @@ describe('EnforcerDelegate', () => { ); }); - it('should remove grouping policy and update role metadata', async () => { + it('should remove grouping policies and update role metadata', async () => { policyMetadataStorageMock.findPolicyMetadata = jest .fn() .mockImplementation(() => { @@ -1329,7 +1488,11 @@ describe('EnforcerDelegate', () => { ); await enfDelegate.removeGroupingPolicies( groupingPoliciesToDelete, - 'user:default/test-user', + { + roleEntityRef: 'role:default/team-dev', + source: 'rest', + modifiedBy, + }, false, ); @@ -1390,7 +1553,11 @@ describe('EnforcerDelegate', () => { const enfDelegate = await createEnfDelegate([], groupingPoliciesToDelete); await enfDelegate.removeGroupingPolicies( groupingPoliciesToDelete, - 'user:default/test-user', + { + roleEntityRef: 'role:default/team-dev', + source: 'rest', + modifiedBy: 'user:default/test-user', + }, true, ); @@ -1430,7 +1597,11 @@ describe('EnforcerDelegate', () => { await expect( enfDelegate.removeGroupingPolicies( groupingPoliciesToDelete, - 'user:default/test-user', + { + roleEntityRef: 'role:default/team-dev', + source: 'rest', + modifiedBy: 'user:default/test-user', + }, false, ), ).rejects.toThrow( @@ -1479,7 +1650,7 @@ describe('EnforcerDelegate', () => { }); describe('addOrUpdateGroupingPolicy', () => { - it('should add grouping policy', async () => { + it('should add grouping policy and create role metadata for method addOrUpdateGroupingPolicy', async () => { (roleMetadataStorageMock.findRoleMetadata as jest.Mock).mockReturnValue( Promise.resolve(undefined), ); @@ -1490,6 +1661,7 @@ describe('EnforcerDelegate', () => { await enfDelegate.addOrUpdateGroupingPolicy(groupingPolicy, { source: 'rest', roleEntityRef: 'role:default/dev-team', + modifiedBy, }); expect(enfUpdateGroupingPolicySpy).toHaveBeenCalledWith( @@ -1514,7 +1686,49 @@ describe('EnforcerDelegate', () => { expect(metadata.roleEntityRef).toEqual('role:default/dev-team'); }); - it('should update grouping policy', async () => { + it('should add grouping policy and update role metadata', async () => { + roleMetadataStorageMock.findRoleMetadata = jest + .fn() + .mockImplementation(() => { + return { + source: 'rest', + roleEntityRef: 'role:default/dev-team', + createdAt: '2024-03-01 00:23:41+00', + }; + }); + enfUpdateGroupingPolicySpy.mockClear(); + + const enfDelegate = await createEnfDelegate([], [groupingPolicy]); + + await enfDelegate.addOrUpdateGroupingPolicy(secondGroupingPolicy, { + source: 'rest', + roleEntityRef: 'role:default/dev-team', + modifiedBy, + }); + + expect(enfUpdateGroupingPolicySpy).toHaveBeenCalledWith( + ...secondGroupingPolicy, + ); + expect( + policyMetadataStorageMock.createPolicyMetadata, + ).toHaveBeenCalledWith('rest', secondGroupingPolicy, expect.anything()); + expect(roleMetadataStorageMock.updateRoleMetadata).toHaveBeenCalled(); + expect( + (roleMetadataStorageMock.updateRoleMetadata as jest.Mock).mock.calls + .length, + ).toEqual(1); + const metadata: RoleMetadataDao = ( + roleMetadataStorageMock.updateRoleMetadata as jest.Mock + ).mock.calls[0][0]; + const createdAtData = new Date(`${metadata.createdAt}`); + const lastModified = new Date(`${metadata.lastModified}`); + expect(lastModified > createdAtData).toBeTruthy(); + + expect(metadata.source).toEqual('rest'); + expect(metadata.roleEntityRef).toEqual('role:default/dev-team'); + }); + + it('should update grouping policy with legacy value', async () => { ( policyMetadataStorageMock.findPolicyMetadata as jest.Mock ).mockReturnValue({ @@ -1535,6 +1749,7 @@ describe('EnforcerDelegate', () => { await enfDelegate.addOrUpdateGroupingPolicy(groupingPolicy, { source: 'rest', roleEntityRef: 'role:default/dev-team', + modifiedBy, }); const metadata: RoleMetadataDao = ( diff --git a/plugins/rbac-backend/src/service/enforcer-delegate.ts b/plugins/rbac-backend/src/service/enforcer-delegate.ts index f05ce551c6..c843af9020 100644 --- a/plugins/rbac-backend/src/service/enforcer-delegate.ts +++ b/plugins/rbac-backend/src/service/enforcer-delegate.ts @@ -228,10 +228,10 @@ export class EnforcerDelegate { oldRole: string[][], newRole: string[][], newRoleMetadata: RoleMetadataDao, - externalTrx?: Knex.Transaction, ): Promise { - const trx = externalTrx ?? (await this.knex.transaction()); const oldRoleName = oldRole.at(0)?.at(1)!; + + const trx = await this.knex.transaction(); try { const currentMetadata = await this.roleMetadataStorage.findRoleMetadata( oldRoleName, @@ -241,20 +241,11 @@ export class EnforcerDelegate { throw new Error(`Role metadata ${oldRoleName} was not found`); } - await this.removeGroupingPolicies( - oldRole, - newRoleMetadata.modifiedBy, - true, - trx, - ); + await this.removeGroupingPolicies(oldRole, currentMetadata, true, trx); await this.addGroupingPolicies(newRole, newRoleMetadata, trx); - if (!externalTrx) { - await trx.commit(); - } + await trx.commit(); } catch (err) { - if (!externalTrx) { - await trx.rollback(err); - } + await trx.rollback(err); throw err; } } @@ -263,20 +254,15 @@ export class EnforcerDelegate { oldPolicies: string[][], newPolicies: string[][], source: Source, - externalTrx?: Knex.Transaction, ): Promise { - const trx = externalTrx ?? (await this.knex.transaction()); + const trx = await this.knex.transaction(); try { await this.removePolicies(oldPolicies, trx); await this.addPolicies(newPolicies, source, trx); - if (!externalTrx) { - await trx.commit(); - } + await trx.commit(); } catch (err) { - if (!externalTrx) { - await trx.rollback(err); - } + await trx.rollback(err); throw err; } } @@ -376,13 +362,11 @@ export class EnforcerDelegate { if (!isUpdate) { const currentRoleMetadata = await this.roleMetadataStorage.findRoleMetadata(roleEntity, trx); - const groupPolicies = await this.enforcer.getFilteredGroupingPolicy( - 1, - roleEntity, - ); + const remainingGroupPolicies = + await this.enforcer.getFilteredGroupingPolicy(1, roleEntity); if ( currentRoleMetadata && - groupPolicies.length === 0 && + remainingGroupPolicies.length === 0 && roleEntity !== ADMIN_ROLE_NAME ) { await this.roleMetadataStorage.removeRoleMetadata(roleEntity, trx); @@ -408,12 +392,13 @@ export class EnforcerDelegate { async removeGroupingPolicies( policies: string[][], - modifiedBy?: string, + roleMetadata: RoleMetadataDao, isUpdate?: boolean, externalTrx?: Knex.Transaction, ): Promise { const trx = externalTrx ?? (await this.knex.transaction()); - const roles = new Set(); + + const roleEntity = roleMetadata.roleEntityRef; try { for (const policy of policies) { const metadata = await this.policyMetadataStorage.findPolicyMetadata( @@ -426,7 +411,6 @@ export class EnforcerDelegate { ); } await this.policyMetadataStorage.removePolicyMetadata(policy, trx); - roles.add(policy[1]); } const ok = await this.enforcer.removeGroupingPolicies(policies); @@ -436,31 +420,23 @@ export class EnforcerDelegate { ); } - for (const roleEntity of roles) { - if (!isUpdate) { - const roleMetadata = await this.roleMetadataStorage.findRoleMetadata( + if (!isUpdate) { + const currentRoleMetadata = + await this.roleMetadataStorage.findRoleMetadata(roleEntity, trx); + const remainingGroupPolicies = + await this.enforcer.getFilteredGroupingPolicy(1, roleEntity); + if ( + currentRoleMetadata && + remainingGroupPolicies.length === 0 && + roleEntity !== ADMIN_ROLE_NAME + ) { + await this.roleMetadataStorage.removeRoleMetadata(roleEntity, trx); + } else if (currentRoleMetadata) { + await this.roleMetadataStorage.updateRoleMetadata( + this.mergeMetadata(currentRoleMetadata, roleMetadata), roleEntity, trx, ); - const groupPolicies = await this.enforcer.getFilteredGroupingPolicy( - 1, - roleEntity, - ); - if ( - roleMetadata && - groupPolicies.length === 0 && - roleEntity !== ADMIN_ROLE_NAME - ) { - await this.roleMetadataStorage.removeRoleMetadata(roleEntity, trx); - } else if (roleMetadata) { - roleMetadata.modifiedBy = modifiedBy; - roleMetadata.lastModified = new Date().toUTCString(); - await this.roleMetadataStorage.updateRoleMetadata( - roleMetadata, - roleEntity, - trx, - ); - } } } @@ -475,12 +451,8 @@ export class EnforcerDelegate { } } - async addOrUpdatePolicy( - policy: string[], - source: Source, - externalTrx?: Knex.Transaction, - ): Promise { - const trx = externalTrx ?? (await this.knex.transaction()); + async addOrUpdatePolicy(policy: string[], source: Source): Promise { + const trx = await this.knex.transaction(); try { if (!(await this.enforcer.hasPolicy(...policy))) { await this.addPolicy(policy, source, trx); @@ -488,13 +460,9 @@ export class EnforcerDelegate { await this.removePolicy(policy, trx); await this.addPolicy(policy, source, trx); } - if (!externalTrx) { - await trx.commit(); - } + await trx.commit(); } catch (err) { - if (!externalTrx) { - await trx.rollback(err); - } + await trx.rollback(err); throw err; } } @@ -502,9 +470,8 @@ export class EnforcerDelegate { async addOrUpdateGroupingPolicy( groupPolicy: string[], roleMetadata: RoleMetadataDao, - externalTrx?: Knex.Transaction, ): Promise { - const trx = externalTrx ?? (await this.knex.transaction()); + const trx = await this.knex.transaction(); try { if (!(await this.hasGroupingPolicy(...groupPolicy))) { await this.addGroupingPolicy(groupPolicy, roleMetadata, trx); @@ -514,13 +481,9 @@ export class EnforcerDelegate { await this.removeGroupingPolicy(groupPolicy, roleMetadata, true, trx); await this.addGroupingPolicy(groupPolicy, roleMetadata, trx); } - if (!externalTrx) { - await trx.commit(); - } + await trx.commit(); } catch (err) { - if (!externalTrx) { - await trx.rollback(err); - } + await trx.rollback(err); throw err; } } @@ -528,9 +491,8 @@ export class EnforcerDelegate { async addOrUpdateGroupingPolicies( groupPolicies: string[][], roleMetadata: RoleMetadataDao, - externalTrx?: Knex.Transaction, ): Promise { - const trx = externalTrx ?? (await this.knex.transaction()); + const trx = await this.knex.transaction(); try { for (const groupPolicy of groupPolicies) { if (!(await this.hasGroupingPolicy(...groupPolicy))) { @@ -542,13 +504,9 @@ export class EnforcerDelegate { await this.addGroupingPolicy(groupPolicy, roleMetadata, trx); } } - if (!externalTrx) { - await trx.commit(); - } + await trx.commit(); } catch (err) { - if (!externalTrx) { - await trx.rollback(err); - } + await trx.rollback(err); throw err; } } diff --git a/plugins/rbac-backend/src/service/permission-policy.test.ts b/plugins/rbac-backend/src/service/permission-policy.test.ts index 43bd023781..36621ba1f3 100644 --- a/plugins/rbac-backend/src/service/permission-policy.test.ts +++ b/plugins/rbac-backend/src/service/permission-policy.test.ts @@ -124,6 +124,14 @@ const knex = Knex.knex({ client: MockClient }); const mockAuthService = mockServices.auth(); +const auditLoggerMock = { + getActorId: jest.fn().mockImplementation(), + createAuditLogDetails: jest.fn().mockImplementation(), + auditLog: jest.fn().mockImplementation(), +}; + +const modifiedBy = 'user:default/some-admin'; + describe('RBACPermissionPolicy Tests', () => { beforeEach(() => { roleMetadataStorageMock.updateRoleMetadata = jest.fn().mockImplementation(); @@ -181,6 +189,13 @@ describe('RBACPermissionPolicy Tests', () => { newIdentityResponse('user:default/guest'), ); expect(decision.result).toBe(AuthorizeResult.ALLOW); + verifyAuditLogForResourcedPermission( + 'user:default/guest', + 'catalog.entity.read', + 'read', + 'catalog-entity', + AuthorizeResult.ALLOW, + ); }); // case2 @@ -190,6 +205,13 @@ describe('RBACPermissionPolicy Tests', () => { newIdentityResponse('user:default/guest'), ); expect(decision.result).toBe(AuthorizeResult.ALLOW); + verifyAuditLogForNonResourcedPermission( + 'user:default/guest', + 'catalog.entity.create', + undefined, + 'use', + AuthorizeResult.ALLOW, + ); }); // case3 @@ -199,6 +221,13 @@ describe('RBACPermissionPolicy Tests', () => { newIdentityResponse('user:default/known_user'), ); expect(decision.result).toBe(AuthorizeResult.ALLOW); + verifyAuditLogForNonResourcedPermission( + 'user:default/known_user', + 'test.resource.deny', + undefined, + 'use', + AuthorizeResult.ALLOW, + ); }); // case1 with role @@ -212,6 +241,13 @@ describe('RBACPermissionPolicy Tests', () => { newIdentityResponse('user:default/guest'), ); expect(decision.result).toBe(AuthorizeResult.ALLOW); + verifyAuditLogForResourcedPermission( + 'user:default/guest', + 'catalog.entity.read', + 'update', + 'catalog-entity', + AuthorizeResult.ALLOW, + ); }); // case2 with role @@ -225,6 +261,13 @@ describe('RBACPermissionPolicy Tests', () => { newIdentityResponse('role:default/catalog-writer'), ); expect(decision.result).toBe(AuthorizeResult.ALLOW); + verifyAuditLogForResourcedPermission( + 'role:default/catalog-writer', + 'catalog.entity.read', + 'update', + 'catalog-entity', + AuthorizeResult.ALLOW, + ); }); }); @@ -441,6 +484,13 @@ describe('RBACPermissionPolicy Tests', () => { newIdentityResponse('user:default/user-old-1'), ); expect(decision.result).toBe(AuthorizeResult.DENY); + verifyAuditLogForNonResourcedPermission( + 'user:default/user-old-1', + 'test.some.resource', + undefined, + 'use', + AuthorizeResult.DENY, + ); }); it('should cleanup old policies and group policies and metadata after re-attach policy file', async () => { @@ -903,6 +953,13 @@ describe('RBACPermissionPolicy Tests', () => { newIdentityResponse('user:default/known_user'), ); expect(decision.result).toBe(AuthorizeResult.DENY); + verifyAuditLogForNonResourcedPermission( + 'user:default/known_user', + 'test.resource.deny', + undefined, + 'use', + AuthorizeResult.DENY, + ); }); // case2 it('should deny access to basic permission for unlisted user', async () => { @@ -911,6 +968,13 @@ describe('RBACPermissionPolicy Tests', () => { newIdentityResponse('unuser:default/known_user'), ); expect(decision.result).toBe(AuthorizeResult.DENY); + verifyAuditLogForNonResourcedPermission( + 'unuser:default/known_user', + 'test.resource', + undefined, + 'use', + AuthorizeResult.DENY, + ); }); // case3 it('should deny access to basic permission for listed user deny and allow', async () => { @@ -919,6 +983,13 @@ describe('RBACPermissionPolicy Tests', () => { newIdentityResponse('user:default/duplicated'), ); expect(decision.result).toBe(AuthorizeResult.DENY); + verifyAuditLogForNonResourcedPermission( + 'user:default/duplicated', + 'test.resource', + undefined, + 'use', + AuthorizeResult.DENY, + ); }); // case4 it('should allow access to basic permission for user listed on policy', async () => { @@ -927,6 +998,13 @@ describe('RBACPermissionPolicy Tests', () => { newIdentityResponse('user:default/known_user'), ); expect(decision.result).toBe(AuthorizeResult.ALLOW); + verifyAuditLogForNonResourcedPermission( + 'user:default/known_user', + 'test.resource', + undefined, + 'use', + AuthorizeResult.ALLOW, + ); }); // case5 it('should deny access to undefined user', async () => { @@ -935,6 +1013,13 @@ describe('RBACPermissionPolicy Tests', () => { newIdentityResponse(), ); expect(decision.result).toBe(AuthorizeResult.DENY); + verifyAuditLogForNonResourcedPermission( + undefined, + 'test.resource', + undefined, + 'use', + AuthorizeResult.DENY, + ); }); // Tests for Resource Permission type @@ -950,6 +1035,13 @@ describe('RBACPermissionPolicy Tests', () => { newIdentityResponse('user:default/known_user'), ); expect(decision.result).toBe(AuthorizeResult.DENY); + verifyAuditLogForResourcedPermission( + 'user:default/known_user', + 'test.resource.deny', + 'update', + 'test-resource-deny', + AuthorizeResult.DENY, + ); }); // case 2 it('should deny access to resource permission for user unlisted on policy', async () => { @@ -962,6 +1054,13 @@ describe('RBACPermissionPolicy Tests', () => { newIdentityResponse('unuser:default/known_user'), ); expect(decision.result).toBe(AuthorizeResult.DENY); + verifyAuditLogForResourcedPermission( + 'unuser:default/known_user', + 'test.resource.update', + 'update', + 'test-resource', + AuthorizeResult.DENY, + ); }); // case 3 it('should deny access to resource permission for user listed deny and allow', async () => { @@ -974,6 +1073,13 @@ describe('RBACPermissionPolicy Tests', () => { newIdentityResponse('user:default/duplicated'), ); expect(decision.result).toBe(AuthorizeResult.DENY); + verifyAuditLogForResourcedPermission( + 'user:default/duplicated', + 'test.resource.update', + 'update', + 'test-resource', + AuthorizeResult.DENY, + ); }); // case 4 it('should allow access to resource permission for user listed on policy', async () => { @@ -986,6 +1092,13 @@ describe('RBACPermissionPolicy Tests', () => { newIdentityResponse('user:default/known_user'), ); expect(decision.result).toBe(AuthorizeResult.ALLOW); + verifyAuditLogForResourcedPermission( + 'user:default/known_user', + 'test.resource.update', + 'update', + 'test-resource', + AuthorizeResult.ALLOW, + ); }); // Tests for actions on resource permissions @@ -1044,6 +1157,14 @@ describe('RBACPermissionPolicy Tests', () => { newIdentityResponse('user:default/guest'), ); expect(decision.result).toBe(AuthorizeResult.ALLOW); + verifyAuditLogForResourcedPermission( + 'user:default/guest', + perm.name, + perm.action, + perm.resource, + AuthorizeResult.ALLOW, + ); + auditLoggerMock.auditLog.mockReset(); } }); }); @@ -1062,6 +1183,7 @@ describe('RBACPermissionPolicy Tests', () => { return { roleEntityRef: 'role:default/catalog-writer', source: 'legacy', + modifiedBy, }; }, ), @@ -1147,6 +1269,7 @@ describe('RBACPermissionPolicy Tests', () => { return { roleEntityRef: 'role:default/catalog-writer', source: 'legacy', + modifiedBy, }; }, ); @@ -1159,6 +1282,7 @@ describe('RBACPermissionPolicy Tests', () => { await enfDelegate.addGroupingPolicy(oldGroupPolicy, { source: 'configuration', roleEntityRef: ADMIN_ROLE_NAME, + modifiedBy: `user:default/tom`, }); policy = await newPermissionPolicy( @@ -1238,6 +1362,13 @@ describe('RBACPermissionPolicy Tests', () => { newIdentityResponse('user:default/test_admin'), ); expect(decision.result).toBe(AuthorizeResult.ALLOW); + verifyAuditLogForResourcedPermission( + 'user:default/test_admin', + 'policy.entity.read', + 'read', + 'policy-entity', + AuthorizeResult.ALLOW, + ); }); it('should allow read access to resource permission for super user from config file', async () => { @@ -1250,6 +1381,15 @@ describe('RBACPermissionPolicy Tests', () => { newIdentityResponse('user:default/super_user'), ); expect(decision.result).toBe(AuthorizeResult.ALLOW); + verifyAuditLogForResourcedPermission( + 'user:default/super_user', + 'policy.entity.read', + 'read', + 'policy-entity', + AuthorizeResult.ALLOW, + ); + auditLoggerMock.auditLog.mockReset(); + const decision2 = await policy.handle( newPolicyQueryWithResourcePermission( 'catalog.entity.delete', @@ -1259,6 +1399,13 @@ describe('RBACPermissionPolicy Tests', () => { newIdentityResponse('user:default/super_user'), ); expect(decision2.result).toBe(AuthorizeResult.ALLOW); + verifyAuditLogForResourcedPermission( + 'user:default/super_user', + 'catalog.entity.delete', + 'delete', + 'catalog-entity', + AuthorizeResult.ALLOW, + ); }); it('should remove users that are no longer in the config file', async () => { @@ -1284,6 +1431,7 @@ describe('Policy checks for resourced permissions defined by name', () => { return { roleEntityRef: 'role:default/catalog-writer', source: 'legacy', + modifiedBy, }; }, ), @@ -1310,7 +1458,11 @@ describe('Policy checks for resourced permissions defined by name', () => { await enfDelegate.addGroupingPolicy( ['user:default/tor', 'role:default/catalog_reader'], - { source: 'csv-file', roleEntityRef: 'role:default/catalog_reader' }, + { + source: 'csv-file', + roleEntityRef: 'role:default/catalog_reader', + modifiedBy, + }, ); await enfDelegate.addPolicy( ['role:default/catalog_reader', 'catalog.entity.read', 'read', 'allow'], @@ -1333,7 +1485,11 @@ describe('Policy checks for resourced permissions defined by name', () => { await enfDelegate.addGroupingPolicy( ['user:default/tor', 'role:default/catalog_reader'], - { source: 'csv-file', roleEntityRef: 'role:default/catalog_reader' }, + { + source: 'csv-file', + roleEntityRef: 'role:default/catalog_reader', + modifiedBy, + }, ); await enfDelegate.addPolicies( [ @@ -1359,8 +1515,13 @@ describe('Policy checks for resourced permissions defined by name', () => { await enfDelegate.addGroupingPolicy( ['user:default/tor', 'role:default/catalog_reader'], - { source: 'csv-file', roleEntityRef: 'role:default/catalog_reader' }, + { + source: 'csv-file', + roleEntityRef: 'role:default/catalog_reader', + modifiedBy, + }, ); + await enfDelegate.addPolicies( [ ['role:default/catalog_reader', 'catalog.entity.read', 'read', 'deny'], @@ -1378,6 +1539,13 @@ describe('Policy checks for resourced permissions defined by name', () => { newIdentityResponse('user:default/tor'), ); expect(decision.result).toBe(AuthorizeResult.DENY); + verifyAuditLogForNonResourcedPermission( + 'user:default/tor', + 'catalog.entity.read', + 'catalog-entity', + 'read', + AuthorizeResult.DENY, + ); }); it('should allow access to resourced permission assigned by name, but user inherits policy from his group', async () => { @@ -1398,8 +1566,13 @@ describe('Policy checks for resourced permissions defined by name', () => { await enfDelegate.addGroupingPolicy( ['group:default/team-a', 'role:default/catalog_user'], - { source: 'csv-file', roleEntityRef: 'role:default/catalog_user' }, + { + source: 'csv-file', + roleEntityRef: 'role:default/catalog_user', + modifiedBy, + }, ); + await enfDelegate.addPolicies( [['role:default/catalog_user', 'catalog.entity.read', 'read', 'allow']], 'csv-file', @@ -1414,6 +1587,13 @@ describe('Policy checks for resourced permissions defined by name', () => { newIdentityResponse('user:default/tor'), ); expect(decision.result).toBe(AuthorizeResult.ALLOW); + verifyAuditLogForNonResourcedPermission( + 'user:default/tor', + 'catalog.entity.read', + 'catalog-entity', + 'read', + AuthorizeResult.ALLOW, + ); }); it('should allow access to resourced permission assigned by name, but user inherits policy from few groups', async () => { @@ -1443,12 +1623,21 @@ describe('Policy checks for resourced permissions defined by name', () => { await enfDelegate.addGroupingPolicy( ['group:default/team-b', 'role:default/catalog_user'], - { source: 'csv-file', roleEntityRef: 'role:default/catalog_user' }, + { + source: 'csv-file', + roleEntityRef: 'role:default/catalog_user', + modifiedBy, + }, ); await enfDelegate.addGroupingPolicy( ['group:default/team-a', 'group:default/team-b'], - { source: 'csv-file', roleEntityRef: 'role:default/catalog_user' }, + { + source: 'csv-file', + roleEntityRef: 'role:default/catalog_user', + modifiedBy, + }, ); + await enfDelegate.addPolicies( [['role:default/catalog_user', 'catalog.entity.read', 'read', 'allow']], 'csv-file', @@ -1463,6 +1652,13 @@ describe('Policy checks for resourced permissions defined by name', () => { newIdentityResponse('user:default/tor'), ); expect(decision.result).toBe(AuthorizeResult.ALLOW); + verifyAuditLogForNonResourcedPermission( + 'user:default/tor', + 'catalog.entity.read', + 'catalog-entity', + 'read', + AuthorizeResult.ALLOW, + ); }); }); @@ -1522,6 +1718,13 @@ describe('Policy checks for users and groups', () => { newIdentityResponse('user:default/alice'), ); expect(decision.result).toBe(AuthorizeResult.DENY); + verifyAuditLogForNonResourcedPermission( + 'user:default/alice', + 'test.resource', + undefined, + 'use', + AuthorizeResult.DENY, + ); }); // case2 @@ -1543,6 +1746,13 @@ describe('Policy checks for users and groups', () => { newIdentityResponse('user:default/akira'), ); expect(decision.result).toBe(AuthorizeResult.DENY); + verifyAuditLogForNonResourcedPermission( + 'user:default/akira', + 'test.resource', + undefined, + 'use', + AuthorizeResult.DENY, + ); }); // case3 @@ -1564,6 +1774,13 @@ describe('Policy checks for users and groups', () => { newIdentityResponse('user:default/antey'), ); expect(decision.result).toBe(AuthorizeResult.DENY); + verifyAuditLogForNonResourcedPermission( + 'user:default/antey', + 'test.resource', + undefined, + 'use', + AuthorizeResult.DENY, + ); }); // case4 @@ -1583,6 +1800,13 @@ describe('Policy checks for users and groups', () => { newIdentityResponse('user:default/julia'), ); expect(decision.result).toBe(AuthorizeResult.ALLOW); + verifyAuditLogForNonResourcedPermission( + 'user:default/julia', + 'test.resource', + undefined, + 'use', + AuthorizeResult.ALLOW, + ); }); // case5 @@ -1604,6 +1828,13 @@ describe('Policy checks for users and groups', () => { newIdentityResponse('user:default/mike'), ); expect(decision.result).toBe(AuthorizeResult.ALLOW); + verifyAuditLogForNonResourcedPermission( + 'user:default/mike', + 'test.resource', + undefined, + 'use', + AuthorizeResult.ALLOW, + ); }); // case6 @@ -1625,6 +1856,13 @@ describe('Policy checks for users and groups', () => { newIdentityResponse('user:default/tom'), ); expect(decision.result).toBe(AuthorizeResult.DENY); + verifyAuditLogForNonResourcedPermission( + 'user:default/tom', + 'test.resource', + undefined, + 'use', + AuthorizeResult.DENY, + ); }); // inheritance case @@ -1660,6 +1898,13 @@ describe('Policy checks for users and groups', () => { newIdentityResponse('user:default/mike'), ); expect(decision.result).toBe(AuthorizeResult.ALLOW); + verifyAuditLogForNonResourcedPermission( + 'user:default/mike', + 'test.resource.2', + undefined, + 'use', + AuthorizeResult.ALLOW, + ); }); // Resource type permissions @@ -1688,6 +1933,13 @@ describe('Policy checks for users and groups', () => { newIdentityResponse('user:default/alice'), ); expect(decision.result).toBe(AuthorizeResult.DENY); + verifyAuditLogForNonResourcedPermission( + 'user:default/alice', + 'test.resource.read', + 'test-resource', + 'read', + AuthorizeResult.DENY, + ); }); // case2 @@ -1710,6 +1962,13 @@ describe('Policy checks for users and groups', () => { newIdentityResponse('user:default/akira'), ); expect(decision.result).toBe(AuthorizeResult.DENY); + verifyAuditLogForNonResourcedPermission( + 'user:default/akira', + 'test.resource.read', + 'test-resource', + 'read', + AuthorizeResult.DENY, + ); }); // case3 @@ -1732,6 +1991,13 @@ describe('Policy checks for users and groups', () => { newIdentityResponse('user:default/antey'), ); expect(decision.result).toBe(AuthorizeResult.DENY); + verifyAuditLogForNonResourcedPermission( + 'user:default/antey', + 'test.resource.read', + 'test-resource', + 'read', + AuthorizeResult.DENY, + ); }); // case4 @@ -1755,6 +2021,13 @@ describe('Policy checks for users and groups', () => { newIdentityResponse('user:default/julia'), ); expect(decision.result).toBe(AuthorizeResult.ALLOW); + verifyAuditLogForNonResourcedPermission( + 'user:default/julia', + 'test.resource.read', + 'test-resource', + 'read', + AuthorizeResult.ALLOW, + ); }); // case5 @@ -1780,6 +2053,13 @@ describe('Policy checks for users and groups', () => { newIdentityResponse('user:default/mike'), ); expect(decision.result).toBe(AuthorizeResult.ALLOW); + verifyAuditLogForNonResourcedPermission( + 'user:default/mike', + 'test.resource.read', + 'test-resource', + 'read', + AuthorizeResult.ALLOW, + ); }); // case6 @@ -1805,6 +2085,13 @@ describe('Policy checks for users and groups', () => { newIdentityResponse('user:default/tom'), ); expect(decision.result).toBe(AuthorizeResult.DENY); + verifyAuditLogForNonResourcedPermission( + 'user:default/tom', + 'test.resource.read', + 'test-resource', + 'read', + AuthorizeResult.DENY, + ); }); // inheritance case @@ -1844,6 +2131,13 @@ describe('Policy checks for users and groups', () => { newIdentityResponse('user:default/mike'), ); expect(decision.result).toBe(AuthorizeResult.ALLOW); + verifyAuditLogForNonResourcedPermission( + 'user:default/mike', + 'test.resource.create', + 'test-resource', + 'create', + AuthorizeResult.ALLOW, + ); }); }); @@ -1873,6 +2167,7 @@ describe('Policy checks for conditional policies', () => { policy = await RBACPermissionPolicy.build( logger, + auditLoggerMock, config, conditionalStorage, enfDelegate, @@ -2237,12 +2532,94 @@ async function newPermissionPolicy( roleMock?: RoleMetadataStorage, ): Promise { const logger = getVoidLogger(); - return await RBACPermissionPolicy.build( + const permissionPolicy = await RBACPermissionPolicy.build( logger, + auditLoggerMock, config, conditionalStorage, enfDelegate, roleMock || roleMetadataStorageMock, knex, ); + auditLoggerMock.auditLog.mockReset(); + return permissionPolicy; +} + +function verifyAuditLogForNonResourcedPermission( + user: string | undefined, + permissionName: string, + resourceType: string | undefined, + action: string, + result: AuthorizeResult, +) { + const expectedUser = user ?? 'user without entity'; + expect(auditLoggerMock.auditLog).toHaveBeenNthCalledWith(1, { + actorId: expectedUser, + eventName: 'PermissionEvaluationStarted', + message: `Policy check for ${expectedUser}`, + metadata: { + action, + permissionName, + resourceType, + userEntityRef: expectedUser, + }, + stage: 'evaluatePermissionAccess', + status: 'succeeded', + }); + + const message = resourceType + ? `${expectedUser ?? 'user without entity'} is ${result} for permission '${permissionName}', resource type '${resourceType}' and action '${action}'` + : `${expectedUser ?? 'user without entity'} is ${result} for permission '${permissionName}' and action '${action}'`; + expect(auditLoggerMock.auditLog).toHaveBeenNthCalledWith(2, { + actorId: expectedUser, + eventName: 'PermissionEvaluationCompleted', + message, + metadata: { + action, + decision: { result }, + permissionName, + resourceType, + userEntityRef: expectedUser ?? 'user without entity', + }, + stage: 'evaluatePermissionAccess', + status: 'succeeded', + }); +} + +function verifyAuditLogForResourcedPermission( + user: string, + permissionName: string, + action: string, + resourceType: string, + result: AuthorizeResult, +) { + expect(auditLoggerMock.auditLog).toHaveBeenNthCalledWith(1, { + actorId: user, + eventName: 'PermissionEvaluationStarted', + message: `Policy check for ${user}`, + metadata: { + action, + permissionName, + resourceType, + userEntityRef: user, + }, + stage: 'evaluatePermissionAccess', + status: 'succeeded', + }); + expect(auditLoggerMock.auditLog).toHaveBeenNthCalledWith(2, { + actorId: user, + eventName: 'PermissionEvaluationCompleted', + message: `${user} is ${result} for permission '${permissionName}', resource type '${resourceType}' and action '${action}'`, + metadata: { + action, + decision: { + result, + }, + permissionName, + resourceType, + userEntityRef: user, + }, + stage: 'evaluatePermissionAccess', + status: 'succeeded', + }); } diff --git a/plugins/rbac-backend/src/service/permission-policy.ts b/plugins/rbac-backend/src/service/permission-policy.ts index f216a2705b..fe73cbf43a 100644 --- a/plugins/rbac-backend/src/service/permission-policy.ts +++ b/plugins/rbac-backend/src/service/permission-policy.ts @@ -3,11 +3,14 @@ import { ConfigApi } from '@backstage/core-plugin-api'; import { BackstageIdentityResponse } from '@backstage/plugin-auth-node'; import { AuthorizeResult, + ConditionalPolicyDecision, isResourcePermission, + Permission, PermissionCondition, PermissionCriteria, PermissionRuleParams, PolicyDecision, + ResourcePermission, } from '@backstage/plugin-permission-common'; import { PermissionPolicy, @@ -17,11 +20,21 @@ import { import { Knex } from 'knex'; import { Logger } from 'winston'; +import { AuditLogger } from '@janus-idp/backstage-plugin-audit-log-node'; import { NonEmptyArray, - PermissionAction, + toPermissionAction, } from '@janus-idp/backstage-plugin-rbac-common'; +import { + createPermissionEvaluationOptions, + EVALUATE_PERMISSION_ACCESS_STAGE, + EvaluationEvents, + HANDLE_RBAC_DATA_STAGE, + PermissionEvents, + RBAC_BACKEND, + RoleEvents, +} from '../audit-log/audit-logger'; import { ConditionalStorage } from '../database/conditional-storage'; import { RoleMetadataDao, @@ -53,6 +66,7 @@ const getAdminRoleMetadata = (): RoleMetadataDao => { const useAdminsFromConfig = async ( admins: Config[], enf: EnforcerDelegate, + auditLogger: AuditLogger, roleMetadataStorage: RoleMetadataStorage, knex: Knex, ) => { @@ -87,11 +101,24 @@ const useAdminsFromConfig = async ( throw error; } + const addedRoleMembers = Array.from(addedGroupPolicies.entries()); await enf.addOrUpdateGroupingPolicies( - Array.from(addedGroupPolicies.entries()), + addedRoleMembers, getAdminRoleMetadata(), ); + await auditLogger.auditLog({ + actorId: RBAC_BACKEND, + message: `Created or updated role`, + eventName: RoleEvents.CREATE_OR_UPDATE_ROLE, + metadata: { + ...getAdminRoleMetadata(), + members: addedRoleMembers.map(gp => gp[0]), + }, + stage: HANDLE_RBAC_DATA_STAGE, + status: 'succeeded', + }); + const configPoliciesMetadata = await enf.getFilteredPolicyMetadata('configuration'); @@ -108,61 +135,80 @@ const useAdminsFromConfig = async ( 'configuration', ADMIN_ROLE_NAME, enf, + auditLogger, ADMIN_ROLE_AUTHOR, ); }; -const setAdminPermissions = async (enf: EnforcerDelegate) => { - const adminReadPermission = [ - ADMIN_ROLE_NAME, - 'policy-entity', - 'read', - 'allow', - ]; - await enf.addOrUpdatePolicy(adminReadPermission, 'configuration'); - - const adminCreatePermission = [ - ADMIN_ROLE_NAME, - 'policy-entity', - 'create', - 'allow', - ]; - await enf.addOrUpdatePolicy(adminCreatePermission, 'configuration'); - - const adminDeletePermission = [ - ADMIN_ROLE_NAME, - 'policy-entity', - 'delete', - 'allow', - ]; - await enf.addOrUpdatePolicy(adminDeletePermission, 'configuration'); - - const adminUpdatePermission = [ - ADMIN_ROLE_NAME, - 'policy-entity', - 'update', - 'allow', - ]; - await enf.addOrUpdatePolicy(adminUpdatePermission, 'configuration'); +const addAdminPermission = async ( + policy: string[], + enf: EnforcerDelegate, + auditLogger: AuditLogger, +) => { + await enf.addOrUpdatePolicy(policy, 'configuration'); + + await auditLogger.auditLog({ + actorId: RBAC_BACKEND, + message: `Created or updated policy`, + eventName: PermissionEvents.CREATE_OR_UPDATE_POLICY, + metadata: { policies: [policy], source: 'configuration' }, + stage: HANDLE_RBAC_DATA_STAGE, + status: 'succeeded', + }); +}; +const setAdminPermissions = async ( + enf: EnforcerDelegate, + auditLogger: AuditLogger, +) => { + await addAdminPermission( + [ADMIN_ROLE_NAME, 'policy-entity', 'read', 'allow'], + enf, + auditLogger, + ); + await addAdminPermission( + [ADMIN_ROLE_NAME, 'policy-entity', 'create', 'allow'], + enf, + auditLogger, + ); + await addAdminPermission( + [ADMIN_ROLE_NAME, 'policy-entity', 'delete', 'allow'], + enf, + auditLogger, + ); + await addAdminPermission( + [ADMIN_ROLE_NAME, 'policy-entity', 'update', 'allow'], + enf, + auditLogger, + ); // needed for rbac frontend. - const adminCatalogReadPermission = [ - ADMIN_ROLE_NAME, - 'catalog-entity', - 'read', - 'allow', - ]; - await enf.addOrUpdatePolicy(adminCatalogReadPermission, 'configuration'); + await addAdminPermission( + [ADMIN_ROLE_NAME, 'catalog-entity', 'read', 'allow'], + enf, + auditLogger, + ); }; +const evaluatePermMsg = ( + userEntityRef: string | undefined, + result: AuthorizeResult, + permission: Permission, +) => + `${userEntityRef} is ${result} for permission '${permission.name}'${ + isResourcePermission(permission) + ? `, resource type '${permission.resourceType}'` + : '' + } and action '${toPermissionAction(permission.attributes)}'`; + export class RBACPermissionPolicy implements PermissionPolicy { private readonly enforcer: EnforcerDelegate; - private readonly logger: Logger; + private readonly auditLogger: AuditLogger; private readonly conditionStorage: ConditionalStorage; private readonly superUserList?: string[]; public static async build( logger: Logger, + auditLogger: AuditLogger, configApi: ConfigApi, conditionalStorage: ConditionalStorage, enforcerDelegate: EnforcerDelegate, @@ -195,10 +241,11 @@ export class RBACPermissionPolicy implements PermissionPolicy { await useAdminsFromConfig( adminUsers || [], enforcerDelegate, + auditLogger, roleMetadataStorage, knex, ); - await setAdminPermissions(enforcerDelegate); + await setAdminPermissions(enforcerDelegate, auditLogger); if ( (!adminUsers || adminUsers.length === 0) && @@ -213,12 +260,13 @@ export class RBACPermissionPolicy implements PermissionPolicy { enforcerDelegate, logger, roleMetadataStorage, + auditLogger, ); await csvFile.initialize(policiesFile, allowReload); return new RBACPermissionPolicy( enforcerDelegate, - logger, + auditLogger, conditionalStorage, superUserList, ); @@ -226,12 +274,12 @@ export class RBACPermissionPolicy implements PermissionPolicy { private constructor( enforcer: EnforcerDelegate, - logger: Logger, + auditLogger: AuditLogger, conditionStorage: ConditionalStorage, superUserList?: string[], ) { this.enforcer = enforcer; - this.logger = logger; + this.auditLogger = auditLogger; this.conditionStorage = conditionStorage; this.superUserList = superUserList; } @@ -240,21 +288,36 @@ export class RBACPermissionPolicy implements PermissionPolicy { request: PolicyQuery, identityResp?: BackstageIdentityResponse | undefined, ): Promise { - this.logger.info( - `Policy check for ${identityResp?.identity.userEntityRef} for permission ${request.permission.name}`, + const userEntityRef = + identityResp?.identity.userEntityRef ?? `user without entity`; + + let auditOptions = createPermissionEvaluationOptions( + `Policy check for ${userEntityRef}`, + userEntityRef, + request, ); + this.auditLogger.auditLog(auditOptions); + try { let status = false; - // We are introducing an action named "use" when action does not exist to avoid - // a more complicated model with multiple policy and request shapes. - const action = request.permission.attributes.action ?? 'use'; - - if (!identityResp?.identity) { + const action = toPermissionAction(request.permission.attributes); + if (!identityResp) { + const msg = evaluatePermMsg( + userEntityRef, + AuthorizeResult.DENY, + request.permission, + ); + auditOptions = createPermissionEvaluationOptions( + msg, + userEntityRef, + request, + { result: AuthorizeResult.DENY }, + ); + await this.auditLogger.auditLog(auditOptions); return { result: AuthorizeResult.DENY }; } - const userEntityRef = identityResp.identity.userEntityRef; const permissionName = request.permission.name; const roles = await this.enforcer.getRolesForUser(userEntityRef); @@ -265,9 +328,7 @@ export class RBACPermissionPolicy implements PermissionPolicy { if (identityResp) { const conditionResult = await this.handleConditions( userEntityRef, - resourceType, - request.permission.name, - action, + request, roles, ); if (conditionResult) { @@ -297,25 +358,25 @@ export class RBACPermissionPolicy implements PermissionPolicy { } const result = status ? AuthorizeResult.ALLOW : AuthorizeResult.DENY; - this.logger.info( - `${userEntityRef} is ${result} for permission '${ - request.permission.name - }'${ - isResourcePermission(request.permission) - ? `, resource type '${request.permission.resourceType}'` - : '' - } and action ${action}`, + + const msg = evaluatePermMsg(userEntityRef, result, request.permission); + auditOptions = createPermissionEvaluationOptions( + msg, + userEntityRef, + request, + { result }, ); - return Promise.resolve({ - result: result, - }); + await this.auditLogger.auditLog(auditOptions); + return { result }; } catch (error) { - this.logger.error( - `Policy check failed with ${error} for permission ${request.permission.name}`, - ); - return Promise.resolve({ - result: AuthorizeResult.DENY, + await this.auditLogger.auditLog({ + message: 'Permission policy check failed', + eventName: EvaluationEvents.PERMISSION_EVALUATION_FAILED, + stage: EVALUATE_PERMISSION_ACCESS_STAGE, + status: 'failed', + errors: [error], }); + return { result: AuthorizeResult.DENY }; } } @@ -349,11 +410,14 @@ export class RBACPermissionPolicy implements PermissionPolicy { private async handleConditions( userEntityRef: string, - resourceType: string, - permissionName: string, - action: PermissionAction, + request: PolicyQuery, roles: string[], ): Promise { + const permissionName = request.permission.name; + const resourceType = (request.permission as ResourcePermission) + .resourceType; + const action = toPermissionAction(request.permission.attributes); + const conditions: PermissionCriteria< PermissionCondition >[] = []; @@ -374,9 +438,16 @@ export class RBACPermissionPolicy implements PermissionPolicy { // this error is unexpected and should not happen, but just in case handle it. if (conditionalDecisions.length > 1) { - this.logger.error( - `Detected ${conditionalDecisions.length} collisions for conditional policies. Expected to find a stored single condition for permission with name ${permissionName}, resource type ${resourceType}, action ${action} for user ${userEntityRef}`, + const msg = `Detected ${JSON.stringify( + conditionalDecisions, + )} collisions for conditional policies. Expected to find a stored single condition for permission with name ${permissionName}, resource type ${resourceType}, action ${action} for user ${userEntityRef}`; + const auditOptions = createPermissionEvaluationOptions( + msg, + userEntityRef, + request, + { result: AuthorizeResult.DENY }, ); + await this.auditLogger.auditLog(auditOptions); return { result: AuthorizeResult.DENY, }; @@ -384,10 +455,7 @@ export class RBACPermissionPolicy implements PermissionPolicy { } if (conditions.length > 0) { - this.logger.info( - `${userEntityRef} executed condition for permission ${permissionName}, resource type ${resourceType} and action ${action}`, - ); - return { + const result: ConditionalPolicyDecision = { pluginId, result: AuthorizeResult.CONDITIONAL, resourceType, @@ -399,6 +467,15 @@ export class RBACPermissionPolicy implements PermissionPolicy { >, }, }; + const msg = `Send condition to plugin with id ${pluginId} to evaluate permission ${permissionName} with resource type ${resourceType} and action ${action} for user ${userEntityRef}`; + const auditOptions = createPermissionEvaluationOptions( + msg, + userEntityRef, + request, + result, + ); + await this.auditLogger.auditLog(auditOptions); + return result; } return undefined; } 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..756483e6e9 100644 --- a/plugins/rbac-backend/src/service/policies-rest-api.test.ts +++ b/plugins/rbac-backend/src/service/policies-rest-api.test.ts @@ -160,6 +160,12 @@ jest.mock('../validation/condition-validation', () => { }; }); +const auditLoggerMock = { + getActorId: jest.fn().mockImplementation(), + createAuditLogDetails: jest.fn().mockImplementation(), + auditLog: jest.fn().mockImplementation(() => Promise.resolve()), +}; + const mockHttpAuth = mockServices.httpAuth(); const mockAuth = mockServices.auth(); const credentials = mockCredentials.user(); @@ -196,6 +202,8 @@ const expectedConditions: RoleConditionalPolicyDecision[] = [ }, ]; +const modifiedBy = 'user:default/some-admin'; + describe('REST policies api', () => { let app: express.Express; @@ -276,7 +284,11 @@ describe('REST policies api', () => { .fn() .mockImplementation( async (roleEntityRef: string): Promise => { - return { source: 'rest', roleEntityRef: roleEntityRef }; + return { + source: 'rest', + roleEntityRef: roleEntityRef, + modifiedBy: 'user:default/some-user', + }; }, ); @@ -287,6 +299,7 @@ describe('REST policies api', () => { identity: mockIdentityClient, policy: await RBACPermissionPolicy.build( logger, + auditLoggerMock, config, conditionalStorage, mockEnforcer as EnforcerDelegate, @@ -305,12 +318,14 @@ describe('REST policies api', () => { conditionalStorage, backendPluginIDsProviderMock, roleMetadataStorageMock, + auditLoggerMock, ); const router = await server.serve(); app = express().use(router); app.use(errorHandler()); conditionalStorage.getCondition.mockReset(); validateRoleConditionMock.mockReset(); + auditLoggerMock.auditLog.mockClear(); jest.clearAllMocks(); }); @@ -383,7 +398,7 @@ describe('REST policies api', () => { expect(result.statusCode).toBe(403); expect(result.body.error).toEqual({ name: 'NotAllowedError', - message: 'User not found', + message: 'User identity not found', }); }); @@ -489,6 +504,7 @@ describe('REST policies api', () => { const roleMeta: RoleMetadataDao = { roleEntityRef: 'user:default/permission_admin', source: 'csv-file', + modifiedBy, }; roleMetadataStorageMock.findRoleMetadata = jest @@ -498,7 +514,7 @@ describe('REST policies api', () => { if (roleEntityRef === roleMeta.roleEntityRef) { return roleMeta; } - return { source: 'rest', roleEntityRef: roleEntityRef }; + return { source: 'rest', roleEntityRef: roleEntityRef, modifiedBy }; }, ); @@ -526,6 +542,7 @@ describe('REST policies api', () => { const roleMeta: RoleMetadataDao = { roleEntityRef: 'user:default/permission_admin', source: 'configuration', + modifiedBy, }; roleMetadataStorageMock.findRoleMetadata = jest @@ -535,7 +552,7 @@ describe('REST policies api', () => { if (roleEntityRef === roleMeta.roleEntityRef) { return roleMeta; } - return { source: 'rest', roleEntityRef: roleEntityRef }; + return { source: 'rest', roleEntityRef: roleEntityRef, modifiedBy }; }, ); @@ -928,6 +945,7 @@ describe('REST policies api', () => { const roleMeta: RoleMetadataDao = { roleEntityRef: 'user:default/permission_admin', source: 'csv-file', + modifiedBy, }; roleMetadataStorageMock.findRoleMetadata = jest @@ -937,7 +955,7 @@ describe('REST policies api', () => { if (roleEntityRef === roleMeta.roleEntityRef) { return roleMeta; } - return { source: 'rest', roleEntityRef: roleEntityRef }; + return { source: 'rest', roleEntityRef: roleEntityRef, modifiedBy }; }, ); @@ -971,6 +989,7 @@ describe('REST policies api', () => { const roleMeta: RoleMetadataDao = { roleEntityRef: 'user:default/permission_admin', source: 'configuration', + modifiedBy, }; roleMetadataStorageMock.findRoleMetadata = jest @@ -980,7 +999,7 @@ describe('REST policies api', () => { if (roleEntityRef === roleMeta.roleEntityRef) { return roleMeta; } - return { source: 'rest', roleEntityRef: roleEntityRef }; + return { source: 'rest', roleEntityRef: roleEntityRef, modifiedBy }; }, ); @@ -1680,6 +1699,7 @@ describe('REST policies api', () => { const roleMeta: RoleMetadataDao = { roleEntityRef: 'user:default/permission_admin', source: 'csv-file', + modifiedBy, }; roleMetadataStorageMock.findRoleMetadata = jest @@ -1689,7 +1709,7 @@ describe('REST policies api', () => { if (roleEntityRef === roleMeta.roleEntityRef) { return roleMeta; } - return { source: 'rest', roleEntityRef: roleEntityRef }; + return { source: 'rest', roleEntityRef: roleEntityRef, modifiedBy }; }, ); @@ -1737,6 +1757,7 @@ describe('REST policies api', () => { const roleMeta: RoleMetadataDao = { roleEntityRef: 'user:default/permission_admin', source: 'configuration', + modifiedBy, }; roleMetadataStorageMock.findRoleMetadata = jest @@ -1746,7 +1767,7 @@ describe('REST policies api', () => { if (roleEntityRef === roleMeta.roleEntityRef) { return roleMeta; } - return { source: 'rest', roleEntityRef: roleEntityRef }; + return { source: 'rest', roleEntityRef: roleEntityRef, modifiedBy }; }, ); @@ -1838,6 +1859,7 @@ describe('REST policies api', () => { name: 'role:default/test', metadata: { source: 'rest', + modifiedBy: 'user:default/some-user', }, }, { @@ -1845,6 +1867,7 @@ describe('REST policies api', () => { name: 'role:default/team_a', metadata: { source: 'rest', + modifiedBy: 'user:default/some-user', }, }, ]); @@ -1900,6 +1923,7 @@ describe('REST policies api', () => { name: 'role:default/rbac_admin', metadata: { source: 'rest', + modifiedBy: 'user:default/some-user', }, }, ]); @@ -1930,6 +1954,11 @@ describe('REST policies api', () => { }); describe('POST /roles', () => { + beforeEach(() => { + mockedAuthorize.mockImplementation(async () => [ + { result: AuthorizeResult.ALLOW }, + ]); + }); it('should return a status of Unauthorized', async () => { mockedAuthorize.mockImplementationOnce(async () => [ { result: AuthorizeResult.DENY }, @@ -2138,6 +2167,7 @@ describe('REST policies api', () => { const roleMeta: RoleMetadataDao = { roleEntityRef: 'role:default/rbac_admin', source: 'configuration', + modifiedBy, }; roleMetadataStorageMock.findRoleMetadata = jest @@ -2147,7 +2177,7 @@ describe('REST policies api', () => { if (roleEntityRef === roleMeta.roleEntityRef) { return roleMeta; } - return { source: 'rest', roleEntityRef: roleEntityRef }; + return { source: 'rest', roleEntityRef: roleEntityRef, modifiedBy }; }, ); @@ -2756,6 +2786,7 @@ describe('REST policies api', () => { const roleMeta: RoleMetadataDao = { roleEntityRef: 'role:default/rbac_admin', source: 'configuration', + modifiedBy, }; roleMetadataStorageMock.findRoleMetadata = jest @@ -2765,7 +2796,7 @@ describe('REST policies api', () => { if (roleEntityRef === roleMeta.roleEntityRef) { return roleMeta; } - return { source: 'rest', roleEntityRef: roleEntityRef }; + return { source: 'rest', roleEntityRef: roleEntityRef, modifiedBy }; }, ); @@ -2909,6 +2940,7 @@ describe('REST policies api', () => { const roleMeta: RoleMetadataDao = { roleEntityRef: 'role:default/rbac_admin', source: 'configuration', + modifiedBy, }; roleMetadataStorageMock.findRoleMetadata = jest @@ -2918,7 +2950,7 @@ describe('REST policies api', () => { if (roleEntityRef === roleMeta.roleEntityRef) { return roleMeta; } - return { source: 'rest', roleEntityRef: roleEntityRef }; + return { source: 'rest', roleEntityRef: roleEntityRef, modifiedBy }; }, ); @@ -2981,7 +3013,7 @@ describe('REST policies api', () => { createdAt: undefined, description: undefined, lastModified: undefined, - modifiedBy: undefined, + modifiedBy: 'user:default/some-user', source: 'rest', }, }, @@ -3097,6 +3129,13 @@ describe('REST policies api', () => { }); describe('DELETE /roles/conditions/:id', () => { + beforeEach(() => { + conditionalStorage.getCondition = jest + .fn() + .mockImplementation(async () => { + return expectedConditions[0]; + }); + }); it('should return a status of Unauthorized', async () => { mockedAuthorize.mockImplementationOnce(async () => [ { result: AuthorizeResult.DENY }, @@ -3129,6 +3168,7 @@ describe('REST policies api', () => { expect(result.statusCode).toEqual(204); expect(mockIdentityClient.getIdentity).toHaveBeenCalledTimes(1); + expect(conditionalStorage.deleteCondition).toHaveBeenCalled(); }); it('should fail to delete condition decision by id', async () => { diff --git a/plugins/rbac-backend/src/service/policies-rest-api.ts b/plugins/rbac-backend/src/service/policies-rest-api.ts index eb0074ab0a..8145bba2ee 100644 --- a/plugins/rbac-backend/src/service/policies-rest-api.ts +++ b/plugins/rbac-backend/src/service/policies-rest-api.ts @@ -25,11 +25,12 @@ import { MetadataResponse, } from '@backstage/plugin-permission-node'; -import { Router } from 'express'; +import express from 'express'; import { Request } from 'express-serve-static-core'; import { isEmpty, isEqual } from 'lodash'; import { ParsedQs } from 'qs'; +import { AuditLogger } from '@janus-idp/backstage-plugin-audit-log-node'; import { PermissionAction, PermissionInfo, @@ -45,6 +46,15 @@ import { } from '@janus-idp/backstage-plugin-rbac-common'; import { PluginIdProvider } from '@janus-idp/backstage-plugin-rbac-node'; +import { + ConditionEvents, + ListConditionEvents, + ListPluginPoliciesEvents, + PermissionEvents, + RoleEvents, + SEND_RESPONSE_STAGE, +} from '../audit-log/audit-logger'; +import { auditError as logAuditError } from '../audit-log/rest-errors-interceptor'; import { ConditionalStorage } from '../database/conditional-storage'; import { daoToMetadata, @@ -73,6 +83,7 @@ export class PoliciesServer { private readonly conditionalStorage: ConditionalStorage, private readonly pluginIdProvider: PluginIdProvider, private readonly roleMetadata: RoleMetadataStorage, + private readonly aLog: AuditLogger, ) {} private async authorize( @@ -81,9 +92,9 @@ export class PoliciesServer { permission: ResourcePermission, ): Promise { if (permission !== policyEntityReadPermission) { - const user = await identity.getIdentity({ request }); - if (!user) { - throw new NotAllowedError('User not found'); + const userIdentity = await identity.getIdentity({ request }); + if (!userIdentity) { + throw new NotAllowedError('User identity not found'); } } @@ -99,7 +110,7 @@ export class PoliciesServer { return decision; } - async serve(): Promise { + async serve(): Promise { const router = await createRouter(this.options); const { identity, discovery, logger, config } = this.options; @@ -168,7 +179,18 @@ export class PoliciesServer { policies = await this.enforcer.getPolicy(); } - response.json(await this.transformPolicyArray(...policies)); + const body = await this.transformPolicyArray(...policies); + + await this.aLog.auditLog({ + message: `Return list permission policies`, + eventName: PermissionEvents.GET_POLICY, + stage: SEND_RESPONSE_STAGE, + status: 'succeeded', + request, + response: { status: 200, body }, + }); + + response.json(body); }); router.get( @@ -188,7 +210,18 @@ export class PoliciesServer { const policy = await this.enforcer.getFilteredPolicy(0, entityRef); if (policy.length !== 0) { - response.json(await this.transformPolicyArray(...policy)); + const body = await this.transformPolicyArray(...policy); + + await this.aLog.auditLog({ + message: `Return permission policy`, + eventName: PermissionEvents.GET_POLICY, + stage: SEND_RESPONSE_STAGE, + status: 'succeeded', + request, + response: { status: 200, body }, + }); + + response.json(body); } else { throw new NotFoundError(); // 404 } @@ -222,6 +255,17 @@ export class PoliciesServer { const processedPolicies = await this.processPolicies(policyRaw, true); await this.enforcer.removePolicies(processedPolicies); + + await this.aLog.auditLog({ + message: `Deleted permission policies`, + eventName: PermissionEvents.DELETE_POLICY, + metadata: { policies: processedPolicies, source: 'rest' }, + stage: SEND_RESPONSE_STAGE, + status: 'succeeded', + request, + response: { status: 204 }, + }); + response.status(204).end(); }, ); @@ -245,8 +289,24 @@ export class PoliciesServer { const processedPolicies = await this.processPolicies(policyRaw); + const entityRef = processedPolicies[0][0]; + const roleMetadata = await this.roleMetadata.findRoleMetadata(entityRef); + if (entityRef.startsWith('role:default') && !roleMetadata) { + throw new Error(`Corresponding role ${entityRef} was not found`); + } + await this.enforcer.addPolicies(processedPolicies, 'rest'); + await this.aLog.auditLog({ + message: `Created permission policies`, + eventName: PermissionEvents.CREATE_POLICY, + metadata: { policies: processedPolicies, source: 'rest' }, + stage: SEND_RESPONSE_STAGE, + status: 'succeeded', + request, + response: { status: 201 }, + }); + response.status(201).end(); }); @@ -313,12 +373,28 @@ export class PoliciesServer { 'new policy', ); + const roleMetadata = + await this.roleMetadata.findRoleMetadata(entityRef); + if (entityRef.startsWith('role:default') && !roleMetadata) { + throw new Error(`Corresponding role ${entityRef} was not found`); + } + await this.enforcer.updatePolicies( processedOldPolicy, processedNewPolicy, 'rest', ); + await this.aLog.auditLog({ + message: `Updated permission policies`, + eventName: PermissionEvents.UPDATE_POLICY, + metadata: { policies: processedNewPolicy, source: 'rest' }, + stage: SEND_RESPONSE_STAGE, + status: 'succeeded', + request, + response: { status: 200 }, + }); + response.status(200).end(); }, ); @@ -338,7 +414,18 @@ export class PoliciesServer { const roles = await this.enforcer.getGroupingPolicy(); - response.json(await this.transformRoleArray(...roles)); + const body = await this.transformRoleArray(...roles); + + await this.aLog.auditLog({ + message: `Return list roles`, + eventName: RoleEvents.GET_ROLE, + stage: SEND_RESPONSE_STAGE, + status: 'succeeded', + request, + response: { status: 200, body }, + }); + + response.json(body); }); router.get('/roles/:kind/:namespace/:name', async (request, response) => { @@ -359,7 +446,18 @@ export class PoliciesServer { ); if (role.length !== 0) { - response.json(await this.transformRoleArray(...role)); + const body = await this.transformRoleArray(...role); + + await this.aLog.auditLog({ + message: `Return ${body[0].name}`, + eventName: RoleEvents.GET_ROLE, + stage: SEND_RESPONSE_STAGE, + status: 'succeeded', + request, + response: { status: 200, body }, + }); + + response.json(body); } else { throw new NotFoundError(); // 404 } @@ -414,15 +512,30 @@ export class PoliciesServer { } const user = await identity.getIdentity({ request }); + const modifiedBy = user?.identity.userEntityRef!; const metadata: RoleMetadataDao = { roleEntityRef: roleRaw.name, source: 'rest', description: roleRaw.metadata?.description ?? '', - author: user?.identity.userEntityRef, - modifiedBy: user?.identity.userEntityRef, + author: modifiedBy, + modifiedBy, }; + await this.enforcer.addGroupingPolicies(roles, metadata); + await this.aLog.auditLog({ + message: `Created ${metadata.roleEntityRef}`, + eventName: RoleEvents.CREATE_ROLE, + metadata: { + ...metadata, + members: roles.map(gp => gp[0]), + }, + stage: SEND_RESPONSE_STAGE, + status: 'succeeded', + request, + response: { status: 201 }, + }); + response.status(201).end(); }); @@ -472,7 +585,7 @@ export class PoliciesServer { ...newRoleRaw.metadata, source: newRoleRaw.metadata?.source ?? 'rest', roleEntityRef: newRoleRaw.name, - modifiedBy: user?.identity.userEntityRef, + modifiedBy: user?.identity.userEntityRef!, }; const oldMetadata = @@ -546,6 +659,23 @@ export class PoliciesServer { await this.enforcer.updateGroupingPolicies(oldRole, newRole, newMetadata); + let message = `Updated ${oldMetadata.roleEntityRef}.`; + if (newMetadata.roleEntityRef !== oldMetadata.roleEntityRef) { + message = `${message}. Role entity reference renamed to ${newMetadata.roleEntityRef}`; + } + await this.aLog.auditLog({ + message, + eventName: RoleEvents.UPDATE_ROLE, + metadata: { + ...newMetadata, + members: newRole.map(gp => gp[0]), + }, + stage: SEND_RESPONSE_STAGE, + status: 'succeeded', + request, + response: { status: 200 }, + }); + response.status(200).end(); }); @@ -583,21 +713,39 @@ export class PoliciesServer { } } - const metadata = + const currentMetadata = await this.roleMetadata.findRoleMetadata(roleEntityRef); - - const err = await validateSource('rest', metadata); + const err = await validateSource('rest', currentMetadata); if (err) { throw new NotAllowedError(`Unable to delete role: ${err.message}`); } const user = await identity.getIdentity({ request }); + const metadata: RoleMetadataDao = { + roleEntityRef, + source: 'rest', + modifiedBy: user?.identity.userEntityRef!, + }; + await this.enforcer.removeGroupingPolicies( roleMembers, - user?.identity.userEntityRef, + metadata, false, ); + await this.aLog.auditLog({ + message: `Deleted ${metadata.roleEntityRef}`, + eventName: RoleEvents.DELETE_ROLE, + metadata: { + ...metadata, + members: roleMembers.map(gp => gp[0]), + }, + stage: SEND_RESPONSE_STAGE, + status: 'succeeded', + request, + response: { status: 204 }, + }); + response.status(204).end(); }, ); @@ -613,8 +761,18 @@ export class PoliciesServer { throw new NotAllowedError(); // 403 } - const policies = await pluginPermMetaData.getPluginPolicies(this.auth); - response.json(policies); + const body = await pluginPermMetaData.getPluginPolicies(this.auth); + + await this.aLog.auditLog({ + message: `Return list plugin policies`, + eventName: ListPluginPoliciesEvents.GET_PLUGINS_POLICIES, + stage: SEND_RESPONSE_STAGE, + status: 'succeeded', + request, + response: { status: 200, body }, + }); + + response.json(body); }); router.get('/plugins/condition-rules', async (request, response) => { @@ -628,8 +786,18 @@ export class PoliciesServer { throw new NotAllowedError(); // 403 } - const rules = await pluginPermMetaData.getPluginConditionRules(this.auth); - response.json(rules); + const body = await pluginPermMetaData.getPluginConditionRules(this.auth); + + await this.aLog.auditLog({ + message: `Return list conditional rules and schemas`, + eventName: ListConditionEvents.GET_CONDITION_RULES, + stage: SEND_RESPONSE_STAGE, + status: 'succeeded', + request, + response: { status: 200, body }, + }); + + response.json(body); }); router.get('/roles/conditions', async (request, response) => { @@ -650,14 +818,24 @@ export class PoliciesServer { this.getActionQueries(request.query.actions), ); - const result: RoleConditionalPolicyDecision[] = + const body: RoleConditionalPolicyDecision[] = conditions.map(condition => { return { ...condition, permissionMapping: condition.permissionMapping.map(pm => pm.action), }; }); - response.json(result); + + await this.aLog.auditLog({ + message: `Return list conditional permission policies`, + eventName: ConditionEvents.GET_CONDITION, + stage: SEND_RESPONSE_STAGE, + status: 'succeeded', + request, + response: { status: 200, body }, + }); + + response.json(body); }); router.post('/roles/conditions', async (request, response) => { @@ -683,7 +861,19 @@ export class PoliciesServer { const id = await this.conditionalStorage.createCondition(conditionToCreate); - response.status(201).json({ id: id }); + const body = { id: id }; + + await this.aLog.auditLog({ + message: `Created conditional permission policy`, + eventName: ConditionEvents.CREATE_CONDITION, + metadata: { condition: conditionToCreate }, + stage: SEND_RESPONSE_STAGE, + status: 'succeeded', + request, + response: { status: 201, body }, + }); + + response.status(201).json(body); }); router.get('/roles/conditions/:id', async (request, response) => { @@ -707,12 +897,21 @@ export class PoliciesServer { throw new NotFoundError(); } - const result: RoleConditionalPolicyDecision = { + const body: RoleConditionalPolicyDecision = { ...condition, permissionMapping: condition.permissionMapping.map(pm => pm.action), }; - response.json(result); + await this.aLog.auditLog({ + message: `Return conditional permission policy by id`, + eventName: ConditionEvents.GET_CONDITION, + stage: SEND_RESPONSE_STAGE, + status: 'succeeded', + request, + response: { status: 200, body }, + }); + + response.json(body); }); router.delete('/roles/conditions/:id', async (request, response) => { @@ -731,7 +930,28 @@ export class PoliciesServer { throw new InputError('Id is not a valid number.'); } + const condition = await this.conditionalStorage.getCondition(id); + if (!condition) { + throw new NotFoundError(`Condition with id ${id} was not found`); + } + const conditionToDelete: RoleConditionalPolicyDecision = + { + ...condition, + permissionMapping: condition.permissionMapping.map(pm => pm.action), + }; + await this.conditionalStorage.deleteCondition(id); + + await this.aLog.auditLog({ + message: `Deleted conditional permission policy`, + eventName: ConditionEvents.DELETE_CONDITION, + metadata: { condition: conditionToDelete }, + stage: SEND_RESPONSE_STAGE, + status: 'succeeded', + request, + response: { status: 204 }, + }); + response.status(204).end(); }); @@ -762,9 +982,22 @@ export class PoliciesServer { ); await this.conditionalStorage.updateCondition(id, conditionToUpdate); + + await this.aLog.auditLog({ + message: `Updated conditional permission policy`, + eventName: ConditionEvents.UPDATE_CONDITION, + metadata: { conditionId: id, condition: conditionToUpdate }, + stage: SEND_RESPONSE_STAGE, + status: 'succeeded', + request, + response: { status: 200 }, + }); + response.status(200).end(); }); + router.use(logAuditError(this.aLog)); + return router; } @@ -1014,11 +1247,6 @@ export class PoliciesServer { }' and action ${JSON.stringify(action)}`, ); } - console.log( - `Found permission ${JSON.stringify(perm)} for resource type ${ - roleConditionPolicy.resourceType - } and action ${action}`, - ); permInfo.push({ name: perm.name, action }); } diff --git a/plugins/rbac-backend/src/service/policy-builder.ts b/plugins/rbac-backend/src/service/policy-builder.ts index 6b093a6042..2cc5a48f6f 100644 --- a/plugins/rbac-backend/src/service/policy-builder.ts +++ b/plugins/rbac-backend/src/service/policy-builder.ts @@ -14,6 +14,7 @@ import { newEnforcer, newModelFromString } from 'casbin'; import { Router } from 'express'; import { Logger } from 'winston'; +import { DefaultAuditLogger } from '@janus-idp/backstage-plugin-audit-log-node'; import { PluginIdProvider } from '@janus-idp/backstage-plugin-rbac-node'; import { CasbinDBAdapterFactory } from '../database/casbin-adapter-factory'; @@ -98,6 +99,12 @@ export class PolicyBuilder { databaseClient, ); + const defAuditLog = new DefaultAuditLogger({ + logger: env.logger, + authService: auth, + httpAuthService: httpAuth, + }); + const options: RouterOptions = { config: env.config, logger: env.logger, @@ -105,6 +112,7 @@ export class PolicyBuilder { identity: env.identity, policy: await RBACPermissionPolicy.build( env.logger, + defAuditLog, env.config, conditionStorage, enforcerDelegate, @@ -138,6 +146,7 @@ export class PolicyBuilder { conditionStorage, pluginIdProvider, roleMetadataStorage, + defAuditLog, ); return server.serve(); } diff --git a/plugins/rbac-backend/src/validation/policies-validation.test.ts b/plugins/rbac-backend/src/validation/policies-validation.test.ts index 0039d68c9c..b65872b366 100644 --- a/plugins/rbac-backend/src/validation/policies-validation.test.ts +++ b/plugins/rbac-backend/src/validation/policies-validation.test.ts @@ -17,6 +17,8 @@ import { validateSource, } from './policies-validation'; +const modifiedBy = 'user:default/some-admin'; + const roleMetadataStorageMock: RoleMetadataStorage = { findRoleMetadata: jest .fn() @@ -28,6 +30,7 @@ const roleMetadataStorageMock: RoleMetadataStorage = { return { roleEntityRef: 'role:default/catalog-reader', source: 'rest', + modifiedBy, }; }, ), @@ -231,6 +234,7 @@ describe('rest data validation', () => { const roleMeta: RoleMetadataDao = { roleEntityRef: 'role:default/catalog-reader', source: 'rest', + modifiedBy, }; it('should not return an error whenever the source that is passed matches the source of the role', async () => { @@ -245,6 +249,7 @@ describe('rest data validation', () => { const roleMetaLegacy: RoleMetadataDao = { roleEntityRef: 'role:default/legacy-reader', source: 'legacy', + modifiedBy, }; const source: Source = 'rest'; @@ -274,6 +279,7 @@ describe('rest data validation', () => { const roleMeta: RoleMetadataDao = { roleEntityRef: 'role:default/catalog-reader', source: 'rest', + modifiedBy, }; it('should not return an error during validation', async () => { diff --git a/plugins/rbac-common/src/types.ts b/plugins/rbac-common/src/types.ts index 2ff74fd93f..da25c2e35f 100644 --- a/plugins/rbac-common/src/types.ts +++ b/plugins/rbac-common/src/types.ts @@ -1,5 +1,8 @@ import { NotAllowedError } from '@backstage/errors'; -import { ConditionalPolicyDecision } from '@backstage/plugin-permission-common'; +import { + ConditionalPolicyDecision, + PermissionAttributes, +} from '@backstage/plugin-permission-common'; export type Source = | 'rest' // created via REST API @@ -50,7 +53,13 @@ export type PermissionPolicy = { export type NonEmptyArray = [T, ...T[]]; +// Permission framework attributes action has values: 'create' | 'read' | 'update' | 'delete' | undefined. +// But we are introducing an action named "use" when action does not exist('undefined') to avoid +// a more complicated model with multiple policy and request shapes. export type PermissionAction = 'create' | 'read' | 'update' | 'delete' | 'use'; +export const toPermissionAction = ( + attr: PermissionAttributes, +): PermissionAction => attr.action ?? 'use'; export type PermissionInfo = { name: string;