C# : So if a static class is bad practice for stor

Question

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.

Answer 1

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.

Answer 2

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)

Answer 3

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.