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

[Bug] NotImplementedException when loading a user token cache in the app token cache #3218

Closed
1 of 7 tasks
bgavrilMS opened this issue Mar 10, 2022 · 1 comment · Fixed by #3925
Closed
1 of 7 tasks

Comments

@bgavrilMS
Copy link
Member

bgavrilMS commented Mar 10, 2022

Logs and network traces
Without logs or traces, it is unlikely that the team can investigate your issue. Capturing logs and network traces is described in Logging wiki.

Which version of MSAL.NET are you using?
Msal 4.42

Platform
confidential client

What authentication flow has the issue?

  • Desktop / Mobile
    • Interactive
    • Integrated Windows Authentication
    • Username Password
    • Device code flow (browserless)
  • Web app
    • Authorization code
    • On-Behalf-Of
  • Daemon app
    • Service to Service calls

Repro

  1. Configure an app to use both a user flow (like authorization code, but you can repro with ROPC too) and S2S and point the UserTokenCache and the AppTokenCache to the same location (e.g. file for simplicity)
  2. Make sure you exercise the user flow first
  3. Call S2S

Expected behavior
S2S should just work

Actual behavior
A NotImplementedException is thrown

Root cause: the app token cache does not expect RTs to be given to it.
Note: this does not happen if you use Microsoft.Identity.Web.TokenCache

Possible solution
Throw a better exception that NotImplementedException, asking to use Microsoft.Identity.Web.TokenCache or a partitioned cache.

@pmaytak
Copy link
Contributor

pmaytak commented Jan 31, 2023

Thoughts previous discussion:

We can’t just replace the exceptions with no-op; we’d have to do minor refactoring. Since during the subsequent S2S call we will not load user RTs, IdTs, Accounts into MSAL when deserializing; and then when serializing during the same call, the cache file will be overwritten with only ATs from the internal cache. The refactoring should be minor – use one type of cache accessor instead of two types (app accessor and user accessor) as currently

Option 1:
No refactoring, just throw a more descriptive error.

  • Encourages the case of doing caching right.
  • Setting up the caching the right way, doesn’t seem like much extra work.
  • If it’s an obscure scenario, may only affect small number of users.

Option 2:
Allow flat external cache to work/be loaded.

  • Keeping user and app tokens in one internal cache structure is a bit better with current codebase than it was before. Previously the token dictionary was keyed by token key, so during filtering through tokens we would search through both, user and app tokens. Now since that dictionary is first partitioned by a partition key, the filtering is done only either through app or user tokens for that partition. So less chance to make an error and return a wrong token.
  • The race condition in multi-threaded scenarios may be a bigger issue with this. In normal scenarios, we only ever load one partition into internal cache, that’s why multiple threads can get just a cache miss. But in this case internal cache would have all the cached data.
  • This does allow for a simpler caching set up on developer’s end, with usage simplicity being one of MSAL’s goals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
2 participants