Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed static currentPrincipal (issue #32) #33

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Fixed static currentPrincipal (issue #32) #33

wants to merge 1 commit into from

Conversation

FUB4R
Copy link

@FUB4R FUB4R commented Sep 29, 2015

Avoids multiple users in ASP.NET application sharing a single identity.

@leleuj
Copy link

leleuj commented Oct 16, 2015

I have tried to reproduce the issue using my own demo (with the .Net CAS client v1.0.2): https://github.com/leleuj/dotnet-cas-client-demo, but unsuccessfully.

I open two browsers and authenticates in each of them with different identities and I do have two different identities.

Can you check on your side on give me more details? Thanks

@FUB4R
Copy link
Author

FUB4R commented Oct 16, 2015

Since the backing variable is [ThreadStatic], it might be required for both requests to be served by the same webserver thread. This behaviour will depend on whether you are using VS's internal server, IIS, etc.

Honestly I'm not sure of the conditions to make this happen, only that it happened consistently in our staging environment (web app hosted in IIS7), and that this diff solved it.

@leleuj
Copy link

leleuj commented Oct 16, 2015

OK. So if I want to reproduce it, I should simulate some load to make authentications on the same request, right?

@FUB4R
Copy link
Author

FUB4R commented Oct 16, 2015

Maybe... It may also be that the issue only appears on some web servers (such as IIS but not visual studio's integrated one).

@rdev5
Copy link
Contributor

rdev5 commented Nov 9, 2015

Should user principal objects even be static?

@leleuj
Copy link

leleuj commented Nov 10, 2015

Yes, I have the same feeling: I would have expected the principal to be stored in the web session like in the Java or PHP CAS client for example.

@FUB4R
Copy link
Author

FUB4R commented Nov 11, 2015

Thread.CurrentPrincipal is the "official" method of authenticating a user in .NET.

This article was a good read : http://www.lhotka.net/weblog/ASPNETThreadSwitching.aspx

I feel like with this PR the code is OK, maybe it would be a good idea to set Thread.CurrentPrincipal = null explicitly at the start of the method.

@serac
Copy link
Contributor

serac commented Nov 11, 2015

The purpose of that field is similar to that of AssertionThreadLocalFilter in the Java CAS Client; it provides access to the current principal in other arbitrary components in the request pipeline. I'm open to other solutions, but session storage may not be appropriate since the session may not have been allocated yet. Authentication happens pretty early in request processing as I recall.

Looking at b345f1f, where the field was introduced, we should probably just remove the field and use HttpContext.Current.User directly. Several references indicate that is the best way to determine the identity of the authenticated user; here's a good one: http://stackoverflow.com/questions/1843271/whats-the-difference-between-httpcontext-current-user-and-thread-currentprincip.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants