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

[7.x] Used SO for saving the API key IDs that should be deleted (#82211) #83547

Merged
merged 1 commit into from
Nov 17, 2020
Merged

[7.x] Used SO for saving the API key IDs that should be deleted (#82211) #83547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
View file
Open in desktop
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
1 change: 0 additions & 1 deletion x-pack/plugins/alerts/server/alerts_client/tests/aggregate.test.ts
View file
Open in desktop
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
View file
Open in desktop
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: '123',
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: '123',
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
View file
Open in desktop
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: '123',
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: '123',
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: '123',
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: '123',
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