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.

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

[Bug] WebApiAuthenticationBuilderExtensions.cs should consider cloning TokenValidationParameters #241

Closed
brentschmaltz opened this issue Jun 23, 2020 · 5 comments
Assignees
Labels
bug Something isn't working fixed .NET Core 5 P1

Comments

@brentschmaltz
Copy link
Member

brentschmaltz commented Jun 23, 2020

Which Version of Microsoft Identity Web are you using ?
Note that to get help, you need to run the latest version.
Microsoft Identity Web 0.1.5-preview

Where is the issue?
AddMicrosoftWebApi uses the value of TokenValidationParameters from JwtBearerOptions. Any modifications will affect any other users of JwtBearerOptions. It would be better to use TokenValidationParameters.Clone().

@jmprieur jmprieur added .NET Core 5 bug Something isn't working P1 labels Jun 24, 2020
@jmprieur
Copy link
Collaborator

Thanks @brentschmaltz !

@pmaytak pmaytak self-assigned this Jun 30, 2020
@jmprieur
Copy link
Collaborator

@brentschmaltz is it only about adding

options.TokenValidationParameters = options.TokenValidationParameters.Clone();

before starting using the options.TokenValidationParameters ? for instance here: https://github.com/AzureAD/microsoft-identity-web/blob/master/src/Microsoft.Identity.Web/WebApiAuthenticationBuilderExtensions.cs#L102 ?

cc: @jennyf19 @pmaytak

@brentschmaltz
Copy link
Member Author

@jmprieur i haven't reviewed the entire call graph, but the idea is if you modify TokenValidationParameters by adding an audience, issuer, etc, then you should call clone first.
When we developed JwtBearer, we created a local copy before any modifications. To get an idea see: https://github.com/dotnet/aspnetcore/blob/4a93863b8d865bb4d65f77b34ee335d7b23a79fc/src/Security/Authentication/JwtBearer/src/JwtBearerHandler.cs#L91 . This issue is to make sure we follow the same pattern. Let me know if you want me to review anything.

@jmprieur
Copy link
Collaborator

Thanks @brentschmaltz
@pmaytak I see you self-assigned the issue. please add Brent to the PR review.

@jennyf19
Copy link
Collaborator

Included in 0.2.0-preview release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed .NET Core 5 P1
Projects
None yet
Development

No branches or pull requests

4 participants