Add ability to define additional attributes on ContactPerson element#509
Conversation
Cannot provide array_filter() output directly to empty() in conditional in PHP <= 5.4.
thijskh
left a comment
There was a problem hiding this comment.
I've tested this and indeed does the trick, thanks! One thing is that the documention does not mention this option. Could you also add some docs for it? Then I think it's ready to merge.
| "ext-hash": "*", | ||
| "ext-json": "*", | ||
| "simplesamlphp/saml2": "dev-master#00e38f85b417be1e10a2d738dd2f5ea82edb472c as 2.2", | ||
| "simplesamlphp/saml2": "dev-master#a94403bfe5627c90fe3764e0ada5a44841a11e80 as 2.3.3", |
There was a problem hiding this comment.
@jaimeperez Can we 'safely' do this in master w.r.t. the changes to epti-handling?
|
We can merge this when it's safe to bump the saml2 dependency to after the latest EPTI-changes. |
|
Yes, if we want to update the dependency on the SAML2 library, we need to make sure SimpleSAMLphp uses the |
| 'company' => 'Example Inc.', | ||
| 'attributes' => array( | ||
| 'xmlns:remd' => 'http://refeds.org/metadata', | ||
| 'remd:contactType => 'http://refeds.org/metadata/contactType/security', |
There was a problem hiding this comment.
remd:contactType is missing closing quote
| 'contacts' => array( | ||
| array( | ||
| 'contactType' => 'other', | ||
| 'emailAddress' => 'abuse@example.org', |
There was a problem hiding this comment.
Perhaps it would be better to add the mailto: scheme in the example email address to be consistent with the SIRTFI guide: https://wiki.refeds.org/display/SIRTFI/Guide+for+Federation+Operators
…ID objects. This enables too the implementation of additional contact attributes, as requested in #509 to support the SIRTFI framework.
|
|
||
| // check attributes is an associative array | ||
| if (isset($contact['attributes'])) { | ||
| if (empty($contact['attributes']) || !is_array($contact['attributes']) || count(array_filter(array_keys($contact['attributes']), 'is_string')) == 0) { |
There was a problem hiding this comment.
As a rule of thumb, strict comparisons (===) are always preferred to keep us in the safe side in case of an unexpected return value. In this case it should be ok, but the strict comparison could protect us from future changes in the signature of count().
Also, I suspect the line is a bit too long. Can you split it to make it a bit more readable? I.e.:
if (empty($contact['attributes']) || !is_array($contact['attributes']) ||
count(array_filter(array_keys($contact['attributes']), 'is_string')) == 0
) {|
Hi Tyler! I've just committed an update of the SAML2 lib dependency so that simplesamlphp/saml2#79 is now available in SSP. Could you please update this PR to remove the update to Thanks and sorry for the delay taking care of this! 😅 |
|
Hey @jaimeperez I think I addressed all the concerns brought up by @NicolasLiampotis. Let me know if I missed anything. |
|
Merged, thanks a lot @tdiscuit! |
This will allow you to configure an attributes array on contact definitions for trust frameworks such as SIRTFI. This is a continuation from the simplesamlphp/saml2 pull #79.
I thought about having a SIRTFI class that could be included onto the contact person element, but I couldn't think of a clean way of adding it on, but I am open to discussion about that. There are also some todo's on the addContact function that I decided was out of scope for this PR that would have to be addressed if I went down the route of having a separate class for SIRTFI.
Also, saml2 doesn't have a new tag with the updated PR that I submitted there, so I've simply updated the commit hash in the composer config. That can be fixed whenever a new tag is created with the changes that were made to support this in saml2.