[Discover] Do not update defaultIndex in case of insufficient permissions #134202

kertal · 2022-06-13T11:41:42Z

Summary

This PR prevents Discover from updating the defaultIndex uiSettings which can lead to a 403 error message when the user doesn't have permissions. getDefaultDataView of DataViewsService returns the needed defaultIndex also without updating, when the user has insufficient permissions. Works also in Dashboard, Visualizations ...

Testing

Add demo date e.g. Kibana eCommerce orders
Create a user having a role that has only readonly permissions to Discover.
Remove the defaultIndex value in Advanced settings.
Login with the new user
When navigating to Discover, there should be no error toast

Fixes #46124

Checklist

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

…cloud_security_posture plugin (elastic#130630)

…e-defaultIndex

elasticmachine · 2022-06-14T06:23:22Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

jughosta

The issue is still present for Dashboard and Visualize apps.

jughosta · 2022-06-14T07:58:48Z

src/plugins/data_views/common/data_views/data_views.ts

    const patterns = await this.getIdsWithTitle();
    let defaultId: string | undefined = await this.config.get('defaultIndex');
    const exists = defaultId ? patterns.some((pattern) => pattern.id === defaultId) : false;

    if (defaultId && !exists) {
-      await this.config.remove('defaultIndex');
+      if (update) {


It could be better here to check user's write-rights before trying to modify the settings. Do we have such API?

that's a good question, @mattkime, can we? of course this would be a better pattern. What we did currently in Discover is, preventing to set the value in config, because it's not necessary in our domain.

Could we use capabilities.advangedSettings.save?

@jughosta yes, thx, we should! I've refactored it to check for permissions, instead of adding another param to the function. This way, it also should work for other consumers of this functionality (Dashboard ...) ... let's see, if CI agrees

kertal · 2022-06-14T08:38:20Z

The issue is still present for Dashboard and Visualize apps.

Yes, we can be sure that it works in Discover, but e.g in has to be set defaultIndex has to be set, because it's requested in TSVB .. however it shouldn't be a big thing to remove it, so I would suggest opening 2 separate issues so the teams can check.

kertal · 2022-06-17T18:15:22Z

@elasticmachine merge upstream

kertal · 2022-06-20T07:54:56Z

@elasticmachine merge upstream

src/plugins/data_views/common/data_views/data_views.ts

kertal · 2022-06-21T07:05:08Z

@elasticmachine merge upstream

jughosta

Thanks for the changes! LGTM 👍

We should probably also update the copy for settings with "data view"

mattkime

I'd rather see the defaultIndex value updated with a system level privileged but I'm not sure thats possible right now so 🚢 it!

Oh, I'd like to see getCanSaveAdvancedSettings marked private before this is merged

mattkime · 2022-06-22T04:53:09Z

src/plugins/data_views/common/data_views/data_views.ts

+  /**
+   * Can the user save advanced settings?
+   */
+  public getCanSaveAdvancedSettings: () => Promise<boolean>;


This should be private, as its only use is internal to getDefaultDataView

@mattkime I did but now TypeScript is complaining, can you help

property 'getCanSaveAdvancedSettings' is private in type 'DataViewsServicePublic' but not in type 'DataViewsServicePublic'.

kibana-ci · 2022-06-24T05:15:45Z

💚 Build Succeeded

Metrics [docs]

Page load bundle

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

id	before	after	diff
`dataViews`	40.5KB	40.8KB	+281.0B

Unknown metric groups

API count

id	before	after	diff
`dataViews`	927	928	+1

History

💔 Build #52428 failed cab0d7f
💔 Build #52405 failed 85042cff69036bbe85b88c8fda96c6858353f3a9
💔 Build #52300 failed cab0d7f
💚 Build #52148 succeeded 94b48f0
💔 Build #52013 failed 9093a00

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

cc @kertal

kertal · 2022-06-24T05:16:46Z

@mattkime :) so simple! Thx!

mistic and others added 2 commits June 10, 2022 11:08

fix(NA): update codeowners for kibana-cloud-security-posture team on …

6eca532

…cloud_security_posture plugin (elastic#130630)

Implement to not update defaultIndex if not set

2e842bf

kertal self-assigned this Jun 13, 2022

kertal added Feature:Discover Discover Application Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Jun 13, 2022

kertal added 2 commits June 13, 2022 18:15

Merge remote-tracking branch 'upstream/main' into discover-omit-updat…

6e33cc1

…e-defaultIndex

Fix tests, improve docs

f88d295

kertal changed the title ~~[Discover] Do not update defaultIndex when not set~~ [Discover] Do not update defaultIndex when not set or invalid Jun 14, 2022

kertal added release_note:fix v8.4.0 labels Jun 14, 2022

Fix type error

f70a0ba

kertal marked this pull request as ready for review June 14, 2022 06:23

kertal requested review from a team as code owners June 14, 2022 06:23

kertal added the :DataDiscovery/fix-it-week label Jun 14, 2022

jughosta reviewed Jun 14, 2022

View reviewed changes

kertal requested a review from mattkime June 14, 2022 08:41

Merge branch 'main' into discover-omit-update-defaultIndex

ed17a52

Merge branch 'main' into discover-omit-update-defaultIndex

ca0ebe3

mattkime requested changes Jun 20, 2022

View reviewed changes

src/plugins/data_views/common/data_views/data_views.ts Outdated Show resolved Hide resolved

Refactor to check for permissions before updating advancedSettings

6d4a5a1

Merge branch 'main' into discover-omit-update-defaultIndex

9093a00

jughosta approved these changes Jun 21, 2022

View reviewed changes

Fix type

94b48f0

kertal requested a review from mattkime June 21, 2022 15:59

kertal changed the title ~~[Discover] Do not update defaultIndex when not set or invalid~~ [Discover] Do not update defaultIndex when insufficient permissions Jun 22, 2022

mattkime approved these changes Jun 22, 2022

View reviewed changes

mattkime reviewed Jun 22, 2022

View reviewed changes

Define getCanSaveAdvancedSettings as privat

cab0d7f

kertal force-pushed the discover-omit-update-defaultIndex branch from 85042cf to cab0d7f Compare June 22, 2022 14:03

mattkime added 2 commits June 23, 2022 22:35

Merge branch 'main' into discover-omit-update-defaultIndex

cb7d04b

typescript fix

568c1b3

kertal changed the title ~~[Discover] Do not update defaultIndex when insufficient permissions~~ [Discover] Do not update defaultIndex in case of insufficient permissions Jun 24, 2022

kertal merged commit b475ed6 into elastic:main Jun 24, 2022

kibanamachine added the backport:skip This commit does not require backporting label Jun 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Do not update defaultIndex in case of insufficient permissions #134202

[Discover] Do not update defaultIndex in case of insufficient permissions #134202

kertal commented Jun 13, 2022 •

edited

Loading

elasticmachine commented Jun 14, 2022

jughosta left a comment

jughosta Jun 14, 2022

kertal Jun 14, 2022

jughosta Jun 16, 2022

kertal Jun 21, 2022

kertal commented Jun 14, 2022

kertal commented Jun 17, 2022

kertal commented Jun 20, 2022

kertal commented Jun 21, 2022

jughosta left a comment

mattkime left a comment •

edited

Loading

mattkime Jun 22, 2022

kertal Jun 22, 2022

kibana-ci commented Jun 24, 2022

API count

kertal commented Jun 24, 2022

[Discover] Do not update defaultIndex in case of insufficient permissions #134202

[Discover] Do not update defaultIndex in case of insufficient permissions #134202

Conversation

kertal commented Jun 13, 2022 • edited Loading

Summary

Testing

Checklist

elasticmachine commented Jun 14, 2022

jughosta left a comment

Choose a reason for hiding this comment

jughosta Jun 14, 2022

Choose a reason for hiding this comment

kertal Jun 14, 2022

Choose a reason for hiding this comment

jughosta Jun 16, 2022

Choose a reason for hiding this comment

kertal Jun 21, 2022

Choose a reason for hiding this comment

kertal commented Jun 14, 2022

kertal commented Jun 17, 2022

kertal commented Jun 20, 2022

kertal commented Jun 21, 2022

jughosta left a comment

Choose a reason for hiding this comment

mattkime left a comment • edited Loading

Choose a reason for hiding this comment

mattkime Jun 22, 2022

Choose a reason for hiding this comment

kertal Jun 22, 2022

Choose a reason for hiding this comment

kibana-ci commented Jun 24, 2022

💚 Build Succeeded

Metrics [docs]

Page load bundle

API count

History

kertal commented Jun 24, 2022

kertal commented Jun 13, 2022 •

edited

Loading

mattkime left a comment •

edited

Loading