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

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

Conversation

LuccaRebelloToledo
Copy link
Contributor

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.

  • Refactored the afterCacheAccess method to check if accountEntities is empty.
  • If accountEntities is empty, fetch partitionKey from partitionManager.

Before the fix:
beforeRemoveAccount

After the fix:
afterRemoveAccount

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

@LuccaRebelloToledo
Copy link
Contributor Author

@microsoft-github-policy-service agree

@Robbie-Microsoft
Copy link
Collaborator

Robbie-Microsoft commented Oct 24, 2024

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

@LuccaRebelloToledo
Copy link
Contributor Author

LuccaRebelloToledo commented Oct 25, 2024

@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?

…ing cache state consistency post-serialization
Copy link
Contributor Author

@LuccaRebelloToledo LuccaRebelloToledo left a comment

Choose a reason for hiding this comment

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

Result of all tests:
image

Result of tests in DistributedCachePlugin.spec.ts
image

@bgavrilMS
Copy link
Member

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

@Robbie-Microsoft
Copy link
Collaborator

Robbie-Microsoft commented Nov 12, 2024

@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
Copy link
Collaborator

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

@LuccaRebelloToledo
Copy link
Contributor Author

@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!

@Robbie-Microsoft Robbie-Microsoft requested review from a team and bgavrilMS November 13, 2024 06:32
@LuccaRebelloToledo
Copy link
Contributor Author

Merged in #7426

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
msal-node Related to msal-node package
Projects
None yet
3 participants