dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.6k stars 10.06k forks source link

MicrosoftAccount should use OIDC (was: AddMicrosoftAccount & Azure Active Directory - App registrations signin/signout problems) #10037

Open damienbod opened 5 years ago

damienbod commented 5 years ago

Problem Description:

If a user logs in from an ASP.NET Core application using the AddMicrosoftAccount extension method, it is impossible to change user on the aad, because the app does an auto login if only one aad account is logged in.

If 2 aad accounts are logged in, then it waits, and I can choose. This is correct.

If only 1 aad user is logged in, and if I click the popup window to change an account, I then get a Correlation failed exception (If I’m fast enough to complete the login) in the ASP.NET Core application.

The auto login breaks everything.

a) How can you turn this off? b) How can you logout from the ASP.NET Core application? The signout is not sent to the aad APP Registration. This is possible in the Azure Portal. Maybe I want to logout in the aad using the application.

Code to reproduce:

https://github.com/damienbod/AspNetCoreID4External/blob/master/src/IdentityServerWithAspNetIdentity/Startup.cs#L61-L67

Azure Portal

Used Azure Active Directory / App registrations to configure the client.

Tratcher commented 5 years ago

For AAD you should be using OpenIdConnect rather than Microsoft Account. OIDC has the remote signout capability.

damienbod commented 5 years ago

@Tratcher I use https://apps.dev.microsoft.com which now recommends you do this in the portal as a application personal account. It's not in the aad, but this is how you navigate to it in the portal. A bit confusing I think.

Here's the existing docs which probably should be updated.

https://docs.microsoft.com/en-us/aspnet/core/security/authentication/social/microsoft-logins?view=aspnetcore-2.2

Eilon commented 5 years ago

It seems like the best path forward is to add a new API, such as .AddMicrosoftAccountOidc(...) that uses OIDC (instead of OAuth) to connect Microsoft Account for auth.

Using OIDC will help gain newer features such as what's being discussed in this issue. However, it's a behavioral breaking change, so we should add this via a new API, rather than changing the existing APIs.

Eilon commented 5 years ago

The above is for part (b).

For part (a), you can handle the OnRedirectToAuthorizationEndpoint event and add a prompt=login query string parameter to the RedirectUri to force prompting for accounts.

damienbod commented 5 years ago

Thanks for your replies

I have switched from MS Account extension to OpenID Connect. By using OpenID connect I can send a prompt=login using the property, and this prevents the auto login. I could also do it the way @Eilon suggested, but @Tratcher recommended OIDC as does the Azure docs for this type on APP.

Secondly, I had many problems with Correlation cookie (also for single instance), we solved this by customizing the creation of the cookie and the correlation ID. The default values did not work. The multiple instance was solved by moving the data protection to a cache.

I think the AddMicrosoftAccount should be made obsolete. Maybe the new API is not required, but an OIDC recommendation should be made in the Obsolete text.

Still have problems with one deployment, the user always receives a correlation exception, cookie not found. No idea why this is happening here yet.

Tratcher commented 5 years ago

customizing the creation of the cookie and the correlation ID

You shouldn't need to do that. See https://docs.microsoft.com/en-us/aspnet/core/host-and-deploy/web-farm?view=aspnetcore-2.2#required-configuration for hosting multiple instances of your site.

damienbod commented 5 years ago

@Tratcher thanks for you answer. I had/ have no problems with the multiple instance. The correlation exceptions was caused by time outs, SameSite and cookie not found. The timeouts and the same site cookie problems are fixed.

options.RemoteAuthenticationTimeout = TimeSpan.FromSeconds(30);
options.CorrelationCookie = new Microsoft.AspNetCore.Http.CookieBuilder()
{
    Expiration = new TimeSpan(0, 0, 30),
    SameSite = Microsoft.AspNetCore.Http.SameSiteMode.None,
    IsEssential = true
};

Still have correlation Cookie not Found exceptions by some clients and no idea what causes this.

Greetings Damien

Tratcher commented 5 years ago

@damienbod Don't change the cookie builder. SameSite and IsEssential are the default values. https://github.com/aspnet/AspNetCore/blob/208299aa31ef78de7da8d7bf2b96e8b8d5a98786/src/Security/Authentication/Core/src/RemoteAuthenticationOptions.cs#L26-L33 RemoteAuthenticationTimeout and Expiration do the same thing. https://github.com/aspnet/AspNetCore/blob/d7a7c65b2b40a97bb0ba78c07380c46bc5920400/src/Security/Authentication/Core/src/RemoteAuthenticationOptions.cs#L160-L163

damienbod commented 5 years ago

The correlation Cookie not found exceptions were caused by an anti-virus, firewall setting on the client PCs. Now the external provider is working stable.

I think making AddMicrosoftAccount obsolete would be good. I also think AddOpenIdConnect is enough, and the .AddMicrosoftAccountOidc is not required. This needs documentation.

Thanks for you feedback and answers.

damienbod commented 5 years ago

I made some docs for this, and how I could fix this for me, maybe it will help others.

https://damienbod.com/2019/05/17/updating-microsoft-account-logins-in-asp-net-core-with-openid-connect-and-azure-active-directory/

Tratcher commented 4 years ago

Update: For 6.0 we should obsolete AddMicrosoftAccount and AddGoogle and rewrite the docs to use AddOpenIdConnect.

ericsampson commented 3 years ago

@Tratcher just bumping this as 6.0 is almost upon us.

This issue that got autoclosed/locked still looks valid to me, but I can't comment on it as it's locked for externals: https://github.com/dotnet/aspnetcore/issues/26919

Tratcher commented 3 years ago

26919 was a rather complicated proposal for something that should already be possible with named options. We didn't have any plans to pursue it.

@blowdart looks like this got stuck in backlog. Any ambition to address it for 6.0?

blowdart commented 3 years ago

We aren't going to deprecate in an LTS unfortunately, because that's now a principal. So it'll be 7

Tratcher commented 3 years ago

Should we at least re-write the docs to recommend OIDC?

ericsampson commented 3 years ago

Thanks Chris.

blowdart commented 3 years ago

We should, or IdentityWeb which should work with MSAs