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.

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

[Discover] Prevent error message of read only user without default index pattern set #54122

Conversation

kertal
Copy link
Member

@kertal kertal commented Jan 7, 2020

Summary

Given there's no default index pattern set and a read only user accesses Discover, he'll get the error message: "Unable to update UI setting"

Bildschirmfoto 2020-01-07 um 14 47 55

Discover tries to set and persist a new default index pattern. But without permissions, this fails.
This PR extends the ensureDefaultIndexPattern function with a persist flag, so Discover can get a default index pattern without persisting it.

Testing

  1. Create a readonly role and user for discover.
  2. Go to Advanced Settings and remove the default index pattern. Bildschirmfoto 2020-01-07 um 15 28 01
  3. Login with this user and go to discover.

Without this PR you should get the error message.

Fixes #46124

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [ ] This was checked for cross-browser compatibility, including a check against IE11
- [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), 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 keyboard-only and screenreader accessibility

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately
- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately

@kertal kertal self-assigned this Jan 7, 2020
@kertal kertal added :KibanaApp/fix-it-week Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes v7.6.0 v8.0.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Jan 7, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@kertal kertal marked this pull request as ready for review January 7, 2020 17:03
@kertal kertal requested review from a team and flash1293 January 7, 2020 17:03
@flash1293
Copy link
Contributor

@elasticmachine merge upstream

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

There are a bunch of calls to this in visualize and dashboard as well:

src/legacy/core_plugins/kibana/public/visualize/np_ready/legacy_app.js
30:  ensureDefaultIndexPattern,
88:            ensureDefaultIndexPattern(deps.core, deps.data, $rootScope, kbnUrl),
100:            ensureDefaultIndexPattern(deps.core, deps.data, $rootScope, kbnUrl),
126:            return ensureDefaultIndexPattern(core, data, $rootScope, kbnUrl)
149:            return ensureDefaultIndexPattern(core, data, $rootScope, kbnUrl)

src/legacy/core_plugins/kibana/public/dashboard/np_ready/legacy_app.js
26:  ensureDefaultIndexPattern,
136:            return ensureDefaultIndexPattern(deps.core, deps.npDataStart, $rootScope, kbnUrl).then(
176:            return ensureDefaultIndexPattern(deps.core, deps.npDataStart, $rootScope, kbnUrl)
196:            return ensureDefaultIndexPattern(deps.core, deps.npDataStart, $rootScope, kbnUrl)

Wouldn't they have the same problem? And if they do and we also need to set persist to false we can remove the logic of actually setting the default index pattern because it's never safe to call it. Additionally we could try to set in the settings and just hide the error (not sure whether that's possible). Also we should check whether there are any calls that depend on a default index pattern after the ensureDefaultIndexPattern check. So far this could always be assumed.

@kertal
Copy link
Member Author

kertal commented Jan 13, 2020

thx @flash1293 for you review, I'll do research about visualize & dashboard, we might not need this kind of persistence, and what is better than adding code? removing code 💇‍♂

@kertal kertal added v7.7.0 and removed v7.6.0 labels Jan 15, 2020
@kertal
Copy link
Member Author

kertal commented Feb 4, 2020

@elasticmachine merge upstream

@dhurley14 dhurley14 requested a review from a team as a code owner February 27, 2020 16:11
@kertal kertal added v7.8.0 and removed v7.7.0 labels Mar 25, 2020
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested and works as expected, default index pattern is not set automatically anymore but no failures with readonly users. LGTM besides my comment above and @lizozom s remark.

@lizozom
Copy link
Contributor

lizozom commented May 31, 2020

@kertal do we still want to merge this?

@kertal
Copy link
Member Author

kertal commented Jun 2, 2020

@kertal do we still want to merge this?

currently no, but I will have a closer look at it this week

- in no longer persists a new uiSettings defaultIndex in case there in none
@kertal kertal force-pushed the kertal-pr-2020-01-07-fix-discover-default-index-readonly-error branch from 65691e0 to 3e468f9 Compare June 4, 2020 11:09
@kertal kertal requested a review from lizozom June 5, 2020 07:56
@kertal
Copy link
Member Author

kertal commented Jun 8, 2020

@elasticmachine merge upstream

@kertal kertal added v7.9.0 and removed v7.8.0 labels Jun 8, 2020
@kertal kertal added this to In progress in Discover via automation Jun 8, 2020
@kertal
Copy link
Member Author

kertal commented Jun 10, 2020

@elasticmachine merge upstream

@kertal kertal added release_note:fix and removed release_note:skip Skip the PR/issue when compiling release notes labels Jun 10, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@kertal kertal closed this Jul 1, 2020
@kertal kertal deleted the kertal-pr-2020-01-07-fix-discover-default-index-readonly-error branch July 1, 2020 10:15
Discover automation moved this from In progress to Done Jul 1, 2020
@alexnelson-es
Copy link

How do I determine which Kibana release will have this fix?

@alexnelson-es
Copy link

I upgraded to the last OpenDistro release (1.13.1.0) and the problem still exists.
When I login as a readonly user, I get the (scary for my users) message about the 403 forbidden.
I do have a default index created.
I guess I'll file another ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Discover Discover Application release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.0 v8.0.0
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Error when accessing Discover as a read only user without index patterns / default index pattern
6 participants