11

I have found out that I need the current logged in user data in nearly every class (controllers, view, HTML helpers, services and so on). So I thought about to create an "Ambient Context" instead of injecting an IUserService or the User directly.

My approach looks something like that.

public class Bootstrapper
{
    public void Boot()
    {
        var container = new Container();
        // the call to IUserService.GetUser is cached per Http request
        // by using a dynamic proxy caching mechanism, that also handles cases where we want to 
        // invalidate a cache within an Http request
        UserContext.ConfigureUser = container.GetInstance<IUserService>().GetUser;
    }
}

public interface IUserService
{
    User GetUser();
}

public class User
{
    string Name { get; set; }
}

public class UserContext : AbstractFactoryBase<User>
{
    public static Func<User> ConfigureUser = NotConfigured;

    public static User ActiveUser { get { return ConfigureUser(); } }
}

public class AbstractFactoryBase<T>
{
    protected static T NotConfigured()
    {
        throw new Exception(String.Format("{0} is not configured", typeof(T).Name));
    }
}

Example usage:

public class Controller
{
     public ActionResult Index()
     {
         var activeUser = UserContext.ActiveUser;
         return View();
     }
}

Is my approach correct or do I missing something? Do you have better solutions in mind?

UPDATE:

More Detail of the User class:

public class User
{
   string Name { get; set; }
   bool IsSuperUser { get; set;}
   IEnumerable<AzManOperation> Operations { get; set}
}

In Controllers we need to check if an User is a SuperUser to only provide the SuperUser some extra functionality.

public class BaseController : Controller
{
    private readonly IUserService _userService;

    BaseControler(IUserService userService)
    {
        _userService = userService
    }

    public User ActiveUser
    {
        get { return _userService.GetUser(); }
    }
}

In Views we check Operations to only show an edit or delete button if the user has the right to do so. A view never uses the DependencyResolver, but ViewBag or ViewModel. My idea here is to implementing a custom ViewBasePage and providing an ActiveUser property, so that Views have an easy accesss.

In HtmlHelpers we render controls depending on IsSuperUser and Operations (passing in the User object or using DependencyResolver).

In Service Classes we need those properties too. For instance to decide if a basket is valid or not (check if the User is allowed to buy articles that are not in a standard list). So the Service class depends on IUserService and calling GetUser().

In Action Filters to force the user to change his password (only if it is not a SuperUser and User.ForcePasswordChange is true). Here we use the DependencyResolver.

My wish is to have a more easily way to get the User object, instead of using DependencyResolver.Current.GetService().GetUser() or using things like ViewBag.ActiveUser = User. The User object is an object that is almost everywhere needed to check permissions or the like.

