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

Sample for ms-identity-aspnet-webapp-openidconnect-master throws exception on AcquireTokenSilent #2324

Closed
bmukes opened this issue Jan 4, 2021 · 11 comments

Comments

@bmukes
Copy link

bmukes commented Jan 4, 2021

I ran your sample app and replaced the values for ida:ClientId, ida:ClientSecret and the Authority with values from my Azure Active Directory Tenant.
I registered an application within my tenant and set API permissions as shown in the image below

image

Authentication for the application is setup as shown in the image below

image

I get logged in successfully but when I press the Send Email link I notice that the call to app.AcquireTokenSilent always throws and exception. The exception is thrown because the call to await app.GetAccountAsync(ClaimsPrincipal.Current.GetAccountId()); always returns null.

See the partial code on the HomeController.cs below

        [Authorize]
	[HttpGet]
        public async Task<ActionResult> SendMail()
        {
            // Before we render the send email screen, we use the incremental consent to obtain and cache the access token with the correct scopes
            IConfidentialClientApplication app = await MsalAppBuilder.BuildConfidentialClientApplication();
            AuthenticationResult result = null;
            var account = await app.GetAccountAsync(ClaimsPrincipal.Current.GetAccountId());
            string[] scopes = { "Mail.Send" };

            try
            {
				// try to get an already cached token
				result = await app.AcquireTokenSilent(scopes, account).ExecuteAsync().ConfigureAwait(false);
            }

My assumption was that this sample would show that MSAL would have cached any of the tokens necessary for the call and that the call to GetAccountAsync would not return null. The Active Directory Tenant is not verified so the user login ends with onmicrosoft.com

I am seeing the same behavior in my ASP.NET MVC application using your code so I wondered if I am missing something?

@bmukes bmukes changed the title Sample for ms-identity-aspnet-webapp-openidconnect-master throws exceptions on AcquireTokenSilent Sample for ms-identity-aspnet-webapp-openidconnect-master throws exception on AcquireTokenSilent Jan 4, 2021
@bmukes
Copy link
Author

bmukes commented Jan 4, 2021

I think I need to provide additional information on how I am using MSAL.
My application is ASP.NET web application using the 4.72 framework.
I am using the OIDC Authorization Code Flow
I am using the MsalAppBuilder class from the ms-identity-aspnet-webapp-openidconnect-master sample app within the OpenIdConnectAuthenticationNotifications.AuthorizationCodeReceived delegate to acquire an access token and a refresh token.
all of that works as expected.

I have found two issues I cannot get around.
The first issue is that the call to app.GetAccountAsync in any controller returns null which means that I cannot get the cached tokens. I know it's caching the token because in the notification I call GetAccounts and it returns me the expected account.

The second issue depends upon the AAD member type of the user logging in.
Here is the setup.

  • I have an unverified AAD account (call it Tenant A) with 2 users.
  • One user is AAD member type Member and the other is AAD member type Guest
  • Assume that the Guest user is from its (Home Tenant)
  • I have the code var accounts = await clientApp.GetAccountsAsync(); System.Diagnostics.Debugger.Break() in OpenIdConnectAuthenticationNotifications.AuthorizationCodeReceived
  • When the user is of member type Member the values returned are the objectid.tenantid from Tenant A
  • When the user is of member type Guest the values returned are the objectid.tenantid from the Home Tenant.

Their are extension methods in Microsoft.Identity.Web

  • GetHomeObjectId
  • GetHomeTenantId

that could return these values but they always return null because they are looking for claim values that are not returned by AAD and I see no way to add them.

@jmprieur
Copy link
Contributor

jmprieur commented Jan 5, 2021

@bmukes :

To answer some of your questions:

@jmprieur
Copy link
Contributor

jmprieur commented Jan 5, 2021

Thinking more, Microsoft.Identity.Web request the ClientInfo: https://github.com/AzureAD/microsoft-identity-web/blob/c5681fa91e444efca09621e1a5a2fadf9cc5dd0a/src/Microsoft.Identity.Web/WebAppExtensions/MicrosoftIdentityWebAppAuthenticationBuilderExtensions.cs#L326, which is the way the disambiguation works for guest accounts.

@bmukes : proposing to move this issue to the sample or Microsoft.Identity.Web if you don't mind.

@bmukes
Copy link
Author

bmukes commented Jan 5, 2021

I am using AAD configured for the OIDC Authorization Code Flow.
I am NOT using AD B2C.
I will try GetMsalAccountId but looking at the source code shows that it is looking for claims {uid}.{utid} and those claims are NOT returned by AAD and if you attempt to customize the access or id token in the AAD application those claims DO NOT show up as available to be returned. This is based on me looking in the portal and the Microsoft documentation
Microsoft identity platform ID tokens
and
Microsoft identity platform access tokens

as far as the claims being returned in my application they are below

image

Ultimately what I am trying to accomplish is outlined below

  • Configure a verified AAD tenant
  • Configure an application for OIDC Authorization code flow
  • Use the IConfidentialClientApplication in MSAL in the override to OpenIdConnectAuthenticationNotifications.AuthorizationCodeReceived to get the id, access, refresh tokens and have the access and refresh tokens cached.
  • Use IConfidentialClientApplication to call GetAccountAsync BUT IT ALWAYS RETURNS NULL
  • Use the result from GetAccountAsync to call AcquireTokenSilent

This seems simple but my debugging shows

  1. IConfidentialClientApplication.AcquireTokenByAuthorizationCode stores different information in the cache for AAD member type Member than for AAD member type Guest
  2. Acquiring a IConfidentialClientApplication instance using the code in your sample results in NO account being returned in GetAccountAsync which will cause AcquireTokenSilent to fail.
  3. I can get the cached access tokens (calling GetAccountAsync does not return null) if I create my IConfidentialClientApplication instance as a singleton and use a user with AAD member type Member but that is not what your sample shows and unless the IConfidentialClientApplication instance is thread safe it's not a good idea.

@bmukes
Copy link
Author

bmukes commented Jan 5, 2021

Also if you want to move this to Microsoft.Identity.Web I do not mind. I just need some way to get the cached access token in a controller or a sample that shows how this can be accomplished.

@bmukes
Copy link
Author

bmukes commented Jan 5, 2021

I am beginning to wonder if the problem is that I am not using the OIDC Hybrid flow and maybe MSAL will not work properly with the OIDC Authorization Code flow. I am basing this on the Microsoft sample
active-directory-dotnet-admin-restricted-scopes-v2-master
In this sample the code instantiates an IConfidentialClientApplication instance using the ClaimsPrincipal in the AuthenticationTicket.Identity property. There would be no value in the AuthenticationTicket.Identity property in the OIDC Authorization Code flow because no id Token is returned in that flow. Any thoughts?

        private async Task OnAuthorizationCodeReceived(AuthorizationCodeReceivedNotification context)
        {
            /*
			 The `MSALPerUserMemoryTokenCache` is created and hooked in the `UserTokenCache` used by `IConfidentialClientApplication`.
			 At this point, if you inspect `ClaimsPrinciple.Current` you will notice that the Identity is still unauthenticated and it has no claims,
			 but `MSALPerUserMemoryTokenCache` needs the claims to work properly. Because of this sync problem, we are using the constructor that
			 receives `ClaimsPrincipal` as argument and we are getting the claims from the object `AuthorizationCodeReceivedNotification context`.
			 This object contains the property `AuthenticationTicket.Identity`, which is a `ClaimsIdentity`, created from the token received from 
			 Azure AD and has a full set of claims.
			 */
            IConfidentialClientApplication confidentialClient = MsalAppBuilder.BuildConfidentialClientApplication(new ClaimsPrincipal(context.AuthenticationTicket.Identity));

            // Upon successful sign in, get & cache a token using MSAL
            AuthenticationResult result = await confidentialClient.AcquireTokenByAuthorizationCode(new[] { "user.readbasic.all" }, context.Code).ExecuteAsync();
        }

@bmukes
Copy link
Author

bmukes commented Jan 5, 2021

Also keep in mind that I am STUCK using .NET Framework 4.7.2 and not .NET Core

@bmukes
Copy link
Author

bmukes commented Jan 5, 2021

I also thought I would include a screen shot of the api permissions for my application

image

@bmukes
Copy link
Author

bmukes commented Jan 5, 2021

OK if a picture is worth a 1,000 words then code is worth 10,000
I have uploaded sample code to the github repository in the link below so that you can see my problems.
To use the code

  • Create an AAD Application in an AAD verified Tenant
  • Add a user of member type Member to the AAD application
  • Add a user of member type Guest to the AAD application. This guest should be from a different verified Tenant
  • Set the AAD application to single tenant
  • Set the redirect uri's in the AAD application to https://localhost:44388/signin-oidc
  • Set a client secret in the in the AAD application
  • Modify the web.config app settings in the ASP.NET code to the AAD client id, client secret, tenant id
  • configure the API permissions in the AAD application as shown in the image below:

image

At this point you should be able to do OIDC Authorization grant with the sample code.
I have added Debug.WriteLine statements in the Startup.Auth.cs (line 95) and HomeController.cs (lines 17 - 27)

What you will observe in your output window.

  • In Startup.Auth.cs you will see that one item is being cached and and you will see the IAccount.HomeAccountId displayed in the output window
  • in HomeController.cs you will see that no IAccount instances will ever be returned.

GitHub of ASP.NET project using MSAL

@bmukes
Copy link
Author

bmukes commented Jan 6, 2021

Last thing to help in testing the application is that I test using Chrome in incognito mode this helps ensure that the session cookie does not get in the way by automatically logging you in and bypassing authentication.

@seannybgoode
Copy link

seannybgoode commented Jan 26, 2022

I'm experiencing a related issue. I don't think this has anything to do with guest accounts, I think there are just some use-cases where GetAccount is returning null. GetAccounts will return a collection with a single account, but GetAccount will return null. Using MSAL 4.40.

I think the bug below has possibly been reintroduced. It should throw an exception rather than inexplicably return null, if this is as-designed behavior.

Possible related to the now closed #2141 issue there.

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

No branches or pull requests

4 participants