search

OpenConext / Stepup-saml-bundle

A PHP Symfony bundle that adds SAML capabilities to your application using simplesamlphp/saml2
Apache License 2.0
14 stars 24 forks source link

Allow unkown attributes in profile #50

Closed mrvanes closed 7 years ago

mrvanes commented 8 years ago

This patch also requires a translation for profile.saml.attributes.unknown in profile app/Resources/translations

DRvanR commented 8 years ago

If this is a profile only issue, I very much would prefer to solve this in profile only (register the unknown type there, catch and handle the exception by returning the new type) rather than introduce a BC break in this library used in various other projects as well. Furthermore this change introduces a strange thing: if I make a typo when attempting to get a specific attribute, I might get the "unknown" attribute rather than the expected error (see also the failing test) that I attempt to get an unknown urn, masking the failure.

/cc @rjkip @arothuis

rjkip commented 8 years ago

I agree with @DRvanR for all the same reasons :)

thijskh commented 8 years ago

This issue is high on the Profile backlog: OpenConext/OpenConext-profile#55.

arothuis commented 8 years ago

Thanks for looking into the issue. However, I agree with @DRvanR, I think the stepup-saml-bundle should not prescribe what has to be done with invalidly typed attributes or unknown attributes. Clients of the bundle are, IMHO, best fitted to discern expected, mistyped and unknown attributes. This PR would make that difficult as setting an 'unknown' attribute hides what the attribute was.

arothuis commented 8 years ago

This is more complicated than I thought.

The AttributeDictionary is responsible for defining attributes based on urns and allows assertions to be translated to an AssertionAdapter. The AssertionAdapter constructs an AttributeSet based on the attributes in the assertion, using the AttributeDictionary.

An UnknownUrnException is thrown when an urn cannot be defined by the AttributeDictionary. Construction of the AttributeSet (thus of the AssertionAdapter) is then stopped, therefore responding to the UnknownUrnException would not suffice as the translation can be incomplete.

I still think the client should determine how to deal with expected, mistyped or unknown attributes.

arothuis commented 8 years ago

We might want to reconsider how AttributeDefinitions (and fallbacks) currently work.

If we want to keep their current behavior, one solution could be to extract the interfaces from AttributeDictionary and AttributeDefinition and offer alternative, non-strict implementations. This would not affect existing functionality, yet gives clients the ability to use a non-strict alternative.

arothuis commented 7 years ago

Closing as this is no longer relevant.