Abstracted account validation#7944
Conversation
|
The only issue I see is that the localization strings will need to be updated, and these are quite used. Maybe just keep the email validation in the controller? It's ugly, but it doesn't break existing users. |
|
I can see the point you are making, but I'd still have validation in a single place, rather than copy-paste it in everyplace I need to use it. It's more maintainable in the long term, I think. On the other hand, I can't think of a realistic scenario where I would need different email validation. |
| using Orchard.Localization; | ||
|
|
||
| namespace Orchard.Security { | ||
| public interface IAccountValidationService : IDependency { |
There was a problem hiding this comment.
Why so many methods and not just one method with a UserVallidationContext ?
You could do validation that needs many of these properties.
There was a problem hiding this comment.
I had thought of that, but decided against it.
The reason was to provide building blocks for a method like that so that with a single implementation of this service it would be possible to build different validation "combinations".
For example, the ValidateRegistration method of AccountController uses these methods, then adds a check for the password confirmation.
Suppose in the same tenant I want to have a newsletter service, that would allowme to send newsletters to people that are not registered on the tenant. The "registration" for a newsletter may skip the password and username validations entirely, but may still check for unicity (I would add this in the method from the controller, like it's done in the AccountController).
If I had a single big method with an implementation doing all the validations, that would be injected in both of those controllers, and then I would require the passed context to have a lot of properties to control the validation steps to perform, and possibly their order.
There was a problem hiding this comment.
I disagree. If you need a newsletter module to create accounts then you should handle it by either using the same default logic that is used for new accounts, or create a custom one.
Also by separating all calls you can't ensure a rule where a password for a user can't be reused. Just an example.
| /// </summary> | ||
| /// <param name="userName">The string we are evaluating as a UserName.</param> | ||
| /// <returns>true if the string is a valid UserName, false otherwise.</returns> | ||
| bool ValidateUserName(string userName); |
There was a problem hiding this comment.
Why have two different methods?
Also an out parameter is not necessary if it a referenced instance like a dictionary. Just pass an existing dictionary, or use an out array.
…ountController, so it's easier to upgrade it and use the same validation elsewhere.
83d669c to
eeded6c
Compare
|
I rebased this on the current dev branch. I think I took care of most of the comments. I still kept three separate methods to validate Password, UserName and Email, rather than a single method to validate the whole thing. My reasoning is as follows:
Don't merge this yet as I update the code based on the comments and suggestions here and what we'll do for #8516 |
|
I think merging #8521 should take priority over this. |
* Added a service to abstract some account validation away from the AccountController, so it's easier to upgrade it and use the same validation elsewhere. * Added a validation context to carry information used for validation of account information. * Refactored password validation in the AccountController * Updated tests * fixed value read from context.ValidationSuccessful
2 participants
Added a service to abstract some account validation away from the AccountController, so it's easier to upgrade it and use the same validation elsewhere.
The idea is to make it easier to use the same validation across all modules when dealing with user registration. On top of that, eventually adding validation policies is easier this way.
The reasoning behind the methods from the interface is to cover the most common "parameters" of user registration (username, password, email). I did not add a
ValidateRegistrationmethod because different systems may have much different requirements there (e.g. we may not want password confirmation, or maybe we are not going to have an input for the username).