27

I've been noticing static classes getting a lot of bad rep on SO in regards to being used to store global information. (And global variables being scorned upon in general) I'd just like to know what a good alternative is for my example below...

I'm developing a WPF app, and many views of the data retrieved from my db are filtered based on the ID of the current logged in user. Similarly, certain points in my app should only be accessable to users who are deemed as 'admins'.

I'm currently storing a loggedInUserId and an isAdmin bool in a static class.

Various parts of my app need this info and I'm wondering why it's not ideal in this case, and what the alternatives are. It seems very convienient to get up and running.

The only thing I can think of as an alternative is to use an IoC Container to inject a Singleton instance into classes which need this global information, the classes could then talk to this through its interface. However, is this overkill / leading me into analysis paralysis?

Thanks in advance for any insight.


Update

So I'm leaning towards dependency injection via IoC as It would lend itself better to testability, as I could swap in a service that provides "global" info with a mock if needed. I suppose what remains is whether or not the injected object should be a singleton or static. :-)

Will prob pick Mark's answer although waiting to see if there's any more discussion. I don't think there's a right way as such. I'm just interested to see some discussion which would enlighten me as there seems to be a lot of "this is bad" "that is bad" statements on some similar questions without any constructive alternatives.


Update #2 So I picked Robert's answer seeing as it is a great alternative (I suppose alternative is a weird word, probably the One True Way seeing as it is built into the framework). It's not forcing me to create a static class/singleton (although it is thread static).

The only thing that still makes me curious is how this would have been tackled if the "global" data I had to store had nothing to do with User Authentication.

RekrowYnapmoc
  • 2,166
  • 2
  • 22
  • 23
  • I think that the global data is fine in this case. Can it be modified anywhere in your application, or can it only be set in one place? The latter would be better if possible. Sometimes you just need global data and it does not make sense to have it any other way. People tend to get religious about this stuff. – Ed S. Aug 11 '09 at 20:51
  • Rules are meant to be broken. ASP.NET provides static places to store data, for example. – Greg Aug 11 '09 at 20:54
  • @Ed - These properties will be set after my login screen when it performs the checks on the user in the DB – RekrowYnapmoc Aug 11 '09 at 21:04
  • It's not Analysis Paralysis if you just do it :) – Mark Seemann Aug 11 '09 at 21:10
  • @John : in my situation there can only be one user logged in as it is a client WPF app. – RekrowYnapmoc Aug 11 '09 at 21:24
  • If there are few places to inject, then use DI. My answer was predicated on you needing the information throughout the application. If not, then not. – John Saunders Aug 11 '09 at 22:34

3 Answers3

12

Forget Singletons and static data. That pattern of access is going to fail you at some time.

Create your own custom IPrincipal and replace Thread.CurrentPrincipal with it at a point where login is appropriate. You typically keep the reference to the current IIdentity.

In your routine where the user logs on, e.g. you have verified their credentials, attach your custom principal to the Thread.

IIdentity currentIdentity = System.Threading.Thread.CurrentPrincipal.Identity;
System.Threading.Thread.CurrentPrincipal 
   = new MyAppUser(1234,false,currentIdentity);

in ASP.Net you would also set the HttpContext.Current.User at the same time

public class MyAppUser : IPrincipal
{
   private IIdentity _identity;

   private UserId { get; private set; }
   private IsAdmin { get; private set; } // perhaps use IsInRole

   MyAppUser(userId, isAdmin, iIdentity)
   {
      if( iIdentity == null ) 
         throw new ArgumentNullException("iIdentity");
      UserId = userId;
      IsAdmin = isAdmin;
      _identity = iIdentity;          
   }

   #region IPrincipal Members
   public System.Security.Principal.IIdentity Identity
   {
      get { return _identity; }
   }

   // typically this stores a list of roles, 
   // but this conforms with the OP question
   public bool IsInRole(string role)
   {  
      if( "Admin".Equals(role) )
         return IsAdmin;     

      throw new ArgumentException("Role " + role + " is not supported");
   }
   #endregion
}

This is the preferred way to do it, and it's in the framework for a reason. This way you can get at the user in a standard way.

We also do things like add properties if the user is anonymous (unknown) to support a scenario of mixed anonymous/logged-in authentication scenarios.

