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

[dotnet] [🚀 Feature]: Consider using the new Base64Url #14813

Closed
RenderMichael opened this issue Nov 26, 2024 · 8 comments
Closed

[dotnet] [🚀 Feature]: Consider using the new Base64Url #14813

RenderMichael opened this issue Nov 26, 2024 · 8 comments

Comments

@RenderMichael
Copy link
Contributor

RenderMichael commented Nov 26, 2024

Feature and motivation

With the recent release of .NET 9, the Base64Url type was released. This type fills a common need that .NET developers have, and up until recently, has been implemented, re-implemented, and polyfilled in many projects.

Selenium .NET has its own implementation: found here:
https://github.com/SeleniumHQ/selenium/blob/trunk/dotnet/src/webdriver/Internal/Base64UrlEncoder.cs

As the links in that file show, this was ripped from the Azure Active Directory implementation. They recently migrated to using the Base64Url type as well: AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#2817

Luckily for us, the .NET team released the Base64Url type as a .NET Standard 2.0 NuGet package, found here: https://www.nuget.org/packages/Base64Url

We can take this dependency unconditionally for now, and if/when we support a .NET 9+ TFM, we can conditionally remove the dependency there.

Usage example

The polyfill type Base64UrlEncoder is public (should it be internal?). Anyone currently relying on it should not be affected though.

As noted in the linked PR on Azure AD, there are minor differences in behavior between the Base64Url type and the Base64UrlEncoder type. For Azure AD, it seems like that requires them to keep the type around.

For us, we should investigate what exactly those differences are and if it would be acceptable to just use Base64Url directly.

Copy link

@RenderMichael, thank you for creating this issue. We will troubleshoot it as soon as we can.


Info for maintainers

Triage this issue by using labels.

If information is missing, add a helpful comment and then I-issue-template label.

If the issue is a question, add the I-question label.

If the issue is valid but there is no time to troubleshoot it, consider adding the help wanted label.

If the issue requires changes or fixes from an external project (e.g., ChromeDriver, GeckoDriver, MSEdgeDriver, W3C), add the applicable G-* label, and it will provide the correct link and auto-close the issue.

After troubleshooting the issue, please add the R-awaiting answer label.

Thank you!

@nvborisenko
Copy link
Member

nvborisenko commented Nov 27, 2024

No dependencies please for simple tasks! I stole this code in part of the #12777. Thanks for highlighting that it is shipped in .net 9. It is not yet clear, what we will do with it. At this moment, maximum what I can propose: just mention that it is available in .net 9 in source code near with link, and... that's it.

@nvborisenko
Copy link
Member

It should be internal, I already tried it, but there is technical limitation: #13331. The worst feeling is that I am closing this issue because of it's partially resolved - we cannot fix it because of bazel. It is as is. Thanks for understanding.

@RenderMichael
Copy link
Contributor Author

Can we add the [EditorBrowsable(EditorBrowsableState.Never)] attribute to these types, so they don’t pop up in people’s IDEs even if they are publicly accessible?

@nvborisenko
Copy link
Member

It is more workaround rather than a proper solution, internal classes should be internal. It is pity that we cannot do it.

@RenderMichael
Copy link
Contributor Author

RenderMichael commented Nov 29, 2024

I opened #14833 to make these types invisible to IDEs.

Alternatively, we can add <Compile Include="Path/To/Base64UrlEncoder.cs" /> in the test projects. I have never used this advanced feature, but it's a way to specify arbitrary files in the build. I think we only need to add it to the Common project?

If that's preferable, I can start experimenting.

@nvborisenko
Copy link
Member

It should be internal.

Copy link

This issue has been automatically locked since there has not been any recent activity since it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants