Adding support for WSTrustChannelFactory and WSTrustChannel

[brentschmaltz]

This is similar to what existed in .net framework with some small changes.
WSTrustChannel ctor takes a ChannelFactory rather than a WSTrustChannelFactory.
The new Async model has replaced the Begin / End model found in the original WCF release.
Common methods that were used in WSTrustSecurityTokenProvider and WSTrustChannel were moved to WSTrustUtilities.
An additional constructor was added that takes a separate IRequestChannel.

[mconnew]

We don't have this on any of our other interfaces. Is this here for a specific reason or just because of existing coding patterns?

[brentschmaltz]

Existing patterns.

[brentschmaltz]

removed [ComVisible(false)] in: ab9b3c1

[mconnew]

All of these methods are interchangeable as they take and return a Message and they all have the Action and ReplyAction to be "*". I need to spend a little bit of time trying some things with this code, but I'm pretty certain that the Name property is only used to provide the Action and ReplyAction if they aren't explicitly specified. This whole interface seems like it could be eliminated and replaced with a Task based equivalent of IRequestChannel. IWSTrustChannelContract has the strongly typed operations on it. I'll update once I know more.

[mconnew]

I have done some local testing and confirmed what I believed. When an operation is defined like this, the is zero request shape that comes from the operation. Every method in this interface will generate the exact same on the wire message. I have no idea what the original intention was of the authors of the class in NetFx, but this interface make no sense at all to exist. We can remove this interface and just have IWSTrustChannelContract live on it's own.

[brentschmaltz]

It does make a difference, the responsibility is on the user to create the correct message.
To make it easier for users we probably would want to make WSTrustBodyWriter public, although the code is simple.

see code here: https://github.com/brentschmaltz/WCF/blob/9b084e846fea828fbb5d65bbe8ac70bdd26f9ded/WsTrustClientCore/Program.cs#L46

SelfHostSTS will identify the different messages and route them.
For Renew see: https://github.com/brentschmaltz/WCF/blob/9b084e846fea828fbb5d65bbe8ac70bdd26f9ded/SelfHostSTS/SelfHostSecurityTokenService.cs#L215

[mconnew]

Should the cref say Federation.ChannelFactory? I thought this was a regular S.SM.ChannelFactory?

[brentschmaltz]

addressed in: ab9b3c1

[mconnew]

I'm a little confused by this. Should the passed in ChannelFactory implement IRequestChannel, or should it produce clients which implement IRequestChannel? If the latter, there's no need to do any check as in Core, all channels implement IRequestChannel, but this code doesn't check for that. ChannelFactory itself doesn't implement IRequestChannel so this would always throw.

[brentschmaltz]

addressed in: ab9b3c1

[mconnew]

If the passed in IRequestChannel is already open, this will throw. Either need a state check here and only call Open if in state Created, or add validation that the passed in channel should be in the Created state.

[brentschmaltz]

When would it be called if it was already open?

[mconnew]

Using our code it won't be. But you've got to protect against people using the public API's outside of our usage. So someone could do this:

var factory = new ChannelFactory<IRequestChannel>(binding, endpoint);
var channel = factory.CreateChannel();
channel.Open();
var trustChannel = new WSTrustChannel(factory, channel, WsTrustVersion.Trust13);
((ICommunicationObject)trustChannel).Open();

If this code is a little more convoluted and the IRequestChannel gets passed in from somewhere else, someone could presume a newly created WSTrustChannel needs to be opened as that's the normal case. So thinking about it, I think we should throw in the constructor if the passed in IRequestChannel isn't in the Created state as that's really where the problem comes from.
Also, while it's not part of the ICommunicationObject interface (which is explicitly implemented so you must cast to call open), do you think there's any benefit in adding an OpenAsync and CloseAsync method to this class? No cast would be needed, but this class isn't like a regular channel instance anyway so I think that's fine. Just use Task.Factory.FromAsync to wrap the Begin/End methods. Otherwise we're expecting people to either handle APM (I refer you to our teams chat about that) or use the synchronous versions.

[brentschmaltz]

I check to make sure the IRequestChannel is in the Created state.

[mconnew]

We don't support endpoint configuration name as we don't support app.config. Remove this constructor as the base class will just throw.

[brentschmaltz]

addressed in: ab9b3c1

[mconnew]

Same for this one, it will just throw so remove.

[brentschmaltz]

addressed in: ab9b3c1

[mconnew]

All members of this class are internal. I think you intended for the class to be internal too?

[brentschmaltz]

addressed in: ab9b3c1

[mconnew]

I think this can probably be removed. It looks like you added it because originally on NetFx this was there.

[brentschmaltz]

addressed in: ab9b3c1

[ericstj]

cc @safern @joperezr
@mconnew let me know that they're facing an issue with these tests using the reference assemblies.

All the other tests in this repo don't actually use the System.ServiceModel.* contracts, instead they compile against System.Private.ServiceModel so they don't need the facades. This test needs the facades, since it's testing the System.ServiceModel.Federation product assembly which compiles against the reference assemblies.

I think the solution might be to add references from the test project to the src projects for all the shims used by System.ServiceModel.Federation, and make sure that those don't conflict with the transitive references from the product assembly.

The way we solve a similar problem in dotnet/runtime is to flow the reference assemblies across along with the src project reference using ReferenceAssembly metadata. This might be another option to allow System.ServiceModel.Federation to reference the src projects, yet compile against the reference assemblies.

[mconnew]

@ericstj I replaced the reference to S.P.SM project with a reference to System.ServiceModel.Primitives.Facade.csproj and it now compiles and the tests run. I'm seeing an odd problem though. When editing in VS, I get the error:
CS7069 Reference to type 'Binding' claims it is defined in 'System.ServiceModel.Primitives', but it could not be found System.ServiceModel.Federation.Tests (net5.0), System.ServiceModel.Federation.Tests (netcoreapp3.1) C:\git\wcf\src\System.ServiceModel.Federation\tests\WSTrustChannelFactoryTest.cs 15 Active

[ericstj]

I replaced the reference to S.P.SM project with a reference to System.ServiceModel.Primitives.Facade.csproj and it now compiles and the tests run. I'm seeing an odd problem though. When editing in VS, I get the error:
CS7069 Reference to type 'Binding' claims it is defined in 'System.ServiceModel.Primitives', but it could not be found System.ServiceModel.Federation.Tests (net5.0), System.ServiceModel.Federation.Tests (netcoreapp3.1)

Is System.Private.ServiceModel in the SLN? Can you try just adding facade references (in addition to S.P.SM) rather than replacing S.P.SM? That error indicates that the design time build is unable to follow the type-forward, which indicates it didn't have a reference to S.P.SM (or that the typeforward was missing from S.SM.P in design time). I would expect the design time build should see both S.SM.P and S.P.SM with only the S.SM.P reference, but something about your build might be preventing that. You can debug design time builds with information described here: https://github.com/dotnet/project-system/blob/main/docs/design-time-builds.md