flipboxfactory / saml-sp

SAML Service Provider (SP) Plugin for Craft CMS
https://saml-sp.flipboxfactory.com/
Other
19 stars 5 forks source link

Potential bug with getNameID #99

Closed andrewfairlie closed 3 years ago

andrewfairlie commented 3 years ago

I'm new to SSO so this might not be a bug on your side, but I'll report what I'm seeing in case it is.

I'm using the Username/NameID Override feature to override to an email address, and when I go to login I see that it creates the user correctly, but then on the frontend I get a Craft PHP error

Call to a member function getValue() on null

image

I think this might be where the code is looking for the nameID even though I'm overriding it. Is that possibly a bug? Or am I misunderstanding?

andrewfairlie commented 3 years ago

If this helps at all, and I'm not sure if this is a good solution but I managed to hack something together by changing https://github.com/flipboxfactory/saml-sp/blob/master/src/services/ProviderIdentity.php#L53-L59 to...

        $firstAssertion = $this->getFirstAssertion($response, $serviceProvider);
        $nameIdOverride = $idpProvider->nameIdOverride;
        if ($nameIdOverride) {
            // use override
            $attributes = $firstAssertion->getAttributes();
            if (isset($attributes[$nameIdOverride])) {
                $nameID = $attributes[$nameIdOverride][0];
            }
        } else {
            $nameID = $firstAssertion->getNameID()->getValue();
        }

        // Get Identity
        $identity = $this->forceGet(
            $nameID,
            $idpProvider
        );
dsmrt commented 3 years ago

Just off the top of my head, I believe I still need the NameID to exist, but your patch might work fine. Let me look this over and think about it a little bit and see if this would work ok.

dsmrt commented 3 years ago

I think I have this working in a way that shouldn't mess with anyone else's current install.

@andrewfairlie, want to give the latest (version 2.6.5) a try and see how that works for you?

andrewfairlie commented 3 years ago

Hi @dsmrt this looks promising! I just need to get something from my client in order to test this fully, but I'll report back as soon as I have it :)

andrewfairlie commented 3 years ago

Hi @dsmrt yes this works perfectly now, thank you.