fix null ref in tokenCache when UserInfo is absent #1607
Conversation
utest for IdToken and WriteMsalRefreshToken()
| result.Result.UserInfo.GivenName, | ||
| result.Result.UserInfo.FamilyName, | ||
| result.Result.UserInfo?.GivenName, | ||
| result.Result.UserInfo?.FamilyName, |
There was a problem hiding this comment.
adding ? is the actual fix. the customer was getting a null ref because UserInfo is null here. There will no longer be a null ref here.
#Closed
There was a problem hiding this comment.
Is it then possible to complete the scenario? I believe the MSAL cache will now be empty and thus only when using the ADAL cache will there be any values in the cache... thus depending on how the customer persists the cache (which serializer/deserializer) this may or may not work? #Closed
There was a problem hiding this comment.
you are right. if there is no id_token, then we will exit out of writing items to the MSAL cache...see here however, i'm assuming, since this has been out for awhile, that it hasn't been very impactful, or our customers are not using the cache in that way?
In reply to: 288842460 [](ancestors = 288842460)
There was a problem hiding this comment.
addressed the comments. lmk if you'd like to discuss anything further. thanks for the utest suggestions and improvements.
In reply to: 288855740 [](ancestors = 288855740,288842460)
| // arrange | ||
| TokenCache tokenCache = new TokenCache(); | ||
|
|
||
| var result = CreateAdalResultWrapper(); |
There was a problem hiding this comment.
nit: wrapper (or similar) #Closed
| }; | ||
| } | ||
|
|
||
| private void WriteMsalRefreshToken(AdalResultWrapper result, ITokenCacheAccessor tokenCacheAccessor) |
There was a problem hiding this comment.
suggestion: WriteMsalRefreshTokenWithNullAsGivenNameAndFamilyName / WriteMsalRefreshTokenWithNullUserInfoValues #Closed
| // assert | ||
| // IdToken present | ||
| Assert.AreEqual(tokenCache.TokenCacheAccessor.AccountCount, 1); | ||
| Assert.AreEqual(tokenCache.TokenCacheAccessor.RefreshTokenCount, 1); |
There was a problem hiding this comment.
Assert.AreEqual [](start = 12, length = 15)
can it be tested that the userinfo values are null? #Closed
|
|
||
| var result = CreateAdalResultWrapper(); | ||
|
|
||
| result.Result.IdToken = "some-id-token"; |
There was a problem hiding this comment.
result.Result.IdToken = "some-id-token"; [](start = 12, length = 40)
perhaps something to be passed into the wrapper? Or another method which does this? seems like magic what you are doing here, this can be better explained with a method names elegantly imo #Resolved
|
I believe the added null checks should go in, have a few questions around the change more than actual comments relevance of the fix. #Resolved |
|
sure. we can sync up tomorrow. In reply to: 497186624 [](ancestors = 497186624) |
utest for IdToken and WriteMsalRefreshToken()
Alternative for issue 1604