AzureAD / microsoft-authentication-library-for-dotnet

Microsoft Authentication Library (MSAL) for .NET
https://aka.ms/msal-net
MIT License
1.39k stars 340 forks source link

[Bug] GetAccounts being obsolete #2571

Closed fuocor closed 3 years ago

fuocor commented 3 years ago

Which Version of MSAL are you using ? Current

Platform All

Other? - please describe; I have an confidential client application that implements token caching and used the GetAccounts() to identify the account for the instance. By obsoleting the method, I am forced to carry around the account id for any token operations.

I find it poor judgement to have just cut it out without providing a means to identify the account associated with the instance of the client application.

jmprieur commented 3 years ago

@fuocor

Depending on the case you can get the account ID easily from the ClaimsPrincipal: https://github.com/AzureAD/microsoft-identity-web/blob/master/src/Microsoft.Identity.Web/ClaimsPrincipalExtensions.cs#L19

fuocor commented 3 years ago

I am not dragging around a ClaimsPrincipal. They are multi-tenant micro-services running as their owner, which is tied to a MSGraph DirectoryObject. I have the DirectoryObject.Id. By making the change, I need to pull in the TenantId in order to come up with the AccountId

jmprieur commented 3 years ago

you mean, plain .NET Core? or plain .NET Framework, @fuocor ?

fuocor commented 3 years ago

.NET Core 5

jmprieur commented 3 years ago

Do no ASP.NET Core 5.0? and you don't have an HttpContext?

fuocor commented 3 years ago

No, they are Orleans stateless grains tied to an IoT device. I plowed through my code and found that I can work around the problem for the most part. But from a design perspective, if the ClientApplication instance is tied to a single account (as recommended), there should be no need to know the account to acquire a token

I should be able to use dependency injection and get the ClientApplication and get a token. Which is the pattern I am using.

jmprieur commented 3 years ago

Which flow are you using, @fuocor ? I'd assumer OBO? If that's the case the user is not needed. Do you want to share how you use MSAL.NET ?

fuocor commented 3 years ago

Sure. it's a little long winded so I will write it up this evening.

fuocor commented 3 years ago

I am tracing through the code and have an issue. Using a multi-tenant confidential client, and OBO flow with caching. If user1 lives in tenant1, but is a member of tenant2, When authenticating using the authority for Tenant2, the oid and tid in a JWT will be from will be from tenant2, but the AccountId in the AuthenticationResult will be from tenant1. This makes finding the AccountId very difficult if I do not have the tenant1 information at hand. Previously, the GetAccountsAsync() would always return the correct information, because there was only ever one account held by the ConfidentialClientApplication

For time being I am working around it using:

        public static async Task<IAccount> GetAccountAsync(this IConfidentialClientApplication confidentialClient, Guid userId, Guid tenantId)
        {
            var clientApplication = (IClientApplicationBase)confidentialClient;
            var accounts = await clientApplication.GetAccountsAsync().ConfigureAwait(false);
            return accounts.Single();
        }

but some means of mapping the userId and tenantId to the HomeAccountId, when the user lives in multiple tenants, needs to be addressed.

jmprieur commented 3 years ago

@fuocor : For the OBO flow, you don't need to use AcquireTokenSilent before using AcquireTokenOnBehalfOf. OBO does the cache lookup itself (because the cache key is the incoming token hash, and not the account).

About accounts, indeed, in guest scenarios, the HomeAccountId / HomeObjectId are the one from the home tenant (tenant1 in your case), as this is in general the key for AcquireTokenSilent. But indeed, for OBO you don't need it.

See https://github.com/AzureAD/microsoft-identity-web/blob/b93c67676f8755f5a9c692aeb77686fc1757e3aa/src/Microsoft.Identity.Web/TokenAcquisition.cs#L598-L648 for an example of implementation

fuocor commented 3 years ago

My scenario is not quite that.

  1. The user logs in to a client application and acquires a token to the web API using the user_impersonation scope.
  2. The user then allocates a virtual actor. The virtual actor executes the AquireTokenOnBehalfOf. That token is stored away and the VirtualActor/MicroService is deallocated when not referenced.
  3. At some point something happens to active the VirtualActor (non-interactive), Upon activation, the actor will recall the token that was stored away in step 2. It does this by creating a ConfidentialClientApplication tied to the cache of the OBO.
  4. Now it will call AquireTokenSilent and leverage the refresh token to get an updated token.
jmprieur commented 3 years ago

@fuocor

fuocor commented 3 years ago

I am already caching and it works fine. The issue is the discrepancy between the AccountId in the MSAL AuthenticationResult and the oid and tid in an access token when the caller is a guest member of the authority domain. The values in the token come from the authority, whereas the values in the MSAL AccountId come from the home directory of the user.

jmprieur commented 3 years ago

yes, @fuocor : this is by design of the AccountId, and I understand what you are saying. With GetAccounts (even if you'll have a warning when you use it), does it work for you?

@henrik-me : it might be time to implement the "Tenant Profile" consistency item?

fuocor commented 3 years ago

The GetAccountsAsync is what I am dependent on. Without it I have nothing to correlate between the same user account coming out of different tenants

bgavrilMS commented 3 years ago

The scenario you are looking for is "OBO for long lived processes" - see https://github.com/AzureAD/microsoft-identity-web/wiki/get-token-in-event-handler

bgavrilMS commented 3 years ago

Closing for now, please reopen if you have other questions.