Skip to content
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

Add alerting route to update API key #45925

Merged
merged 10 commits into from
Sep 20, 2019
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const createAlertsClientMock = () => {
update: jest.fn(),
enable: jest.fn(),
disable: jest.fn(),
updateApiKey: jest.fn(),
};
return mocked;
};
Expand Down
600 changes: 325 additions & 275 deletions x-pack/legacy/plugins/alerting/server/alerts_client.test.ts

Large diffs are not rendered by default.

29 changes: 29 additions & 0 deletions x-pack/legacy/plugins/alerting/server/alerts_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import Boom from 'boom';
import { omit } from 'lodash';
import { SavedObjectsClientContract, SavedObjectReference } from 'src/core/server';
import { Alert, RawAlert, AlertTypeRegistry, AlertAction, Log } from './types';
Expand Down Expand Up @@ -215,6 +216,34 @@ export class AlertsClient {
return this.getAlertFromRaw(id, updatedObject.attributes, updatedObject.references);
}

public async updateApiKey({ id }: { id: string }) {
const {
references,
attributes: { enabled },
} = await this.savedObjectsClient.get('alert', id);

if (enabled === false) {
throw Boom.badRequest(`Can't update API key on a disabled alert`);
mikecote marked this conversation as resolved.
Show resolved Hide resolved
}

const apiKey = await this.createAPIKey();
const username = await this.getUserName();
await this.savedObjectsClient.update(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So wondering here if we also want to invalidate the API key that was there previously. Seems like we're just going to leave behind a trail of unused but still valid API keys.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YES, we should definitely do that. Although, is there some chance an old one could be available - like cached on a different Kibana instance? Seems possible. Which would imply so check to see when using an api key if we get back a "key not found" response, and use that as a cache invaldation signal (re-read the key).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a card for api key cleanup (https://github.com/elastic/kibana/projects/26#card-25354998). The problem that arises is we don't know if any scheduled actions rely on the old api key. To solve this we would need to change a few things and we were planning to do so under that card.

'alert',
id,
{
updatedBy: username,
apiKeyOwner: apiKey.created ? username : null,
apiKey: apiKey.created
? Buffer.from(`${apiKey.result.id}:${apiKey.result.api_key}`).toString('base64')
: null,
},
{
references,
}
);
}

