Removes duplicated email validation in user registration.

[gvkries]

Fixes #11765

[dnfadmin]

All CLA requirements met.

[hishamco]

You break the build

[gvkries]

Sorry, one should not send pull requests when in a rush... I've updated the unit test, but it's still breaking the build - don't know why.

[Skrypt]

No, we can't assume that the View that is posting to that form is using that ViewModel. It is important that the controller makes a double verification to make sure it is a valid email.

[gvkries]

Sorry, but that statement isn't correct, validation takes place in the controller. It does it twice!

[Skrypt]

It is fine to validate in the Controller. The ViewModel validation is just for that View. What if you use a different form to post to that controller action and that you don't validate the email?

[gvkries]

The issue is that the controller does the normal model validation with TryValidateModel but does also add the same error to the model state manually again. This causes two equal validation messages on the view.

[Skrypt]

Add validation on the ViewModel itself to make sure it is never NULL on that controller action before doing anything else. That might fix the issue I was referring to... It is not given that the ViewModel databinding will not fail.

[gvkries]

Maybe I'm not getting your point, but it's already doing all of that in

OrchardCore/src/OrchardCore.Modules/OrchardCore.Users/ViewModels/RegisterViewModel.cs

Line 31 in 40d3f65

if (string.IsNullOrWhiteSpace(Email))

It doesn't make sense to redo the validation a second time IMHO.

[Skrypt]

Here, when we call the action :

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

Lines 64 to 67 in cf16293

	public async Task<IActionResult> Register(RegisterViewModel model, string returnUrl = null)
	{
	var settings = (await _siteService.GetSiteSettingsAsync()).As<RegistrationSettings>();

The RegisterViewModel model is passed to the action by doing data binding magical stuff. It is not given that the model here will not fail data binding if we are using a RazorView that has an AlternateRegistrationViewModel that has a completely different structure. The model will then be "null" in that case. So, we should drop the request.

So basically, make sure that the model passed is never null.

[gvkries]

That's the normal way it is handled everywhere in asp.net and I didn't change that here. If model is null, an ArgumentNullException will be thrown by TryValidateModel now, before it would be an NRE by accessing its properties. If you override RegisterViewModel with your own type and remove the validation, you are also going to fail in other places, e.g. this.RegisterUser(), which also doesn't recheck every property.

[Skrypt]

We need to try to make sure. Because else keeping the validation in the controller makes sense. I don't want to rely on an NRE to say that it is safe.

[gvkries]

This is what model binding and model validation is build for, no need to reinvent the wheel here in a single instance.

[Skrypt]

Well for this PR I assume that it is ok. I don't mind. if you omit to send only an email param from an AJAX POST request then it should throw an NRE at line 73. We could instead on the first line do a redirect with a notification message if model == null.
But I guess this should be something handled by the Validator at that point.

Need to try I guess.

[Skrypt]

By the way, if the request throws an NRE from an AJAX request it can be used for a DDOS attack. So while we have the Antiforgery Token I feel less concerned but we should probably still take some time to think about potential security issues on known routes like these.

[gvkries]

I don't think there is a problem in this anyway, validation will always take place before the controller is executed. And if I remember correctly, polymorphic model binding is also not possible by default in Asp.net core, so it will never reach line 73 without an email address. This might be different in Orchard, but that would surprise me.

[Skrypt]

It happens quite often when I'm using ApiControllers that the model binding is not working. I don't want to assume that it works without throwing an NRE which is what I will test. Anyways let's not digress to other topics. Thanks for your contribution. ❤️