search

panva / openid-client

OAuth 2 / OpenID Connect Client API for JavaScript Runtimes
MIT License
1.83k stars 392 forks source link

fix: cache not used as object different each time #162

Closed sami-sweng closed 5 years ago

sami-sweng commented 5 years ago

The library used for the cache supports using objects as keys, but checks for pointer equality and not deep or JSON equality (lru-cache).

Since the key that we want to cache originate from a JSON.parse call (client.js#L642, client.js#L557), it is different each time.

Thus, the cache only works for 60 sec for a particular key (thanks to the throttle key).

codecov[bot] commented 5 years ago

Codecov Report

Merging #162 into master will not change coverage. The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #162   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          18     18           
  Lines         798    802    +4     
=====================================
+ Hits          798    802    +4
panva commented 5 years ago

Hi @sami-sweng, thank you for taking the time to make openid-client better. I have a different fix for this in the 3.x branch that more reflects the intended logic (not just kid cache), if you want to backport that, please do so. Otherwise this I will not accept.

sami-sweng commented 5 years ago

Should I create a new PR?

panva commented 5 years ago

Just update this one and point it to master please.

sami-sweng commented 5 years ago

Ah, I get it,

you want me to check what you did in v.3 branch and apply it in v2?

Alright, I'll try something if it's not too time consuming.

Thank you for your work on this library.

sami-sweng commented 5 years ago

Alright,

here is a light version of your v3 fix based on object hash.

panva commented 5 years ago

can you include this part too?

const defHash = objectHash(def, {
  algorithm: 'sha256',
  ignoreUnknown: true,
  unorderedArrays: true,
  unorderedSets: true,
});
panva commented 5 years ago

Okay, there are more things missed, i really intend to cherry-pick the properties that matter for selecting a key, kid, kty, alg, use, key_ops. Would you mind not making this "light" but rather do a full backport of this particular fix?

sami-sweng commented 5 years ago

From blame it seems to be part of a big commit (https://github.com/panva/node-openid-client/commit/7bb5847664bdcfc6ce96bcf2aee420a93476bcb7) so I'm not sure about the scope of this backport.

But I can hash based on these properties...?

sami-sweng commented 5 years ago

So are there more things that I should add in this PR?

@panva as a side question: I'm making this PR because I randomly have a few failed login against a Microsoft implementation. It seems that their keys route (jwks_uri) is randomly failing with ETIMEDOUT. Does this sound familiar to you?

panva commented 5 years ago

So are there more things that I should add in this PR?

Yes, it's incomplete, but don't worry, I'll redo this as part of my efforts to backport some of the v3.x fixes back to v2.x

It seems that their keys route (jwks_uri) is randomly failing with ETIMEDOUT. Does this sound familiar to you?

Nope.

panva commented 5 years ago

I'll do the backport with the last release on the v2.x line right before releasing v3.x