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] Incorrect Test Data in WriteKeyInfoTheoryData (Microsoft.IdentityModel.Xml.Tests) #3159

Open
1 of 14 tasks
omidmloo opened this issue Mar 6, 2025 · 3 comments · May be fixed by #3166
Open
1 of 14 tasks

[Bug] Incorrect Test Data in WriteKeyInfoTheoryData (Microsoft.IdentityModel.Xml.Tests) #3159

omidmloo opened this issue Mar 6, 2025 · 3 comments · May be fixed by #3166

Comments

@omidmloo
Copy link

omidmloo commented Mar 6, 2025

Which version of Microsoft.IdentityModel are you using?
Microsoft.IdentityModel 8.6.0

Where is the issue?

  • M.IM.JsonWebTokens
  • M.IM.KeyVaultExtensions
  • M.IM.Logging
  • M.IM.ManagedKeyVaultSecurityKey
  • M.IM.Protocols
  • M.IM.Protocols.OpenIdConnect
  • M.IM.Protocols.SignedHttpRequest
  • M.IM.Protocols.WsFederation
  • M.IM.TestExtensions
  • M.IM.Tokens
  • M.IM.Tokens.Saml
  • M.IM.Validators
  • M.IM.Xml
  • S.IM.Tokens.Jwt
  • Other (please describe):

Is this a new or an existing app?
The app is in production and I have upgraded to a new version of Microsoft.IdentityModel.

Repro

While fixing issue #3130, I identified that some test data in WriteKeyInfoTheoryData is incorrect, leading to mismatches between the actual and expected results in the WriteKeyInfo test method in the DSigSerializerTests class inside the Microsoft.IdentityModel.Xml.Tests test project.

Code Snippet

[Theory, MemberData(nameof(WriteKeyInfoTheoryData), DisableDiscoveryEnumeration = true)]
public void WriteKeyInfo(DSigSerializerTheoryData theoryData)
{
    TestUtilities.WriteHeader($"{this}.WriteKeyInfo", theoryData);
    var context = new CompareContext($"{this}.WriteKeyInfo, {theoryData.TestId}");
    try
    {
        var keyInfo = theoryData.Serializer.ReadKeyInfo(XmlUtilities.CreateDictionaryReader(theoryData.Xml));
        theoryData.ExpectedException.ProcessNoException(context.Diffs);
        IdentityComparer.AreKeyInfosEqual(keyInfo, theoryData.KeyInfo, context);
        var ms = new MemoryStream();
        var writer = XmlDictionaryWriter.CreateTextWriter(ms);
        theoryData.Serializer.WriteKeyInfo(writer, keyInfo);
        writer.Flush();
        var xml = Encoding.UTF8.GetString(ms.ToArray());

        // FIXME: Test data needs to be corrected because the test data and expected results differ.
        // Used CompareContext.Default to prevent this test from failing.
        IdentityComparer.AreEqual(theoryData.Xml, xml, CompareContext.Default);
    }
    catch (Exception ex)
    {
        theoryData.ExpectedException.ProcessException(ex, context.Diffs);
    }

    TestUtilities.AssertFailIfErrors(context);
}

Expected behavior
The test should correctly compare the serialized XML output with the expected XML structure. The test data in WriteKeyInfoTheoryData should align with the actual expected results.

Actual behavior
Currently, the test data and expected results do not match, which forces the test to use CompareContext.Default to avoid failure. This indicates that the test data needs to be updated for accurate validation.

Possible solution

  • Review and update the test data in WriteKeyInfoTheoryData to ensure consistency with the expected serialized XML output.
  • Remove reliance on CompareContext.Default and use created context to validate the XML structure against the corrected expected results.

Additional context / logs / screenshots / links to code

@jennyf19
Copy link
Collaborator

jennyf19 commented Mar 7, 2025

Did you want to fix this one @omidmloo or should we? Wasn't sure if you were interested and already started.

@omidmloo
Copy link
Author

omidmloo commented Mar 7, 2025

Yes, I'm interested in fixing this. Please let me know if you'd like me to move forward.

@jennyf19
Copy link
Collaborator

jennyf19 commented Mar 9, 2025

sounds good, thanks @omidmloo , feel free to propose a PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants