Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow to set a custom timeout for ldap connections #35355

Merged
merged 1 commit into from
Nov 30, 2022
Merged

Allow to set a custom timeout for ldap connections #35355

merged 1 commit into from
Nov 30, 2022

Conversation

artonge
Copy link
Contributor

@artonge artonge commented Nov 23, 2022 •
edited
Loading

Not tested, only blind copying and pasting. Could @blizzz or @come-nc test this new setting ? I could test with a single LDAP server, but I guess we should try with a primary and a backup server.

@artonge artonge self-assigned this Nov 23, 2022
@artonge artonge added this to the Nextcloud 26 milestone Nov 23, 2022
@artonge artonge added 3. to review Waiting for reviews feature: ldap labels Nov 23, 2022
@artonge
Allow to set a custom timeout for ldap connections
a781ae3 
Signed-off-by: Louis Chemineau <louis@chmn.me>
@artonge artonge force-pushed the artonge/feat/ldap_connection_timeout branch from 0ad63f6 to a781ae3 Compare November 23, 2022 10:37
@artonge
Copy link
Contributor Author

artonge commented Nov 23, 2022

/backport to stable24

@artonge
Copy link
Contributor Author

artonge commented Nov 23, 2022

/backport to stable25

github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 23, 2022
View reviewed changes
apps/user_ldap/lib/Connection.php
@@ -649,6 +649,10 @@
throw new ServerNotAvailableException('Could not disable LDAP referrals.');
}

if (!$this->ldap->setOption($this->ldapConnectionRes, LDAP_OPT_NETWORK_TIMEOUT, $this->configuration->ldapConnectionTimeout)) {

Check failure

Code scanning / Psalm

UndefinedConstant

Const LDAP_OPT_NETWORK_TIMEOUT is not defined
apps/user_ldap/lib/Connection.php
@@ -649,6 +649,10 @@
throw new ServerNotAvailableException('Could not disable LDAP referrals.');
}

if (!$this->ldap->setOption($this->ldapConnectionRes, LDAP_OPT_NETWORK_TIMEOUT, $this->configuration->ldapConnectionTimeout)) {

Check notice

Code scanning / Psalm

PossiblyFalseArgument

Argument 1 of OCA\User_LDAP\ILDAPWrapper::setOption cannot be false, possibly LDAP\Connection|resource value expected
@artonge
Copy link
Contributor Author

artonge commented Nov 24, 2022

@blizzz is this enough ? What about psalm errors ? Why aren't they triggered for the other lines 🤔

@blizzz
Copy link
Member

blizzz commented Nov 30, 2022 •
edited
Loading

@blizzz is this enough ? What about psalm errors ? Why aren't they triggered for the other lines thinking

They actually are, but de facto we will not see a false here. Only resources or resource classes, but I failed to get a false from ldap_connect, so it is very theoretical.

@blizzz blizzz added the pending documentation This pull request needs an associated documentation update label Nov 30, 2022
@blizzz blizzz merged commit b16c983 into master Nov 30, 2022
@blizzz blizzz deleted the artonge/feat/ldap_connection_timeout branch November 30, 2022 13:44
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Nov 30, 2022
Merged
@artonge
Copy link
Contributor Author

artonge commented Dec 5, 2022

/backport to stable25

@artonge artonge mentioned this pull request Dec 5, 2022
Merged
@backportbot-nextcloud
Copy link

The backport to stable25 failed. Please do this backport manually.

artonge added a commit to nextcloud/documentation that referenced this pull request Feb 7, 2023
@artonge
Add entry for ldapConnectionTimeout ldap option
9725a4e 
nextcloud/server#35355
Signed-off-by: Louis Chemineau <louis@chmn.me>
@artonge artonge mentioned this pull request Feb 7, 2023
Merged
backportbot-nextcloud bot pushed a commit to nextcloud/documentation that referenced this pull request Feb 8, 2023
@artonge @backportbot-nextcloud
Add entry for ldapConnectionTimeout ldap option
2d83039 
nextcloud/server#35355
Signed-off-by: Louis Chemineau <louis@chmn.me>
backportbot-nextcloud bot pushed a commit to nextcloud/documentation that referenced this pull request Feb 8, 2023
@artonge @backportbot-nextcloud
Add entry for ldapConnectionTimeout ldap option
af53c1e 
nextcloud/server#35355
Signed-off-by: Louis Chemineau <louis@chmn.me>
@DaphneMuller
Copy link

hello @artonge
Thank you so much for your work on this pull request! This ticket seems to have the tag 'pending documentation'.
Is there any chance you could clarify what documentation is missing? Is this for admins or for app developers?

@artonge
Copy link
Contributor Author

artonge commented Feb 21, 2023 •
edited
Loading

Simply forgot to remove it nextcloud/documentation#9618
Thanks for the ping :)

@artonge artonge removed the pending documentation This pull request needs an associated documentation update label Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blizzz blizzz blizzz approved these changes

@come-nc come-nc come-nc approved these changes

@PVince81 PVince81 Awaiting requested review from PVince81

@CarlSchwan CarlSchwan Awaiting requested review from CarlSchwan

Assignees

@artonge artonge

Labels
3. to review Waiting for reviews feature: ldap
Projects
None yet
Milestone
Nextcloud 26
Development

Successfully merging this pull request may close these issues.
5 participants
@artonge @blizzz @DaphneMuller @come-nc @skjnldsv