Skip to content

LDAP's checkPassword should only catch when a user was not found, fixes #2431#3324

Merged
LukasReschke merged 1 commit into
masterfrom
fix-2431
Feb 13, 2017
Merged

LDAP's checkPassword should only catch when a user was not found, fixes #2431#3324
LukasReschke merged 1 commit into
masterfrom
fix-2431

Conversation

@blizzz

@blizzz blizzz commented Jan 30, 2017

Copy link
Copy Markdown
Member

LDAP's checkPassword() always returned false, no matter whether the user who tried to log in could not be found on the server, or the server is unavailable. This was due to too broadly catching Exceptions, which now is fixed.

While debugging I figured out that ldap_search calls did not pass error processing in the wrapper, because of a different return format.

In order to reproduce the whole thing I

  1. set a breakpoint at https://github.com/nextcloud/server/blob/master/lib/private/User/Session.php#L632 (often it wouldn't stop, a breakpoint at L518 and some stepping helps then)
  2. I kept the client running, restarted the webserver, and changed something in the synced folder
  3. When the IDE stopped at the breakpoint, I turned of the LDAP server

When letting it go, usually the app password got revoked.

#2431

Also fixes error processing after ldap_search, due to different return format

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz added this to the Nextcloud 12.0 milestone Jan 30, 2017
@blizzz blizzz added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 30, 2017
@blizzz

blizzz commented Jan 30, 2017

Copy link
Copy Markdown
Member Author

@eggithub @laurentdinclaux @lephisto could you check whether this solves your issues with #2431 ?

@blizzz blizzz mentioned this pull request Jan 30, 2017
@eggithub

Copy link
Copy Markdown

@blizz I do not use LDAP...

@blizzz

blizzz commented Jan 30, 2017

Copy link
Copy Markdown
Member Author

@eggithu how would I know? Please state your full setup in the original issue (ideally using https://raw.githubusercontent.com/nextcloud/server/master/issue_template.md)

@eggithub

Copy link
Copy Markdown

Hi @blizz,

You could not have known I don't use LDAP. My issue is #2431. I'm not the one who made notice of this issue in the #2431, just that I mentioned also seeing app passwords disappear. I don't use any external authentication api, is all I can say for now. I'm heading back to #2431...

Cheers!

return true;
}

if($this->curFunc === 'ldap_search' && is_array($result)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we could simplify this with return in_array(false, $result, true)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah we could. With big results sets I expect this to be faster (not that it would matter to much). At least few years ago it was better to loop through an array, create a new one with those values as key and isset them over an in_array call. http://nickology.com/2012/07/03/php-faster-array-lookup-than-using-in_array/

@ChristophWurst ChristophWurst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tested and works 👍

I set up an LDAP user, created an app password and sent requests with curl. After turning off the LDAP server, requests fail with

<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAV\Exception\ServiceUnavailable</s:exception>
  <s:message>OC\ServerNotAvailableException: Lost connection to LDAP server.</s:message>
</d:error>

but the app password does not vanish.

@LukasReschke LukasReschke merged commit 5b9f96a into master Feb 13, 2017
@LukasReschke LukasReschke deleted the fix-2431 branch February 13, 2017 22:21
@oparoz

oparoz commented Feb 13, 2017

Copy link
Copy Markdown
Member

Backport please @blizzz , we need this in for 11.0.2 :)

@blizzz

blizzz commented Feb 15, 2017

Copy link
Copy Markdown
Member Author

Backport to stable11 goes #3493, reviewers needed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants