Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Bug] AddProtectedWebApiCallsProtectedWebApi overwrites existing OnTokenValidated event handler #154

Closed
jg11jg opened this issue May 10, 2020 · 2 comments
Closed

[Bug] AddProtectedWebApiCallsProtectedWebApi overwrites existing OnTokenValidated event handler #154

jg11jg opened this issue May 10, 2020 · 2 comments
Labels
bug Something isn't working fixed P1
Milestone
0.1.3-preview

Comments

@jg11jg
Copy link
Contributor

jg11jg commented May 10, 2020

The AddProtectedWebApiCallsProtectedWebApi method registers an event handler for OnTokenValidated, but without preserving any existing registered event handler. As a side effect, any existing event registration gets lost, for example, the OnTokenValidated event handler registered in AddProtectedWebApi(), and indeed the OnTokenValidated event handler in JwtBearerMiddlewareDiagnostics (if subscribed).

I believe this is a bug.

microsoft-identity-web/src/Microsoft.Identity.Web/WebApiServiceCollectionExtensions.cs

Line 127 in 3151e1c

options.Events.OnTokenValidated = async context =>

jg11jg added a commit to jg11jg/microsoft-identity-web that referenced this issue May 11, 2020
@jg11jg
fix: AddPr..WebApiCallPr..WebApi overwrites existing OnTokenValidated…
935deae 
… 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
@jg11jg jg11jg mentioned this issue May 11, 2020
Merged
@jmprieur jmprieur added bug Something isn't working P1 labels May 11, 2020
@jmprieur jmprieur added this to the 0.1.3-preview milestone May 11, 2020
@jennyf19 jennyf19 added the fixed label May 11, 2020
@jennyf19
Copy link
Collaborator

automatically closed, will reopen until it's in the release

@jennyf19 jennyf19 reopened this May 11, 2020
jmprieur added a commit that referenced this issue May 12, 2020
@jmprieur
- Removing a useless instruction (as pointed to by @jg11jg in #154)
f0d5593 
- Regenerating the XML comments
@jmprieur jmprieur mentioned this issue May 12, 2020
Merged
jmprieur added a commit that referenced this issue May 12, 2020
@jmprieur
Removing a useless instruction and generating XML comments (#156)
09ee45a 
* - Removing a useless instruction (as pointed to by @jg11jg in #154)
- Regenerating the XML comments

* Fixing a couple of static analysis warnings
@pmaytak pmaytak mentioned this issue May 14, 2020
Merged
@jennyf19
Copy link
Collaborator

Included in 0.1.3-preview release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working fixed P1
Projects
None yet
Milestone
0.1.3-preview
Development

No branches or pull requests
3 participants
@jmprieur @jennyf19 @jg11jg