Closed bkaankose closed 3 months ago
bgavrilMS
commented
10 months ago Just to set expectations, there are quite a few WAM bugs reported recently on win32 apps, and those are a priority for the team over UWP. Please consider moving from UWP to WinUI...
nbevans
commented
7 months ago Pretty sure we're also seeing this. It appears that its sometimes unable to do a silent refresh of the token via WAM.
This line of code is getting hit. But this doesn't seem right to me? Shouldn't ListWindowsWorkAndSchoolAccounts = true if using AAD with organizations tenant?
_logger.Info("WAM::FindAllAccountsAsync returning no accounts due to configuration option");
nbevans
commented
7 months ago I think our problem may actually be slightly different (but still relevant).
As per basically all sample code for MSAL - we call GetAccountsAsync() prior to AcquireTokenSilent(). The purpose of the GetAccountsAsync() call is to get the "first" account, so as to have an input for the 2nd parameter on AcquireTokenSilent() (an IAccount or loginHint string). In our case, we go further than just finding the "first" account - we search for a matching account that meets our expected account ID - as an additional security precaution.
The problem is likely because WAM doesn't provide Refresh Tokens into the MSAL user cache - but GetAccountsAsync() is (for some "privacy" reasons according to a code comment) defaulted to exclude Work and School Accounts. So our call to GetAccountsAsync() will only work whilst the token temporarily exists in the MSAL user cache. Once the access token has expired, GetAccountsAsync() needs to talk to WAM but it cannot because Work and School Accounts are excluded and the function returns an empty array.
I am trying a fix whereby if GetAccountsAsync() returns no matching accounts, then we will still call AcquireTokenSilent() anyway, but this time passing in a loginHint string instead (a UPN).
nbevans
commented
7 months ago No that didn't work.
Eventually, GetAccountsAsync() returns an empty list.
Passing in a loginHint instead will also result in the following exception.
Exception type: Microsoft.Identity.Client.MsalUiRequiredException
[21748] , ErrorCode: no_account_for_login_hint
[21748] HTTP StatusCode 0
[21748] CorrelationId
[21748]
[21748] at Microsoft.Identity.Client.Internal.Requests.Silent.SilentRequest.GetSingleAccountForLoginHintAsync(String loginHint)
[21748] at Microsoft.Identity.Client.Internal.Requests.Silent.SilentRequest.GetAccountFromParamsOrLoginHintAsync(IAccount account, String loginHint)
[21748] at Microsoft.Identity.Client.Internal.Requests.Silent.SilentRequest.UpdateRequestWithAccountAsync()
[21748] at Microsoft.Identity.Client.Internal.Requests.Silent.SilentRequest.ExecuteAsync(CancellationToken cancellationToken)
[21748] at Microsoft.Identity.Client.Internal.Requests.RequestBase.RunAsync(CancellationToken cancellationToken) I've tried configuring the broker to include OS/work/school accounts as follows but this also seems to be ignored:
.WithBroker(new BrokerOptions(BrokerOptions.OperatingSystems.Windows) { ListOperatingSystemAccounts = true });... still produces this warning trace output:
WAM::FindAllAccountsAsync returning no accounts due to configuration option This thing seems to be full of bugs?
nbevans
commented
7 months ago It seems the UAP broker doesn't honour the BrokerOptions as the code checks for UwpBrokerOptions (which cannot be and is not set by anything in newer versions)
I used reflection to set UwpBrokerOptions on the ApplicationConfiguration object. I verified with VS debugger that it was indeed set so that ListWorkAndSchoolAccounts = true and a HeaderText for extra points.
I re-ran the auth process and was now seeing this:
[WAM Proxy] WebAuthenticationCoreManager.FindAllAccountsAsync failed with error code 2147942405 error message Access is denied. and status NotAllowedByProvider So it seems enabling the work/school accounts mode by force also doesn't help. I guess that's why the UAP broker doesn't honour this setting.
So it does appear the UAP WAM broker is faulty. It will always sign you out eventually when doing a silent request by throwing a UiRequiredException eventually. I am talking like less than 12 hours. There is no logical reason for it to do this. The organisation account is the PRT account on the machine. It says "Connected with Windows" on the account picker displayed by the broker. The app registration /.default includes offline_access scope.
bgavrilMS
commented
7 months ago Hi @nbevans - the option to "ListWindowsWorkAndSchoolAccounts" is not needed and most apps are anyway prevented from actually discovering accounts for privacy reasons. We are considering removing it because it causes too much confusion.
The ideal pattern to use with WAM and get SSO with Windows (i.e. silently login the current Windows user) is described at https://aka.ms/msal-net-wam. In particular, pay attention to OperatingSystemAccount
Does this pattern not work for you?
IEnumerable<IAccount> accounts = await app.GetAccountsAsync();
IAccount existingAccount = accounts.FirstOrDefault();
try
{
if (existingAccount != null)
{
result = await app.AcquireTokenSilent(scopes, existingAccount).ExecuteAsync();
}
// Next, try to sign in silently with the account that the user is signed into Windows
else
{
result = await app.AcquireTokenSilent(scopes, PublicClientApplication.OperatingSystemAccount)
.ExecuteAsync();
}
}
// Can't get a token silently, go interactive
catch (MsalUiRequiredException ex)
{
result = await app.AcquireTokenInteractive(scopes).ExecuteAsync();
}If you don't want to get SSO with Windows, just remove the AcquireTokenSilent(scopes, PublicClientApplication.OperatingSystemAccount) - doesn't that work?
nbevans
commented
7 months ago Thanks for your reply, it is highly appreciated!
I have taken a look at your suggestion.
Signing into the PublicClientApplication.OperatingSystemAccount as a fallback seems like a potential security issue.
They might be signed into Windows with Account A. But deliberately signed into our app using Account B. If our app then "silently" switches them to Account A because GetAccountsAsync() suddenly returned no results... this might be considered a very surprising and unexpected behaviour?
I guess I could implement a safeguard to verify the returned IAccount matches our expected one and, if not, then do a RemoveAsync() call and throw an exception about the unexpected account. But whilst this solves the security issue, it still leaves open the issue that in this scenario the user is going to get signed out and require Interactive auth again.
I am currently trying the following:
var credentialAccount = (await app.GetAccountsAsync()).FirstOrDefault(candidateAccount => candidateAccount.HomeAccountId.Identifier == expectedUniqueAccountIdentifier);
var result = credentialAccount != null ? await app.AcquireTokenSilent(scopes, credentialAccount).ExecuteAsync() : await app.AcquireTokenSilent(scopes, PublicClientApplication.OperatingSystemAccount).ExecuteAsync();
if (result.Account.HomeAccountId.Identifier == expectedUniqueAccountIdentifier) {
return AuthenticateIdentityProviderAuthenticationResult.NewOkay(new AuthenticateIdentityProviderAuthenticationToken(AuthenticateIdentityProviderImpl.MicrosoftIdentity, result.Account.HomeAccountId.Identifier, result.Account.Username, result.CreateAuthorizationHeader()));
} else {
await app.RemoveAsync(result.Account);
return AuthenticateIdentityProviderAuthenticationResult.NewReauthenticate(null, "Operating system account was used in silent request but the returned account did not match our expectation.");
}Okay so that didn't work - but not quite for the reason I initially suspected.
Total number of access tokens in the cache: 0
Broker responded to silent request.
Broker responded to silent request.
=== Token Acquisition finished successfully:
AT expiration time: 07/02/2024 07:16:40 +00:00, scopes: redacted-client-id/.default redacted-client-id/email redacted-client-id/offline_access redacted-client-id/openid redacted-client-id/profile redacted-client-id/User.Read. source: Broker
Fetched access token from host login.microsoftonline.com.
[LogMetricsFromAuthResult] Cache Refresh Reason: NotApplicable
[LogMetricsFromAuthResult] DurationInCacheInMs: 327
[LogMetricsFromAuthResult] DurationTotalInMs: 2518
[LogMetricsFromAuthResult] DurationInHttpInMs: 0 It can be seen here that two silent calls were made. So that indicates the first silent call failed because either the IAccount was not acceptable or it was null (probably null). The second silent call (that uses the OperatingSystemAccount) apparently produced a result with an AT expiry of 07/02/2024 07:16:40. And this AT - albeit only 30mins till expiry - was valid at the time it was issued.
However when the HTTP request was made by my app the AT that was used was dated 06/02/2024 i.e. expired and so my API returned Unauthorized... so it was not the same AT as what was supposedly returned by MSAL. I have checked my app code to see if this is at all possible and it is not. It will only use the AT provided by MSAL's auth at that moment in time. My app doesn't do any untoward "caching" of AT itself (duh!) and it relies blindly on MSAL's results.
So is this suggesting there is some sort of bug in MSAL/WAM where by a genuine AT is produced via a silent OperatingSystemAccount call but MSAL then produces a result with a wrong/expired/former AT - perhaps inadvertently pulling an old AT from the user cache? That's the current theory.
I have added additional trace code to my app to try to understand the sequence of events in more detail. Now I'm waiting for the problem to occur again.
nbevans
commented
7 months ago I think it's a race condition with the system clock after the virtual machine (Parallels) is unpaused after having been paused for several hours (I.e. the AT has certainly expired)
Hard to repro.
Thinking of adding a request header of the current timestamp so server side can reject it with say BadRequest if the clock drift is too much.
bgavrilMS
commented
7 months ago I think it's a race condition with the system clock after the virtual machine (Parallels) is unpaused after having been paused for several hours (I.e. the AT has certainly expired)
Hard to repro.
Thinking of adding a request header of the current timestamp so server side can reject it with say BadRequest if the clock drift is too much.
Timestamps are tricky. The identity provider returns an expiration timespan (e.g. 3600 seconds) and MSAL, when persisting the token in its cache, uses Unix timestamps (so tied to the machine time). MSAL also uses 5 min buffer to account for clock skews.
I guess even adding a request header with the current timestamp is vulnerable to the race condition, but probably the window is smaller.
Does the resource return a good error message, e.g. "Access token is expired" ? In which case you can always go back to MSAL and ask for a new token, possibly using "WithForceRefresh(true)" which bypasses the access token cache (still uses the refresh token)
nbevans
commented
7 months ago The WithForceRefresh idea in response to the first Unauthorized seems like another idea worth exploring.
My idea with the timestamp header is to basically get a DateTime.UtcNow as the first thing I do before even touching MSAL. Then after MSAL has done its work and returned an AT (whether a good or bad one) then I throw all of that across to the server/resource and the server can check the timestamp header in the request and immediately reject the request but do it with a status code or response header combo that my client app can detect and realise it just suffered from the race condition.
But yeah - probably WithForceRefresh might be more robust, cover more potential scenarios than I presently know about, and be simpler to implement.
nbevans
commented
6 months ago I just witnessed my Parallels VM (Windows) literally take a good 30 seconds or so to synchronise the clock after resuming from being "paused" (not suspended). The clock was skewed by 24+ hours. Though I was not signed in via MSAL SSO at the time, this would definitely have been sufficient to trigger the issue.
I am wondering though - if the clock is skewed to that extent - won't WithForceRefresh actually fail too? Will WAM / Azure AD actually issue a token when the clock is so skewed? If so, will MSAL deal with a token whose "issued timestamp" is in the future? It raises many questions.
bgavrilMS
commented
3 months ago Support for UWP has been dropped. Please migrate to net8-windows
Library version used
4.57.0
.NET version
.NET Standard 2.0
Scenario
PublicClient - desktop app
Is this a new or an existing app?
The app is in production, and I have upgraded to a new version of MSAL
Issue description and reproduction steps
I have an UWP application and a .NET Standard 2.0 library. MSAL authentication is done in this .NET standard library, from a call in UWP application.
My first authentication goes fine. I'm able to run a native dialog, login with WAM and use the token until it expires. When the token is expired, by code does a silent refresh. This is the part that MSAL fails.
I receive the following error "Could not find a WAM account for the silent request."
When I run
await _publicClientApplication.GetAccountsAsync()I'm able to see the same account that was cached. It's there but MSAL fails to find it and fails to refresh the token. This is how I configure my public client application in the .NET Standard library:
Here are the logs that are generated by MSAL:
Relevant code snippets
No response
Expected behavior
No response
Identity provider
Microsoft Entra ID (Work and School accounts and Personal Microsoft accounts)
Regression
No response
Solution and workarounds
I can disable Windows broker options and it will work fine with old web authentication broker. I just want to use the native authentication broker dialog and integrated Windows login screen for authentication.