Null Ref when IdToken is not returned

[jennyf19]

Which Version of ADAL are you using ?
5.0.5 - v4.

Repro

AuthenticationResult res = await new AuthenticationContext(authority)
                                           .AcquireTokenByAuthorizationCodeAsync(arg.Code, new Uri(postLogoutRedirectUri), new ClientCredential(clientId, clientSecret));
           string token = res.AccessToken;

ADAL does not send the scope OpenId w/the AuthCode flow, so no idToken is returned. The idToken is used when writing to the MSAL cache.

In CacheFallbackOperations.WriteMsalRefreshToken, we don't check for null in result.Result.UserInfo.GivenName, so there is a null ref thrown here.

Expected behavior

1. OpenId scope should be sent w/each request, as we do in MSAL
2. Check for null in UserInfo and not throw, later the code will return if no IdToken is present and not attempt to create an MSAL refresh token and account object.

Actual behavior
System.NullReferenceException: Object reference not set to an instance of an object. at Microsoft.IdentityModel.Clients.ActiveDirectory.TokenCache.StoreToCacheCommon(AdalResultWrapper result, String authority, String resource, String clientId, TokenSubjectType subjectType, RequestContext requestContext) in D:\a\1\s\src\Microsoft.IdentityModel.Clients.ActiveDirectory\TokenCache.cs:line 692

[jmprieur]

@jennyf19
In ASP.NET / ASP.NET core apps things are a bit different as ASP.NET request the scopes and starts the authorization flow, and then the code is redeemed by ADAL or MSAL when the code is received
I don't think that the issue is with ADAL.NET. Isn't the issue that the ResponseType requested in the ASP.NET app did not contain the idtoken.

See this line: https://github.com/Azure-Samples/active-directory-aspnetcore-webapp-openidconnect-v2/blob/418e4880ce3307befb25c7af600a886560cadcaa/Microsoft.Identity.Web/StartupHelpers.cs#L101 for ASP.NET core and https://github.com/Azure-Samples/ms-identity-aspnet-webapp-openidconnect/blob/aed83053c39e9acb5a9b031fa67d05161e5faaa2/WebApp/App_Start/Startup.Auth.cs#L41 for ASP.NET?

I would think that the code redemption just provides the scopes that were requested during the first leg (going to the Authorize endpoint)

[jmprieur]

Let's discuss this at standup.

[henrik-me]

@jmprieur : How do you envision that we should ensure we get the idtoken? How do we handle the error? provide a message saying how to fix this for web applications? Is this something the webextensions.withMicrosoftIdentityPlatform is supposed to address? Ideas on what we do short term to improve supportability?

[henrik-me]

@jennyf19 : which scenario is this addressing? As JM says web apps using asp.net and aspnet core should be changed as it's done in the samples. However I was thinking this was solving it for someone else for some other scenario?

[jmprieur]

webextensions.withMicrosoftIdentityPlatform ? do you mean Microsoft.Identity.Web, @henrik-me : if that's the case, yes, it handles it.

BTW, I'm not opposed in adding the openid scope but only in flows where we need it (not in Client credentials). In the case of AcquireTokenByAuthorizationCode does it add more network calls, @jennyf19 ? also does it prevent from having other scopes?

[jennyf19]

@jmprieur @henrik-me I'll just revert the changes and include the one for the null ref check. in msal we are sending openId w/all the calls, so was doing something similar here, but i'll leave it. It's not an extra network call as it's already included in the request. No reason to spend time on a non-issue.

[jmprieur]

@jennyf19 I actually checked today that the v2.0 endpoint demands the openid claim for grant_type == "authorization_code".
Apparently does not, but if it does not harm feel free to add it.

[jennyf19]

@jmprieur yes...it is one of the three required scopes for v2, but I don't see a good reason to make the changes now in ADAL.

@henrik-me - what are your thoughts? I can just do the null ref check and that's it. Seems like the best option.

[jmprieur]

What I would not want is that because we request the scope, something negative side effect happen like changing the session, which will break existing ADAL apps. Maybe the right fix is indeed to just check for the null ref as you propose, @jennyf19
cc: @henrik-me

[jennyf19]

fixed in adal 5.1.0