Add Microsoft.AspNetCore.Authentication.WsFederation, samples, and tests. #1639

Tratcher · 2018-02-05T22:42:00Z

#1635 Merge the new WsFed package from patch branch into dev
The only diffs from the patch branch involve reacting to the new scheme forwarders in SignOut.

Squashed commits from the patch branch:
#43 Add Microsoft.AspNetCore.Authentication.WsFederation, samples, and tests.
#1443 Block unsolicited wsfed logins by default.
#1520 Update WsFed to use the 2.0 event structure
#1425 Implement WsFed remote signout cleanup
Rework WsFed RemoteSignOutPath logic to work with ADFS #1581
Update versions, dependencies.

HaoK · 2018-02-05T22:49:07Z

src/Microsoft.AspNetCore.Authentication.WsFederation/WsFederationOptions.cs

+        /// CallbackPath to a dedicated endpoint may provide better error handling.
+        /// This is disabled by default.
+        /// </summary>
+        public bool SkipUnrecognizedRequests { get; set; } = false;


nit: you don't set some of the other defaults to false

HaoK · 2018-02-05T22:51:56Z

...uthentication.WsFederation.Test/Microsoft.AspNetCore.Authentication.WsFederation.Test.csproj

@@ -0,0 +1,33 @@
+<Project Sdk="Microsoft.NET.Sdk">


Today we only have a single test project, do you want to merge this into a new WsFed folder similar to OIDC?

We've been trying to separate out OIDC into it's own project for a while but the work keeps getting backlogged. I'm not sure what we'd gain by merging this one.

I didn't think that was a goal for OIDC, just to refactor/clean up tests. I don't really care one way or another really long term (between one test project per package or one uber test project), but today since there's only one uber test project for all of auth, it seems weird to have this be stand alone.

Personally I think having all the tests in one project are useful (when doing refactoring especially), running them all together doesn't even take long right now. If these ever become slow then splitting them apart would be more useful, but if we are putting the functional tests in other repos like AuthSamples/Identity/templates, I don't think there's much value in splitting them up

HaoK · 2018-02-05T22:53:43Z

src/Microsoft.AspNetCore.Authentication.WsFederation/WsFederationHandler.cs

+                _configuration = await Options.ConfigurationManager.GetConfigurationAsync(Context.RequestAborted);
+            }
+
+            string baseUri =


HaoK · 2018-02-05T22:53:56Z

src/Microsoft.AspNetCore.Authentication.WsFederation/WsFederationHandler.cs

+                    Request.Host +
+                    Request.PathBase;
+
+            string currentUri =


Note: Use base.CurrentUri

HaoK · 2018-02-05T22:54:06Z

src/Microsoft.AspNetCore.Authentication.WsFederation/WsFederationHandler.cs

+                properties.RedirectUri = currentUri;
+            }
+
+            WsFederationMessage wsFederationMessage = new WsFederationMessage()


HaoK · 2018-02-05T22:54:29Z

src/Microsoft.AspNetCore.Authentication.WsFederation/WsFederationHandler.cs

+
+            wsFederationMessage.Wctx = Uri.EscapeDataString(Options.StateDataFormat.Protect(properties));
+
+            string redirectUri = wsFederationMessage.CreateSignInUrl();


HaoK · 2018-02-05T23:01:39Z

src/Microsoft.AspNetCore.Authentication.WsFederation/WsFederationOptions.cs

+        /// The Authentication Scheme to use with SignOutAsync from RemoteSignOutPath. SignInScheme will be used if this
+        /// is not set.
+        /// </summary>
+        public string SignOutScheme { get; set; }


Can consider using ForwardSignOut instead.. But I guess this is fine for now, we can clean up the whole signin/signout scheme stuff for the remote handlers in 2.2/3.0

Let's wait until we figure that out system-wide.

Tratcher · 2018-02-06T18:49:44Z

Updated

HaoK · 2018-02-06T21:11:50Z

src/Microsoft.AspNetCore.Authentication.WsFederation/WsFederationHandler.cs

+        /// Handles Signout
+        /// </summary>
+        /// <returns></returns>
+        public async virtual Task SignOutAsync(AuthenticationProperties properties)


Do you think we should add a SignOutHandler and have this derive from that so you'd only have to implement HandleSignOut like the other helpers? I've been debating adding those base classes for SignIn/SignOut.