Additionally:

  • you can still use DI (Dependancy Injection) by injecting the Membership Service that retrieves / checks credentials.
  • you can use the Repository pattern to also gain access to the current MyAppUser (although arguably it's just making the cast to MyAppUser for you, there can be benefits to this)
Robert Paulson
  • 17,603
  • 5
  • 34
  • 53
  • +1... learned something new today (Thanks!). Any "it's in the framework for a reason" objects where you could check this against classes on-the-fly prior to loading them? My answer here could be rendered obsolete if there is... – Austin Salonen Aug 11 '09 at 22:10
  • Sorry @Austin, I don't wholly understand the "check this against classes on-the-fly prior to loading them" bit. Could you elaborate? – Robert Paulson Aug 11 '09 at 22:14
  • @Robert, when you saying use DI to inject the Membership Service, do you mean that as a generic term or are you specifically referring to the ASP.NET Membership functionality (I'm using WPF). – RekrowYnapmoc Aug 11 '09 at 22:34
  • Why will singletons eventually fail? – John Saunders Aug 11 '09 at 22:35
  • @ac2u both. If you look at the ASP.NET MVC NerdDinner example, they create their own IMembershipService and IFormsAuth to wrap the built-in one's. The linked blog looks like it talks about it: http://weblogs.asp.net/shijuvarghese/archive/2009/03/12/applying-dependency-injection-in-asp-net-mvc-nerddinner-com-application.aspx – Robert Paulson Aug 11 '09 at 22:42
  • Singletons are hard to maintain and test, and there's been plenty written on it already. A quick google yields http://blogs.msdn.com/scottdensmore/archive/2004/05/25/140827.aspx – Robert Paulson Aug 11 '09 at 22:45
  • Accepted, thanks! Will use this with DI.. won't bother implementing my own full-on MemberShipService as it seems a bit overkill for the needs of my App. However working with the IPrincipal and IIdentity interfaces seems pretty straightforward. P.S. Just out of curiosity, say the "global" data in my original question had nothing to do with user authentication, how would you go about a handy way of accessing it then? Even with DI the IoC Container would surely be providing a Singleton? Cheers again Robert. – RekrowYnapmoc Aug 11 '09 at 23:00
  • Thx. User is something that cuts through a whole application, and in .Net there is a well known place to access it, and to extend it (IPrincipal) hence my answer. When it's not so clear cut, DI is probably the best way to go. Singleton's are 'this class exists once and only once for the entire application' and in the Singleton pattern, the class instantiate themselves and lives forever, which is fundamentally different from IoC. There is always going to be state or context that gets dragged around, it's just how you set up the code to make testing/maintenance easier. – Robert Paulson Aug 11 '09 at 23:29
  • 2
    Thread.CurrentPrincipal ? so in the end you're still storing data in a static member... – Thomas Levesque Aug 11 '09 at 23:30
  • It's only thread static, and is a well known place to store the information. See my other comment. – Robert Paulson Aug 11 '09 at 23:32
  • @Thomas Correct. I never said I wouldn't, in fact, my question was why shouldn't I? Thread.CurrentPrincipal is static to a certain degree, however, as opposed to rolling my own static solution, I can use one in .net, which will be easy to plug into a MembershipService should I decide to in the future. – RekrowYnapmoc Aug 12 '09 at 00:01
  • @Robert, yeah I understand the concepts of IoC and Singletons, however, what I'm saying is if I'm using IoC to Inject a dependency which contains "state" data for the application into a class, my natural method of doing this would be to configure the Container provide a Singleton instance when this state data is requested. Otherwise I'd be populating this state data by having the Container instansiate and return the object everytime, which seems wasteful. Do you understand? Esentially, the same issue, but now it's the Container's problem. – RekrowYnapmoc Aug 12 '09 at 00:05
  • You never get away from having some kind of state, and DI doesn't change that, it just provides a better way to manage it / test / change concrete implementation. State that doesn't change isn't the same as a singleton though, and proper caching can do a better job than singleton's can (typically why people want the singleton in the first place). Also, lots of little, short lived objects aren't necessarily a bad thing. The GC does a good job, and as usual, if you think it's an issue, profile it first (intuition rarely being correct). Sometimes some objects need to be [ThreadStatic] too .. – Robert Paulson Aug 12 '09 at 01:19
2

There are many other answers here on SO that explains why statics (including Singleton) is bad for you, so I will not go into details (although I wholeheartedly second those sentiments).

As a general rule, DI is the way to go. You can then inject a service that can tell you anything you need to know about the environment.

However, since you are dealing with user information, Thread.CurrentPrincipal may be a viable alternative (although it is Thread Static).

For convenience, you can wrap a strongly typed User class around it.

Mark Seemann
  • 225,310
  • 48
  • 427
  • 736
  • In this case, with user data, that will be used throughout the application, just about every class in the application would need to have the ILoggedInUser injected into it. – John Saunders Aug 11 '09 at 21:19
  • Also, the question asked about "the same convenience". – John Saunders Aug 11 '09 at 21:19
  • @Mark, So Mark, say I do inject a service using dependency injection, I essentially have a similar issue except moved to a different layer. I'm thinking that this service should be providing this information using a Singleton as well (see the last paragraph of my question). Thanks for taking the time to help. – RekrowYnapmoc Aug 11 '09 at 21:27
  • The article you linked to is interesting, thanks, it seems to lead back round to the same issue when accessing it though, as the author ends up using a static property for handiness. – RekrowYnapmoc Aug 11 '09 at 21:49
  • Can't understand why you're getting downmodded, was a perfectly reasonable answer. – RekrowYnapmoc Aug 11 '09 at 21:51
2

I'd try a different approach. The static data class is going to get you in trouble -- this is from experience. You could have a User object (see @Robert Paulson's answer for a great way to do this) and pass that to every object as you construct it -- it might work for you but you'll get a lot template code that just repeats everywhere.

You could store all your objects in a database / encrypted file with the necessary permissions and then dynamically load all of them based on your Users permissions. With a simple admin form on the database, it's pretty easy to maintain (the file is a little bit harder).

You could create a RequiresAdminPermissionAttribute object to apply to all your sensitive objects and check it at run-time against your User object to conditionally load to objects.

While the route you're on now has merit, I think there are some better options to try.

Austin Salonen
  • 49,173
  • 15
  • 109
  • 139