Always delegate JWT to storage backend #89

wadetandy · 2018-03-13T21:54:20Z

Previously we had an incomplete implementation of the
CredentialStorageBackend concept, as we were initializing from and
storing to it, but we also memoized it into the model class itself,
which meant that particularly when using localStorage, an update from
outside of JSORM wouldn't track into already initialized JSORM models.
Now it's possible to pass an InMemoryStorageBackend instead of the
default localStorage, which will isolate a particular model and prevent
it from persisting.

wadetandy · 2018-03-13T21:54:31Z

@richmolj

wadetandy · 2018-03-13T22:31:05Z

src/model.ts

  }

  static set credentialStorageBackend(backend: StorageBackend | undefined) {
+    if (this.baseClass === JSORMBase) {


I am only choosing to do this because it was the way to preserve as much of the previous behavior as possible while allowing us to fix this functionality. If we are fine with users being able to set a base credential that must be manually overridden at lower levels (instead of setting one for each base class by default), then I'm fine with that as well, it will just result in a couple more breaking changes.

I'm fine with it. I see manually overriding at lower-levels as a non-starter. I would like to somehow share this between here and 186-188

richmolj

A few comments but looks good overall. Suggest applying this to Nimitz first to verify everything works as expected.

richmolj · 2018-03-14T14:49:19Z

src/credential-storage.ts

-      return null
-    }
+  getJWT(): string | undefined {
+    return this._backend.getItem(this._jwtKey) || undefined


getItem specifically works to return null instead of undefined, but then we just convert it back to undefined?

richmolj · 2018-03-14T14:52:20Z

src/model.ts

      ModelClass[k] = config[k]
    }
  }

+  // JWT depends on some other settings, so wait for everything
+  // else before initializing it


Could you explain this a bit more? This and line 121 jumped out to me. I would think we'd just call the setter as normal, no?

Per my comment above, a lot of weirdness here is around ensuring backwards compatibility, which we can always choose to not do. Essentially if we don't do this, the order of declaration of jwtStorage and jwt will change where the provided jwt gets persisted to. My ideal world would have us get rid of the jwt API entirely and just use setJWT and getJWT

My ideal world would have us get rid of the jwt API entirely and just use setJWT and getJWT

I'm fine with this if we have to. But I'm not 100% clear on why calling setJWT underneath the hood of jwt= wouldn't work.

richmolj · 2018-03-14T14:53:44Z

src/model.ts

  }

  static set credentialStorageBackend(backend: StorageBackend | undefined) {
+    if (this.baseClass === JSORMBase) {


I'm fine with it. I see manually overriding at lower-levels as a non-starter. I would like to somehow share this between here and 186-188

richmolj · 2018-03-14T14:54:49Z

src/model.ts

+      throw new Error(`Cannot access credentials on JSORMBase`)
+    }
+
+    if (!this._credentialStorageBackend) {


I think we could call the non _ version and get lines 186-188 for free?

richmolj · 2018-03-14T14:57:57Z

src/model.ts

+  static set jwt(token: string | undefined) { 
+    // If setting the JWT on a model class directly, we want it to have its own
+    // credential storage
+    this._initializeCredentialStorage()


Want to double-check this isn't having an unexpected side-effect. If Post and Comment subclass ApplicationRecord, and Comment.first() fires and gets an updated JWT in the headers of the response, are we going to hit this method and inadvertently store the updated JWT on Comment instead of ApplicationRecord?

No, as the internal parts of the library should all be using setJWT which won't have this behavior.

richmolj · 2018-03-14T15:04:15Z

src/model.ts

+    }
+
+    if (!this._credentialStorageBackend) {
+      if (this.jwtStorage && typeof localStorage !== "undefined") {


Just noting that this.jwtStorage could be false and we'd still create an in-memory backend. I don't care too much but heads up.

yeah, that's the intended functionality. Essentially In-memory store is the same as not storing it.

richmolj · 2018-03-14T15:04:56Z

test/integration/authorization.test.ts

@@ -235,20 +230,35 @@ describe("authorization headers", () => {

            await Author.find(1)
          })
+
+          describe("when JWT is update from outside the model classes", () => {


richmolj · 2018-03-14T15:06:30Z

test/unit/model.test.ts

@@ -438,7 +437,8 @@ describe("Model", () => {
      describe("class options", () => {
        const config = {
          apiNamespace: "api/v1",
-          jwt: "abc123"
+          jwt: "abc123",
+          jwtStorage: 'foobarJWT',


trailing comma

wadetandy · 2018-03-19T20:13:22Z

cc @AndrewO

Previously we had an incomplete implementation of the CredentialStorageBackend concept, as we were initializing from and storing to it, but we also memoized it into the model class itself, which meant that particularly when using localStorage, an update from outside of JSORM wouldn't track into already initialized JSORM models. Now it's possible to pass an InMemoryStorageBackend instead of the default localStorage, which will isolate a particular model and prevent it from persisting.

richmolj

one comment that's up to you, otherwise LGTM 👍 !

richmolj · 2018-03-20T15:33:04Z

src/model.ts

@@ -118,11 +118,18 @@ export const applyModelConfig = <T extends typeof JSORMBase>(
  let k: keyof ModelConfigurationOptions

  for (k in config) {
-    if (config.hasOwnProperty(k)) {
+    if (config.hasOwnProperty(k) && k !== 'jwt') {


I think I'm fine with this with a comment. Alternatively, I wonder if it might make more sense to manually do the JWT setup on line 119, then end the function with iterating props and assigning them. This way the logic isn't broken up between line 121 and 130, might read a bit more sensibly.

do the JWT setup on line 119

Do you mean dealing specifically with all of the jwt-related ones and then finishing up with everything else in the config?

richmolj · 2018-03-20T15:33:45Z

src/model.ts

-        this._credentialStorageBackend
-      )
+  static set jwtStorage(val : string | false) {
+    if (val !== this._jwtStorage) {


What's the downside if this check were removed?

using an in-memory store and running ApplicationRecord.jwtStorage = ApplicationRecord.jwtStorage would wipe out the record. Otherwise nothing I can think of.

wadetandy · 2018-03-20T18:23:21Z

@richmolj fix pushed

wadetandy commented Mar 13, 2018

View reviewed changes

richmolj approved these changes Mar 14, 2018

View reviewed changes

wadetandy force-pushed the master branch from 0f815b8 to 37744f0 Compare March 20, 2018 08:11

richmolj approved these changes Mar 20, 2018

View reviewed changes

Clean up model extend options logic

41b62dd

wadetandy merged commit 0e38b6d into jsonapi-suite:master Mar 21, 2018

Always delegate JWT to storage backend #89

Always delegate JWT to storage backend #89

Uh oh!

Conversation

wadetandy commented Mar 13, 2018

Uh oh!

wadetandy commented Mar 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richmolj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wadetandy commented Mar 19, 2018

Uh oh!

richmolj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wadetandy commented Mar 20, 2018

Uh oh!

Uh oh!