Oh, did those get dropped from the final version? Yeah, it's awkward that all implementors need to include this for the forwarders feature to work. File a followup issue?

Not dropped, I just merged the minimal changes in for now, nothing was using the new base classes yet so I didn't put them in... we should probably add them if only for wsfed to use... Its a breaking change to change the other ones so I couldn't fix the existing handers to use them...

It may also be a breaking change for WsFed then, this code was merged into 2.0 first. It's shipping out of band.

We shipped WsFed already? I just meant its a breaking change on the handlers since we would be making SignOut non virtual and replacing it with protected virtual HandleSignOut etc.

So WsFed can't depend on 2.1?

We've nearly shipped WsFed 2.0.0 as an out of band release. We're only waiting for verification sign-off on the final package. It's also been merged into the next patch release.

HaoK · 2018-02-06T21:16:54Z

test/Microsoft.AspNetCore.Authentication.Test/WsFederation/WsFederationTest.cs

+            // Did the request end in the actual resource requested for
+            Assert.Equal("AuthenticationFailed", await response.Content.ReadAsStringAsync());
+        }
+


Consider adding a test to verify that map'd urls are picked up in redirects, see https://github.com/aspnet/Security/blob/dev/test/Microsoft.AspNetCore.Authentication.Test/FacebookTests.cs#L597

We actually have a large set of common tests now, we should probably consider adding a base handler specification test rather than cloning all of the tests that really should be common across all the handlers. But in the mean time you might want to consider copying a lot of the tests from one of the other common ones from facebook like ThrowsIfAppIdMissing variants, VerifySchemeDefaults, VerifySignInSchemeCannotBeSetToSelfUsingDefaultSignInScheme etc.

I created #1640 to track moving as many as possible into base classes, so I'd just do the ones specific to wsfed, like VerifySchemeDefaults and the appropriate validations that are required on the options, and the redirect + map test

Rebased and added a few tests.

Tratcher · 2018-02-10T00:00:53Z

src/Microsoft.AspNetCore.Authentication.WsFederation/WsFederationOptions.cs

+
+        /// <summary>
+        /// Indicates if requests to the CallbackPath may also be for other components. If enabled the handler will pass
+        /// requests through that do not contain OpenIdConnect authentication responses. Disabling this and setting the


Justin offline: OpenIdConnect -> WsFederation

…d tests. #1443 Block unsolicited wsfed logins by default. #1520 Update WsFed to use the 2.0 event structure #1425 Implement WsFed remote signout cleanup Rework WsFed RemoteSignOutPath logic to work with ADFS #1581 Update versions, dependencies.

Tratcher added this to the 2.1.0-preview2 milestone Feb 5, 2018

Tratcher self-assigned this Feb 5, 2018

Tratcher requested a review from HaoK February 5, 2018 22:42

HaoK reviewed Feb 5, 2018

View reviewed changes

Tratcher force-pushed the tratcher/wsfed21 branch from eab171d to 6413305 Compare February 6, 2018 18:37

HaoK reviewed Feb 6, 2018

View reviewed changes

Tratcher commented Feb 10, 2018

View reviewed changes

Tratcher force-pushed the tratcher/wsfed21 branch from 6413305 to eafc687 Compare February 12, 2018 19:25

Tratcher force-pushed the tratcher/wsfed21 branch from eafc687 to d95109c Compare February 26, 2018 23:31

HaoK approved these changes Feb 27, 2018

View reviewed changes

Tratcher merged commit d95109c into dev Feb 28, 2018

Tratcher deleted the tratcher/wsfed21 branch February 28, 2018 17:28

Tratcher mentioned this pull request Feb 28, 2018

Add new WsFederation package aspnet/Universe#927

Merged


		wsFederationMessage.Wctx = Uri.EscapeDataString(Options.StateDataFormat.Protect(properties));

		string redirectUri = wsFederationMessage.CreateSignInUrl();

Add Microsoft.AspNetCore.Authentication.WsFederation, samples, and tests. #1639

Add Microsoft.AspNetCore.Authentication.WsFederation, samples, and tests. #1639

Uh oh!

Conversation

Tratcher commented Feb 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher commented Feb 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HaoK Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Tratcher Feb 6, 2018 •

edited

Loading

Tratcher Feb 6, 2018 •

edited

Loading

HaoK Feb 6, 2018 •

edited

Loading