Reverse attribute mapping..

[Rod Widdowson]

> >4) I am still intending spending some time to see whether it is possible
> >to
> >create a system to map from the attribute filtering engine to this
> >configuration.

After a bit of playing around I have just checked in code to do this.  As I
indicated on the call yesterday I am not 100% happy with this so any
comments would be welcome.

I should warn at the top that this can get quote confusing as you navigate
between SAML attributes their values, formats  and names and IdP attribute,
their values and names.  Further on in this mail I will confuse the issue
more by throwing in references to RequestedAttributes (both sorts).  

Anyway...

At the lowest level, the work is done by an optional
'AttributeMapperFactory' method to the attribute encoders.  This returns the
analogous attribute mapper.  Hence the code which maps  IdPAttribute to
SAMLAttribute can also produce a class whose job is to do the reverse
(SAMLAttribute to IdPAttribute).

At the top level we add a constructor to AttributesMapper which takes an
AttributeResolver as a parameter.  The constructor "just" iterates over all
the attribute encoders and, where relevant, calls the new method to create
an AttributeMapper.  These are stored in a multimap where the key is the
mapper and the result is a collection of (IdP) attribute IDs.  The multimap
is needed to deal with the case where the same (SAML) attribute and format
is encoded by two or more attribute definitions.  In this case the mapper
will output more than one (IdP) attribute for each matching (SAML)
attribute.

Once the iteration is over we can complete the creation of the
AttributeMappers by teaching each mapper about the IdP attribute names it
has to generate.

All this hangs together reasonably simply - with the note that we have to be
careful to implement meaningful #equals() and #hashCode() methods on the
AttributeMappers and AttributeValueMappers.

However, as always, things are not that simple.

What concerns me most is the addition of the factory method to the encoders.
As described above this seems OK, and it makes for quite clean code, but
having code which works at this level of detail inside idp-saml-* 'feels
wrong'.  This become particularly notably  when one realizes that in this
case the attribute mapper is not from a SAML Attribute to an IdP Attribute
but from a SAML RequestedAttribute to an IdP RequestedAttribute.  

The other nasty side effect of all this is in testing and dependency
ordering.  In order to do sensible testing I wanted to build an XML
description of an attribute resolver, create the AttributeResolver from it,
and from that generate the mapper which I could then interrogate.  Indeed I
have done this, but because of the new dependencies introduced I cannot make
idp-mapper-impl depend on idp-resolver-spring without introducing
circularity and so the test has actually been added in idp-resolver-spring.

I can see three solutions to this:

1) Live with it.  I have been with this code too long to be objective and
the code as is might just be the right thing.  It certainly does what we
need.  Further, all of the concerns I raise above already have precedents in
other parts of the system.

2) Move some of idp-attribute-mapper-* into idp-saml-* (or move the
attribute encoders into idp-attribute-mapper).  This may well introduce
other circularity (the resolver definition needs to depend on the attribute
encoders, but the attribute mapper depends on the resolver definition).

3) Move the factory method out from idp-saml-* and replace it with a big
case statement (on class type) inside the mapper code, or possibly
polymorphic methods.  Yuck.

I'm tending towards (1) - there is plenty of other work to keep us busy and
the problems are not that bad...

Rod