bugfix: wrapped the building of authsource config with issets. #539

doedje · 2016-12-23T09:14:01Z

The change in AttributeAddFromLDAP.php was needed to discover the bug in the first place. Without it there was never an error in the log while clearly an exception was thrown somewhere during $this->getLdap().

The change in BaseFilter.php makes sure that undefined configuration parameters in $authsource are not set to null in $authconfig, but rather stay undefined.

Not doing so did result in the following exeception because ldap.port was indeed undefined in my authsources.php but was set to null because of the code in BaseFilter.php. The error never ended up in the log and I was presuming the value to be set to it's default, as it does when using the authsource as a direct authsource. Confusion all along.... =]

Dec 23 08:28:10 simplesamlphp WARNING [94b0f44d76] AttributeAddFromLDAP: exception = exception 'Exception' with message 'ldap:AuthProcess: The option 'ldap.port' is not a valid integer value.' in /Users/remy/git/saml-IdP/lib/SimpleSAML/Configuration.php:737
Stack trace:
#0 /Users/remy/git/saml-IdP/modules/ldap/lib/Auth/Process/BaseFilter.php(267): SimpleSAML_Configuration->getInteger('ldap.port', 389)
#1 /Users/remy/git/saml-IdP/modules/ldap/lib/Auth/Process/AttributeAddFromLDAP.php(172): sspmod_ldap_Auth_Process_BaseFilter->getLdap()
....

the silent fail on searchformultiple(...) did not show anything in the log when actually it was the $this->getLdap() that failed.

Not doing this gave me errors about ldap.port and ldap.timeout not being an integer (but NULL) from Configuration.php Dec 23 08:28:10 simplesamlphp WARNING [94b0f44d76] AttributeAddFromLDAP: exception = exception 'Exception' with message 'ldap:AuthProcess: The option 'ldap.port' is not a valid integer value.' in /Users/remy/git/saml-IdP/lib/SimpleSAML/Configuration.php:737 Stack trace: #0 /Users/remy/git/saml-IdP/modules/ldap/lib/Auth/Process/BaseFilter.php(267): SimpleSAML_Configuration->getInteger('ldap.port', 389) #1 /Users/remy/git/saml-IdP/modules/ldap/lib/Auth/Process/AttributeAddFromLDAP.php(172): sspmod_ldap_Auth_Process_BaseFilter->getLdap() ...

thijskh · 2016-12-23T09:32:23Z

Thanks Remy. If you are using the isset tests, can we then leave off those ugly @ modifiers that tried to silence any notices?

doedje · 2016-12-23T09:38:45Z

@thijskh I guess so, my php skills are a bit rusty from old days. Didn't bother to google what those @ do anyway. I removed some, my test still works. Will remove all of them and push them in this PR.

doedje · 2016-12-23T15:33:05Z

No more ugly @ in the code... =]

thijskh · 2016-12-23T15:41:13Z

Great. I'll review it in more detail next week... (unless someone beats me to it)

doedje · 2016-12-23T15:45:32Z

Fijne kerst! (dutch for 'merry christmas')

doedje · 2017-01-19T14:57:16Z

any change you looked into this issue?

doedje · 2017-01-24T09:58:59Z

accidentally pushed another feature against my master..... will set this straight right away..

This reverts commit 78ccac0.

thijskh · 2017-01-31T13:25:37Z

modules/ldap/lib/Auth/Process/BaseFilter.php

