fix: #154 AddProtectedWebApiCallProtectedWebApi overwrites handler #155
Conversation
… event handler AzureAD#154 Preserving existing event hander when registering OnTokenValidated event handler in AddProtectedWebApiCallsProtectedWebApi. Previous code was overwriting existing event handler meaning that logging via JwtBearerMiddlewareDiagnostics would no longer work and perhaps more seriously the OnTokenValidated event handler registered in AddProtectedWebApi (intended to "This check is required to ensure that the Web API only accepts tokens from tenants where it has been consented and provisioned.") AzureAD#154
|
Thanks @jg11jg. Good catch! |
src/Microsoft.Identity.Web/WebApiServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
| options.Events ??= new JwtBearerEvents(); | ||
|
|
||
| var onTokenValidatedHandler = options.Events.OnTokenValidated; | ||
|
|
||
| options.Events.OnTokenValidated = async context => |
There was a problem hiding this comment.
@jmprieur Related to your suggestion we can just += our handler to the existing ones, right?
| options.Events ??= new JwtBearerEvents(); | |
| var onTokenValidatedHandler = options.Events.OnTokenValidated; | |
| options.Events.OnTokenValidated = async context => | |
| options.Events ??= new JwtBearerEvents(); | |
| options.Events.OnTokenValidated += async context => |
There was a problem hiding this comment.
possibly but the convention within the rest of the library is to do it as I originally did it (in fact I have used exactly the same variable names to be consistent with the "house style". e.g:
Possibly the += syntax requires null checks or similar? Also in the *MiddlewareDiagnostics classes += is not used. So I (personally) would stick with "house style" (or change it everywhere if += truly better).
There was a problem hiding this comment.
I agree with @jg11jg, we should keep things consistent. I would leave as-is and we can update things later if we want.
Co-authored-by: Jean-Marc Prieur <jmprieur@microsoft.com>
Code now preserves existing event handler when registering OnTokenValidated event handler in AddProtectedWebApiCallsProtectedWebApi. Previous code was overwriting existing event handler meaning that logging via JwtBearerMiddlewareDiagnostics would no longer work and perhaps more seriously the OnTokenValidated event handler registered in AddProtectedWebApi (intended to "ensure that the Web API only accepts tokens from tenants where it has been consented and provisioned.")
#154