WebAPI Authentication

Question

Do you see any security issues with this authentication approach or have any suggestions?

WebAPI is REST-based. The user logs in by calling the Authentication Controller and expects a 401 if unauthenticated or a 200 with a session token in the header.

GitHub

Session session = null;
try
{
 session = Models.User.Login(user);
}
catch (Exception ex)
{
 var errResponse = new HttpResponseMessage(System.Net.HttpStatusCode.Unauthorized)
 {
 Content = new StringContent("Invalid Username or Password"),
 ReasonPhrase = ex.InnerException.ToString()
 };
 throw new HttpResponseException(errResponse);
}

On every call except Authentication the request is intercepted and expects a session header. If a session exists in the database then the request is authenticated.

protected async override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
{
 try
 {
 //Unauthorized
 if (HttpContext.Current.Request.Headers[SessionHeader] == null)
 {
 var errResponse = new HttpResponseMessage(System.Net.HttpStatusCode.Forbidden)
 {
 Content = new StringContent(SessionError),
 ReasonPhrase = SessionError
 };
 return errResponse;
 }
 string sessionHeader = HttpContext.Current.Request.Headers[SessionHeader];
 Session session = Session.Lookup(sessionHeader);
 if (session == null)
 {
 throw new SecurityException("No session record exists");
 }
 else if (session.Expired)
 {
 throw new SecurityException("Session Expired");
 }
 else
 {
 session.LastAccess = DateTime.Now;
 session.Save();
 var identity = new SecurityPrincipal(session);
 Thread.CurrentPrincipal = identity;
 HttpContext.Current.User = identity;
 }

The session lookup is done with Entity Framework

public static Session Lookup(string sessionId) {
 using (var context = new SingleAppContextCustom())
 {
 var session = (from s in context.Sessions.IncludeChildren()
 where s.SessionId == sessionId
 select s).FirstOrDefault();
 return session;
 }
}

• user70216: are you looking for general improvements of this code? or if this is the way you should authenticate users? Not quite clear what you expect for answers.
• coder: Mostly if there's any security issues but general code review as well
• Greg: You shouldn't use a Session, you should really use something along these lines. weblogs.asp.net/jongalloway/… That way you can use the Authorization Attributes.
• coder: Its not using a server side Session. That's just the name of the variable. Its using a token passed in the header. the session is looked up in the database.

Answer 1

In general, you want to give as little information as possible for why access was denied. So I would change that section of code to:

if (session == null || session.Expired)
{
 throw new SecurityException("Access Denied");
}

Presumably this exception gets converted to an appropriate HTTP status code.

SendAsyc is an odd name for a function that enforces security, but maybe you have a reason for that. That said, it seems like there's something missing since I don't see where it is async or returns a Task.