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

Fixing incorrect error that was being displayed on Share to Spaces flyout #142410

Merged

Conversation

kc13greiner
Copy link
Contributor

@kc13greiner kc13greiner commented Sep 30, 2022

Summary

An error was being displayed when ES was started without security enabled. since the downstream code was expecting a 404 instead of the bad request error that was being sent on the response.

Steps to reproduce

  1. Start ES with security disabled
  2. Login as elastic
  3. Create another space
  4. Load any of the sample data sets
  5. Stack Management > Saved Objects > click on the space icon on one of the saved objects
  6. When the flyout opens, there shouldn't be an error

Before:

Screen Shot 2022-10-03 at 2 22 00 PM

After:

Screen Shot 2022-10-03 at 2 17 03 PM

@kc13greiner kc13greiner linked an issue Sep 30, 2022 that may be closed by this pull request
@kc13greiner kc13greiner marked this pull request as ready for review September 30, 2022 21:51
@kc13greiner kc13greiner requested a review from a team as a code owner September 30, 2022 21:51
@kc13greiner kc13greiner added Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! release_note:skip Skip the PR/issue when compiling release notes labels Sep 30, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

@kc13greiner kc13greiner added v8.5.1 ci:cloud-deploy Create or update a Cloud deployment labels Sep 30, 2022
@watson
Copy link
Contributor

watson commented Oct 3, 2022

I just tried to spin up this PR an I'm still getting an error:

construct@[native code]
construct@[native code]
_construct@[...]/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:216838:26
Wrapper@[...]/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:216788:23
construct@[native code]
_createSuperInternal@[...]/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:216527:33
HttpFetchError@[...]/bundles/core/core.entry.js:11852:24
_callee3$@[...]/bundles/core/core.entry.js:11729:92
tryCatch@[...]/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:216932:21
@[...]/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:216912:32
asyncGeneratorStep@[...]/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:77457:24
_next@[...]/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:77479:27
promiseReactionJob@[native code]

I'm not sure if I'm doing it correct however. Your instructions in the PR description says to "Login as elastic", but since security is disabled, the login screen is skipped. I run Elasticsearch with node scripts/es snapshot -E xpack.security.enabled=false.

@kc13greiner
Copy link
Contributor Author

I just tried to spin up this PR an I'm still getting an error:

construct@[native code]
construct@[native code]
_construct@[...]/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:216838:26
Wrapper@[...]/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:216788:23
construct@[native code]
_createSuperInternal@[...]/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:216527:33
HttpFetchError@[...]/bundles/core/core.entry.js:11852:24
_callee3$@[...]/bundles/core/core.entry.js:11729:92
tryCatch@[...]/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:216932:21
@[...]/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:216912:32
asyncGeneratorStep@[...]/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:77457:24
_next@[...]/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:77479:27
promiseReactionJob@[native code]

I'm not sure if I'm doing it correct however. Your instructions in the PR description says to "Login as elastic", but since security is disabled, the login screen is skipped. I run Elasticsearch with node scripts/es snapshot -E xpack.security.enabled=false.

@watson Sorry for the confusion; Ive updated the instructions to better reflect the steps to reproduce and screenshots of the expected results.

As for the error you posted: was that on startup or when trying to reproduce this bug?

@jeramysoucy
Copy link
Contributor

I'm not sure if I'm doing it correct however. Your instructions in the PR description says to "Login as elastic", but since security is disabled, the login screen is skipped. I run Elasticsearch with node scripts/es snapshot -E xpack.security.enabled=false.

@watson Sorry for the confusion; Ive updated the instructions to better reflect the steps to reproduce and screenshots of the expected results.

As for the error you posted: was that on startup or when trying to reproduce this bug?

I just pulled this down and everything looks good to the eye, but if you open the browser console or network tab you see a 404 for _share_saved_object_permissions when you click the space icon to open the flyout.

@kc13greiner
Copy link
Contributor Author

I'm not sure if I'm doing it correct however. Your instructions in the PR description says to "Login as elastic", but since security is disabled, the login screen is skipped. I run Elasticsearch with node scripts/es snapshot -E xpack.security.enabled=false.

@watson Sorry for the confusion; Ive updated the instructions to better reflect the steps to reproduce and screenshots of the expected results.
As for the error you posted: was that on startup or when trying to reproduce this bug?

I just pulled this down and everything looks good to the eye, but if you open the browser console or network tab you see a 404 for _share_saved_object_permissions when you click the space icon to open the flyout.

I believe the 404 is the desired outcome (I changed the 4XX error to an anticipated 404 as part of this PR) or at least that's how I interpreted this info in the issue

@jeramysoucy
Copy link
Contributor

I believe the 404 is the desired outcome (I changed the 4XX error to an anticipated 404 as part of this PR) or at least that's how I interpreted this info in the issue

Gotcha. Is this an internal-only way of running ES (sec disabled)? If so, I think this is fine.

@watson
Copy link
Contributor

watson commented Oct 4, 2022

As for the error you posted: was that on startup or when trying to reproduce this bug?

That was when I was trying to see if the bug addressed by this PR was fixed. I.e. when I clicked on one of the spaces icons to open the fly-out. Let's sync IRL today to see what's going on

@kc13greiner
Copy link
Contributor Author

kc13greiner commented Oct 4, 2022

I believe the 404 is the desired outcome (I changed the 4XX error to an anticipated 404 as part of this PR) or at least that's how I interpreted this info in the issue

Gotcha. Is this an internal-only way of running ES (sec disabled)? If so, I think this is fine.

Exactly @jeramysoucy - the spaces_manager was looking for a 404 if security is disabled:

.catch((err) => { const isNotFound = err?.body?.statusCode === 404; if (isNotFound) { // security is not enabled return { shareToAllSpaces: true }; }

Copy link
Contributor

@watson watson left a comment

Choose a reason for hiding this comment

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

For some reason I my initial checkout of the PR was still running the old code, hence why I got the same error. Looks fine now 👍

@kibana-ci
Copy link
Collaborator

kibana-ci commented Oct 4, 2022

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@kc13greiner kc13greiner merged commit 36b2296 into elastic:main Oct 4, 2022
@kc13greiner kc13greiner deleted the feature/fix_share_to_spaces_error branch October 4, 2022 16:29
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 4, 2022
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.5

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Oct 4, 2022
… is expected downstream (#142410) (#142638)

(cherry picked from commit 36b2296)

Co-authored-by: Kurt <[email protected]>
WafaaNasr pushed a commit to WafaaNasr/kibana that referenced this pull request Oct 11, 2022
WafaaNasr pushed a commit to WafaaNasr/kibana that referenced this pull request Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:cloud-deploy Create or update a Cloud deployment release_note:skip Skip the PR/issue when compiling release notes Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v8.5.0 v8.5.1 v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The Share To Space UI is broken if security is disabled
6 participants