Scott Hanselman

IPrincipal (User) ModelBinder in ASP.NET MVC for easier testing

February 6, '09 Comments [17] Posted in ASP.NET | ASP.NET MVC
Sponsored By

ModelBinders are great. I've blogged a bit about them before like the File Upload Model Binder.

I am working on some code like this:

[Authorize]
public ActionResult Edit(int id) {

Dinner dinner = dinnerRepository.FindDinner(id);

if (dinner.HostedBy != User.Identity.Name)
return View("InvalidOwner");

var viewModel = new DinnerFormViewModel {
Dinner = dinner,
Countries = new SelectList(PhoneValidator.Countries, dinner.Country)
};

return View(viewModel);
}

It's pretty straight forward, but this Controller knows too much. It's reaching into implicit parameters. The id was passed in, but the User is actually a property of the Controller base class and ultimately requires an HttpContext. Having this method "know" about the User object, and worse yet, having the User object go reaching into HttpContext.Current makes this hard to test.

I'd like to have the convenience of passing in the User (actually an IPrincipal interface) when I want to test, but when I'm running the app, I'd like to have the IPrincipal get passed into my method automatically. Enter the Model Binder. I need to teach ASP.NET MVC what to do when it sees a type as a parameter.

This quickie model binder is now responsible for one thing - it knows how to reach down into the HttpContext and get the current User (IPrincipal). It has one single responsibility.

public class IPrincipalModelBinder : IModelBinder
{
public object BindModel(ControllerContext controllerContext, ModelBindingContext bindingContext)
{
if (controllerContext == null) {
throw new ArgumentNullException("controllerContext");
}
if (bindingContext == null) {
throw new ArgumentNullException("bindingContext");
}
IPrincipal p = controllerContext.HttpContext.User;
return p;
}
}

Now I can release the Controller from the emotional baggage of knowing too much about the User object. It can just have that passed in automatically by the framework. I just need to register the binder to tell folks about it. I can either do it on a one-off basis and put an attribute on this one method parameter:

public ActionResult Edit(int id,                        
[ModelBinder(typeof(IPrincipalModelBinder))]
IPrincipal user)
{...}

But even better, I can just tell the whole application once in the global.asax:

void Application_Start() {
RegisterRoutes(RouteTable.Routes); //unrelated, don't sweat this line.
ModelBinders.Binders[typeof(IPrincipal)] = new IPrincipalModelBinder();
}

Now that ASP.NET MVC knows what to do when it see an IPrincipal as a method parameter, my method gets nicer.

[Authorize]
public ActionResult Edit(int id, IPrincipal user) {

Dinner dinner = dinnerRepository.FindDinner(id);

if (dinner.HostedBy != user.Identity.Name)
return View("InvalidOwner");

var viewModel = new DinnerFormViewModel {
Dinner = dinner,
Countries = new SelectList(PhoneValidator.Countries, dinner.Country)
};

return View(viewModel);
}

Now I can test my controller more easily by passing in fake users. No need for mocking in this case!

[TestMethod]
public void EditAllowsUsersToEditDinnersTheyOwn()
{
// Arrange
DinnersController controller = new DinnersController(new TestDinnerRespository());

// Act
IPrincipal FakeUser = new GenericPrincipal(new GenericIdentity("Scott","Forms"),null);
ViewResult result = controller.Edit(4, FakeUser) as ViewResult;

// Yada yada yada assert etc etc etc
Assert.IsTrue(result.ViewName != "InvalidOwner");
}

Fun stuff.

UPDATE: Phil had an interesting idea. He said, why not make method overloads, one for testing and one for without. I can see how this might be controversial, but it's very pragmatic.

[Authorize]
public ActionResult Edit(int id)
{
return Edit(id, User); //This one uses HttpContext
}

You'd use this one as before at runtime, and call the overload that takes the IPrincipal explicitly for testing.

Yes, I realize I could use an IoC container for this also.

About Scott

Scott Hanselman is a former professor, former Chief Architect in finance, now speaker, consultant, father, diabetic, and Microsoft employee. He is a failed stand-up comic, a cornrower, and a book author.

facebook twitter subscribe
About   Newsletter
Sponsored By
Hosting By
Dedicated Windows Server Hosting by ORCS Web
Friday, February 06, 2009 8:42:19 PM UTC
Scott:

Good idea, but I think you can go farther. I see the role of the controller as ensuring authorization has occurred and dispatching to the view. But the logic of whom is authorized should go somewhere else. For example, the ability to edit the dinner may be shared by various roles (like an application administrator) and you'd probably rather have that in separate class.
dave
Friday, February 06, 2009 8:44:02 PM UTC
(that last comment was me)
Friday, February 06, 2009 8:53:50 PM UTC
I think this:


public class IPrincipalModelBinder : IModelBinder


Should just be this:


public class PrincipalModelBinder : IModelBinder


It is a class, not an interface.
Eddie
Friday, February 06, 2009 10:10:28 PM UTC
Thanks Scott - exactly what I was looking for last week, when decided to postpone building some tests. Now I can easily wrap them up.

-Dave
Friday, February 06, 2009 10:40:36 PM UTC
> why not make method overloads, one for testing and one for without

Because it would make the design less truthful. Taking this route would merely make it work without regard to making it right.
Saturday, February 07, 2009 12:58:02 AM UTC
Clean and neat soution Scott! ;)

May suggest, though, a better name for the "IPrincipalModelBinder" class? Maybe UserInfoModelBinder or even PrincipalModelBinder. Why? Because the Framework Design Guidelines (Brad Abrams & Kryzstof Cwalina) teaches us to prefix with "I" just interface types not classes.

Thank you :)
Saturday, February 07, 2009 12:59:16 AM UTC
... however I bet you knew some people would jump on this notation anyway :))
Saturday, February 07, 2009 3:35:10 PM UTC
I was wondering about this syntax:

ModelBinders.Binders[typeof(IPrincipal)] = new IPrincipalModelBinder();

Would this be better?

ModelBinders.Register<IPrincipal>(new IPrincipalModelBinder());

Starts to look like IoC (I use AutoFac).
Mike
Saturday, February 07, 2009 4:28:47 PM UTC
FYI, this is method-level dependency injection and you've essentially written your own method-level DI tool :)

ASP.NET MVC has good IoC support (Controller factory), you could just inject the current principal into the c'tor of the controller.

That would probably be the easiest way to do this and works well with the framework.
Saturday, February 07, 2009 4:49:18 PM UTC
For StructureMap, in your configuration:

ForRequestedType<IPrincipal>()
.CacheBy(InstanceScope.Hybrid)
.TheDefault.Is.ConstructedBy(ctx => HttpContext.Current.User);


Then, your Controller would look like:


public MyController : ControllerBase {
private readonly IPrincipal _principal;

public MyController(IPrincipal principal) {
_principal = principal;
}

[Authorize]
public ActionResult Edit(int id) {
...
Saturday, February 07, 2009 8:58:08 PM UTC
I must be missing something here. AFAIK the controller ought to be ignornant.

From what I've read (I don't believe everything that I read), the controller
is supposed to perform like Jason Statham's character Frank Martin in
the 2002 movie "The Transporter" (http://www.rottentomatoes.com/m/transporter/).
i.e., the controller should never know what's in the package.

I admit that I'm in way over my head. Nevertheless, I do not think that Phil should be encouraging
you to overload your real code to separate testing from production. Your unit tests
should be where this is happening. Where necessary, I'd guess you need to be using dependency
injection and mocking in your unit tests and not in your production code.

To the greatest extent possible, it seems to me that the view must be
relegated only to rendering in ignorance while the controller
has the unknowing behaviour of a Frank Martin as servant to the model's
authoritarian role of decision maker.
gerry lowry
Sunday, February 08, 2009 6:13:14 AM UTC
with the RC1, mocking a user reduces to only a couple lines of code.

/// <summary>
/// Helper to set a barebones controller context so that the user identity can resolve for purpose of unit testing controllers.
/// </summary>
/// <param name="c"></param>
public static void SetMockControllerContext(this Controller c)
{
var mockContext = new Mock<ControllerContext>();
// mock an authenticated user
mockContext.ExpectGet(p => p.HttpContext.User.Identity.Name).Returns("test33");
mockContext.ExpectGet(p => p.HttpContext.User.Identity.IsAuthenticated).Returns(true);
c.ControllerContext = mockContext.Object;
}
Shawn
Sunday, February 08, 2009 9:35:03 PM UTC
@Hanselman, you said:
Having this method "know" about the User object, and worse yet, having the User object go reaching into HttpContext.Current makes this hard to test.

What User object uses HttpContext.Current? Only a handful of places in ASP.NET MVC use HttpContext.Current. I think that in this particular case using a mock object framework is just fine, and arguable just as easy. @Shawn's example is pretty darn simple. It's simple to read and simple to understand.

Your example of writing a model binder is very interesting, but I don't think that the current state of writing testable controllers with ASP.NET MVC is quite as bad as you imply.

Regarding Phil's suggestion of adding an overload: I think that in the case of controllers adding method overloads makes things quite messy. ASP.NET MVC doesn't support method overloads unless you add a variety of attributes to them to disambiguate. If you want it to work without attributes you have to make the overload that takes in an IPrincipal internal. That's quite a bit of extra goop for such a trivial scenario.

Also, one last thing: In your unit test sample code the "Act" section should be only one statement. The creation of the FakeUser is part of "Arrange" since it isn't related to the code being tested.

- Eilon
Eilon
Monday, February 09, 2009 2:58:44 AM UTC
Yet another option would be to use a functional approach:

public ActionResult Edit(int id, Func<IPrincipal> getCurrentUser) {
// ...
}
Tuesday, February 10, 2009 5:12:31 AM UTC
What's wrong with this?

public ActionResult Edit(int id, string userName) {
// ...
}
Tuesday, February 10, 2009 10:20:41 PM UTC
Since your action does absolutely nothing with the user, other than authorizing it as the event owner, I wouldn't use a binder or pass it in. Instead, use a custom attribute like [DinnerAuthorization] that does that part separately.

This seems a cleaner approach.
Tuesday, February 17, 2009 7:33:21 AM UTC
Scott,

In your example it seems as if everytime the user wants to edit a dinner it has to be retrieved from the DB (or whatever) first. This makes sense for this example, but one thing I haven't seen yet is a way to preserve the state of the model (repository) between controller calls. Is this so? Is there a way to do this?

Thanks in advance!
Comments are closed.

Disclaimer: The opinions expressed herein are my own personal opinions and do not represent my employer's view in any way.