Updated functionality for logging failed access

- Added testing to cover.
- Linked logging into Laravel's monolog logging system and made log
channel configurable.
- Updated env var names to be specific to login access.
- Added extra locations as to where failed logins would be captured.

Related to #1881 and #728

.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

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
 {
@@ -161,19 +162,18 @@ class ActivityService
     }
 
     /**
-     * Log failed accesses, for further processing by tools like Fail2Ban
-     *
-     * @param username
-     * @return void
+     * Log out a failed login attempt, Providing the given username
+     * as part of the message if the '%u' string is used.
      */
-    public function logFailedAccess($username)
+    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);
     }
 }

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'),
+    ],
 
 ];

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);
     }
 

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>

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
      */

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'));
+    }
 }

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.