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.

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

[RAM] Category fields endpoint #138245

Merged
merged 36 commits into from
Sep 12, 2022
Merged
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
c06a11a
first commit
jcger Aug 8, 2022
861f5df
Merge branch 'main' of github.com:jcger/kibana into 137988-category-f…
jcger Aug 8, 2022
fad8830
get auth index and try field caps
jcger Aug 8, 2022
61a5a49
use esClient
jcger Aug 8, 2022
c56de28
[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…
kibanamachine Aug 8, 2022
56d89be
wait for promise to finish
jcger Aug 9, 2022
c30862b
Merge branch '137988-category-field-api' of github.com:jcger/kibana i…
jcger Aug 9, 2022
fbd357b
format field capabilities
jcger Aug 10, 2022
e0df512
add simplier browserFields mapper
jcger Aug 10, 2022
f5cf16b
update response
jcger Aug 10, 2022
8461aeb
Merge branch 'main' of github.com:jcger/kibana into 137988-category-f…
jcger Aug 10, 2022
a3284bd
refactor
jcger Aug 10, 2022
34a2cfd
types and refactor
jcger Aug 11, 2022
2eb8d92
Merge branch 'main' of github.com:jcger/kibana into 137988-category-f…
jcger Aug 11, 2022
03cf83e
Merge branch 'main' of github.com:jcger/kibana into 137988-category-f…
jcger Sep 5, 2022
7148021
Merge branch 'main' of github.com:jcger/kibana into 137988-category-f…
jcger Sep 7, 2022
43c3d62
remove browser fields dependency
jcger Sep 7, 2022
46423b9
update fn name
jcger Sep 7, 2022
f111f4c
update types
jcger Sep 7, 2022
17f6171
update imported type package
jcger Sep 7, 2022
106fdf1
Merge branch 'main' into 137988-category-field-api
kibanamachine Sep 7, 2022
2c96b5c
update mock object
jcger Sep 7, 2022
018bd72
error message for no o11y alert indices
jcger Sep 8, 2022
6af56ff
add endpoint integration test
jcger Sep 8, 2022
dc1b1e0
Merge branch 'main' of github.com:jcger/kibana into 137988-category-f…
jcger Sep 9, 2022
1969cff
Merge branch 'main' of github.com:jcger/kibana into 137988-category-f…
jcger Sep 9, 2022
b7324cb
activate commented tests
jcger Sep 9, 2022
f950a35
add unit test
jcger Sep 9, 2022
9fbc94c
Merge branch 'main' of github.com:jcger/kibana into 137988-category-f…
jcger Sep 9, 2022
9873093
comment uncommented tests
jcger Sep 9, 2022
0997730
fix tests
jcger Sep 9, 2022
1d32de9
review by Xavier
XavierM Sep 9, 2022
cbb46d3
[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…
kibanamachine Sep 9, 2022
0d9c674
Merge branch 'main' into 137988-category-field-api
jcger Sep 12, 2022
f9ba067
update param names + right type
jcger Sep 12, 2022
f10c273
Merge branch 'main' into 137988-category-field-api
jcger Sep 12, 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
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ const createAlertsClientMock = () => {
bulkUpdate: jest.fn(),
find: jest.fn(),
getFeatureIdsByRegistrationContexts: jest.fn(),
getBrowserFields: jest.fn(),
};
return mocked;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
} from '@kbn/alerting-plugin/server';
import { Logger, ElasticsearchClient, EcsEventOutcome } from '@kbn/core/server';
import { AuditLogger } from '@kbn/security-plugin/server';
import { IndexPatternsFetcher } from '@kbn/data-plugin/server';
import { alertAuditEvent, operationAlertAuditActionMap } from './audit_events';
import {
ALERT_WORKFLOW_STATUS,
Expand All @@ -40,6 +41,8 @@ import {
import { ParsedTechnicalFields } from '../../common/parse_technical_fields';
import { Dataset, IRuleDataService } from '../rule_data_plugin_service';
import { getAuthzFilter, getSpacesFilter } from '../lib';
import { fieldDescriptorToBrowserFieldMapper } from './browser_fields';
import { BrowserFields } from '../types';

// TODO: Fix typings https://github.com/elastic/kibana/issues/101776
type NonNullableProps<Obj extends {}, Props extends keyof Obj> = Omit<Obj, Props> & {
Expand Down Expand Up @@ -716,4 +719,23 @@ export class AlertsClient {
throw Boom.failedDependency(errMessage);
}
}

async getBrowserFields({
indices,
metaFields,
allowNoIndex,
}: {
indices: string[];
metaFields: string[];
allowNoIndex: boolean;
}): Promise<BrowserFields> {
const indexPatternsFetcherAsInternalUser = new IndexPatternsFetcher(this.esClient);
const { fields } = await indexPatternsFetcherAsInternalUser.getFieldsForWildcard({
pattern: indices,
metaFields,
fieldCapsOptions: { allow_no_indices: allowNoIndex },
});

return fieldDescriptorToBrowserFieldMapper(fields);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { FieldSpec } from '@kbn/data-plugin/common';
import { BrowserField, BrowserFields } from '../../types';

const getFieldCategory = (fieldCapability: FieldSpec) => {
const name = fieldCapability.name.split('.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Since it is an array, names would be a better naming.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I agree. It's still the name capability but split into one array, each element of the name would a part of the name but not a name by itself. Calling it names would mean that each part is a name but I think it isn't, not sure though

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean, maybe nameParts or nameSections? In general, it helps when for arrays, we have a plural name but up to you :)


if (name.length === 1) {
return 'base';
}

return name[0];
};

const browserFieldFactory = (
fieldCapability: FieldSpec,
category: string
): { [fieldName in string]: BrowserField } => {
return {
[fieldCapability.name]: {
...fieldCapability,
category,
},
};
};

export const fieldDescriptorToBrowserFieldMapper = (
fieldDescriptor: FieldSpec[]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We can call this fields for this variable and when we iterate over it, use field for each item.

): BrowserFields => {
return fieldDescriptor.reduce((browserFields: BrowserFields, fieldCapability: FieldSpec) => {
const category = getFieldCategory(fieldCapability);
const field = browserFieldFactory(fieldCapability, category);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field is browserField, right? We can name it browserField


if (browserFields[category]) {
browserFields[category] = { fields: { ...browserFields[category].fields, ...field } };
} else {
browserFields[category] = { fields: field };
}

return browserFields;
}, {});
};
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,10 @@ export const getReadFeatureIdsRequest = () =>
path: `${BASE_RAC_ALERTS_API_PATH}/_feature_ids`,
query: { registrationContext: ['security'] },
});

export const getO11yBrowserFields = () =>
requestMock.create({
method: 'get',
path: `${BASE_RAC_ALERTS_API_PATH}/browser_fields`,
query: { featureIds: ['apm', 'logs'] },
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { BASE_RAC_ALERTS_API_PATH } from '../../common/constants';
import { getBrowserFieldsByFeatureId } from './get_browser_fields_by_feature_id';
import { requestContextMock } from './__mocks__/request_context';
import { getO11yBrowserFields } from './__mocks__/request_responses';
import { requestMock, serverMock } from './__mocks__/server';

describe('getBrowserFieldsByFeatureId', () => {
let server: ReturnType<typeof serverMock.create>;
let { clients, context } = requestContextMock.createTools();
const path = `${BASE_RAC_ALERTS_API_PATH}/browser_fields`;

beforeEach(async () => {
server = serverMock.create();
({ clients, context } = requestContextMock.createTools());
});

describe('when racClient returns o11y indices', () => {
beforeEach(() => {
clients.rac.getAuthorizedAlertsIndices.mockResolvedValue([
'.alerts-observability.logs.alerts-default',
]);

getBrowserFieldsByFeatureId(server.router);
});

test('route registered', async () => {
const response = await server.inject(getO11yBrowserFields(), context);

expect(response.status).toEqual(200);
});

test('rejects invalid featureId type', async () => {
await expect(
server.inject(
requestMock.create({
method: 'get',
path,
query: { featureIds: undefined },
}),
context
)
).rejects.toThrowErrorMatchingInlineSnapshot(
`"Request was rejected with message: 'Invalid value \\"undefined\\" supplied to \\"featureIds\\"'"`
);
});

test('returns error status if rac client "getAuthorizedAlertsIndices" fails', async () => {
clients.rac.getAuthorizedAlertsIndices.mockRejectedValue(new Error('Unable to get index'));
const response = await server.inject(getO11yBrowserFields(), context);

expect(response.status).toEqual(500);
expect(response.body).toEqual({
attributes: { success: false },
message: 'Unable to get index',
});
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { IRouter } from '@kbn/core/server';
import { transformError } from '@kbn/securitysolution-es-utils';
import * as t from 'io-ts';

import { RacRequestHandlerContext } from '../types';
import { BASE_RAC_ALERTS_API_PATH } from '../../common/constants';
import { buildRouteValidation } from './utils/route_validation';

export const getBrowserFieldsByFeatureId = (router: IRouter<RacRequestHandlerContext>) => {
router.get(
{
path: `${BASE_RAC_ALERTS_API_PATH}/browser_fields`,
validate: {
query: buildRouteValidation(
t.exact(
t.type({
featureIds: t.union([t.string, t.array(t.string)]),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcger That's one of the reason why your API integration was not working because before you only allow for array. However, when you just have one item in a query parameter, it is automatically converted to just be a string.

})
)
),
},
options: {
tags: ['access:rac'],
},
},
async (context, request, response) => {
try {
const racContext = await context.rac;
const alertsClient = await racContext.getAlertsClient();
const { featureIds = [] } = request.query;

const indices = await alertsClient.getAuthorizedAlertsIndices(
Array.isArray(featureIds) ? featureIds : [featureIds]
);
const o11yIndices =
indices?.filter((index) => index.startsWith('.alerts-observability')) ?? [];
if (o11yIndices.length === 0) {
return response.notFound({
body: {
message: `No alerts-observability indices found for featureIds [${featureIds}]`,
attributes: { success: false },
},
});
}

const browserFields = await alertsClient.getBrowserFields({
indices: o11yIndices,
metaFields: ['_id', '_index'],
allowNoIndex: true,
});

return response.ok({
body: browserFields,
});
} catch (error) {
const formatedError = transformError(error);
const contentType = {
'content-type': 'application/json',
};
const defaultedHeaders = {
...contentType,
};

return response.customError({
headers: defaultedHeaders,
statusCode: formatedError.statusCode,
body: {
message: formatedError.message,
attributes: {
success: false,
},
},
});
}
}
);
};
2 changes: 2 additions & 0 deletions x-pack/plugins/rule_registry/server/routes/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { getAlertsIndexRoute } from './get_alert_index';
import { bulkUpdateAlertsRoute } from './bulk_update_alerts';
import { findAlertsByQueryRoute } from './find';
import { getFeatureIdsByRegistrationContexts } from './get_feature_ids_by_registration_contexts';
import { getBrowserFieldsByFeatureId } from './get_browser_fields_by_feature_id';

export function defineRoutes(router: IRouter<RacRequestHandlerContext>) {
getAlertByIdRoute(router);
Expand All @@ -21,4 +22,5 @@ export function defineRoutes(router: IRouter<RacRequestHandlerContext>) {
bulkUpdateAlertsRoute(router);
findAlertsByQueryRoute(router);
getFeatureIdsByRegistrationContexts(router);
getBrowserFieldsByFeatureId(router);
}
9 changes: 9 additions & 0 deletions x-pack/plugins/rule_registry/server/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
RuleTypeState,
} from '@kbn/alerting-plugin/common';
import { RuleExecutorOptions, RuleExecutorServices, RuleType } from '@kbn/alerting-plugin/server';
import { FieldSpec } from '@kbn/data-plugin/common';
import { AlertsClient } from './alert_data_client/alerts_client';

type SimpleAlertType<
Expand Down Expand Up @@ -71,3 +72,11 @@ export interface RacApiRequestHandlerContext {
export type RacRequestHandlerContext = CustomRequestHandlerContext<{
rac: RacApiRequestHandlerContext;
}>;

export type BrowserField = FieldSpec & {
jcger marked this conversation as resolved.
Show resolved Hide resolved
category: string;
};

export type BrowserFields = {
[category in string]: { fields: { [fieldName in string]: BrowserField } };
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import expect from '@kbn/expect';

import { superUser, obsOnlySpacesAll, secOnlyRead } from '../../../common/lib/authentication/users';
import type { User } from '../../../common/lib/authentication/types';
import { FtrProviderContext } from '../../../common/ftr_provider_context';
import { getSpaceUrlPrefix } from '../../../common/lib/authentication/spaces';

// eslint-disable-next-line import/no-default-export
export default ({ getService }: FtrProviderContext) => {
const supertestWithoutAuth = getService('supertestWithoutAuth');
const esArchiver = getService('esArchiver');
const SPACE1 = 'space1';
const TEST_URL = '/internal/rac/alerts/browser_fields';

const getBrowserFieldsByFeatureId = async (
user: User,
featureIds: string[],
expectedStatusCode: number = 200
) => {
const resp = await supertestWithoutAuth
.get(`${getSpaceUrlPrefix(SPACE1)}${TEST_URL}`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcger I had to add the space because your user secOnlyRead only have access to space 1. On Monday let's go over kibana privileges together. It is always confusing for everybody.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

.query({ featureIds })
.auth(user.username, user.password)
.set('kbn-xsrf', 'true')
.expect(expectedStatusCode);
return resp.body;
};

describe('Alert - Get browser fields by featureId', () => {
before(async () => {
await esArchiver.load('x-pack/test/functional/es_archives/rule_registry/alerts');
});

describe('Users:', () => {
it(`${obsOnlySpacesAll.username} should be able to get browser fields for o11y featureIds`, async () => {
const browserFields = await getBrowserFieldsByFeatureId(obsOnlySpacesAll, [
'apm',
'infrastructure',
'logs',
'uptime',
]);
expect(Object.keys(browserFields)).to.eql(['base']);
});

it(`${superUser.username} should be able to get browser fields for o11y featureIds`, async () => {
const browserFields = await getBrowserFieldsByFeatureId(superUser, [
'apm',
'infrastructure',
'logs',
'uptime',
]);
expect(Object.keys(browserFields)).to.eql(['base']);
});

it(`${superUser.username} should NOT be able to get browser fields for siem featureId`, async () => {
await getBrowserFieldsByFeatureId(superUser, ['siem'], 404);
});

it(`${secOnlyRead.username} should NOT be able to get browser fields for siem featureId`, async () => {
await getBrowserFieldsByFeatureId(secOnlyRead, ['siem'], 404);
});
});
});
};
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,6 @@ export default ({ loadTestFile, getService }: FtrProviderContext): void => {
loadTestFile(require.resolve('./get_alerts_index'));
loadTestFile(require.resolve('./find_alerts'));
loadTestFile(require.resolve('./search_strategy'));
loadTestFile(require.resolve('./get_browser_fields_by_feature_id'));
});
};