kibana icon indicating copy to clipboard operation
kibana copied to clipboard

[Cloud Security] Remove DataView labels update logic and override table columns instead

Open opauloh opened this issue 1 year ago • 5 comments

Motivation

Currently, the Findings data table uses the useLatestFindingsDataView hook to update the DataView fields Label when the labels are empty. This can lead to unreliability when testing, and issues with side effects due to updating data in a method that should only retrieve data.

This ticket aims, to refactor that logic and split it into two separate methods, keep the hook to only fetch the data view, and move the update logic to the Kibana server-side in the plugin initialization

Update: we wish to stop updating the data view. When doing it from the client side it requires permissions to data view management, and when the user lack these permissions the findings fails to load. see https://github.com/elastic/sdh-security-team/issues/949. And in a second thought, the Alerts page doesn't update the data view, it just overrides the table's column label. We should do the same.

Definition of done

  • [ ] useLatestFindingsDataView hook only retrieves the DataView data, remove any logic to update the columns
  • [ ] Override the table columns to the given labels
  • [ ] Make sure the FTR tests pass
  • [ ] ~Vulnerabilities and Misconfigurations DataViews labels are updated independently~
  • [ ] ~The labels are still updated~

Out of scope

Related tasks/epics

opauloh avatar Dec 05 '23 18:12 opauloh

Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security)

elasticmachine avatar Dec 05 '23 18:12 elasticmachine

Hi @opauloh can you remind me what blocks this issue?

kfirpeled avatar Mar 24 '24 14:03 kfirpeled

Hi @opauloh can you remind me what blocks this issue?

Yes, the implemented solution relies on information stored in the DataViews at the moment of installing the CSP integration, but we have some FTR tests, such as this one, that erases the information on the DataViews, so a better solution is still to be found to address this issue, so I moved to blocked as we had other tasks with higher priority.

opauloh avatar Mar 26 '24 18:03 opauloh

we discussed it and we think it worth moving the FTR tests to the api from the functional to test that the data view is being updated properly

kfirpeled avatar Apr 02 '24 18:04 kfirpeled

Updated the ticket to fix the sdh we have today and assigning this ticket to @animehart

kfirpeled avatar May 12 '24 12:05 kfirpeled

Hi @kfirpeled , it seems like the fix this issue has already been included in @opauloh PR (https://github.com/elastic/kibana/pull/176266) the only issue with that PR is that it's breaking some of our FTR as such in my opinion wouldn't it be better to just go with that PR and then create a follow up ticket or PR to address the affected FTRs ? Unless the solution on that PR is not the solution that we wanted or I'm misunderstood something

animehart avatar May 14 '24 00:05 animehart

@opauloh @kfirpeled After reviewing related PRs I realized that this is a breaking change for our users as we don't allow set custom names to the column headers anymore (before it was possible). Maybe with our current user base, it's not a big deal, but I think we should be more transparent and clear about it, the ticket description is quite technical and doesn't capture this. As we removed this feature from our data grid, I would also advice also remove the option to edit the data view field from the data grid as the "set custom label" functionality doesn't have any effect on the data grid anymore

Screenshot 2024-06-19 at 12 04 55

maxcold avatar Jun 19 '24 10:06 maxcold

@opauloh @kfirpeled After reviewing related PRs I realized that this is a breaking change for our users as we don't allow set custom names to the column headers anymore (before it was possible). Maybe with our current user base, it's not a big deal, but I think we should be more transparent and clear about it, the ticket description is quite technical and doesn't capture this. As we removed this feature from our data grid, I would also advice also remove the option to edit the data view field from the data grid as the "set custom label" functionality doesn't have any effect on the data grid anymore

Screenshot 2024-06-19 at 12 04 55

The option to "Edit data view field" was removed in a separate PR. @animehart Can you link the PR that removes the edit data view option from the Findings page data tables?

opauloh avatar Jul 09 '24 21:07 opauloh

Follow these steps to verify this ticket:

  1. Create a new space
  2. Define a new role with read access only, and fleet all
  3. Login with new user
  4. Check findings page, dashboard works as expected - shows findings
  5. Check benchmark page works as expected
  6. Check you cannot change benchmark rule enabled toggle

kfirpeled avatar Jul 25 '24 15:07 kfirpeled

Verified

BC 4

Permissions

Minimum Permissions Elasticsearch

Image

Minimum Permissions Kibana

Image

Image

⚠️ Note:

  • Saved objects read access is necessary to load the Benchmarks page
  • Fleet all access is necessary to load with the Findings page and Benchmarks page

User access

Image

Image

https://github.com/user-attachments/assets/a9d01119-9ee1-4184-a031-2fc57f09b2f9

⚠️ Note

There's currently an issue when attempting to mute the rules without permission still displays the success toast, I consider this a non-blocking issue and added a quick wins to solve that.

opauloh avatar Jul 26 '24 04:07 opauloh

@opauloh @kfirpeled how are the verification steps related to the issue itself? The issue talks about removing the Data View update logic, I'm not sure how it is related to the problem with permissions. I also had some confusion around the required permissions, described them in the thread https://elastic.slack.com/archives/C03E5KGNWT1/p1722002692188249 , I think we need to document the additional required privileges otherwise it might be very confusing for the users

maxcold avatar Jul 26 '24 15:07 maxcold

how are the verification steps related to the issue itself? The issue talks about removing the Data View update logic, I'm not sure how it is related to the problem with permissions.

I was mainly focused on following the steps described here. Maybe those instructions belonged to another ticket? cc @kfirpeled

But I can also confirm that this ticket itself is verified:

image

opauloh avatar Jul 26 '24 23:07 opauloh

The issue with access roles can be reproduced on serverless, I opened a separate ticket for it as the access control logic on Serverless is very different from ESS

  • https://github.com/elastic/kibana/issues/189538

Marking this issue as Verified on serverless

maxcold avatar Jul 30 '24 16:07 maxcold