-
Notifications
You must be signed in to change notification settings - Fork 603
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
Catering for IPrincipal implementations rather than just ClaimsPrincipal on single logout requests #1472
Comments
I'm a bit surprised by this as the .NET Framework has changed all existing implementations of |
|
There must be an actual implementation type. Try calling |
Sorry, I’m not quite sure what you mean, @AndersAbel… 🤔
So, Therefore, if your code tries to cast |
IIRC, non-ClaimsPrincipal/ClaimsIdentity types were unofficially no longer supported by .NET [Core]. |
In many (sophisticated) authentication frameworks like, e.g., Sitecore federated authentication, the
User
class is an implementation of theIPrincipal
interface, not merely an instance ofClaimsPrincipal
.Now, because of the way
OwinContextExtensions.ToHttpRequestData
is implemented (in v2, at least, wherecontext.Request.User
is assumed to be an instance ofClaimsPrincipal
), single logout fails miserably in any such frameworks. 😕The proper fix would be for
HttpRequestData.HttpRequestData
to be rewritten to accept anIPrincipal
rather than aClaimsPrincipal
parameter, but of course that would be a rather big job, which understandably you might not want to consider for v2 (…moreover —I honestly haven’t checked— this might be totally irrelevant for v3). Yet, it’s a pity that such frameworks cannot take advantage of this nice library solely because there is no way of getting single logout to work.As it would appear that the value of
context.Request.User
is really only read in theLogoutCommand
class, then a quick “tactical” fix would be to fall back toClaimsPrincipal.Current
whenevercontext.Request.User
cannot be cast to a non-nullClaimsPrincipal
, as in this commit.The text was updated successfully, but these errors were encountered: