Refactoring the ASP.NET MVC project template authentication
I know this subject has been touched on before, but I can never find the links when I need them. So I decided I would write up my attempt at creating a better implementation of the MVC project template that ships with ASP.NET MVC Beta. I don’t claim any of these ideas were originally mine, if you know of a link to the original content please let me know and I’ll update this article. Now on with the show.
Lets start by looking at the original Login action method:
1: public ActionResult Login(string username, string password, bool rememberMe, string returnUrl)
2: {
3:
4: ViewData["Title"] = "Login";
5:
6: // Basic parameter validation
7: if(String.IsNullOrEmpty(username))
8: {
9: ModelState.AddModelError("username", "You must specify a username.");
10: }
11: if(String.IsNullOrEmpty(password))
12: {
13: ModelState.AddModelError("password", "You must specify a password.");
14: }
15:
16: if(ViewData.ModelState.IsValid)
17: {
18: // Attempt to login
19: bool loginSuccessful = Provider.ValidateUser(username, password);
20:
21: if(loginSuccessful)
22: {
23: FormsAuth.SetAuthCookie(username, rememberMe);
24: if(!String.IsNullOrEmpty(returnUrl))
25: {
26: return Redirect(returnUrl);
27: }
28: else
29: {
30: return RedirectToAction("Index", "Home");
31: }
32: }
33: else
34: {
35: ModelState.AddModelError("_FORM", "The username or password provided is incorrect.");
36: }
37: }
38:
39: // If we got this far, something failed, redisplay form
40: ViewData["rememberMe"] = rememberMe;
41: return View();
42: }
Whoa, 42 lines just to authenticate a user. Even if i reformatted it to remove whitespace and extra line breaks its would still be 32 lines. We have ViewData, validation, authentication, cookie handling and redirection. I think is safe to say we are violating the single responsibility principle. If we listen to Jeremy Miller and Ayende, and I think we should, the controller should have no knowledge of the view. So line 4 has to go. We’ll add a new ContentPlaceHolder to the head section of the site.master file like this:
1: <head>
2: <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1" />
3: <asp:ContentPlaceHolder ID="HeaderContent" runat="server" />
4: </head>
Now we can set the title and even add css and javascript resources that are needed for each view.
Moving on the validation is simply checking that the form was filled out. To me making a trip to the server to do that is a waste of perfectly good bandwidth. A little bit of javascript can do all of this on the client. For now we’ll leave the client side stuff for another article and strip out the checks in the controller.
Now we are getting to the meat of things. Authenticating the user is responsibility on this action. The controller provides us with a property to access the static System.Web.Security.MembershipProvider class on which we can call ValidateUser and a whole lot of other stuff too. That other stuff is the issue here, we are again violating SRP. Lets start putting together some code to solve the problem.
The LoginController
1: public class LoginController : Controller {
2:
3: public LoginController() : this(new AspAuthenticationProvider()) {}
4:
5: public LoginController(IAuthenticationProvider authenticationProvider) {
6: this.AuthenticationProvider = authenticationProvider;
7: }
8:
9: public IAuthenticationProvider AuthenticationProvider { get; private set; }
10:
11: ...
12: }
The IAuthenticationProvider Interface
1: public interface IAuthenticationProvider {
2: bool ValidateUser(string username, string password);
3: }
The AspAuthenticationProvider
1: public class AspAuthenticationProvider() : IAuthenticationProvider {
2: public bool ValidateUser(string username, string password){
3: return Membership.Provider.ValidateUser(username, password);
4: }
5: }
Now why all the trouble and the extra code? Well because tomorrow you boss is going to walk in and drop the bomb that all user of your system are going to be stored in the ACME User Management System. Now you only have to implement one new class to handle that change. And if you are using and IOC container, (you are using an IOC container arn’t you?), you simply change which class you instantiate and it gets injected. Otherwise you need to update the constructor in the LoginController to new up the AcmeAuthenticationProvider class.
Lets look at how to let the system know that we have an authenticated user. The original calls into FormsAuth.SetAuthCookie(username, rememberMe) This works great right up until you unit test your controller action. Then things go BOOM! The FormsAuth class is a dependency and in this case a dependency we do not control. We could wrap it up like we did with the AuthenticationProvider, but in this case the MVC framework provides us with different solution. The ActionResult.
1: public class FormsAuthenticationResult : ActionResult {
2: public FormsAuthenticationResult(string userName, bool persistentCookie) {
3: UserName = userName;
4: PersistentCookie = persistentCookie;
5: }
6:
7: public string UserName { get; private set; }
8:
9: public bool PersistentCookie { get; private set; }
10:
11: public override void ExecuteResult(ControllerContext context) {
12: FormsAuthentication.SetAuthCookie(UserName, PersistentCookie);
13:
14: response.Redirect(FormsAuthentication.GetRedirectUrl(UserName, PersistentCookie));
15: }
16: }
Now we have all of the parts needed to build the controller action so lets look at the complete method.
1: public ActionResult Login(string username, string password, bool rememberMe) {
2: if (AuthenticationProvider.ValidateUser(username, password)) {
3: return new FormsAuthenticationResult(username, rememberMe);
4: }
5:
6: ModelState.AddModelError("_FORM", "The username or password provided is incorrect.");
7: return View();
8: }
That’s a lot better. The controller now only has one responsibility authenticating the user. All of the other concerns are now separated and handled in there own class.
None of this means that there is no room for improvement. If you see a way to make this cleaner please speak up we’ll all be better off.
Wow! Thank you!
I always wanted to write in my site something like that. Can I take part of your post to my site?
Of course, I will add backlink?
Sincerely, Timur I.
Yes, feel free to quote and reuse.
Nice work – one problem springs to mind… If you remove the validation of values entered to JavaScript then if users have JS turned off this will fail. You need to have validation in both client and server side… what do ya think?
Mike – You are correct, but for a login the validation will happen during authentication anyway. So for this example I didn’t see any threat by eliminating the server side validation. For more advanced data submissions I was planning some posts on validation which should be handled in the model. But with the release of RC1 I need to play around with it first and make sure everything still applies.