Closed TiagoBrenck closed 5 years ago
jmprieur
commented
5 years ago @TiagoBrenck : thanks for investigating Given that the sample used to work fine with guest accounts in the past (and even the recent past), are you suggesting there is a regression in MSAL.NET ?
antonGritsenko
commented
5 years ago @jmprieur yes, there is a regression somewhere in MSAL.NET either Azure AD itself.
When I sent request for JWT token the answer contains following:
"oid": "124b8735-*db4a0" "tid": "4cc757a6-**0b37"
and same id returned by GetMsalAccountId() extension method. And this Id is of the guest account in the app's tenant:
ObjectId UserPrincipalName
-------- -----------------
124b8735-********db4a0 anton.gritsenko_tenat1.d#EXT#@tenant2.onmicrosoft.com
But Microsoft.Identity.Client.AccountId returned by TokenCacheNotificationArgs contains totally different id. This id match to id of the my user object in the my account's tenant.
ObjectId UserPrincipalName
-------- -----------------
1254cb48-*******159cbbaf anton.gritsenko@tenat1.d
henrik-me
commented
5 years ago @MarkZuber: can you pls. investigate this one?
jmprieur
commented
5 years ago About https://github.com/Azure-Samples/active-directory-aspnetcore-webapp-openidconnect-v2/issues/115: I've looked at the history for the ASP.NET Core Web App tutorial and although I know I tested it in guest scenarios (and it worked), I could not find out how we made it work. I suppose that this was before the advanced cache implementations we've done since. (and the coordinates of my test app use common ... I think we do have beeen having a test gap with the guest scenario for a while). Thanks @TiagoBrenck for the heads-up.
I'm tempted to think that indeed, the initial cache implementations were writing all the users, as this cannot work today with proper cache in confidential client apps (where there is one cache per user) See https://github.com/Azure-Samples/active-directory-aspnetcore-webapp-openidconnect-v2/blob/47f1fc57b3574c011cd52d2bcc45b240f29fcc03/Microsoft.Identity.Web/Client/TokenAcquisition.cs#L362-L367
These lines of code are, in particular, suspect:
// Special case for guest users as the Guest iod / tenant id are not surfaced.
if (account == null)
{
var accounts = await application.GetAccountsAsync();
account = accounts.FirstOrDefault(a => a.Username == loginHint);
}Now back to this issue: MSAL.NET returns what it claims it does: IAccount has a member name [HomeAccountId] which is the account information (tenant ID and object ID) in the home tenant.
I've tested with MSAL 3.x and the behavior is the same, so I don't think that this is a regression with MSAL.NET nor Azure AD. I've thinking that this is a regression in the sample, because the cache implementations were wrong (and not concurrent) but worked in that case (pure chance)
Now, @MarkZuber, would we want to pass to the TokenCacheNotificationArgs an account that would not be the home account id ? but the account id. I would not think so, but please jump-in (this would be a change of behaviro, but as guest scenarios don't seem to work today, I don't think that this would break anybody)
What happens? In the single-tenant Web App scenario with a guest account. When the user signs-in we get the tenantID and the object id in the guest tenant. We could also use the idp claim to get the home tenant ID, but the home object ID is not available. It is only available in the UserInfo which we get when we call the token endpoint.
How to fix the original issue?
I'm not sure we don't have a chicken and egg problem here.
Two solutions to fix the problem:
1) Have MSAL.NET expose the (guest) iod/tid. For instance IAccount could expose both the HomeAccountId and the AccountId. We have an API Review spec for that: See Account & Tenant Profiles). We could in particular already implement the Claims part (as the iod/tid is in the ID token)
2) Have the Web App infer the home iod/tid. For instance by going to the UserInfo endpoint to get the additional information about the home coordinates. I don't know if we can do that
cc: @MarkZuber as @henrik-me asked you to investigate cc: @bgavrilMS as we discussed the userinfo this afternoon.
For now: proposing to: 1) create another issue to expose the claims of the IDToken (MSAL Android / iOS already do it, and JS has a PR in progress) 1) close this issue as external. Although, @MarkZuber : would it make sense that the 2) reopen the sample's issue as there will be a bit of work in the sample when we surface the claims. https://github.com/Azure-Samples/active-directory-aspnetcore-webapp-openidconnect-v2/issues/115. Meanwhile a work around is to extract iod and tid from the decoded ID token.
antonGritsenko
commented
5 years ago Thanks for detailed clarification. IMHO, as a customer, the solution 1 is more preferred, because if we talking about claims then there is no any information about a home account inside of the JWT (at least by default), and additional query to get home account is the performance penalty.
jmprieur
commented
5 years ago yes, @antonGritsenko you're right BTW the claims would be the claims in the IDToken. The claims in the Access token are only for the Web API for which the token was emitted (the Web App should not even look at them)
TiagoBrenck
commented
5 years ago On TokenCache.ITokenCacheInternal.cs, line 500 and 502, a TokenCacheNotificationArgs is instantiated with account null then passed to OnBeforeAccessAsync, which will trigger our TokenCacheProviders and it wont be able to get they key from args.Account?.HomeAccountId?.Identifier;. The reason that account is null, is because that method, on line 500, is the one to find an account. It makes totally sense that it doesn't have an IAccount object at that point.
Like Jean-Marc mentioned, we are on a classic chicken and egg dilemma here. My opinion is that the Microsoft.Identity.Client is correct, but our Microsoft.Identity.Web project is not following the library design. To access an entry from msal cache, we need the key HomeAccountId, which is uid.utid. The way our CacheProviders get this key is what needs to be changed. If it cant get from args.Account?.HomeAccountId?.Identifier the fallback should be a method that would find the logged user uid and utid. Never oid.tid (like our CacheProviders are currently doing).
I will try to fix it and propose a PR for our Microsoft.Identity.Web project. I don't think this issue belongs to the library.
cc: @jmprieur @bgavrilMS @MarkZuber @henrik-me Doesn't this make sense to you?
jmprieur
commented
5 years ago Thanks for confirming, @TiagoBrenck closing. cc; @henrik-me @MarkZuber
Which Version of MSAL are you using ? MSAL 4.0.0
Platform netcore
What authentication flow has the issue?
Repro
After investigating the issue, https://github.com/Azure-Samples/active-directory-aspnetcore-webapp-openidconnect-v2/issues/115, I found that the args.Account.HomeAccountId.Identifier has a problem when dealing with Guest Users (if it is not the Identifier problem, the cache key will be the issue). To reproduce the scenario, you must have a guest user in your testing Azure AD.
Here are the steps to help understand the issue:
You will observe that right after logging in, the breakpoint on StartupHelpers.cs will get hit and for this flow, args.Identifier has a value. This means that we added an entry in the cache with the key that came from args.Account.HomeAccountId.Identifier. Then, when you click on Profile menu, the breakpoint on line 41 will get hit but for this flow, args.Identifier will be null. The fallback code is to use httpContextAccessor.HttpContext.User.GetMsalAccountId(), which will return a value, however this value is not the same as the args.Identifier`s returned on the previous flow resulting in not finding any entry in the cache which causes this whole loop problem.
Expected Behavior
I would expect that args.Account.HomeAccountId.Identifier would have a value on both flows or that httpContextAccessor.HttpContext.User.GetMsalAccountId() returned the same value as args.Account.HomeAccountId.Identifier