Skip to content

Conversation

@omidmloo
Copy link

Fix ValidationParameters Inconsistencies

Summary

This PR fixes inconsistencies in ValidationParameters by ensuring properties are copied correctly from TokenValidationParameters.

Description

  • The TryAllIssuerSigningKeys property was not being copied in the ValidationParameters(ValidationParameters other) constructor.
    • Fix: Now properly copies TryAllIssuerSigningKeys.
    • Behavior Change: In the new model, TryAllIssuerSigningKeys was false by default, whereas it was true in the current model. This has been aligned accordingly.
    • Fix: Updated ValidateActor in ValidationParameters for consistency.

Fixes #3131

@omidmloo omidmloo requested a review from a team as a code owner March 13, 2025 06:33
/// all available keys will be tried.
/// </summary>
/// <remarks>Default is false.</remarks>
[DefaultValue(true)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also needs to be set in the empty constructor.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

@brentschmaltz brentschmaltz Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better to set the value on the property as if a new ctor is created it may be missed.
It would be a good idea to set the other properties: LogTokenId, TryAllDecryptionKeys
Add the attribute [DefaultValue(...)] as some of the bool properties as missing.

public bool TryAllIssuerSigningKeys { get; set; } = true;

JoshLozensky
JoshLozensky previously approved these changes Mar 18, 2025
/// If the IssuerSigningKeyResolver is unable to resolve the key when validating the signature of the SecurityToken,
/// all available keys will be tried.
/// </summary>
/// <remarks>Default is false.</remarks>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the remarks to state that the default value is now true.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@sruke
Copy link
Contributor

sruke commented Mar 18, 2025

Few unit tests are failing that require investigation.

@brentschmaltz
Copy link
Contributor

@omidmloo we are working on a rather large change at the moment to get the API's for the new model completed.
We are working out of this branch, https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/tree/brentsch/ValidationError

We may want to wait until this PR is in, before additional cleanup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

In the new validation model, ValidationParameters differs from TokenValidationParameters.

6 participants