Skip to content

Commit 5632fef

Browse files
committed
Auth: Added specific guards against guest account login
Hardened things to enforce the intent that the guest account should not be used for logins. Currently this would not be allowed due to empty set password, and no password fields on user edit forms, but an error could occur if the login was attempted. This adds: - Handling to show normal invalid user warning on login instead of a hash check error. - Prevention of guest user via main login route, in the event that inventive workarounds would be used by admins to set a password for this account. - Test for guest user login.
1 parent 8ec26e8 commit 5632fef

File tree

3 files changed

+59
-3
lines changed

3 files changed

+59
-3
lines changed

app/Access/LoginService.php

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
use BookStack\Access\Mfa\MfaSession;
66
use BookStack\Activity\ActivityType;
77
use BookStack\Exceptions\LoginAttemptException;
8+
use BookStack\Exceptions\LoginAttemptInvalidUserException;
89
use BookStack\Exceptions\StoppedAuthenticationException;
910
use BookStack\Facades\Activity;
1011
use BookStack\Facades\Theme;
@@ -29,10 +30,14 @@ public function __construct(
2930
* a reason to (MFA or Unconfirmed Email).
3031
* Returns a boolean to indicate the current login result.
3132
*
32-
* @throws StoppedAuthenticationException
33+
* @throws StoppedAuthenticationException|LoginAttemptInvalidUserException
3334
*/
3435
public function login(User $user, string $method, bool $remember = false): void
3536
{
37+
if ($user->isGuest()) {
38+
throw new LoginAttemptInvalidUserException('Login not allowed for guest user');
39+
}
40+
3641
if ($this->awaitingEmailConfirmation($user) || $this->needsMfaVerification($user)) {
3742
$this->setLastLoginAttemptedForUser($user, $method, $remember);
3843

@@ -58,7 +63,7 @@ public function login(User $user, string $method, bool $remember = false): void
5863
*
5964
* @throws Exception
6065
*/
61-
public function reattemptLoginFor(User $user)
66+
public function reattemptLoginFor(User $user): void
6267
{
6368
if ($user->id !== ($this->getLastLoginAttemptUser()->id ?? null)) {
6469
throw new Exception('Login reattempt user does align with current session state');
@@ -152,16 +157,40 @@ public function awaitingEmailConfirmation(User $user): bool
152157
*/
153158
public function attempt(array $credentials, string $method, bool $remember = false): bool
154159
{
160+
if ($this->areCredentialsForGuest($credentials)) {
161+
return false;
162+
}
163+
155164
$result = auth()->attempt($credentials, $remember);
156165
if ($result) {
157166
$user = auth()->user();
158167
auth()->logout();
159-
$this->login($user, $method, $remember);
168+
try {
169+
$this->login($user, $method, $remember);
170+
} catch (LoginAttemptInvalidUserException $e) {
171+
// Catch and return false for non-login accounts
172+
// so it looks like a normal invalid login.
173+
return false;
174+
}
160175
}
161176

162177
return $result;
163178
}
164179

180+
/**
181+
* Check if the given credentials are likely for the system guest account.
182+
*/
183+
protected function areCredentialsForGuest(array $credentials): bool
184+
{
185+
if (isset($credentials['email'])) {
186+
return User::query()->where('email', '=', $credentials['email'])
187+
->where('system_name', '=', 'public')
188+
->exists();
189+
}
190+
191+
return false;
192+
}
193+
165194
/**
166195
* Logs the current user out of the application.
167196
* Returns an app post-redirect path.
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
<?php
2+
3+
namespace BookStack\Exceptions;
4+
5+
class LoginAttemptInvalidUserException extends LoginAttemptException
6+
{
7+
}

tests/Auth/AuthTest.php

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
namespace Tests\Auth;
44

55
use BookStack\Access\Mfa\MfaSession;
6+
use Illuminate\Support\Facades\Hash;
67
use Illuminate\Testing\TestResponse;
78
use Tests\TestCase;
89

@@ -144,6 +145,25 @@ public function test_login_attempts_are_rate_limited()
144145
$resp->assertSee('Too many login attempts. Please try again in');
145146
}
146147

148+
public function test_login_specifically_disabled_for_guest_account()
149+
{
150+
$guest = $this->users->guest();
151+
152+
$resp = $this->post('/login', ['email' => $guest->email, 'password' => 'password']);
153+
$resp->assertRedirect('/login');
154+
$resp = $this->followRedirects($resp);
155+
$resp->assertSee('These credentials do not match our records.');
156+
157+
// Test login even with password somehow set
158+
$guest->password = Hash::make('password');
159+
$guest->save();
160+
161+
$resp = $this->post('/login', ['email' => $guest->email, 'password' => 'password']);
162+
$resp->assertRedirect('/login');
163+
$resp = $this->followRedirects($resp);
164+
$resp->assertSee('These credentials do not match our records.');
165+
}
166+
147167
/**
148168
* Perform a login.
149169
*/

0 commit comments

Comments
 (0)