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

[Feature Request] Simplify the message for ASP.NET classic and daemon apps (by splitting Microsoft.Identity.Web into sub packages for token cache and cert management) #1431

Closed
jmprieur opened this issue Sep 7, 2021 · 9 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@jmprieur
Copy link
Collaborator

jmprieur commented Sep 7, 2021

Is your feature request related to a problem? Please describe.
Microsoft identity web provide some functionalities for apps developed with .NET framework, or .NET Core (not necessarily ASP.NET Core). See Support for ASP.NET classic and more generally .NET 4.7.2

Some developers told us they are reluctant about referencing the whole Microsoft.Identity.Web package to just get the token caches, and or the certificate management

Describe the solution you'd like
Split Microsoft.Identity.Web to separate out:

  • Microsoft.Identity.TokenCache, which would contain In Memory token cache, distributed cache (but not session cache that says in Microsoft.Identity.Web). This would be available for netcoreapp3.1, net5.0, net462, net472 and netstandard2.0. The .NET Standard 2.0 platform would reference the Microsoft.Extension version 3.*
  • Microsoft.Identity.Certificates which would contain DefaultCertificateLoader and certificate description. This would be available for netcoreapp3.1, net5.0, net462, net472 and netstandard2.0. The .NET Standard 2.0 platform would reference the Microsoft.Extension version 3.*. It takes a dependency on KeyVault.

image

Describe alternatives you've considered
Have token cache implementation in MSAL.NET. This is possible for an in-memory token cache, and is the object of AzureAD/microsoft-authentication-library-for-dotnet#2861. It's not, however, possible to have MSAL.NET reference the Microsoft.Extension assembllies (to keep it small, and avoid diamond dependencies issues)

Out of scope
When MSAL.NET provides a shared cache for all flows,

@jmprieur jmprieur added the enhancement New feature or request label Sep 7, 2021
@bgavrilMS
Copy link
Member

A few questions:

  1. Should we move the "cache adapter" types and logic and the Certificate Loader types and logic to MSAL? This will avoid making a breaking change in these packages later on.
  2. If so, should these packages be tied to MSAL (from versioning perspective) rather than M.I.W? As they can be consumed from
  3. From a naming perspective, should these be more like Microsoft.Identity.Client.TokenCache or Microsoft.Identity.Web.TokenCache ?

@rymeskar
Copy link

rymeskar commented Sep 7, 2021

I guess this would be also addressing the #1336, right?

Have you also thought about splitting the DownstreamAPI/TokenAcquisition into a separate package?

For the Microsoft.Extension.* and the internal R9 platform, we use the following multi-targeting and multi-versioning strategy:

  • 5 for .NET 5
  • 3 for .NET Core 3.1, NET Standard 2.1
  • 2 for .NET Standard 2.0, NET 461, NET 472

@jennyf19
Copy link
Collaborator

jennyf19 commented Sep 8, 2021

@rymeskar #1336 will be addressed at a later date, and is more ambitious (part of the higher level API work for MSAL.NET).

why did you pick those versions specifically for those platforms? Specifically 2 for .NET Standard 2.0 and NET 461/472? We have a lot of partners referencing Microsoft.Identity.Web today with 5, and we are worried this would be a breaking change for them. thoughts? thanks.

@rymeskar
Copy link

rymeskar commented Sep 8, 2021

The intent is to give the customers the default pairing around how the ecosystem was delivered -- there are always official version pairings between Extensions, ASP.NET Core and runtime from the release. Breaking away with the major version of a single component of the pairing can lead to headache of the common customer.
Still, in case one wants to update a version, the options are:

  • Update runtime to pick up different hollistic targetting set,
  • Update ASP.NET Core to update both the Extensions.* as well as ASP.NET Core together,
    • Downside: the code of your dependencies might not be ready for this
  • Update Extensions.* only,
    • Downside: the code of your dependencies might not be ready for this

Specifically, one cannot run ASP.NET Core ver 3+ on .NET framework (462, 472). The versions of Microsoft.Extensions.* and ASP.NET Core usually go hand in hand (custom setup can happen with some work). So we don't want .NET partners to be forced to run ASP.NET Core ver 2 & Extensions ver 3+. This has caused a dependency hell for them historically.

Having said all this, initially, there were a lot of breaking changes in the .NET Core ecosystem, so the multi-targeting was more important. Now, I believe the amount of breaking changes decreased in Extensions.* and ASP.NET Core significantly.

@jmprieur
Copy link
Collaborator Author

jmprieur commented Sep 9, 2021

@rymeskar
Given that Microsoft.Extentions* in .NET 5.0 work for all our customers and 2 will be deprecated in a few months, we'll go ahead with the initial plans. @jennyf19 will have a PR soon

@rymeskar
Copy link

The compatibilities of library versions are quite a tedious problem.
Here's an issue from aspnetcore where folks go into more details on this topic.

Nice sum-up:
"Yes Microsoft.Extensions are mostly backwards compatible but ASP.NET Core decided to constrain the versions of those assemblies because it was tested with 2.x versions. There may be behavior changes that make things work differnetly (after all it is a major version).

That creates a dilemma for library authors though, you need to target 2.x if you want to run on ASP.NET Core 2.1, 2.2 and 3.0 (if you want to avoid multi-targeting)."

@geeknoid
Copy link

The challenge we've seen in R9 is that services adopting an R9 package were having to upgrade a bunch of downstream packages that R9 depended on. So adopting a single R9 package meant that potentially dozens of packages were upgraded in their service, leading to much more change and turmoil than they bargained for.

As a library provider, the right choice is for you to target the lowest version of your dependencies that your code will work with. This reduces the barrier to adoption for customers pulling in your library for the first time. If you're lucky, in most of the cases your library will end up running with newer dependencies, but maybe not. It's up to the customers to decide.

In R9, our libraries want to take advantage of new features only available in later versions of .NET. For example, maybe some Span overloads have been added in .NET 5. To support those cases we thus do the following:

  • Ship net462, netstandard2.0, netcoreapp3.1, and net5.0 versions of our packages. Some packages actually have the same bits for all targets, while other packages are specialized per framework or a set of framework versions.

  • When building for each of these target frameworks, we use downstream dependencies that are appropriate to the specific framework. Basically, if someone is using net462, their minimum version of package X should be YYY. This reduces the likelihood that adopting our packages will trigger transitive version upgrades of the customer's dependencies.

For specific cases, we sometimes overrule the per-framework setting. Specifically, if the implementation of a package depends on a new version of a given package, then in that case only do we demand a newer version of the dependency.

Anyway, the point is to let us leverage modern framework features where we can, yet deliver a hassle free experience for customers trying to use our code.

@jennyf19
Copy link
Collaborator

included in 1.17.0 release

@jennyf19 jennyf19 added this to the 1.17.0 milestone Sep 20, 2021
@jennyf19
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants