Skip to content

Fix issue #571: adhere to authsource referrals-setting#587

Merged
thijskh merged 4 commits into
simplesamlphp:masterfrom
tvdijen:patch-4
Aug 3, 2017
Merged

Fix issue #571: adhere to authsource referrals-setting#587
thijskh merged 4 commits into
simplesamlphp:masterfrom
tvdijen:patch-4

Conversation

@tvdijen

@tvdijen tvdijen commented Mar 30, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Fixes issue #571

@tvdijen

tvdijen commented Mar 30, 2017

Copy link
Copy Markdown
Member Author

It would be great if @tsmgeek could test this

@jaimeperez jaimeperez mentioned this pull request Jun 21, 2017
Closed
mschwager
mschwager reviewed Jul 8, 2017
View reviewed changes
Comment thread modules/ldap/lib/Auth/Process/BaseFilter.php Outdated
$authconfig['ldap.port'] = @$authsource['port'];
$authconfig['ldap.timeout'] = @$authsource['timeout'];
$authconfig['ldap.debug'] = @$authsource['debug'];
$authconfig['ldap.referrals'] = (@$authsource['referrals'] ? @$authsource['referrals'] : false);

@mschwager mschwager Jul 8, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should default to true since SimpleSAML_Auth_LDAP::__construct does.

mschwager
mschwager reviewed Jul 8, 2017
View reviewed changes
Comment thread modules/ldap/lib/Auth/Process/BaseFilter.php Outdated
$port = $this->config->getInteger('ldap.port', 389);
$enable_tls = $this->config->getBoolean('ldap.enable_tls', false);
$debug = $this->config->getBoolean('ldap.debug', false);
$referrals = $this->config->getBoolean('ldap.referrals', false);

@mschwager mschwager Jul 8, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

@mschwager

mschwager commented Jul 8, 2017

Copy link
Copy Markdown
Contributor

Unfortunately I'm not able to easily test this since I don't have access to an LDAP server where turning off referrals would change anything.

However, I'd be okay with pulling in the changes anyway. We're simply providing a method to access the already existing argument in SimpleSAML_Auth_LDAP::__construct via configuration. What do you think @jaimeperez?

@tvdijen tvdijen changed the title Fix issue #571: adhere to authsource refferals-setting Fix issue #571: adhere to authsource referrals-setting Jul 9, 2017
@thijskh

thijskh commented Aug 3, 2017

Copy link
Copy Markdown
Member

I agree with you @mschwager that this is good to merge. However, it seems the branch has lost some content now. And @mschwager has IMO a valid comment. @tvdijen Can you look into that?

@tvdijen

tvdijen commented Aug 3, 2017

Copy link
Copy Markdown
Member Author

@thijskh What are you missing?
What I did, recently, is resolving a conflict, because the SSP master branch has been changed since I sent in this pull request..
I'll fix the default value later today

@thijskh

thijskh commented Aug 3, 2017

Copy link
Copy Markdown
Member

I was mistaken, indeed it looks fine otherwise.

@tvdijen

tvdijen commented Aug 3, 2017

Copy link
Copy Markdown
Member Author

I just set the default value to true.. Tests are running

@thijskh thijskh merged commit aa7723a into simplesamlphp:master Aug 3, 2017
@tvdijen tvdijen deleted the patch-4 branch August 3, 2017 11:52
tvdijen added a commit to tvdijen/simplesamlphp that referenced this pull request Aug 7, 2017
@tvdijen
Fix issue simplesamlphp#571: adhere to authsource referrals-setting (s…
3dd7267 
…implesamlphp#587)
jaimeperez pushed a commit that referenced this pull request Aug 8, 2017
@tvdijen @jaimeperez
Fix issue #571: adhere to authsource referrals-setting (#587)
b3a8cd6
@thijskh

thijskh commented Aug 15, 2017

Copy link
Copy Markdown
Member

@tvdijen Do you think this option needs to be added to the documentation?

@tvdijen

tvdijen commented Aug 15, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

Nope, this was an actual bug, not something new.. If you specify an authsource for the authproc-filter to use, you'd expect it to take over the settings defined in that authsource.

Paragraph 3:
https://simplesamlphp.org/docs/stable/ldap:ldap

* LDAP connection settings can be retrieved from an ldap:LDAP
* authsource. Specify the authsource name here to pull that
* data from the authsources.php file in the config folder.

Same text is also in the doc-file of the ldap-module.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Mar 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@thijskh thijskh thijskh approved these changes

+1 more reviewer

@mschwager mschwager mschwager left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tvdijen @mschwager @thijskh @jaimeperez