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

Bugfix/name id policy #984

Merged
merged 14 commits into from Nov 20, 2018

Conversation

Projects
None yet
3 participants
@thijskh
Member

thijskh commented Nov 13, 2018

Updated the branch to current master, codestyle and fixed the remaining issues.

It works fine for all the use cases I tested, the ability to not send any NameIDPolicy which I need indeed also is available again.

jaimeperez and others added some commits Jan 26, 2018

Handle string, array and false values in NameIDPolicy in the Message …
…class.

This allows us to both specify the details of the policy, and also to avoid skipping sending one by setting the configuration option to false.
NameIDPolicy is not a valid configuration option in remote SP metadat…
…a, and therefore it doesn't make sense to use it in a filter generating persistent NameIDs (IdP side).

@thijskh thijskh added this to the 1.17 milestone Nov 13, 2018

@thijskh thijskh requested review from tvdijen and jaimeperez Nov 13, 2018

@thijskh

This comment has been minimized.

Member

thijskh commented Nov 13, 2018

This is to fix #771

@jaimeperez

I've added a couple of minor comments. Other than that, the PR looks very good to me 👍

{
// Test null or unset
$nameIdPolicy = null;
$this->assertEquals(Metadata::parseNameIdPolicy($nameIdPolicy), ['Format' => \SAML2\Constants::NAMEID_TRANSIENT]);

This comment has been minimized.

@jaimeperez

jaimeperez Nov 20, 2018

Member

I think the assert*() methods receive the expected value first, then the actual value. It's really a minor detail, but could make it harder to debug when getting errors during testing, as the values would then be inverted.

$this->assertEquals(Metadata::parseNameIdPolicy($nameIdPolicy), [
'Format' => 'urn:oasis:names:tc:SAML:1.1:nameid-format:persistent',
'AllowCreate' => false
]);

This comment has been minimized.

@jaimeperez

jaimeperez Nov 20, 2018

Member

This could be simplified:

        $nameIdPolicy = [
            'Format' => 'urn:oasis:names:tc:SAML:1.1:nameid-format:persistent',
            'AllowCreate' => false
        ];
        $this->assertEquals($nameIdPolicy, Metadata::parseNameIdPolicy($nameIdPolicy));

@thijskh thijskh merged commit df523ac into master Nov 20, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@thijskh thijskh deleted the bugfix/NameIDPolicy branch Nov 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment