Skip to content

Commit 0a34da4

Browse files
vitaliidmvitaliidm
committed
revisist of API keys invalidation
1 parent 2ca3afc commit 0a34da4

2 files changed

Lines changed: 141 additions & 3 deletions

File tree

‎x-pack/plugins/alerting/server/rules_client/rules_client.ts‎

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1520,6 +1520,7 @@ export class RulesClient {
15201520
const rules: Array<SavedObjectsBulkUpdateObject<RawRule>> = [];
15211521
const errors: BulkEditError[] = [];
15221522
const apiKeysToInvalidate: string[] = [];
1523+
const apiKeysMap = new Map<string, { oldApiKey?: string; newApiKey?: string }>();
15231524
const username = await this.getUserName();
15241525

15251526
for await (const response of rulesFinder.find()) {
@@ -1528,7 +1529,7 @@ export class RulesClient {
15281529
async (rule) => {
15291530
try {
15301531
if (rule.attributes.apiKey) {
1531-
apiKeysToInvalidate.push(rule.attributes.apiKey);
1532+
apiKeysMap.set(rule.id, { oldApiKey: rule.attributes.apiKey });
15321533
}
15331534

15341535
const ruleType = this.ruleTypeRegistry.get(rule.attributes.alertTypeId);
@@ -1602,8 +1603,17 @@ export class RulesClient {
16021603
} catch (error) {
16031604
throw Error(`Error updating rule: could not create API key - ${error.message}`);
16041605
}
1606+
16051607
const apiKeyAttributes = this.apiKeyAsAlertAttributes(createdAPIKey, username);
16061608

1609+
// collect generated API keys
1610+
if (apiKeyAttributes.apiKey) {
1611+
apiKeysMap.set(rule.id, {
1612+
...apiKeysMap.get(rule.id),
1613+
newApiKey: apiKeyAttributes.apiKey,
1614+
});
1615+
}
1616+
16071617
// get notifyWhen
16081618
const notifyWhen = getRuleNotifyWhenType(
16091619
attributes.notifyWhen,
@@ -1652,7 +1662,41 @@ export class RulesClient {
16521662
);
16531663
}
16541664

1655-
const result = await this.unsecuredSavedObjectsClient.bulkUpdate(rules);
1665+
let result;
1666+
try {
1667+
result = await this.unsecuredSavedObjectsClient.bulkUpdate(rules);
1668+
} catch (e) {
1669+
// avoid unused newly generated API keys
1670+
if (apiKeysMap.size > 0) {
1671+
await bulkMarkApiKeysForInvalidation(
1672+
{
1673+
apiKeys: Array.from(apiKeysMap.values()).reduce<string[]>((acc, value) => {
1674+
if (value.newApiKey) {
1675+
acc.push(value.newApiKey);
1676+
}
1677+
return acc;
1678+
}, []),
1679+
},
1680+
this.logger,
1681+
this.unsecuredSavedObjectsClient
1682+
);
1683+
}
1684+
throw e;
1685+
}
1686+
1687+
result.saved_objects.map(({ id, error }) => {
1688+
const oldApiKey = apiKeysMap.get(id)?.oldApiKey;
1689+
const newApiKey = apiKeysMap.get(id)?.newApiKey;
1690+
1691+
// if SO wasn't saved and has new API key it will be invalidated
1692+
if (error && newApiKey) {
1693+
apiKeysToInvalidate.push(newApiKey);
1694+
// if SO saved and has old Api Key it will be invalidate
1695+
} else if (!error && oldApiKey) {
1696+
apiKeysToInvalidate.push(oldApiKey);
1697+
}
1698+
});
1699+
16561700
return { apiKeysToInvalidate, resultSavedObjects: result.saved_objects, errors, rules };
16571701
}
16581702

‎x-pack/plugins/alerting/server/rules_client/tests/bulk_edit.test.ts‎

Lines changed: 95 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ const actionsAuthorization = actionsAuthorizationMock.create();
3333
const auditLogger = auditLoggerMock.create();
3434

3535
const kibanaVersion = 'v8.2.0';
36+
const createAPIKeyMock = jest.fn();
3637
const rulesClientParams: jest.Mocked<ConstructorOptions> = {
3738
taskManager,
3839
ruleTypeRegistry,
@@ -42,7 +43,7 @@ const rulesClientParams: jest.Mocked<ConstructorOptions> = {
4243
spaceId: 'default',
4344
namespace: 'default',
4445
getUserName: jest.fn(),
45-
createAPIKey: jest.fn(),
46+
createAPIKey: createAPIKeyMock,
4647
logger: loggingSystemMock.create().get(),
4748
encryptedSavedObjectsClient: encryptedSavedObjects,
4849
getActionsClient: jest.fn(),
@@ -581,6 +582,99 @@ describe('bulkEdit()', () => {
581582
);
582583
});
583584

585+
test('should call bulkMarkApiKeysForInvalidation to invalidate unused keys if bulkUpdate failed', async () => {
586+
createAPIKeyMock.mockReturnValue({ apiKeysEnabled: true, result: { api_key: '111' } });
587+
mockCreatePointInTimeFinderAsInternalUser({
588+
saved_objects: [
589+
{
590+
...existingDecryptedRule,
591+
attributes: { ...existingDecryptedRule.attributes, enabled: true },
592+
},
593+
],
594+
});
595+
596+
unsecuredSavedObjectsClient.bulkUpdate.mockImplementation(() => {
597+
throw new Error('Fail');
598+
});
599+
600+
await expect(
601+
rulesClient.bulkEdit({
602+
filter: 'alert.attributes.tags: "APM"',
603+
operations: [
604+
{
605+
field: 'tags',
606+
operation: 'add',
607+
value: ['test-1'],
608+
},
609+
],
610+
})
611+
).rejects.toThrow('Fail');
612+
613+
expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledTimes(1);
614+
expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledWith(
615+
{ apiKeys: ['dW5kZWZpbmVkOjExMQ=='] },
616+
expect.any(Object),
617+
expect.any(Object)
618+
);
619+
});
620+
621+
test('should call bulkMarkApiKeysForInvalidation to invalidate unused keys if SO update failed', async () => {
622+
createAPIKeyMock.mockReturnValue({ apiKeysEnabled: true, result: { api_key: '111' } });
623+
mockCreatePointInTimeFinderAsInternalUser({
624+
saved_objects: [
625+
{
626+
...existingDecryptedRule,
627+
attributes: { ...existingDecryptedRule.attributes, enabled: true },
628+
},
629+
],
630+
});
631+
632+
unsecuredSavedObjectsClient.bulkUpdate.mockResolvedValue({
633+
saved_objects: [
634+
{
635+
id: '1',
636+
type: 'alert',
637+
attributes: {
638+
enabled: true,
639+
tags: ['foo'],
640+
alertTypeId: 'myType',
641+
schedule: { interval: '1m' },
642+
consumer: 'myApp',
643+
scheduledTaskId: 'task-123',
644+
params: { index: ['test-index-*'] },
645+
throttle: null,
646+
notifyWhen: null,
647+
actions: [],
648+
},
649+
references: [],
650+
version: '123',
651+
error: {
652+
error: 'test failure',
653+
statusCode: 409,
654+
message: 'test failure',
655+
},
656+
},
657+
],
658+
});
659+
660+
await rulesClient.bulkEdit({
661+
filter: 'alert.attributes.tags: "APM"',
662+
operations: [
663+
{
664+
field: 'tags',
665+
operation: 'add',
666+
value: ['test-1'],
667+
},
668+
],
669+
});
670+
671+
expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledWith(
672+
{ apiKeys: ['dW5kZWZpbmVkOjExMQ=='] },
673+
expect.any(Object),
674+
expect.any(Object)
675+
);
676+
});
677+
584678
test('should not call create apiKey if rule is disabled', async () => {
585679
await rulesClient.bulkEdit({
586680
filter: 'alert.attributes.tags: "APM"',

0 commit comments

Comments
 (0)