Adding an app for redirects when storing state in session storage #10822

kobelb · 2017-03-20T21:00:26Z

When using the state:storeInSessionStorage setting with the short-urls, we were previously doing a reply().redirect(fullUrl) and then the state was being extracted from the URL and being put into sessionStorage. This was causing issues with IE where the URL was too long.

This approach uses a separate 'app' and a variable that it's grabbing from the kbn-initial-state to get all the information to the browser, where it then puts it in sessionStorage before redirecting the user to the URL with the shortened hashes. It's not an incredibly elegant solution, and the user experience is somewhat 'choppy' for lack of a better description.

I'm open to alternatives to the current approach, but what's in this PR should work.

Resolves #10128

kobelb · 2017-03-21T14:27:30Z

jenkins test this

spalger

Works great, while I still don't love the solution we don't have many options 🤷‍♂️

spalger · 2017-04-07T17:37:36Z

src/core_plugins/state_session_storage_redirect/public/index.js

+};
+
+import uiRoutes from 'ui/routes';
+uiRoutes.enable();


Do we really need the router here? Enabling the router runs all of the route setup work, which just slows down this simple page.

Makes sense, I'll remove it.

spalger · 2017-04-07T17:37:43Z

src/core_plugins/state_session_storage_redirect/public/index.js

+import { parse as parseUrl, format as formatUrl } from 'url';
+import { stringify as stringifyQuerystring } from 'querystring';
+
+const conservativeStringifyQuerystring = (query) => {


All of this hashing logic should be put in the state_management module next to src/ui/public/state_management/state_hashing/unhash_url.js, and it definitely needs tests.

spalger · 2017-04-07T17:38:04Z

src/server/http/index.js

+        const uiSettings = server.uiSettings();
+        const stateStoreInSessionStorage = await uiSettings.get(request, 'state:storeInSessionStorage');
+        if (!stateStoreInSessionStorage) {
+          reply().redirect(config.get('server.basePath') + url);


Make sure to return here

spalger · 2017-04-07T17:45:41Z

src/server/http/index.js

      } catch (err) {
+        console.log(err);


Make sure to remove this

Good catch, thanks!

spalger · 2017-04-07T17:46:38Z

src/ui/index.js

@@ -83,12 +83,12 @@ export default async (kbnServer, server, config) => {
      vars: await reduceAsync(
        uiExports.injectedVarsReplacers,
        async (acc, replacer) => await replacer(acc, request, server),
-        defaults(await app.getInjectedVars() || {}, uiExports.defaultInjectedVars)
+        defaults(await app.getInjectedVars() || {}, uiExports.defaultInjectedVars, vars)


I think we should rename vars -> injectedVarsOverrides and move it to the front of the list here. If the code calling renderApp() has specific vars in mind they shouldn't be overridden by the defaults or the standard injected vars, only the "replacers" could override them.

kobelb · 2017-04-11T13:10:12Z

@spalger I've addressed your concerns/comments, except the comment about not using the routing. I tried to do the following to remove the need for routing 14b09e3 but this led to the code being run whenever the javascript was loaded, is there a different way that I should be removing the need to use routing?

spalger · 2017-04-11T18:03:49Z

this led to the code being run whenever the javascript was loaded

I'm not sure I understand the issue.

kobelb · 2017-04-11T18:27:54Z

If the following is run every-time that load Kibana in a browser:

const redirectUrl = chrome.getInjected('redirectUrl');

const hashedUrl = hashUrl([new AppState(), globalState], redirectUrl);
const url = chrome.addBasePath(hashedUrl);

$window.location = url;

it'll always try to redirect the user, when we only want it to be ran after the following call:

 const app = kbnServer.uiExports.apps.byId.stateSessionStorageRedirect;
 reply.renderApp(app, {
  redirectUrl: url,
});

stacey-gammon · 2017-04-17T17:32:21Z

fixes #10128

stacey-gammon

LGTM, aside from a couple minor things.

stacey-gammon · 2017-04-17T18:52:17Z

src/ui/public/state_management/state_hashing/hash_url.js

+  });
+};
+
+export default hashUrl;


no more default exports, pleeease :) I will add an eslint rule to the ui/public folder this week, working on the last chunk of conversions.

you got it! old habits die hard...

stacey-gammon · 2017-04-17T18:52:44Z

src/core_plugins/state_session_storage_redirect/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "state_session_storage_redirect",
+  "version": "kibana"


Either here in a description block, or in index.js, can you add a comment describing the need for this and link to the github issue?

Good call, I'll add a description in here.

…astic#10822) * Adding an app for redirects when storing state in session storage * Removing errant console.log * Adding early return after reply().redirect * No longer using the router * Renaming vars to injectedVarsOverrides * Putting uiRoutes back in so the code is only executed for this app * Extracting hash_url to it's own module, and adding tests * Addressing peer-review comments

…0822) (#11305) * Adding an app for redirects when storing state in session storage * Removing errant console.log * Adding early return after reply().redirect * No longer using the router * Renaming vars to injectedVarsOverrides * Putting uiRoutes back in so the code is only executed for this app * Extracting hash_url to it's own module, and adding tests * Addressing peer-review comments

kobelb · 2017-04-18T16:24:35Z

Backported

5.x - 9ca5059

kobelb added v6.0.0 v5.4.0 labels Mar 20, 2017

kobelb requested a review from spalger March 20, 2017 21:01

tbragin added the :Sharing label Mar 21, 2017

spalger suggested changes Apr 7, 2017

View reviewed changes

kobelb force-pushed the state_session_storage_redirect branch from a45365b to e3051c3 Compare April 11, 2017 13:07

kobelb added v5.5.0 and removed v5.4.0 labels Apr 12, 2017

spalger approved these changes Apr 14, 2017

View reviewed changes

kobelb requested a review from stacey-gammon April 14, 2017 17:35

stacey-gammon approved these changes Apr 17, 2017

View reviewed changes

kobelb added 8 commits April 18, 2017 09:18

Adding an app for redirects when storing state in session storage

db1d6ef

Removing errant console.log

b8bd200

Adding early return after reply().redirect

1dd85f9

No longer using the router

aaa5b30

Renaming vars to injectedVarsOverrides

4151bdb

Putting uiRoutes back in so the code is only executed for this app

9f7ce32

Extracting hash_url to it's own module, and adding tests

21c09b0

Addressing peer-review comments

3f559e3

kobelb force-pushed the state_session_storage_redirect branch from e3051c3 to 3f559e3 Compare April 18, 2017 13:55

kobelb merged commit 6d81baf into elastic:master Apr 18, 2017

stacey-gammon mentioned this pull request Apr 18, 2017

state:storeInSessionStorage is not respected for generated short urls #10128

Closed

kobelb added the release_note:fix label Jun 15, 2017

kobelb mentioned this pull request Nov 14, 2019

Remove session storage redirect plugin? #50649

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding an app for redirects when storing state in session storage #10822

Adding an app for redirects when storing state in session storage #10822

kobelb commented Mar 20, 2017 •

edited

Loading

kobelb commented Mar 21, 2017

spalger left a comment

spalger Apr 7, 2017

kobelb Apr 7, 2017

spalger Apr 7, 2017

spalger Apr 7, 2017 •

edited

Loading

spalger Apr 7, 2017

kobelb Apr 7, 2017

spalger Apr 7, 2017

kobelb commented Apr 11, 2017

spalger commented Apr 11, 2017

kobelb commented Apr 11, 2017 •

edited

Loading

stacey-gammon commented Apr 17, 2017

stacey-gammon left a comment

stacey-gammon Apr 17, 2017

kobelb Apr 18, 2017

stacey-gammon Apr 17, 2017

kobelb Apr 18, 2017

kobelb commented Apr 18, 2017

Adding an app for redirects when storing state in session storage #10822

Adding an app for redirects when storing state in session storage #10822

Conversation

kobelb commented Mar 20, 2017 • edited Loading

kobelb commented Mar 21, 2017

spalger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kobelb commented Apr 11, 2017

spalger commented Apr 11, 2017

kobelb commented Apr 11, 2017 • edited Loading

stacey-gammon commented Apr 17, 2017

stacey-gammon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kobelb commented Apr 18, 2017

kobelb commented Mar 20, 2017 •

edited

Loading

spalger Apr 7, 2017 •

edited

Loading

kobelb commented Apr 11, 2017 •

edited

Loading