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

[Cloud Security] Clicking on Contextual Flyout popout Icon now opens page in new tab #196763

Merged
merged 15 commits into from
Oct 27, 2024
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
export * from './src/types';
export * from './src/constants/component_constants';
export * from './src/constants/navigation';
export type { NavFilter } from './src/hooks/use_navigate_findings';
export type { NavFilter } from './src/utils/query_utils';
export { showErrorToast } from './src/utils/show_error_toast';
export { encodeQuery, decodeQuery } from './src/utils/query_utils';
export { CspEvaluationBadge } from './src/components/csp_evaluation_badge';
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* 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 { CoreStart } from '@kbn/core-lifecycle-browser';
import { useKibana } from '@kbn/kibana-react-plugin/public';
import { useCallback } from 'react';
import { CspClientPluginStartDeps } from '../types';
import { NavFilter, encodeQueryUrl, queryFilters } from '../utils/query_utils';

export const useGetNavigationUrlParams = () => {
const { services } = useKibana<CoreStart & CspClientPluginStartDeps>();

const getNavUrlParams = useCallback(
(
filterParams: NavFilter = {},
findingsType?: 'configurations' | 'vulnerabilities',
groupBy?: string[]
) => {
const filters = queryFilters(filterParams);

const searchParams = new URLSearchParams(encodeQueryUrl(services.data, filters, groupBy));

return `${findingsType ? findingsType : ''}?${searchParams.toString()}`;
},
[services]
Copy link
Contributor

Choose a reason for hiding this comment

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

can be replaced with services.data as we only rely on data service

);

return getNavUrlParams;
};
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

import { useCallback } from 'react';
import { useHistory } from 'react-router-dom';
import { Filter } from '@kbn/es-query';
import {
SECURITY_DEFAULT_DATA_VIEW_ID,
CDR_MISCONFIGURATIONS_DATA_VIEW_ID_PREFIX,
Expand All @@ -17,64 +16,23 @@ import { useKibana } from '@kbn/kibana-react-plugin/public';
import { findingsNavigation } from '../constants/navigation';
import { useDataView } from './use_data_view';
import { CspClientPluginStartDeps } from '../..';
import { encodeQuery } from '../utils/query_utils';
import { NavFilter, encodeQueryUrl, queryFilters } from '../utils/query_utils';

interface NegatedValue {
value: string | number;
negate: boolean;
}

type FilterValue = string | number | NegatedValue;

export type NavFilter = Record<string, FilterValue>;

const createFilter = (key: string, filterValue: FilterValue, dataViewId: string): Filter => {
let negate = false;
let value = filterValue;
if (typeof filterValue === 'object') {
negate = filterValue.negate;
value = filterValue.value;
}
// If the value is '*', we want to create an exists filter
if (value === '*') {
return {
query: { exists: { field: key } },
meta: { type: 'exists', index: dataViewId },
};
}
return {
meta: {
alias: null,
negate,
disabled: false,
type: 'phrase',
key,
index: dataViewId,
},
query: { match_phrase: { [key]: value } },
};
};
// dataViewId is used to prevent FilterManager from falling back to the default in the sorcerer (logs-*)
const useNavigate = (pathname: string, dataViewId = SECURITY_DEFAULT_DATA_VIEW_ID) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need to fallback to SECURITY_DEFAULT_DATA_VIEW_ID as the fallback already a part of queryFilters

const history = useHistory();
const { services } = useKibana<CoreStart & CspClientPluginStartDeps>();

const { services } = useKibana<CoreStart & CspClientPluginStartDeps>();
return useCallback(
(filterParams: NavFilter = {}, groupBy?: string[]) => {
const filters = Object.entries(filterParams).map(([key, filterValue]) =>
createFilter(key, filterValue, dataViewId)
);
const filters = queryFilters(filterParams, dataViewId);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think the code is more readable when a function has some verb in the name, eg. composeQueryFilters or smth along these lines


history.push({
pathname,
search: encodeQuery({
// Set query language from user's preference
query: services.data.query.queryString.getDefaultQuery(),
filters,
...(groupBy && { groupBy }),
}),
search: encodeQueryUrl(services.data, filters, groupBy),
});
},
[history, pathname, services.data.query.queryString, dataViewId]
[dataViewId, history, pathname, services.data]
);
};

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
/*
* 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 { encodeQueryUrl, queryFilters } from './query_utils';
import { dataPluginMock } from '@kbn/data-plugin/public/mocks';

const DEFAULT_DATA_VIEW_ID = 'security-solution-default';

describe('queryFilters', () => {
it('Should return correct filters given some filterParams', () => {
const testFilterParams = {
test_field: 'test_value',
};
const testResult = [
{
meta: {
alias: null,
negate: false,
disabled: false,
type: 'phrase',
key: 'test_field',
index: DEFAULT_DATA_VIEW_ID,
},
query: { match_phrase: { test_field: 'test_value' } },
},
];
expect(queryFilters(testFilterParams)).toEqual(testResult);
});

it('Should return empty filters given empty filterParams', () => {
expect(queryFilters({})).toEqual([]);
});

it('Should return correct filters given some filterParams and dataviewId', () => {
const testFilterParams = {
test_field: 'test_value',
};
const testResult = [
{
meta: {
alias: null,
negate: false,
disabled: false,
type: 'phrase',
key: 'test_field',
index: 'test-data-view',
},
query: { match_phrase: { test_field: 'test_value' } },
},
];
expect(queryFilters(testFilterParams, 'test-data-view')).toEqual(testResult);
});
});

describe('encodeQueryUrl', () => {
const getServicesMock = () => ({
data: dataPluginMock.createStartContract(),
});

it('Should return correct URL given empty filters', () => {
const result = 'cspq=(filters:!())';
expect(encodeQueryUrl(getServicesMock().data, [])).toEqual(result);
});

it('should return correct URL given filters', () => {
const filter = [
{
meta: {
alias: null,
negate: false,
disabled: false,
type: 'phrase',
key: 'test_field',
index: DEFAULT_DATA_VIEW_ID,
},
query: { match_phrase: { test_field: 'test_value' } },
},
];
const result =
'cspq=(filters:!((meta:(alias:!n,disabled:!f,index:security-solution-default,key:test_field,negate:!f,type:phrase),query:(match_phrase:(test_field:test_value)))))';
expect(encodeQueryUrl(getServicesMock().data, filter)).toEqual(result);
});

it('should return correct URL given filters and group by', () => {
const filter = [
{
meta: {
alias: null,
negate: false,
disabled: false,
type: 'phrase',
key: 'test_field',
index: DEFAULT_DATA_VIEW_ID,
},
query: { match_phrase: { test_field: 'test_value' } },
},
];
const groupByFilter = ['filterA'];
const result =
'cspq=(filters:!((meta:(alias:!n,disabled:!f,index:security-solution-default,key:test_field,negate:!f,type:phrase),query:(match_phrase:(test_field:test_value)))),groupBy:!(filterA))';
expect(encodeQueryUrl(getServicesMock().data, filter, groupByFilter)).toEqual(result);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,21 @@
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { encode, decode } from '@kbn/rison';
import type { LocationDescriptorObject } from 'history';
import { Filter } from '@kbn/es-query';
import { SECURITY_DEFAULT_DATA_VIEW_ID } from '@kbn/cloud-security-posture-common';
import { DataPublicPluginStart } from '@kbn/data-plugin/public';

interface NegatedValue {
value: string | number;
negate: boolean;
}

type FilterValue = string | number | NegatedValue;

export type NavFilter = Record<string, FilterValue>;

const encodeRison = (v: any): string | undefined => {
try {
Expand Down Expand Up @@ -38,3 +51,51 @@ export const decodeQuery = <T extends unknown>(search?: string): Partial<T> | un
if (!risonQuery) return;
return decodeRison<T>(risonQuery);
};

export const encodeQueryUrl = (
maxcold marked this conversation as resolved.
Show resolved Hide resolved
servicesStart: DataPublicPluginStart,
filters: Filter[],
groupBy?: string[]
): any => {
return encodeQuery({
query: servicesStart.query.queryString.getDefaultQuery(),
filters,
...(groupBy && { groupBy }),
});
};

export const queryFilters = (
filterParams: NavFilter = {},
dataViewId = SECURITY_DEFAULT_DATA_VIEW_ID
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the use of SECURITY_DEFAULT_DATA_VIEW_ID . Similarly I don't understand why useNavigateFindings and useNavigateVulnerabilities differ in the data view id logic. Similarly to CDR_MISCONFIGURATIONS_DATA_VIEW_ID_PREFIX we have CDR_VULNERABILITIES_DATA_VIEW_ID_PREFIX. Why don't we use it in useNavigateVulnerabilities then? And in your new logic you only use SECURITY_DEFAULT_DATA_VIEW_ID , so we have a variety of difference here. @opauloh do you recall anything about the use of data view ids for the navigation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey that was added on this PR

If we don't specify the DataView on the filter when navigating to the findings page, the FilterManager will fallback to the default in the sorcerer (logs-*). This doesn't prevent filtering from working, but it doesn't allow for editing the filter, since the SearchBar component is configured to accept only the Latest Misconfigurations Dataview (or the Latest Vulnerabilities DataView on the Vulnerabilities tab).

image

So the fix was to specify the Dataview for each filter, and then the edit filter works:

image

Should probably have added it as a comment, which I suggest @animehart to add so we can recall when going over it in the future.

Another solution could be to update the navigation to set the sorcerer index pattern instead of setting it per filter, but that also involves setting a Dataview in the navigation logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the detailed clarification, makes sense! Agree that we need to add a comment as this Data View logic pieces are quite complicated to grasp without all the context. I think this logic should also be added to the vulnerability navigation logic, right now it looks like it is not part of it, unless I missed that in the code

Copy link
Contributor

Choose a reason for hiding this comment

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

@animehart we still don't pass vulnerabiltiy dataview id to the navigation, while for findings we do. Should we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maxcold
if there aren't anything that can be broken by it then I think we should

Copy link
Contributor

Choose a reason for hiding this comment

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

as we plan to backport this to 8.16 let's not add this new logic, just to be on the safe side. We can do it separately for 8.17

): Filter[] => {
return Object.entries(filterParams).map(([key, filterValue]) =>
createFilter(key, filterValue, dataViewId)
);
};

export const createFilter = (key: string, filterValue: FilterValue, dataViewId: string): Filter => {
let negate = false;
let value = filterValue;
if (typeof filterValue === 'object') {
negate = filterValue.negate;
value = filterValue.value;
}
// If the value is '*', we want to create an exists filter
if (value === '*') {
return {
query: { exists: { field: key } },
meta: { type: 'exists', index: dataViewId },
};
}
return {
meta: {
alias: null,
negate,
disabled: false,
type: 'phrase',
key,
index: dataViewId,
},
query: { match_phrase: { [key]: value } },
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,6 @@
"@kbn/ui-theme",
"@kbn/i18n-react",
"@kbn/rison",
"@kbn/core-lifecycle-browser",
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { FormattedMessage } from '@kbn/i18n-react';
import { css } from '@emotion/react';
import { i18n } from '@kbn/i18n';
import { useNavigateFindings } from '@kbn/cloud-security-posture/src/hooks/use_navigate_findings';
import type { NavFilter } from '@kbn/cloud-security-posture/src/hooks/use_navigate_findings';
import type { NavFilter } from '@kbn/cloud-security-posture/src/utils/query_utils';
import type {
BenchmarkData,
ComplianceDashboardDataV2,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
import { i18n } from '@kbn/i18n';
import { css } from '@emotion/react';
import { CSPM_POLICY_TEMPLATE, KSPM_POLICY_TEMPLATE } from '@kbn/cloud-security-posture-common';
import type { NavFilter } from '@kbn/cloud-security-posture/src/hooks/use_navigate_findings';
import type { NavFilter } from '@kbn/cloud-security-posture/src/utils/query_utils';
import { useNavigateFindings } from '@kbn/cloud-security-posture/src/hooks/use_navigate_findings';
import { useCspIntegrationLink } from '../../../common/navigation/use_csp_integration_link';
import { DASHBOARD_COUNTER_CARDS, DASHBOARD_SUMMARY_CONTAINER } from '../test_subjects';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
useEuiTheme,
} from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import type { NavFilter } from '@kbn/cloud-security-posture/src/hooks/use_navigate_findings';
import type { NavFilter } from '@kbn/cloud-security-posture/src/utils/query_utils';
import { useNavigateVulnerabilities } from '@kbn/cloud-security-posture/src/hooks/use_navigate_findings';
import type { VulnSeverity } from '@kbn/cloud-security-posture-common';
import { CVSScoreBadge, SeverityStatusBadge } from '@kbn/cloud-security-posture';
Expand Down
Loading