Rookian
  • 19,841
  • 28
  • 110
  • 180
  • 3
    I'd look at all my alternatives before going with Ambient Context. See the excellent book [Dependency Injection in .NET](http://manning.com/seemann/). – TrueWill Jun 09 '13 at 16:05
  • 1
    I already own the book. As I mentioned, I need the user almost everywhere in the application, so I thought it might be ok to start using an Ambient Context, instead of using constructor injection or service location. – Rookian Jun 09 '13 at 16:13
  • Maybe you should user `Thread.CurrentPrincipal` or `HttpContext.Current.User` – Sławomir Rosiek Jun 09 '13 at 16:45
  • No, the User object is a custom class and not an IPrincipal. – Rookian Jun 09 '13 at 16:54
  • Could you show your composition root? Is it in application start? In this case you have a problem with concurrency. – Kirill Bestemyanov Jun 09 '13 at 17:06
  • And your ambient context should not throw exceptions when not initialized (there should be good local default for this dependency) – Kirill Bestemyanov Jun 09 '13 at 17:13
  • @KirillBestemyanov Yes it is in the application start. Why is it a problem? I mean the user data is stored in the HttpContect.Current.Items ... – Rookian Jun 09 '13 at 17:40
  • Could you show how you set it there? – Kirill Bestemyanov Jun 09 '13 at 17:51
  • @KirillBestemyanov Do you mean the caching mechanism? – Rookian Jun 09 '13 at 17:57
  • 3
    If you "need the current logged in user data in nearly every class", you may be looking at a a Cross-Cutting Concern instead. Have you considered a Decorator, Dynamic Interceptor, or Filter instead? – Mark Seemann Jun 09 '13 at 18:44
  • @MarkSeemann No I didn't. I can't imagine how these patterns may help. What I have right now is a User property on the BaseController and on the base class of a MVC view, using Service Location in HtmlHelpers and ActionFilters and passing in a User object into methods and constructors. – Rookian Jun 09 '13 at 19:06
  • @MarkSeemann can you explain you ideas more in detail? – Rookian Jun 09 '13 at 19:54
  • 4
    @Rookian I can't (yet) help you, as I don't know what you are trying to do. Can you explain why you need user data in "controllers, view, HTML helpers, services and so on"? I can understand Controllers, but Views, HTML helpers, services, etc? That sounds like a lack of cohesion to me... – Mark Seemann Jun 09 '13 at 21:43
  • 4
    My thoughts are the same as @MarkSeemann's. If you need that service in "neary every class", it is an indication that there is a problem with your design. Especially needing a service in a View or Html helper smells bad. You shouldn't need any dependencies in those. But for us to give any usefull feedback, you'll need to show some examples of views, html helpers and controllers that show how you are using that service in there. – Steven Jun 10 '13 at 08:10
  • I am not allowed to share code, I can only explain some things, sorry. I have updated my question. – Rookian Jun 10 '13 at 20:11

3 Answers3

8

In Views we check Operations to only show an edit or delete button if the user has the right to do so.

The view should not do this check. The Controller should return a view model to the view that contains boolean properties that state whether those buttons should be visible. Returning a bool with IsSuperUser already moves to much knownledge into the view. The view shouldn't know that it should show a certain button for a super user: that's up to the controller. The view should only be told what to display.

If almost all views have this code, there are ways to extract repetitive parts out of your views, for instance with partial views. If you're finding yourself repeating those properties over many view models, perhaps you should define an envelope view model (a generic view model that wraps the specific model as T). A controller can create its view model, while you create a service or cross-cutting concern that wraps it in your envelope.

In Service Classes we need those properties too. For instance to decide if a basket is valid or not

In this case you are talking about validation, which is a cross-cutting concern. You should use decorators to add this behavior instead.

Steven
  • 166,672
  • 24
  • 332
  • 435
  • So instead of User.IsSuperUser, ViewModel.ShowCreateButton, right? I am scared to overcomplicate things, because of the mapping stuff. But I get your point. I don't like the idea of a base view model at all. Moreover the base view model would be damn fat, because it would need to serve the purpose for a lot of views, when something like IsSuperUser is not allowed, so that I need to have a ViewModel.ShowFavListCreateButton, ViewModel.ShowCreateAdressListButton. Both buttons are nothing than User.IsSuperUser. The the idea of one model can't work when I am not allowed to share User.IsSuperUser. – Rookian Jun 16 '13 at 10:32
  • I don't get your point about a decorator for my service classes. Can you show a simple example, so I can get your point? – Rookian Jun 16 '13 at 10:33
  • @Rookian: Are you familiar with [this article](http://bit.ly/11yJdS5) of mine? It talks about hiding business operations behind a single generic `ICommandHandler` abstraction. When you do this, you can easily create decorators that wrap those business operations, such as a decorator that does validation. – Steven Jun 16 '13 at 12:42
  • Yes, I will take a look on them later on. – Rookian Jun 16 '13 at 13:50
  • I read your articles. They are really good. But I would still need to pass User or IUserService to get the User information. – Rookian Jun 17 '13 at 18:41
  • @Rookian: You need to pass an `IUserService` into the decorator, but not in all your business logic (the command handler or query handlers). In case of a validator, you write a validator for a certain type of command and write a command handler decorator that uses the validator(s) for that command and calls their verify method. – Steven Jun 17 '13 at 20:29
  • The problem is that I have to develop on a legay app that uses a "stored procedure only" approach, where a lot of business logic is in the DB. But as I said I like the separation of commands and queries a lot! My biggest problem with CommandHandlers is that I have to create a lot of classes and have to use a lot of mappings. InputModel -> CommandMessage -> CommandHandler. But it is really EASY. Maybe you have a hint on my criticism. – Rookian Jun 17 '13 at 20:56
  • A lot of classes is fine, that's separation of concerns. A lot of mappig is overhead that should be avoided or automated IMO. – Steven Jun 17 '13 at 21:09
  • So how do you handle the InputModel -> CommandMessage mapping part? Do you use AutoMapper to just map those things automatically? Or do you just skip the CommandMessage and just use the InputModel (ViewModel) as a CommandMessage? – Rookian Jun 17 '13 at 21:11
  • let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/31891/discussion-between-steven-and-rookian) – Steven Jun 17 '13 at 21:19
