diff --git a/.env.example.complete b/.env.example.complete
index 26dfdc54b..d7dd02b9a 100644
--- a/.env.example.complete
+++ b/.env.example.complete
@@ -272,8 +272,10 @@ API_MAX_ITEM_COUNT=500
# The number of API requests that can be made per minute by a single user.
API_REQUESTS_PER_MIN=180
-# Failed access
-# message to log into webserver logs in case of failed access, for further processing by tools like Fail2Ban
-# Apache users should use : user "%u" authentication failure for "BookStack"
-# Nginx users should use : user "%u" was not found in "BookStack"
-FAILED_ACCESS_MESSAGE=''
+# Enable the logging of failed email+password logins with the given message
+# The defaul log channel below uses the php 'error_log' function which commonly
+# results in messages being output to the webserver error logs.
+# The message can contain a %u parameter which will be replaced with the login
+# user identifier (Username or email).
+LOG_FAILED_LOGIN_MESSAGE=false
+LOG_FAILED_LOGIN_CHANNEL=errorlog_plain_webserver
diff --git a/app/Actions/ActivityService.php b/app/Actions/ActivityService.php
index 0e3ac7861..e6b004f01 100644
--- a/app/Actions/ActivityService.php
+++ b/app/Actions/ActivityService.php
@@ -4,6 +4,7 @@ use BookStack\Auth\Permissions\PermissionService;
use BookStack\Auth\User;
use BookStack\Entities\Entity;
use Illuminate\Support\Collection;
+use Illuminate\Support\Facades\Log;
class ActivityService
{
@@ -49,7 +50,7 @@ class ActivityService
protected function newActivityForUser(string $key, ?int $bookId = null): Activity
{
return $this->activity->newInstance()->forceFill([
- 'key' => strtolower($key),
+ 'key' => strtolower($key),
'user_id' => $this->user->id,
'book_id' => $bookId ?? 0,
]);
@@ -64,8 +65,8 @@ class ActivityService
{
$activities = $entity->activity()->get();
$entity->activity()->update([
- 'extra' => $entity->name,
- 'entity_id' => 0,
+ 'extra' => $entity->name,
+ 'entity_id' => 0,
'entity_type' => '',
]);
return $activities;
@@ -99,7 +100,7 @@ class ActivityService
$query = $this->activity->newQuery()->where('entity_type', '=', $entity->getMorphClass())
->where('entity_id', '=', $entity->id);
}
-
+
$activity = $this->permissionService
->filterRestrictedEntityRelations($query, 'activities', 'entity_id', 'entity_type')
->orderBy('created_at', 'desc')
@@ -161,19 +162,18 @@ class ActivityService
}
/**
- * Log failed accesses, for further processing by tools like Fail2Ban
- *
- * @param username
- * @return void
- */
- public function logFailedAccess($username)
+ * Log out a failed login attempt, Providing the given username
+ * as part of the message if the '%u' string is used.
+ */
+ public function logFailedLogin(string $username)
{
- $log_msg = config('logging.failed_access_message');
-
- if (!is_string($username) || !is_string($log_msg) || strlen($log_msg)<1)
+ $message = config('logging.failed_login.message');
+ if (!$message) {
return;
+ }
- $log_msg = str_replace("%u", $username, $log_msg);
- error_log($log_msg, 4);
+ $message = str_replace("%u", $username, $message);
+ $channel = config('logging.failed_login.channel');
+ Log::channel($channel)->warning($message);
}
}
diff --git a/app/Config/logging.php b/app/Config/logging.php
index ba77ba81e..afd56e482 100644
--- a/app/Config/logging.php
+++ b/app/Config/logging.php
@@ -1,5 +1,7 @@
<?php
+use Monolog\Formatter\LineFormatter;
+use Monolog\Handler\ErrorLogHandler;
use Monolog\Handler\NullHandler;
use Monolog\Handler\StreamHandler;
@@ -73,6 +75,19 @@ return [
'level' => 'debug',
],
+ // Custom errorlog implementation that logs out a plain,
+ // non-formatted message intended for the webserver log.
+ 'errorlog_plain_webserver' => [
+ 'driver' => 'monolog',
+ 'level' => 'debug',
+ 'handler' => ErrorLogHandler::class,
+ 'handler_with' => [4],
+ 'formatter' => LineFormatter::class,
+ 'formatter_with' => [
+ 'format' => "%message%",
+ ],
+ ],
+
'null' => [
'driver' => 'monolog',
'handler' => NullHandler::class,
@@ -86,9 +101,12 @@ return [
],
],
- // Failed Access Message
- // Defines the message to log into webserver logs in case of failed access,
- // for further processing by tools like Fail2Ban.
- 'failed_access_message' => env('FAILED_ACCESS_MESSAGE', ''),
+
+ // Failed Login Message
+ // Allows a configurable message to be logged when a login request fails.
+ 'failed_login' => [
+ 'message' => env('LOG_FAILED_LOGIN_MESSAGE', null),
+ 'channel' => env('LOG_FAILED_LOGIN_CHANNEL', 'errorlog_plain_webserver'),
+ ],
];
diff --git a/app/Http/Controllers/Auth/LoginController.php b/app/Http/Controllers/Auth/LoginController.php
index f031c12cf..cd7a4db32 100644
--- a/app/Http/Controllers/Auth/LoginController.php
+++ b/app/Http/Controllers/Auth/LoginController.php
@@ -99,6 +99,7 @@ class LoginController extends Controller
public function login(Request $request)
{
$this->validateLogin($request);
+ $username = $request->get($this->username());
// If the class is using the ThrottlesLogins trait, we can automatically throttle
// the login attempts for this application. We'll key this by the username and
@@ -107,9 +108,7 @@ class LoginController extends Controller
$this->hasTooManyLoginAttempts($request)) {
$this->fireLockoutEvent($request);
- // Also log some error message
- Activity::logFailedAccess($request->get($this->username()));
-
+ Activity::logFailedLogin($username);
return $this->sendLockoutResponse($request);
}
@@ -118,6 +117,7 @@ class LoginController extends Controller
return $this->sendLoginResponse($request);
}
} catch (LoginAttemptException $exception) {
+ Activity::logFailedLogin($username);
return $this->sendLoginAttemptExceptionResponse($exception, $request);
}
@@ -126,9 +126,7 @@ class LoginController extends Controller
// user surpasses their maximum number of attempts they will get locked out.
$this->incrementLoginAttempts($request);
- // Also log some error message
- Activity::logFailedAccess($request->get($this->username()));
-
+ Activity::logFailedLogin($username);
return $this->sendFailedLoginResponse($request);
}
diff --git a/phpunit.xml b/phpunit.xml
index 85538c446..70f1c1f9c 100644
--- a/phpunit.xml
+++ b/phpunit.xml
@@ -51,5 +51,7 @@
<server name="DEBUGBAR_ENABLED" value="false"/>
<server name="SAML2_ENABLED" value="false"/>
<server name="API_REQUESTS_PER_MIN" value="180"/>
+ <server name="LOG_FAILED_LOGIN_MESSAGE" value=""/>
+ <server name="LOG_FAILED_LOGIN_CHANNEL" value="testing"/>
</php>
</phpunit>
diff --git a/tests/Auth/AuthTest.php b/tests/Auth/AuthTest.php
index f1f476966..8900eeeba 100644
--- a/tests/Auth/AuthTest.php
+++ b/tests/Auth/AuthTest.php
@@ -401,6 +401,18 @@ class AuthTest extends BrowserKitTest
$this->assertFalse(auth('saml2')->check());
}
+ public function test_failed_logins_are_logged_when_message_configured()
+ {
+ $log = $this->withTestLogger();
+ config()->set(['logging.failed_login.message' => 'Failed login for %u']);
+
+ $this->post('/login', ['email' => 'admin@example.com', 'password' => 'cattreedog']);
+ $this->assertTrue($log->hasWarningThatContains('Failed login for admin@example.com'));
+
+ $this->post('/login', ['email' => 'admin@admin.com', 'password' => 'password']);
+ $this->assertFalse($log->hasWarningThatContains('Failed login for admin@admin.com'));
+ }
+
/**
* Perform a login
*/
diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php
index ed8748f08..0d92241bb 100644
--- a/tests/Auth/LdapTest.php
+++ b/tests/Auth/LdapTest.php
@@ -593,4 +593,17 @@ class LdapTest extends BrowserKitTest
$this->see('A user with the email tester@example.com already exists but with different credentials');
}
+
+ public function test_failed_logins_are_logged_when_message_configured()
+ {
+ $log = $this->withTestLogger();
+ config()->set(['logging.failed_login.message' => 'Failed login for %u']);
+
+ $this->commonLdapMocks(1, 1, 1, 1, 1);
+ $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1)
+ ->andReturn(['count' => 0]);
+
+ $this->post('/login', ['username' => 'timmyjenkins', 'password' => 'cattreedog']);
+ $this->assertTrue($log->hasWarningThatContains('Failed login for timmyjenkins'));
+ }
}
diff --git a/tests/Unit/ConfigTest.php b/tests/Unit/ConfigTest.php
index 69b737d7d..1374b3aa9 100644
--- a/tests/Unit/ConfigTest.php
+++ b/tests/Unit/ConfigTest.php
@@ -1,5 +1,6 @@
<?php namespace Tests\Unit;
+use Illuminate\Support\Facades\Log;
use Tests\TestCase;
/**
@@ -36,6 +37,28 @@ class ConfigTest extends TestCase
$this->checkEnvConfigResult('APP_URL', $oldDefault, 'app.url', '');
}
+ public function test_errorlog_plain_webserver_channel()
+ {
+ // We can't full test this due to it being targeted for the SAPI logging handler
+ // so we just overwrite that component so we can capture the error log output.
+ config()->set([
+ 'logging.channels.errorlog_plain_webserver.handler_with' => [0],
+ ]);
+
+ $temp = tempnam(sys_get_temp_dir(), 'bs-test');
+ $original = ini_set( 'error_log', $temp);
+
+ Log::channel('errorlog_plain_webserver')->info('Aww, look, a cute puppy');
+
+ ini_set( 'error_log', $original);
+
+ $output = file_get_contents($temp);
+ $this->assertStringContainsString('Aww, look, a cute puppy', $output);
+ $this->assertStringNotContainsString('INFO', $output);
+ $this->assertStringNotContainsString('info', $output);
+ $this->assertStringNotContainsString('testing', $output);
+ }
+
/**
* Set an environment variable of the given name and value
* then check the given config key to see if it matches the given result.