-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Discover] Prevent error message of read only user without default index pattern set #54122
Conversation
Pinging @elastic/kibana-app (Team:KibanaApp) |
@elasticmachine merge upstream |
There was a problem hiding this 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.
src/legacy/ui/public/legacy_compat/ensure_default_index_pattern.tsx
Outdated
Show resolved
Hide resolved
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 💇♂ |
@elasticmachine merge upstream |
src/plugins/kibana_utils/public/history/ensure_default_index_pattern.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
src/plugins/kibana_utils/public/history/ensure_default_index_pattern.tsx
Outdated
Show resolved
Hide resolved
@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
65691e0
to
3e468f9
Compare
…-07-fix-discover-default-index-readonly-error
@elasticmachine merge upstream |
…index-readonly-error
@elasticmachine merge upstream |
…index-readonly-error
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
How do I determine which Kibana release will have this fix? |
I upgraded to the last OpenDistro release (1.13.1.0) and the problem still exists. |
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"
Discover tries to set and persist a new default index pattern. But without permissions, this fails.
This PR extends the
ensureDefaultIndexPattern
function with apersist
flag, so Discover can get a default index pattern without persisting it.Testing
Without this PR you should get the error message.
Fixes #46124
Checklist
Use
strikethroughsto 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 accessibilityFor 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