Interface design

Interface design

BB:

This will fail because the entityID isn't set:

$ed = new \SimpleSAML\XML\md\EntityDescriptor();
$ed->toXML(new DOMDocument());

If you're using DI you could enforce that same thing by requiring the EntityID as a constructor parameter.

OM:

I think that leads to rather complicated constructors after a while. The EntityDescriptor element is simple -- it only requires an entityID in order to be valid. Other elements are more complicated, and sometimes the requirements varies depending on what attributes / elements you add.

BB:

If it does then that's a smell that your class does too much. Off course we have a spec to mirror so sometimes it may get a bit silly, but I honestly can't think of an example with many required attributes / elements right now. I don't think it's that bad.

OM:

I think the code will end up being rather ugly and difficult to read if this is used as a policy. For example, an AttributeConsumingService-element:

$serviceName = array(
   'en' => 'testSP',
);
$eppn = new ...RequestedAttribute('urn:oid:1.3.6.1.4.1.5923.1.1.1.6');
$eppn->setNameFormat('urn:oasis:names:tc:SAML:2.0:attrname-format:uri');
$eppn->setIsRequired(true);
$requestedAttributes = array(
    $eppn,
);
$acs = new ...AttributeConsumingService(0, $serviceName, $requestedAttributes);
$acs->setServiceDescription(array(
    'en' => 'A SP used to test stuff.',
));

BB:

Yes, the 'new' keyword is problematic there, that's why you have factories:

AttributeConsumingService::create(
ServiceNameCollection::create(array(ServiceName::create(Lang::ENGLISH, 'Name'))),
RequestedAttributeCollection::create(array(
    RequestedAttribute::create('urn:oid:1.3.6.1.4.1.5923.1.1.1.6')
        ->betterKnownAs('EduPersonPrincipalName')
        ->useNameFormat(Constants::NAMEFORMAT_URI)
        ->require()
    ))
)
->setServiceDescriptions(
    ServiceDescriptionCollection::create(array(ServiceDescription::create(LANG::ENGLISH, 'Consume attributes')))
);

You could even have a Builder that trades IDE autocompletion over convenience, but at least the underlying model would have type checking. Nice thing here too is that you emulate the XML nesting, so you won't be tempted to write one large swath of code, you'll hit a point pretty soon where you hit your limit on the number of indents you can live with and be forced to extract part of this building to another method.

Actually OpenSAML goes even further and has the creation done by injected factories so you can inject your own factory at any point and have it create a "MyCustom\RequestedAttribute" instead of a 'normal' one. That was very helpful in testing the signature validation vulnerability (being able to trick it to add assertions in an assertion with extensions). But very boilerplate heavy though.

OM:

You end up creating things in a strange order. (I.e. some fields must be prepared before the object is create, some are set after the object is created.) Compare to code where you can set all fields afterwards:

$acs = new ...AttributeConsumingService();
$acs->setIndex(0);
$acs->setServiceName(array(
    'en' => 'testSP',
));
$acs->setServiceDescription(array(
    'en' => 'A SP used to test stuff.',
));
$eppn = new ...RequestedAttribute();
$eppn->setName('urn:oid:1.3.6.1.4.1.5923.1.1.1.6');
$eppn->setNameFormat('urn:oasis:names:tc:SAML:2.0:attrname-format:uri');
$eppn->setIsRequired(true);
$acs->setRequestedAttribute(array(
    $eppn,
));

Or without setters/getters:

$acs = new ...AttributeConsumingService();
$acs->index = 0;
$acs->ServiceName['en'] = 'testSP';
$acs->ServiceDescription['en'] = 'A SP used to test stuff.';

$eppn = new ...RequestedAttribute();
$eppn->Name = 'urn:oid:1.3.6.1.4.1.5923.1.1.1.6';
$eppn->NameFormat = 'urn:oasis:names:tc:SAML:2.0:attrname-format:uri';
$eppn->isRequired = true;
$acs->RequestedAttribute[] = $eppn;

BB:

I have to admit, staying away from Anemic domain models (with lots of getters and setters instead of mutators) are a bit of a learning point for me (it's just so tempting to make an API with a little auto generation).

OM:

I, at least, find the last example easiest to read and write. The downside, of course, is that there is no run-time validation of the assignments until later. (Though you could probably get static validation depending on how good the PHP static analyzers are.)

BB:

That's true actually, if we annotated the public properties with their types then that would at least help the IDEs out. And with more focus on testing (now that we have something that's actually testable) we could get the safety. So while I would not have written it with public member variables, we can work with it.