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.

Sign up for GitHub

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

Don't create API key for disabled alerts when calling create API #57041

Merged
merged 5 commits into from
Feb 10, 2020
Merged

Don't create API key for disabled alerts when calling create API #57041

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2b43dfd
Don't create API key for disabled alerts when calling create API
mikecote Feb 6, 2020
31a28c1
Merge branch 'master' of github.com:elastic/kibana into alerting/api-…
mikecote Feb 7, 2020
cfb7dd6
Fix failing test
mikecote Feb 7, 2020
36d83c3
Merge branch 'master' of github.com:elastic/kibana into alerting/api-…
mikecote Feb 10, 2020
31ead6d
Remove unused code in test
mikecote Feb 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
181 changes: 125 additions & 56 deletions x-pack/legacy/plugins/alerting/server/alerts_client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,15 +79,20 @@ function getMockData(overwrites: Record<string, any> = {}) {
}

describe('create()', () => {
test('creates an alert', async () => {
const alertsClient = new AlertsClient(alertsClientParams);
const data = getMockData();
alertTypeRegistry.get.mockReturnValueOnce({
let alertsClient: AlertsClient;

beforeEach(() => {
alertsClient = new AlertsClient(alertsClientParams);
alertTypeRegistry.get.mockReturnValue({
id: '123',
name: 'Test',
actionGroups: ['default'],
async executor() {},
});
});

test('creates an alert', async () => {
const data = getMockData();
savedObjectsClient.bulkGet.mockResolvedValueOnce({
saved_objects: [
{
Expand Down Expand Up @@ -263,7 +268,6 @@ describe('create()', () => {
});

test('creates an alert with multiple actions', async () => {
const alertsClient = new AlertsClient(alertsClientParams);
const data = getMockData({
actions: [
{
Expand All @@ -289,12 +293,6 @@ describe('create()', () => {
},
],
});
alertTypeRegistry.get.mockReturnValueOnce({
id: '123',
name: 'Test',
actionGroups: ['default'],
async executor() {},
});
savedObjectsClient.bulkGet.mockResolvedValueOnce({
saved_objects: [
{
Expand Down Expand Up @@ -446,14 +444,7 @@ describe('create()', () => {
});

test('creates a disabled alert', async () => {
const alertsClient = new AlertsClient(alertsClientParams);
const data = getMockData({ enabled: false });
alertTypeRegistry.get.mockReturnValueOnce({
id: '123',
name: 'Test',
actionGroups: ['default'],
async executor() {},
});
savedObjectsClient.bulkGet.mockResolvedValueOnce({
saved_objects: [
{
Expand Down Expand Up @@ -527,9 +518,8 @@ describe('create()', () => {
});

test('should validate params', async () => {
const alertsClient = new AlertsClient(alertsClientParams);
const data = getMockData();
alertTypeRegistry.get.mockReturnValueOnce({
alertTypeRegistry.get.mockReturnValue({
id: '123',
name: 'Test',
actionGroups: [],
Expand All @@ -547,14 +537,7 @@ describe('create()', () => {
});

test('throws error if loading actions fails', async () => {
const alertsClient = new AlertsClient(alertsClientParams);
const data = getMockData();
alertTypeRegistry.get.mockReturnValueOnce({
id: '123',
name: 'Test',
actionGroups: ['default'],
async executor() {},
});
savedObjectsClient.bulkGet.mockRejectedValueOnce(new Error('Test Error'));
await expect(alertsClient.create({ data })).rejects.toThrowErrorMatchingInlineSnapshot(
`"Test Error"`
Expand All @@ -564,14 +547,7 @@ describe('create()', () => {
});

test('throws error if create saved object fails', async () => {
const alertsClient = new AlertsClient(alertsClientParams);
const data = getMockData();
alertTypeRegistry.get.mockReturnValueOnce({
id: '123',
name: 'Test',
actionGroups: ['default'],
async executor() {},
});
savedObjectsClient.bulkGet.mockResolvedValueOnce({
saved_objects: [
{
Expand All @@ -592,14 +568,7 @@ describe('create()', () => {
});

test('attempts to remove saved object if scheduling failed', async () => {
const alertsClient = new AlertsClient(alertsClientParams);
const data = getMockData();
alertTypeRegistry.get.mockReturnValueOnce({
id: '123',
name: 'Test',
actionGroups: ['default'],
async executor() {},
});
savedObjectsClient.bulkGet.mockResolvedValueOnce({
saved_objects: [
{
Expand Down Expand Up @@ -655,14 +624,7 @@ describe('create()', () => {
});

test('returns task manager error if cleanup fails, logs to console', async () => {
const alertsClient = new AlertsClient(alertsClientParams);
const data = getMockData();
alertTypeRegistry.get.mockReturnValueOnce({
id: '123',
name: 'Test',
actionGroups: ['default'],
async executor() {},
});
savedObjectsClient.bulkGet.mockResolvedValueOnce({
saved_objects: [
{
Expand Down Expand Up @@ -714,7 +676,6 @@ describe('create()', () => {
});

test('throws an error if alert type not registerd', async () => {
const alertsClient = new AlertsClient(alertsClientParams);
const data = getMockData();
alertTypeRegistry.get.mockImplementation(() => {
throw new Error('Invalid type');
Expand All @@ -725,14 +686,7 @@ describe('create()', () => {
});

test('calls the API key function', async () => {
const alertsClient = new AlertsClient(alertsClientParams);
const data = getMockData();
alertTypeRegistry.get.mockReturnValueOnce({
id: '123',
name: 'Test',
actionGroups: ['default'],
async executor() {},
});
alertsClientParams.createAPIKey.mockResolvedValueOnce({
apiKeysEnabled: true,
result: { id: '123', api_key: 'abc' },
Expand Down Expand Up @@ -845,6 +799,121 @@ describe('create()', () => {
}
);
});

test(`doesn't create API key for disabled alerts`, async () => {
const data = getMockData({ enabled: false });
alertsClientParams.createAPIKey.mockResolvedValueOnce({
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is needed, given

expect(alertsClientParams.createAPIKey).not.toHaveBeenCalled();

But I also suspect it could be useful if we ever start calling this, so ... it's fine. Perhaps a comment that it's not expected this will actually get called ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! I went ahead and removed it here: 31ead6d and maybe in a future refactor this code will move into a beforeEach for the ones that do call that function.

apiKeysEnabled: true,
result: { id: '123', api_key: 'abc' },
});
savedObjectsClient.bulkGet.mockResolvedValueOnce({
saved_objects: [
{
id: '1',
type: 'action',
attributes: {
actionTypeId: 'test',
},
references: [],
},
],
});
savedObjectsClient.create.mockResolvedValueOnce({
id: '1',
type: 'alert',
attributes: {
alertTypeId: '123',
schedule: { interval: '10s' },
params: {
bar: true,
},
actions: [
{
group: 'default',
actionRef: 'action_0',
actionTypeId: 'test',
params: {
foo: true,
},
},
],
},
references: [
{
name: 'action_0',
type: 'action',
id: '1',
},
],
});
taskManager.schedule.mockResolvedValueOnce({
id: 'task-123',
taskType: 'alerting:123',
scheduledAt: new Date(),
attempts: 1,
status: TaskStatus.Idle,
runAt: new Date(),
startedAt: null,
retryAt: null,
state: {},
params: {},
ownerId: null,
});
savedObjectsClient.update.mockResolvedValueOnce({
id: '1',
type: 'alert',
attributes: {
scheduledTaskId: 'task-123',
},
references: [
{
id: '1',
name: 'action_0',
type: 'action',
},
],
});
await alertsClient.create({ data });

expect(alertsClientParams.createAPIKey).not.toHaveBeenCalled();
expect(savedObjectsClient.create).toHaveBeenCalledWith(
'alert',
{
actions: [
{
actionRef: 'action_0',
group: 'default',
actionTypeId: 'test',
params: { foo: true },
},
],
alertTypeId: '123',
consumer: 'bar',
name: 'abc',
params: { bar: true },
apiKey: null,
apiKeyOwner: null,
createdBy: 'elastic',
createdAt: '2019-02-12T21:01:22.479Z',
updatedBy: 'elastic',
enabled: false,
schedule: { interval: '10s' },
throttle: null,
muteAll: false,
mutedInstanceIds: [],
tags: ['foo'],
},
{
references: [
{
id: '1',
name: 'action_0',
type: 'action',
},
],
}
);
});
});

describe('enable()', () => {
Expand Down
7 changes: 4 additions & 3 deletions x-pack/legacy/plugins/alerting/server/alerts_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,13 +151,14 @@ export class AlertsClient {
const alertType = this.alertTypeRegistry.get(data.alertTypeId);
const validatedAlertTypeParams = validateAlertTypeParams(alertType, data.params);
const username = await this.getUserName();
const createdAPIKey = data.enabled ? await this.createAPIKey() : null;

this.validateActions(alertType, data.actions);

const { references, actions } = await this.denormalizeActions(data.actions);
const rawAlert: RawAlert = {
...data,
...this.apiKeyAsAlertAttributes(await this.createAPIKey(), username),
...this.apiKeyAsAlertAttributes(createdAPIKey, username),
actions,
createdBy: username,
updatedBy: username,
Expand Down Expand Up @@ -317,10 +318,10 @@ export class AlertsClient {
}

private apiKeyAsAlertAttributes(
apiKey: CreateAPIKeyResult,
apiKey: CreateAPIKeyResult | null,
username: string | null
): Pick<RawAlert, 'apiKey' | 'apiKeyOwner'> {
return apiKey.apiKeysEnabled
return apiKey && apiKey.apiKeysEnabled
? {
apiKeyOwner: username,
apiKey: Buffer.from(`${apiKey.result.id}:${apiKey.result.api_key}`).toString('base64'),
Expand Down
2 changes: 1 addition & 1 deletion x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/find.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ export default function createFindTests({ getService }: FtrProviderContext) {
createdBy: 'elastic',
throttle: '1m',
updatedBy: 'elastic',
apiKeyOwner: 'elastic',
apiKeyOwner: null,
muteAll: false,
mutedInstanceIds: [],
createdAt: match.createdAt,
Expand Down