AzureAD / microsoft-identity-web

Helps creating protected web apps and web APIs with Microsoft identity platform and Azure AD B2C
MIT License
672 stars 208 forks source link

Should clear session auth cookie if cache is missing account #13

Open jennyf19 opened 4 years ago

jennyf19 commented 4 years ago

from @onovotny and copied from https://github.com/Azure-Samples/active-directory-aspnetcore-webapp-openidconnect-v2/issues/240

In the Microsoft.Identity.Web library, the system should automatically clear (RejectPrincipal()) the auth cookie if the corresponding account entry is missing from the token cache. This can happen if the cache expired, if it was a memory cache and the server bounced, etc.

The issue is that the system is now in an inconsistent state, where the user is considered logged-in, but any operations to call API's won't succeed because there's no token cache, no refresh token, etc.

I've worked around this here: https://github.com/dotnet-foundation/membership

In order to do so, I had to make some changes to ITokenAquisition and the MsalAbstractCacheProvider's GetCacheKey method.

ITokenAcquisition needed a method to check for a user's token cache: https://github.com/dotnet-foundation/membership/blob/97b75e30e50aab76bfa5a21f1ab88bf31ae66da4/Microsoft.Identity.Web/TokenAcquisition.cs#L406-L426

In there, it takes the CookieValidatePrincipalContext to get the incoming ClaimsPrincpal as HtttpContext.User is not yet set at that point. It stores it in the HttpContext.Items via the StoreCookieValidatePrincipalContext extension method (used later by the GetCacheKey method so it can derive the appropriate key): https://github.com/dotnet-foundation/membership/blob/97b75e30e50aab76bfa5a21f1ab88bf31ae66da4/Microsoft.Identity.Web/TokenCacheProviders/MsalAbstractTokenCacheProvider.cs#L68-L69

Finally, the CookieAuthenticationOptions needs to be configured to check for and reject the incoming principal (this could/should be moved into the main IdentityPlatform AddMsal extension methods): https://github.com/dotnet-foundation/membership/blob/97b75e30e50aab76bfa5a21f1ab88bf31ae66da4/Membership/Startup.cs#L110-L123

I can submit these changes as PR if you're in agreement with these changes.

DerekFoulk commented 2 years ago

Thank you @jennyf19, I always feel like a whiner on here lol

DerekFoulk commented 2 years ago

@jmprieur I actually used File | New (instead of dotnet) to create both the Blazor Server App and ASP.NET Core Web API projects, using the Connected Services window to setup Azure AD B2C. After I got both projects created, calls to the ASP.NET Core Web API didn't authenticate properly, if I remember correctly. I had to add these properties to the appsettings.json file to get things working as the VS template didn't include them:

I also had to blow away the auto-generated App Registrations in the Azure Portal and redo them step-by-step using the examples in https://github.com/Azure-Samples/ms-identity-blazor-server/blob/main/WebApp-OIDC/B2C/README.md as a guide on how to configure them properly. I never did figure out what was configured incorrectly in the auto-generated app registrations, I just know that after I deleted them and created new ones that mirror the repo in the link above- things started working properly.

Just try to create a Blazor Server App and an ASP.NET Core Web API project, using the VS2022 interface to create the projects and configure Azure AD B2C to secure the Web API calls between the two projects and you'll see what I mean. Something is not right when you use this flow- the auth doesn't work. I wish I had more time available to provide a repro, but I have a couple family emergencies going on at the moment.

Thanks for listening

jmprieur commented 2 years ago

Thanks @DerekFoulk Yes, I've noticed the B2C experience in Visual Studio is broken. I passed the information to the VS team.

For the moment, I'd recommend you use the msidentity-app-sync dotnet tool. See https://github.com/AzureAD/microsoft-identity-web/tree/master/tools/app-provisioning-tool

codepwner commented 2 years ago

This issue stole 3-4 days of my week with great confusion and frustration. Please ensure it gets fixed!

lucmoco commented 2 years ago

@jmprieur, I've implemented your work-around, which works great. However, I see two problems with this solution:

In order to avoid these problems, I came up with a different solution:

I share my code in case someone is interested:

public class RejectSessionCookieWhenAccountNotInCacheEvents : CookieAuthenticationEvents
{
    public async override Task ValidatePrincipal(CookieValidatePrincipalContext context)
    {
        var msalTokenCacheProvider = context.HttpContext.RequestServices.GetRequiredService<IMsalTokenCacheProvider>();
        var configuration = context.HttpContext.RequestServices.GetRequiredService<IConfiguration>();

        /* Build a confidential application to be able to access the token cache. */

        ConfidentialClientApplicationOptions options = new();
        configuration.GetSection(Constants.AzureAd).Bind(options);

        var app = ConfidentialClientApplicationBuilder
            .CreateWithApplicationOptions(options)
            .Build();

        /* Check in the cache if the current user is present. */

        msalTokenCacheProvider.Initialize(app.UserTokenCache);
        var accountId = (context.Principal ?? throw new InvalidOperationException("Missing cookie principal")).GetMsalAccountId();
        var account = await app.GetAccountAsync(accountId);
        if (account == null)
        {
            /* The current user is not present in the token cache, and needs to authenticate again in order to get security tokens. */

            context.RejectPrincipal();
        }
    }
}
MauRiEEZZZ commented 2 years ago

