intuit / oauth-jsclient

Intuit's NodeJS OAuth client provides a set of methods to make it easier to work with OAuth2.0 and Open ID
https://developer.intuit.com/
Apache License 2.0
124 stars 159 forks source link

isAccessTokenValid() gives false positive result #75

Closed AlbertHambardzumyan closed 4 years ago

AlbertHambardzumyan commented 4 years ago

The isAccessTokenValid() function always returns true if token has expires_in greater then 60.

Tried to dig deeper and found that internal _checkExpiry() function makes use of createdAt property. The internal setToken() function sets now if its missing in provided token this.createdAt = tokenData.createdAt || Date.now()

Note, createToken() does not provide createdAt in the response, so SDK user cannot provide that in token.

In other words, if createdAtis missing in provided token, then system sets Date.now() and uses that in expiry computation (plus 60 sec latency)., thus gives false positive result.

abisalehalliprasan commented 4 years ago

I will take a look at it and provide more details.

abisalehalliprasan commented 4 years ago

createToken() does provide createdAt in the response :

 {
   "token_type": "bearer",
   "access_token": "eyJlbmMiOiJxxxxxxxxxxxxxxx628fsF.hcuBUVyq6aTGZKvxZ27sVw",
   "expires_in": 3600,
   "refresh_token": "AB1158973896xxxxxxxxxxxxxxxxxxxxxxxbwv1uUVgevE6sUo",
   "x_refresh_token_expires_in": 8726400,
   "realmId": "4620xxxxxxxxxxx867780",
   "id_token": "",
   "createdAt": 1581012560611
}

Yes, if you are setting a previosuly created token using the SDK, you would need to pass that. But if all your tokens are handled by the SDK, you need not worry. The SDK will use it.

In other words, if you are setting a token that is created using this SDK then you are good. If not, the createdAt would give rise to a false-positive results. This is mentioned in the Note section here

AlbertHambardzumyan commented 4 years ago

@abisalehalliprasan Unfortunately, I'm not getting createdAt in the response.

      const authResponse = await oauthClient.createToken(request.url)
      console.log('Token', authResponse.getJson())

Output

{
  x_refresh_token_expires_in: 8726400,
  access_token: 'eyJlbmMiOiJBMTI4xxxxxxxxMjU2IiwiYWxnIjoiZGlyIn0..N0kptu',
  token_type: 'bearer',
  refresh_token: 'AB11589739716xxxxxxxxx51OBUAHRhTywDsSsQpwscqu',
  expires_in: 3600
}
AlbertHambardzumyan commented 4 years ago

@abisalehalliprasan The rest will be okey if I get the createdAt.

Im storing entire Token object in the storage on login. Later, getting this token and checking with isAccessTokenValid() before trying to get some resource.

If isAccessTokenValid() gives correct result, then I can make a decision about refreshing it or using as it is if its not expired.

abisalehalliprasan commented 4 years ago

Please refer to retrieve the tokens under documentation here

Just replace console.log('The Token is '+ JSON.stringify(authResponse.getJson())); with console.log('The Token is '+ JSON.stringify(oauthClient.token.getToken()));

AlbertHambardzumyan commented 4 years ago

@abisalehalliprasan Works with oauthClient.token.getToken(). However that looks weird that response of createToken() does not produce the same output. In both cases the output is Token, but some diff exist.

Note, getting createdAt via oauthClient. createToken() brings additional problem:

I have to create a new oauthClient each time I want to create a token On the other hand, the oauthClient might be reused if we could use createToken() response instead of oauthClient.token.getToken()

abisalehalliprasan commented 4 years ago

As long as you are using the same app credentials, you can use the oauthClient as a global variable.

In order to switch between multiple users in parallel you could : set the token

AlbertHambardzumyan commented 4 years ago

@abisalehalliprasan Im using the same app credentials. However, I might have wrong result with global oauthClient

Consider the following case where I'm doing multipole createToken() a) User 1 - createToken() - output 1 b) User 2 - createToken() - output 2 c) User 3 - createToken() - output 3 d) User 4 - createToken() - output 4

There is a chance that the output of the first call might be overridden by the any of b), c) or d) before I read it via oauthClient.token.getToken()

abisalehalliprasan commented 4 years ago

@AlbertHambardzumyan Welcome any suggestions or PR if it helps solve this issue.

AlbertHambardzumyan commented 4 years ago

@abisalehalliprasan I see a solution in the response of const authResponse = await oauthClient.createToken(request.url)

If authResponse comes directly from api response and not from oauthClient.setToken() + oauthClient.getToken(), then this might be resolved by simple providing all the fields of Token object in authResponse.

In other words, the output of oauthClient.token.getToken() should be identical with the output of await oauthClient.createToken(request.url)

Any idea why there is a diff between this two ?

AlbertHambardzumyan commented 4 years ago

@abisalehalliprasan Looks like the api response is returned in createToken()

    const authResponse = res.json ? res : null;
    const json = (authResponse && authResponse.getJson()) || res;
    this.token.setToken(json);
    this.log('info', 'Create Token response is : ', JSON.stringify(authResponse, null, 2));
    return authResponse

This means we can have global oauthClient and use the response of await oauthClient.createToken(request.url) instead of oauthClient.token.getToken(), if we get createdAt in the first one.

AlbertHambardzumyan commented 4 years ago

@abisalehalliprasan The tokenData passed to setToken() does not have createdAt. You can see by logging tokenData.

This means oauthClient.token.getToken() gives createdAt which is not coming from server, but set in SDK in setToken().

Thats a reason why await oauthClient.createToken(request.url) is not equal to oauthClient.token.getToken()

NOTE, server sends createdAt property in res.token instead of res.json which used to return. See https://github.com/intuit/oauth-jsclient/blob/master/src/OAuthClient.js#L159

abisalehalliprasan commented 4 years ago

@AlbertHambardzumyan : Do you think that would solve the Global variable issue addressed in the thread here ?

I might need to test it before releasing a new version of the library.

AlbertHambardzumyan commented 4 years ago

@abisalehalliprasan yes.

const token = await oauthClient.createToken(request.url)

This resolved global variable issue. Needs only to have the all the fields of res.token instead of res.json.

abisalehalliprasan commented 4 years ago

Please send a PR if you have the changes handy.

michaelyohanes commented 3 years ago

Feb 2021. same here, isTokenValid() always gave false positive result.

Steps to reproduce:

After closer look, seems that isTokenValid() basically -only- check the createdAt value, without validating the tokens itself.

@abisalehalliprasan : Any plan in the future to have it improved ?