Skip to content

Commit

Permalink
Added suggestions from code review
Browse files Browse the repository at this point in the history
  • Loading branch information
@thomheymann
thomheymann committed Oct 16, 2020
1 parent c6a7976 commit ccb42d4
Show file tree
Hide file tree
Showing 4 changed files with 63 additions and 16 deletions.
29 changes: 26 additions & 3 deletions x-pack/plugins/security/server/audit/audit_service.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ const getSpaceId = jest.fn().mockReturnValue('default');

beforeEach(() => {
logger.info.mockClear();
logging.configure.mockClear();
http.registerOnPostAuth.mockClear();
});

Expand All @@ -53,10 +54,18 @@ describe('#setup', () => {
`);
});

it('configures logging correctly', async () => {
it('configures logging correctly when using ecs logger', async () => {
new AuditService(logger).setup({
license,
config,
config: {
enabled: true,
appender: {
kind: 'console',
layout: {
kind: 'pattern',
},
},
},
logging,
http,
getCurrentUser,
Expand All @@ -65,6 +74,20 @@ describe('#setup', () => {
expect(logging.configure).toHaveBeenCalledWith(expect.any(Observable));
});

it('does not configure logging when using legacy logger', async () => {
new AuditService(logger).setup({
license,
config: {
enabled: true,
},
logging,
http,
getCurrentUser,
getSpaceId,
});
expect(logging.configure).not.toHaveBeenCalled();
});

it('registers post auth hook', () => {
new AuditService(logger).setup({
license,
Expand Down Expand Up @@ -158,7 +181,7 @@ describe('#createLoggingConfig', () => {
"appenders": Array [
"auditTrailAppender",
],
"context": "audit.beta",
"context": "audit",
"level": "info",
},
],
Expand Down
26 changes: 13 additions & 13 deletions x-pack/plugins/security/server/audit/audit_service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,8 @@ export class AuditService {
* @deprecated
*/
private allowAuditLogging = false;
private readonly betaLogger: Logger;

constructor(private readonly logger: Logger) {
this.betaLogger = this.logger.get('beta');
}
constructor(private readonly logger: Logger) {}

setup({
license,
Expand All @@ -86,13 +83,16 @@ export class AuditService {
});
}

// Configure logging during setup and when license changes
logging.configure(
license.features$.pipe(
distinctUntilKeyChanged('allowAuditLogging'),
createLoggingConfig(config)
)
);
// Do not change logging for legacy logger
if (config.appender) {
// Configure logging during setup and when license changes
logging.configure(
license.features$.pipe(
distinctUntilKeyChanged('allowAuditLogging'),
createLoggingConfig(config)
)
);
}

/**
* Creates an {@link AuditLogger} scoped to the current request.
Expand Down Expand Up @@ -146,7 +146,7 @@ export class AuditService {
},
};
if (filterEvent(meta, config.ignore_filters)) {
this.betaLogger.info(event.message!, meta);
this.logger.info(event.message!, meta);
}
};
return { log };
Expand Down Expand Up @@ -203,7 +203,7 @@ export const createLoggingConfig = (config: ConfigType['audit']) =>
},
loggers: [
{
context: 'audit.beta',
context: 'audit',
level: config.enabled && config.appender && features.allowAuditLogging ? 'info' : 'off',
appenders: ['auditTrailAppender'],
},
Expand Down
3 changes: 3 additions & 0 deletions x-pack/plugins/security/server/authentication/authenticator.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ describe('Authenticator', () => {
};

beforeEach(() => {
auditLogger.log.mockClear();
mockOptions = getMockOptions({ providers: { basic: { basic1: { order: 0 } } } });
mockOptions.session.get.mockResolvedValue(null);
mockOptions.audit.asScoped.mockReturnValue(auditLogger);
Expand Down Expand Up @@ -294,6 +295,7 @@ describe('Authenticator', () => {
);
await authenticator.login(request, { provider: { type: 'basic' }, value: {} });

expect(auditLogger.log).toHaveBeenCalledTimes(1);
expect(auditLogger.log).toHaveBeenCalledWith(
expect.objectContaining({
event: { action: 'user_login', category: 'authentication', outcome: 'success' },
Expand All @@ -309,6 +311,7 @@ describe('Authenticator', () => {
);
await authenticator.login(request, { provider: { type: 'basic' }, value: {} });

expect(auditLogger.log).toHaveBeenCalledTimes(1);
expect(auditLogger.log).toHaveBeenCalledWith(
expect.objectContaining({
event: { action: 'user_login', category: 'authentication', outcome: 'failure' },
Expand Down
21 changes: 21 additions & 0 deletions x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -419,12 +419,15 @@ describe('#addToNamespaces', () => {
const apiCallReturnValue = Symbol();
clientOpts.baseClient.addToNamespaces.mockReturnValue(apiCallReturnValue as any);
await client.addToNamespaces(type, id, namespaces);

expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(1);
expectAuditEvent('saved_object_add_to_spaces', EventOutcome.UNKNOWN, { type, id });
});

test(`adds audit event when not successful`, async () => {
clientOpts.checkSavedObjectsPrivilegesAsCurrentUser.mockRejectedValue(new Error());
await expect(() => client.addToNamespaces(type, id, namespaces)).rejects.toThrow();
expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(1);
expectAuditEvent('saved_object_add_to_spaces', EventOutcome.FAILURE, { type, id });
});
});
Expand Down Expand Up @@ -487,13 +490,15 @@ describe('#bulkCreate', () => {
const objects = [obj1, obj2];
const options = { namespace };
await expectSuccess(client.bulkCreate, { objects, options });
expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(2);
expectAuditEvent('saved_object_create', EventOutcome.UNKNOWN, { type: obj1.type, id: obj1.id });
expectAuditEvent('saved_object_create', EventOutcome.UNKNOWN, { type: obj2.type, id: obj2.id });
});

test(`adds audit event when not successful`, async () => {
clientOpts.checkSavedObjectsPrivilegesAsCurrentUser.mockRejectedValue(new Error());
await expect(() => client.bulkCreate([obj1, obj2], { namespace })).rejects.toThrow();
expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(2);
expectAuditEvent('saved_object_create', EventOutcome.FAILURE, { type: obj1.type, id: obj1.id });
expectAuditEvent('saved_object_create', EventOutcome.FAILURE, { type: obj2.type, id: obj2.id });
});
Expand Down Expand Up @@ -543,13 +548,15 @@ describe('#bulkGet', () => {
const objects = [obj1, obj2];
const options = { namespace };
await expectSuccess(client.bulkGet, { objects, options });
expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(2);
expectAuditEvent('saved_object_get', EventOutcome.SUCCESS, obj1);
expectAuditEvent('saved_object_get', EventOutcome.SUCCESS, obj2);
});

test(`adds audit event when not successful`, async () => {
clientOpts.checkSavedObjectsPrivilegesAsCurrentUser.mockRejectedValue(new Error());
await expect(() => client.bulkGet([obj1, obj2], { namespace })).rejects.toThrow();
expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(2);
expectAuditEvent('saved_object_get', EventOutcome.FAILURE, obj1);
expectAuditEvent('saved_object_get', EventOutcome.FAILURE, obj2);
});
Expand Down Expand Up @@ -610,13 +617,15 @@ describe('#bulkUpdate', () => {
const objects = [obj1, obj2];
const options = { namespace };
await expectSuccess(client.bulkUpdate, { objects, options });
expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(2);
expectAuditEvent('saved_object_update', EventOutcome.UNKNOWN, { type: obj1.type, id: obj1.id });
expectAuditEvent('saved_object_update', EventOutcome.UNKNOWN, { type: obj2.type, id: obj2.id });
});

test(`adds audit event when not successful`, async () => {
clientOpts.checkSavedObjectsPrivilegesAsCurrentUser.mockRejectedValue(new Error());
await expect(() => client.bulkUpdate<any>([obj1, obj2], { namespace })).rejects.toThrow();
expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(2);
expectAuditEvent('saved_object_update', EventOutcome.FAILURE, { type: obj1.type, id: obj1.id });
expectAuditEvent('saved_object_update', EventOutcome.FAILURE, { type: obj2.type, id: obj2.id });
});
Expand Down Expand Up @@ -706,12 +715,14 @@ describe('#create', () => {
clientOpts.baseClient.create.mockResolvedValue(apiCallReturnValue as any);
const options = { namespace };
await expectSuccess(client.create, { type, attributes, options });
expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(1);
expectAuditEvent('saved_object_create', EventOutcome.UNKNOWN, { type });
});

test(`adds audit event when not successful`, async () => {
clientOpts.checkSavedObjectsPrivilegesAsCurrentUser.mockRejectedValue(new Error());
await expect(() => client.create(type, attributes, { namespace })).rejects.toThrow();
expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(1);
expectAuditEvent('saved_object_create', EventOutcome.FAILURE, { type });
});
});
Expand Down Expand Up @@ -749,12 +760,14 @@ describe('#delete', () => {
clientOpts.baseClient.delete.mockReturnValue(apiCallReturnValue as any);
const options = { namespace };
await expectSuccess(client.delete, { type, id, options });
expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(1);
expectAuditEvent('saved_object_delete', EventOutcome.UNKNOWN, { type, id });
});

test(`adds audit event when not successful`, async () => {
clientOpts.checkSavedObjectsPrivilegesAsCurrentUser.mockRejectedValue(new Error());
await expect(() => client.delete(type, id)).rejects.toThrow();
expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(1);
expectAuditEvent('saved_object_delete', EventOutcome.FAILURE, { type, id });
});
});
Expand Down Expand Up @@ -883,6 +896,7 @@ describe('#find', () => {
clientOpts.baseClient.find.mockReturnValue(apiCallReturnValue as any);
const options = Object.freeze({ type: type1, namespaces: ['some-ns'] });
await expectSuccess(client.find, { options });
expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(2);
expectAuditEvent('saved_object_find', EventOutcome.SUCCESS, obj1);
expectAuditEvent('saved_object_find', EventOutcome.SUCCESS, obj2);
});
Expand All @@ -892,6 +906,7 @@ describe('#find', () => {
getMockCheckPrivilegesFailure
);
await client.find({ type: type1 });
expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(1);
expectAuditEvent('saved_object_find', EventOutcome.FAILURE);
});
});
Expand Down Expand Up @@ -934,12 +949,14 @@ describe('#get', () => {
clientOpts.baseClient.get.mockReturnValue(apiCallReturnValue as any);
const options = { namespace };
await expectSuccess(client.get, { type, id, options });
expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(1);
expectAuditEvent('saved_object_get', EventOutcome.SUCCESS, { type, id });
});

test(`adds audit event when not successful`, async () => {
clientOpts.checkSavedObjectsPrivilegesAsCurrentUser.mockRejectedValue(new Error());
await expect(() => client.get(type, id, { namespace })).rejects.toThrow();
expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(1);
expectAuditEvent('saved_object_get', EventOutcome.FAILURE, { type, id });
});
});
Expand Down Expand Up @@ -1018,12 +1035,14 @@ describe('#deleteFromNamespaces', () => {
const apiCallReturnValue = Symbol();
clientOpts.baseClient.deleteFromNamespaces.mockReturnValue(apiCallReturnValue as any);
await client.deleteFromNamespaces(type, id, namespaces);
expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(1);
expectAuditEvent('saved_object_delete_from_spaces', EventOutcome.UNKNOWN, { type, id });
});

test(`adds audit event when not successful`, async () => {
clientOpts.checkSavedObjectsPrivilegesAsCurrentUser.mockRejectedValue(new Error());
await expect(() => client.deleteFromNamespaces(type, id, namespaces)).rejects.toThrow();
expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(1);
expectAuditEvent('saved_object_delete_from_spaces', EventOutcome.FAILURE, { type, id });
});
});
Expand Down Expand Up @@ -1067,12 +1086,14 @@ describe('#update', () => {
clientOpts.baseClient.update.mockReturnValue(apiCallReturnValue as any);
const options = { namespace };
await expectSuccess(client.update, { type, id, attributes, options });
expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(1);
expectAuditEvent('saved_object_update', EventOutcome.UNKNOWN, { type, id });
});

test(`adds audit event when not successful`, async () => {
clientOpts.checkSavedObjectsPrivilegesAsCurrentUser.mockRejectedValue(new Error());
await expect(() => client.update(type, id, attributes, { namespace })).rejects.toThrow();
expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(1);
expectAuditEvent('saved_object_update', EventOutcome.FAILURE, { type, id });
});
});
Expand Down

0 comments on commit ccb42d4

Please sign in to comment.