Skip to content

Commit 2ed0317

Browse files
ssddanbrownssddanbrown
committed
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 BookStackApp#1881 and BookStackApp#728
1 parent 2f6ff07 commit 2ed0317

File tree

8 files changed

+98
-30
lines changed

8 files changed

+98
-30
lines changed

‎.env.example.complete‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -272,8 +272,10 @@ API_MAX_ITEM_COUNT=500
272272
# The number of API requests that can be made per minute by a single user.
273273
API_REQUESTS_PER_MIN=180
274274

275-
# Failed access
276-
# message to log into webserver logs in case of failed access, for further processing by tools like Fail2Ban
277-
# Apache users should use : user "%u" authentication failure for "BookStack"
278-
# Nginx users should use : user "%u" was not found in "BookStack"
279-
FAILED_ACCESS_MESSAGE=''
275+
# Enable the logging of failed email+password logins with the given message
276+
# The defaul log channel below uses the php 'error_log' function which commonly
277+
# results in messages being output to the webserver error logs.
278+
# The message can contain a %u parameter which will be replaced with the login
279+
# user identifier (Username or email).
280+
LOG_FAILED_LOGIN_MESSAGE=false
281+
LOG_FAILED_LOGIN_CHANNEL=errorlog_plain_webserver

‎app/Actions/ActivityService.php‎

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
use BookStack\Auth\User;
55
use BookStack\Entities\Entity;
66
use Illuminate\Support\Collection;
7+
use Illuminate\Support\Facades\Log;
78

89
class ActivityService
910
{
@@ -49,7 +50,7 @@ public function addMessage(string $activityKey, string $message, ?int $bookId =
4950
protected function newActivityForUser(string $key, ?int $bookId = null): Activity
5051
{
5152
return $this->activity->newInstance()->forceFill([
52-
'key' => strtolower($key),
53+
'key' => strtolower($key),
5354
'user_id' => $this->user->id,
5455
'book_id' => $bookId ?? 0,
5556
]);
@@ -64,8 +65,8 @@ public function removeEntity(Entity $entity): Collection
6465
{
6566
$activities = $entity->activity()->get();
6667
$entity->activity()->update([
67-
'extra' => $entity->name,
68-
'entity_id' => 0,
68+
'extra' => $entity->name,
69+
'entity_id' => 0,
6970
'entity_type' => '',
7071
]);
7172
return $activities;
@@ -99,7 +100,7 @@ public function entityActivity(Entity $entity, int $count = 20, int $page = 1):
99100
$query = $this->activity->newQuery()->where('entity_type', '=', $entity->getMorphClass())
100101
->where('entity_id', '=', $entity->id);
101102
}
102-
103+
103104
$activity = $this->permissionService
104105
->filterRestrictedEntityRelations($query, 'activities', 'entity_id', 'entity_type')
105106
->orderBy('created_at', 'desc')
@@ -161,19 +162,18 @@ protected function setNotification(string $activityKey)
161162
}
162163

163164
/**
164-
* Log failed accesses, for further processing by tools like Fail2Ban
165-
*
166-
* @param username
167-
* @return void
168-
*/
169-
public function logFailedAccess($username)
165+
* Log out a failed login attempt, Providing the given username
166+
* as part of the message if the '%u' string is used.
167+
*/
168+
public function logFailedLogin(string $username)
170169
{
171-
$log_msg = config('logging.failed_access_message');
172-
173-
if (!is_string($username) || !is_string($log_msg) || strlen($log_msg)<1)
170+
$message = config('logging.failed_login.message');
171+
if (!$message) {
174172
return;
173+
}
175174

176-
$log_msg = str_replace("%u", $username, $log_msg);
177-
error_log($log_msg, 4);
175+
$message = str_replace("%u", $username, $message);
176+
$channel = config('logging.failed_login.channel');
177+
Log::channel($channel)->warning($message);
178178
}
179179
}

‎app/Config/logging.php‎

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
<?php
22

3+
use Monolog\Formatter\LineFormatter;
4+
use Monolog\Handler\ErrorLogHandler;
35
use Monolog\Handler\NullHandler;
46
use Monolog\Handler\StreamHandler;
57

@@ -73,6 +75,19 @@
7375
'level' => 'debug',
7476
],
7577

78+
// Custom errorlog implementation that logs out a plain,
79+
// non-formatted message intended for the webserver log.
80+
'errorlog_plain_webserver' => [
81+
'driver' => 'monolog',
82+
'level' => 'debug',
83+
'handler' => ErrorLogHandler::class,
84+
'handler_with' => [4],
85+
'formatter' => LineFormatter::class,
86+
'formatter_with' => [
87+
'format' => "%message%",
88+
],
89+
],
90+
7691
'null' => [
7792
'driver' => 'monolog',
7893
'handler' => NullHandler::class,
@@ -86,9 +101,12 @@
86101
],
87102
],
88103

89-
// Failed Access Message
90-
// Defines the message to log into webserver logs in case of failed access,
91-
// for further processing by tools like Fail2Ban.
92-
'failed_access_message' => env('FAILED_ACCESS_MESSAGE', ''),
104+
105+
// Failed Login Message
106+
// Allows a configurable message to be logged when a login request fails.
107+
'failed_login' => [
108+
'message' => env('LOG_FAILED_LOGIN_MESSAGE', null),
109+
'channel' => env('LOG_FAILED_LOGIN_CHANNEL', 'errorlog_plain_webserver'),
110+
],
93111

94112
];

‎app/Http/Controllers/Auth/LoginController.php‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,7 @@ public function getLogin(Request $request)
9999
public function login(Request $request)
100100
{
101101
$this->validateLogin($request);
102+
$username = $request->get($this->username());
102103

103104
// If the class is using the ThrottlesLogins trait, we can automatically throttle
104105
// the login attempts for this application. We'll key this by the username and
@@ -107,9 +108,7 @@ public function login(Request $request)
107108
$this->hasTooManyLoginAttempts($request)) {
108109
$this->fireLockoutEvent($request);
109110

110-
// Also log some error message
111-
Activity::logFailedAccess($request->get($this->username()));
112-
111+
Activity::logFailedLogin($username);
113112
return $this->sendLockoutResponse($request);
114113
}
115114

@@ -118,6 +117,7 @@ public function login(Request $request)
118117
return $this->sendLoginResponse($request);
119118
}
120119
} catch (LoginAttemptException $exception) {
120+
Activity::logFailedLogin($username);
121121
return $this->sendLoginAttemptExceptionResponse($exception, $request);
122122
}
123123

@@ -126,9 +126,7 @@ public function login(Request $request)
126126
// user surpasses their maximum number of attempts they will get locked out.
127127
$this->incrementLoginAttempts($request);
128128

129-
// Also log some error message
130-
Activity::logFailedAccess($request->get($this->username()));
131-
129+
Activity::logFailedLogin($username);
132130
return $this->sendFailedLoginResponse($request);
133131
}
134132

‎phpunit.xml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,5 +51,7 @@
5151
<server name="DEBUGBAR_ENABLED" value="false"/>
5252
<server name="SAML2_ENABLED" value="false"/>
5353
<server name="API_REQUESTS_PER_MIN" value="180"/>
54+
<server name="LOG_FAILED_LOGIN_MESSAGE" value=""/>
55+
<server name="LOG_FAILED_LOGIN_CHANNEL" value="testing"/>
5456
</php>
5557
</phpunit>

‎tests/Auth/AuthTest.php‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -401,6 +401,18 @@ public function test_login_authenticates_nonadmins_on_default_guard_only()
401401
$this->assertFalse(auth('saml2')->check());
402402
}
403403

404+
public function test_failed_logins_are_logged_when_message_configured()
405+
{
406+
$log = $this->withTestLogger();
407+
config()->set(['logging.failed_login.message' => 'Failed login for %u']);
408+
409+
$this->post('/login', ['email' => 'admin@example.com', 'password' => 'cattreedog']);
410+
$this->assertTrue($log->hasWarningThatContains('Failed login for admin@example.com'));
411+
412+
$this->post('/login', ['email' => 'admin@admin.com', 'password' => 'password']);
413+
$this->assertFalse($log->hasWarningThatContains('Failed login for admin@admin.com'));
414+
}
415+
404416
/**
405417
* Perform a login
406418
*/

‎tests/Auth/LdapTest.php‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -593,4 +593,17 @@ public function test_new_ldap_user_login_with_already_used_email_address_shows_e
593593

594594
$this->see('A user with the email tester@example.com already exists but with different credentials');
595595
}
596+
597+
public function test_failed_logins_are_logged_when_message_configured()
598+
{
599+
$log = $this->withTestLogger();
600+
config()->set(['logging.failed_login.message' => 'Failed login for %u']);
601+
602+
$this->commonLdapMocks(1, 1, 1, 1, 1);
603+
$this->mockLdap->shouldReceive('searchAndGetEntries')->times(1)
604+
->andReturn(['count' => 0]);
605+
606+
$this->post('/login', ['username' => 'timmyjenkins', 'password' => 'cattreedog']);
607+
$this->assertTrue($log->hasWarningThatContains('Failed login for timmyjenkins'));
608+
}
596609
}

‎tests/Unit/ConfigTest.php‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
<?php namespace Tests\Unit;
22

3+
use Illuminate\Support\Facades\Log;
34
use Tests\TestCase;
45

56
/**
@@ -36,6 +37,28 @@ public function test_app_url_blank_if_old_default_value()
3637
$this->checkEnvConfigResult('APP_URL', $oldDefault, 'app.url', '');
3738
}
3839

40+
public function test_errorlog_plain_webserver_channel()
41+
{
42+
// We can't full test this due to it being targeted for the SAPI logging handler
43+
// so we just overwrite that component so we can capture the error log output.
44+
config()->set([
45+
'logging.channels.errorlog_plain_webserver.handler_with' => [0],
46+
]);
47+
48+
$temp = tempnam(sys_get_temp_dir(), 'bs-test');
49+
$original = ini_set( 'error_log', $temp);
50+
51+
Log::channel('errorlog_plain_webserver')->info('Aww, look, a cute puppy');
52+
53+
ini_set( 'error_log', $original);
54+
55+
$output = file_get_contents($temp);
56+
$this->assertStringContainsString('Aww, look, a cute puppy', $output);
57+
$this->assertStringNotContainsString('INFO', $output);
58+
$this->assertStringNotContainsString('info', $output);
59+
$this->assertStringNotContainsString('testing', $output);
60+
}
61+
3962
/**
4063
* Set an environment variable of the given name and value
4164
* then check the given config key to see if it matches the given result.

0 commit comments

Comments
 (0)