Implement a default service to link local user accounts with external ones during the registration process

[PiemP]

feat(Users): changed account controller code to use new services
feat(Users): load all services in reversed enum to ret last registered services
feat(Users): updated view code

resolve #16026

[PiemP]

I have some issue to test this changes ... I have my orchardcore project that could be useful to test this but I have to use the orchardcore code from this PR inside my solution... or I link all the source code on my PC to the solution but I have a lot of modules and is a long work or I create a nuget package to load in my solution... there is some best practice about it? Thank You.

[PiemP]

ok I have tested the code changes and seems work.

[MikeAlhayek]

Please also add a new line at the end of each new file you introduced.

[PiemP]

Please also add a new line at the end of each new file you introduced.

fixed in bd01473

[MikeAlhayek]

@PiemP I pushed a direct comment to your branch with some cleanup to your code. I did the change online so I hope I broke nothing.

Here are some suggestions for you to consider.

[MikeAlhayek]

@PiemP I did break the build. Please also move the class DefaultExternalLoginUserToRelateFinder to OrchardCore.Users.Core project and that would fix the build.

[MikeAlhayek]

@PiemP please request my review once you fix the build error and address my other comments.

[Piedone]

Only this should be in the if. Otherwise, it would require a unique e-mail even for accounts found by FindByLoginAsync() above.

[PiemP]

I have no problem to make the change but if FindByLoginAsync() find a User, this code can't be execute.

OrchardCore/src/OrchardCore.Modules/OrchardCore.Users/Controllers/AccountController.cs

Lines 382 to 403 in 3853b97

	var iUser = await _userManager.FindByLoginAsync(info.LoginProvider, info.ProviderKey);
	CopyTempDataErrorsToModelState();
	if (iUser != null)
	{
	if (!await AddConfirmEmailErrorAsync(iUser) && !AddUserEnabledError(iUser))
	{
	await _accountEvents.InvokeAsync((e, user, modelState) => e.LoggingInAsync(user.UserName, (key, message) => modelState.AddModelError(key, message)), iUser, ModelState, _logger);
	var signInResult = await ExternalLoginSignInAsync(iUser, info);
	if (signInResult.Succeeded)
	{
	return await LoggedInActionResultAsync(iUser, returnUrl, info);
	}
	else
	{
	ModelState.AddModelError(string.Empty, S["Invalid login attempt."]);
	}
	}
	}
	else

fixed in f2faf17

[Piedone]

I'm not sure I understand. If FindByLoginAsync() finds a user, then the code block you show will execute, and rightfully so. In your case, that would mean that their PLUTO ID matched, what means that they're definitely the same user. In your use case, it won't find a user when you want the second account with the same e-mail to log in. And you'll need to configure RequireUniqueEmail as false too.

[Piedone]

Anybody with any further remarks? Otherwise, I'll merge this on Tuesday.

[hishamco]

Suggested change

	_identityOptions = identityOptions.Value;
	_identityOptions = identityOptions.Value;

[PiemP]

Sorry guys,

I have remember why I have set up the PR with the services IExternalLoginMapperand, finally, I have understand why exist the "second check" done on FindByEmailAsync and what really do FindByLoginAsync.

EXAMPLE CASE 1 (unwanted link user-login data request):
external login provider PLUTO, PLUTO identifier ID

USER A (registered in orchard by provider PLUTO)
email: [email protected]
ID: 1

USER B (not registered in orchard, authentication with provider PLUTO)
email: [email protected]
ID: 2

EXAMPLE CASE 2 (registered in local, but not with PLUTO, no link operation fired):
external login provider PLUTO, PLUTO identifier ID

USER A (registered in orchard by local registration)
email: [email protected]
ID: 1

USER A (authentication with provider PLUTO)
email: [email protected]
ID: 1

---> FindByLoginAsync doesn't find USER A, because doesn't exist a user already authenticated with PLUTO and with ProviderKey 1 in UserByLoginInfoIndex, and FindByEmailAsync doesn't find USER A, because the login data email is different from the registered ones. The result is that link process to relate a local registered user to the login data doesn't happen (or happen in wrong cases like the first example).

The "second check" ratio is to relate a the login data to a local user but if a login provider have a different identifier it can't happen in the right way. I have create IExternalLoginMapper to allow each login provider to implement it's way to do it.

Sorry. I have made you to lose a lot of your time.

[Piedone]

Example 1 is now covered.

Example 2 then is the inverse: it's not that two different user accounts have the same email, but the same user account has two emails. This was covered previously too: you need to save a ProviderKey at one point for the user, so it'll be found by FindByLoginAsync during Pluto login. Or, you can override FindByLoginAsync as mentioned previously.

[Piedone]

So, I don't think anything else is needed here and this can be merged. @hishamco you had the latest remarks, please merge when you're OK with it.

[PiemP]

Example 2 then is the inverse: it's not that two different user accounts have the same email, but the same user account has two emails. This was covered previously too: you need to save a ProviderKey at one point for the user, so it'll be found by FindByLoginAsync during Pluto login. Or, you can override FindByLoginAsync as mentioned previously.

FindByEmailAsync is the real point where a local user could be related to a new external login provider. But if the check is done over something (email) that is not an identifier for the external login provider, the check doesn't work as expected.

FindByLoginAsync simply check if exist a user that have previosly registered an external login system by verifiy if exist a record with the name of the login provider and the user identifier for the login provider. In the EXAMPLE 2, it try to read a record related to PLUTO external login provider and a user identified by 1.

In the EXAMPLE 2, USER A is created locally by an administrator and doesn't have the record that say that is authenticated with PLUTO. FindByEmailAsync doesn't find a valid user (because the login data email is not registered) and this cause the call of RegisterExternalLogin that goes wrong because already exist a user with the same username (the error about username is not a problem, the problem is about the request to create a new user when I already have an user for this ID). To record this external login provider for USER A, I have to link USER A to PLUTO login provider by LinkExternalLogin. To do this I have to match a local user with the login provider identifier and not with email.

If I override FindByLoginAsync, I can only force the system to return a local user. But I want to remain in the standard OrchardCore procedure. I want to use LinkExternalLogin to relate existing user with login data that coming from PLUTO.

FindByEmailAsync is the crucial point... where a local user could be related to a new external login provider but it force to do a check over the email and could cause a wrong match between login data and local user. I have to do this check with the external login provider identifier (that commonly is email, but not in my case).

[Piedone]

At the end of the day, OC needs to be able to tell if a local user related to the external one exists. In your Example 2 it can't do this, because this information doesn't exist. Adding an extension point doesn't help with this.

However, you can solve this, as mentioned above, by saving a ProviderKey for the user before their first login. OC doesn't have a UI for this, you need to code this (you can extend the user management UI, then you can e.g. implement IUserEventHandler). Since FindByLoginAsync() runs before FindByEmailAsync(), the e-mail not matching doesn't matter, you just have to save a correct ProviderKey. No need to override anything in this case.

As far as I understand, after this PR, both of your use cases are covered.

[hishamco]

@PiemP I will merge this, please consider Zoltan's last comment