New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NameIDFormat as array (#91) #931

Merged
merged 5 commits into from Nov 13, 2018

Conversation

Projects
None yet
4 participants
@ghalse
Contributor

ghalse commented Sep 20, 2018

This patch allows for the NameIDFormat in SSP's metadata format to be either an array or a string, thus facilitating the generation of multiple elements in XML metadata. This, in turn, allows IdPs to advertise support for zero or more NameIDFormats, in line with SAML2Meta.

Background:

In issue #91, @id812510 requested that NameIDFormat become an array. We encountered the same issue when initially setting up our federation hub in that we wanted the generated metadata to include both a transient and a persistent NameIDFormat, since the hub has authproc filters for both.

I developed a patch for this in 2016, but at the time I was not confident enough in my understanding of SSP's internals to convert that into a pull request. I instead left the branch in my own fork and referenced the issue. However, we've been running this patch in production for well over a year over several version changes of SSP and we've fixed the only bug we ever encountered.

Realising this might still be an issue for other people, I'm now converting that into a pull request :)

ghalse added some commits Jul 1, 2016

Allow multiple NameIDFormats for IdPs
The SAML2int spec suggests that IdPs should advertise two name
identifier formats, and SAML itself supports this. It seems that
SimpleSAMLphp does too, when handling metadata in XML (it is implemented
as an array). However the internal metadata format uses getString,
limiting us to only a single NameIDFormat. So far as I can tell, all
that's needed to fix this is to change the metadata parser to use
getArrayizeString to accept either a string or an array, and to cast
that as a string when necessary.
This may solve issue #91
@jaimeperez

Hi @ghalse! Thanks a lot for this!

Please see my comment in the code. I think we should document this properly.

Show resolved Hide resolved modules/saml/lib/IdP/SAML2.php Outdated

ghalse and others added some commits Oct 2, 2018

@tvdijen tvdijen added this to the 1.17 milestone Oct 17, 2018

@tvdijen

This comment has been minimized.

Member

tvdijen commented Oct 17, 2018

We should have a look at this report from Scrutinizer before merging this:

$spMetadata->getArrayize...g('NameIDFormat', null) cannot be passed to array_shift() as the parameter $array expects a reference

@thijskh

Good change, only thing as noted by Tim is that array_shift generates a notice, will merge and fix

@thijskh thijskh merged commit dfe603c into simplesamlphp:master Nov 13, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 31.628%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment