-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Cases] RBAC Refactoring audit logging #100952
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
jonathan-buttner
merged 9 commits into
elastic:cases-rbac-poc
from
jonathan-buttner:cases-rbac-audit-log-refactor
Jun 3, 2021
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
6878922
Refactoring audit logging
jonathan-buttner 294c99e
Merge branch 'cases-rbac-poc' of github.com:elastic/kibana into cases…
jonathan-buttner 20e49ed
Merge branch 'cases-rbac-poc' of github.com:elastic/kibana into cases…
jonathan-buttner 83146b3
Adding unit tests for authorization classes
jonathan-buttner 95f2f58
Addressing feedback and adding util tests
jonathan-buttner 2829509
return undefined on empty array
jonathan-buttner e69c1cf
Merge branch 'cases-rbac-poc' of github.com:elastic/kibana into cases…
jonathan-buttner f5ee7dc
Merge branch 'cases-rbac-poc' of github.com:elastic/kibana into cases…
jonathan-buttner aa3bc6a
fixing eslint
jonathan-buttner File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
1,765 changes: 1,765 additions & 0 deletions
1,765
x-pack/plugins/cases/server/authorization/__snapshots__/audit_logger.test.ts.snap
Large diffs are not rendered by default.
Oops, something went wrong.
208 changes: 208 additions & 0 deletions
208
x-pack/plugins/cases/server/authorization/audit_logger.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,208 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0; you may not use this file except in compliance with the Elastic License | ||
| * 2.0. | ||
| */ | ||
|
|
||
| import { AuditLogger } from '../../../../plugins/security/server'; | ||
| import { Operations } from '.'; | ||
| import { AuthorizationAuditLogger } from './audit_logger'; | ||
| import { ReadOperations } from './types'; | ||
|
|
||
| describe('audit_logger', () => { | ||
| it('creates a failure message without any owners', () => { | ||
| expect( | ||
| AuthorizationAuditLogger.createFailureMessage({ | ||
| owners: [], | ||
| operation: Operations.createCase, | ||
| }) | ||
| ).toBe('Unauthorized to create case of any owner'); | ||
| }); | ||
|
|
||
| it('creates a failure message with owners', () => { | ||
| expect( | ||
| AuthorizationAuditLogger.createFailureMessage({ | ||
| owners: ['a', 'b'], | ||
| operation: Operations.createCase, | ||
| }) | ||
| ).toBe('Unauthorized to create case with owners: "a, b"'); | ||
| }); | ||
|
|
||
| describe('log function', () => { | ||
| const mockLogger: jest.Mocked<AuditLogger> = { | ||
| log: jest.fn(), | ||
| }; | ||
|
|
||
| let logger: AuthorizationAuditLogger; | ||
|
|
||
| beforeEach(() => { | ||
| mockLogger.log.mockReset(); | ||
| logger = new AuthorizationAuditLogger(mockLogger); | ||
| }); | ||
|
|
||
| it('does not throw an error when the underlying audit logger is undefined', () => { | ||
| const authLogger = new AuthorizationAuditLogger(); | ||
| jest.spyOn(authLogger, 'log'); | ||
|
|
||
| expect(() => { | ||
| authLogger.log({ | ||
| operation: Operations.createCase, | ||
| entity: { | ||
| owner: 'a', | ||
| id: '1', | ||
| }, | ||
| }); | ||
| }).not.toThrow(); | ||
|
|
||
| expect(authLogger.log).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('logs a message with a saved object ID in the message field', () => { | ||
| logger.log({ | ||
| operation: Operations.createCase, | ||
| entity: { | ||
| owner: 'a', | ||
| id: '1', | ||
| }, | ||
| }); | ||
| expect(mockLogger.log.mock.calls[0][0]?.message).toContain('[id=1]'); | ||
| }); | ||
|
|
||
| it('creates the owner part of the message when no owners are specified', () => { | ||
| logger.log({ | ||
| operation: Operations.createCase, | ||
| }); | ||
|
|
||
| expect(mockLogger.log.mock.calls[0][0]?.message).toContain('as any owners'); | ||
| }); | ||
|
|
||
| it('creates the owner part of the message when an owner is specified', () => { | ||
| logger.log({ | ||
| operation: Operations.createCase, | ||
| entity: { | ||
| owner: 'a', | ||
| id: '1', | ||
| }, | ||
| }); | ||
|
|
||
| expect(mockLogger.log.mock.calls[0][0]?.message).toContain('as owner "a"'); | ||
| }); | ||
|
|
||
| it('creates a failure message when passed an error', () => { | ||
| logger.log({ | ||
| operation: Operations.createCase, | ||
| entity: { | ||
| owner: 'a', | ||
| id: '1', | ||
| }, | ||
| error: new Error('error occurred'), | ||
| }); | ||
|
|
||
| expect(mockLogger.log.mock.calls[0][0]?.message).toBe( | ||
| 'Failed attempt to create cases [id=1] as owner "a"' | ||
| ); | ||
|
|
||
| expect(mockLogger.log.mock.calls[0][0]?.event?.outcome).toBe('failure'); | ||
| }); | ||
|
|
||
| it('creates a write operation message', () => { | ||
| logger.log({ | ||
| operation: Operations.createCase, | ||
| entity: { | ||
| owner: 'a', | ||
| id: '1', | ||
| }, | ||
| }); | ||
|
|
||
| expect(mockLogger.log.mock.calls[0][0]?.message).toBe( | ||
| 'User is creating cases [id=1] as owner "a"' | ||
| ); | ||
|
|
||
| expect(mockLogger.log.mock.calls[0][0]?.event?.outcome).toBe('unknown'); | ||
| }); | ||
|
|
||
| it('creates a read operation message', () => { | ||
| logger.log({ | ||
| operation: Operations.getCase, | ||
| entity: { | ||
| owner: 'a', | ||
| id: '1', | ||
| }, | ||
| }); | ||
|
|
||
| expect(mockLogger.log.mock.calls[0][0]?.message).toBe( | ||
| 'User has accessed cases [id=1] as owner "a"' | ||
| ); | ||
|
|
||
| expect(mockLogger.log.mock.calls[0][0]?.event?.outcome).toBe('success'); | ||
| }); | ||
|
|
||
| describe('event structure', () => { | ||
| // I would have preferred to do these as match inline but that isn't supported because this is essentially a for loop | ||
| // for reference: https://github.com/facebook/jest/issues/9409#issuecomment-629272237 | ||
|
|
||
| // This loops through all operation keys | ||
| it.each(Array.from(Object.keys(Operations)))( | ||
| `creates the correct audit event for operation: "%s" without an error or entity`, | ||
| (operationKey) => { | ||
| // forcing the cast here because using a string throws a type error | ||
| const key = operationKey as ReadOperations; | ||
| logger.log({ | ||
| operation: Operations[key], | ||
| }); | ||
| expect(mockLogger.log.mock.calls[0][0]).toMatchSnapshot(); | ||
| } | ||
| ); | ||
|
|
||
| // This loops through all operation keys | ||
| it.each(Array.from(Object.keys(Operations)))( | ||
| `creates the correct audit event for operation: "%s" with an error but no entity`, | ||
| (operationKey) => { | ||
| // forcing the cast here because using a string throws a type error | ||
| const key = operationKey as ReadOperations; | ||
| logger.log({ | ||
| operation: Operations[key], | ||
| error: new Error('an error'), | ||
| }); | ||
| expect(mockLogger.log.mock.calls[0][0]).toMatchSnapshot(); | ||
| } | ||
| ); | ||
|
|
||
| // This loops through all operation keys | ||
| it.each(Array.from(Object.keys(Operations)))( | ||
| `creates the correct audit event for operation: "%s" with an error and entity`, | ||
| (operationKey) => { | ||
| // forcing the cast here because using a string throws a type error | ||
| const key = operationKey as ReadOperations; | ||
| logger.log({ | ||
| operation: Operations[key], | ||
| entity: { | ||
| owner: 'awesome', | ||
| id: '1', | ||
| }, | ||
| error: new Error('an error'), | ||
| }); | ||
| expect(mockLogger.log.mock.calls[0][0]).toMatchSnapshot(); | ||
| } | ||
| ); | ||
|
|
||
| // This loops through all operation keys | ||
| it.each(Array.from(Object.keys(Operations)))( | ||
| `creates the correct audit event for operation: "%s" without an error but with an entity`, | ||
| (operationKey) => { | ||
| // forcing the cast here because using a string throws a type error | ||
| const key = operationKey as ReadOperations; | ||
| logger.log({ | ||
| operation: Operations[key], | ||
| entity: { | ||
| owner: 'super', | ||
| id: '5', | ||
| }, | ||
| }); | ||
| expect(mockLogger.log.mock.calls[0][0]).toMatchSnapshot(); | ||
| } | ||
| ); | ||
| }); | ||
| }); | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,12 +5,15 @@ | |
| * 2.0. | ||
| */ | ||
|
|
||
| import { DATABASE_CATEGORY, ECS_OUTCOMES, OperationDetails } from '.'; | ||
| import { AuditLogger } from '../../../security/server'; | ||
| import { EcsEventOutcome } from 'kibana/server'; | ||
| import { DATABASE_CATEGORY, ECS_OUTCOMES, isWriteOperation, OperationDetails } from '.'; | ||
| import { AuditEvent, AuditLogger } from '../../../security/server'; | ||
| import { OwnerEntity } from './types'; | ||
|
|
||
| enum AuthorizationResult { | ||
| Unauthorized = 'Unauthorized', | ||
| Authorized = 'Authorized', | ||
| interface CreateAuditMsgParams { | ||
| operation: OperationDetails; | ||
| entity?: OwnerEntity; | ||
| error?: Error; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -19,106 +22,80 @@ enum AuthorizationResult { | |
| export class AuthorizationAuditLogger { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👏 this looks much better! |
||
| private readonly auditLogger?: AuditLogger; | ||
|
|
||
| constructor(logger: AuditLogger | undefined) { | ||
| constructor(logger?: AuditLogger) { | ||
| this.auditLogger = logger; | ||
| } | ||
|
|
||
| private static createMessage({ | ||
| result, | ||
| owners, | ||
| operation, | ||
| }: { | ||
| result: AuthorizationResult; | ||
| owners?: string[]; | ||
| operation: OperationDetails; | ||
| }): string { | ||
| const ownerMsg = owners == null ? 'of any owner' : `with owners: "${owners.join(', ')}"`; | ||
| /** | ||
| * This will take the form: | ||
| * `Unauthorized to create case with owners: "securitySolution, observability"` | ||
| * `Unauthorized to find cases of any owner`. | ||
| */ | ||
| return `${result} to ${operation.verbs.present} ${operation.docType} ${ownerMsg}`; | ||
| } | ||
| /** | ||
| * Creates an AuditEvent describing the state of a request. | ||
| */ | ||
| private static createAuditMsg({ operation, error, entity }: CreateAuditMsgParams): AuditEvent { | ||
| const doc = | ||
| entity !== undefined | ||
| ? `${operation.savedObjectType} [id=${entity.id}]` | ||
| : `a ${operation.docType}`; | ||
|
|
||
| private logSuccessEvent({ | ||
| message, | ||
| operation, | ||
| username, | ||
| }: { | ||
| message: string; | ||
| operation: OperationDetails; | ||
| username?: string; | ||
| }) { | ||
| this.auditLogger?.log({ | ||
| message: `${username ?? 'unknown user'} ${message}`, | ||
| const ownerText = entity === undefined ? 'as any owners' : `as owner "${entity.owner}"`; | ||
|
|
||
| let message: string; | ||
| let outcome: EcsEventOutcome; | ||
|
|
||
| if (error) { | ||
| message = `Failed attempt to ${operation.verbs.present} ${doc} ${ownerText}`; | ||
| outcome = ECS_OUTCOMES.failure; | ||
| } else if (isWriteOperation(operation)) { | ||
| message = `User is ${operation.verbs.progressive} ${doc} ${ownerText}`; | ||
| outcome = ECS_OUTCOMES.unknown; | ||
| } else { | ||
| message = `User has ${operation.verbs.past} ${doc} ${ownerText}`; | ||
| outcome = ECS_OUTCOMES.success; | ||
| } | ||
|
|
||
| return { | ||
| message, | ||
| event: { | ||
| action: operation.action, | ||
| category: DATABASE_CATEGORY, | ||
| type: [operation.type], | ||
| outcome: ECS_OUTCOMES.success, | ||
| type: [operation.ecsType], | ||
| outcome, | ||
| }, | ||
| ...(username != null && { | ||
| user: { | ||
| name: username, | ||
| ...(entity !== undefined && { | ||
| kibana: { | ||
| saved_object: { type: operation.savedObjectType, id: entity.id }, | ||
| }, | ||
| }), | ||
| }); | ||
| ...(error !== undefined && { | ||
| error: { | ||
| code: error.name, | ||
| message: error.message, | ||
| }, | ||
| }), | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Creates a audit message describing a failure to authorize | ||
| * Creates a message to be passed to an Error or Boom. | ||
| */ | ||
| public failure({ | ||
| username, | ||
| public static createFailureMessage({ | ||
| owners, | ||
| operation, | ||
| }: { | ||
| username?: string; | ||
| owners?: string[]; | ||
| owners: string[]; | ||
| operation: OperationDetails; | ||
| }): string { | ||
| const message = AuthorizationAuditLogger.createMessage({ | ||
| result: AuthorizationResult.Unauthorized, | ||
| owners, | ||
| operation, | ||
| }); | ||
| this.auditLogger?.log({ | ||
| message: `${username ?? 'unknown user'} ${message}`, | ||
| event: { | ||
| action: operation.action, | ||
| category: DATABASE_CATEGORY, | ||
| type: [operation.type], | ||
| outcome: ECS_OUTCOMES.failure, | ||
| }, | ||
| // add the user information if we have it | ||
| ...(username != null && { | ||
| user: { | ||
| name: username, | ||
| }, | ||
| }), | ||
| }); | ||
| return message; | ||
| }) { | ||
| const ownerMsg = owners.length <= 0 ? 'of any owner' : `with owners: "${owners.join(', ')}"`; | ||
| /** | ||
| * This will take the form: | ||
| * `Unauthorized to create case with owners: "securitySolution, observability"` | ||
| * `Unauthorized to access cases of any owner` | ||
| */ | ||
| return `Unauthorized to ${operation.verbs.present} ${operation.docType} ${ownerMsg}`; | ||
| } | ||
|
|
||
| /** | ||
| * Creates a audit message describing a successful authorization | ||
| * Logs an audit event based on the status of an operation. | ||
| */ | ||
| public success({ | ||
| username, | ||
| operation, | ||
| owners, | ||
| }: { | ||
| username?: string; | ||
| owners: string[]; | ||
| operation: OperationDetails; | ||
| }): string { | ||
| const message = AuthorizationAuditLogger.createMessage({ | ||
| result: AuthorizationResult.Authorized, | ||
| owners, | ||
| operation, | ||
| }); | ||
| this.logSuccessEvent({ message, operation, username }); | ||
| return message; | ||
| public log(auditMsgParams: CreateAuditMsgParams) { | ||
| this.auditLogger?.log(AuthorizationAuditLogger.createAuditMsg(auditMsgParams)); | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a test ensuring that we can call
logger.logwithout consequence if themockLoggeris undefined?