Consider making SignInAsync throw for an unauthenticated identity

[rynowak]

Yeah, I'll admit it. 😢 I burned 1.5 hours trying to use this API and not knowing why it didn't work.

The difference here is between:

var identity = new ClaimsIdentity(new[] { new Claim("ClaimA", "Value") });
await HttpContext.SignInAsync(scheme: null, new ClaimsPrincipal(identity));

var identity = new ClaimsIdentity(new[] { new Claim("ClaimA", "Value") }, CookieAuthenticationDefaults.AuthenticationScheme);
await HttpContext.SignInAsync(scheme: null, new ClaimsPrincipal(identity));

The shorter overload of ClaimsIdentity does the wrong thing for most cases. I'm also not sure if we should be validating whether scheme and the authentication scheme passed to SignInAsync should match.

---

Since there are use cases for SignInAsync with an unauthenticated identity, consider adding more parameters, or using a different method name for that case. Both of these can communicate that you're doing something non-default.

/cc @HaoK @blowdart

[blowdart]

This does bite the unwary, especially the principal that isn't signed in.

Extra validation wouldn't hurt, with an opt-out for people that have valid reasons for not creating an authenticated principal.

Also tagging @Tratcher

[Tratcher]

What happens to the unauthenticated identity? Does it round trip the empty auth type and IsAuthenticated fails on the next request? That's an easy check to add in CookieAuth.

What's the scenario for using SignInAsync with unauthenticated identities?

Note we explicitly do not require the auth types to match, that's why they're separate fields. There was too much confusion when you'd take an OAuth identity, sign in as cookies, etc..

[rynowak]

What happens to the unauthenticated identity? Does it round trip the empty auth type and IsAuthenticated fails on the next request? That's an easy check to add in CookieAuth.

Yes, this is what I saw.

What's the scenario for using SignInAsync with unauthenticated identities?

@blowdart said this is used when people do things like associate a user with an IP address. He can probably provide more detail than I could.

[blowdart]

Well i was stretching. I'm sure someone, somewhere does this, and we'll break them without a way to turn the fix off.

[Tratcher]

Sure, but we're just asking them to set the auth type field. They can set to anything they want, and it would only checked in CookieAuth.

[HaoK]

We can just add an option to turn off requireAuthenticatedIdentity or something to cookie options doesn’t seem too unreasonable to me. I got hit with this years ago too it’s pretty subtle.

…

On Apr 10, 2019, at 2:45 PM, Chris Ross ***@***.***> wrote: Sure, but we're just asking them to set the auth type field. They can set to anything they want, and it would only checked in CookieAuth. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[HaoK]

Announcement for breaking change posted here: #10718

[HaoK]

Created announcement in the right place this time aspnet/Announcements#361

[poke]

This has bitten me in the past too, so thank you for this change! The authentication type is just too invisible for this (and it is somewhat cryptic that this is what IsAuthenticated requires).