-
Notifications
You must be signed in to change notification settings - Fork 9
split middleware, so ClaimsEnrichment middleware can be implemented #80
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
base: master
Are you sure you want to change the base?
split middleware, so ClaimsEnrichment middleware can be implemented #80
Conversation
…etween AuthN and AuthZ
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.
Can we move this to a new project? Darkloop.Azure.Functions.Authentication.Isolated
right now we don't have a need for any abstractions.
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.
please make sure to keep default indentation of the rest of the project. 4 spaces
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.
Please take a look at these items
|
||
var authenticateFeature = context.Features.Get<IAuthenticateResultFeature>(); | ||
|
||
var authenticateResult = authenticateFeature?.AuthenticateResult ?? |
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.
This is the only statement that really should change in here. Everything else should remain the same
/// </summary> | ||
public static class FunctionsAuthorizationWorkerAppBuilderExtensions | ||
/// <param name="builder">The current builder.</param> | ||
public static IFunctionsWorkerApplicationBuilder UseFunctionsAuth(this IFunctionsWorkerApplicationBuilder builder) |
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 don't think this should be a thing. We want to follow aspnetcore approach.
If user wants to do authentication separate, then explicitly call UseFunctionsAuthentication
. Otherwise UseFunctionsAuthorization
should execute both.
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.
We should separate extension methods, each one in their own assembly
@artmasa Not sure why you used a MS namespace here. it's not needed, extension methods can be in any namespace....LMK if you want me to change it to match the folder structure. |
…hrow errors on missing HttpContext
This is so that when you initialize your app these extension methods just pop-up. If I don't use this namespace, these methods require you to add the namespace of this class in order for them to show up. Some editors look into all referenced assemblies and suggest the methods, but this is not the case for every editor. |
Understood. I've never seen a developer use a Microsoft namespace before, just for that reason. Personal preference. |
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.
thanks for the changes, take a look at the latest ones.
Also ensure tests pass and set the IsPreview
flag to true in the \.build\release.props
file.
using System.Threading.Tasks; | ||
using Microsoft.AspNetCore.Http.Extensions; | ||
|
||
namespace DarkLoop.Azure.Functions.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.
please let's stick to a convention for the whole solution. Let's add braces for namespaces, since I don't think we want to go through every file and change to this format.
IPolicyEvaluator policyEvaluator, | ||
IOptionsMonitor<FunctionsAuthorizationOptions> configOptions, | ||
ILogger<FunctionsAuthorizationMiddleware> logger) | ||
IFunctionsAuthorizationProvider authorizationProvider, |
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.
let's reset indentation on these parameters
context.Features.Set<IAuthenticateResultFeature>(authenticateFeature); | ||
context.Features.Set<IHttpAuthenticationFeature>(authenticateFeature); | ||
|
||
if (filter.AllowAnonymous) |
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 check removed? We still need to allow for execution of an action regardless of authentication result if it's marked with AllowAnonymous
|
||
// We also make the features available in the FunctionContext | ||
context.Features.Set<IAuthenticateResultFeature>(authenticateFeature); |
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.
If user does not configure authentication middleware the feature needs to be set. We don't want to break this functionality. This is the same way it happens in ASPNET Core
return; | ||
} | ||
|
||
if (authenticateResult is not null && !authenticateResult.Succeeded) |
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.
this section shouldn't be removed either
this PR: