Populate the ValidIssuer and the ValidAudience properties of the JwtBearerOptions.TokenValidationParameters.

[satma0745]

PR for the Issue #52820.

[ghost]

Thanks for your PR, @satma0745. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[martincostello]

Ideally this PR should include 1 or more tests that validate the fix and that it doesn't get broken again in the future by accident.

[satma0745]

@dotnet-policy-service agree

[satma0745]

@martincostello, I've refactored old tests a bit and provide my own one.

Now we have 3 tests related to reading TokenValidationParameters from configuration:

1. CanReadJwtBearerOptionsFromConfig tests the base case for simple flat values
2. CanReadMultipleAudiencesFromConfig tests if the array of ValidAudiences is loaded properly
3. CanReadMultipleIssuersFromConfig tests that ValueIssuers and IssuerSigningKeys are loaded and threated properly. I think it's better to have it in a separate test-method since they have complex rules (in contrast to the ValidAudiences property).

[satma0745]

I tried sticking with the established convention of testing and designed the tests not to stand out too much.
Anyway, let me know if You find something looking out of place or little bit off.

[satma0745]

/azp run

[azure-pipelines]

Commenter does not have sufficient privileges for PR 52821 in repo dotnet/aspnetcore

[martincostello]

or by simply closing and reopening

[satma0745]

or by simply closing and reopening

But what's about the Assigned Developer? Won't he be un-assigned when I close the PR?

[martincostello]

Not to my knowledge.

[halter73]

@captainsafia Can you take a look at this since you worked on this and very clearly intentionally set just the multi-value properties?

I noticed while exploring the identity model code that even though they expose single-value properties for config (e.g. ValidIssuer and ValidAudience) it looks like the multi-value properties are ultimately used as the source of truth. Updated this to just set the multi-value property.

#42679 (comment)

I think the current behavior of not setting the single-value properties is fine since it gets included in the multi-value property. And based on my reading of the ValidateIssuerAsync method in IdentityModel, this change is redundant since it checks against ValidIssuer and then all the ValidIssuers.

If we decide to change the config binding at all, we should make it align with the TokenValidationParameters more directly and not include the single-value property in the multi-value property. That might make things less confusing, and I don't think it would cause any real problems since it appears IdentityModel looks at both together.

[halter73]

Can we include a test that configures both ValidIssuer and ValidIssuers? Or ValidAudience and ValidAudiences?

[halter73]

I talked to @captainsafia, and we're fine taking this change since we have quite a few previews left to see if there are any problems with this simplification.

[dotnet-policy-service]

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.