Skip to content

Commit 24e48a9

Browse files
YulNaumenkoYulNaumenko
committed
Used SO for saving the API key IDs that should be deleted (#82211)
* Used SO for saving the API key IDs that should be deleted and create a configuration option where can set an execution interval for a TM task which will get the data from this SO and remove marked for delete keys. * removed invalidateApiKey from AlertsClient * Fixed type checks * Fixed jest tests * Removed test code * Changed SO name * fixed type cheks * Moved invalidate logic out of alerts client * fixed type check * Added functional tests * Fixed due to comments * added configurable delay for invalidation task * added interval to the task response * Fixed jest tests * Fixed due to comments * Fixed task * fixed paging * Fixed date filter * Fixed jest tests * fixed due to comments * fixed due to comments * Fixed e2e test * Fixed e2e test * Fixed due to comments. Changed api key invalidation task to use SavedObjectClient * Use encryptedSavedObjectClient * set back flaky test comment
1 parent 3a29b3b commit 24e48a9

36 files changed

+847
-126
lines changed

‎x-pack/plugins/alerts/server/alerts_client/alerts_client.ts‎

Lines changed: 50 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ import {
3131
} from '../types';
3232
import { validateAlertTypeParams, alertExecutionStatusFromRaw } from '../lib';
3333
import {
34-
InvalidateAPIKeyParams,
3534
GrantAPIKeyResult as SecurityPluginGrantAPIKeyResult,
3635
InvalidateAPIKeyResult as SecurityPluginInvalidateAPIKeyResult,
3736
} from '../../../security/server';
@@ -48,6 +47,7 @@ import { IEvent } from '../../../event_log/server';
4847
import { parseDuration } from '../../common/parse_duration';
4948
import { retryIfConflicts } from '../lib/retry_if_conflicts';
5049
import { partiallyUpdateAlert } from '../saved_objects';
50+
import { markApiKeyForInvalidation } from '../invalidate_pending_api_keys/mark_api_key_for_invalidation';
5151

5252
export interface RegistryAlertTypeWithAuth extends RegistryAlertType {
5353
authorizedConsumers: string[];
@@ -72,7 +72,6 @@ export interface ConstructorOptions {
7272
namespace?: string;
7373
getUserName: () => Promise<string | null>;
7474
createAPIKey: (name: string) => Promise<CreateAPIKeyResult>;
75-
invalidateAPIKey: (params: InvalidateAPIKeyParams) => Promise<InvalidateAPIKeyResult>;
7675
getActionsClient: () => Promise<ActionsClient>;
7776
getEventLogClient: () => Promise<IEventLogClient>;
7877
kibanaVersion: PluginInitializerContext['env']['packageInfo']['version'];
@@ -172,9 +171,6 @@ export class AlertsClient {
172171
private readonly authorization: AlertsAuthorization;
173172
private readonly alertTypeRegistry: AlertTypeRegistry;
174173
private readonly createAPIKey: (name: string) => Promise<CreateAPIKeyResult>;
175-
private readonly invalidateAPIKey: (
176-
params: InvalidateAPIKeyParams
177-
) => Promise<InvalidateAPIKeyResult>;
178174
private readonly getActionsClient: () => Promise<ActionsClient>;
179175
private readonly actionsAuthorization: ActionsAuthorization;
180176
private readonly getEventLogClient: () => Promise<IEventLogClient>;
@@ -191,7 +187,6 @@ export class AlertsClient {
191187
namespace,
192188
getUserName,
193189
createAPIKey,
194-
invalidateAPIKey,
195190
encryptedSavedObjectsClient,
196191
getActionsClient,
197192
actionsAuthorization,
@@ -207,7 +202,6 @@ export class AlertsClient {
207202
this.unsecuredSavedObjectsClient = unsecuredSavedObjectsClient;
208203
this.authorization = authorization;
209204
this.createAPIKey = createAPIKey;
210-
this.invalidateAPIKey = invalidateAPIKey;
211205
this.encryptedSavedObjectsClient = encryptedSavedObjectsClient;
212206
this.getActionsClient = getActionsClient;
213207
this.actionsAuthorization = actionsAuthorization;
@@ -263,7 +257,11 @@ export class AlertsClient {
263257
);
264258
} catch (e) {
265259
// Avoid unused API key
266-
this.invalidateApiKey({ apiKey: rawAlert.apiKey });
260+
markApiKeyForInvalidation(
261+
{ apiKey: rawAlert.apiKey },
262+
this.logger,
263+
this.unsecuredSavedObjectsClient
264+
);
267265
throw e;
268266
}
269267
if (data.enabled) {
@@ -487,7 +485,13 @@ export class AlertsClient {
487485

488486
await Promise.all([
489487
taskIdToRemove ? deleteTaskIfItExists(this.taskManager, taskIdToRemove) : null,
490-
apiKeyToInvalidate ? this.invalidateApiKey({ apiKey: apiKeyToInvalidate }) : null,
488+
apiKeyToInvalidate
489+
? markApiKeyForInvalidation(
490+
{ apiKey: apiKeyToInvalidate },
491+
this.logger,
492+
this.unsecuredSavedObjectsClient
493+
)
494+
: null,
491495
]);
492496

493497
return removeResult;
@@ -526,7 +530,11 @@ export class AlertsClient {
526530

527531
await Promise.all([
528532
alertSavedObject.attributes.apiKey
529-
? this.invalidateApiKey({ apiKey: alertSavedObject.attributes.apiKey })
533+
? markApiKeyForInvalidation(
534+
{ apiKey: alertSavedObject.attributes.apiKey },
535+
this.logger,
536+
this.unsecuredSavedObjectsClient
537+
)
530538
: null,
531539
(async () => {
532540
if (
@@ -591,7 +599,11 @@ export class AlertsClient {
591599
);
592600
} catch (e) {
593601
// Avoid unused API key
594-
this.invalidateApiKey({ apiKey: createAttributes.apiKey });
602+
markApiKeyForInvalidation(
603+
{ apiKey: createAttributes.apiKey },
604+
this.logger,
605+
this.unsecuredSavedObjectsClient
606+
);
595607
throw e;
596608
}
597609

@@ -671,28 +683,20 @@ export class AlertsClient {
671683
await this.unsecuredSavedObjectsClient.update('alert', id, updateAttributes, { version });
672684
} catch (e) {
673685
// Avoid unused API key
674-
this.invalidateApiKey({ apiKey: updateAttributes.apiKey });
686+
markApiKeyForInvalidation(
687+
{ apiKey: updateAttributes.apiKey },
688+
this.logger,
689+
this.unsecuredSavedObjectsClient
690+
);
675691
throw e;
676692
}
677693

678694
if (apiKeyToInvalidate) {
679-
await this.invalidateApiKey({ apiKey: apiKeyToInvalidate });
680-
}
681-
}
682-
683-
private async invalidateApiKey({ apiKey }: { apiKey: string | null }): Promise<void> {
684-
if (!apiKey) {
685-
return;
686-
}
687-
688-
try {
689-
const apiKeyId = Buffer.from(apiKey, 'base64').toString().split(':')[0];
690-
const response = await this.invalidateAPIKey({ id: apiKeyId });
691-
if (response.apiKeysEnabled === true && response.result.error_count > 0) {
692-
this.logger.error(`Failed to invalidate API Key [id="${apiKeyId}"]`);
693-
}
694-
} catch (e) {
695-
this.logger.error(`Failed to invalidate API Key: ${e.message}`);
695+
await markApiKeyForInvalidation(
696+
{ apiKey: apiKeyToInvalidate },
697+
this.logger,
698+
this.unsecuredSavedObjectsClient
699+
);
696700
}
697701
}
698702

@@ -752,7 +756,11 @@ export class AlertsClient {
752756
await this.unsecuredSavedObjectsClient.update('alert', id, updateAttributes, { version });
753757
} catch (e) {
754758
// Avoid unused API key
755-
this.invalidateApiKey({ apiKey: updateAttributes.apiKey });
759+
markApiKeyForInvalidation(
760+
{ apiKey: updateAttributes.apiKey },
761+
this.logger,
762+
this.unsecuredSavedObjectsClient
763+
);
756764
throw e;
757765
}
758766
const scheduledTask = await this.scheduleAlert(
@@ -764,7 +772,11 @@ export class AlertsClient {
764772
scheduledTaskId: scheduledTask.id,
765773
});
766774
if (apiKeyToInvalidate) {
767-
await this.invalidateApiKey({ apiKey: apiKeyToInvalidate });
775+
await markApiKeyForInvalidation(
776+
{ apiKey: apiKeyToInvalidate },
777+
this.logger,
778+
this.unsecuredSavedObjectsClient
779+
);
768780
}
769781
}
770782
}
@@ -825,7 +837,13 @@ export class AlertsClient {
825837
attributes.scheduledTaskId
826838
? deleteTaskIfItExists(this.taskManager, attributes.scheduledTaskId)
827839
: null,
828-
apiKeyToInvalidate ? this.invalidateApiKey({ apiKey: apiKeyToInvalidate }) : null,
840+
apiKeyToInvalidate
841+
? await markApiKeyForInvalidation(
842+
{ apiKey: apiKeyToInvalidate },
843+
this.logger,
844+
this.unsecuredSavedObjectsClient
845+
)
846+
: null,
829847
]);
830848
}
831849
}

‎x-pack/plugins/alerts/server/alerts_client/tests/aggregate.test.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@ const alertsClientParams: jest.Mocked<ConstructorOptions> = {
3434
namespace: 'default',
3535
getUserName: jest.fn(),
3636
createAPIKey: jest.fn(),
37-
invalidateAPIKey: jest.fn(),
3837
logger: loggingSystemMock.create().get(),
3938
encryptedSavedObjectsClient: encryptedSavedObjects,
4039
getActionsClient: jest.fn(),

‎x-pack/plugins/alerts/server/alerts_client/tests/create.test.ts‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@ const alertsClientParams: jest.Mocked<ConstructorOptions> = {
3434
namespace: 'default',
3535
getUserName: jest.fn(),
3636
createAPIKey: jest.fn(),
37-
invalidateAPIKey: jest.fn(),
3837
logger: loggingSystemMock.create().get(),
3938
encryptedSavedObjectsClient: encryptedSavedObjects,
4039
getActionsClient: jest.fn(),
@@ -711,7 +710,7 @@ describe('create()', () => {
711710
expect(taskManager.schedule).not.toHaveBeenCalled();
712711
});
713712

714-
test('throws error and invalidates API key when create saved object fails', async () => {
713+
test('throws error and add API key to invalidatePendingApiKey SO when create saved object fails', async () => {
715714
const data = getMockData();
716715
alertsClientParams.createAPIKey.mockResolvedValueOnce({
717716
apiKeysEnabled: true,
@@ -731,11 +730,25 @@ describe('create()', () => {
731730
],
732731
});
733732
unsecuredSavedObjectsClient.create.mockRejectedValueOnce(new Error('Test failure'));
733+
const createdAt = new Date().toISOString();
734+
unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
735+
id: '1',
736+
type: 'api_key_pending_invalidation',
737+
attributes: {
738+
apiKeyId: '123',
739+
createdAt,
740+
},
741+
references: [],
742+
});
734743
await expect(alertsClient.create({ data })).rejects.toThrowErrorMatchingInlineSnapshot(
735744
`"Test failure"`
736745
);
737746
expect(taskManager.schedule).not.toHaveBeenCalled();
738-
expect(alertsClientParams.invalidateAPIKey).toHaveBeenCalledWith({ id: '123' });
747+
expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(2);
748+
expect(unsecuredSavedObjectsClient.create.mock.calls[1][1]).toStrictEqual({
749+
apiKeyId: '123',
750+
createdAt,
751+
});
739752
});
740753

741754
test('attempts to remove saved object if scheduling failed', async () => {

‎x-pack/plugins/alerts/server/alerts_client/tests/delete.test.ts‎

Lines changed: 47 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@ const alertsClientParams: jest.Mocked<ConstructorOptions> = {
3232
namespace: 'default',
3333
getUserName: jest.fn(),
3434
createAPIKey: jest.fn(),
35-
invalidateAPIKey: jest.fn(),
3635
logger: loggingSystemMock.create().get(),
3736
encryptedSavedObjectsClient: encryptedSavedObjects,
3837
getActionsClient: jest.fn(),
@@ -94,11 +93,22 @@ describe('delete()', () => {
9493
});
9594

9695
test('successfully removes an alert', async () => {
96+
unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
97+
id: '1',
98+
type: 'api_key_pending_invalidation',
99+
attributes: {
100+
apiKeyId: '123',
101+
createdAt: '2019-02-12T21:01:22.479Z',
102+
},
103+
references: [],
104+
});
97105
const result = await alertsClient.delete({ id: '1' });
98106
expect(result).toEqual({ success: true });
99107
expect(unsecuredSavedObjectsClient.delete).toHaveBeenCalledWith('alert', '1');
100108
expect(taskManager.remove).toHaveBeenCalledWith('task-123');
101-
expect(alertsClientParams.invalidateAPIKey).toHaveBeenCalledWith({ id: '123' });
109+
expect(unsecuredSavedObjectsClient.create.mock.calls[0][0]).toBe(
110+
'api_key_pending_invalidation'
111+
);
102112
expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', {
103113
namespace: 'default',
104114
});
@@ -107,12 +117,21 @@ describe('delete()', () => {
107117

108118
test('falls back to SOC.get when getDecryptedAsInternalUser throws an error', async () => {
109119
encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValue(new Error('Fail'));
120+
unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
121+
id: '1',
122+
type: 'api_key_pending_invalidation',
123+
attributes: {
124+
apiKeyId: '123',
125+
createdAt: '2019-02-12T21:01:22.479Z',
126+
},
127+
references: [],
128+
});
110129

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

135154
test(`doesn't invalidate API key when apiKey is null`, async () => {
155+
unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
156+
id: '1',
157+
type: 'api_key_pending_invalidation',
158+
attributes: {
159+
apiKeyId: '123',
160+
createdAt: '2019-02-12T21:01:22.479Z',
161+
},
162+
references: [],
163+
});
136164
encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue({
137165
...existingAlert,
138166
attributes: {
@@ -142,24 +170,34 @@ describe('delete()', () => {
142170
});
143171

144172
await alertsClient.delete({ id: '1' });
145-
expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled();
173+
expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
146174
});
147175

148176
test('swallows error when invalidate API key throws', async () => {
149-
alertsClientParams.invalidateAPIKey.mockRejectedValueOnce(new Error('Fail'));
150-
177+
unsecuredSavedObjectsClient.create.mockRejectedValueOnce(new Error('Fail'));
151178
await alertsClient.delete({ id: '1' });
152-
expect(alertsClientParams.invalidateAPIKey).toHaveBeenCalledWith({ id: '123' });
179+
expect(unsecuredSavedObjectsClient.create.mock.calls[0][0]).toBe(
180+
'api_key_pending_invalidation'
181+
);
153182
expect(alertsClientParams.logger.error).toHaveBeenCalledWith(
154-
'Failed to invalidate API Key: Fail'
183+
'Failed to mark for API key [id="MTIzOmFiYw=="] for invalidation: Fail'
155184
);
156185
});
157186

158187
test('swallows error when getDecryptedAsInternalUser throws an error', async () => {
188+
unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
189+
id: '1',
190+
type: 'api_key_pending_invalidation',
191+
attributes: {
192+
apiKeyId: '123',
193+
createdAt: '2019-02-12T21:01:22.479Z',
194+
},
195+
references: [],
196+
});
159197
encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValue(new Error('Fail'));
160198

161199
await alertsClient.delete({ id: '1' });
162-
expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled();
200+
expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
163201
expect(alertsClientParams.logger.error).toHaveBeenCalledWith(
164202
'delete(): Failed to load API key to invalidate on alert 1: Fail'
165203
);

0 commit comments

Comments
 (0)