dotnet / wcf

This repo contains the client-oriented WCF libraries that enable applications built on .NET Core to communicate with WCF services.
MIT License
1.7k stars 559 forks source link

Cannot inherit System.ServiceModel.Security.Tokens.SecurityTokenParameters #5026

Open martintro opened 1 year ago

martintro commented 1 year ago

Describe the bug I am trying to move from .NET Framework to .NET 6 and I get errors with overriding members/methods and the error "Cannot access internal constructor 'SecurityTokenParameters()' here". What I've found there is a partial class of this class in the System.ServiceModel.Primitives.Ref which has an internal constructor, while the class in System.ServiceModel.Primitives has an protected constructor. Is this a bug or made with intention for some reason?

To Reproduce Steps to reproduce the behavior:

  1. Create a .NET 6 class library project
  2. Reference System.ServiceModel.Primitives 6.0.0-preview1.23060.3
  3. Create a class
public class CustomSecurityTokenParameters : System.ServiceModel.Security.Tokens.SecurityTokenParameters
{
    public CustomSecurityTokenParameters()
    { }

    protected override SecurityTokenParameters CloneCore()
    {
        throw new NotImplementedException();
    }
}

Expected behavior Be possible to create a class which inherits from SecurityTokenParameters from my project.

mconnew commented 1 year ago

It was done because SecurityTokenParameters is an abstract class, and some of the abstract members aren't applicable for client usage. We had a few options.

Bring the API over from Framework as is

We're trying to keep the API surface "clean" so having methods/properties which are only for service side usage sends a confusing message (less so now CoreWCF exists) and is just messy.

Make it derivable with only the necessary pieces made available

This runs the risk of creating a future code break. The breaking scenario is your code is a library and compiles against version X of WCF and overrides everything that's abstract. The actual app which consumes your library forces upgrading the version of WCF to X+1 which has a missing abstract method/property added back. At runtime, when the CLR is populating the v-table (for abstract/virtual methods), it will throw a JIT exception as you don't have a method to override the abstract method.

Being over the immediately necessary API's for the features we were porting and make ctor internal

By making the constructor internal, we prevent anyone deriving the class so we're free to add things to the API later without causing a runtime break for anyone.

We took the last approach of making the constructor internal in the reference assembly to ensure we didn't break anyone else and deferred the work of working out which abstract members to bring over until it was actually needed. Now we've had some time and added more features since then, it looks like everything in SecurityTokenParameters is reasonable to add to the public contract. We exposed ISecurityCapabilities which the internal implementation uses SecurityTokenParameters to answer SupportsServerAuthentication, which was exposed because we do eventually want to support full Message security and I think that's needed there. So even the api's which look like they could potentially be server only api's have a limited usage in the client code.

2 things need to be done to enable your scenario.

  1. Compare implementation with .NET Framework implementation to make sure there's no abstract api's missing.
  2. Add all abstract members to the reference assembly source file including the changing the constructor.

If you would like to submit a PR for that, we have a preview release our already and this would get added to the next release which should be relatively soon.

martintro commented 1 year ago

Thanks for a great answer. I will start the PR asap. What is the current time plan for next preview version as well as RTM version of 6.0.0?

mconnew commented 1 year ago

There are 2 bugs I need to work on in the named pipe implementation (issues around which thread code executes on), but my time is spent on other things right now (implementation in CoreWCF largely) so it's not imminent. Because we're dropping netstandard2.0 support in the package, we want a slightly longer preview period to get feedback if this was the wrong thing before we go RTM. We have a few other bug fixes already merged so we could consider releasing a preview2 package with your changes depending on the timing of everything.

martintro commented 1 year ago

I see. As you might guess, we are in need of all my three issues (this one, #5027 and #5029) to be able to migrate our client application to .NET 6. I would be glad if you had time to give some feedback for #5029 as well, just for us to get a feeling for if it is doable at all.

While I looked further through our code I found out that we also may need SecurityTokenReferenceStyle and LocalIdKeyIdentifierClause, but I guess that won't be a biggie.