Share cookies, add logout on core to sign out cookie in MVC samples #121

HaoK · 2022-07-21T19:36:53Z

Note this required updating a bunch of packages to pull in the cookie/dataprotection interop 2.2 code

samples/MvcCoreApp/Program.cs

HaoK · 2022-07-21T22:31:31Z

samples/MvcApp/App_Start/Startup.Auth.cs

            app.UseCookieAuthentication(new CookieAuthenticationOptions
            {
                AuthenticationType = DefaultAuthenticationTypes.ApplicationCookie,
+                CookieName = ".AspNet.ApplicationCookie",


Need to add docs talking about this cookie sharing in this PR

twsouthwick · 2022-07-21T22:34:03Z

This looks to only be changes to samples, right? So it's more of a doc update on how to use the existing library?

HaoK · 2022-07-21T22:38:18Z

Yeah I believe so, so if we configure both apps to use the same cookie, and just add a controller action to logout/clear the cookie on the Core App side, both logouts work and there's no antiforgery issue

twsouthwick · 2022-07-21T22:51:30Z

By enabling this, it will cause all encrypted cookies to be invalid on the framework app, right? Effectively logging everyone off as a one-time part of the migration?

HaoK · 2022-07-22T04:11:44Z

Yeah changing the cookies settings like this will cause all all existing cookies to no longer be valid

mjrousos · 2022-07-22T13:42:53Z

I think a one-time logout is fine. It's inevitable that it will happen eventually as part of migration.

mjrousos

Looks good! My only concern is that there's a lot of changes needed in the ASP.NET app; it would be great if we could encapsulate some of that in a helper method in the adapters library so that it's easier to get started with this scenario.

NuGet.config

mjrousos · 2022-07-22T13:47:54Z

samples/MvcApp/App_Start/Startup.Auth.cs

+                        .CreateProtector(
+                            "Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationMiddleware",
+                            "Identity.Application",
+                            "v2"))),


Can we add a helper method to the adapters that takes care of this? This is a lot of code the user has to dump into their project and we know what most of it needs to look like. So, instead, we could create a UseSharedCookieAuthentication method that takes a CookieAuthenticationOptions parameter and takes care of adjusting the parameters to have the right name, ticket data format, etc. Then, the only change in the user's code would be to replace UseCookieAuthentication with UseSharedCookieAuthentication and probably provide one extra parameter with data protection setup information.

For providing data protection configuration, users could just pass a DataProtectionProvider into UseSharedCookieAuthentication or, if we know they're likely to use directories or Redis or SQL or something we could have overloads that allow them to pass paths/connection strings for those and setup data protection based on those parameters.

There was a change to chunking format for large cookies that's not covered here.
https://github.com/dotnet/aspnetcore/blob/de68feabb4dc84d2d5fbcfd7b3ed0b08f53216fa/src/Shared/ChunkingCookieManager/ChunkingCookieManager.cs#L42
https://github.com/aspnet/AspNetKatana/blob/e003703a8bbf003f56ad160a8d034d9560639bc8/src/Microsoft.Owin/Infrastructure/ChunkingCookieManager.cs#L46
https://github.com/dotnet/aspnetcore/blob/release/2.1/src/Security/Interop/src/ChunkingCookieManager.cs

One option would be to service Cookies.Interop with the recommended improvements. It's a stand-alone 2.1 package.

Note any new API would need to very flexible in how DataProtection is configured. Many apps pull their keys from DBs or vaults.

I think we should just go with adding whatever API we need to the adapters in the short term, can always move it somewhere later if there's demand

@mjrousos where do you think an appropriate place for the UseSharedCookie helper to live in adapters, the MVC app isn't currently using any adapter code right now right?

The MVC app already has a dependency on the adapters library to configure the server side of remote auth and remote session (look for the AddSystemWebAdapters call in global.asax.cs).

If there was an easy way to add another extension method for ISystemWebAdapterBuilder that would be nice since then all the adapter code would be together in one place in global.asax.cs. But for cookie auth configuration, it may be easier to do in Startup.Auth.cs as an IAppBuilder extension. Maybe have a method users could call in place of UseCookieAuthentication (which takes CookieAuthenticationOptions like that method does and uses them except where we need to update things to enable shared cookie usage).

So, short answer: I think in global.asax.cs extending ISystemWebAdapterBuilder or in Startup.auth.cs extending IAppBuilder would both be fine. In Startup.auth.cs might be the easiest way to allow the user to customize their cookie usage while still updating things for shared cookie scenarios.

So extending IAppBuilder would require pulling in Owin for adapters I think to add the extension to IAppBuilder, is that something we are okay doing?

Okay I see what I was missing, VS wasn't showing me any of the .Framework 472 files this makes more sense to me now

That's a good point about extending IAppBuilder requiring Owin. If we can come up with a nice surface area that extends ISystemWebBuilder that may be the better way to go. My only concern is that it may be difficult to come up with an API that has the user configure cookie sharing on the ISystemWebBuilder while configuring cookies on IAppBuilder.

An alternative would be to have a separate project (Microsoft.AspNetCore.SystemWebAdapaters.Owin or something like that) where we add IAppBuilder extensions for things like this. Then users who care about that scenario can reference that package (and they will already have OWIN dependencies) and others can ignore it.

mjrousos · 2022-07-22T13:52:13Z

samples/MvcApp/MvcApp.csproj

    </Reference>
    <Reference Include="System.Data" />
    <Reference Include="System.Data.DataSetExtensions" />
+    <Reference Include="System.Data.OracleClient" />


Registry? Oracle? This really does pull in the world! I'm guessing most of these are pulled in transitively and we're only using a couple of them directly. Makes me appreciate that <PackageReference> model in sdk style projects!

samples/MvcCoreApp/Program.cs

samples/MvcApp/packages.config

Tratcher · 2022-07-22T18:50:55Z

Here's a fun thread for background reading: aspnet/AspNetKatana#435

HaoK · 2022-07-27T18:19:59Z

samples/MvcApp/App_Start/Startup.Auth.cs

+                        // Must match the Scheme name on the MvcCoreApp, i.e. IdentityConstants.ApplicationScheme
+                        "SharedCookie",
+                        "v2");
+


@mjrousos I cleaned things up to remove a lot of the extraneous settings, while we could still add a ConfigureSharedCookie("appName", "sharedKeyPath", "authenticationScheme", cookieAuthenticationOptions), this at least makes the changes into 2 logical sections (configure the data protection, and then use it for the cookie ticket format. Thoughts ?

Perhaps its good enough with just a single helper to hide the hardcoded things out of their control with a var sharedDataProtectionProvider = CreateSharedDataProtectionProvider(sharedKeyDirectory, appname, authScheme)

I'm liking this better - a ConfigureSharedCookie method seems easy to implement and would hide even more of the stuff people don't need to muck with (i.e. do they need access to the data protection provider?). Someone could always build it up themselves, but simplifying the set up as much as possible would be nice (espeically to get rid of any hard coded things)

I like the idea of minimizing the code the customer has to write, but unfortunately I think it will be common to need access to the data protection provider. A shared directory won't work for a lot of scenarios (anything where the ASP.NET and ASP.NET Core apps run on different machines. App Service?) so I think it's important that customers be able to provide their own data protection providers (which may use Redis or blob storage or something as a backing store).

Can we have a helper method that will make the CreateProtector call with all the right string constants, but allow the user to pass in a DataProtectionProvider?

Maybe an API like ConfigureSharedCookie(this IAppBuilder app, CookieAuthenticationOptions options, IDataProtectionProvider dataProtectionProvider) or something like that. It would mean pulling in a reference to OWIN stuff in the adatpers library, but we could split this API out into a separate package that users would only depend on if they wanted this sort of shared auth enabled (in which case they were using OWIN anyhow).

See #2 - it could be put in there.

mjrousos

I think this is a useful update to the MVC sample. I still think it would be nice to have an extension method or two to make setting the scenario up easier for developers.

mjrousos · 2022-07-28T18:23:57Z

samples/MvcApp/App_Start/Startup.Auth.cs

+                        // Must match the Scheme name on the MvcCoreApp, i.e. IdentityConstants.ApplicationScheme
+                        "SharedCookie",
+                        "v2");
+


I like the idea of minimizing the code the customer has to write, but unfortunately I think it will be common to need access to the data protection provider. A shared directory won't work for a lot of scenarios (anything where the ASP.NET and ASP.NET Core apps run on different machines. App Service?) so I think it's important that customers be able to provide their own data protection providers (which may use Redis or blob storage or something as a backing store).

Can we have a helper method that will make the CreateProtector call with all the right string constants, but allow the user to pass in a DataProtectionProvider?

Maybe an API like ConfigureSharedCookie(this IAppBuilder app, CookieAuthenticationOptions options, IDataProtectionProvider dataProtectionProvider) or something like that. It would mean pulling in a reference to OWIN stuff in the adatpers library, but we could split this API out into a separate package that users would only depend on if they wanted this sort of shared auth enabled (in which case they were using OWIN anyhow).

HaoK · 2022-07-28T18:40:14Z

Are we okay with merging this as is, and I'll file a follow up issue/PR to address / add the new helpers?

twsouthwick

I think the POC in the samples is good for now - we can add APIs in the adapters later

mjrousos · 2022-07-28T18:55:26Z

Yep, I think we can merge this as is but let's get an issue to track adding helper methods

mjrousos · 2022-07-28T18:55:48Z

Actually, I see you're using #121 for that. That works. Thanks!

HaoK added 2 commits July 21, 2022 12:36

Share cookies, add logout on core to sign out cookie

a895854

Cleanup

5150c27

HaoK commented Jul 21, 2022

View reviewed changes

samples/MvcCoreApp/Program.cs Outdated Show resolved Hide resolved

HaoK commented Jul 21, 2022

View reviewed changes

HaoK requested review from Tratcher, mjrousos and twsouthwick July 21, 2022 22:31

HaoK changed the title ~~Share cookies, add logout on core to sign out cookie~~ Share cookies, add logout on core to sign out cookie in MVC samples Jul 21, 2022

mjrousos requested changes Jul 22, 2022

View reviewed changes

Tratcher reviewed Jul 22, 2022

View reviewed changes

samples/MvcApp/packages.config Show resolved Hide resolved

HaoK added 4 commits July 25, 2022 11:21

Switch to using shared key in user temp

6c39671

Remove public nuget

f91139d

Cleanup cookie sharing

130e904

Cleanup

c4f9d9f

HaoK commented Jul 27, 2022

View reviewed changes

HaoK marked this pull request as ready for review July 27, 2022 23:04

HaoK mentioned this pull request Jul 28, 2022

Auth: Share cookies between Framework and Core apps #14

Closed

mjrousos approved these changes Jul 28, 2022

View reviewed changes

twsouthwick approved these changes Jul 28, 2022

View reviewed changes

HaoK merged commit 9fc7391 into main Jul 28, 2022

HaoK deleted the haok/logout branch July 28, 2022 18:48

HaoK mentioned this pull request Jul 28, 2022

Logging out from ASP.NET Core app using ASP.NET Identity with remote app auth doesn't work #75

Closed

Share cookies, add logout on core to sign out cookie in MVC samples #121

Share cookies, add logout on core to sign out cookie in MVC samples #121

Uh oh!

Conversation

HaoK commented Jul 21, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

twsouthwick commented Jul 21, 2022

Uh oh!

HaoK commented Jul 21, 2022

Uh oh!

twsouthwick commented Jul 21, 2022

Uh oh!

HaoK commented Jul 22, 2022

Uh oh!

mjrousos commented Jul 22, 2022

Uh oh!

mjrousos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher Jul 22, 2022 • 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!

Tratcher Jul 22, 2022 • 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!

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!

Uh oh!

Uh oh!

Tratcher commented Jul 22, 2022

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!

mjrousos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HaoK commented Jul 28, 2022

Uh oh!

twsouthwick left a comment

Choose a reason for hiding this comment

Uh oh!

mjrousos commented Jul 28, 2022

Uh oh!

mjrousos commented Jul 28, 2022

Uh oh!

Reviewers

Assignees

Tratcher Jul 22, 2022 •

edited

Loading

Tratcher Jul 22, 2022 •

edited

Loading