Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[Bug] [M] MSAL.NET fails if B2C user flows/policies have a name containing a dot (.) #2444

Closed
endintiers opened this issue Mar 3, 2021 · 5 comments · Fixed by #2648
Closed

Comments

@endintiers
Copy link

endintiers commented Mar 3, 2021

Which Version of MSAL are you using ?
MSAL 4.27

Platform
Xamarin/UWP

What authentication flow has the issue?
Other? - please describe;
AcquireTokenSilent/Identity Token Cache

Is this a new or existing app?
c. This is a new app or experiment

Repro
Create a B2C flow called {a}.signupsignin
AcquireTokenInteractive using that flow - works.
... later ...
GetAccountsAsync, which returns a valid Account
call IPublicClientApplication.AcquireTokenSilent with that account

Expected behavior
A user token is acquired from the token cache for the account.

Actual behavior
throws Microsoft.Identity.Client.MsalClientException: 'Returned user identifier does not match the sent user identifier when saving the token to the cache. '

Possible Solution
I assume the problem is the extra '.' in the homeAccountId.Identifier which looks like: {uid}-{a}.signupsignin.{tenantId}

I notice that in various places in the library, for example AccountId.ParseFromString, there is code such as
string[] elements = str.Split('.');
And then an assumption that there will only be 2 elements...

Rather than changing the split character (in many places) - which would probably cause other problems, or trying to deal with an array of length 'n', I would suggest:

  1. Documenting: "Don't use '.' in your B2C flow/Policy names"
  2. Checking for extra '.'s in homeAccountId.Identifiers when first seeing an Account - probably only needed in 3(ish) places? and throwing an exception (referencing the documentation).
@jmprieur jmprieur changed the title [Bug] [Bug] MSAL.NET fails if B2C user flows/policies have a name containing a dot (.) Mar 3, 2021
@endintiers
Copy link
Author

endintiers commented Mar 4, 2021

An alternative could be to unseal "Account".
In which case: string userIdentifier = ((Microsoft.Identity.Client.Account)account).HomeAccountId.Identifier.
would work, so no need to split the string?
(assuming we check that the IAccount is an Account first of course)

@bgavrilMS
Copy link
Member

Looks more like a bug to me @jmprieur

@jmprieur jmprieur added bug and removed enhancement labels Mar 4, 2021
@bgavrilMS bgavrilMS added the P2 label Mar 31, 2021
@bgavrilMS bgavrilMS added this to Todo in MSAL.NET (legacy) via automation Mar 31, 2021
@bgavrilMS bgavrilMS added this to the 4.30 milestone Mar 31, 2021
@bgavrilMS bgavrilMS changed the title [Bug] MSAL.NET fails if B2C user flows/policies have a name containing a dot (.) [Bug] [M] MSAL.NET fails if B2C user flows/policies have a name containing a dot (.) Mar 31, 2021
@bgavrilMS
Copy link
Member

See if we can use better parsing on AccountId. Otherwise, we can disallow dots.

@pmaytak pmaytak modified the milestones: 4.30, 4.31 Apr 20, 2021
@bgavrilMS bgavrilMS moved this from Todo to Estimated/Committed in MSAL.NET (legacy) Apr 22, 2021
@pmaytak pmaytak modified the milestones: 4.31.0, 4.32.0 May 11, 2021
@bgavrilMS bgavrilMS moved this from Estimated/Committed to Todo in MSAL.NET (legacy) May 13, 2021
@bgavrilMS bgavrilMS moved this from Todo to Estimated/Committed in MSAL.NET (legacy) May 19, 2021
@neha-bhargava neha-bhargava self-assigned this May 20, 2021
@neha-bhargava neha-bhargava moved this from Estimated/Committed to In Progress in MSAL.NET (legacy) May 20, 2021
@bgavrilMS bgavrilMS linked a pull request May 24, 2021 that will close this issue
MSAL.NET (legacy) automation moved this from In Progress to Fixed May 24, 2021
@jmprieur
Copy link
Contributor

@bgavrilMS : did we decide to close issues instead of adding the "Fixed" label?

@bgavrilMS
Copy link
Member

Yes, I did not hear any objections.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

5 participants