-
Notifications
You must be signed in to change notification settings - Fork 184
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
AuthN Config V2 -> HotReload Aware Authentication settings in dev mode. #2414
base: main
Are you sure you want to change the base?
Conversation
…ider stub with authN settings populated now that ClientRoleHeaderAuthenticationMiddleware queries the runtimeconfigprovider for every request to determine authentication scheme to authenticate with.
…efaulted properly. Also updated engine to properly set runtimeconfigprovider.IsLateConfigured == true in the event handler registration conditioned on dab not starting with a runtimeconfig. THat way in the clientroleheaderauthenticationmiddleware, we now check whether config is late bound and set SWA auth appropriately.
/azp run |
src/Core/AuthenticationHelpers/AuthenticationSimulator/SimulatorAuthenticationHandler.cs
Show resolved
Hide resolved
src/Core/AuthenticationHelpers/EasyAuthAuthenticationBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Core/AuthenticationHelpers/EasyAuthAuthenticationBuilderExtensions.cs
Show resolved
Hide resolved
/azp run |
/// <seealso cref="https://github.com/dotnet/aspnetcore/issues/21491#issuecomment-624240160">Guidance for registering named options.</seealso> | ||
private static void ConfigureAuthenticationV2(IServiceCollection services, RuntimeConfigProvider runtimeConfigProvider) | ||
{ | ||
services.AddSingleton<IOptionsChangeTokenSource<JwtBearerOptions>>(new JwtBearerOptionsChangeTokenSource(runtimeConfigProvider)); |
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.
In V1, we added authentication conditionally. But in V2, we are adding all schemes unconditionally. Is there benefit in keeping V1 around? If not, to keep the code base simple, is it better to do V2 irrespective of whether its a hot reload scenario or not?
@@ -61,6 +89,9 @@ protected void SendEventNotification(string message = "") | |||
OnConfigChangedEvent(new HotReloadEventArgs(MYSQL_QUERY_EXECUTOR_ON_CONFIG_CHANGED, message)); | |||
OnConfigChangedEvent(new HotReloadEventArgs(POSTGRESQL_QUERY_EXECUTOR_ON_CONFIG_CHANGED, message)); | |||
OnConfigChangedEvent(new HotReloadEventArgs(DOCUMENTOR_ON_CONFIG_CHANGED, message)); | |||
|
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.
nit: rename to indicate this function also handles signaling the change token. Currently, it says send event notification only.
// https://learn.microsoft.com/aspnet/core/security/authorization/limitingidentitybyscheme?view=aspnetcore-8.0 | ||
if (authNResult.Succeeded) | ||
{ | ||
httpContext.User = authNResult.Principal; |
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 setup of httpContext.User is the only required change for hot reload in this file,right? Is my understanding correct that the rest of the changes in this file are good to have but not really relevant to hot reload?
I dont mind the changes but trying to understand how the value of scheme affects hot reload..
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 need all the code in this file and on lines that precede this code. The following line is important because we must pass scheme to authenticateAsync to ensure the proper authentication handler is used. Scheme is determined using the runtime config. If the provider changes in runtime config, DAB must resolve that change and ensure the request is authenticated using the hotreloaded scheme.
await httpContext.AuthenticateAsync(scheme);
private readonly RuntimeConfigProvider _configProvider; | ||
|
||
/// <summary> | ||
/// Get RuntimeConfigProvider to use as the change event source. |
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.
/// Get RuntimeConfigProvider to use as the change event source. | |
/// Get RuntimeConfigProvider to use as the change token source. |
@@ -4,14 +4,27 @@ | |||
namespace Azure.DataApiBuilder.Core.AuthenticationHelpers; | |||
|
|||
/// <summary> | |||
/// Default values related to EasyAuthAuthentication handler. | |||
/// EasyAuth authentication scheme names granularized by provider | |||
/// to enable compatility with HotReloading authentication settings. |
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.
/// to enable compatility with HotReloading authentication settings. | |
/// to enable compatibility with HotReloading authentication settings. |
@@ -41,17 +41,24 @@ public static class WebHostBuilderHelper | |||
/// - DAB's Simulator/ EasyAuth authentication middleware and ClientRoleHeader middleware | |||
/// - dotnet's authorization middleware. | |||
/// </summary> | |||
/// <param name="provider">Runtime configured identity provider name.</param> | |||
/// <param name="provider">Runtime configured identity provider name. This is different that authentication scheme name because |
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.
/// <param name="provider">Runtime configured identity provider name. This is different that authentication scheme name because | |
/// <param name="provider">Runtime configured identity provider name. This is different than authentication scheme name because |
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.
LGTM, left few nits and questions. Thanks for this change.
case nameof(EasyAuthType.StaticWebApps): | ||
return EasyAuthAuthenticationDefaults.SWAAUTHSCHEME; | ||
case "AzureAD": | ||
case "EntraID": |
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.
should we make these constants and use case-insensitive matching?
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.
LGTM, thanks for the quick update and clarifying doubts.
Why make this change?
What is this change?
The following changes are only honored in development mode when testing locally using "Hot Reload"
ConfigureAuthenticationV2()
in startup.cs which wires up all DAB supported Authentication providers:How was this tested?
Scenario 1 - Swap JWT Provider Details
Mode: Development
Scenario 2 - Swap from JWT to Simulator
Mode: Development
Usage of ChangeTokens versus EventHandler in this specific workstream:
The objective was to trigger the
IOptionsMonitor
within theJwtBearerHandler
to detect changes to authentication programmatically (as opposed to the out-of-box change detection for a bound configuration file such asappsettings.json
, which we don't use). In order to alert theIOptionsMonitor
that a change has occurred, I needed to register:which is this resolved via DI in the
OptionsMonitor
class:https://github.com/dotnet/runtime/blob/88397049d6aa2c8505bd11309ddd314169e3f73f/src/libraries/Microsoft.Extensions.Options/src/OptionsMonitor.cs#L31-L53
once our
runtimeconfigloader
detects a file change, it signals the provider that newruntimeconfig
is available. Then theRuntimeConfigProvider
(which is the service resolved via DI in most of our classes already) can signal the change token whose listener is theOptionsMonitor
via registering theIOptionsChangeTokenSource
.This mechanism was not as intrusive to our services' constructors:
IOptionsMonitorCache<jwtBearerOptions>
in ourRuntimeConfigProvider
orRuntimeConfigLoader
class constructors. Updating theoptionsmonitorcache
would be( i tested this) the manual way of changingIOptionsMonitor<JwtBearerOptions>
used by theJwtBearerHandler
, but this didn't work because of the next bullet point:2. I made a judgement call that injecting
IoptionsmonitorCache<jwtBearerOptions>
into either of the two referenced classes seemed to leak implementation details of the provider/loader and wouldn't be straightforward without more time consuming and costly refactors:- Because both the provider/loader are manually instantiated in
Startup::ConfigureServices
and don't have the opportunity to resolve services via DI.- Because we'd then need to make even more cascading changes in the test projects to accommodate setting up the
ioptionsmonitorcache
object to then be added to our mock runtimeconfigprovider/loader objects.The event-handlers we have recently added don't fit this specific use case of updating the underlying authentication configuration. In addition to the above design decisions, I didn't identify a solution where we could subscribe the
JwtBearerHandler
'sOptionsMonitor
to an eventhandler signal to notify that a change occurred. I pursued using the built-in mechanism of change detection for optionsmonitor provided to us viaIOptionsChangeTokenSource
.