-
Notifications
You must be signed in to change notification settings - Fork 39
Changes from all commits
de340e4
9516e86
23f3a3e
bc56c3a
2370336
bbc4675
0122711
f99755f
17418f6
b3f6351
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
{ "sdk": { "version": "2.0.0-preview1-005783" } } |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,9 +2,12 @@ | |
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Diagnostics.Contracts; | ||
using System.Linq; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Microsoft.AspNetCore.Authentication; | ||
using Microsoft.AspNetCore.Hosting.Server; | ||
using Microsoft.AspNetCore.Hosting.Server.Features; | ||
using Microsoft.AspNetCore.Http.Features; | ||
|
@@ -30,7 +33,7 @@ internal class MessagePump : IServer | |
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For this to work UseHttpSysServer has to register the service? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well maybe AddAuthentication needs to be added as part of hosting similar to options, its kinda required... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's required for HttpSysServer, but only if auth is enabled. Same for IIS Integration. There are plenty of apps that don't need it though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably need to factor the new auth stuff into the 'core stuff' (and the security base class stuff). Hosting should add the core stuff so everyone can add schemes... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolve it as optional? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this mimics how anyone could hook their own IAuthenticationHandler on httpContext today, its always there today There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well do we really want context.AuthenticateAsync to throw due to no service found if AddAuthentication isn't called? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds fine. Why Authenticate without Authentication? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok so we are leaving it as an exercise for the user to ensure something is calling AddAuthentication, MVC/identity will do this, as well as all of our security projects There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated to resolve IAuthenticationSchemeProvider as enumerable and only throw if AuthentionSchemes for the server options is not none. |
||
private readonly ServerAddressesFeature _serverAddresses; | ||
|
||
public MessagePump(IOptions<HttpSysOptions> options, ILoggerFactory loggerFactory) | ||
public MessagePump(IOptions<HttpSysOptions> options, ILoggerFactory loggerFactory, IEnumerable<IAuthenticationSchemeProvider> authentication) | ||
{ | ||
if (options == null) | ||
{ | ||
|
@@ -40,10 +43,21 @@ public MessagePump(IOptions<HttpSysOptions> options, ILoggerFactory loggerFactor | |
{ | ||
throw new ArgumentNullException(nameof(loggerFactory)); | ||
} | ||
|
||
_options = options.Value; | ||
Listener = new HttpSysListener(_options, loggerFactory); | ||
_logger = LogHelper.CreateLogger(loggerFactory, typeof(MessagePump)); | ||
|
||
if (_options.Authentication.Schemes != AuthenticationSchemes.None) | ||
{ | ||
var auth = authentication.FirstOrDefault(); | ||
if (auth == null) | ||
{ | ||
throw new InvalidOperationException("AddAuthentication() is required to use Authentication."); | ||
} | ||
|
||
auth.AddScheme(new AuthenticationScheme("Windows", displayName: null, handlerType: typeof(AuthenticationHandler))); | ||
} | ||
|
||
Features = new FeatureCollection(); | ||
_serverAddresses = new ServerAddressesFeature(); | ||
Features.Set<IServerAddressesFeature>(_serverAddresses); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comments about !Accepted aren't true anymore. SignIn should throw NotSupportedException. SignOut is something we're discussing elsewhere.