Added data streams privileges to better control delete actions in UI #83573

yuliacech · 2020-11-17T18:13:10Z

Summary

This PR adds a privileges check to data streams tab in Index Management so that the user without a delete privilege wont see a delete button/action anymore.

Screenshots

How to test

Instructions

create 2 data streams with following dev tools commands:

PUT /_ilm/policy/my-data-stream-policy
{
  "policy": {
    "phases": {
      "hot": {
        "actions": {
          "rollover": {
            "max_size": "25GB"
          }
        }
      },
      "delete": {
        "min_age": "30d",
        "actions": {
          "delete": {}
        }
      }
    }
  }
}

PUT /_index_template/my-data-stream-template
{
  "index_patterns": [ "my-data-stream*" ],
  "data_stream": { },
  "priority": 200,
  "template": {
    "settings": {
      "index.lifecycle.name": "my-data-stream-policy"
    }
  }
}

POST /my-data-stream/_doc/
{
  "@timestamp": "2020-12-06T11:04:05.000Z",
  "user": {
    "id": "vlb44hny"
  },
  "message": "Login attempt failed"
}

PUT /_index_template/my-data-stream-template-2
{
  "index_patterns": [ "no-delete-data-stream*" ],
  "data_stream": { },
  "priority": 200,
  "template": {
    "settings": {
      "index.lifecycle.name": "my-data-stream-policy"
    }
  }
}

POST /no-delete-data-stream/_doc/
{
  "@timestamp": "2020-12-06T11:04:05.000Z",
  "user": {
    "id": "vlb44hny"
  },
  "message": "Login attempt failed"
}

Navigate to Index Management -> Data Streams to check delete button/actions for both data streams
Create a user with some delete privileges missing:

Navigate to Roles and create a role with manage privileges on cluster, view_index_metadata, monitor on all indices (*) and delete_index privileges on my-data-stream index.

- Also add all Kibana privileges to this user

- Create a test user and add the test role to the user

Login with the test user and check that delete button/actions are not displayed for no-delete-data-stream

Checklist

Unit or functional tests were updated or added to match the most common scenarios

TODO

Add api integration tests in a following PR

Release note

In the Index Management app, buttons to delete a data stream are now controlled by user's privileges.

yuliacech · 2020-11-18T11:02:42Z

@elasticmachine merge upstream

elasticmachine · 2020-11-18T13:46:41Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

sebelga

Great job @yuliacech ! I reviewed the code but I haven't yet been able to test locally (do you mind adding the console commands required?) For the privileges I guess I only need to create a user without the "delete_index" privilege, or is there other privileges to take into account?

Regarding the approach taken: I wonder if it would not be better to fetch the privileges at the same time as we fetch de data streams (a single request) and enhance them. Wouldn't that simplify the client code? I was wondering what happens "in between" the 2 calls (fetch the resource, fetch its privileges). Does the button to delete the stream appear or not? If the HTTP request for the privileges fails, how do we handle it client-side? For those scenarios, it seems easier to do it all server-side and have a fallback mechanism in a single place.

We'd end up with objects like this

{
  name: 'my-data-stream',
  ...
  __meta__: {
    privileges: {
      delete: true, // defaults to "true" if privilege request failed
    }
  }
}

WDYT?

sebelga · 2020-11-19T09:48:34Z

...gins/index_management/public/application/sections/home/data_stream_list/data_stream_list.tsx

@@ -240,6 +248,9 @@ export const DataStreamList: React.FunctionComponent<RouteComponentProps<MatchPa
              reload();
            }
          }}
+          canDelete={
+            !deletePrivileges || deletePrivileges[attemptToURIDecode(dataStreamName)!]?.delete_index
+          }


I see that there are 3 calls to attemptToURIDecode(dataStreamName) in the file and we use the decoded name everywhere. Can we make the call once at the top of the component and used it everywhere?

const decodedName = dataStreamName !== undefined ? attemptToURIDecode(dataStreamName) : undefined;

sebelga · 2020-11-19T09:52:28Z

x-pack/plugins/index_management/common/types/data_streams.ts

@@ -47,3 +47,5 @@ export interface DataStreamIndex {
  name: string;
  uuid: string;
 }
+
+export type DataStreamPrivileges<K extends string> = Record<string, { [key in K]: boolean }>;


Using generic is an interesting approach but I wonder if it is worth the lack of explicitness and what exactly we gain from it.

This would be much clearer to me

export interface DataStreamPrivileges { delete_index?: boolean; }

Whenever we read the interface we know what privileges the data stream app (or section) relies on. What are your thoughts on this?

sebelga · 2020-11-19T10:36:04Z

...nt/public/application/sections/home/data_stream_list/data_stream_table/data_stream_table.tsx

@@ -162,6 +165,9 @@ export const DataStreamTable: React.FunctionComponent<Props> = ({
        },
        isPrimary: true,
        'data-test-subj': 'deleteDataStream',
+        available: ({ name }: DataStream) => {


nit: We could have a method for this and use it here and below (L168)

const hasDeletePrivilege = ({ name }: DataStream) => !deletePrivileges || deletePrivileges[name]?.delete_index; ... available: hasDeletePrivilege ... toolsLeft: selection.length > 0 && (!deletePrivileges || selection.every(hasDeletePrivilege)) ? ( ...

sebelga · 2020-11-19T10:46:26Z

x-pack/plugins/index_management/__jest__/client_integration/home/data_streams_tab.test.ts

@@ -449,4 +460,78 @@ describe('Data Streams tab', () => {
      expect(tableCellsValues).toEqual([['', 'non-managed-data-stream', 'green', '1', 'Delete']]);
    });
  });
+
+  describe('delete data stream privileges', () => {


The title is a bit misleading as it seems that we will delete the privileges :) What about simply: "Privileges: 'delete data stream'"

yuliacech · 2020-11-19T12:20:09Z

Hi @sebelga , thanks a lot for your suggestions! I really like the idea of fetching privileges at the same time as data streams, let me add privileges to the data stream response instead. If you still want to check it locally in the mean time, there is a 'how to test' section in PR description, I've just made the title bigger for better visibility.

sebelga · 2020-11-19T13:28:22Z

Great, I hadn't seen the description sorry! I just tested locally and it works as expected! 👍 I also throttled the requests and indeed the delete button first appears then disappears. It is not a huge deal, if the user is quick enough to click on it before it disappears he will get a non authorized error from ES anyway.
I think that the main advantage of having a single request for both the resource and the privileges is more the simplicity of how it is later consumed in the client. I'll review once you make the change, cheers!

yuliacech · 2020-11-23T09:25:44Z

@elasticmachine merge upstream

yuliacech · 2020-11-23T12:16:45Z

Hi @sebelga , could you please have another look? I moved the privileges request inside the call for data streams as you suggested.

sebelga

Thanks for making those changes @yuliacech ! I think it is much cleaner like this on the client-side. I tested locally and works as expected 👍

I left a few comments but no blockers.
I see that the link to the ILM policy now works from inside the detail panel, I just wonder why we need a full Kibana reload to access it. If we click on ILM in the menu on the left there is no page reload. Maybe something to investigate in a separate work.

sebelga · 2020-11-25T08:36:34Z

x-pack/plugins/index_management/common/types/data_streams.ts

+  delete_index: boolean;
+}
+
+type PrivilegesField = PrivilegesFieldFromEs;


I am not sure I understand why we create an alias here. Do you mind explaining?

Also not sure why there is a "Field" in the naming :) I would have called the interface simply Privileges instead of PrivilegesFieldFromEs but maybe I'm missing something.

sebelga · 2020-11-25T08:44:52Z

x-pack/plugins/index_management/server/routes/api/data_streams/register_get_route.ts

+
+    if (dataStreamsStats) {
+      // eslint-disable-next-line @typescript-eslint/naming-convention
+      const { store_size, maximum_timestamp } = dataStreamsStats.find(


The find can potentially return undefined so I wonder if deconstructing the value without asserting it is defined could potentially throw. I wonder why Typescript does not complain on this 🤔

you are right, it can be undefined at the end of the line for TypeScript not to complain. But I was thinking that we rely too much on ES returning the same data streams in both requests, but if a data stream is deleted between the requests then this logic would fail. So I added a default empty object instead.

yuliacech · 2020-11-25T11:10:46Z

@elasticmachine merge upstream

kibanamachine · 2020-11-26T13:49:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: f86ae06

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`indexManagement`	1.5MB	1.5MB	+257.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`indexManagement`	114.2KB	114.2KB	+61.0B

History

💚 Build #90199 succeeded 9260755
💚 Build #89530 succeeded 475ad3d
💚 Build #89325 succeeded 5eeb8d7
💔 Build #89283 failed 25ea117
💔 Build #89258 failed b9d34f7

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…lastic#83573) * Added data streams privileges to better control delete actions in UI * Fix type check issues * Change data streams privileges request * Fixed type check issue * Fixed api integration test * Cleaned up not needed code * Renamed some data streams and added a default value for stats find Co-authored-by: Kibana Machine <[email protected]>

* master: [Security Solution] Exceptions Cypress tests (elastic#81759) [ML] Fix spaces job ID check (elastic#84404) [Security Solution][Detections] Handle dupes when processing threshold rules (elastic#83062) skip flaky suite (elastic#84440) skip flaky suite (elastic#84445) [APM] Fix missing `service.node.name` (elastic#84269) Upgrade fp-ts to 2.8.6 (elastic#83866) Added data streams privileges to better control delete actions in UI (elastic#83573) Improve short-url redirect validation (elastic#84366) TSVB offsets (elastic#83051) [Discover] Fix navigating back when changing index pattern (elastic#84061) [Logs UI] Polish the UI for the log entry examples in the anomaly table (elastic#82139) [Logs UI] Limit the height of the "view in context" container (elastic#83178) [Application Usage] Update `schema` with new `fleet` rename (elastic#84327) fix identation in list (elastic#84301)

…83573) (#84428) * Added data streams privileges to better control delete actions in UI * Fix type check issues * Change data streams privileges request * Fixed type check issue * Fixed api integration test * Cleaned up not needed code * Renamed some data streams and added a default value for stats find Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

yuliacech added Feature:Index Management Index and index templates UI Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.11.0 v8.0.0 labels Nov 18, 2020

yuliacech marked this pull request as ready for review November 18, 2020 13:46

yuliacech requested a review from a team as a code owner November 18, 2020 13:46

yuliacech requested a review from sebelga November 18, 2020 13:46

yuliacech added the release_note:enhancement label Nov 18, 2020

yuliacech added 2 commits November 19, 2020 11:14

Added data streams privileges to better control delete actions in UI

168dd56

Fix type check issues

1549295

yuliacech force-pushed the data_streams_privileges branch from b2ab845 to 1549295 Compare November 19, 2020 10:16

sebelga reviewed Nov 19, 2020

View reviewed changes

yuliacech marked this pull request as draft November 20, 2020 15:09

yuliacech added 3 commits November 20, 2020 16:09

Change data streams privileges request

b9d34f7

Fixed type check issue

25ea117

Fixed api integration test

5eeb8d7

yuliacech marked this pull request as ready for review November 23, 2020 09:19

kibanamachine and others added 2 commits November 23, 2020 04:26

Merge branch 'master' into data_streams_privileges

3af9550

Cleaned up not needed code

475ad3d

sebelga approved these changes Nov 25, 2020

View reviewed changes

kibanamachine and others added 2 commits November 25, 2020 06:10

Merge branch 'master' into data_streams_privileges

9260755

Renamed some data streams and added a default value for stats find

f86ae06

yuliacech merged commit 51f75a5 into elastic:master Nov 26, 2020

yuliacech mentioned this pull request Nov 26, 2020

[7.x] Added data streams privileges to better control delete actions in UI (#83573) #84428

Merged

yuliacech deleted the data_streams_privileges branch December 3, 2020 10:53

alisonelizabeth mentioned this pull request Dec 15, 2020

Refining UX around user privileges #86016

Open

yuliacech mentioned this pull request Jan 7, 2021

[DOCS] Update data streams list screenshots and description of "delete data stream" elastic/elasticsearch#67145

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added data streams privileges to better control delete actions in UI #83573

Added data streams privileges to better control delete actions in UI #83573

yuliacech commented Nov 17, 2020 •

edited

Loading

yuliacech commented Nov 18, 2020

elasticmachine commented Nov 18, 2020

sebelga left a comment •

edited

Loading

sebelga Nov 19, 2020

sebelga Nov 19, 2020

sebelga Nov 19, 2020 •

edited

Loading

sebelga Nov 19, 2020

yuliacech commented Nov 19, 2020

sebelga commented Nov 19, 2020

yuliacech commented Nov 23, 2020

yuliacech commented Nov 23, 2020

sebelga left a comment

sebelga Nov 25, 2020

sebelga Nov 25, 2020

yuliacech Nov 26, 2020

yuliacech commented Nov 25, 2020

kibanamachine commented Nov 26, 2020

Added data streams privileges to better control delete actions in UI #83573

Added data streams privileges to better control delete actions in UI #83573

Conversation

yuliacech commented Nov 17, 2020 • edited Loading

Summary

Screenshots

How to test

Checklist

TODO

Release note

yuliacech commented Nov 18, 2020

elasticmachine commented Nov 18, 2020

sebelga left a comment • edited Loading

Choose a reason for hiding this comment

sebelga Nov 19, 2020

Choose a reason for hiding this comment

sebelga Nov 19, 2020

Choose a reason for hiding this comment

sebelga Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

sebelga Nov 19, 2020

Choose a reason for hiding this comment

yuliacech commented Nov 19, 2020

sebelga commented Nov 19, 2020

yuliacech commented Nov 23, 2020

yuliacech commented Nov 23, 2020

sebelga left a comment

Choose a reason for hiding this comment

sebelga Nov 25, 2020

Choose a reason for hiding this comment

sebelga Nov 25, 2020

Choose a reason for hiding this comment

yuliacech Nov 26, 2020

Choose a reason for hiding this comment

yuliacech commented Nov 25, 2020

kibanamachine commented Nov 26, 2020

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

History

yuliacech commented Nov 17, 2020 •

edited

Loading

sebelga left a comment •

edited

Loading

sebelga Nov 19, 2020 •

edited

Loading