search

616b2f / IdentityServer4.WsFederation

Sample for implementing WS-Federation IdP support for IdentityServer4 using ASP.NET Core
Apache License 2.0
6 stars 9 forks source link

Fix for URL parsing from doeringp requires UrlDecode #11

Open jesslilly opened 3 years ago

jesslilly commented 3 years ago

Hi Alexej,

I think I did the pull request correctly this time. The fix you merged in from doeringp does not pass the unit tests I made. We must still call UrlDecode so that "new Uri" will work correctly. I did use doeringp's good idea to use a dummy.com host since it just makes the URI parsing easier and uses less code. Does not rely on parsing for a question mark.

Also I like having these tests. I the other tests in the solution do not cover this issue.

image

When I add decode back, the new tests pass.

image

Also, there should be a comment why we use a dummy URL.

Thank you. I hope this PR is merged. 😄

616b2f commented 3 years ago

Thank you for your PR, I think your tests could be a good addition to our unit tests, here are really not much of them at the moment and I would be happy to merge if you resolve my concerns in comments.

jesslilly commented 3 years ago

No problem Alexej. I will try again.

jesslilly commented 3 years ago

Haha. Hi again @616b2f . For some reason I thought we were speaking different languages, but now I figured out why I get encoded URLs. It is because my IdentityServer implementation never decodes the URL before it is passed to IsValidReturnUrl. (I did not implement IdentityServer myself. I have inherited this project.)

In this picture you can see the encoded URL and it is passed as encoded all the way down the call stack.

image

I am surprised because the code for WebUtility.UrlDecode was in the source code before. So it must have been needed.

WsFederationMessage.FromUri does not decode by itself as seen in the implementation here:

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/bbf721fedca5dabc0d77e22828a5948be0d3fa86/src/Microsoft.IdentityModel.Protocols.WsFederation/WsFederationMessage.cs#L84

Also I added a throw new NotImplementedException(); as the first line in GetSignInRequestMessage. I cannot get the tests in WsFederationInterfaceTests to exercise that code. They still pass.

I'm not sure what to do now. I am finishing my day, so I will sleep on it.

Cordially, Jess

616b2f commented 2 years ago

@jesslilly if you could remove the "WebUtility.UrlDecode" and change the URL in the test. I would love to merge your PR.

jesslilly commented 2 years ago

Hi Alexej, I'm sorry it has been so long, I have forgotten what this is about.

On Wed, Feb 9, 2022 at 3:07 AM Alexej Kowalew @.***> wrote:

@jesslilly https://github.com/jesslilly if you could remove the "WebUtility.UrlDecode" and change the URL in the test. I would love to merge your PR.

— Reply to this email directly, view it on GitHub https://github.com/616b2f/IdentityServer4.WsFederation/pull/11#issuecomment-1033466225, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKXMGTV5QTWIIZH3VBKHJ3U2IOD5ANCNFSM4YMUXGZA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

-- Kind Regards, Jess

616b2f commented 2 years ago

@jesslilly me too so don't worry :)