Skip to content

Commit

Permalink
[7.17] [ResponseOps] fix alerting_authorization_kuery to stop generat…
Browse files Browse the repository at this point in the history
…ing nulls (#133823) (#136075)

* [ResponseOps] fix alerting_authorization_kuery to stop generating nulls (#133823)

resolves #114536

We've seen a couple of situations where the KQL auth filter is generating
some garbage (null entries where KQL AST nodes are expected):

- issuing a _find HTTP API call against a non-existent Kibana space
- user has Connectors and Actions feature enabled, but nothing else

In these cases, there are no consumers available, and the KQL AST for
expressions like `consumer-field:(alerts or myApp or myOtherApp)` would
end up generating a `null` value instead of a KQL AST node.

The code was changed to not generate the null value.

This led to the next problem - the UX hung because it was waiting for a
health check that ended up throwing an authorization error.

We now catch that error and allow the health check to proceed.

(cherry picked from commit cb40727)

# Conflicts:
#	x-pack/plugins/triggers_actions_ui/public/application/components/health_check.tsx
#	x-pack/test/functional_with_es_ssl/config.ts

* fix jest test after backport merge conflict

* fix module references

* fix rules/alerts naming issue in test

* fix pretty error in test

Co-authored-by: Kibana Machine <[email protected]>
  • Loading branch information
pmuellr and kibanamachine authored Jul 11, 2022
1 parent 8e30bb8 commit 40c897a
Show file tree
Hide file tree
Showing 7 changed files with 191 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,35 @@ describe('asKqlFiltersByRuleTypeAndConsumer', () => {
)
);
});

test('constructs KQL filter for single rule type with no authorized consumer', async () => {
const result = asFiltersByRuleTypeAndConsumer(
new Set([
{
actionGroups: [],
defaultActionGroupId: 'default',
recoveryActionGroup: RecoveredActionGroup,
id: 'myAppAlertType',
name: 'myAppAlertType',
producer: 'myApp',
minimumLicenseRequired: 'basic',
isExportable: true,
authorizedConsumers: {},
enabledInLicense: true,
},
]),
{
type: AlertingAuthorizationFilterType.KQL,
fieldNames: {
ruleTypeId: 'path.to.rule_type_id',
consumer: 'consumer-field',
},
},
'space1'
);

expect(result).toEqual(esKuery.fromKueryExpression(`path.to.rule_type_id:myAppAlertType`));
});
});

describe('asEsDslFiltersByRuleTypeAndConsumer', () => {
Expand Down Expand Up @@ -604,6 +633,48 @@ describe('asEsDslFiltersByRuleTypeAndConsumer', () => {
},
});
});

test('constructs KQL filter for single rule type with no authorized consumer', async () => {
const result = asFiltersByRuleTypeAndConsumer(
new Set([
{
actionGroups: [],
defaultActionGroupId: 'default',
recoveryActionGroup: RecoveredActionGroup,
id: 'myAppAlertType',
name: 'myAppAlertType',
producer: 'myApp',
minimumLicenseRequired: 'basic',
isExportable: true,
authorizedConsumers: {},
enabledInLicense: true,
},
]),
{
type: AlertingAuthorizationFilterType.ESDSL,
fieldNames: {
ruleTypeId: 'path.to.rule_type_id',
consumer: 'consumer-field',
},
},
'space1'
);

expect(result).toMatchInlineSnapshot(`
Object {
"bool": Object {
"minimum_should_match": 1,
"should": Array [
Object {
"match": Object {
"path.to.rule_type_id": "myAppAlertType",
},
},
],
},
}
`);
});
});

describe('asFiltersBySpaceId', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,20 @@ export function asFiltersByRuleTypeAndConsumer(
const kueryNode = nodeBuilder.or(
Array.from(ruleTypes).reduce<KueryNode[]>((filters, { id, authorizedConsumers }) => {
ensureFieldIsSafeForQuery('ruleTypeId', id);
const andNodes = [
nodeBuilder.is(opts.fieldNames.ruleTypeId, id),
nodeBuilder.or(
Object.keys(authorizedConsumers).map((consumer) => {
ensureFieldIsSafeForQuery('consumer', consumer);
return nodeBuilder.is(opts.fieldNames.consumer, consumer);
})
),
];

const andNodes: KueryNode[] = [nodeBuilder.is(opts.fieldNames.ruleTypeId, id)];

const authorizedConsumersKeys = Object.keys(authorizedConsumers);
if (authorizedConsumersKeys.length) {
andNodes.push(
nodeBuilder.or(
authorizedConsumersKeys.map((consumer) => {
ensureFieldIsSafeForQuery('consumer', consumer);
return nodeBuilder.is(opts.fieldNames.consumer, consumer);
})
)
);
}

if (opts.fieldNames.spaceIds != null && spaceId != null) {
andNodes.push(nodeBuilder.is(opts.fieldNames.spaceIds, spaceId));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,4 +190,24 @@ describe('health check', () => {
`"https://www.elastic.co/guide/en/kibana/mocked-test-branch/alerting-setup.html#alerting-prerequisites"`
);
});

it('renders children and no warnings if error thrown getting alerting health', async () => {
useKibanaMock().services.http.get = jest
.fn()
// result from triggers_actions_ui health
.mockResolvedValueOnce({ isAlertsAvailable: true })
// result from alerting health
.mockRejectedValueOnce(new Error('for example, not authorized for rules / 403 response'));
const { queryByText } = render(
<HealthContextProvider>
<HealthCheck waitForCheck={true}>
<p>{'should render'}</p>
</HealthCheck>
</HealthContextProvider>
);
await act(async () => {
// wait for useEffect to run
});
expect(queryByText('should render')).toBeInTheDocument();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,16 @@
*/

import React from 'react';
import { Option, none, some, fold } from 'fp-ts/lib/Option';
import { Option, none, some, fold, isSome } from 'fp-ts/lib/Option';
import { pipe } from 'fp-ts/lib/pipeable';
import { FormattedMessage } from '@kbn/i18n/react';

import { EuiLink, EuiSpacer } from '@elastic/eui';
import { i18n } from '@kbn/i18n';

import { EuiEmptyPrompt } from '@elastic/eui';
import { DocLinksStart } from 'kibana/public';
import { DocLinksStart, HttpSetup } from 'kibana/public';
import { AlertingFrameworkHealth } from '../../../../alerting/common';
import './health_check.scss';
import { useHealthContext } from '../context/health_context';
import { useKibana } from '../../common/lib/kibana';
Expand Down Expand Up @@ -52,12 +53,22 @@ export const HealthCheck: React.FunctionComponent<Props> = ({
hasPermanentEncryptionKey: false,
};
if (healthStatus.isAlertsAvailable) {
const alertingHealthResult = await alertingFrameworkHealth({ http });
healthStatus.isSufficientlySecure = alertingHealthResult.isSufficientlySecure;
healthStatus.hasPermanentEncryptionKey = alertingHealthResult.hasPermanentEncryptionKey;
// Get the framework health, but if not available, do NOT cause the
// framework health errors/toasts to appear, since the state is
// actually unknown. These also need to be set to clear the busy
// indicator.
const alertingHealthResult = await getAlertingFrameworkHealth(http);
if (isSome(alertingHealthResult)) {
healthStatus.isSufficientlySecure = alertingHealthResult.value.isSufficientlySecure;
healthStatus.hasPermanentEncryptionKey =
alertingHealthResult.value.hasPermanentEncryptionKey;
} else {
healthStatus.isSufficientlySecure = true;
healthStatus.hasPermanentEncryptionKey = true;
}
setAlertingHealth(some(healthStatus));
}

setAlertingHealth(some(healthStatus));
setLoadingHealthCheck(false);
})();
}, [http, setLoadingHealthCheck]);
Expand Down Expand Up @@ -93,6 +104,19 @@ export const HealthCheck: React.FunctionComponent<Props> = ({
);
};

// Return as an Option, returning none if error occurred getting health.
// Currently, alerting health returns a 403 if the user is not authorized
// for rules.
async function getAlertingFrameworkHealth(
http: HttpSetup
): Promise<Option<AlertingFrameworkHealth>> {
try {
return some(await alertingFrameworkHealth({ http }));
} catch (err) {
return none;
}
}

interface PromptErrorProps {
docLinks: DocLinksStart;
className?: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,21 @@ export default ({ getPageObjects, getService }: FtrProviderContext) => {
});
});

describe('Loads the app with actions but not alerting privilege', () => {
before(async () => {
await security.testUser.setRoles(['only_actions_role']);
});
after(async () => {
await security.testUser.restoreDefaults();
});

it('Loads the Alerts page but with error', async () => {
await pageObjects.common.navigateToApp('triggersActions');
const headingText = await pageObjects.triggersActionsUI.getRulesListTitle();
expect(headingText).to.be('No permissions to create rules');
});
});

describe('Loads the app', () => {
before(async () => {
await pageObjects.common.navigateToApp('triggersActions');
Expand Down
35 changes: 35 additions & 0 deletions x-pack/test/functional_with_es_ssl/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,41 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) {
},
],
},
only_actions_role: {
kibana: [
{
feature: {
actions: ['all'],
},
spaces: ['*'],
},
],
},
discover_alert: {
kibana: [
{
feature: {
actions: ['all'],
stackAlerts: ['all'],
discover: ['all'],
advancedSettings: ['all'],
indexPatterns: ['all'],
},
spaces: ['*'],
},
],
elasticsearch: {
cluster: [],
indices: [
{
names: ['search-source-alert', 'search-source-alert-output'],
privileges: ['read', 'view_index_metadata', 'manage', 'create_index', 'index'],
field_security: { grant: ['*'], except: [] },
},
],
run_as: [],
},
},
},
defaultRoles: ['superuser'],
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@ export function TriggersActionsPageProvider({ getService }: FtrProviderContext)
await createBtn.click();
}
},
async getRulesListTitle() {
const noPermissionsTitle = await find.byCssSelector(
'[data-test-subj="alertsList"] .euiTitle'
);
return await noPermissionsTitle.getVisibleText();
},
async clickCreateConnectorButton() {
const createBtn = await testSubjects.find('createActionButton');
const createBtnIsVisible = await createBtn.isDisplayed();
Expand Down

0 comments on commit 40c897a

Please sign in to comment.