Skip to content
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

Enable authenticating with remote app #40

Merged
merged 45 commits into from
Jun 8, 2022
Merged

Enable authenticating with remote app #40

merged 45 commits into from
Jun 8, 2022

Conversation

mjrousos
Copy link
Member

This PR enables remote authentication so that the ASP.NET Core app in an upgrade workflow can defer to the original ASP.NET app for user identity.

Fixes #15

@mjrousos mjrousos requested a review from twsouthwick May 23, 2022 20:15
Copy link
Member

@twsouthwick twsouthwick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Can you add some documentation about what the flow here looks like? (kind of like the remote session stuff).

@mjrousos
Copy link
Member Author

Yep, for sure. I'll get some docs added today.

@twsouthwick twsouthwick requested a review from Tratcher May 24, 2022 15:36
@twsouthwick
Copy link
Member

Question: Do we want this in a separate library? The session stuff was put in a separate package because it would force new dependencies brought in by System.Text.Json. Does this bring in new dependencies?

Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which kinds of auth have you tried this with?

/// <returns></returns>
public Task ProcessAsync(RemoteAuthenticationResult result, HttpContext context)
{
if (result.ResponseHeaders.TryGetValue(LocationHeaderName, out var locationHeaders))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set-Cookie headers can have a similar problem with their path and domain fields.

@mjrousos
Copy link
Member Author

@twsouthwick - that's a good point. This doesn't pull in any new dependencies, so we could include it in the SystemWebAdapters project without adding a new one. I suppose it might be nice for customers to not have too many packages they need to install.

@Tratcher - so far, I've tested it with OWIN cookie auth (ASP.NET Idenity) and it works for that scenario. I'll try out forms auth and JWT tokens to confirm it works for those scenarios, too.

@mjrousos
Copy link
Member Author

I've added a sample in this PR demonstrating authenticating with the ASP.NET app for bearer token scenarios (so we have samples now for cookies and bearer auth). I'll spend some time today updating the PR based on the remaining feedback here and then will try it out with forms auth to make sure it works for that scenario, too.

mjrousos and others added 2 commits June 1, 2022 10:33
# Conflicts:
#	samples/MvcApp/Global.asax.cs
#	samples/MvcCoreApp/Program.cs
#	src/Microsoft.AspNetCore.SystemWebAdapters.SessionState/RemoteSession/RemoteAppSessionStateOptions.cs
#	src/Microsoft.AspNetCore.SystemWebAdapters/Microsoft.AspNetCore.SystemWebAdapters.csproj
Co-authored-by: Chris Ross <Tratcher@Outlook.com>
@Tratcher
Copy link
Member

Tratcher commented Jun 3, 2022

Like cookie auth, logging out from the ASP.NET Core app doesn't work (because forms auth logout does some magic that I haven't implemented on the ASP.NET Core side yet) but everything else works.

What kind of magic?

@mjrousos
Copy link
Member Author

mjrousos commented Jun 3, 2022

The LoginStatus element of the forms app (<asp:LoginStatus ID="LoginStatus1" runat="server" LogoutAction="Redirect" LogoutPageUrl="~/Logout.aspx" />) generates a logout link that logs the user out via a post back (__doPostBack('ctl00$LoginStatus1$ctl00','')).

I expect the user could mimic this action from the ASP.NET Core app, but my guess is they'll need to make a request that looks like a post back and I haven't dug into what all that entails.

Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems workable but I have a few concerns.

  • This makes sense for things like forms auth, but how do people eventually migrate off of forms auth after their content has moved over?
  • Doing JWT this way is a lot less efficient and more complicated than setting it up separately in each app. While it may be possible, I don't think we should encourage it. We should create a sample showing how JWT can be set up separately in each app.

@mjrousos
Copy link
Member Author

mjrousos commented Jun 6, 2022

Thanks, @Tratcher. Those are good questions. I agree that this isn't going to be the right approach for all auth scenarios (JWT can be done better pretty easily and we can potentially provide some code to help with cookies). I think this is a useful first step to getting auth working that can be used as a fallback for most scenarios (everything except Windows auth, I think) until we have a better solution or better guidance available.

As far as how to migrate off of it, that's a good point. I think we'll probably want a follow-up feature that allows users to authenticate in the ASP.NET Core app and forward the identity to the ASP.NET app (so, the reverse of what's happening here). I've already prototyped that approach and it works. I realize that doesn't make migrating off of forms auth any easier - that's a separate problem - but it does enable a user to make that transition without having to have all their endpoints migrated to ASP.NET Core first. That can be a separate PR, though, and I think this current one is the higher priority based on a principle of getting people started with .NET 6 as quickly and painlessly as possible.

@Tratcher
Copy link
Member

Tratcher commented Jun 8, 2022

As far as how to migrate off of it, that's a good point. I think we'll probably want a follow-up feature that allows users to authenticate in the ASP.NET Core app and forward the identity to the ASP.NET app (so, the reverse of what's happening here).

I don't know how many people will want that given they'll want to limit changes/investments in the old app. Have any customers we've interviewed expressed interest in that?

I realize that doesn't make migrating off of forms auth any easier - that's a separate problem

This is the main thing we need guidance for, we can't strand people in this hybrid state.

@mjrousos
Copy link
Member Author

mjrousos commented Jun 8, 2022

This is the main thing we need guidance for, we can't strand people in this hybrid state.

Agreed. Figuring out how to migrate off of forms auth is orthogonal to this PR, though, since this is just about bridging the gap so that the apps can share auth while endpoints migrate over gradually. Having a path forward for folks who used forms auth will be important, though, whether a user is taking advantage of these tools or just migrating all at once on their own.

@mjrousos mjrousos merged commit 56226bd into main Jun 8, 2022
@mjrousos mjrousos deleted the mikerou/remote-auth branch June 8, 2022 20:33
@CZEMacLeod
Copy link
Contributor

As far as how to migrate off of it, that's a good point. I think we'll probably want a follow-up feature that allows users to authenticate in the ASP.NET Core app and forward the identity to the ASP.NET app (so, the reverse of what's happening here).

I don't know how many people will want that given they'll want to limit changes/investments in the old app. Have any customers we've interviewed expressed interest in that?

I realize that doesn't make migrating off of forms auth any easier - that's a separate problem

This is the main thing we need guidance for, we can't strand people in this hybrid state.

I can state for a fact, that I would be interested in moving the auth to core, and forwarding to my asp.net app.
There are a number of improvements in core auth, and leaving it to last (e.g. move everything but auth to core, so that is all the asp.net app is doing then moving that) seems a bad way of progressing to me.

You already have to make changes to your old app (to handle session state etc.), so I don't see anything wrong with adding something else to provide auth sharing. I think there is already some stuff to handle this in Microsoft.Owin.Security.Interop with the DataProtectorShim, AspNetTicketDataFormat, and ChunkingCookieManager - although that probably only applies if you are using OWIN on both sides.

The only other thing I would say, is that being able to migrate my customized Microsoft.AspNet.Identity.Core / Microsoft.AspNet.Identity.Owin implementation without breaking my database format etc. would be a major help - but probably outside the scope of this project.

@davidfowl
Copy link
Member

@CZEMacLeod That's one customers opinion 😄, we have many of them at different stages of migration and they not be as willing to risk changing the old app up front.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auth: Cookie auth
5 participants