Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SDK-2039] Change cache lookup mechanism #652

Merged
merged 8 commits into from
Dec 7, 2020
Merged

Conversation

frederikprijck
Copy link
Member

Given a token is stored in the cache with scopes of read:books write:books and an audience of http://my-api,
I should be able to retrieve that token by only asking for one of those scopes + audience

  • Added a function to find any existing key in the cache based on the provided data (client id, audience, scopes)

@frederikprijck frederikprijck requested a review from a team as a code owner November 19, 2020 15:20
@frederikprijck frederikprijck added the review:small Small review label Nov 19, 2020
Copy link
Contributor

@stevehobbsdev stevehobbsdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to review this further, and I have left some comments as well, but I'm not sure about this approach.

We'll need to check this but in its current state (in general, not this PR), the scopes that are present in the key might not accurately reflect what is stored in the cache, and I think that's a bug. This scenario might occur if you ask for scopes that are not registered in the API client on Auth0. So you can ask for a set of scopes, but Auth0 may not return them. In which case, the cache key might include the scopes you asked for, but might not necessarily appear in the cached data. Does that make sense? That's the part we need to check. What's the behaviour of the SDK today?

In which case, I'm not sure the approach taken in this PR will be accurate. Instead of looking at the scopes that are present in the cache key, we might want to look at the scopes that are actually in each cached item.

__tests__/cache.test.ts Outdated Show resolved Hide resolved
src/cache.ts Outdated Show resolved Hide resolved
src/cache.ts Outdated Show resolved Hide resolved
src/cache.ts Outdated Show resolved Hide resolved
@frederikprijck
Copy link
Member Author

frederikprijck commented Nov 20, 2020

I need to review this further, and I have left some comments as well, but I'm not sure about this approach.

We'll need to check this but in its current state (in general, not this PR), the scopes that are present in the key might not accurately reflect what is stored in the cache, and I think that's a bug. This scenario might occur if you ask for scopes that are not registered in the API client on Auth0. So you can ask for a set of scopes, but Auth0 may not return them. In which case, the cache key might include the scopes you asked for, but might not necessarily appear in the cached data. Does that make sense? That's the part we need to check. What's the behaviour of the SDK today?

In which case, I'm not sure the approach taken in this PR will be accurate. Instead of looking at the scopes that are present in the cache key, we might want to look at the scopes that are actually in each cached item.

So let us take the following scenario:

  • I ask for a token for Scope:A, Scope:B and Scope:C for AudienceX
  • The given API only has Scope A and Scope B registered, so C is not returned.
  • This will result in a cache value with the keys containing Scope:A, Scope:B and Scope:C (which represents the call we made) and the data containing Scope:A and Scope:B (which represents the response we got).

Now imagine some time later we do the exact same call again:

  • I ask for another token for Scope:A, Scope:B and Scope:C for AudienceX
  • The cache tells me I already did this call earlier and I have a token for this combination of scopes and audience. Even though the data does not contain all the Scopes I am asking for (the data does not have Scope C), the data is the response for the request using the same scopes that I am asking for now.

Imagine in the above scenario, we decide to use the data to lookup the scopes and not the keys, in this case we will never use the cache because the requested scopes differ from the data. This results in unexpected extra calls.

I also think that using the data to do the lookup is a change in behavior as to the current scenario. Because currently it is also using the key to find a match. The only difference is that this PR now also ensures that it doesn't have to be a strict match, but it will also use the cache if we ask for a token only for Scope A.

With the above, I would argue that we should not lookup based on the data but lookup based on the key.

@stevehobbsdev stevehobbsdev self-requested a review December 7, 2020 10:48
@frederikprijck frederikprijck added the CH: Changed PR is changing something label Dec 7, 2020
@frederikprijck frederikprijck added this to the vNext milestone Dec 7, 2020
@frederikprijck frederikprijck merged commit e770d89 into master Dec 7, 2020
@frederikprijck frederikprijck deleted the frederik/sdk-2039 branch December 7, 2020 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CH: Changed PR is changing something review:small Small review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants