Don't reference window in default options if it doesn't exist #73

kball · 2017-10-26T18:29:45Z

I'm setting up a nuxt.js app with vue-authenticate, which involves having server-side rendering as well as client-side. When we try to do SSR, it breaks when loading the default options due to window not existing.

I'm fine with overriding these options, but with current implementation it breaks on import, before I even have the chance to override. This changeset fixes that by checking for window before referencing it, and just using empty if it does not.

This is the minimal I need to keep moving forward - so far actually doing all the auth on the client side, I just need it to not break when loading on the server. As I keep going may submit some more PRs to get things working better on the server as well.

dgrubelic · 2017-10-26T19:26:21Z

Is this working, have you tested this? What about this window reference? https://github.com/dgrubelic/vue-authenticate/blob/master/src/oauth/popup.js#L21

kball · 2017-10-26T19:55:56Z

I tested it locally, though only for relatively limited usecases so far... the thing is, for any code that is running interactively in response to user action (e.g. open popup) it will be run on client no matter what and it's fine. The problem is more for things loaded at javascript parse time (e.g. options are loaded on import)

dgrubelic · 2017-10-28T08:36:29Z

src/options.js

@@ -13,7 +13,7 @@ export default {
  storageType: 'localStorage',
  storageNamespace: 'vue-authenticate',
  cookieStorage: {
-    domain: window.location.hostname,
+    domain: typeof(window) === 'undefined' ? '' : window.location.hostname,


Please use isUndefined or isDefined helper inside utils.js and extract this into a helper function like getCookieDomainUrl().

dgrubelic · 2017-10-28T08:38:58Z

src/options.js

@@ -55,7 +55,7 @@ export default {
      name: 'facebook',
      url: '/auth/facebook',
      authorizationEndpoint: 'https://www.facebook.com/v2.5/dialog/oauth',
-      redirectUri: window.location.origin + '/',
+      redirectUri: typeof(window) === 'undefined' ? '/' : window.location.origin + '/',


A lot of repeated work here. Again, use isUndefined or isDefined helper and extract this into a helper function like getRedirectUri(uri) where you check if window is defined. If false, return uri or null. If true, return window.location.origin + append uri.

kball · 2017-10-30T20:43:33Z

Updated!

dgrubelic · 2017-10-30T21:21:36Z

src/options.js

@@ -55,7 +60,7 @@ export default {
      name: 'facebook',
      url: '/auth/facebook',
      authorizationEndpoint: 'https://www.facebook.com/v2.5/dialog/oauth',
-      redirectUri: typeof(window) === 'undefined' ? '/' : window.location.origin + '/',
+      redirectUri: getCookieDomainUrl('/'),


This is not good. You are using window.location.hostname for redirectUri. That information is only "{domain_name}.{TLD}" => "github.com". This means that if you have site on https, if you setup only hostname as your redirectUri, you will be redirected to http version of the site.

ack good point, this time around missed the distinction that we were using hostname in just the first domain, origin in the rest. Fixing.

dgrubelic · 2017-11-02T15:21:48Z

src/options.js

+  return isUndefined(window) ? '' : `${window.location.hostname}`;
+}
+
+function getCookieDomainUrl(path = '') {


Contextually, you are not returning CookieDomainUrl but redirectUri, so getRedirectUri would be a much better name for this function.

…ookie-storage

kball · 2017-11-02T21:46:01Z

I did some additional testing this afternoon after updating where my build was referencing, and found that for some reason using isUndefined actually still breaks with the reference error, while if I do a direct typeof comparison it works... I do not understand why, but I'm going to update going back to using typeof.

kball · 2017-11-02T21:57:05Z

Also starting to get deeper in and wanting to actually do more auth on the server side & not just make sure we don't break and then do auth on client... I think the primary blocker for this using cookie store is not having server-side access to cookies (it checks for document in _getCookie and _setCookie). It looks like you're trying to keep dependencies minimal, but what do you think about pulling in the cookie npm package to provide uniform server & client access to cookies? (https://github.com/jshttp/cookie)

It's a single file with no deps, so if you like we could even copy it in...

dgrubelic · 2017-11-03T06:27:26Z

No, this will not solve SSR problem because this library you suggested is only used for parsing and serialization in cookie format, but it doesn't actually sets the cookie into the document which will make it persistant. What is actually the problem is document.cookie = cookie here: https://github.com/dgrubelic/vue-authenticate/blob/master/src/storage/cookie-storage.js#L45 in method _setCookie() (_getCookie() already has check if document is undefined).

All you need to do is to add isUndefined check to the _setCookie() method.

kball · 2017-11-03T20:37:55Z

I don't think that's all.. the issue is we want to be able to read & write the cookies on the server the same as on the client... (or at least read them; auth change may happen only on client, but server needs to be able to read...)

_getCookie needs to understand how to read cookies on server side. Right now it always returns unauthed

kball · 2017-11-10T18:45:23Z

Ok, finally had time to really dig my teeth into the question of auth on server side, and realized where I'd had the gap... vue-authenticate doesn't have the context to know where to get the cookies from on the server, as it will vary by implementation.

To remedy this, I updated the cookieOptions to allow passing in your own function to get cookies, so the server can pass a function that is bound with the appropriate context (e.g the req object for an express implementation). Have this now working in my dev environment, reading cookies and recognizing authentication correctly on server side.

I don't think we need to do the same thing for writing cookies, as that interaction is likely always on the client, but we could use this same approach if you think it's relevant.

dgrubelic · 2017-11-10T20:59:20Z

src/options.js

@@ -1,3 +1,6 @@
+import { getCookieDomain, getRedirectUri } from './utils.js';


These two functions are contextually highly connected only to options.jsfile and nowhere else. There is no reason they should be located in utils.js file. Please move them back in options.js file.

dgrubelic · 2017-11-10T21:12:50Z

src/storage/cookie-storage.js

      expires: null,
      path: '/',
-      secure: false
+      secure: false,
+      getCookieFn: this._getCookie,


I didn't have time to think about this logic these past few days, but my first impression is that I don't like it too much. The main reason is that the ability to authenticate a user on the server side is by using cookies. I will have to think a bit more about this.

For now, I suggest that you make it compilable in SSR, and later we can try to think about a solution to persist authentication state over SSR.

TitanFighter · 2017-12-10T22:49:09Z

As a temporary solution, possibly can be helpful this component which wraps non SSR friendly components (excludes from SSR and renders only on client-side)

Don't reference window in default options if it doesn't exist

4709e57

dgrubelic requested changes Oct 28, 2017

View reviewed changes

dgrubelic requested changes Oct 30, 2017

View reviewed changes

Refactor cookie domain url

77976c8

kball force-pushed the default-options-shouldnt-break-in-ssr branch from 45a9f6d to 77976c8 Compare October 30, 2017 21:44

dgrubelic requested changes Nov 2, 2017

View reviewed changes

kball added 2 commits November 2, 2017 09:16

Update to more appropriate name for redirectUri function

ba3bf2c

Refactor cookie & redirect utils into utils and use for src/storage/c…

3435bf9

…ookie-storage

Update to use typeof directly

8d398d4

Add ability to pass in a function to get cookies

3794148

dgrubelic requested changes Nov 10, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't reference window in default options if it doesn't exist #73

Don't reference window in default options if it doesn't exist #73

kball commented Oct 26, 2017

dgrubelic commented Oct 26, 2017

kball commented Oct 26, 2017

dgrubelic Oct 28, 2017

dgrubelic Oct 28, 2017

kball commented Oct 30, 2017

dgrubelic Oct 30, 2017

kball Oct 30, 2017

dgrubelic Nov 2, 2017

kball commented Nov 2, 2017

kball commented Nov 2, 2017

dgrubelic commented Nov 3, 2017

kball commented Nov 3, 2017

kball commented Nov 10, 2017

dgrubelic Nov 10, 2017

dgrubelic Nov 10, 2017

TitanFighter commented Dec 10, 2017

		@@ -1,3 +1,6 @@
		import { getCookieDomain, getRedirectUri } from './utils.js';

Don't reference window in default options if it doesn't exist #73

Are you sure you want to change the base?

Don't reference window in default options if it doesn't exist #73

Conversation

kball commented Oct 26, 2017

dgrubelic commented Oct 26, 2017

kball commented Oct 26, 2017

dgrubelic Oct 28, 2017

Choose a reason for hiding this comment

dgrubelic Oct 28, 2017

Choose a reason for hiding this comment

kball commented Oct 30, 2017

dgrubelic Oct 30, 2017

Choose a reason for hiding this comment

kball Oct 30, 2017

Choose a reason for hiding this comment

dgrubelic Nov 2, 2017

Choose a reason for hiding this comment

kball commented Nov 2, 2017

kball commented Nov 2, 2017

dgrubelic commented Nov 3, 2017

kball commented Nov 3, 2017

kball commented Nov 10, 2017

dgrubelic Nov 10, 2017

Choose a reason for hiding this comment

dgrubelic Nov 10, 2017

Choose a reason for hiding this comment

TitanFighter commented Dec 10, 2017