Skip to content

Fixed bug on case-insensitive properties on OpenIdConnectConfigurationSerializer.cs#2404

Closed
stvkouvaris wants to merge 2 commits into
AzureAD:devfrom
stvkouvaris:dev
Closed

Fixed bug on case-insensitive properties on OpenIdConnectConfigurationSerializer.cs#2404
stvkouvaris wants to merge 2 commits into
AzureAD:devfrom
stvkouvaris:dev

Conversation

@stvkouvaris

Copy link
Copy Markdown

Bug fix on case-insensitive property deserialization on OpenIdConnectConfigurationSerializer.cs

Fixes #2402

The json reader was getting out of sync after line 289
string propertyName = JsonPrimitives.ReadPropertyName(ref reader, OpenIdConnectConfiguration.ClassName, true);
inside the else statement that is handling case-insensitive property deserialization.

Removing the read: true parameter from all following json property reads, resolves the issue.

@stvkouvaris

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@keegan-caruso

Copy link
Copy Markdown
Contributor

@stvkouvaris would you be willing to add some tests for this?

@davek-dev

Copy link
Copy Markdown

This is causing an issue for me as well, was it supposed to get into yesterday's release? The PR looks good to me.

@jennyf19 jennyf19 modified the milestones: 7.1.1, 7.2.1 Jan 10, 2024
@jennyf19

Copy link
Copy Markdown
Contributor

This is causing an issue for me as well, was it supposed to get into yesterday's release? The PR looks good to me.

We need a bit more time to review it @davek-dev and it will go in the 7.2.1 release (end of month or sooner).

@jennyf19 jennyf19 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:shipit:

@brentschmaltz

Copy link
Copy Markdown
Contributor

Reviewing....

@keegan-caruso keegan-caruso left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@keegan-caruso keegan-caruso modified the milestones: 7.3.0, 7.3.1 Jan 29, 2024
@stvkouvaris

stvkouvaris commented Feb 4, 2024

Copy link
Copy Markdown
Author

@jennyf19 I can see that the issue was fixed on 7.3.1 release, however without this pull request getting merged to the main branch. Will it be merged on 7.3.2? Or is this now unnecessary?

@brentschmaltz

Copy link
Copy Markdown
Contributor

@stvkouvaris We had to make a change in the serialization model to ensure that the entire Json Token was read and position the reader on the next token.

This PR (#2491).
We incorporated your ideas into that PR.

@brentschmaltz brentschmaltz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We have used the ideas here in this PR: #2491

And extended the fixes to JsonWebTokens, JsonWebKey and JsonWebKeySet.

@brentschmaltz

Copy link
Copy Markdown
Contributor

We have used the ideas here in this PR: #2491

And extended the fixes to JsonWebTokens, JsonWebKey and JsonWebKeySet.

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.

[Bug] OpenIdConnectConfigurationSerializer.cs bug (on case-insensitive property deserialization)

6 participants