2
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ – Ernst May 15 '16 at 8:26
  • \$\begingroup\$ Mostly if there's any security issues but general code review as well \$\endgroup\$ – coder May 15 '16 at 16:02
  • \$\begingroup\$ 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. \$\endgroup\$ – Greg May 16 '16 at 13:34
  • \$\begingroup\$ 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. \$\endgroup\$ – coder May 16 '16 at 20:26
2
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.