Skip to content

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

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

Open
damienbod opened this issue May 7, 2019 · 17 comments
Labels
affected-medium This issue impacts approximately half of our customers area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer enhancement This issue represents an ask for new feature or an enhancement to an existing one severity-minor This label is used by an internal tool
Milestone

Comments

@damienbod
Copy link
Contributor

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.

@Eilon Eilon added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label May 7, 2019
@Tratcher
Copy link
Member

Tratcher commented May 7, 2019

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

@damienbod
Copy link
Contributor Author

@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 Eilon added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label May 9, 2019
@Eilon Eilon added this to the Backlog milestone May 9, 2019
@Eilon Eilon changed the title AddMicrosoftAccount & Azure Active Directory - App registrations signin/signout problems MicrosoftAccount should use OIDC (was: AddMicrosoftAccount & Azure Active Directory - App registrations signin/signout problems) May 9, 2019
@Eilon
Copy link
Contributor

Eilon commented May 9, 2019

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
Copy link
Contributor

Eilon commented May 9, 2019

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
Copy link
Contributor Author

damienbod commented May 10, 2019

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
Copy link
Member

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
Copy link
Contributor Author

damienbod commented May 12, 2019

@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
Copy link
Member

@damienbod
Copy link
Contributor Author

damienbod commented May 13, 2019

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
Copy link
Contributor Author

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
Copy link
Member

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

@ericsampson
Copy link

@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:
#26919

@Tratcher
Copy link
Member

#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
Copy link
Contributor

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

@Tratcher
Copy link
Member

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

@ericsampson
Copy link

Thanks Chris.

@blowdart
Copy link
Contributor

We should, or IdentityWeb which should work with MSAs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected-medium This issue impacts approximately half of our customers area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer enhancement This issue represents an ask for new feature or an enhancement to an existing one severity-minor This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests

5 participants