Skip to content

AuthorizeHelper will no-op if endpoint routing is used #10471

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

Merged
merged 12 commits into from
Jun 3, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@ public class StartupWithUseSignalR
{
public void Configure(IApplicationBuilder app)
{
#pragma warning disable CS0618 // Type or member is obsolete
app.UseSignalR(routes =>
{

});
#pragma warning restore CS0618 // Type or member is obsolete
}
}
}
Original file line number Diff line number Diff line change
@@ -1,18 +1,10 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.IdentityModel.Tokens.Jwt;
using System.Security.Claims;
using Microsoft.AspNetCore.Authentication.JwtBearer;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Connections;
using Microsoft.AspNetCore.SignalR.Protocol;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.DependencyInjection.Extensions;
using Microsoft.IdentityModel.Tokens;
using Newtonsoft.Json;

namespace Microsoft.AspNetCore.SignalR.Client.FunctionalTests
{
Expand All @@ -33,11 +25,12 @@ public void ConfigureServices(IServiceCollection services)

public void Configure(IApplicationBuilder app)
{
app.UseRouting();
app.UseAuthentication();

app.UseSignalR(routes =>
app.UseEndpoints(endpoints =>
{
routes.MapHub<VersionHub>("/version");
endpoints.MapHub<VersionHub>("/version");
});
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ public static partial class ConnectionEndpointRouteBuilderExtensions
}
public static partial class ConnectionsAppBuilderExtensions
{
[System.ObsoleteAttribute("This method is obsolete and will be removed in a future version. The recommended alternative is to use MapConnections or MapConnectionHandler<TConnectionHandler> inside Microsoft.AspNetCore.Builder.UseEndpoints(...).")]
public static Microsoft.AspNetCore.Builder.IApplicationBuilder UseConnections(this Microsoft.AspNetCore.Builder.IApplicationBuilder app, System.Action<Microsoft.AspNetCore.Http.Connections.ConnectionsRouteBuilder> configure) { throw null; }
}
}
Expand All @@ -28,6 +29,7 @@ public partial class ConnectionOptionsSetup : Microsoft.Extensions.Options.IConf
public ConnectionOptionsSetup() { }
public void Configure(Microsoft.AspNetCore.Http.Connections.ConnectionOptions options) { }
}
[System.ObsoleteAttribute("This class is obsolete and will be removed in a future version. The recommended alternative is to use MapConnection and MapConnectionHandler<TConnectionHandler> inside Microsoft.AspNetCore.Builder.UseEndpoints(...).")]
public partial class ConnectionsRouteBuilder
{
internal ConnectionsRouteBuilder() { }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@

using System;
using System.Collections.Generic;
using System.Linq;
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Connections;
using Microsoft.AspNetCore.Http.Connections;
using Microsoft.AspNetCore.Http.Connections.Internal;
Expand Down Expand Up @@ -48,20 +46,14 @@ public static IEndpointConventionBuilder MapConnectionHandler<TConnectionHandler
public static IEndpointConventionBuilder MapConnectionHandler<TConnectionHandler>(this IEndpointRouteBuilder endpoints, string pattern, Action<HttpConnectionDispatcherOptions> configureOptions) where TConnectionHandler : ConnectionHandler
{
var options = new HttpConnectionDispatcherOptions();
// REVIEW: WE should consider removing this and instead just relying on the
// AuthorizationMiddleware
var attributes = typeof(TConnectionHandler).GetCustomAttributes(inherit: true);
foreach (var attribute in attributes.OfType<AuthorizeAttribute>())
{
options.AuthorizationData.Add(attribute);
}
configureOptions?.Invoke(options);

var conventionBuilder = endpoints.MapConnections(pattern, options, b =>
{
b.UseConnectionHandler<TConnectionHandler>();
});

var attributes = typeof(TConnectionHandler).GetCustomAttributes(inherit: true);
conventionBuilder.Add(e =>
{
// Add all attributes on the ConnectionHandler has metadata (this will allow for things like)
Expand Down Expand Up @@ -93,7 +85,7 @@ public static IEndpointConventionBuilder MapConnections(this IEndpointRouteBuild
var connectionDelegate = connectionBuilder.Build();

// REVIEW: Consider expanding the internals of the dispatcher as endpoint routes instead of
// using if statemants we can let the matcher handle
// using if statements we can let the matcher handle

var conventionBuilders = new List<IEndpointConventionBuilder>();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,6 @@

using System;
using Microsoft.AspNetCore.Http.Connections;
using Microsoft.AspNetCore.Http.Connections.Internal;
using Microsoft.AspNetCore.Routing;
using Microsoft.Extensions.DependencyInjection;

namespace Microsoft.AspNetCore.Builder
{
Expand All @@ -16,25 +13,29 @@ public static class ConnectionsAppBuilderExtensions
{
/// <summary>
/// Adds support for ASP.NET Core Connection Handlers to the <see cref="IApplicationBuilder"/> request execution pipeline.
/// <para>
/// This method is obsolete and will be removed in a future version.
/// The recommended alternative is to use MapConnections or MapConnectionHandler&#60;TConnectionHandler&#62; inside Microsoft.AspNetCore.Builder.UseEndpoints(...).
/// </para>
/// </summary>
/// <param name="app">The <see cref="IApplicationBuilder"/>.</param>
/// <param name="configure">A callback to configure connection routes.</param>
/// <returns>The same instance of the <see cref="IApplicationBuilder"/> for chaining.</returns>
[Obsolete("This method is obsolete and will be removed in a future version. The recommended alternative is to use MapConnections or MapConnectionHandler<TConnectionHandler> inside Microsoft.AspNetCore.Builder.UseEndpoints(...).")]
public static IApplicationBuilder UseConnections(this IApplicationBuilder app, Action<ConnectionsRouteBuilder> configure)
{
if (configure == null)
{
throw new ArgumentNullException(nameof(configure));
}

var dispatcher = app.ApplicationServices.GetRequiredService<HttpConnectionDispatcher>();

var routes = new RouteBuilder(app);

configure(new ConnectionsRouteBuilder(routes, dispatcher));

app.UseWebSockets();
app.UseRouter(routes.Build());
app.UseRouting();
app.UseAuthorization();
app.UseEndpoints(endpoints =>
Copy link
Member Author

@BrennanConroy BrennanConroy May 24, 2019

Choose a reason for hiding this comment

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

@davidfowl
Have any thoughts on this? Converting app.UseSignalR(r => r.Map...()) to use Endpoints internally. This lets us get rid of our custom auth stuff in HttpConnectionDispatcher.

And side-effect, I believe will allow apps that port to 3.0 but don't change to endpoints to use the service automagically without UseAzureSignalR. (Once I figure out the HubMetadata issue)

Copy link
Member

Choose a reason for hiding this comment

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

This to me seems super spicy. Does it make sense to deprecate this API now that it's literally equivalent to something else? It's goodness if we can converge all of the code that we show in samples/docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would be ok deprecating the old stuff
@anurse @davidfowl ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm OK marking UseSignalR as obsolete if that's what we're talking about. We should have done that already anyway, since we want people to use UseEndpoints.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would be obsoleting:
UseConnections
ConnectionsRouteBuilder
HubRouteBuilder
UseSignalR

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good.

{
configure(new ConnectionsRouteBuilder(endpoints));
});
return app;
}
}
Expand Down
48 changes: 14 additions & 34 deletions src/SignalR/common/Http.Connections/src/ConnectionsRouteBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,27 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Reflection;
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Connections;
using Microsoft.AspNetCore.Http.Connections.Internal;
using Microsoft.AspNetCore.Routing;

namespace Microsoft.AspNetCore.Http.Connections
{
/// <summary>
/// Maps routes to ASP.NET Core Connection Handlers.
/// <para>
/// This class is obsolete and will be removed in a future version.
/// The recommended alternative is to use MapConnection and MapConnectionHandler&#60;TConnectionHandler&#62; inside Microsoft.AspNetCore.Builder.UseEndpoints(...).
/// </para>
/// </summary>
[Obsolete("This class is obsolete and will be removed in a future version. The recommended alternative is to use MapConnection and MapConnectionHandler<TConnectionHandler> inside Microsoft.AspNetCore.Builder.UseEndpoints(...).")]
public class ConnectionsRouteBuilder
{
private readonly HttpConnectionDispatcher _dispatcher;
private readonly RouteBuilder _routes;
private readonly IEndpointRouteBuilder _endpoints;

internal ConnectionsRouteBuilder(RouteBuilder routes, HttpConnectionDispatcher dispatcher)
internal ConnectionsRouteBuilder(IEndpointRouteBuilder endpoints)
{
_routes = routes;
_dispatcher = dispatcher;
_endpoints = endpoints;
}

/// <summary>
Expand All @@ -38,45 +39,24 @@ public void MapConnections(PathString path, Action<IConnectionBuilder> configure
/// <param name="path">The request path.</param>
/// <param name="options">Options used to configure the connection.</param>
/// <param name="configure">A callback to configure the connection.</param>
public void MapConnections(PathString path, HttpConnectionDispatcherOptions options, Action<IConnectionBuilder> configure)
{
var connectionBuilder = new ConnectionBuilder(_routes.ServiceProvider);
configure(connectionBuilder);
var socket = connectionBuilder.Build();
_routes.MapRoute(path, c => _dispatcher.ExecuteAsync(c, options, socket));
_routes.MapRoute(path + "/negotiate", c => _dispatcher.ExecuteNegotiateAsync(c, options));
}
public void MapConnections(PathString path, HttpConnectionDispatcherOptions options, Action<IConnectionBuilder> configure) =>
_endpoints.MapConnections(path, options, configure);

/// <summary>
/// Maps incoming requests with the specified path to the provided connection pipeline.
/// </summary>
/// <typeparam name="TConnectionHandler">The <see cref="ConnectionHandler"/> type.</typeparam>
/// <param name="path">The request path.</param>
public void MapConnectionHandler<TConnectionHandler>(PathString path) where TConnectionHandler : ConnectionHandler
{
public void MapConnectionHandler<TConnectionHandler>(PathString path) where TConnectionHandler : ConnectionHandler =>
MapConnectionHandler<TConnectionHandler>(path, configureOptions: null);
}

/// <summary>
/// Maps incoming requests with the specified path to the provided connection pipeline.
/// </summary>
/// <typeparam name="TConnectionHandler">The <see cref="ConnectionHandler"/> type.</typeparam>
/// <param name="path">The request path.</param>
/// <param name="configureOptions">A callback to configure dispatcher options.</param>
public void MapConnectionHandler<TConnectionHandler>(PathString path, Action<HttpConnectionDispatcherOptions> configureOptions) where TConnectionHandler : ConnectionHandler
{
var authorizeAttributes = typeof(TConnectionHandler).GetCustomAttributes<AuthorizeAttribute>(inherit: true);
var options = new HttpConnectionDispatcherOptions();
foreach (var attribute in authorizeAttributes)
{
options.AuthorizationData.Add(attribute);
}
configureOptions?.Invoke(options);

MapConnections(path, options, builder =>
{
builder.UseConnectionHandler<TConnectionHandler>();
});
}
public void MapConnectionHandler<TConnectionHandler>(PathString path, Action<HttpConnectionDispatcherOptions> configureOptions) where TConnectionHandler : ConnectionHandler =>
_endpoints.MapConnectionHandler<TConnectionHandler>(path, configureOptions);
}
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,6 @@ public async Task ExecuteAsync(HttpContext context, HttpConnectionDispatcherOpti
var logScope = new ConnectionLogScope(GetConnectionId(context));
using (_logger.BeginScope(logScope))
{
if (!await AuthorizeHelper.AuthorizeAsync(context, options.AuthorizationData))
{
return;
}

if (HttpMethods.IsPost(context.Request.Method))
{
// POST /{path}
Expand Down Expand Up @@ -95,11 +90,6 @@ public async Task ExecuteNegotiateAsync(HttpContext context, HttpConnectionDispa
var logScope = new ConnectionLogScope(connectionId: string.Empty);
using (_logger.BeginScope(logScope))
{
if (!await AuthorizeHelper.AuthorizeAsync(context, options.AuthorizationData))
{
return;
}

if (HttpMethods.IsPost(context.Request.Method))
{
// POST /{path}/negotiate
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
<Compile Include="$(SignalRSharedSourceRoot)WebSocketExtensions.cs" Link="WebSocketExtensions.cs" />
<Compile Include="$(SignalRSharedSourceRoot)StreamExtensions.cs" Link="StreamExtensions.cs" />
<Compile Include="$(SignalRSharedSourceRoot)DuplexPipe.cs" Link="DuplexPipe.cs" />
<Compile Include="$(SignalRSharedSourceRoot)TaskCache.cs" Link="Internal\TaskCache.cs" />
</ItemGroup>

<ItemGroup>
Expand Down
Loading