pydio/pydio-core — pull request #571 by chusopr (2014-05-21 16:14:52+00:00)
Title: Log invalid logins

A costumer asked us to log invalid log in attempts to detect brute force attacks and I'm pushing it upstream so you can merge it if you find it useful too.

Please note an interesting change in AJXP_Sabre_AuthBackendDigest::authenticate().
Original code in core/src/core/classes/sabredav/ajaxplorer/class.AJXP_Sabre_AuthBackendDigest.php was:

```php
$success = parent::authenticate($server, $realm);
if ($success) {
// Do stuff
}
if ($success === false) {
throw new Sabre\DAV\Exception\NotAuthenticated();
}
```

$success value is parent::authenticate() return value. But when parent::authenticate() is successful it returns true, otherwise an exception is thrown.
So the second code block —the one when $success === false— will never be run because $success will never be false.
This can be fixed by catching exception thrown by parent::authenticate(), but this makes no sense because the only thing this unreachable statement does is to throw the exact same exception.

But, what this pull request intends to do is to log invalid log in attempts. In that case, that if statement will be modified this way:

```php
if ($success === false) {
AJXP_Logger::warning(__CLASS__, "Login failed", array("user" => $this->currentUser, "error" => "Invalid WebDAV user or password"));
throw new Sabre\DAV\Exception\NotAuthenticated();
}
```

So, this 'if' statement needs to be executed. To accomplish this, I caught parent exception and set $success to false as expected:

```php
try {
$success = parent::authenticate($server, $realm);
}
catch(Exception $e) {
$success = false;
}
```

But this is not enough. The same exception is thrown when no authentication data is received, i.e., if user tries to access anonymously. I think that this should not be logged because this is usually the default behavior in WebDAV clients, should fill logs with a lot of lines and is not actually a break-in attempt.
So we need to know what caused parent exception. This would be possible if parent class threw the exception using optional 'code' parameter (http://www.php.net/manual/en/exception.construct.php) so you can know what caused the exception depending on different codes.
This is not the case and you can only tell what caused the exception depending on exception message:

```php
public function authenticate(DAV\Server $server, $realm)
{
$digest = new HTTP\DigestAuth();

// Hooking up request and response objects
$digest->setHTTPRequest($server->httpRequest);
$digest->setHTTPResponse($server->httpResponse);

$digest->setRealm($realm);
$digest->init();

$username = $digest->getUsername();

// No username was given
if (!$username) {
$digest->requireLogin();
throw new DAV\Exception\NotAuthenticated('No digest authentication headers were found');
}

$hash = $this->getDigestHash($realm, $username);
// If this was false, the user account didn't exist
if ($hash===false || is_null($hash)) {
$digest->requireLogin();
throw new DAV\Exception\NotAuthenticated('The supplied username was not on file');
}
if (!is_string($hash)) {
throw new DAV\Exception('The returned value from getDigestHash must be a string or null');
}

// If this was false, the password or part of the hash was incorrect.
if (!$digest->validateA1($hash)) {
$digest->requireLogin();
throw new DAV\Exception\NotAuthenticated('Incorrect username');
}

$this->currentUser = $username;
return true;
}
```

AJXP_Sabre_AuthBackendDigest::authenticate() performs actions for successful login after calling parent method and doing a weak comparison (==) that checks if $success is true. Exception is only thrown after a strong comparison (===) that checks if $successful is false. Remember:
```php
$success = parent::authenticate($server, $realm);
if ($success) {
// Do stuff
}
if ($success === false) {
throw new Sabre\DAV\Exception\NotAuthenticated();
}
```
So there may be a case —e.g. $success == false, although there is no such case, for the moment— where both weak and opposed strong comparison fail and nothing is done.

In AJXP_Sabre_AuthBackendDigest::authenticate(), log action should be performed for every Exception excepting the one with message 'No digest authentication headers were found'. But Exception should be thrown for every parent Exception with no exceptions. I know, this has begun to sound like a tongue-twister, I'm sorry.

So I solved this by introducing that third case where $success == false && $success !== false and moving Exception there. And log action is only performed when $success === false.
When parent Exception is 'No digest authentication headers were found', $success = 0 (so $success == false && $success !== false) and for all other Exceptions, $success = false:

This will be probably better understood watching the code:

```php
try {
$success = parent::authenticate($server, $realm);
}
catch(Exception $e) {
$success = 0;
if ($e->getMessage() != "No digest authentication headers were found")
$success = false;
}
if ($success) {
// Login successful
}
else
{
if ($success === false) {
// Wrong login
AJXP_Logger::warning(__CLASS__, "Login failed", ...);
}
// Either wrong login or no login data
throw new Sabre\DAV\Exception\NotAuthenticated();
}
```

Of course, instead of doing all this I could modify parent class and use AJXP_Logger there just before those exceptions that should be logged. But since parent class belongs to a third-party library, I preferred to modify nothing there.