@jmprieur is it correct to assume this scenario, on Blazor Server, should be resolved by catching the exception and let ConsentHandler.HandleException() resolve the situation?

And is there a way to implement the [AuthorizeForScopes(Scopes = new[] { "scopes" } ] attribute in Blazor pages? This solution would still throw the exception on GetAccessTokenForUserAsync right?

jmprieur commented 2 years ago

Yes @MauRiEEZZZ

The attribute works on MVC controllers, MVC actions, Razor pages and Razor actions. I have not tested on Blazor pages recently. It did not work in Net 5.0,

Did you try?

MauRiEEZZZ commented 2 years ago

Yes @MauRiEEZZZ

The attribute works on MVC controllers, MVC actions, Razor pages and Razor actions. I have not tested on Blazor pages recently. It did not work in Net 5.0,

Did you try?

I will build an example that uses the Web Api from https://github.com/Azure-Samples/active-directory-dotnet-native-aspnetcore-v2 and report the behaviour.

absolutebandit commented 1 year ago

Is there any update or solution for the "Multiple access tokens found for matching authority, client_id, user and scopes" issue mentioned in the comments below?

https://github.com/AzureAD/microsoft-identity-web/issues/13#issuecomment-1090057855 https://github.com/AzureAD/microsoft-identity-web/issues/13#issuecomment-1097672455

Bidthedog commented 1 year ago

I believe this is the issue I'm facing in my app, related to the above #2185. Can someone confirm, and provide an update as to if / when this will be fixed? It is a significant issue for some fairly high profile clients with tens of thousands of users.

HerrJanker commented 2 months ago

Are there any updates? I'm still facing this issue.

JVita-Code commented 2 months ago

Not sure if this belongs here, I reached this issue by googling, I speak Spanish and this is the first time ever writing here.

please let me know if this could be a work around for the issue, trying to implement this for my MVC .NET Framework 4.7.2 Web App with Microsoft.Identity.Web and Microsoft Graph SDK:

app will try to get the token because the auth claims are still there (hence why user is still logged in), yet this will fail.

public async Task<ActionResult> Index()
{
    try
    {
        //Lets say user already logged in, authenticated = true

        var isAuthenticatedInMicrosoft= ComunesModel.ObtenerEstaAutenticadoEnMicrosoft();
        var homeVM = await HomeModel.CargarHomeViewModel(GraphClient, isAuthenticatedInMicrosoft);

        return View(homeVM);
    }
    catch (Exception ex)
    {

       //Deleted token registry manually in order to trigger ex
        var exception = ex.InnerException as MsalUiRequiredException;

        if (exception != null && exception .ErrorCode.Contains(MsalError.UserNullError))
        {
            //Removing claims from auth ticket
            HttpContext.GetOwinContext().Response.Cookies.Delete("EntraCookie");

           //CargarHomeViewModel (has default bool value false)
           //bool is false because the user is not authenticated anymore.
            var homeVmSinAutenticacion = await HomeModel.CargarHomeViewModel(GraphClient);

            return View(homeVmSinAutenticacion);
        }

        var homeVM = await HomeModel.CargarHomeViewModel(GraphClient);
        return View(homeVM);
    }         
}
internal static bool ObtenerEstaAutenticadoEnMicrosoft()
{
    var authenticated = ClaimsPrincipal.Current.Identities.Any(i => i.AuthenticationType == "Cookies");

    return authenticated;
}
edahlberg commented 1 month ago

This MsalClientException: The cache contains multiple tokens satisfying the requirements. Try to clear token cache issue happens because TokenAcquisition.cs requests and filters tokens for any and all scopes supplied EXCEPT _scopesRequestedByMsal ["openid", "profile", "offline_access"]. That means requesting any of those scopes is not safe and will break things. A separate issue could be filed for that.

Specifically, this exception happens because the "openid" scope is filtered out resulting in filtering for "no scope" in the cache. And that means it will return any and all tokens for that user, because every token matches those requirements. Thus this exception.

One workaround for this is that you change "openid" to "User.Read" (or some other scope) and add that scope to the application in Azure.

bgavrilMS commented 1 month ago

@edahlberg - MSAL should have logic to always disambiguate tokens. That exception is always a bug. If you can provide a repro, we will aim to fix it.

DerekFoulk commented 1 month ago

Thanks @DerekFoulk for this feedback. Did you create your app from the File | New project experience?

Holy necromancer- I'm 2 years late to respond! I'm sure that I did. I almost never use the cli to create projects/solutions. After File > New, I most likely jumped straight into the docs and followed them to a "T".

edahlberg commented 3 weeks ago

@bgavrilMS I created #2998 as it's not strictly related to this issue