Conversation
…when an API key is absent
# Conflicts: # Microsoft.AspNetCore.SystemWebAdapters.sln # test/Microsoft.AspNetCore.SystemWebAdapters.Framework.Tests/Microsoft.AspNetCore.SystemWebAdapters.Framework.Tests.csproj
twsouthwick
left a comment
twsouthwick
left a comment
There was a problem hiding this comment.
LGTM. I think we'll want @Tratcher to review as well.
twsouthwick
left a comment
twsouthwick
left a comment
There was a problem hiding this comment.
Ooh this seems much more straightforward. LGTM!
…teAppAuthenticationHttpHandler.Framework.cs Co-authored-by: Chris Ross <[email protected]>
This was removed when we were considering a global return url query string in all authenticate flows but now that we're not using that approach, this needs added back.
# Conflicts: # src/Microsoft.AspNetCore.SystemWebAdapters/Authentication/RemoteAppAuthenticationModule.Framework.cs
|
While testing this, I found that the OWIN property approach worked well for OIDC but broke Identity/cookie auth scenarios. In OIDC scenarios, the .redirect property was used to know which URL to re-direct the user back to after authentication completes. However, in cookie scenarios, the .redirect property is being used to determine which URL to re-direct the user to in order to authenticate! The end-result is that the OWIN property-based change that fixed the OIDC scenario broke cookie scenarios because the URL the user was ultimately trying to get to was set as .redirect and when the OWIN cookie auth handler tried to authenticate the user, it would send them to that URL to sign in causing an infinite loop. I spent a while trying to figure out if there was a clean way of determining in our HTTP handler which OWIN auth handler was being used (OIDC, cookie, or something else) so that we could change what we set .redirect to accordingly but I couldn't find a good way to do that. Consequently, I've reverted back to an early approach where the relative path that the user is trying to reach is included as a query string in the ASP.NET Core's call to /authenticate to determine user identity. The ASP.NET Core app also includes a special header indicating that the request is from the ASP.NET Core app for purposes of authentication. All of the OWIN auth flows I've tested will eventually redirect back to the URL where authentication began (/authenticate in this case) and will preserve the query strings but not the headers. This allows the /authenticate HTTP handler to recognize when it's being called as part of a redirect in the auth process (the special "I'm authenticating" header will be missing). In those situations, it redirects back to the path in the query string (relative to the ASP.NET Core app's host) and the user is returned to the path they expected to be at after authenticating. I've already chatted offline with @Tratcher about this approach, but given that the code is different from how it was approved previously, I'd appreciate another round of reviews, @Tratcher and @twsouthwick. Thanks! |
twsouthwick
left a comment
twsouthwick
left a comment
There was a problem hiding this comment.
This seems fine. I'm glad it's now a relative path - that seems a lot better.
…teAppAuthenticationService.cs Co-authored-by: Taylor Southwick <[email protected]>
| var redirect = new Uri(context.Request.Url, originalUrlPath); | ||
| context.Response.Headers["Location"] = redirect.ToString(); |
There was a problem hiding this comment.
Location can be relative. That's less likely to break.
| var redirect = new Uri(context.Request.Url, originalUrlPath); | |
| context.Response.Headers["Location"] = redirect.ToString(); | |
| context.Response.Headers["Location"] = originalUrlPath |
There are two validations you might want to do before redirecting:
- originalUrlPath is not null or empty.
- originalUrlPath starts with '/'
Otherwise return a 4xx.
# Conflicts: # src/Microsoft.AspNetCore.SystemWebAdapters/Adapters/Authentication/AuthenticationConstants.cs # src/Microsoft.AspNetCore.SystemWebAdapters/Authentication/AuthenticationConstants.Shared.cs # src/Microsoft.AspNetCore.SystemWebAdapters/Authentication/AuthenticationConstants.cs
3 participants
Some auth scenarios (WS-Fed, OIDC) were having issues with remote app auth because, as part of authentication, they redirected to an external auth provider to log the user in. When the auth provider redirected back to the app, it would re-direct back to the ASP.NET app's /systemweb-adapters/authenticate endpoint (which the ASP.NET Core app calls to in order to authenticate the user) instead of to the actual endpoint in the ASP.NET Core app that the user was originally trying to get to.
This PR fixes that by including the original request URL as a query string parameter when calling the authenticate endpoint (for example: https://localhost:4321/systemweb-adapters/authenticate?original-url=http://localhost:8765/SomeController/SomeAction (except that the original-url value is URL encoded)). Then, when the ASP.NET app handles request to /systemweb-adapters/authenticate, if no API key is provided (this is only present when called from the ASP.NET Core app), it will assume that the request may be a redirect back and if an original-url query string is present (the auth flows I tested all preserved the query string), the ASP.NET app will re-direct the request to that URL instead.
This PR also adds an OIDC sample to demonstrate this scenario working.
Fixes #72