Implementation review for a MVC.NET app with custom membership

Question

I'd like to hear if anyone sees any problems with how I implemented the security in this Oracle based MVC.NET app, either security issues, concurrency issues or scalability issues.

First, I implemented a CustomOracleMembershipProvider to handle the database interface to the membership store.

I implemented a custom Principal named User which implements IPrincipal, and it has a hashtable of Roles.

I also created a separate class named AuthCache which has a simple cache for User objects. Its purpose is simple to avoid return trips to the database, while decoupling the caching from either the web layer or the data layer. (So I can share the cache between MVC.NET, WCF, etc.)

The MVC.NET stock MembershipService uses the CustomOracleMembershipProvider (configured in web.config), and both MembershipService and FormsService share access to the singleton AuthCache.

My AccountController.LogOn() method:

1) Validates the user via the MembershipService.Validate() method, also loads the roles into the User.Roles container and then caches the User in AuthCache.

2) Signs the user into the Web context via FormsService.SignIn() which accesses the AuthCache (not the database) to get the User, sets HttpContext.Current.User to the cached User Principal.

In global.asax.cs, Application_AuthenticateRequest() is implemented. It decrypts the FormsAuthenticationTicket, accesses the AuthCache by the ticket.Name (Username) and sets the Principal by setting Context.User = user from the AuthCache.

So in short, all these classes share the AuthCache, and I have, for thread synchronization, a lock() in the cache store method. No lock in the read method.

The custom membership provider doesn't know about the cache, the MembershipService doesn't know about any HttpContext (so could be used outside of a web app), and the FormsService doesn't use any custom methods besides accessing the AuthCache to set the Context.User for the initial login, so it isn't dependent on a specific membership provider.

The main thing I see now is that the AuthCache will be sharing a User object if a user logs in from multiple sessions. So I may have to change the key from just UserId to something else (maybe using something in the FormsAuthenticationTicket for the key?).

Answer 1

Why use a Hashtable for the roles? A plain list would likely be faster to search unless you expect people to have more than a handful of roles. If you can predict all roles in advance then using a bitmask/flags enumeration would be even better.

You should try to avoid writing your own locking mechanism, as it's fairly easy to get wrong. Use the new System.Collections.Concurrent classes, or if you must roll your own then be sure to use Interlocked (as all the other locking mechanisms are quite expensive).

Caching should use WeakReference encapsulation to allow entries to be GCed and support retrieving user information from the database if an entry is missing. Maybe have a look at Velocity if you need a distributed cache.

Sharing user objects might not be a problem, but is probably not a recommended strategy. Many database access frameworks will track objects retrieved in a session or unit of work, and sharing objects across sessions would then be problematic. If you do go for sharing user objects then be sure to make them immutable.

Last, I personally despise the whole Membership Provider API, because it uses GUIDs for identification and the default SQL Server database design for user profiles is just horrible (aka performance killer). This does not seem to be a concern for you as you've rolled your own (db and implementation) but you might want to evaluate whether there are any real benefits involved from implementing the API, or whether it's mostly shackles tying you to specific ways of doing things.