podio / podio-js

Official Podio JavaScript SDK for node and the browser
http://podio.github.io/podio-js/
MIT License
45 stars 49 forks source link

Potential issue with server-auth sessionStore sample? #44

Closed myktra closed 8 years ago

myktra commented 8 years ago

Not entirely sure if this is legit, but I suspect there is a bug in the server_auth sessionStore sample. If you have an older access token in your store that requires a refresh and you make a call such as:

const appId = 1234; // your appId
Podio
  .isAuthenticated()
  .then(() => {
    Podio
      .request('GET', `/item/app/${appId}/count`)
      .then(response => {
        // do something with the response
      })
      .catch(error => console.error(error));
  })
  .catch(error => console.error(error));

The .isAuthenticated() call will automatically refresh your existing access token if it has expired so the subsequent .request() succeeds. Great. However, the sample sessionStore.set() function, which issues a callback on line 32...

if (typeof callback === 'function') {
  callback();
}

...does not pass any parameters to the callback. The caller of the sessionStore.set() function – in this case the _refreshToken method called from _onResponse – tests for the presence of an error parameter from the callback against the value null. But this value is undefined since there were no parameters passed to the callback inside the sample sessionStore.set(). The promise winds up getting rejected improperly and though the .request() ultimately may run, a PodioError is thrown.

.
.
.
this._getAuth()._refreshToken(function (err, response) {
  if (err !== null) {
    // undefined !== null, so, it rejects unnecessarily (?) and
   // throws a PodioError
    options.reject(this._handleTransportError(options, response));
  }
  .
  .
  .
  // this appears to run and make the request behind the scenes, 
  // but the caller of Podio.request() has been rejected.
  this._onTokenRefreshed(options.requestParams).then(function (response) {
    options.resolve(response);
  }, function (err) {
    options.reject(err);
  })
  .
  .
  .

I believe a correction for the sample sessionStore for server-auth ought to be:

if (typeof callback === 'function') {
  callback(null);
}

Does this make sense? It worked for me.

dmatteo commented 8 years ago

Hi @myktra, I think you're right on this.

Would you be able to write a PR to address the change?

myktra commented 8 years ago

I'll give it a go!