[ML] Update file data visualizer permissions #101169

jgowdyelastic · 2021-06-02T14:50:27Z

Removes the ML supplied capability access:fileUpload:import and adds fileUpload:analyzeFile to Discover's api all capabilities so the feature can be used in Home's Add Data page without the user needing any ML permissions.
Using Discover's capabilities was suggested here as the ability to analyze a file should be a very low permission and it is reasonable to assume that a user with the ability to use Discover could also have the ability to analyze a file.

If the file analysis fails with a 403 a permission warning is shown.

@lcawl or @szabosteve can you please check the wording of the warning for me?

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for breaking API changes and was labeled appropriately

jgowdyelastic · 2021-06-08T13:36:02Z

@elasticmachine merge upstream

jgowdyelastic · 2021-06-14T09:22:36Z

@elasticmachine merge upstream

kibanamachine · 2021-06-14T09:22:38Z

merge conflict between base and head

jgowdyelastic · 2021-06-17T17:10:00Z

@elasticmachine merge upstream

kibanamachine · 2021-06-17T17:10:02Z

merge conflict between base and head

…sualizer-permissions

jgowdyelastic · 2021-06-18T11:22:04Z

@elasticmachine merge upstream

droberts195 · 2021-06-21T13:34:02Z

IMO, the ability to analyse a file should not require a high level of permissions

Yes, I agree. This stage is only telling the user things about the file they uploaded, and could have worked out themselves by counting things in a text editor on their machine if they had the time and inclination. They can cause CPU usage in Elasticsearch by analysing lots of files, although they can do that anyway by running searches. So I think for the file analysis stage of the process a very low level of permissions is adequate. Something on a par with using Discover.

Then, as people have said, to actually import the file you need to be able to create an index, create an ingest pipeline and index documents into that index. But that's always been the case and has always been controlled using ES permissions.

I can't remember what decisions were made when the _text_structure/find_structure was added and why monitor_text_structure was used.

In Elasticsearch every action has to have an action name and the permissions allow you to use actions whose name begins with a certain prefix. All the action names that are for cluster actions rather than index actions and which don't make changes begin with monitor. Then the monitor cluster privilege lets you call all those actions. However, even though it's giving access to read-only actions, monitor is still a pretty high level of privilege because it lets you get the cluster state, and if you can get the cluster state then you can find out an enormous amount of information that might allow you to attack the cluster in other ways.

When _text_structure/find_structure was _ml/find_file_structure the lower privilege level that gave access was monitor_ml. When we moved it out of the ML plugin monitor_ml no longer gave access. So we added a new privilege, monitor_text_structure, to give admins a way to grant users access to the action without having to give them monitor (which is too high for a lowly user). However, that doesn't mean it's friendly for Kibana to require that lowly Kibana users have that Elasticsearch privilege.

To summarise, I think it's fine to either not protect the analysis step (beyond having to log into Kibana) and bearing in mind that the upload privilege checks that happen naturally due to creating an index and an ingest pipeline protect the upload step or protect the analysis step with a really basic Kibana privilege like being allowed to use Discover or something else that's widely granted, again with the upload step protected naturally.

jgowdyelastic · 2021-06-22T14:32:15Z

@elasticmachine merge upstream

jgowdyelastic · 2021-06-29T06:14:53Z

@elasticmachine merge upstream

jgowdyelastic · 2021-06-29T11:56:17Z

@legrego i've updated this PR with your suggested changes. Could you please take a look. Also I'm not that happy with the text in the error callout, but I can't think of a better way of wording it.

legrego · 2021-06-29T12:59:56Z

x-pack/plugins/features/server/oss_features.ts

@@ -37,6 +37,7 @@ export const buildOSSFeatures = ({
      privileges: {
        all: {
          app: ['discover', 'kibana'],
+          api: ['fileUpload:analyzeFile'],


I know we discussed Discover as one example of a feature that should grant this functionality, but is that the only one? If I'm reading this right, it looks like we are taking this away from the maps and ml features, which would surprise our users when they upgrade

Yes, failing tests have just shown that ML still needs to supply this privilege.
Maps does not as it does not use the file structure finder feature.

Updated in 3d3c2c0

legrego · 2021-06-29T13:41:36Z

...pplication/file_data_visualizer/components/file_data_visualizer_view/file_error_callouts.tsx

+      >
+        <FormattedMessage
+          id="xpack.dataVisualizer.file.fileErrorCallouts.findFileStructurePermissionDenied.description"
+          defaultMessage="You do not have sufficient privileges to analyze files."


I like this updated text 👍

qn895 · 2021-06-29T15:02:06Z

Code LGTM 🎉

kibanamachine · 2021-06-29T17:15:48Z

💚 Build Succeeded

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
`dataVisualizer`	1.1MB	1.1MB	+1.3KB
`ml`	5.9MB	5.9MB	-20.0B
total			+1.3KB

History

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

cc @jgowdyelastic

* [ML] Update file data visualizer permissions * adding home bundle * fixing translations * removing home from bundles * switching to current user for analysis * adding find structure permission check * clean up * updating text * updating maps * removing has_find_file_structure_permission endpoint * removing more code * adding permission error message * renaming variable * adding fileUpload:analyzeFile back into ML * updating error text * updating snapshots Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-06-29T20:10:10Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

* [ML] Update file data visualizer permissions * adding home bundle * fixing translations * removing home from bundles * switching to current user for analysis * adding find structure permission check * clean up * updating text * updating maps * removing has_find_file_structure_permission endpoint * removing more code * adding permission error message * renaming variable * adding fileUpload:analyzeFile back into ML * updating error text * updating snapshots Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: James Gowdy <jgowdy@elastic.co>

jgowdyelastic added 3 commits June 2, 2021 15:48

[ML] Update file data visualizer permissions

22b84ab

adding home bundle

15df2e6

fixing translations

2473bcf

This was referenced Jun 2, 2021

[Home] Adding file upload to add data page #100863

Merged

Change file data visualizer links to point to new location in home application #101393

Merged

Merge branch 'master' into update-file-data-visualizer-permissions

1e199bd

jgowdyelastic added 2 commits June 17, 2021 18:12

Merge remote-tracking branch 'origin/master' into update-file-data-vi…

5fd67e8

…sualizer-permissions

removing home from bundles

8cdff37

kibanamachine and others added 4 commits June 18, 2021 07:22

Merge branch 'master' into update-file-data-visualizer-permissions

988a9fa

switching to current user for analysis

ef820e5

adding find structure permission check

62a7868

clean up

d542bf8

jgowdyelastic requested review from peteharverson, qn895 and nreese June 18, 2021 14:30

jgowdyelastic self-assigned this Jun 18, 2021

Merge branch 'master' into update-file-data-visualizer-permissions

db90076

kibanamachine and others added 2 commits June 29, 2021 02:14

Merge branch 'master' into update-file-data-visualizer-permissions

34518bf

removing has_find_file_structure_permission endpoint

8e7c537

jgowdyelastic requested a review from a team as a code owner June 29, 2021 10:59

jgowdyelastic added 3 commits June 29, 2021 12:08

removing more code

df2b11b

adding permission error message

d77e042

renaming variable

fc9949f

legrego reviewed Jun 29, 2021

View reviewed changes

jgowdyelastic added 2 commits June 29, 2021 14:19

adding fileUpload:analyzeFile back into ML

3d3c2c0

updating error text

6e49678

legrego reviewed Jun 29, 2021

View reviewed changes

qn895 approved these changes Jun 29, 2021

View reviewed changes

updating snapshots

940f7b1

legrego approved these changes Jun 29, 2021

View reviewed changes

pgayvallet approved these changes Jun 29, 2021

View reviewed changes

jgowdyelastic merged commit 21dad7e into elastic:master Jun 29, 2021

kibanamachine mentioned this pull request Jun 29, 2021

[7.x] [ML] Update file data visualizer permissions (#101169) #103763

Merged

jgowdyelastic deleted the update-file-data-visualizer-permissions branch June 30, 2021 08:09

lcawl mentioned this pull request Aug 4, 2021

[DOCS] Updates file data visualizer details #107609

Merged

jgowdyelastic mentioned this pull request Aug 5, 2021

Consider adding a File Upload or Add Data reserved role #107731

Closed

lcawl mentioned this pull request Aug 10, 2021

[DOCS] Updates file upload details for geospatial data #107985

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Update file data visualizer permissions #101169

[ML] Update file data visualizer permissions #101169

jgowdyelastic commented Jun 2, 2021 •

edited

Loading

jgowdyelastic commented Jun 8, 2021

jgowdyelastic commented Jun 14, 2021

kibanamachine commented Jun 14, 2021

jgowdyelastic commented Jun 17, 2021

kibanamachine commented Jun 17, 2021

jgowdyelastic commented Jun 18, 2021

droberts195 commented Jun 21, 2021

jgowdyelastic commented Jun 22, 2021

jgowdyelastic commented Jun 29, 2021

jgowdyelastic commented Jun 29, 2021

legrego Jun 29, 2021

jgowdyelastic Jun 29, 2021

jgowdyelastic Jun 29, 2021

legrego Jun 29, 2021

qn895 commented Jun 29, 2021

kibanamachine commented Jun 29, 2021

kibanamachine commented Jun 29, 2021

[ML] Update file data visualizer permissions #101169

[ML] Update file data visualizer permissions #101169

Conversation

jgowdyelastic commented Jun 2, 2021 • edited Loading

Checklist

jgowdyelastic commented Jun 8, 2021

jgowdyelastic commented Jun 14, 2021

kibanamachine commented Jun 14, 2021

jgowdyelastic commented Jun 17, 2021

kibanamachine commented Jun 17, 2021

jgowdyelastic commented Jun 18, 2021

droberts195 commented Jun 21, 2021

jgowdyelastic commented Jun 22, 2021

jgowdyelastic commented Jun 29, 2021

jgowdyelastic commented Jun 29, 2021

legrego Jun 29, 2021

Choose a reason for hiding this comment

jgowdyelastic Jun 29, 2021

Choose a reason for hiding this comment

jgowdyelastic Jun 29, 2021

Choose a reason for hiding this comment

legrego Jun 29, 2021

Choose a reason for hiding this comment

qn895 commented Jun 29, 2021

kibanamachine commented Jun 29, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

kibanamachine commented Jun 29, 2021

💚 Backport successful

jgowdyelastic commented Jun 2, 2021 •

edited

Loading