Sustainsys / Saml2

Saml2 Authentication services for ASP.NET
Other
961 stars 604 forks source link

Catering for IPrincipal implementations rather than just ClaimsPrincipal on single logout requests #1472

Open Infarinato opened 4 weeks ago

Infarinato commented 4 weeks ago

In many (sophisticated) authentication frameworks like, e.g., Sitecore federated authentication, the User class is an implementation of the IPrincipal interface, not merely an instance of ClaimsPrincipal.

Now, because of the way OwinContextExtensions.ToHttpRequestData is implemented (in v2, at least, where context.Request.User is assumed to be an instance of ClaimsPrincipal), single logout fails miserably in any such frameworks. šŸ˜•

The proper fix would be for HttpRequestData.HttpRequestData to be rewritten to accept an IPrincipal rather than a ClaimsPrincipal 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 the LogoutCommand class, then a quick ā€œtacticalā€ fix would be to fall back to ClaimsPrincipal.Current whenever context.Request.User cannot be cast to a non-null ClaimsPrincipal, as in this commit.

AndersAbel commented 6 days ago

I'm a bit surprised by this as the .NET Framework has changed all existing implementations of IPrincipal to derive from ClaimsPrincipal. What is the actual type of the IPrincipal in the Sitecore setup?

Infarinato commented 6 days ago

What is the actual type of the IPrincipal in the Sitecore setup?

System.Security.Principal.IPrincipal, as far as I can tell, @AndersAbelā€¦