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

Retain APIKey when disabling/enabling a rule #131581

Merged
merged 24 commits into from
May 18, 2022
Merged

Retain APIKey when disabling/enabling a rule #131581

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
329f1de
Retain APIKey when disabling/enabling a rule
ersin-erdal May 3, 2022
9b9717d
Merge branch 'main' of https://github.com/elastic/kibana into 131234-…
ersin-erdal May 4, 2022
4c4b6c8
Merge branch 'main' into 131234-retain-api-key
ersin-erdal May 5, 2022
76b3afe
fix failing test
ersin-erdal May 5, 2022
ceb4e26
Merge remote-tracking branch 'origin/131234-retain-api-key' into 1312…
ersin-erdal May 5, 2022
ad84a32
Fix failing test
ersin-erdal May 5, 2022
487dfbf
Translations
ersin-erdal May 6, 2022
7c5cbd9
Add delete and Update API Key buttons to rule details page
ersin-erdal May 9, 2022
267abf4
Remove Refresh button from the dropdown
ersin-erdal May 9, 2022
94b7a3a
revert rules list
ersin-erdal May 9, 2022
8df86ac
merge
ersin-erdal May 9, 2022
7e49f56
Merge branch 'main' of https://github.com/elastic/kibana into 131234-…
ersin-erdal May 9, 2022
c38f350
fix type check error
ersin-erdal May 9, 2022
b485ff8
Add enable/disable rule buttons
ersin-erdal May 10, 2022
446003e
Use correct mock data in enable rule test
ersin-erdal May 10, 2022
fba81a9
Update meta when disable a rule,
ersin-erdal May 11, 2022
ef30e4e
Update error message
ersin-erdal May 11, 2022
92d4dce
Copy and styling fixes
ersin-erdal May 11, 2022
29168a7
Fix Delete modal text.
ersin-erdal May 11, 2022
72598c3
Merge branch 'main' of https://github.com/elastic/kibana into 131234-…
ersin-erdal May 16, 2022
d0c0dfc
Add functional test and use classname for styling
ersin-erdal May 16, 2022
0da7084
Update Auth docs
ersin-erdal May 16, 2022
521ef6e
use alerts_fixture in _get_api_key ep path
ersin-erdal May 17, 2022
5e87f3e
fix object remover
ersin-erdal May 17, 2022
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
3 changes: 1 addition & 2 deletions docs/user/alerting/alerting-setup.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,9 @@ Rules and connectors are isolated to the {kib} space in which they were created.
Rules are authorized using an <<api-keys,API key>> associated with the last user to edit the rule. This API key captures a snapshot of the user's privileges at the time of edit and is subsequently used to run all background tasks associated with the rule, including condition checks like {es} queries and triggered actions. The following rule actions will re-generate the API key:

* Creating a rule
* Enabling a disabled rule
* Updating a rule

[IMPORTANT]
==============================================
If a rule requires certain privileges, such as index privileges, to run, and a user without those privileges updates, disables, or re-enables the rule, the rule will no longer function. Conversely, if a user with greater or administrator privileges modifies the rule, it will begin running with increased privileges.
If a rule requires certain privileges, such as index privileges, to run, and a user without those privileges updates the rule, the rule will no longer function. Conversely, if a user with greater or administrator privileges modifies the rule, it will begin running with increased privileges.
==============================================
78 changes: 28 additions & 50 deletions x-pack/plugins/alerting/server/rules_client/rules_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1828,7 +1828,7 @@ export class RulesClient {
}

private async enableWithOCC({ id }: { id: string }) {
let apiKeyToInvalidate: string | null = null;
let existingApiKey: string | null = null;
let attributes: RawRule;
let version: string | undefined;

Expand All @@ -1837,14 +1837,11 @@ export class RulesClient {
await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser<RawRule>('alert', id, {
namespace: this.namespace,
});
apiKeyToInvalidate = decryptedAlert.attributes.apiKey;
existingApiKey = decryptedAlert.attributes.apiKey;
attributes = decryptedAlert.attributes;
version = decryptedAlert.version;
} catch (e) {
// We'll skip invalidating the API key since we failed to load the decrypted saved object
this.logger.error(
`enable(): Failed to load API key to invalidate on alert ${id}: ${e.message}`
);
this.logger.error(`enable(): Failed to load API key of alert ${id}: ${e.message}`);
// Still attempt to load the attributes and version using SOC
const alert = await this.unsecuredSavedObjectsClient.get<RawRule>('alert', id);
attributes = alert.attributes;
Expand Down Expand Up @@ -1886,19 +1883,10 @@ export class RulesClient {
if (attributes.enabled === false) {
const username = await this.getUserName();

let createdAPIKey = null;
try {
createdAPIKey = await this.createAPIKey(
this.generateAPIKeyName(attributes.alertTypeId, attributes.name)
);
} catch (error) {
throw Boom.badRequest(`Error enabling rule: could not create API key - ${error.message}`);
}

const updateAttributes = this.updateMeta({
...attributes,
...(!existingApiKey && (await this.createNewAPIKeySet({ attributes, username }))),
enabled: true,
...this.apiKeyAsAlertAttributes(createdAPIKey, username),
updatedBy: username,
updatedAt: new Date().toISOString(),
executionStatus: {
Expand All @@ -1909,15 +1897,10 @@ export class RulesClient {
warning: null,
},
});

try {
await this.unsecuredSavedObjectsClient.update('alert', id, updateAttributes, { version });
ersin-erdal marked this conversation as resolved.
Show resolved Hide resolved
} catch (e) {
// Avoid unused API key
await bulkMarkApiKeysForInvalidation(
{ apiKeys: updateAttributes.apiKey ? [updateAttributes.apiKey] : [] },
this.logger,
this.unsecuredSavedObjectsClient
);
throw e;
}
const scheduledTask = await this.scheduleRule({
Expand All @@ -1930,16 +1913,28 @@ export class RulesClient {
await this.unsecuredSavedObjectsClient.update('alert', id, {
scheduledTaskId: scheduledTask.id,
});
if (apiKeyToInvalidate) {
await bulkMarkApiKeysForInvalidation(
{ apiKeys: [apiKeyToInvalidate] },
this.logger,
this.unsecuredSavedObjectsClient
);
}
}
}

private async createNewAPIKeySet({
attributes,
username,
}: {
attributes: RawRule;
username: string | null;
}): Promise<Pick<RawRule, 'apiKey' | 'apiKeyOwner'>> {
let createdAPIKey = null;
try {
createdAPIKey = await this.createAPIKey(
this.generateAPIKeyName(attributes.alertTypeId, attributes.name)
);
} catch (error) {
throw Boom.badRequest(`Error creating API key for rule: ${error.message}`);
}

return this.apiKeyAsAlertAttributes(createdAPIKey, username);
}

public async disable({ id }: { id: string }): Promise<void> {
return await retryIfConflicts(
this.logger,
Expand All @@ -1949,7 +1944,6 @@ export class RulesClient {
}

private async disableWithOCC({ id }: { id: string }) {
let apiKeyToInvalidate: string | null = null;
let attributes: RawRule;
let version: string | undefined;

Expand All @@ -1958,14 +1952,10 @@ export class RulesClient {
await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser<RawRule>('alert', id, {
namespace: this.namespace,
});
apiKeyToInvalidate = decryptedAlert.attributes.apiKey;
attributes = decryptedAlert.attributes;
version = decryptedAlert.version;
} catch (e) {
// We'll skip invalidating the API key since we failed to load the decrypted saved object
this.logger.error(
`disable(): Failed to load API key to invalidate on alert ${id}: ${e.message}`
);
this.logger.error(`disable(): Failed to load API key of alert ${id}: ${e.message}`);
// Still attempt to load the attributes and version using SOC
const alert = await this.unsecuredSavedObjectsClient.get<RawRule>('alert', id);
attributes = alert.attributes;
mikecote marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -2058,26 +2048,14 @@ export class RulesClient {
...attributes,
enabled: false,
scheduledTaskId: null,
apiKey: null,
apiKeyOwner: null,
updatedBy: await this.getUserName(),
updatedAt: new Date().toISOString(),
}),
{ version }
);

await Promise.all([
attributes.scheduledTaskId
? this.taskManager.removeIfExists(attributes.scheduledTaskId)
: null,
apiKeyToInvalidate
? await bulkMarkApiKeysForInvalidation(
{ apiKeys: [apiKeyToInvalidate] },
this.logger,
this.unsecuredSavedObjectsClient
)
: null,
]);
if (attributes.scheduledTaskId) {
await this.taskManager.removeIfExists(attributes.scheduledTaskId);
}
}
}

Expand Down
67 changes: 11 additions & 56 deletions x-pack/plugins/alerting/server/rules_client/tests/disable.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import { auditLoggerMock } from '@kbn/security-plugin/server/audit/mocks';
import { getBeforeSetup, setGlobalDate } from './lib';
import { eventLoggerMock } from '@kbn/event-log-plugin/server/event_logger.mock';
import { TaskStatus } from '@kbn/task-manager-plugin/server';
import { bulkMarkApiKeysForInvalidation } from '../../invalidate_pending_api_keys/bulk_mark_api_keys_for_invalidation';

jest.mock('../../invalidate_pending_api_keys/bulk_mark_api_keys_for_invalidation', () => ({
bulkMarkApiKeysForInvalidation: jest.fn(),
Expand Down Expand Up @@ -111,6 +110,7 @@ describe('disable()', () => {
attributes: {
...existingAlert.attributes,
apiKey: Buffer.from('123:abc').toString('base64'),
apiKeyOwner: 'elastic',
},
version: '123',
references: [],
Expand Down Expand Up @@ -206,11 +206,11 @@ describe('disable()', () => {
alertTypeId: 'myType',
enabled: false,
meta: {
ersin-erdal marked this conversation as resolved.
Show resolved Hide resolved
versionApiKeyLastmodified: kibanaVersion,
versionApiKeyLastmodified: 'v7.10.0',
},
scheduledTaskId: null,
apiKey: null,
apiKeyOwner: null,
apiKey: 'MTIzOmFiYw==',
apiKeyOwner: 'elastic',
updatedAt: '2019-02-12T21:01:22.479Z',
updatedBy: 'elastic',
actions: [
Expand All @@ -230,12 +230,6 @@ describe('disable()', () => {
}
);
expect(taskManager.removeIfExists).toHaveBeenCalledWith('task-123');
expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledTimes(1);
expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledWith(
{ apiKeys: ['MTIzOmFiYw=='] },
expect.any(Object),
expect.any(Object)
);
});

test('disables the rule with calling event log to "recover" the alert instances from the task state', async () => {
Expand Down Expand Up @@ -282,11 +276,11 @@ describe('disable()', () => {
alertTypeId: 'myType',
enabled: false,
meta: {
versionApiKeyLastmodified: kibanaVersion,
versionApiKeyLastmodified: 'v7.10.0',
},
scheduledTaskId: null,
apiKey: null,
apiKeyOwner: null,
apiKey: 'MTIzOmFiYw==',
apiKeyOwner: 'elastic',
updatedAt: '2019-02-12T21:01:22.479Z',
updatedBy: 'elastic',
actions: [
Expand All @@ -306,12 +300,6 @@ describe('disable()', () => {
}
);
expect(taskManager.removeIfExists).toHaveBeenCalledWith('task-123');
expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledTimes(1);
expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledWith(
{ apiKeys: ['MTIzOmFiYw=='] },
expect.any(Object),
expect.any(Object)
);

expect(eventLogger.logEvent).toHaveBeenCalledTimes(1);
expect(eventLogger.logEvent.mock.calls[0][0]).toStrictEqual({
Expand Down Expand Up @@ -369,11 +357,11 @@ describe('disable()', () => {
alertTypeId: 'myType',
enabled: false,
meta: {
versionApiKeyLastmodified: kibanaVersion,
versionApiKeyLastmodified: 'v7.10.0',
},
scheduledTaskId: null,
apiKey: null,
apiKeyOwner: null,
apiKey: 'MTIzOmFiYw==',
ersin-erdal marked this conversation as resolved.
Show resolved Hide resolved
apiKeyOwner: 'elastic',
updatedAt: '2019-02-12T21:01:22.479Z',
updatedBy: 'elastic',
actions: [
Expand All @@ -393,12 +381,6 @@ describe('disable()', () => {
}
);
expect(taskManager.removeIfExists).toHaveBeenCalledWith('task-123');
expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledTimes(1);
expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledWith(
{ apiKeys: ['MTIzOmFiYw=='] },
expect.any(Object),
expect.any(Object)
);

expect(eventLogger.logEvent).toHaveBeenCalledTimes(0);
expect(rulesClientParams.logger.warn).toHaveBeenCalledWith(
Expand All @@ -408,7 +390,6 @@ describe('disable()', () => {

test('falls back when getDecryptedAsInternalUser throws an error', async () => {
encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValueOnce(new Error('Fail'));

await rulesClient.disable({ id: '1' });
expect(unsecuredSavedObjectsClient.get).toHaveBeenCalledWith('alert', '1');
expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', {
Expand All @@ -422,12 +403,7 @@ describe('disable()', () => {
schedule: { interval: '10s' },
alertTypeId: 'myType',
enabled: false,
meta: {
versionApiKeyLastmodified: kibanaVersion,
},
scheduledTaskId: null,
apiKey: null,
apiKeyOwner: null,
updatedAt: '2019-02-12T21:01:22.479Z',
updatedBy: 'elastic',
actions: [
Expand All @@ -447,7 +423,6 @@ describe('disable()', () => {
}
);
expect(taskManager.removeIfExists).toHaveBeenCalledWith('task-123');
expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
});

test(`doesn't disable already disabled alerts`, async () => {
Expand All @@ -463,14 +438,6 @@ describe('disable()', () => {
await rulesClient.disable({ id: '1' });
expect(unsecuredSavedObjectsClient.update).not.toHaveBeenCalled();
expect(taskManager.removeIfExists).not.toHaveBeenCalled();
expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
});

test(`doesn't invalidate when no API key is used`, async () => {
encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce(existingAlert);

await rulesClient.disable({ id: '1' });
expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
});

test('swallows error when failing to load decrypted saved object', async () => {
Expand All @@ -479,9 +446,8 @@ describe('disable()', () => {
await rulesClient.disable({ id: '1' });
expect(unsecuredSavedObjectsClient.update).toHaveBeenCalled();
expect(taskManager.removeIfExists).toHaveBeenCalled();
expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
expect(rulesClientParams.logger.error).toHaveBeenCalledWith(
'disable(): Failed to load API key to invalidate on alert 1: Fail'
'disable(): Failed to load API key of alert 1: Fail'
);
});

Expand All @@ -493,17 +459,6 @@ describe('disable()', () => {
);
});

test('swallows error when invalidate API key throws', async () => {
unsecuredSavedObjectsClient.create.mockRejectedValueOnce(new Error('Fail'));
await rulesClient.disable({ id: '1' });
expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledTimes(1);
expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledWith(
{ apiKeys: ['MTIzOmFiYw=='] },
expect.any(Object),
expect.any(Object)
);
});

test('throws when failing to remove task from task manager', async () => {
taskManager.removeIfExists.mockRejectedValueOnce(new Error('Failed to remove task'));

Expand Down
Loading