4

I have created an ADO.NET model of my database. Created a new controller with CRUD (entity framework and using the ADO.NET entity model I created).

In my database I have a simple Users table. The Password row in the table will hold the users passwords encrypted with SimpleCrypto (PBKDF2).

In my ADO.NET Users.cs model I have added following validation:

[Required]
[DataType(DataType.Password)]
[StringLength(20, MinimumLength = 6)]
[Display(Name = "Password")]
public string Password { get; set; }

That works with jQuery in the browser with validation. But in my controller I am encrypting the Password, and then the Password string will be way more than 20 chars in lenght.

var crypto = new SimpleCrypto.PBKDF2();
var encryptedPass = crypto.Compute(user.Password);

user.Password = encryptedPass;
user.PasswordSalt = crypto.Salt;

_db.Users.Add(user);
_db.SaveChanges();

And this gives me and "Validation failed for one or more entities."-error.

I can copy the user over to a "var newUser" and then set all the properties there, but isn't there a easier way to bypass the model validation in this case?

EDIT: If I remove the validation of the Password prop in the model then everything works. So it is the validation that gives me the error because I alter the Password from 6-20 length chars to +100 lengt chars because of the encryption in the controller.

EDIT: Complete controller section inserted to this question.

[HttpPost]
public ActionResult Create(Users user)
{
    if (!ModelState.IsValid)
    {
        return View();
    }
    if (_db.Users.FirstOrDefault(u => u.Email == user.Email) != null)
    {
        ModelState.AddModelError("", "User already exists in database!");
        return View();
    }

    var crypto = new SimpleCrypto.PBKDF2();
    var encryptedPass = crypto.Compute(user.Password);

    user.Password = encryptedPass;
    user.PasswordSalt = crypto.Salt;

    _db.Users.Add(user);
    _db.SaveChanges();

    return RedirectToAction("Index", "User");
}
John Saunders
  • 160,644
  • 26
  • 247
  • 397
user1281991
  • 763
  • 1
  • 12
  • 34
  • 1
    I have edited your title. Please see, "[Should questions include “tags” in their titles?](http://meta.stackexchange.com/questions/19190/)", where the consensus is "no, they should not". – John Saunders Oct 10 '13 at 09:46

3 Answers3

8

This is where ViewModels enter the game. You should create a model which you pass to the view and map that back to the domain model later.

The ViewModel:

public class RegisterModel
{
    [Required]
    public string UserName { get; set; }

    [Required]
    [DataType(DataType.Password)]
    [StringLength(20, MinimumLength = 6)]
    [Display(Name = "Password")]
    public string Password { get; set; }
}

The domain model (your current User model):

public class User
{
    // other properties..

    [Required]
    public string Password { get; set; }
}

You can use these models in your controller like this:

The GET action:

public ActionResult Register()
{
    var registerModel = new RegisterModel();
    return View(registerModel)
}

With a view like this:

@model RegisterModel

@Html.LabelFor(model => model.UserName)
@Html.TextBoxFor(model => model.UserName)
@Html.ValidationMessageFor(model => model.UserName)

@Html.LabelFor(model => model.Password)
@Html.PasswordFor(model => model.Password)
@Html.ValidationMessageFor(model => model.Password)

And the POST action:

[HttpPost]
public ActionResult Register(RegisterModel registerModel)
{
    // Map RegisterModel to a User model.       
    var user = new User
                   {
                        UserName = registerModel.UserName,
                        Password = registerModel.Password   // Do the hasing here for example.
                    };
    db.Users.Add(user);
    db.SaveChanges();                           
}
Henk Mollema
  • 44,194
  • 12
  • 93
  • 104
  • Thanks for this. Stupid question then. How do I "bind" the RegisterModel to the view and the User model to the controller only? – user1281991 Oct 10 '13 at 08:24
  • 1
    Sorry to hijack this question, but you shouldn't be binding two different models to your view and controller. You'd pass `RegisterModel` to your view and you'd expect `RegisterModel` back - MVC has its own binding handlers for this very reason. I suggest you do some more reading: http://www.asp.net/mvc/tutorials/mvc-music-store/mvc-music-store-part-3 – Paul Aldred-Bann Oct 10 '13 at 08:30
  • @user1281991 I updated my anwer with an example of two controller actions and a view. – Henk Mollema Oct 10 '13 at 09:21
3

You say your password encryption is occurring in the controller. In this case shouldn't you be encrypting after validation? For example:

public ActionResult SomeControllerAction(UserViewModel user)
{
    if (!ModelState.IsValid)
    {
        // at this point the human readable (and assuming < 20 length) password
        // would be getting validated
        return View(user);
    }

    // now when you're writing the record to the DB, encrypt the password
    var crypto = new SimpleCrypto.PBKDF2();
    var encryptedPass = crypto.Compute(user.Password);

    user.Password = encryptedPass;
    user.PasswordSalt = crypto.Salt;

    _db.Users.Add(user);
    _db.SaveChanges();

    // return or redirect to whatever route you need
}

If you wanted to specifically control your validation, then try implementing IValidatableObject on your view model class and perform validation here, instead of via attributes. For example:

public class UserViewModel : IValidatableObject
{
    public string Username { get; set; }
    public string Password { get; set; }

