A warning about Spring converters

[Rod Widdowson]

I just diagnosed a nasty little bug in the legacy parsing. I'm not really
sure what we could have done to avoid this and I'll be happy to hear
suggestions, but mainly this is a warning to everyone to look out for this
pattern.

I turns out that the bug was in the legacy parsing but it could equally have
been in the spring configuration, so I'll explain in terms of Spring beans  

We are constructing the predicate to select the configuration.  In V2
configuration the id attribute (yes, that one again) is used to select
either the relying party EntityID or any encapsulating EntitiesDescriptor's
Id.

So the predicate is an OR of a RelyingPartyIdPredicate and an
EntitiesDescriptorPredicate.

So what the code was constructing was:

<bean class="com.google.common.base.Predicates" factoryMethod="or">
   <constructor-arg>
	<bean
class="net.shibboleth.idp.profile.logic.RelyingPartyIdPredicate">
		<constructor-arg>TheIdFromTheConfig</constructor-arg>
 	</bean>
    </constructor-arg>
   <constructor-arg>
	<bean
class="net.shibboleth.idp.saml.profile.logic.EntitiesDescriptorPredicate">
		<constructor-arg>TheIdFromTheConfig</constructor-arg>
 	</bean>
    </constructor-arg>
</bean>

This was fine.  RelyingPartyIdPredicate has two constructors, one which
takes a single parameter a Collection<String> and one which takes a
Predicate.  The Spring code seamlessly did the type conversion from String
to List<String> and called the first constructor.

But some time ago we added an explicit Converter<String,Predicate> to allow
us to say "false" and get back the "AlwaysFalse" predicate.  This makes a
lot of sense for easy configuration.

The trouble is that this then meant that when it encountered the stanza
Spring first of all converted the String into a Predicate and then calle the
*other* constructor for the predicate (which now always returns false).

This fix is of course easy - 

	<bean
class="net.shibboleth.idp.profile.logic.RelyingPartyIdPredicate">
		<util:list><value>TheIdFromTheConfig</value></util:list>
 	</bean>

So to go back to the top, you should be aware of this pothole, and if anyone
has suggestions of ways to look out for duplicates of bugs like this I'd be
glad to hear them...

Rod