Static CurrentPrincipal leads to shared authentication between threads

FUB4R commented 9 years ago

Just got bitten by this today : once a user was authenticated on my ASP.NET website, everyone shared his/her identity!

This was due to the currentPrincipal variable being shared between requests (even though there is the ThreadStatic annotation).

I fixed it by making the following change :

public static ICasPrincipal CurrentPrincipal
    {
        //get { return currentPrincipal; }
        get { return Thread.CurrentPrincipal as ICasPrincipal; }
    }

mmoayyed commented 9 years ago

Would you be able to post a PR?

FUB4R commented 9 years ago

See PR #33

lanorkin commented 7 years ago

I think that fix is valid as quick-fix, but in long run it's bad idea to rely on Thread.CurrentPrincipal, as it highly restricts design of library user.

I can easily imagine applications where CAS authentication will be just one of allowed authentications, which for example provide additional permissions / roles to user already authenticated with say usual login / password.

In these cases developers might want to use their own principal classes, and utilize Thread.CurrentPrincipal for own purposes, but they still will need access to result of CAS authentication,

I think that best option here is to use HttpContext.Current.Items to keep principal like this:

public static ICasPrincipal CurrentPrincipal
{
    get { return (ICasPrincipal)HttpContext.Current.Items["DotNetCasClient.CasAuthentication.CurrentPrincipal"]; }
    private set { HttpContext.Current.Items["DotNetCasClient.CasAuthentication.CurrentPrincipal"] = value; }
}

FUB4R commented 7 years ago

The problem with using anything from HttpContext is that you may not have access to this class in the places where you want to check the current user (for various reasons). I also vaguely remember experimenting with using the HttpContext before submitting this PR, and found it wasn't appropriate.

Regardless, this bug is a serious issue (which luckily got caught before it went into production). It's disappointing to see that, almost 2 years later, no fix has been merged.

I appreciate that there may be better ways to solve the issue. Whilst possibly imperfect, the code I suggested has been proven to work for over a year in a large business application with hundreds of simultaneous users. Any other deployments out there are basically subject to "random authentication", which is pretty catastrophic...

apereo / dotnet-cas-client

Static CurrentPrincipal leads to shared authentication between threads #32