search

kingstarter / laravel-saml

A laravel 5.4 / 5.5 SAML plugin that transforms laravel into an IDP.
MIT License
55 stars 29 forks source link

AudienceRestriction #1

Closed S43534 closed 7 years ago

S43534 commented 7 years ago

Hey,

great package! Just at the time when I needed it :-)

I am not familiar at all with SAML, but I had to make one change to make this package work (as IdP for Nextcloud SAML):

SAMLAuth, line 181 before new \LightSaml\Model\Assertion\AudienceRestriction([$authnRequest->getAssertionConsumerServiceURL()])

after new \LightSaml\Model\Assertion\AudienceRestriction([$authnRequest->getIssuer()->getValue()])

I don't know if that's a general issue or an issue related to Nextcloud. If it's not a general issue I would propose to make the audience restriction a value that is configurable (defaulting to the current value).

Best regards Sebastian

steve-ks commented 7 years ago

Hi Sebastian, thanks for the contribution. I was just thinking if this might be a problem with the correct configuration within the config/saml.php file. Using the issuer as assertion URL sounds for me that something is placed wrongly there, but I am not sure for Nextcloud. Have not yet used it with laravel.

As far as I can tell and see most other applications are working pretty much ok using the assertion URL (this is actually also the expected behaviour from lightSaml). At least it works with canvas LMS, kanboard and InvoicePlane.

Could you please test if switching for your nextcloud entry under the sp array within the saml config file both issuer and destination or maybe replacing the destination value with the one from issuer might solve the problem?

Directly adding a configuration value might be a problem as it could break the functionality for all other webapps. But I might consider adding something to the sp array.

Greetings, Steve

S43534 commented 7 years ago

Hey,

thanks for the answer!

It might well have to do with Nextcloud, I have strange effects there while other SPs work. Regarding the config value: new \LightSaml\Model\Assertion\AudienceRestriction([config('new.config.value', $authnRequest->getAssertionConsumerServiceURL())]) shouldn't break other apps as it defaults to the current value if the config value is not present.

But I will check the configuration and see if the error is there.

Sebastian

steve-ks commented 7 years ago

Good to know that this does not affect other SPs in your setting. Problem would be in case of using multiple SPs at once when having one overall config value. Even with a default setting this might break all SPs. I will think of a solution setting a specific SP value within the SP array, by this we could enable the config value for specific SPs only. I already tried something yesterday. I will push a solution as soon as it works.

S43534 commented 7 years ago

Ah, got it. Great, thx!

steve-ks commented 7 years ago

Added Tag 1.0.1 with the bugfix. Tried everything, my SPs are not at all affected regardless of the new config value. Anyways debugging the SAMLResponse message showed that the audience restriction value is modified for specific SPs when using the config value.

Could you please test if this works for your problem?

S43534 commented 7 years ago

Sorry for not coming back to you earlier.

It works just as intended - thx very much again!