{"action":"opened","issue":{"url":"https://api.github.com/repos/AzureAD/microsoft-authentication-library-for-dotnet/issues/104","repository_url":"https://api.github.com/repos/AzureAD/microsoft-authentication-library-for-dotnet","labels_url":"https://api.github.com/repos/AzureAD/microsoft-authentication-library-for-dotnet/issues/104/labels{/name}","comments_url":"https://api.github.com/repos/AzureAD/microsoft-authentication-library-for-dotnet/issues/104/comments","events_url":"https://api.github.com/repos/AzureAD/microsoft-authentication-library-for-dotnet/issues/104/events","html_url":"https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/issues/104","id":165695428,"number":104,"title":"Application.AcquireTokenAsync() occasionally returns null access token from cache","user":{"login":"kpanwar","id":5713915,"avatar_url":"https://avatars.githubusercontent.com/u/5713915?v=3","gravatar_id":"","url":"https://api.github.com/users/kpanwar","html_url":"https://github.com/kpanwar","followers_url":"https://api.github.com/users/kpanwar/followers","following_url":"https://api.github.com/users/kpanwar/following{/other_user}","gists_url":"https://api.github.com/users/kpanwar/gists{/gist_id}","starred_url":"https://api.github.com/users/kpanwar/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/kpanwar/subscriptions","organizations_url":"https://api.github.com/users/kpanwar/orgs","repos_url":"https://api.github.com/users/kpanwar/repos","events_url":"https://api.github.com/users/kpanwar/events{/privacy}","received_events_url":"https://api.github.com/users/kpanwar/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2016-07-15T01:52:48Z","updated_at":"2016-07-15T01:52:48Z","closed_at":null,"body":"Since upgrading to ADAL library version 3.10.305231913 (from 2.4.x), we have been seeing occasional cases where the access token returned from the cache is null. Specifically, we’re seeing a call to AcquireTokenAsync returns a successful AuthenticationResult, but the AccessToken member is null. There is no indication of any failure.\r\n \r\nWe are using application identities in a multi-tenant application.  We see this issue especially when running tests under high-volume, multi-threaded conditions across multiple tenants.  In addition, we’ve seen many extra calls out to the Azure Active Directory Identity Provider that we did not expect, after the first token is successfully acquired.  Based on looking at and debugging the ADAL library, it appears that the multi-tenant scenario with application identities may not be handled correctly and that legitimate items are being altered and removed from the cache when they shouldn’t be.\r\n\r\nThe area in question is in TokenCache.cs, with code added during commit 74eb116886104222ccf6b2eb48f8e0f29f8d1d67, 28 Sep 2015.  The following was added to **LoadSingleItemFromCache()**.  We believe this case should only apply when a user identity is involved, since an application identity should not have a refresh token.  Focusing in on the highlighted lines below:\r\n\r\n```C#\r\n// check for tokens issued to same client_id/user_id combination, but any tenant.\r\nif (returnValue == null)\r\n{\r\n    List<KeyValuePair<TokenCacheKey, AuthenticationResultEx>> itemsForAllTenants = this.QueryCache(null, cacheQueryData.ClientId, cacheQueryData.SubjectType, cacheQueryData.UniqueId, cacheQueryData.DisplayableId);\r\n    if (itemsForAllTenants.Count != 0)\r\n    {\r\n        returnValue = itemsForAllTenants.First();\r\n    }\r\n\r\n    // check if the token was issued by AAD\r\n//>>>>>\r\n    if (returnValue != null &&\r\n        Authenticator.DetectAuthorityType(returnValue.Value.Key.Authority) != AuthorityType.ADFS)\r\n//<<<<<\r\n    {\r\n        //remove access token to redeem refresh token against a different tenant.\r\n        returnValue.Value.Value.Result.AccessToken = null;\r\n    }\r\n    else\r\n    {\r\n        returnValue = null;\r\n    }\r\n}\r\n```\r\n\r\nThe basic scenario is as follows:\r\n\r\n1. Create an AuthenticationContext with a **tenant ID 1** as part of the authority URI and call AcquireTokenAsync().  Token is acquired successfully from the Azure Active Directory Identity Provider and placed in the cache.\r\n2. Create an AuthenticationContext with a **tenant ID 2** and call AcquireTokenAsync().  The exact authentication result is not found in the cache because it’s a different authority.  However, this branch now gets the previous result from the _all tenants_ query and nulls out the access token.  The old AuthenticationResultEx is removed from the cache in LoadFromCache() because ```AccessToken == null and RefreshToken == null```.  A new token is acquired from the Azure Active Directory Identity Provider and placed in the cache.\r\n3. Create an AuthenticationContext with **tenant ID 1** again and call AcquireTokenAsync().  Since the previous authentication result was removed from the cache, this now goes to the _all tenants_ query again, nulling out the access token for **tenant ID 2**, causing that item to be removed from the cache and an unexpected call to the Azure Active Directory Identity Provider.\r\n4. Rinse, lather, and repeat.\r\n\r\nWhile access to the cache itself is protected by locks, access to the fields in the returned AuthenticationResultEx does not appear to be.  Hence we believe the null access token is caused under the following scenario:\r\n\r\n1. Two calls come in at nearly the same time, one for **tenant 1** and one for **tenant 2**.\r\n2. We acquire the token for **tenant 1**, the authentication result is placed in the cache and returned to the caller (and the cache is unlocked).\r\n3. Attempting to get a token for **tenant 2**, but _before the first caller has had the chance to use the token for tenant 1_, an AuthenticationResult is not found in the cache for tenant 2, so the first AuthenticationResult is returned from the *all tenants* query and the AccessToken is nulled out.\r\n4. The first caller attempts to use the access token in the AuthenticationResult, that has now been nulled out.\r\n\r\nA suggestion would be for the highlighted lines to read something like:\r\n```C#\r\n    // check if the token was issued by AAD\r\n    if (returnValue != null && returnValue.Value.Value.RefreshToken != null &&\r\n        Authenticator.DetectAuthorityType(returnValue.Value.Key.Authority) != AuthorityType.ADFS)\r\n```\r\n\r\nor:\r\n```C#\r\n    // check if the token was issued by AAD\r\n    if (returnValue != null && returnValue.Value.Key.TokenSubjectType != TokenSubjectType.Client &&\r\n        Authenticator.DetectAuthorityType(returnValue.Value.Key.Authority) != AuthorityType.ADFS)\r\n```\r\n\r\nIn addition, we suspect that setting the AccessToken on an existing AuthenticationResult to null may not be safe because a reference to the AuthenticationResult may have previously been handed off to the caller and being used simultaneously at that point.  Setting the AccessToken to null seems mainly to be an indicator that an item needs to be removed from the cache, *and can also occur during a refresh case*.  A separate flag might be a better option here instead."}}
{"actor":{"display_login":"kpanwar"}}