AzureAD / microsoft-authentication-library-for-js

Microsoft Authentication Library (MSAL) for JS
http://aka.ms/aadv2
MIT License
3.67k stars 2.65k forks source link

fix: handle case where account entities is empty in after cache access #7329

Open LuccaRebelloToledo opened 1 month ago

LuccaRebelloToledo commented 1 month ago

Based on the Issue 7324, after implementing the DistributedCachePlugin class, when using the removeAccount method, the credentials were not cleared from my client cache. So, I started investigating and uploaded new personal versions of the library with logs during the entire account removal process and noticed that at the final moment of passing where the cache object was cleared, when trying to recover the KVStore cache, it was empty, that is, it was impossible to identify the partition key, preventing the clearance of the credentials in the client cache. However, I performed a validation where, if the accountEntities were empty, it would search for the partition key from my partition manager, correcting the problem of not clearing the credentials.

Previously, when using the removeAccount method, the accountEntities array was empty during the afterCacheAccess phase, preventing extraction of the partitionKey.

Now, if no accountEntities are available, the partitionKey is fetched from the partitionManager to ensure that the cache can still be updated.

Before the fix: beforeRemoveAccount

After the fix: afterRemoveAccount

Test results: {46EB06E5-254B-47B1-AA48-4EFE055FD220}

LuccaRebelloToledo commented 1 month ago

@microsoft-github-policy-service agree

Robbie-Microsoft commented 3 weeks ago

@LuccaRebelloToledo This fix will require a new unit test in DistributedCachePlugin.spec.ts.

LuccaRebelloToledo commented 3 weeks ago

@LuccaRebelloToledo This fix will require a new unit test in DistributedCachePlugin.spec.ts.

@Robbie-Microsoft At this point I'm in doubt about how to run the unit test because I found that in TokenCache.spec.ts the data is not fully mocked and actually uses the TokenCache functions. Should I change to use it like in TokenCache.spec.ts or should I continue mocking the functions and results?

bgavrilMS commented 2 weeks ago

Thanks so much for highlighting this issue and proposing a fix @LuccaRebelloToledo

Robbie-Microsoft commented 3 days ago

@LuccaRebelloToledo, you've done a fantastic job with that unit test + improvements to the testing constants. Thank you! We can get this merged in to our dev branch after this passes the CI pipeline, which I've enabled for this PR.

Robbie-Microsoft commented 3 days ago

You will need to run the "beachball" command in the root of the repo. See more info in our contributing doc.

LuccaRebelloToledo commented 3 days ago

@LuccaRebelloToledo, you've done a fantastic job with that unit test + improvements to the testing constants. Thank you! We can get this merged in to our dev branch after this passes the CI pipeline, which I've enabled for this PR.

Thank you so much Robbie, this means a lot to me!!

I'll remove the empty line and run the "beachball" process right now. Thanks again!