    public IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
    {
        // validate the unencrypted password's length to be < 20
        if (this.Password.Length > 20)
        {
            yield return new ValidationResult("Password too long!");
        }
    }        
}
Paul Aldred-Bann
  • 5,840
  • 4
  • 36
  • 55
  • Hi thanks for this. I am new to MVC. I have pasted in the controller section in my question above. – user1281991 Oct 10 '13 at 08:23
  • @user1281991 no problem, it looks like your validation is failing at the database level - you're encrypting the password to be a length longer than your `User.Password` DB table column can reasonably hold. Is this project code first or database first? – Paul Aldred-Bann Oct 10 '13 at 08:25
  • Can't be the length in DB either that gives the problem. The Password length in DB is set to 300. Tried debugging in Visual Studio and a random password that I entered got encrypted to this: "HS90iFjoivSHcc1D6Ynt6liYKr+VKpcOu1nPwUhe5qPqGbjRUfEzff93VdsicETbFOnNmyaxyc6VrVimiQGNww==" which is like 100 chars I guess? If I remove the validation of the Password in my model then it works? No errors occurs and everything stores fine in the DB – user1281991 Oct 10 '13 at 08:30
2

If i understand correctly you have a database table with a password field.
According to your model this password field is 20 characters long

[StringLength(20, MinimumLength = 6)]

And you want to insert a value greater then 20 characters.
If entity framework did not stop you then you would get a database error.(Entity framework doesn't know there is a mismatch between your data model and the database and it doesn't wanna take the risk of pushing the insert through)

I assume however that you ment to specify a clientSide validation rule on your viewmodel and not a length constraint on the database.
I hope you see why this is a confusing setup.

My advice would be to either split your viewModel and model up so you can post a viewModel with unencrypted password of maxlength 20 that you can convert to a model password with length 100.

If you find that too much hassle you could create an unmapped password property which you set from html when you post it and you convert it to the password property in your controller.
Your class could look like this :

public class RegisterModel
{
    [Required]
    public string UserName { get; set; }

    [Required]
    [NotMapped]
    [StringLength(20, MinimumLength = 6)]
    [Display(Name = "Password")]
    public string PlainTextPassword { get; set; }

    [Required]
    [StringLength(300)]//This is optional
    [DataType(DataType.Password)]
    public string Password { get; set; }
}
Kristof
  • 3,267
  • 1
  • 20
  • 30
  • Yes, you understand me correctly. However, the actual maxlength of the Password field is 300. I guess I need to read about ViewModels. Thank you – user1281991 Oct 10 '13 at 08:40
  • You don't need a viewmodel Per se. The stringLength property triggers both entity framework validation as well as client side validation. So if you say StringLength 20 your clientside valdation triggers as well as an entity framework check when you submit your changes. By using an unmapped property(which EF does not use) you can trigger the clientSide validation on the PlainTextPassword but keep the entity framework binding on Password. You could add Stringlength 300 if you want. – Kristof Oct 10 '13 at 08:43
  • Mixing domain and ViewModels is a bad practice. It might look like a nice and quick solution, but in the end it will cause headaches and domain models full with properties not mapped to the database. Especially when you want to use one domain models on multiple views, but just a tad different. You might want to read [this article](http://www.asp.net/mvc/tutorials/mvc-music-store/mvc-music-store-part-3). – Henk Mollema Oct 10 '13 at 09:26
  • Imho if your model can be reused as your viewmodel you should do it to avoid overhead, copy pasting properties and mapping code. If at some point it is giving you any headache at all you can easily refactor your model into a viewmodel + model. I know some people will disaggree with me on this but i consider ALWAYS going for model + viewmodel a yagni. It should be looked upon case by case. – Kristof Oct 10 '13 at 09:30
  • It never occurred to me that I could exactly use a domain model as ViewModel. I also don't like that fact that attributes like `[Required]` affect both the db generation of EF and the unobtrusive client-side validation. And what if I want to use EF Fluent API and I want to use the `IsOptional()` method in the configuration and the `[Required]` attribute for my validation.. – Henk Mollema Oct 10 '13 at 09:35
  • Actually when displaying simple forms it makes sense that you only need to specify 1 kind of attribute on a required property and that all consumers "listen" to this attribute. But i aggree that it can cause friction(like in this specific case actually). I find the case where your viewmodel NEEDS something and your database doesn't kinda inconsistent but hey it's like i said : if it causes you trouble or headache you should switch to viewmodels + models asap. – Kristof Oct 10 '13 at 09:42
  • One of the advantages of View Models is removing security risks. http://www.codeproject.com/Articles/223547/Three-reasons-to-why-you-should-use-view-models . We have an example here where the Password property could be set by a greedy Model Binder. Perhaps on this occasion you are going to need it... – Colin Oct 10 '13 at 09:47
  • He is going to replace it with the encrypted version so it should be okay security wise. – Kristof Oct 10 '13 at 09:58
  • So we are safe to use the `User` domain model on the "Create" page and the "Change password" page, just be careful not to use it on the "Edit my details" page right? – Colin Oct 10 '13 at 10:13
  • For every page that you make you have to think about security vurnabilities. If he creates an edit my details page he has to either use a Bind(Exclude attribute or write some code or switch to view models. Not saying you don't have a point in the security risk but just because there is a risk doesn't mean you can't use it. – Kristof Oct 10 '13 at 10:51
  • 1
    Agreed. I don't think its bad practise to mix domain and viewmodels, as long as you understand the pitfalls. But for me, as soon as there are less fields on the View than in the Model, or there is a problem like this one - time for a ViewModel – Colin Oct 10 '13 at 12:04