+            if (isset($authsource['debug'])) {
+              $authconfig['ldap.debug']      = $authsource['debug'];
+            }
+            if (isset($authsource['search.base']) && $authsource['search.enable']) {


This will now generate a notice when search.enable is not set. I think that is not desired...

Same for checks below.

I'll change the code on these...

thijskh · 2017-01-31T13:26:03Z

modules/ldap/lib/Auth/Process/BaseFilter.php

+              $authconfig['ldap.enable_tls'] = $authsource['enable_tls'];
+            }
+            if (isset($authsource['port'])) {
+              $authconfig['ldap.port']       = $authsource['port'];


Spacing does not seem to follow the coding guidelines (4 spaces for indent)

sorry 'bout that.... fixed in upcoming push.

thijskh · 2017-01-31T13:28:43Z

modules/ldap/lib/Auth/Process/BaseFilter.php

            // Only set the username attribute if the authsource specifies one attribute
-            if (@$authsource['search.enable'] && is_array(@$authsource['search.attributes'])
+            if ($authsource['search.enable'] && is_array($authsource['search.attributes'])


I think we're not sure these two will be set at this point?

true... both need a check to avoid a notice...

….... Since I moved this code into an if that already only gets executed when authsource['search.enable'] = true it is no longer needed in this check....

doedje · 2017-02-06T15:17:58Z

fixed indent and restructured code to avoid notices.

jaimeperez · 2017-06-21T12:43:49Z

@thijskh, you've been following this. Do you think the latest changes by @doedje are fine so that we can merge this?

thijskh · 2017-06-21T14:36:47Z

modules/ldap/lib/Auth/Process/AttributeAddFromLDAP.php

 *
 * @author Yørn de Jong
 * @author Jaime Perez
 * @author Steve Moitozo
 * @author JAARS, Inc.
 * @author Ryan Panning
+ * @author Remy Blom <remy.blom@hku.nl>


Would think we'd better get rid of these changelog and @author comments; that info is, in more accurate form, in vcs metadata...

thijskh · 2017-06-21T14:37:09Z

Thanks @doedje!

thijskh · 2017-06-21T14:40:17Z

I have taken the liberty to squash the commits, because the accidental changes+revert cause a lot of noise in the history otherwise. Might want to do that yourself next time.

doedje · 2017-06-22T07:20:46Z

I totally agree with you, I will have to look into how I to do that... thanx!

jaimeperez · 2017-06-22T10:10:46Z

Thanks a lot @doedje & @thijskh! 👏

…esamlphp#539) * Adjusted the silent fail to log a warning when $this->getLdap() fails the silent fail on searchformultiple(...) did not show anything in the log when actually it was the $this->getLdap() that failed. * Bugfix: Wrapped the building of authsource config with issets Not doing this gave me errors about ldap.port and ldap.timeout not being an integer (but NULL) from Configuration.php Dec 23 08:28:10 simplesamlphp WARNING [94b0f44d76] AttributeAddFromLDAP: exception = exception 'Exception' with message 'ldap:AuthProcess: The option 'ldap.port' is not a valid integer value.' in /Users/remy/git/saml-IdP/lib/SimpleSAML/Configuration.php:737 Stack trace: #0 /Users/remy/git/saml-IdP/modules/ldap/lib/Auth/Process/BaseFilter.php(267): SimpleSAML_Configuration->getInteger('ldap.port', 389) #1 /Users/remy/git/saml-IdP/modules/ldap/lib/Auth/Process/AttributeAddFromLDAP.php(172): sspmod_ldap_Auth_Process_BaseFilter->getLdap() ... * removed the @ as thijskh suggested... * feature: AttributeCopy can take array for 1 attribute * Revert "feature: AttributeCopy can take array for 1 attribute" This reverts commit 78ccac0. * BaseFilter.php: fix indent and added more isset checks... * BaseFilter.php: removed an unneeded if ($authsource['search.enable'] .... Since I moved this code into an if that already only gets executed when authsource['search.enable'] = true it is no longer needed in this check....

doedje added 3 commits December 23, 2016 08:39

Adjusted the silent fail to log a warning when $this->getLdap() fails

fd867db

the silent fail on searchformultiple(...) did not show anything in the log when actually it was the $this->getLdap() that failed.

Merge branch 'master' of github.com:simplesamlphp/simplesamlphp

be6ba26

removed the @ as thijskh suggested...

a836adf

doedje added 2 commits January 23, 2017 17:10

feature: AttributeCopy can take array for 1 attribute

78ccac0

Merge branch 'master' of github.com:simplesamlphp/simplesamlphp

f260e7e

Revert "feature: AttributeCopy can take array for 1 attribute"

5e0df88

This reverts commit 78ccac0.

thijskh reviewed Jan 31, 2017

View reviewed changes

doedje added 2 commits February 6, 2017 15:32

BaseFilter.php: fix indent and added more isset checks...

10e72a6

BaseFilter.php: removed an unneeded if ($authsource['search.enable'] …

73ba8ec

….... Since I moved this code into an if that already only gets executed when authsource['search.enable'] = true it is no longer needed in this check....

jaimeperez added the bug label Jun 21, 2017

thijskh approved these changes Jun 21, 2017

View reviewed changes

thijskh reviewed Jun 21, 2017

View reviewed changes

thijskh merged commit 88652a2 into simplesamlphp:master Jun 21, 2017

doedje deleted the master branch June 22, 2017 07:21

jaimeperez pushed a commit that referenced this pull request Aug 8, 2017

bugfix: wrapped the building of authsource config with issets. (#539)

66b4fbc

github-actions bot locked as resolved and limited conversation to collaborators Mar 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix: wrapped the building of authsource config with issets. #539

bugfix: wrapped the building of authsource config with issets. #539

doedje commented Dec 23, 2016 •

edited

thijskh commented Dec 23, 2016

doedje commented Dec 23, 2016

doedje commented Dec 23, 2016

thijskh commented Dec 23, 2016

doedje commented Dec 23, 2016

doedje commented Jan 19, 2017

doedje commented Jan 24, 2017

thijskh Jan 31, 2017

thijskh Jan 31, 2017

doedje Feb 6, 2017

thijskh Jan 31, 2017

doedje Feb 6, 2017

thijskh Jan 31, 2017

doedje Feb 6, 2017

doedje commented Feb 6, 2017

jaimeperez commented Jun 21, 2017

thijskh Jun 21, 2017

thijskh commented Jun 21, 2017

thijskh commented Jun 21, 2017

doedje commented Jun 22, 2017

jaimeperez commented Jun 22, 2017

bugfix: wrapped the building of authsource config with issets. #539

bugfix: wrapped the building of authsource config with issets. #539

Conversation

doedje commented Dec 23, 2016 • edited

thijskh commented Dec 23, 2016

doedje commented Dec 23, 2016

doedje commented Dec 23, 2016

thijskh commented Dec 23, 2016

doedje commented Dec 23, 2016

doedje commented Jan 19, 2017

doedje commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doedje commented Feb 6, 2017

jaimeperez commented Jun 21, 2017

Choose a reason for hiding this comment

thijskh commented Jun 21, 2017

thijskh commented Jun 21, 2017

doedje commented Jun 22, 2017

jaimeperez commented Jun 22, 2017

doedje commented Dec 23, 2016 •

edited