2

This is MVC, right?

You're reinventing the wheel.

Add this method to your Global.asax.cs:

protected void Application_AuthenticateRequest(Object sender, EventArgs e)
{
    var authCookie = Request.Cookies[FormsAuthentication.FormsCookieName];
    if (authCookie != null)
    {
        var ticket = FormsAuthentication.Decrypt(authCookie.Value);
        var user = ticket.Name;
        var identity = new GenericIdentity(user, "Forms");
        var principal = new GenericPrincipal(identity, null);
        Context.User = principal;
    }
}

This example shows forms authentication which you can strip if you're using another mechanism. The key is these three lines:

    var identity = new GenericIdentity(user, "Forms");
    var principal = new GenericPrincipal(identity, null);
    Context.User = principal;

GenericIdentity and GenericPrincipal can be replaced with anything you want as long as they implement the (trivial) IIdentity and IPrincipal interfaces. You can create your own implementations of these classes with whatever extra properties you need.

You can then access the authenticated user from all the things you listed - controllers, views, etc. - via HttpContext.Current.User (which is static).

If you created your own implementation of IPrincipal you can just cast that reference to your custom type.

You'll note that IPrincipal has a method called IsInRole, so you'd say:

if (HttpContext.Current.User.IsInRole("SuperUser"))

TL;DR - you are overengineering something ASP.NET has already solved, and I'd have an aneurysm if I saw the types you're proposing in a production application.

Evan Machusak
  • 694
  • 4
  • 9
  • What is with all the other properties I need for the user? It is not only the user name and IsInRole. – Rookian Jun 19 '13 at 20:53
  • 1
    Context.User is a reference to an IPrincipal. You can create any type you want with as many properties as you want and assign it to the User property as long as it also implements IPrincipal. When you need to access those other properties you can just cast the Context.User property at the point of access. You could also create a typed extension method to do the cast for you and return a strongly typed reference if you are one of those people who thinks a cast indicates a design flaw. – Evan Machusak Jun 25 '13 at 13:05
0

I think the easiest and maintainable solution is to create a static class CurrentUserProvider which has only one method Get(HttpContextBase) that returns the current user, behind the scene you can use the DependencyResolver to get the service that actually returns the user. Then where you need the CurrentUser you can call CurrentUserProvider.Get(context) and do whatever custom logic you need to perform.

The other solution that you are trying to do is injecting the service in the base controller constructor which is okay if you have handful of controllers, it would become an issue if you have quite a number of controllers and not all of the controllers requires that service. Writing tests for those controller would be such pain in the neck, because you have to create stubs/mocks for that service for all your controller tests. Maybe you can use property injection instead of constructor to address it.

You can use the same property injection for Filters too.

Now, the remaining two are the view and the helper. For View you can create special base class that inherits from WebViewPage/ViewPage and use the IViewActivator to inject the service and the same applies for the helpers, create helpers that inherits from system helpers and use those in your base controllers and views.

I think the second approach is bit cumbersome and it does not add that much value to do all those custom things.

So my suggestion is to go with the first.

Kazi Manzur Rashid
  • 1,544
  • 13
  • 14