[Dasbhoard][Short URL] Parse embeded in the short URL

[majagrubic]

Summary

Fixes:
#50107

When parsing a short URL, we never took into account embed param. Once the param is set, the embedded dashboard is rendered correctly.

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, 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

[elasticmachine]

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

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 9aae2af

History

• 💔 Build #19117 failed fea39fa

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

[flash1293]

Didn't test yet, but please verify whether this works with both the "new" short url service and the old one that's still in place for a transition period. The old one is used when sessionStorage for state is enabled in the advanced settings.

[kertal]

✅ Tested it with the "new short url service"
❎ Tested it with the "old short url service", currently it doesn't work (thx for the hint @flash1293 )

I think the following code has to be adapted, also the forwarding from goto to goto_LP:

https://github.com/elastic/kibana/blob/master/src/legacy/server/url_shortening/routes/goto.js#L29

[flash1293]

It's more work but we can "do this right' by migrating the remaining piece over to NP. The thing blocking this got merged recently: #52161

I would have started to work on this soon anyway. But I'm also fine with fixing this in both places to have this bug resolved for 7.6

Your call @majagrubic

[majagrubic]

Thanks for spotting this 🙌 Let's do this the right way, even if it means it won't get in by 7.6

[flash1293]

Adding the migration label as this just became a migration-relevant issue :) Thanks @majagrubic !

[flash1293]

@joshdover The old implementation of short urls with session storage enabled used an injected var to place the actual target url into the meta data so the client can forward to the actual URL after hash the actual data (src/legacy/core_plugins/state_session_storage_redirect/public/index.js). However as we want to get rid of injected meta data we have to find another way.

A clean way of implementing this is something like this: Add a very small app via the core application service (app/redirect) that is rendered by the goto route when session storage is enabled. This app calls a new API /api/short_url/:id and fetches the target url again in a second request. Then it hashes the URL parameters and redirects - basically what src/legacy/core_plugins/state_session_storage_redirect/public/index.js is doing today. The downside of this is that an additional roundtrip is introduced in comparison to the current setup.

What do you think? Is there a better approach?

[joshdover]

@flash1293 The workaround you mentioned should work, though I'd recommend creating a "chromeless" application that has its own custom route that matches the backend route (/goto/...) to avoid an additional redirect.

Though after taking a quick look at src/legacy/core_plugins/state_session_storage_redirect/public/index.js it seems that the hashUrl function doesn't access any state in the browser? It's possible I'm missing something (the kibana_utils code here is convoluted), but if it's not relying on anything that has to be executed in the browser, can we not run this code on the server-side instead?

[flash1293]

@joshdover

I'd recommend creating a "chromeless" application that has its own custom route that matches the backend route (/goto/...) to avoid an additional redirect.

Thanks for the hint. I never worked with chromeless applications, so not 100% sure how they function - in most cases the redirected URL will resolve to something in the legacy plugin, so we would have to subsequently load the legacy kibana app without a page reload. Will that work?

Though after taking a quick look at src/legacy/core_plugins/state_session_storage_redirect/public/index.js it seems that the hashUrl function doesn't access any state in the browser? It's possible I'm missing something (the kibana_utils code here is convoluted), but if it's not relying on anything that has to be executed in the browser, can we not run this code on the server-side instead?

It's indeed hard to follow but the hashUrl function eventually calls persistState in expandedStateToHashedState in src/plugins/kibana_utils/public/state_management/state_encoder/encode_decode_state.ts which puts the actual URL parameters into the local storage and replaces them with references (the "hashes") in the URL. So that definitely has to happen on the client (and we can't pass the parameters to the client via another way because the point of session storage is to avoid long URL parameters at all times)

[joshdover]

Thanks for the hint. I never worked with chromeless applications, so not 100% sure how they function - in most cases the redirected URL will resolve to something in the legacy plugin, so we would have to subsequently load the legacy kibana app without a page reload. Will that work?

I'm sure it will work, but I think the question is whether or not that extra hop it is acceptable. Once the apps that this is redirecting to are on the New Platform, this wouldn't require a full-page refresh. Maybe it'd be better to hold off on migrating this part until most or all of the applications are running as NP apps.

I don't see us adding support for injectedMetadata in the New Platform because it can slow down the perceived load time of Kibana by delaying the first byte of the response. Ideally we want to be loading everything as soon as we can so the user gets feedback quickly.

[flash1293]

@joshdover It's already doing a full page refresh, the only thing getting worse would be the roundtrip to the server fetching the redirect url whereas currently it's embedded within the initial html document. But as I'm thinking about it - it's also saving a roundtrip because it doesn't have to redirect.

Current state:

• Request /goto route (roundtrip 1)
• Redirect to /goto_LP route (roundtrip 2)
• Load /goto_LP route with unhashed URL embedded in HTML (pageload 1)
• Put URL parameters in session storage
• Redirect to actual path with hashed parameters (pageload 2) (roundtrip 3)

State after the approach outlined above:

• Request /goto route (roundtrip 1)
• Load /goto app without URL in payload (pageload 1)
• Do fetch call fetching unhashed URL (roundtrip 2)
• Put URL parameters in session storage
• Redirect to actual path with hashed parameters (pageload 2) (roundtrip 3)

State after apps went to NP:

• Request /goto route (roundtrip 1)
• Load /goto app without URL in payload (pageload 1)
• Do fetch call fetching unhashed URL (roundtrip 2)
• Put URL parameters in session storage
• Load bundle for target app (roundtrip 3)
• Show target app without pageload

We could hack in something like injected meta data just for this one case (I know what you are thinking...) by manipulating the string returned by the render function to save roundtrip 2 because the unhashed URL will already be embedded into the initial HTML of the pageload. But it's probably not super important to do so and not worth introducing a hack which is difficult to maintain.

[joshdover]

We could hack in something like injected meta data just for this one case (I know what you are thinking...) by manipulating the string returned by the render function to save roundtrip 2 because the unhashed URL will already be embedded into the initial HTML of the pageload. But it's probably not super important to do so and not worth introducing a hack which is difficult to maintain.

Agreed, probably not worth it. Please add any thoughts you have to #54925 which I have opened to re-discuss adding an injected metadata-like API.

[flash1293]

Superseded by #58846