Implement loading user credentials from storage #9

dmitrizagidulin · 2017-06-08T16:36:13Z

Addresses issue #8.

Implements ClientAuthOIDC.from(options) factory method, which loads
previously selected provider uri and user credentials from storage.
(.for() wasn't used as it was a reserved word).

Usage:

let auth = ClientAuthOIDC.from({})
// credentials / tokens have now been loaded from local storage

auth.currentUser()
  .then(webId => {
    // previously saved web id
  })

dmitrizagidulin · 2017-06-08T16:45:26Z

cc @dan-f for review.

dan-f · 2017-06-13T14:47:49Z

test/auth.test.js

  describe('login()', () => {
+    it('should return the current webId if one is already present', () => {


I would have expected login() to log you in no matter what, especially if the provider uri does not match the provider for the current session.

dan-f · 2017-06-13T14:58:22Z

src/index.js

   */
  constructor (options = {}) {
    this.window = options.window || global.window
    this.store = options.store || global.localStorage

+    this.debug = options.debug || console.error.bind(console)


Perhaps console.info or just console.log would be more appropriate. I think console.error is more for actual runtime errors.

Sure, I'll change it to log.

dan-f · 2017-06-13T15:00:54Z

src/index.js

+   * @returns {ClientAuthOIDC}
+   */
+  static from (options) {
+    let store = options.store || global.localStorage


window.localStorage is what you want. global throws a ReferenceError in the browser.

global gets changed to window by Webpack.

Cool! I didn't know that. I still think it'd be best not to write code that piggy-backs on a specific webpack feature.

No prob, I'll remove global usage.

dan-f · 2017-06-13T15:04:42Z

src/index.js

+   *   (typically loaded from storage)
+   * @param [options.redirectUri] {string} This app's callback redirect uri,
+   *   defaults to the current window's uri.
+   * @param [options.debug] {Function}
   */
  constructor (options = {}) {
    this.window = options.window || global.window


I'm confused becauseglobal isn't defined in the browser, and calling global.window in the console throws a ReferencError. Did this work when you tested it in the browser?

dan-f · 2017-06-13T15:08:21Z

src/index.js

+    this.loadCurrentCredentials()
+    if (this.webId) {
+      return Promise.resolve(this.webId)
+    }


These same three lines exist right above...?

I was debating on what the best way to do this was. The idea is - it first checks this.webId, and only then tries to loadCurrentCredentials() (which potentially initializes this.webId) and retries checking it. I suppose it doesn't hurt to drop the first check, and just to always try and load the current credentials from storage. I'll change it.

dan-f · 2017-06-13T15:23:23Z

src/index.js

-    // Lastly, kick off a Select Provider popup window workflow
-    return this.providerFromUI()
+    // If not available, kick off a Select Provider popup window workflow
+    this.selectProviderUI()


Shouldn't the return value of this be handed back to the caller?

That was my initial thought too, when I thought there was a way that this function can actually return the provider uri. Currently, though, the selectProviderUI() function just opens the Select Provider popup window, and sets up the event listeners on it.
What would you like to see as a return value - the handle to the opened popup window?

dan-f · 2017-06-13T15:25:07Z

src/index.js

+    this.redirectUri = options.redirectUri
+    this.webId = options.webId
+    this.idToken = options.idToken
+    this.accessToken = options.accessToken
    this.method = REDIRECT  // only redirect is currently supported


Looks like this isn't used anymore.

dan-f · 2017-06-13T15:28:07Z

src/index.js

@@ -337,6 +493,7 @@ class ClientAuthOIDC {

  /**
   * Redirects the current window to the given uri.
+   *


The return false below will never get executed.

Yeah, it looks strange, doesn't it :( It has to do with some horrible browser quirk - on some browsers, the window.location.href redirect only works if you return false from that function. (See https://stackoverflow.com/a/6109823/525338 for an example)

dan-f · 2017-06-13T15:29:40Z

src/index.js

+      .then(webId => {
+        credentials.webId = webId
+
+        this.saveCurrentCredentials(credentials)


What about setCurrentCredentials?

dan-f · 2017-06-13T15:31:54Z

src/index.js

      .catch(error => {
        this.clearAuthResponseFromUrl()
        if (error.message === 'Cannot resolve signing key for ID Token.') {
-          console.log('ID Token found, but could not validate. Provider likely has changed their public keys. Please retry login.')
+          this.debug('ID Token found, but could not validate. Provider likely has changed their public keys. Please retry login.')


"retry login" is a noop currently if this.webId is stored.

dmitrizagidulin · 2017-06-13T15:39:31Z

@dan-f thank you, great feedback.

dmitrizagidulin force-pushed the load-current-user branch from e21bf49 to b973517 Compare June 8, 2017 16:38

Implement loading user credentials from storage

3f955a2

dmitrizagidulin force-pushed the load-current-user branch from b973517 to 3f955a2 Compare June 8, 2017 16:41

dan-f reviewed Jun 13, 2017

View reviewed changes

dmitrizagidulin closed this Jan 8, 2020

dmitrizagidulin deleted the load-current-user branch January 8, 2020 17:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement loading user credentials from storage #9

Implement loading user credentials from storage #9

dmitrizagidulin commented Jun 8, 2017

dmitrizagidulin commented Jun 8, 2017

dan-f Jun 13, 2017

dan-f Jun 13, 2017

dmitrizagidulin Jun 13, 2017

dan-f Jun 13, 2017

dmitrizagidulin Jun 13, 2017

dan-f Jun 13, 2017

dmitrizagidulin Jun 13, 2017

dan-f Jun 13, 2017

dan-f Jun 13, 2017

dmitrizagidulin Jun 13, 2017

dan-f Jun 13, 2017

dmitrizagidulin Jun 13, 2017

dan-f Jun 13, 2017

dan-f Jun 13, 2017

dmitrizagidulin Jun 13, 2017

dan-f Jun 13, 2017

dan-f Jun 13, 2017

dmitrizagidulin commented Jun 13, 2017

		describe('login()', () => {
		it('should return the current webId if one is already present', () => {

Implement loading user credentials from storage #9

Implement loading user credentials from storage #9

Conversation

dmitrizagidulin commented Jun 8, 2017

dmitrizagidulin commented Jun 8, 2017

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitrizagidulin commented Jun 13, 2017