Which part of an application should be responsible for hashing a user's password?

Question

Hi,

I'm writing an ASP.NET MVC application which will provide user registration functionality but i am not sure which part of the application (e.g. User Domain model object, Controller, ViewModelMappers) should be responsible for hashing the user's password. I have a registration page that uses a strongly typed ViewModel and a Register action in my UserController as shown:

[AcceptVerbs(HttpVerbs.Post)]
public ActionResult Register(UserRegisterViewModel userRegisterViewModel)
{
    var user = userViewModelMapper.Map(userRegisterViewModel);
    INotification validationResult = user.ValidateForRegistration(userRepository);

    if (!validationResult.HasErrors)
    {
        user.HashPassword();
        userRepository.AddOrUpdate(user); // i'm using NHibernate
        return View("RegistrationAcknowledgement");
    }

    foreach (IError error in validationResult.Errors)
        ModelState.AddModelError(error.Property, error.Message);

    ViewData["country"] = new SelectList(countryRepository.GetAll(), "Code", "Name", userRegisterViewModel.Country);
    return View("RegistrationForm", userRegisterViewModel);
}

User objects are composed in part by LoginDetail objects as shown and to avoid exposing the internals of the User object beyond what is absolutely required the Password Property is read-only. So i cannot for example do user.LoginDetails.Password = hashedandSaltedPassword;

namespace XXXX.Core.Model
{
    public class User
    {
        private LoginDetails loginDetails;

        public virtual LoginDetails LoginDetails
        {
            get { return loginDetails; }
            private set { loginDetails = value; }
        }

        public virtual void AssignLoginDetails(LoginDetails loginDetails)
        {
            this.loginDetails = loginDetails;
        }

        public virtual void HashPassword()
        {
            IHashGenerator hashGenerator = new HashGenerator(new SaltGenerator());
            IHashResult hashResult = hashGenerator.GenerateHash(loginDetails.Password, HashAlgoritm.SHA512);
            loginDetails.Password = String.Concat(hashResult.HashValue, hashResult.Salt);
        }
    }
}

namespace XXXX.Core.Model
{
    public class LoginDetails
    {
        private string username;
        private string password;
        private string confirmPassword;
        private string passwordReminder;
        private bool changePassword;

        // Properties

        #region Constructors

        ...

        public LoginDetails(string username, string password, string confirmPassword, string passwordReminder, bool changePassword)
        {
            this.username = username;
            this.password = password;
            this.confirmPassword = confirmPassword;
            this.passwordReminder = passwordReminder;
            this.changePassword = changePassword;
        }
    }
}

Currently the responsibility for hashing the password is owned the User (by means of the HashPassword method) but

1. Is this a correct responsibility for the User to have (within the context of DDD and Single Responsibility principle)

2. If not, where should this operation reside?

3. If so, should it be called from the controller as i am doing?

Thanks

Answer 1

Without reading your code, I would argue hashing the password should be done in the model so it can be reused outside of the MVC framework. This tends to be true in all MVC frameworks that are implemented in languages general enough to be useful outside of the web.

Answer 2

Let's take a step back and look at the broader picture: when do we want to take a password in clear and hash it?

1. when the user is creating or changing their password, and we need to store it
2. when the user is logging in, and we need to compare the entered password with the the stored one

Currently your implementation addresses only the first instance. So you need a method which accepts a clear password and returns a hashed one.

As for where that method should go ...

The Single Responsibility Principle does not mean that a class does literally one thing. It means that the class handles only things which are clearly within its remit.

So, consider is the relationship between User and hashed password. Can you have a User wthout a hashed password? Will you ever want to work with a hashed password without its User? Do you have other objects which have a hashed password besides User? If the answer to those questions is "No" then I would argue that the password hashing method clearly belongs to the User class, and indeed increases its cohesiveness.