Additions for OpenIdConnectMiddleware and OAuthBearer Beta1. #112
Conversation
|
|
||
| app.UseCookieAuthentication(options => | ||
| { | ||
| options.AuthenticationType = OpenIdConnectAuthenticationDefaults.AuthenticationType; |
There was a problem hiding this comment.
These auth types are all setup wrong.
There was a problem hiding this comment.
Why is that? What are you suggesting they be?
There was a problem hiding this comment.
CookieAuthenticationDefaults.AuthenticationType?
Or you can simply omit setting the property.
There was a problem hiding this comment.
Not blocking, let's think about that later.
There was a problem hiding this comment.
And the sample is working as expected?
If it is, that's really sad: IMO, having multiple authentication handlers configured with the same AuthenticationType is really bad and should trigger an exception somewhere in Microsoft.AspNet.Security as it can't be a valid scenario (here, the AuthenticationType is shared with the OIDC middleware).
IIRC, that was not the case with Katana 3, but an InvalidOperationException thrown by LINQ was thrown when you called AuthenticateAsync with an AuthenticationType shared my multiple handlers.
If @Tratcher agrees with the general concept, I suggest opening a new ticket to track that.
There was a problem hiding this comment.
We added some validation in the new stack for things like challenging for a missing auth type, but there's not currently any checks for duplicates. Duplicates are not supported and will cause strange side-effects with APIs like SignIn and Challenge.
The correct config is:
ExternalAuthenticationOptions - Use CookieAuthenticationDefaults.AuthenticationType
UseCookieAuthentication - Don't set anything, let it use the default
UseOpenIdConnectAuthentication - Don't set SignInAsAuthenticationType or AuthenticationType, let it use the defaults
|
The file and folder names across this whole commit use inconsistent casing of Openid, OpenID, or OpenId. Pick one. |
|
I have several questions about bearer token, ping me when you have a minute to go over it. |
Added tests for OAuthBearer, OpenIdConnect
| && Request.ContentType.StartsWith("application/x-www-form-urlencoded", StringComparison.OrdinalIgnoreCase) | ||
| && Request.Body.CanRead) | ||
| { | ||
| if (!Request.Body.CanSeek) |
There was a problem hiding this comment.
ReadFormAsync now has built in buffering, you don't need this anymore.
There was a problem hiding this comment.
Removed and changes.
There was a problem hiding this comment.
Why is the buffering still showing up here?
There was a problem hiding this comment.
That's because I didn't know about that new feature. I thought you were speaking to just using the async.
1. Change ns to OAuthBearer 2. Tests for notifications 3. Remove 'Challenge' 4. Notification M...> changed to HttpContext ConfigurationManager can be null
| using Microsoft.AspNet.Builder; | ||
| using Microsoft.AspNet.Security.DataHandler; | ||
| using Microsoft.AspNet.Security.DataProtection; | ||
| using Microsoft.AspNet.Security.Infrastructure; | ||
| using Microsoft.Framework.Logging; | ||
| using Microsoft.Framework.OptionsModel; | ||
| using System; |
There was a problem hiding this comment.
Sure found the magic option.
|
Still waiting for the simple bearer token sample that doesn't use metadata or anything, it just plugs in its own token reader. |
| "aspnetcore50": { } | ||
| }, | ||
| "commands": { | ||
| "web": "Microsoft.AspNet.Hosting server=Microsoft.AspNet.Server.WebListener server.urls=http://localhost:12345", |
There was a problem hiding this comment.
nit: All of our other samples use 5001 as port for weblistener. Yeah but does not matter though.
…ache is available. OAuthBearer - added Challenge Response for OAuthBearer, removed cookieMiddleware.
| @@ -45,18 +45,22 @@ public class OAuthAuthenticationMiddleware<TOptions, TNotifications> : Authentic | |||
| { | |||
| throw new ArgumentException(string.Format(CultureInfo.CurrentCulture, Resources.Exception_OptionMustBeProvided, "AuthenticationType")); | |||
| /// <returns></returns> | ||
| protected override async Task ApplyResponseChallengeAsync() | ||
| { | ||
| if ((Response.StatusCode != 401) || (ChallengeContext == null)) |
|
Merged. |
OpenIdConnect and OAuthBearer modifications.