New 'identifyingAttribute option for SAML Attribute NameID filter#2260
Conversation
Allow a new option in place of identifyingAttribute called identifyingAttributes. This option expects an array of attribute names. The first one in the list that's available in the attributes being released to the SP will be used. This is particularly useful in the hosted IdP configuration. One can specify multiple attribute names in order of precedence, and the IdP will release the first one from that list to the SP that's in the SP's 'attributes' array. This avoids having to define a new authproc filter for each SP that needs a different value than the single one that was previously specified using 'identifyingAttribute'. The change still supports identifyingAttribute option which will be used instead of the new option if both are set.
|
@tvdijen , I was getting tired of having to define an authproc filter for every SP that didn't want to use our default value for the emailaddress nameID format and decided I'd implement things the way Shibboleth does with a list of attributes. Please let me know if you have questions or concerns about this feature. And I completely understand if you don'twant to use it. |
|
I like the idea, it's simple enough and I understand how it will be useful. Maybe I'm beeing too defensive if I'd also add a check if you've set both and log a warning or even error? There's quite some more if/looping now than before. This does in my opinion really beg for unit tests if we want to keep this maintainable, with all the possible combinations now. |
Done: if both are specified, an exception is now thrown. This was an excellent suggestion.
I'm not expert on writing test cases for SSP, but if you point me to something similar that's already in there, I'd be willing to try my hand at it. |
efd1f9a to
054c37d
Compare
… and identifyingAttributes set
054c37d to
5e90c6e
Compare
|
As far as tests go, we might make a new class like I should probably clean up the attribute and format that is in the config copied from above cases as it shouldn't be needed in these. |
* Cleaned up logic for finding a value for the nameID * Corrected bug when $value was empty at end of loop
|
Thank you for the PR! I have added a small number of test cases to the PR today |
These entity ids were copied from the test that I then updated which lead to much better values for test idp and sp entities. I am updating these entities here inline with the comment at simplesamlphp#2349 (review)
Mention that these are mutex options.
lint
) * New 'identifyingAttribute option for SAML Attribute NameID filter Allow a new option in place of identifyingAttribute called identifyingAttributes. This option expects an array of attribute names. The first one in the list that's available in the attributes being released to the SP will be used. This is particularly useful in the hosted IdP configuration. One can specify multiple attribute names in order of precedence, and the IdP will release the first one from that list to the SP that's in the SP's 'attributes' array. This avoids having to define a new authproc filter for each SP that needs a different value than the single one that was previously specified using 'identifyingAttribute'. The change still supports identifyingAttribute option which will be used instead of the new option if both are set. * Throw error when AttributeNameID filter has both identifyingAttribute and identifyingAttributes set * identifyingAttributes clean-up per PR comments * Cleaned up logic for finding a value for the nameID * Corrected bug when $value was empty at end of loop * add a test for the AttributeNameID using an array of attributes * lint * Update AttributeNameIDTest.php These entity ids were copied from the test that I then updated which lead to much better values for test idp and sp entities. I am updating these entities here inline with the comment at #2349 (review) * Update nameid.md Mention that these are mutex options. * Update nameid.md lint --------- Co-authored-by: Ben Martin <monkeyiq@users.sourceforge.net> Co-authored-by: monkeyiq <monkeyiq@gmail.com>
|
I have also cherry picked this back to 2.4. |
Allow a new option in place of identifyingAttribute called identifyingAttributes. This option expects an array of attribute names. The first one in the list that's available in the attributes being released to the SP will be used. This is particularly useful in the hosted IdP configuration. One can specify multiple attribute names in order of precedence, and the IdP will release the first one from that list to the SP that's in the SP's 'attributes' array. This avoids having to define a new authproc filter for each SP that needs a different value than the single one that was previously specified using 'identifyingAttribute'.
The change still supports identifyingAttribute option which will be used instead of the new option if both are set.