-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
#842 AuthenticationOptions in GlobalConfiguration #1215
Changes from all commits
928b24a
8c7866f
06354dc
0bf90a1
bc12ee4
a1b4c7a
78a70dc
eb4e3f8
645b827
c99d512
e8af97f
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 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,12 +1,18 @@ | ||||||||||||
using Ocelot.Configuration.File; | ||||||||||||
|
||||||||||||
using System.Linq; | ||||||||||||
|
||||||||||||
namespace Ocelot.Configuration.Creator | ||||||||||||
{ | ||||||||||||
public class AuthenticationOptionsCreator : IAuthenticationOptionsCreator | ||||||||||||
{ | ||||||||||||
public AuthenticationOptions Create(FileRoute route) | ||||||||||||
{ | ||||||||||||
return new AuthenticationOptions(route.AuthenticationOptions.AllowedScopes, route.AuthenticationOptions.AuthenticationProviderKey); | ||||||||||||
} | ||||||||||||
public AuthenticationOptions Create(FileAuthenticationOptions routeAuthOptions, | ||||||||||||
FileAuthenticationOptions globalConfAuthOptions) | ||||||||||||
Comment on lines
+8
to
+9
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.
Suggested change
Or, Don't make new line at all. Having 2-3 parameters in signature on the same line is totally fine. |
||||||||||||
{ | ||||||||||||
var routeAuthOptionsEmpty = string.IsNullOrEmpty(routeAuthOptions.AuthenticationProviderKey); | ||||||||||||
|
||||||||||||
var resultAuthOptions = routeAuthOptionsEmpty ? globalConfAuthOptions : routeAuthOptions; | ||||||||||||
|
||||||||||||
return new AuthenticationOptions(resultAuthOptions.AllowedScopes, resultAuthOptions.AuthenticationProviderKey); | ||||||||||||
} | ||||||||||||
} | ||||||||||||
} | ||||||||||||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -5,9 +5,9 @@ namespace Ocelot.Configuration.Creator | |||||||||||||||||||||
{ | ||||||||||||||||||||||
public class RouteOptionsCreator : IRouteOptionsCreator | ||||||||||||||||||||||
{ | ||||||||||||||||||||||
public RouteOptions Create(FileRoute fileRoute) | ||||||||||||||||||||||
public RouteOptions Create(FileRoute fileRoute, FileGlobalConfiguration globalConfiguration) | ||||||||||||||||||||||
{ | ||||||||||||||||||||||
var isAuthenticated = IsAuthenticated(fileRoute); | ||||||||||||||||||||||
var isAuthenticated = IsAuthenticated(fileRoute, globalConfiguration); | ||||||||||||||||||||||
var isAuthorized = IsAuthorized(fileRoute); | ||||||||||||||||||||||
var isCached = IsCached(fileRoute); | ||||||||||||||||||||||
var enableRateLimiting = IsEnableRateLimiting(fileRoute); | ||||||||||||||||||||||
|
@@ -26,7 +26,12 @@ public RouteOptions Create(FileRoute fileRoute) | |||||||||||||||||||||
|
||||||||||||||||||||||
private static bool IsEnableRateLimiting(FileRoute fileRoute) => fileRoute.RateLimitOptions?.EnableRateLimiting == true; | ||||||||||||||||||||||
|
||||||||||||||||||||||
private static bool IsAuthenticated(FileRoute fileRoute) => !string.IsNullOrEmpty(fileRoute.AuthenticationOptions?.AuthenticationProviderKey); | ||||||||||||||||||||||
private static bool IsAuthenticated(FileRoute fileRoute, FileGlobalConfiguration globalConfiguration) | ||||||||||||||||||||||
{ | ||||||||||||||||||||||
return (!string.IsNullOrEmpty(globalConfiguration?.AuthenticationOptions?.AuthenticationProviderKey) && | ||||||||||||||||||||||
!fileRoute.AuthenticationOptions.AllowAnonymousForGlobalAuthenticationOptions) || | ||||||||||||||||||||||
!string.IsNullOrEmpty(fileRoute.AuthenticationOptions?.AuthenticationProviderKey); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
Comment on lines
+29
to
+34
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. Because the body consists of one operator only, why not to define via expression body 👇
Suggested change
|
||||||||||||||||||||||
|
||||||||||||||||||||||
private static bool IsAuthorized(FileRoute fileRoute) => fileRoute.RouteClaimsRequirement?.Count > 0; | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,16 @@ public FileAuthenticationOptions() | |
} | ||
|
||
public string AuthenticationProviderKey { get; set; } | ||
public List<string> AllowedScopes { get; set; } | ||
public List<string> AllowedScopes { get; set; } | ||
|
||
/// <summary> | ||
/// Allows anonymous authentication globally. | ||
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 allows per route actually. |
||
/// <para>The property is significant only if the global AuthenticationOptions are used.</para> | ||
/// </summary> | ||
/// <value> | ||
/// <see langword="true"/> if it is allowed; otherwise, <see langword="false"/>. | ||
/// </value> | ||
public bool AllowAnonymousForGlobalAuthenticationOptions { get; set; } | ||
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. The name could be shorter, like that: |
||
|
||
public override string ToString() | ||
{ | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,36 @@ | ||||||||||
using FluentValidation; | ||||||||||
using Microsoft.AspNetCore.Authentication; | ||||||||||
using Ocelot.Configuration.File; | ||||||||||
using System.Linq; | ||||||||||
using System.Threading; | ||||||||||
using System.Threading.Tasks; | ||||||||||
|
||||||||||
namespace Ocelot.Configuration.Validator; | ||||||||||
|
||||||||||
public class FileAuthenticationOptionsValidator : AbstractValidator<FileAuthenticationOptions> | ||||||||||
{ | ||||||||||
private readonly IAuthenticationSchemeProvider _authenticationSchemeProvider; | ||||||||||
|
||||||||||
public FileAuthenticationOptionsValidator(IAuthenticationSchemeProvider authenticationSchemeProvider) | ||||||||||
{ | ||||||||||
_authenticationSchemeProvider = authenticationSchemeProvider; | ||||||||||
|
||||||||||
RuleFor(authOptions => authOptions.AuthenticationProviderKey) | ||||||||||
.MustAsync(IsSupportedAuthenticationProviders) | ||||||||||
.WithMessage("{PropertyName}: {PropertyValue} is unsupported authentication provider"); | ||||||||||
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. What placeholders are here? |
||||||||||
} | ||||||||||
|
||||||||||
private async Task<bool> IsSupportedAuthenticationProviders(string authenticationProviderKey, CancellationToken cancellationToken) | ||||||||||
{ | ||||||||||
if (string.IsNullOrEmpty(authenticationProviderKey)) | ||||||||||
{ | ||||||||||
return true; | ||||||||||
} | ||||||||||
|
||||||||||
var schemes = await _authenticationSchemeProvider.GetAllSchemesAsync(); | ||||||||||
|
||||||||||
var supportedSchemes = schemes.Select(scheme => scheme.Name).ToList(); | ||||||||||
|
||||||||||
return supportedSchemes.Contains(authenticationProviderKey); | ||||||||||
Comment on lines
+32
to
+34
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. Why not?
Suggested change
The goal is removal of |
||||||||||
} | ||||||||||
} |
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -5,10 +5,14 @@ namespace Ocelot.Configuration.Validator | |||||||||||
{ | ||||||||||||
public class FileGlobalConfigurationFluentValidator : AbstractValidator<FileGlobalConfiguration> | ||||||||||||
{ | ||||||||||||
public FileGlobalConfigurationFluentValidator(FileQoSOptionsFluentValidator fileQoSOptionsFluentValidator) | ||||||||||||
public FileGlobalConfigurationFluentValidator(FileQoSOptionsFluentValidator fileQoSOptionsFluentValidator, | ||||||||||||
FileAuthenticationOptionsValidator fileAuthenticationOptionsValidator) | ||||||||||||
Comment on lines
+8
to
+9
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. Each param is located on own line.
Suggested change
Or, Don't make new line. Actually parameter names could be shorter to be on the same line. |
||||||||||||
{ | ||||||||||||
RuleFor(configuration => configuration.QoSOptions) | ||||||||||||
.SetValidator(fileQoSOptionsFluentValidator); | ||||||||||||
|
||||||||||||
RuleFor(configuration => configuration.AuthenticationOptions) | ||||||||||||
.SetValidator(fileAuthenticationOptionsValidator); | ||||||||||||
} | ||||||||||||
} | ||||||||||||
} |
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.
It is better to write a new subsection of the chapter like:
before this line.
And this line is too long! Splitting phrases should help.