Aspnetcore: Creation of an Identity User via External Login doesn't set AuthenticationMethod

[serpent5]

When signing in with an external provider for the first time, the Default UI for Identity doesn't set the ClaimTypes.AuthenticationMethod claim.

In ExternalLoginModel<TUser>.OnPostConfirmationAsync, I think the call to SignInAsync should include an authenticationMethod argument, like this:

await _signInManager.SignInAsync(user, isPersistent: false, info.LoginProvider);

[blowdart]

We've thought about this before, but in reality you're not logging in through the external provider, but you are instead using the external provider to lookup users in your identity database, and then login through identity. It'd be an untruth to flag such logins as coming from Facebook, or Google, because the identity user database overrides, augments and changes any claims from the external provider.

[serpent5]

The second time around, the code succeeds when using ExternalLoginSignInAsync, which just looks up the user from the Identity store and sets the AuthenticationMethod claim to e.g. Google. Is this particularly different to the first-time setup? The database overrides, etc all still apply here, from what I can see.

[blowdart]

Oh that is weird. OK, sounds like a bug, or somewhere we a decision was made and didn't take effect everywhere.

[serpent5]

I'm happy to have a go at a PR for this if it would be helpful. I realise that a decision might need to be made on how to proceed first and that changes would apply to both the V3 and V4 UIs and the scaffolding too. I guess it might be a breaking change for someone that's depending on the claim not being set (or being set if that's what changes).

[blowdart]

Sure, we'd love a PR!

[serpent5]

I’d still like to try and PR this, but I’m having a really hard time trying to build from source, so I’ll come back when I’ve got that going.

https://github.com/aspnet/AspNetCore/issues/12706

[blowdart]

@serpent5 Are you still trying to get this working? We'd like to get this into 3.1

[serpent5]

@blowdart I couldn't ever get past not understanding how to use a locally built version of Microsoft.AspNetCore.Identity.UI so that I could run and test the change. The guidance in Use the result of your build didn't work for me and I didn't get a response on the issue I raised seeking help (which I put down to the team being all kinds of busy or my explanation just being terrible).

[blowdart]

@hoak can you give some guidance here?

---

From: Kirk Larkin [e-mail]
Sent: Wednesday, September 11, 2019 1:20:12 PM
To: aspnet/AspNetCore [e-mail]
Cc: Barry Dorrans [e-mail]; Mention [e-mail]
Subject: Re: [aspnet/AspNetCore] Creation of an Identity User via External Login doesn't set AuthenticationMethod (#10970)

@blowdarthttps://github.com/blowdart I couldn't ever get past not understanding how to use a locally built version of Microsoft.AspNetCore.Identity.UI so that I could run and test the change. The guidance in Use the result of your buildhttps://github.com/aspnet/AspNetCore/blob/master/docs/BuildFromSource.md#use-the-result-of-your-build didn't work for me and I didn't get a response on the issuehttps://github.com/aspnet/AspNetCore/issues/12706 I raised seeking help (which I put down to the team being all kinds of busy or my explanation just being terrible).

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com/aspnet/AspNetCore/issues/10970?email_source=notifications&email_token=AAGCNCVWDORRVIDURIVMR2TQJFHHZA5CNFSM4HVKA27KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6PYRNI#issuecomment-530548917, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AAGCNCSOCBZF6KUSHR6RAFLQJFHHZANCNFSM4HVKA27A.

[serpent5]

@blowdart Thanks, it'd be great to get some help. I guess you meant to tag @HaoK...

[blowdart]

Hah, yes, no autocomplete on email :)

[Pilchie]

Moving to P3, since it's too late for P2. What's the state of this for P3?

[HaoK]

This seems like something safe to move to 5.0 @ajcvickers @blowdart

[ajcvickers]

@HaoK Seems fine to move.

[HaoK]

Thanks again @serpent5 Fixed by https://github.com/dotnet/aspnetcore/pull/18296