public async enable({ id }: { id: string }) {
const existingObject = await this.savedObjectsClient.get('alert', id);
if (existingObject.attributes.enabled === false) {
Expand Down
2 changes: 2 additions & 0 deletions x-pack/legacy/plugins/alerting/server/init.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
updateAlertRoute,
enableAlertRoute,
disableAlertRoute,
updateApiKeyRoute,
} from './routes';

// Extend PluginProperties to indicate which plugins are guaranteed to exist
Expand Down Expand Up @@ -125,6 +126,7 @@ export function init(server: Server) {
updateAlertRoute(server);
enableAlertRoute(server);
disableAlertRoute(server);
updateApiKeyRoute(server);

// Expose functions
server.decorate('request', 'getAlertsClient', function() {
Expand Down
1 change: 1 addition & 0 deletions x-pack/legacy/plugins/alerting/server/routes/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,4 @@ export { listAlertTypesRoute } from './list_alert_types';
export { updateAlertRoute } from './update';
export { enableAlertRoute } from './enable';
export { disableAlertRoute } from './disable';
export { updateApiKeyRoute } from './update_api_key';
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { createMockServer } from './_mock_server';
import { updateApiKeyRoute } from './update_api_key';

const { server, alertsClient } = createMockServer();
updateApiKeyRoute(server);

mikecote marked this conversation as resolved.
Show resolved Hide resolved
test('updates api key for an alert', async () => {
const request = {
method: 'POST',
url: '/api/alert/1/_update_api_key',
};

const { statusCode } = await server.inject(request);
expect(statusCode).toBe(204);
expect(alertsClient.updateApiKey).toHaveBeenCalledWith({ id: '1' });
});
25 changes: 25 additions & 0 deletions x-pack/legacy/plugins/alerting/server/routes/update_api_key.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import Hapi from 'hapi';

export function updateApiKeyRoute(server: Hapi.Server) {
server.route({
method: 'POST',
path: '/api/alert/{id}/_update_api_key',
options: {
tags: ['access:alerting-all'],
response: {
emptyStatusCode: 204,
},
},
async handler(request: Hapi.Request, h: Hapi.ResponseToolkit) {
const alertsClient = request.getAlertsClient!();
await alertsClient.updateApiKey({ id: request.params.id });
return h.response();
},
});
}
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,13 @@ const Space2: Space = {
disabledFeatures: [],
};

export const Spaces: Space[] = [Space1, Space2];
const OtherSpace: Space = {
id: 'other',
name: 'Other',
disabledFeatures: [],
};

export const Spaces: Space[] = [Space1, Space2, OtherSpace];

// For all scenarios, we define both an instance in addition
// to a "type" definition so that we can use the exhaustive switch in
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export default function alertingTests({ loadTestFile }: FtrProviderContext) {
loadTestFile(require.resolve('./get'));
loadTestFile(require.resolve('./list_alert_types'));
loadTestFile(require.resolve('./update'));
loadTestFile(require.resolve('./update_api_key'));
loadTestFile(require.resolve('./alerts'));
});
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import expect from '@kbn/expect';
import { getTestAlertData } from './utils';
import { UserAtSpaceScenarios, Spaces } from '../../scenarios';
import { getUrlPrefix, ObjectRemover } from '../../../common/lib';
import { FtrProviderContext } from '../../../common/ftr_provider_context';

// eslint-disable-next-line import/no-default-export
export default function createUpdateApiKeyTests({ getService }: FtrProviderContext) {
const supertest = getService('supertest');
const supertestWithoutAuth = getService('supertestWithoutAuth');

describe('update_api_key', () => {
const objectRemover = new ObjectRemover(supertest);
const OtherSpace = Spaces.find(space => space.id === 'other');

if (!OtherSpace) {
throw new Error('Space "other" not defined in scenarios');
}

after(() => objectRemover.removeAll());

for (const scenario of UserAtSpaceScenarios) {
const { user, space } = scenario;
describe(scenario.id, () => {
it('should handle update alert api key request appropriately', async () => {
const { body: createdAlert } = await supertest
.post(`${getUrlPrefix(space.id)}/api/alert`)
.set('kbn-xsrf', 'foo')
.send(getTestAlertData())
.expect(200);
objectRemover.add(space.id, createdAlert.id, 'alert');

const response = await supertestWithoutAuth
.post(`${getUrlPrefix(space.id)}/api/alert/${createdAlert.id}/_update_api_key`)
.set('kbn-xsrf', 'foo')
.auth(user.username, user.password);

switch (scenario.id) {
case 'no_kibana_privileges at space1':
case 'space_1_all at space2':
case 'global_read at space1':
expect(response.statusCode).to.eql(404);
expect(response.body).to.eql({
statusCode: 404,
error: 'Not Found',
message: 'Not Found',
});
break;
case 'superuser at space1':
case 'space_1_all at space1':
expect(response.statusCode).to.eql(204);
expect(response.body).to.eql('');
const { body: updatedAlert } = await supertestWithoutAuth
.get(`${getUrlPrefix(space.id)}/api/alert/${createdAlert.id}`)
.set('kbn-xsrf', 'foo')
.auth(user.username, user.password)
.expect(200);
expect(updatedAlert.apiKeyOwner).to.eql(user.username);
break;
default:
throw new Error(`Scenario untested: ${JSON.stringify(scenario)}`);
}
});

it(`should handle update alert api key on a disabled alert appropriately`, async () => {
const { body: createdAlert } = await supertest
.post(`${getUrlPrefix(space.id)}/api/alert`)
.set('kbn-xsrf', 'foo')
.send(getTestAlertData({ enabled: false }))
.expect(200);
objectRemover.add(space.id, createdAlert.id, 'alert');

const response = await supertestWithoutAuth
.post(`${getUrlPrefix(space.id)}/api/alert/${createdAlert.id}/_update_api_key`)
.set('kbn-xsrf', 'foo')
.auth(user.username, user.password);

switch (scenario.id) {
case 'no_kibana_privileges at space1':
case 'space_1_all at space2':
case 'global_read at space1':
expect(response.statusCode).to.eql(404);
expect(response.body).to.eql({
statusCode: 404,
error: 'Not Found',
message: 'Not Found',
});
break;
case 'superuser at space1':
case 'space_1_all at space1':
expect(response.statusCode).to.eql(400);
expect(response.body).to.eql({
statusCode: 400,
error: 'Bad Request',
message: `Can't update API key on a disabled alert`,
});
break;
default:
throw new Error(`Scenario untested: ${JSON.stringify(scenario)}`);
}
});

it(`shouldn't update alert api key from another space`, async () => {
const { body: createdAlert } = await supertest
.post(`${getUrlPrefix(space.id)}/api/alert`)
.set('kbn-xsrf', 'foo')
.send(getTestAlertData())
.expect(200);
objectRemover.add(space.id, createdAlert.id, 'alert');

const response = await supertestWithoutAuth
.post(`${getUrlPrefix(OtherSpace.id)}/api/alert/${createdAlert.id}/_update_api_key`)
.set('kbn-xsrf', 'foo')
.auth(user.username, user.password);

expect(response.statusCode).to.eql(404);
switch (scenario.id) {
case 'no_kibana_privileges at space1':
case 'space_1_all at space2':
case 'global_read at space1':
case 'space_1_all at space1':
expect(response.body).to.eql({
statusCode: 404,
error: 'Not Found',
message: 'Not Found',
});
break;
case 'superuser at space1':
expect(response.body).to.eql({
statusCode: 404,
error: 'Not Found',
message: `Saved object [alert/${createdAlert.id}] not found`,
});
break;
default:
throw new Error(`Scenario untested: ${JSON.stringify(scenario)}`);
}
});
});
}
});
}
7 changes: 7 additions & 0 deletions x-pack/test/alerting_api_integration/spaces_only/scenarios.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@ const Space1: Space = {
disabledFeatures: [],
};

const Other: Space = {
id: 'other',
name: 'Other',
disabledFeatures: [],
};

export const Spaces = {
space1: Space1,
other: Other,
};
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export default function alertingTests({ loadTestFile }: FtrProviderContext) {
loadTestFile(require.resolve('./get'));
loadTestFile(require.resolve('./list_alert_types'));
loadTestFile(require.resolve('./update'));
loadTestFile(require.resolve('./update_api_key'));
loadTestFile(require.resolve('./alerts'));
});
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import expect from '@kbn/expect';
import { getTestAlertData } from './utils';
import { Spaces } from '../../scenarios';
import { getUrlPrefix, ObjectRemover } from '../../../common/lib';
import { FtrProviderContext } from '../../../common/ftr_provider_context';

/**
* Eventhough security is disabled, this test checks the API behavior.
*/

// eslint-disable-next-line import/no-default-export
export default function createUpdateApiKeyTests({ getService }: FtrProviderContext) {
const supertest = getService('supertest');

describe('update_api_key', () => {
const objectRemover = new ObjectRemover(supertest);

after(() => objectRemover.removeAll());

it('should handle update alert api key appropriately', async () => {
const { body: createdAlert } = await supertest
.post(`${getUrlPrefix(Spaces.space1.id)}/api/alert`)
.set('kbn-xsrf', 'foo')
.send(getTestAlertData())
.expect(200);
objectRemover.add(Spaces.space1.id, createdAlert.id, 'alert');

await supertest
.post(`${getUrlPrefix(Spaces.space1.id)}/api/alert/${createdAlert.id}/_update_api_key`)
.set('kbn-xsrf', 'foo')
.expect(204, '');

const { body: updatedAlert } = await supertest
.get(`${getUrlPrefix(Spaces.space1.id)}/api/alert/${createdAlert.id}`)
.set('kbn-xsrf', 'foo')
.expect(200);
expect(updatedAlert.apiKeyOwner).to.eql(null);
});

it(`shouldn't update alert api key from another space`, async () => {
const { body: createdAlert } = await supertest
.post(`${getUrlPrefix(Spaces.space1.id)}/api/alert`)
.set('kbn-xsrf', 'foo')
.send(getTestAlertData())
.expect(200);
objectRemover.add(Spaces.space1.id, createdAlert.id, 'alert');

await supertest
.post(`${getUrlPrefix(Spaces.other.id)}/api/alert/${createdAlert.id}/_update_api_key`)
.set('kbn-xsrf', 'foo')
.expect(404, {
statusCode: 404,
error: 'Not Found',
message: `Saved object [alert/${createdAlert.id}] not found`,
});
});
});
}