Convert \SAML2\Configuration\IdentityProvider to interface

[cb8]

I'd like to integrate this library into my application. However, \SAML2\Configuration\IdentityProvider and \SAML2\Configuration\ServiceProvider are classes.
Is there any interest in converting them to pure interfaces such that one can implement completely custom service/identity provider configuration classes (not based on arrays)? If so, should I submit a pull-request and rename the existing class to QueryableIdentityProvider?

Intent: We'd like to use \SAML2\Assertion\ProcessorBuilder to validate our assertions.

[relaxnow]

@DRvanR thoughts?

[DRvanR]

Most if not all of the methods should already be covered by the existing interfaces. If that is not the case, then that should be fixed (either by adding new or expanding current interfaces).
At that point all typehints against the \SAML\Configuration\ServiceProvider, \SAML2\Configuration\IdentityProvider should be replaced with typehints against the interfaces, and these classes should implement them all. In all honesty this is on my to-do list but I probably wont get to it till march next year at the earliest...

The reason for having these classes is to ease the adoption of this library in the framework it spun off of and by now there are some applications that rely on these classes to exist, converting them to an interface creates two issues: it adds the requirement to always create an implementation as there is no longer a functional default implementation, and it is a major BC break. Both are issues that in my mind should be avoided. By using and expanding the interfaces as illustrated above, these issues are alleviated.

So I agree all typehints must be replaced by interfaces, but I do not agree that these classes should be converted to interfaces 😉

[cb8]

As you mentioned, just converting the classes to interfaces is a BC break. Introducing new interfaces seems to be a good alternative.
If we create \SAML2\Configuration\IdentityProviderInterface this isn't consistent with the other interfaces which don't end in *Interface. Is that OK for you? If not, I'm fine with putting this issue on-hold until we can introduce some BC breaking changes.

[DRvanR]

To me that is an acceptable solution, at the same time the \SAML2\Configuration\ServiceProviderInterface should be created. I personally do not mind having interfaces that end in *Interface and interfaces that don't, however I am not a decider in this project 😄 - I just contributed the Response processing since we needed it 😉.

The rationale for having interfaces with an without *Interface suffix for me is that some define a small distinct functionality (those without Interface, i.e. EntityIdProvider) whereas others define the full API an object must expose (those with Interface), which is an aggregation of a set of other interfaces with some possible expansions. As example the new interface would extend the already implemented interfaces and add any additional methods required.

But as said, that is just my opinion, @relaxnow and @jaimeperez probably need to weigh in on such decisions.

As an aside, in the mean time you can always use the classes provided as DTO's, we do the same in another project where the Response processing is actually being used.

Cheers!

[MKodde]

We are about 5 years later and much has changed in this regard. Is the need for dedicated interfaces for the ServiceProvider and IdentityProvider classes still an issue?

From what I saw in master, the IdentityProvider and SP class now implement several interfaces. We could add additional marker interfaces for these roles. Or even consider letting the Idp/Sp interface implement the interfaces that are now used (CertificateProvider, DecryptionProvider, EntityIdProvider).

In my personal opinion, the introduction of a IdP and SP interface is still valuable as we can further decouple dependencies on hard implementations. I personally opt for an IdentityProviderInterface and an ServiceProviderInterface, that in turn extend the CertificateProvider, DecryptionProvider, EntityIdProvider interfaces.

@jaimeperez, @tvdijen what are your thoughts on this? Also in the naming of the interfaces..

[MKodde]

Just had a discussion IRL about the future of the IdP and SP classes in the SAML2 library. We should investigate if these concepts should be in the SAML2 lib in the first place. They now are only used by the SimpleSAMLphp project and possibly do not add much value to the library.