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

[Cloud Security] Fix for issue where date is not retained when opening new tab #205128

Merged
merged 6 commits into from
Jan 3, 2025
+81 −5
Merged

[Cloud Security] Fix for issue where date is not retained when opening new tab #205128

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f8bceab
initial push for fix
animehart Dec 24, 2024
ffceae9
Merge branch 'main' into fix-alert-newtab-timerange
animehart Dec 24, 2024
de67b4c
Merge branch 'main' into fix-alert-newtab-timerange
animehart Dec 27, 2024
3b1c835
Merge branch 'main' into fix-alert-newtab-timerange
animehart Dec 30, 2024
27fbeda
Merge branch 'main' into fix-alert-newtab-timerange
animehart Jan 2, 2025
0e8bec2
pr comments
animehart Jan 3, 2025
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
17 changes: 15 additions & 2 deletions ...on/public/cloud_security_posture/components/csp_details/alerts_findings_details_table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

import React, { memo, useCallback, useEffect, useState } from 'react';
import { encode } from '@kbn/rison';
import { capitalize } from 'lodash';
import type { Criteria, EuiBasicTableColumn } from '@elastic/eui';
import { EuiSpacer, EuiPanel, EuiText, EuiBasicTable, EuiIcon, EuiLink } from '@elastic/eui';
Expand All @@ -26,6 +27,7 @@ import {
OPEN_IN_ALERTS_TITLE_STATUS,
OPEN_IN_ALERTS_TITLE_USERNAME,
} from '../../../overview/components/detection_response/translations';
import { URL_PARAM_KEY } from '../../../common/hooks/use_url_state';
import { useNavigateToAlertsPageWithFilters } from '../../../common/hooks/use_navigate_to_alerts_page_with_filters';
import { DocumentDetailsPreviewPanelKey } from '../../../flyout/document_details/shared/constants/panel_keys';
import { useGlobalTime } from '../../../common/containers/use_global_time';
Expand Down Expand Up @@ -93,6 +95,16 @@ export const AlertsDetailsTable = memo(
};

const { to, from } = useGlobalTime();
const timerange = encode({
global: {
[URL_PARAM_KEY.timerange]: {
kind: 'absolute',
from,
to,
},
},
});

const { signalIndexName } = useSignalIndex();
const { data, setQuery } = useQueryAlerts({
query: buildEntityAlertsQuery(field, to, from, value, 500, ''),
Expand Down Expand Up @@ -254,9 +266,10 @@ export const AlertsDetailsTable = memo(
fieldName: 'kibana.alert.workflow_status',
},
],
true
true,
timerange
),
[field, openAlertsPageWithFilters, value]
[field, openAlertsPageWithFilters, timerange, value]
);

return (
Expand Down
10 changes: 7 additions & 3 deletions ...plugins/security_solution/public/common/hooks/use_navigate_to_alerts_page_with_filters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,18 @@ import { URL_PARAM_KEY } from './use_url_state';
export const useNavigateToAlertsPageWithFilters = () => {
const { navigateTo } = useNavigation();

return (filterItems: FilterControlConfig | FilterControlConfig[], openInNewTab = false) => {
return (
filterItems: FilterControlConfig | FilterControlConfig[],
openInNewTab = false,
timerange?: string
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, no-one (except the usage in the file alerts_findings_details_table.tsx above) is using the openInNewTab property. It seems to me that the new timerange property is fully related to the fact that we want to open in a new tab. If openInNewTabis false, the user is just navigating to a different page in Kibana within the same browser tab, and the KQL bar and its date time picker remain the same (meaning the date and time are persisted).

I've been trying to find a good way to combine openInNewTab and timerange somehow together?, but I'm not liking any of the solutions I can come up with.

I'm ok keeping things as you have right now, but if you have an idea let me know!

Copy link
Contributor

Choose a reason for hiding this comment

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

would you be ok adding these prop documentations?

    /**
     * Pass one or more filter control configurations to be applied to the alerts page filters
     */
    filterItems: FilterControlConfig | FilterControlConfig[],
    /**
     * If true, the alerts page will be opened in a new tab
     */
    openInNewTab = false,
    /**
     * Allows to customize the timerange url parameter. Should only be used in combination with the openInNewTab=true parameter
     */
    timerange?: string

) => {
const urlFilterParams = encode(
formatPageFilterSearchParam(Array.isArray(filterItems) ? filterItems : [filterItems])
);

const timerangePath = timerange ? `&timerange=${timerange}` : '';
navigateTo({
deepLinkId: SecurityPageName.alerts,
path: `?${URL_PARAM_KEY.pageFilter}=${urlFilterParams}`,
path: `?${URL_PARAM_KEY.pageFilter}=${urlFilterParams}${timerangePath}`,
Copy link
Contributor

@JordanSh JordanSh Dec 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Im wondering if we can't use the existing urlFilterParams and add timerange to them directly.
The problem the might occur with the current fix is that what happens if both urlFilterParams and timerangePath include a timerange filter?

the table file seems to include the needed functions to create this query. and the encoding is happening here on this hook file. so if we can insert the timerange filter from the table it would be ideal.

im approving, but please verify if that double timerange filter case can actually happen. if so, we probably need to address it before merging. i might also be missing the reason why this suggestion wont work so ill be glad if you could explain if you already tried it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmmm so before this actually I Tried adding timerange on the Alerts page filter to no avail
but your question is a good one, I'm not 100% sure if its not doable on adding timerange directly on the pageFilter, Im going to ask Security Team about this

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes this could be a scenario that happens, but I think it is very unlikely, as developers would have to specifically pass a timerange AND a timestamp filter to this hook...

Also in the UI when using this hook, chances are that it would show the following message, letting the user know the filters have changed and would encourage them to look at them
Screenshot 2025-01-03 at 10 11 07 AM

And finally, I'm not expecting many people to use this new timerange prop many often, as the only usage at the moment is for the alerts_findings_details_table.tsx above, all the other instances are not opening in a new tab

openInNewTab,
});
};
Expand Down