-
Notifications
You must be signed in to change notification settings - Fork 894
Enable authorization for proxy routes #265
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
Conversation
{ | ||
endpointBuilder.Metadata.Add(AnonymousAuthorization); | ||
} | ||
else if (!string.IsNullOrEmpty(source.Authorization)) |
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.
Is there a way to add multiple Policy to a route?
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.
No, what would that mean?
This is the same as adding an Authorize attribute on a controller.
{ | ||
endpointBuilder.Metadata.Add(AnonymousAuthorization); | ||
} | ||
else if (!string.IsNullOrEmpty(source.Authorization)) |
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.
Is there a way to set Authentication Scheme for AuthorizeAttribute?
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.
The auth schemes can be set on the policy, is that enough?
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.
A few comments.
src/ReverseProxy/Abstractions/RouteDiscovery/Contract/AuthorizationContants.cs
Outdated
Show resolved
Hide resolved
{ | ||
internal static class AuthorizationContants | ||
{ | ||
internal const string Default = "Default"; |
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.
I believe it's better to set public
visibility on type's member if the type itself is scoped internal
.
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.
Why? You get the same behavior either way.
src/ReverseProxy/Abstractions/RouteDiscovery/Contract/ProxyRoute.cs
Outdated
Show resolved
Hide resolved
src/ReverseProxy/Abstractions/RouteDiscovery/Contract/AuthorizationConstants.cs
Outdated
Show resolved
Hide resolved
using Microsoft.Extensions.DependencyInjection; | ||
using Microsoft.Extensions.Options; | ||
|
||
namespace Microsoft.ReverseProxy.Auth.Sample |
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.
Why is this sample doing auth with MVC?
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.
Because I needed something visual and self-contained.
Or do you mean I should use razor pages instead?
hash ^= ClusterId.GetHashCode(); | ||
} | ||
|
||
if (!string.IsNullOrEmpty(AuthorizationPolicy)) |
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.
I just looked at this hash function and realized how crazy expensive this is. How often is it used? Not very I hope.
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.
Once per config reload.
#122 This enables proxy routes to integrate with the existing ASP.NET Core authentication and authorization infrastructure.
This should unblock most users. I expect some feedback on the scenario for flowing user information to the destination server, we'll need to wait and see what people's scenarios are.