AzureAD / microsoft-authentication-library-for-dotnet

Microsoft Authentication Library (MSAL) for .NET
https://aka.ms/msal-net
MIT License
1.39k stars 342 forks source link

[Bug] [S] UWP race conditions when the token is retrieved from the cache #2616

Closed qmatteoq closed 3 years ago

qmatteoq commented 3 years ago

Logs and Network traces App.log

Which Version of MSAL are you using ? Microsoft.Identity.Client 4.30.1

Platform UWP

What authentication flow has the issue?

Other? - please describe;

Is this a new or existing app?

The app is in production, and I have enabled WAM as authentication method

Repro

  1. Add authentication to a UWP app using WAM
  2. Add multiple background tasks to the application which silently authenticate using the following code:

    var accounts = await _client.GetAccountsAsync();
    _authenticationResult = await _client.AcquireTokenSilent(scopes, accounts.FirstOrDefault())
                                          .ExecuteAsync();
  3. Run the application and authenticate once interactively
  4. Schedule the execution of the background tasks (for example, by configuring them to run every 15 minutes).

Expected behavior Every background task is able to retrieve the cached token successfully.

Actual behavior WAM doesn't support race conditions. If more than one background task is executed at the same exact time, only one will succeed in retrieving the token. All the others will fail with the following error:

The process cannot access the file because it is being used by another process. The file is in use. Please close the file before continuing.

Possible Solution Add a retry logic, so that the WAM will try again to retrieve it in case the file in locked.

bgavrilMS commented 3 years ago

Not sure this issue is related to WAM.

The issue here seems to be that on UWP we use a DPAPI protected file, but no syncronization mechanism. We did not think about background threads making calls. We should use a strategy similar to https://github.com/AzureAD/microsoft-authentication-extensions-for-dotnet, which uses files as syncronization mechanism.

bgavrilMS commented 3 years ago

Hi @qmatteoq . As suggested by yourself by email, the token cache should not delete the underlying file if it cannot load it. The app would simply continue to use the token cache that it has in memory.

I think this is only a partial solution. The complete solution is to add a degree of synchronization - we should not allow background tasks from reading and writing to the token cache at the same time.

On UWP, app developers do have the option of customizing their token cache serialization, so we have a way forward without having to recompile MSAL etc. I have a branch where I coded up a token cache with synchronization - please have a look at this PR https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/pull/2618/files - note that it contains no changes to MSAL.

May I ask you to check if this solution works? I verified that it works with in-process background tasks, but wasn't able to get out-of-process background tasks to work. I am not sure if SemaphoreSlim can be used to synchronize between out-of-proc background tasks... Hopefully you can determine that.

Based on your results, I can also apply a fix to MSAL.

FrayxRulez commented 3 years ago

You might want to use a Mutex for out-of-process synchronization.

bgavrilMS commented 3 years ago

@FrayxRulez - mutex is thread-afine (just like lock), so it won't play nice with the async / await. I think the full named Semaphore (instead of SemaphoreSlim would work...

@qmatteoq tested this implementation and it seems to work fine with out of proc background tasks, so I guess they are all executed in the same process?