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

Used SO for saving the API key IDs that should be deleted #82211

Merged
Merged
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
04ba851
Used SO for saving the API key IDs that should be deleted and create …
YulNaumenko Oct 30, 2020
8c149fc
removed invalidateApiKey from AlertsClient
YulNaumenko Nov 2, 2020
edb0461
Merge remote-tracking branch 'upstream/master' into alerts-invalidate…
YulNaumenko Nov 2, 2020
c4afdfe
Fixed type checks
YulNaumenko Nov 2, 2020
5a0d5c2
Fixed jest tests
YulNaumenko Nov 2, 2020
077c927
Removed test code
YulNaumenko Nov 2, 2020
60edfb3
Changed SO name
YulNaumenko Nov 2, 2020
19c3b74
fixed type cheks
YulNaumenko Nov 2, 2020
1a6e08a
Merge remote-tracking branch 'upstream/master' into alerts-invalidate…
YulNaumenko Nov 4, 2020
7d11d22
Moved invalidate logic out of alerts client
YulNaumenko Nov 4, 2020
7c45205
fixed type check
YulNaumenko Nov 4, 2020
4d5d67d
Added functional tests
YulNaumenko Nov 4, 2020
53d5064
Fixed due to comments
YulNaumenko Nov 6, 2020
47b4737
Merge remote-tracking branch 'upstream/master' into alerts-invalidate…
YulNaumenko Nov 6, 2020
247fe90
added configurable delay for invalidation task
YulNaumenko Nov 9, 2020
121625c
added interval to the task response
YulNaumenko Nov 9, 2020
d677168
Fixed jest tests
YulNaumenko Nov 9, 2020
3faf894
Fixed due to comments
YulNaumenko Nov 9, 2020
fda8b77
Merge remote-tracking branch 'upstream/master' into alerts-invalidate…
YulNaumenko Nov 9, 2020
56cba8e
Fixed task
YulNaumenko Nov 9, 2020
8dada64
fixed paging
YulNaumenko Nov 9, 2020
daad733
Fixed date filter
YulNaumenko Nov 10, 2020
53e0a20
Fixed jest tests
YulNaumenko Nov 10, 2020
719c378
Merged upstream
YulNaumenko Nov 10, 2020
c0f9f27
fixed due to comments
YulNaumenko Nov 10, 2020
86f4c1e
Merge remote-tracking branch 'upstream/master' into alerts-invalidate…
YulNaumenko Nov 10, 2020
5897597
fixed due to comments
YulNaumenko Nov 10, 2020
c5b14a1
Merge remote-tracking branch 'upstream/master' into alerts-invalidate…
YulNaumenko Nov 12, 2020
f706b5a
Fixed e2e test
YulNaumenko Nov 13, 2020
b8c1eae
Fixed e2e test
YulNaumenko Nov 13, 2020
871d2ef
Merge remote-tracking branch upstream/master
YulNaumenko Nov 16, 2020
4092f13
Fixed due to comments. Changed api key invalidation task to use Saved…
YulNaumenko Nov 16, 2020
1cc9d52
Use encryptedSavedObjectClient
YulNaumenko Nov 17, 2020
bd65fb3
set back flaky test comment
YulNaumenko Nov 17, 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
82 changes: 50 additions & 32 deletions x-pack/plugins/alerts/server/alerts_client/alerts_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import {
} from '../types';
import { validateAlertTypeParams, alertExecutionStatusFromRaw } from '../lib';
import {
InvalidateAPIKeyParams,
GrantAPIKeyResult as SecurityPluginGrantAPIKeyResult,
InvalidateAPIKeyResult as SecurityPluginInvalidateAPIKeyResult,
} from '../../../security/server';
Expand All @@ -48,6 +47,7 @@ import { IEvent } from '../../../event_log/server';
import { parseDuration } from '../../common/parse_duration';
import { retryIfConflicts } from '../lib/retry_if_conflicts';
import { partiallyUpdateAlert } from '../saved_objects';
import { markApiKeyForInvalidation } from '../invalidate_pending_api_keys/mark_api_key_for_invalidation';

export interface RegistryAlertTypeWithAuth extends RegistryAlertType {
authorizedConsumers: string[];
Expand All @@ -72,7 +72,6 @@ export interface ConstructorOptions {
namespace?: string;
getUserName: () => Promise<string | null>;
createAPIKey: (name: string) => Promise<CreateAPIKeyResult>;
invalidateAPIKey: (params: InvalidateAPIKeyParams) => Promise<InvalidateAPIKeyResult>;
getActionsClient: () => Promise<ActionsClient>;
getEventLogClient: () => Promise<IEventLogClient>;
kibanaVersion: PluginInitializerContext['env']['packageInfo']['version'];
Expand Down Expand Up @@ -172,9 +171,6 @@ export class AlertsClient {
private readonly authorization: AlertsAuthorization;
private readonly alertTypeRegistry: AlertTypeRegistry;
private readonly createAPIKey: (name: string) => Promise<CreateAPIKeyResult>;
private readonly invalidateAPIKey: (
params: InvalidateAPIKeyParams
) => Promise<InvalidateAPIKeyResult>;
private readonly getActionsClient: () => Promise<ActionsClient>;
private readonly actionsAuthorization: ActionsAuthorization;
private readonly getEventLogClient: () => Promise<IEventLogClient>;
Expand All @@ -191,7 +187,6 @@ export class AlertsClient {
namespace,
getUserName,
createAPIKey,
invalidateAPIKey,
encryptedSavedObjectsClient,
getActionsClient,
actionsAuthorization,
Expand All @@ -207,7 +202,6 @@ export class AlertsClient {
this.unsecuredSavedObjectsClient = unsecuredSavedObjectsClient;
this.authorization = authorization;
this.createAPIKey = createAPIKey;
this.invalidateAPIKey = invalidateAPIKey;
this.encryptedSavedObjectsClient = encryptedSavedObjectsClient;
this.getActionsClient = getActionsClient;
this.actionsAuthorization = actionsAuthorization;
Expand Down Expand Up @@ -263,7 +257,11 @@ export class AlertsClient {
);
} catch (e) {
// Avoid unused API key
this.invalidateApiKey({ apiKey: rawAlert.apiKey });
markApiKeyForInvalidation(
{ apiKey: rawAlert.apiKey },
this.logger,
this.unsecuredSavedObjectsClient
);
throw e;
}
if (data.enabled) {
Expand Down Expand Up @@ -487,7 +485,13 @@ export class AlertsClient {

await Promise.all([
taskIdToRemove ? deleteTaskIfItExists(this.taskManager, taskIdToRemove) : null,
apiKeyToInvalidate ? this.invalidateApiKey({ apiKey: apiKeyToInvalidate }) : null,
apiKeyToInvalidate
? markApiKeyForInvalidation(
{ apiKey: apiKeyToInvalidate },
this.logger,
this.unsecuredSavedObjectsClient
)
: null,
]);

return removeResult;
Expand Down Expand Up @@ -526,7 +530,11 @@ export class AlertsClient {

await Promise.all([
alertSavedObject.attributes.apiKey
? this.invalidateApiKey({ apiKey: alertSavedObject.attributes.apiKey })
? markApiKeyForInvalidation(
{ apiKey: alertSavedObject.attributes.apiKey },
this.logger,
this.unsecuredSavedObjectsClient
)
: null,
(async () => {
if (
Expand Down Expand Up @@ -591,7 +599,11 @@ export class AlertsClient {
);
} catch (e) {
// Avoid unused API key
this.invalidateApiKey({ apiKey: createAttributes.apiKey });
markApiKeyForInvalidation(
{ apiKey: createAttributes.apiKey },
this.logger,
this.unsecuredSavedObjectsClient
);
throw e;
}

Expand Down Expand Up @@ -671,28 +683,20 @@ export class AlertsClient {
await this.unsecuredSavedObjectsClient.update('alert', id, updateAttributes, { version });
} catch (e) {
// Avoid unused API key
this.invalidateApiKey({ apiKey: updateAttributes.apiKey });
markApiKeyForInvalidation(
{ apiKey: updateAttributes.apiKey },
this.logger,
this.unsecuredSavedObjectsClient
);
throw e;
}

if (apiKeyToInvalidate) {
await this.invalidateApiKey({ apiKey: apiKeyToInvalidate });
}
}

private async invalidateApiKey({ apiKey }: { apiKey: string | null }): Promise<void> {
if (!apiKey) {
return;
}

try {
const apiKeyId = Buffer.from(apiKey, 'base64').toString().split(':')[0];
const response = await this.invalidateAPIKey({ id: apiKeyId });
if (response.apiKeysEnabled === true && response.result.error_count > 0) {
this.logger.error(`Failed to invalidate API Key [id="${apiKeyId}"]`);
}
} catch (e) {
this.logger.error(`Failed to invalidate API Key: ${e.message}`);
await markApiKeyForInvalidation(
{ apiKey: apiKeyToInvalidate },
this.logger,
this.unsecuredSavedObjectsClient
);
}
}

Expand Down Expand Up @@ -752,7 +756,11 @@ export class AlertsClient {
await this.unsecuredSavedObjectsClient.update('alert', id, updateAttributes, { version });
} catch (e) {
// Avoid unused API key
this.invalidateApiKey({ apiKey: updateAttributes.apiKey });
markApiKeyForInvalidation(
{ apiKey: updateAttributes.apiKey },
this.logger,
this.unsecuredSavedObjectsClient
);
throw e;
}
const scheduledTask = await this.scheduleAlert(
Expand All @@ -764,7 +772,11 @@ export class AlertsClient {
scheduledTaskId: scheduledTask.id,
});
if (apiKeyToInvalidate) {
await this.invalidateApiKey({ apiKey: apiKeyToInvalidate });
await markApiKeyForInvalidation(
{ apiKey: apiKeyToInvalidate },
this.logger,
this.unsecuredSavedObjectsClient
);
}
}
}
Expand Down Expand Up @@ -825,7 +837,13 @@ export class AlertsClient {
attributes.scheduledTaskId
? deleteTaskIfItExists(this.taskManager, attributes.scheduledTaskId)
: null,
apiKeyToInvalidate ? this.invalidateApiKey({ apiKey: apiKeyToInvalidate }) : null,
apiKeyToInvalidate
? await markApiKeyForInvalidation(
{ apiKey: apiKeyToInvalidate },
this.logger,
this.unsecuredSavedObjectsClient
)
: null,
]);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ const alertsClientParams: jest.Mocked<ConstructorOptions> = {
namespace: 'default',
getUserName: jest.fn(),
createAPIKey: jest.fn(),
invalidateAPIKey: jest.fn(),
logger: loggingSystemMock.create().get(),
encryptedSavedObjectsClient: encryptedSavedObjects,
getActionsClient: jest.fn(),
Expand Down
19 changes: 16 additions & 3 deletions x-pack/plugins/alerts/server/alerts_client/tests/create.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ const alertsClientParams: jest.Mocked<ConstructorOptions> = {
namespace: 'default',
getUserName: jest.fn(),
createAPIKey: jest.fn(),
invalidateAPIKey: jest.fn(),
logger: loggingSystemMock.create().get(),
encryptedSavedObjectsClient: encryptedSavedObjects,
getActionsClient: jest.fn(),
Expand Down Expand Up @@ -711,7 +710,7 @@ describe('create()', () => {
expect(taskManager.schedule).not.toHaveBeenCalled();
});

test('throws error and invalidates API key when create saved object fails', async () => {
test('throws error and add API key to invalidatePendingApiKey SO when create saved object fails', async () => {
const data = getMockData();
alertsClientParams.createAPIKey.mockResolvedValueOnce({
apiKeysEnabled: true,
Expand All @@ -731,11 +730,25 @@ describe('create()', () => {
],
});
unsecuredSavedObjectsClient.create.mockRejectedValueOnce(new Error('Test failure'));
const createdAt = new Date().toISOString();
unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
id: '1',
type: 'api_key_pending_invalidation',
attributes: {
apiKeyId: 'MTIz',
createdAt,
},
references: [],
});
await expect(alertsClient.create({ data })).rejects.toThrowErrorMatchingInlineSnapshot(
`"Test failure"`
);
expect(taskManager.schedule).not.toHaveBeenCalled();
expect(alertsClientParams.invalidateAPIKey).toHaveBeenCalledWith({ id: '123' });
expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(2);
expect(unsecuredSavedObjectsClient.create.mock.calls[1][1]).toStrictEqual({
apiKeyId: 'MTIz',
createdAt,
});
});

test('attempts to remove saved object if scheduling failed', async () => {
Expand Down
56 changes: 47 additions & 9 deletions x-pack/plugins/alerts/server/alerts_client/tests/delete.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ const alertsClientParams: jest.Mocked<ConstructorOptions> = {
namespace: 'default',
getUserName: jest.fn(),
createAPIKey: jest.fn(),
invalidateAPIKey: jest.fn(),
logger: loggingSystemMock.create().get(),
encryptedSavedObjectsClient: encryptedSavedObjects,
getActionsClient: jest.fn(),
Expand Down Expand Up @@ -94,11 +93,22 @@ describe('delete()', () => {
});

test('successfully removes an alert', async () => {
unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
id: '1',
type: 'api_key_pending_invalidation',
attributes: {
apiKeyId: 'MTIz',
createdAt: '2019-02-12T21:01:22.479Z',
},
references: [],
});
const result = await alertsClient.delete({ id: '1' });
expect(result).toEqual({ success: true });
expect(unsecuredSavedObjectsClient.delete).toHaveBeenCalledWith('alert', '1');
expect(taskManager.remove).toHaveBeenCalledWith('task-123');
expect(alertsClientParams.invalidateAPIKey).toHaveBeenCalledWith({ id: '123' });
expect(unsecuredSavedObjectsClient.create.mock.calls[0][0]).toBe(
'api_key_pending_invalidation'
);
expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', {
namespace: 'default',
});
Expand All @@ -107,12 +117,21 @@ describe('delete()', () => {

test('falls back to SOC.get when getDecryptedAsInternalUser throws an error', async () => {
encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValue(new Error('Fail'));
unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
id: '1',
type: 'api_key_pending_invalidation',
attributes: {
apiKeyId: 'MTIz',
createdAt: '2019-02-12T21:01:22.479Z',
},
references: [],
});

const result = await alertsClient.delete({ id: '1' });
expect(result).toEqual({ success: true });
expect(unsecuredSavedObjectsClient.delete).toHaveBeenCalledWith('alert', '1');
expect(taskManager.remove).toHaveBeenCalledWith('task-123');
expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled();
expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
expect(unsecuredSavedObjectsClient.get).toHaveBeenCalledWith('alert', '1');
expect(alertsClientParams.logger.error).toHaveBeenCalledWith(
'delete(): Failed to load API key to invalidate on alert 1: Fail'
Expand All @@ -133,6 +152,15 @@ describe('delete()', () => {
});

test(`doesn't invalidate API key when apiKey is null`, async () => {
unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
id: '1',
type: 'api_key_pending_invalidation',
attributes: {
apiKeyId: 'MTIz',
createdAt: '2019-02-12T21:01:22.479Z',
},
references: [],
});
encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue({
...existingAlert,
attributes: {
Expand All @@ -142,24 +170,34 @@ describe('delete()', () => {
});

await alertsClient.delete({ id: '1' });
expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled();
expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
});

test('swallows error when invalidate API key throws', async () => {
alertsClientParams.invalidateAPIKey.mockRejectedValueOnce(new Error('Fail'));

unsecuredSavedObjectsClient.create.mockRejectedValueOnce(new Error('Fail'));
await alertsClient.delete({ id: '1' });
expect(alertsClientParams.invalidateAPIKey).toHaveBeenCalledWith({ id: '123' });
expect(unsecuredSavedObjectsClient.create.mock.calls[0][0]).toBe(
'api_key_pending_invalidation'
);
expect(alertsClientParams.logger.error).toHaveBeenCalledWith(
'Failed to invalidate API Key: Fail'
'Failed to mark for API key [id="MTIzOmFiYw=="] for invalidation: Fail'
);
});

test('swallows error when getDecryptedAsInternalUser throws an error', async () => {
unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
id: '1',
type: 'api_key_pending_invalidation',
attributes: {
apiKeyId: 'MTIz',
createdAt: '2019-02-12T21:01:22.479Z',
},
references: [],
});
encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValue(new Error('Fail'));

await alertsClient.delete({ id: '1' });
expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled();
expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
expect(alertsClientParams.logger.error).toHaveBeenCalledWith(
'delete(): Failed to load API key to invalidate on alert 1: Fail'
);
Expand Down
Loading