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.

Sign up for GitHub

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

Jump to bottom

[merge after .NET 9 GA] Use new Base64Url API #2817

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

[merge after .NET 9 GA] Use new Base64Url API #2817

wants to merge 1 commit into from

Conversation

msbw2
Copy link
Contributor

@msbw2 msbw2 commented Sep 6, 2024 •
edited
Loading

Use the new Base64Url API.

@msbw2 msbw2 requested a review from a team as a code owner September 6, 2024 19:24
@msbw2 msbw2 linked an issue Sep 6, 2024 that may be closed by this pull request
M.IM8 leverages the .NET 9 Base64Url implementation #2534
Open
@keegan-caruso
Copy link
Contributor

@msbw2 Can you post a before / after of this benchmark as well here: JsonWebTokenHandler_ValidateTokenAsyncWithTVP ?

Would be good to see the difference at that level.

@msbw2 msbw2 force-pushed the brettwhite/base64url branch 5 times, most recently from 4abfe06 to 9b9b583 Compare September 10, 2024 00:11
@pmaytak pmaytak mentioned this pull request Sep 18, 2024
Closed
@pmaytak
Copy link
Contributor

pmaytak commented Sep 20, 2024

@msbw2 Can you post a before / after of this benchmark as well here: JsonWebTokenHandler_ValidateTokenAsyncWithTVP ?

Would be good to see the difference at that level.

Benchmarks are here: #2828 (comment)

jennyf19
jennyf19 reviewed Sep 20, 2024
View reviewed changes
NuGet.Config Outdated Show resolved Hide resolved
@msbw2 msbw2 force-pushed the brettwhite/base64url branch 2 times, most recently from 9e2ff23 to b1d4eed Compare September 20, 2024 22:49
pmaytak
pmaytak reviewed Sep 20, 2024
View reviewed changes
benchmark/Microsoft.IdentityModel.Benchmarks/identitymodel.benchmarks.yml
Comment on lines +60 to +64
Base64UrlEncoderTests:
application:
job: benchmarks
variables:
filterArg: "*Base64UrlEncoderTests*"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a benefit on running these benchmarks in builds? I think these are good to run right now to verify the work. Any perf regressions to encoding/decoding should be caught by other validation benchmarks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think they are today, correct? If not, let's open an issue figure out prioritization.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, these are new tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, but no benchmarks are run today as part of the PR builds, correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think so.

@jennyf19 jennyf19 mentioned this pull request Sep 21, 2024
Merged
@msbw2 msbw2 force-pushed the brettwhite/base64url branch 2 times, most recently from 168bfe8 to 08c0f25 Compare September 21, 2024 05:50
@msbw2 msbw2 changed the title Use .NET 9.0 Base64Url API Use new Base64Url API Sep 21, 2024
@msbw2 msbw2 force-pushed the brettwhite/base64url branch 6 times, most recently from 73666d5 to 55946bb Compare September 23, 2024 17:26
jennyf19
jennyf19 approved these changes Sep 23, 2024
View reviewed changes
Copy link
Collaborator

@jennyf19 jennyf19 left a comment

Choose a reason for hiding this comment

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

:shipit:

pmaytak
pmaytak reviewed Sep 23, 2024
View reviewed changes
src/Microsoft.IdentityModel.JsonWebTokens/JsonWebToken.cs Outdated Show resolved Hide resolved
src/Microsoft.IdentityModel.Tokens/Base64UrlEncoder.cs Show resolved Hide resolved
src/Microsoft.IdentityModel.Tokens/Base64UrlEncoder.cs
@@ -177,47 +120,68 @@ public static byte[] DecodeBytes(string str)
return Decode(str.AsSpan());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have tests for this DecodeBytes(string str)? We use it everywhere but maybe good to have explicit tests like in Base64UrlEncoderTests?

Same questions for Decode(string arg) below.

src/Microsoft.IdentityModel.Tokens/Base64UrlEncoding.cs Outdated Show resolved Hide resolved
src/Microsoft.IdentityModel.Tokens/Base64UrlEncoding.cs Outdated Show resolved Hide resolved
src/Microsoft.IdentityModel.Tokens/Base64UrlEncoding.cs Show resolved Hide resolved
src/Microsoft.IdentityModel.Tokens/Base64UrlEncoding.cs Outdated Show resolved Hide resolved
test/Microsoft.IdentityModel.Tokens.Tests/Base64UrlEncodingTests.cs Outdated Show resolved Hide resolved
keegan-caruso
keegan-caruso reviewed Sep 24, 2024
View reviewed changes
build/dependencies.props
@@ -11,6 +11,7 @@
<SystemMemoryVersion>4.5.5</SystemMemoryVersion>
<SystemSecurityCryptographyCngVersion>4.5.0</SystemSecurityCryptographyCngVersion>
<SystemTextJson>8.0.4</SystemTextJson>
<MicrosoftBclMemory>9.0.0-rc.1.24431.7</MicrosoftBclMemory>
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't take an rc package in targets other than net9.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We'd have to conditionally add it. @msbw2 let's park this to the .NET 9 release in Nov.

@jennyf19 jennyf19 added this to the 8.2.0 milestone Sep 24, 2024
@jennyf19 jennyf19 changed the title Use new Base64Url API [merge after .NET 9 GA] Use new Base64Url API Sep 24, 2024
@keegan-caruso
Copy link
Contributor

Coverage is good

Base64UrlEncoder at 89%
Base64UrlEncoding at 82%

https://identitydivision.visualstudio.com/IDDP/_build/results?buildId=1366079&view=codecoverage-tab

joegoldman2
joegoldman2 reviewed Oct 7, 2024
View reviewed changes
src/Microsoft.IdentityModel.Tokens/Base64UrlEncoder.cs
@@ -17,11 +18,9 @@ namespace Microsoft.IdentityModel.Tokens
/// </summary>
public static class Base64UrlEncoder
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public static class Base64UrlEncoder
[Obsolete("This class is obsolete and will be removed in a future release. Use System.Buffers.Text.Base64Url instead")]
public static class Base64UrlEncoder

Should this class (and all methods) be marked as obsolete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be nice, but unfortunately there's some quirks about this class and the various methods that don't match exactly with the Base64Url API, so I'm not entirely sure that's feasible. I've seen quite a few usages of this downstream that probably wouldn't be able to migrate easily.

@jennyf19 jennyf19 modified the milestones: 8.2.0, 8.2.1 Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jennyf19 jennyf19 jennyf19 approved these changes

@pmaytak pmaytak pmaytak requested changes

@keegan-caruso keegan-caruso keegan-caruso left review comments

@joegoldman2 joegoldman2 joegoldman2 left review comments

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Dotnet9
Projects
None yet
Milestone
8.2.1
Development

Successfully merging this pull request may close these issues.

M.IM8 leverages the .NET 9 Base64Url implementation
5 participants
@msbw2 @keegan-caruso @pmaytak @jennyf19 @joegoldman2