Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Bug] ArgumentNullException when HomeAccountId is null and acquiring OBO token #4670

Closed
Frey-Wang opened this issue Mar 21, 2024 · 6 comments

Comments

@Frey-Wang
Copy link

Library version used

4.59.0

.NET version

All platform

Scenario

ConfidentialClient - web api (AcquireTokenOnBehalfOf)

Is this a new or an existing app?

This is a new app or experiment

Issue description and reproduction steps

When acquiring OBO tokens and the HomeAccountId is null, we'll receive this exception:

Exception type: System.ArgumentNullException
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, TValue value)
   at Microsoft.Identity.Client.PlatformsCommon.Shared.InMemoryPartitionedUserTokenCacheAccessor.SaveIdToken(MsalIdTokenCacheItem item)
   at Microsoft.Identity.Client.TokenCache.<Microsoft-Identity-Client-ITokenCacheInternal-SaveTokenResponseAsync>d__55.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at Microsoft.Identity.Client.TokenCache.<Microsoft-Identity-Client-ITokenCacheInternal-SaveTokenResponseAsync>d__55.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.Identity.Client.Cache.CacheSessionManager.<SaveTokenResponseAsync>d__10.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.Identity.Client.Internal.Requests.RequestBase.<CacheTokenResponseAndCreateAuthenticationResultAsync>d__24.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.Identity.Client.Internal.Requests.OnBehalfOfRequest.<FetchNewAccessTokenAsync>d__5.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.Identity.Client.Internal.Requests.OnBehalfOfRequest.<RefreshRtOrFetchNewAccessTokenAsync>d__4.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.Identity.Client.Internal.Requests.OnBehalfOfRequest.<ExecuteAsync>d__3.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.Identity.Client.Internal.Requests.RequestBase.<RunAsync>d__12.MoveNext()

According to public document this property can be null, thus the library should handle the null case: https://learn.microsoft.com/en-us/dotnet/api/microsoft.identity.client.iaccount.homeaccountid?view=msal-dotnet-latest

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

No response

@Frey-Wang Frey-Wang added needs attention Delete label after triage untriaged Do not delete. Needed for Automation labels Mar 21, 2024
@bgavrilMS bgavrilMS added bug P2 Supportability confidential-client and removed untriaged Do not delete. Needed for Automation needs attention Delete label after triage labels Mar 21, 2024
@bgavrilMS
Copy link
Member

MSAL cannot cache user tokens without an account id. The account id is derived from the client_info (home oid + home tid) or, if this is not present, from the IdToken's sub claim.

MSAL always asks for an ID Token and as per OIDC spec, the sub claim is mandatory.

So the correct behavior is for MSAL to fail with an exception if the account ID is null and we're in OBO scenrio (denoted by API_ID for example). Code should go in TokenCache's SaveMsalResponse.

Exception message should be "No account ID can be inferred from the STS response. In OBO scenarios, this can happen if the OBO call targets a tenant which is different than the tenant from the client token (the tid claim of the client token)"

Note: we have confirmed with ESTS that this behavior can occur with OBO for SP.

@Frey-Wang
Copy link
Author

Hi @bgavrilMS , thanks for the information. Does that mean MSAL currently cannot support SPN OBO for cross tenant scenario?

@bgavrilMS
Copy link
Member

Yes @Frey-Wang, SPN OBO for cross-tenant is not supported by neither ESTS nor MSAL

@Frey-Wang
Copy link
Author

@bgavrilMS, I found this post to support SPN OBO: #1845. So is it supposed for first party app to use the same tenant id as the SPN to generate this token since it only supports single tenant scenario (even though the first party app has a different tenant id)?

@bgavrilMS
Copy link
Member

@Frey-Wang - SPN OBO is currently used by a handful of 1st party applications and must be enabled manually by the STS team. It is not supported to 3p - but this is on the backlog to be productized at some point.

Single tenant scenario only is needed - client app and web api must target the same tenant id.

@Frey-Wang
Copy link
Author

Got it. Thanks for the clarification!

@github-project-automation github-project-automation bot moved this from Committed to Done in MSAL Customer Trust / QM Mar 26, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
Archived in project
2 participants