From 575b85021d4f6d9507816710a5b96f6a9742d6bf Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Sat, 1 Feb 2020 11:42:22 +0000
Subject: [PATCH 1/9] Started alignment of auth services

- Removed LDAP specific logic from login controller, placed in Guard.
- Created safer base user provider for ldap login, to be used for SAML
soon.
- Moved LDAP auth work from user provider to guard.
---
 .../Access/ExternalBaseUserProvider.php}      |  51 +--
 .../Guards/ExternalBaseSessionGuard.php       | 301 ++++++++++++++++++
 app/Auth/Access/Guards/LdapSessionGuard.php   | 126 ++++++++
 app/Auth/Access/LdapService.php               |  11 +-
 app/Config/auth.php                           |  19 +-
 app/Exceptions/AuthException.php              |   6 -
 .../LoginAttemptEmailNeededException.php      |   6 +
 app/Exceptions/LoginAttemptException.php      |   6 +
 app/Http/Controllers/Auth/LoginController.php | 118 +++----
 app/Providers/AuthServiceProvider.php         |  18 +-
 10 files changed, 539 insertions(+), 123 deletions(-)
 rename app/{Providers/LdapUserProvider.php => Auth/Access/ExternalBaseUserProvider.php} (61%)
 create mode 100644 app/Auth/Access/Guards/ExternalBaseSessionGuard.php
 create mode 100644 app/Auth/Access/Guards/LdapSessionGuard.php
 delete mode 100644 app/Exceptions/AuthException.php
 create mode 100644 app/Exceptions/LoginAttemptEmailNeededException.php
 create mode 100644 app/Exceptions/LoginAttemptException.php

diff --git a/app/Providers/LdapUserProvider.php b/app/Auth/Access/ExternalBaseUserProvider.php
similarity index 61%
rename from app/Providers/LdapUserProvider.php
rename to app/Auth/Access/ExternalBaseUserProvider.php
index 9c91def2f..69295ee4e 100644
--- a/app/Providers/LdapUserProvider.php
+++ b/app/Auth/Access/ExternalBaseUserProvider.php
@@ -1,12 +1,11 @@
 <?php
 
-namespace BookStack\Providers;
+namespace BookStack\Auth\Access;
 
-use BookStack\Auth\Access\LdapService;
 use Illuminate\Contracts\Auth\Authenticatable;
 use Illuminate\Contracts\Auth\UserProvider;
 
-class LdapUserProvider implements UserProvider
+class ExternalBaseUserProvider implements UserProvider
 {
 
     /**
@@ -16,21 +15,13 @@ class LdapUserProvider implements UserProvider
      */
     protected $model;
 
-    /**
-     * @var \BookStack\Auth\LdapService
-     */
-    protected $ldapService;
-
-
     /**
      * LdapUserProvider constructor.
      * @param             $model
-     * @param \BookStack\Auth\LdapService $ldapService
      */
-    public function __construct($model, LdapService $ldapService)
+    public function __construct(string $model)
     {
         $this->model = $model;
-        $this->ldapService = $ldapService;
     }
 
     /**
@@ -44,7 +35,6 @@ class LdapUserProvider implements UserProvider
         return new $class;
     }
 
-
     /**
      * Retrieve a user by their unique identifier.
      *
@@ -65,12 +55,7 @@ class LdapUserProvider implements UserProvider
      */
     public function retrieveByToken($identifier, $token)
     {
-        $model = $this->createModel();
-
-        return $model->newQuery()
-            ->where($model->getAuthIdentifierName(), $identifier)
-            ->where($model->getRememberTokenName(), $token)
-            ->first();
+        return null;
     }
 
 
@@ -83,10 +68,7 @@ class LdapUserProvider implements UserProvider
      */
     public function updateRememberToken(Authenticatable $user, $token)
     {
-        if ($user->exists) {
-            $user->setRememberToken($token);
-            $user->save();
-        }
+        //
     }
 
     /**
@@ -97,27 +79,11 @@ class LdapUserProvider implements UserProvider
      */
     public function retrieveByCredentials(array $credentials)
     {
-        // Get user via LDAP
-        $userDetails = $this->ldapService->getUserDetails($credentials['username']);
-        if ($userDetails === null) {
-            return null;
-        }
-
         // Search current user base by looking up a uid
         $model = $this->createModel();
-        $currentUser = $model->newQuery()
-            ->where('external_auth_id', $userDetails['uid'])
+        return $model->newQuery()
+            ->where('external_auth_id', $credentials['external_auth_id'])
             ->first();
-
-        if ($currentUser !== null) {
-            return $currentUser;
-        }
-
-        $model->name = $userDetails['name'];
-        $model->external_auth_id = $userDetails['uid'];
-        $model->email = $userDetails['email'];
-        $model->email_confirmed = false;
-        return $model;
     }
 
     /**
@@ -129,6 +95,7 @@ class LdapUserProvider implements UserProvider
      */
     public function validateCredentials(Authenticatable $user, array $credentials)
     {
-        return $this->ldapService->validateUserCredentials($user, $credentials['username'], $credentials['password']);
+        // Should be done in the guard.
+        return false;
     }
 }
diff --git a/app/Auth/Access/Guards/ExternalBaseSessionGuard.php b/app/Auth/Access/Guards/ExternalBaseSessionGuard.php
new file mode 100644
index 000000000..3022b7f8e
--- /dev/null
+++ b/app/Auth/Access/Guards/ExternalBaseSessionGuard.php
@@ -0,0 +1,301 @@
+<?php
+
+namespace BookStack\Auth\Access\Guards;
+
+use Illuminate\Auth\GuardHelpers;
+use Illuminate\Contracts\Auth\Authenticatable as AuthenticatableContract;
+use Illuminate\Contracts\Auth\StatefulGuard;
+use Illuminate\Contracts\Auth\UserProvider;
+use Illuminate\Contracts\Session\Session;
+
+/**
+ * Class BaseSessionGuard
+ * A base implementation of a session guard. Is a copy of the default Laravel
+ * guard with 'remember' functionality removed. Basic auth and event emission
+ * has also been removed to keep this simple. Designed to be extended by external
+ * Auth Guards.
+ *
+ * @package Illuminate\Auth
+ */
+class ExternalBaseSessionGuard implements StatefulGuard
+{
+    use GuardHelpers;
+
+    /**
+     * The name of the Guard. Typically "session".
+     *
+     * Corresponds to guard name in authentication configuration.
+     *
+     * @var string
+     */
+    protected $name;
+
+    /**
+     * The user we last attempted to retrieve.
+     *
+     * @var \Illuminate\Contracts\Auth\Authenticatable
+     */
+    protected $lastAttempted;
+
+    /**
+     * The session used by the guard.
+     *
+     * @var \Illuminate\Contracts\Session\Session
+     */
+    protected $session;
+
+    /**
+     * Indicates if the logout method has been called.
+     *
+     * @var bool
+     */
+    protected $loggedOut = false;
+
+    /**
+     * Create a new authentication guard.
+     *
+     * @param  string  $name
+     * @param  \Illuminate\Contracts\Auth\UserProvider  $provider
+     * @param  \Illuminate\Contracts\Session\Session  $session
+     * @return void
+     */
+    public function __construct($name,
+        UserProvider $provider,
+        Session $session)
+    {
+        $this->name = $name;
+        $this->session = $session;
+        $this->provider = $provider;
+    }
+
+    /**
+     * Get the currently authenticated user.
+     *
+     * @return \Illuminate\Contracts\Auth\Authenticatable|null
+     */
+    public function user()
+    {
+        if ($this->loggedOut) {
+            return;
+        }
+
+        // If we've already retrieved the user for the current request we can just
+        // return it back immediately. We do not want to fetch the user data on
+        // every call to this method because that would be tremendously slow.
+        if (! is_null($this->user)) {
+            return $this->user;
+        }
+
+        $id = $this->session->get($this->getName());
+
+        // First we will try to load the user using the
+        // identifier in the session if one exists.
+        if (! is_null($id)) {
+            $this->user = $this->provider->retrieveById($id);
+        }
+
+        return $this->user;
+    }
+
+    /**
+     * Get the ID for the currently authenticated user.
+     *
+     * @return int|null
+     */
+    public function id()
+    {
+        if ($this->loggedOut) {
+            return;
+        }
+
+        return $this->user()
+            ? $this->user()->getAuthIdentifier()
+            : $this->session->get($this->getName());
+    }
+
+    /**
+     * Log a user into the application without sessions or cookies.
+     *
+     * @param  array  $credentials
+     * @return bool
+     */
+    public function once(array $credentials = [])
+    {
+        if ($this->validate($credentials)) {
+            $this->setUser($this->lastAttempted);
+
+            return true;
+        }
+
+        return false;
+    }
+
+    /**
+     * Log the given user ID into the application without sessions or cookies.
+     *
+     * @param  mixed  $id
+     * @return \Illuminate\Contracts\Auth\Authenticatable|false
+     */
+    public function onceUsingId($id)
+    {
+        if (! is_null($user = $this->provider->retrieveById($id))) {
+            $this->setUser($user);
+
+            return $user;
+        }
+
+        return false;
+    }
+
+    /**
+     * Validate a user's credentials.
+     *
+     * @param  array  $credentials
+     * @return bool
+     */
+    public function validate(array $credentials = [])
+    {
+        return false;
+    }
+
+
+    /**
+     * Attempt to authenticate a user using the given credentials.
+     *
+     * @param  array  $credentials
+     * @param  bool  $remember
+     * @return bool
+     */
+    public function attempt(array $credentials = [], $remember = false)
+    {
+        return false;
+    }
+
+    /**
+     * Log the given user ID into the application.
+     *
+     * @param  mixed  $id
+     * @param  bool  $remember
+     * @return \Illuminate\Contracts\Auth\Authenticatable|false
+     */
+    public function loginUsingId($id, $remember = false)
+    {
+        if (! is_null($user = $this->provider->retrieveById($id))) {
+            $this->login($user, $remember);
+
+            return $user;
+        }
+
+        return false;
+    }
+
+    /**
+     * Log a user into the application.
+     *
+     * @param  \Illuminate\Contracts\Auth\Authenticatable  $user
+     * @param  bool  $remember
+     * @return void
+     */
+    public function login(AuthenticatableContract $user, $remember = false)
+    {
+        $this->updateSession($user->getAuthIdentifier());
+
+        $this->setUser($user);
+    }
+
+    /**
+     * Update the session with the given ID.
+     *
+     * @param  string  $id
+     * @return void
+     */
+    protected function updateSession($id)
+    {
+        $this->session->put($this->getName(), $id);
+
+        $this->session->migrate(true);
+    }
+
+    /**
+     * Log the user out of the application.
+     *
+     * @return void
+     */
+    public function logout()
+    {
+        $this->clearUserDataFromStorage();
+
+        // Now we will clear the users out of memory so they are no longer available
+        // as the user is no longer considered as being signed into this
+        // application and should not be available here.
+        $this->user = null;
+
+        $this->loggedOut = true;
+    }
+
+    /**
+     * Remove the user data from the session and cookies.
+     *
+     * @return void
+     */
+    protected function clearUserDataFromStorage()
+    {
+        $this->session->remove($this->getName());
+    }
+
+    /**
+     * Get the last user we attempted to authenticate.
+     *
+     * @return \Illuminate\Contracts\Auth\Authenticatable
+     */
+    public function getLastAttempted()
+    {
+        return $this->lastAttempted;
+    }
+
+    /**
+     * Get a unique identifier for the auth session value.
+     *
+     * @return string
+     */
+    public function getName()
+    {
+        return 'login_'.$this->name.'_'.sha1(static::class);
+    }
+
+    /**
+     * Determine if the user was authenticated via "remember me" cookie.
+     *
+     * @return bool
+     */
+    public function viaRemember()
+    {
+        return false;
+    }
+
+    /**
+     * Return the currently cached user.
+     *
+     * @return \Illuminate\Contracts\Auth\Authenticatable|null
+     */
+    public function getUser()
+    {
+        return $this->user;
+    }
+
+    /**
+     * Set the current user.
+     *
+     * @param  \Illuminate\Contracts\Auth\Authenticatable  $user
+     * @return $this
+     */
+    public function setUser(AuthenticatableContract $user)
+    {
+        $this->user = $user;
+
+        $this->loggedOut = false;
+
+        return $this;
+    }
+
+}
diff --git a/app/Auth/Access/Guards/LdapSessionGuard.php b/app/Auth/Access/Guards/LdapSessionGuard.php
new file mode 100644
index 000000000..ad173cf73
--- /dev/null
+++ b/app/Auth/Access/Guards/LdapSessionGuard.php
@@ -0,0 +1,126 @@
+<?php
+
+namespace BookStack\Auth\Access\Guards;
+
+use BookStack\Auth\Access\LdapService;
+use BookStack\Auth\User;
+use BookStack\Auth\UserRepo;
+use BookStack\Exceptions\LdapException;
+use BookStack\Exceptions\LoginAttemptException;
+use BookStack\Exceptions\LoginAttemptEmailNeededException;
+use Illuminate\Contracts\Auth\UserProvider;
+use Illuminate\Contracts\Session\Session;
+
+class LdapSessionGuard extends ExternalBaseSessionGuard
+{
+
+    protected $ldapService;
+    protected $userRepo;
+
+    /**
+     * LdapSessionGuard constructor.
+     */
+    public function __construct($name,
+        UserProvider $provider,
+        Session $session,
+        LdapService $ldapService,
+        UserRepo $userRepo
+    )
+    {
+        $this->ldapService = $ldapService;
+        $this->userRepo = $userRepo;
+        parent::__construct($name, $provider, $session);
+    }
+
+    /**
+     * Validate a user's credentials.
+     *
+     * @param array $credentials
+     * @return bool
+     * @throws LdapException
+     */
+    public function validate(array $credentials = [])
+    {
+        $userDetails = $this->ldapService->getUserDetails($credentials['username']);
+        $this->lastAttempted = $this->provider->retrieveByCredentials([
+            'external_auth_id' => $userDetails['uid']
+        ]);
+
+        return $this->ldapService->validateUserCredentials($userDetails, $credentials['username'], $credentials['password']);
+    }
+
+    /**
+     * Attempt to authenticate a user using the given credentials.
+     *
+     * @param array $credentials
+     * @param bool $remember
+     * @return bool
+     * @throws LoginAttemptEmailNeededException
+     * @throws LoginAttemptException
+     * @throws LdapException
+     */
+    public function attempt(array $credentials = [], $remember = false)
+    {
+        $username = $credentials['username'];
+        $userDetails = $this->ldapService->getUserDetails($username);
+        $this->lastAttempted = $user = $this->provider->retrieveByCredentials([
+            'external_auth_id' => $userDetails['uid']
+        ]);
+
+        if (!$this->ldapService->validateUserCredentials($userDetails, $username, $credentials['password'])) {
+            return false;
+        }
+
+        if (is_null($user)) {
+            $user = $this->freshUserInstanceFromLdapUserDetails($userDetails);
+        }
+
+        $providedEmail = ($credentials['email'] ?? false);
+
+        // Request email if missing from LDAP and model and missing from request
+        if (is_null($user->email) && !$providedEmail) {
+            throw new LoginAttemptEmailNeededException();
+        }
+
+        // Add email to model if non-existing and email provided in request
+        if (!$user->exists && $user->email === null && $providedEmail) {
+            $user->email = $providedEmail;
+        }
+
+        if (!$user->exists) {
+            // Check for existing users with same email
+            $alreadyUser = $user->newQuery()->where('email', '=', $user->email)->count() > 0;
+            if ($alreadyUser) {
+                throw new LoginAttemptException(trans('errors.error_user_exists_different_creds', ['email' => $user->email]));
+            }
+
+            $user->save();
+            $this->userRepo->attachDefaultRole($user);
+            $this->userRepo->downloadAndAssignUserAvatar($user);
+        }
+
+        // Sync LDAP groups if required
+        if ($this->ldapService->shouldSyncGroups()) {
+            $this->ldapService->syncGroups($user, $username);
+        }
+
+        $this->login($user, $remember);
+        return true;
+    }
+
+    /**
+     * Create a fresh user instance from details provided by a LDAP lookup.
+     */
+    protected function freshUserInstanceFromLdapUserDetails(array $ldapUserDetails): User
+    {
+        $user = new User();
+
+        $user->name = $ldapUserDetails['name'];
+        $user->external_auth_id = $ldapUserDetails['uid'];
+        $user->email = $ldapUserDetails['email'];
+        $user->email_confirmed = false;
+
+        return $user;
+    }
+
+}
diff --git a/app/Auth/Access/LdapService.php b/app/Auth/Access/LdapService.php
index 554bc4b48..cc2890817 100644
--- a/app/Auth/Access/LdapService.php
+++ b/app/Auth/Access/LdapService.php
@@ -106,20 +106,15 @@ class LdapService extends ExternalAuthService
      * Check if the given credentials are valid for the given user.
      * @throws LdapException
      */
-    public function validateUserCredentials(Authenticatable $user, string $username, string $password): bool
+    public function validateUserCredentials(array $ldapUserDetails, string $username, string $password): bool
     {
-        $ldapUser = $this->getUserDetails($username);
-        if ($ldapUser === null) {
-            return false;
-        }
-
-        if ($ldapUser['uid'] !== $user->external_auth_id) {
+        if ($ldapUserDetails === null) {
             return false;
         }
 
         $ldapConnection = $this->getConnection();
         try {
-            $ldapBind = $this->ldap->bind($ldapConnection, $ldapUser['dn'], $password);
+            $ldapBind = $this->ldap->bind($ldapConnection, $ldapUserDetails['dn'], $password);
         } catch (ErrorException $e) {
             $ldapBind = false;
         }
diff --git a/app/Config/auth.php b/app/Config/auth.php
index b3e22c7e1..0be5aeee8 100644
--- a/app/Config/auth.php
+++ b/app/Config/auth.php
@@ -18,7 +18,7 @@ return [
     // This option controls the default authentication "guard" and password
     // reset options for your application.
     'defaults' => [
-        'guard' => 'web',
+        'guard' => env('AUTH_METHOD', 'standard') === 'standard' ? 'web' : env('AUTH_METHOD'),
         'passwords' => 'users',
     ],
 
@@ -26,13 +26,16 @@ return [
     // All authentication drivers have a user provider. This defines how the
     // users are actually retrieved out of your database or other storage
     // mechanisms used by this application to persist your user's data.
-    // Supported: "session", "token"
+    // Supported drivers: "session", "api-token", "ldap-session"
     'guards' => [
         'web' => [
             'driver' => 'session',
             'provider' => 'users',
         ],
-
+        'ldap' => [
+            'driver' => 'ldap-session',
+            'provider' => 'external'
+        ],
         'api' => [
             'driver' => 'api-token',
         ],
@@ -42,17 +45,15 @@ return [
     // All authentication drivers have a user provider. This defines how the
     // users are actually retrieved out of your database or other storage
     // mechanisms used by this application to persist your user's data.
-    // Supported: database, eloquent, ldap
     'providers' => [
         'users' => [
             'driver' => env('AUTH_METHOD', 'standard') === 'standard' ? 'eloquent' : env('AUTH_METHOD'),
             'model' => \BookStack\Auth\User::class,
         ],
-
-        // 'users' => [
-        //     'driver' => 'database',
-        //     'table' => 'users',
-        // ],
+        'external' => [
+            'driver' => 'external-users',
+            'model' => \BookStack\Auth\User::class,
+        ],
     ],
 
     // Resetting Passwords
diff --git a/app/Exceptions/AuthException.php b/app/Exceptions/AuthException.php
deleted file mode 100644
index 2ab7d4616..000000000
--- a/app/Exceptions/AuthException.php
+++ /dev/null
@@ -1,6 +0,0 @@
-<?php namespace BookStack\Exceptions;
-
-class AuthException extends PrettyException
-{
-
-}
diff --git a/app/Exceptions/LoginAttemptEmailNeededException.php b/app/Exceptions/LoginAttemptEmailNeededException.php
new file mode 100644
index 000000000..7ffb0c065
--- /dev/null
+++ b/app/Exceptions/LoginAttemptEmailNeededException.php
@@ -0,0 +1,6 @@
+<?php namespace BookStack\Exceptions;
+
+class LoginAttemptEmailNeededException extends LoginAttemptException
+{
+
+}
diff --git a/app/Exceptions/LoginAttemptException.php b/app/Exceptions/LoginAttemptException.php
new file mode 100644
index 000000000..22cc980a0
--- /dev/null
+++ b/app/Exceptions/LoginAttemptException.php
@@ -0,0 +1,6 @@
+<?php namespace BookStack\Exceptions;
+
+class LoginAttemptException extends \Exception
+{
+
+}
diff --git a/app/Http/Controllers/Auth/LoginController.php b/app/Http/Controllers/Auth/LoginController.php
index a74e53617..1ff86fff6 100644
--- a/app/Http/Controllers/Auth/LoginController.php
+++ b/app/Http/Controllers/Auth/LoginController.php
@@ -2,12 +2,10 @@
 
 namespace BookStack\Http\Controllers\Auth;
 
-use BookStack\Auth\Access\LdapService;
 use BookStack\Auth\Access\SocialAuthService;
-use BookStack\Auth\UserRepo;
-use BookStack\Exceptions\AuthException;
+use BookStack\Exceptions\LoginAttemptEmailNeededException;
+use BookStack\Exceptions\LoginAttemptException;
 use BookStack\Http\Controllers\Controller;
-use Illuminate\Contracts\Auth\Authenticatable;
 use Illuminate\Foundation\Auth\AuthenticatesUsers;
 use Illuminate\Http\Request;
 
@@ -27,32 +25,21 @@ class LoginController extends Controller
     use AuthenticatesUsers;
 
     /**
-     * Where to redirect users after login.
-     *
-     * @var string
+     * Redirection paths
      */
     protected $redirectTo = '/';
-
     protected $redirectPath = '/';
     protected $redirectAfterLogout = '/login';
 
     protected $socialAuthService;
-    protected $ldapService;
-    protected $userRepo;
 
     /**
      * Create a new controller instance.
-     *
-     * @param \BookStack\Auth\\BookStack\Auth\Access\SocialAuthService $socialAuthService
-     * @param LdapService $ldapService
-     * @param \BookStack\Auth\UserRepo $userRepo
      */
-    public function __construct(SocialAuthService $socialAuthService, LdapService $ldapService, UserRepo $userRepo)
+    public function __construct(SocialAuthService $socialAuthService)
     {
         $this->middleware('guest', ['only' => ['getLogin', 'postLogin']]);
         $this->socialAuthService = $socialAuthService;
-        $this->ldapService = $ldapService;
-        $this->userRepo = $userRepo;
         $this->redirectPath = url('/');
         $this->redirectAfterLogout = url('/login');
         parent::__construct();
@@ -64,47 +51,11 @@ class LoginController extends Controller
     }
 
     /**
-     * Overrides the action when a user is authenticated.
-     * If the user authenticated but does not exist in the user table we create them.
-     * @throws AuthException
-     * @throws \BookStack\Exceptions\LdapException
+     * Get the needed authorization credentials from the request.
      */
-    protected function authenticated(Request $request, Authenticatable $user)
+    protected function credentials(Request $request)
     {
-        // Explicitly log them out for now if they do no exist.
-        if (!$user->exists) {
-            auth()->logout($user);
-        }
-
-        if (!$user->exists && $user->email === null && !$request->filled('email')) {
-            $request->flash();
-            session()->flash('request-email', true);
-            return redirect('/login');
-        }
-
-        if (!$user->exists && $user->email === null && $request->filled('email')) {
-            $user->email = $request->get('email');
-        }
-
-        if (!$user->exists) {
-            // Check for users with same email already
-            $alreadyUser = $user->newQuery()->where('email', '=', $user->email)->count() > 0;
-            if ($alreadyUser) {
-                throw new AuthException(trans('errors.error_user_exists_different_creds', ['email' => $user->email]));
-            }
-
-            $user->save();
-            $this->userRepo->attachDefaultRole($user);
-            $this->userRepo->downloadAndAssignUserAvatar($user);
-            auth()->login($user);
-        }
-
-        // Sync LDAP groups if required
-        if ($this->ldapService->shouldSyncGroups()) {
-            $this->ldapService->syncGroups($user, $request->get($this->username()));
-        }
-
-        return redirect()->intended('/');
+        return $request->only('username', 'email', 'password');
     }
 
     /**
@@ -130,6 +81,61 @@ class LoginController extends Controller
         ]);
     }
 
+    /**
+     * Handle a login request to the application.
+     *
+     * @param  \Illuminate\Http\Request  $request
+     * @return \Illuminate\Http\RedirectResponse|\Illuminate\Http\Response|\Illuminate\Http\JsonResponse
+     *
+     * @throws \Illuminate\Validation\ValidationException
+     */
+    public function login(Request $request)
+    {
+        $this->validateLogin($request);
+
+        // 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
+        // the IP address of the client making these requests into this application.
+        if (method_exists($this, 'hasTooManyLoginAttempts') &&
+            $this->hasTooManyLoginAttempts($request)) {
+            $this->fireLockoutEvent($request);
+
+            return $this->sendLockoutResponse($request);
+        }
+
+        try {
+            if ($this->attemptLogin($request)) {
+                return $this->sendLoginResponse($request);
+            }
+        } catch (LoginAttemptException $exception) {
+            return $this->sendLoginAttemptExceptionResponse($exception, $request);
+        }
+
+        // If the login attempt was unsuccessful we will increment the number of attempts
+        // to login and redirect the user back to the login form. Of course, when this
+        // user surpasses their maximum number of attempts they will get locked out.
+        $this->incrementLoginAttempts($request);
+
+        return $this->sendFailedLoginResponse($request);
+    }
+
+    /**
+     * Send a response when a login attempt exception occurs.
+     */
+    protected function sendLoginAttemptExceptionResponse(LoginAttemptException $exception, Request $request)
+    {
+        if ($exception instanceof LoginAttemptEmailNeededException) {
+            $request->flash();
+            session()->flash('request-email', true);
+        }
+
+        if ($message = $exception->getMessage()) {
+            $this->showWarningNotification($message);
+        }
+
+        return redirect('/login');
+    }
+
     /**
      * Log the user out of the application.
      */
diff --git a/app/Providers/AuthServiceProvider.php b/app/Providers/AuthServiceProvider.php
index ab7dd5195..0b299551a 100644
--- a/app/Providers/AuthServiceProvider.php
+++ b/app/Providers/AuthServiceProvider.php
@@ -4,7 +4,10 @@ namespace BookStack\Providers;
 
 use Auth;
 use BookStack\Api\ApiTokenGuard;
+use BookStack\Auth\Access\ExternalBaseUserProvider;
+use BookStack\Auth\Access\Guards\LdapSessionGuard;
 use BookStack\Auth\Access\LdapService;
+use BookStack\Auth\UserRepo;
 use Illuminate\Support\ServiceProvider;
 
 class AuthServiceProvider extends ServiceProvider
@@ -19,6 +22,17 @@ class AuthServiceProvider extends ServiceProvider
         Auth::extend('api-token', function ($app, $name, array $config) {
             return new ApiTokenGuard($app['request']);
         });
+
+        Auth::extend('ldap-session', function ($app, $name, array $config) {
+            $provider = Auth::createUserProvider($config['provider']);
+            return new LdapSessionGuard(
+                $name,
+                $provider,
+                $this->app['session.store'],
+                $app[LdapService::class],
+                $app[UserRepo::class]
+            );
+        });
     }
 
     /**
@@ -28,8 +42,8 @@ class AuthServiceProvider extends ServiceProvider
      */
     public function register()
     {
-        Auth::provider('ldap', function ($app, array $config) {
-            return new LdapUserProvider($config['model'], $app[LdapService::class]);
+        Auth::provider('external-users', function ($app, array $config) {
+            return new ExternalBaseUserProvider($config['model']);
         });
     }
 }

From 7728931f150cb9f80a98cf6a2f947d7f25532cc4 Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Sat, 1 Feb 2020 14:30:23 +0000
Subject: [PATCH 2/9] Set more appropriate login validation and broken up LDAP
 guide a bit

---
 app/Auth/Access/Guards/LdapSessionGuard.php   | 65 ++++++++++++-------
 app/Http/Controllers/Auth/LoginController.php | 37 +++++++++++
 2 files changed, 79 insertions(+), 23 deletions(-)

diff --git a/app/Auth/Access/Guards/LdapSessionGuard.php b/app/Auth/Access/Guards/LdapSessionGuard.php
index ad173cf73..223088d05 100644
--- a/app/Auth/Access/Guards/LdapSessionGuard.php
+++ b/app/Auth/Access/Guards/LdapSessionGuard.php
@@ -75,29 +75,8 @@ class LdapSessionGuard extends ExternalBaseSessionGuard
             $user = $this->freshUserInstanceFromLdapUserDetails($userDetails);
         }
 
-        $providedEmail = ($credentials['email'] ?? false);
-
-        // Request email if missing from LDAP and model and missing from request
-        if (is_null($user->email) && !$providedEmail) {
-            throw new LoginAttemptEmailNeededException();
-        }
-
-        // Add email to model if non-existing and email provided in request
-        if (!$user->exists && $user->email === null && $providedEmail) {
-            $user->email = $providedEmail;
-        }
-
-        if (!$user->exists) {
-            // Check for existing users with same email
-            $alreadyUser = $user->newQuery()->where('email', '=', $user->email)->count() > 0;
-            if ($alreadyUser) {
-                throw new LoginAttemptException(trans('errors.error_user_exists_different_creds', ['email' => $user->email]));
-            }
-
-            $user->save();
-            $this->userRepo->attachDefaultRole($user);
-            $this->userRepo->downloadAndAssignUserAvatar($user);
-        }
+        $this->checkForUserEmail($user, $credentials['email'] ?? '');
+        $this->saveIfNew($user);
 
         // Sync LDAP groups if required
         if ($this->ldapService->shouldSyncGroups()) {
@@ -108,6 +87,46 @@ class LdapSessionGuard extends ExternalBaseSessionGuard
         return true;
     }
 
+    /**
+     * Save the give user if they don't yet existing in the system.
+     * @throws LoginAttemptException
+     */
+    protected function saveIfNew(User $user)
+    {
+        if ($user->exists) {
+            return;
+        }
+
+        // Check for existing users with same email
+        $alreadyUser = $user->newQuery()->where('email', '=', $user->email)->count() > 0;
+        if ($alreadyUser) {
+            throw new LoginAttemptException(trans('errors.error_user_exists_different_creds', ['email' => $user->email]));
+        }
+
+        $user->save();
+        $this->userRepo->attachDefaultRole($user);
+        $this->userRepo->downloadAndAssignUserAvatar($user);
+    }
+
+    /**
+     * Ensure the given user has an email.
+     * Takes the provided email in the request if a value is provided
+     * and the user does not have an existing email.
+     * @throws LoginAttemptEmailNeededException
+     */
+    protected function checkForUserEmail(User $user, string $providedEmail)
+    {
+        // Request email if missing from user and missing from request
+        if (is_null($user->email) && !$providedEmail) {
+            throw new LoginAttemptEmailNeededException();
+        }
+
+        // Add email to model if non-existing and email provided in request
+        if (!$user->exists && is_null($user->email) && $providedEmail) {
+            $user->email = $providedEmail;
+        }
+    }
+
     /**
      * Create a fresh user instance from details provided by a LDAP lookup.
      */
diff --git a/app/Http/Controllers/Auth/LoginController.php b/app/Http/Controllers/Auth/LoginController.php
index 1ff86fff6..2302937cb 100644
--- a/app/Http/Controllers/Auth/LoginController.php
+++ b/app/Http/Controllers/Auth/LoginController.php
@@ -119,6 +119,43 @@ class LoginController extends Controller
         return $this->sendFailedLoginResponse($request);
     }
 
+    /**
+     * Validate the user login request.
+     *
+     * @param  \Illuminate\Http\Request  $request
+     * @return void
+     *
+     * @throws \Illuminate\Validation\ValidationException
+     */
+    protected function validateLogin(Request $request)
+    {
+        $rules = [];
+        $authMethod = config('auth.method');
+
+        if ($authMethod === 'standard') {
+            $rules = [
+                'email' => 'required|string|email',
+                'password' => 'required|string'
+            ];
+        }
+
+        if ($authMethod === 'ldap') {
+            $rules = [
+                'username' => 'required|string',
+                'password' => 'required|string',
+                'email' => 'email',
+            ];
+        }
+
+        if ($authMethod === 'saml2') {
+            $rules = [
+                'email' => 'email',
+            ];
+        }
+
+        $request->validate($rules);
+    }
+
     /**
      * Send a response when a login attempt exception occurs.
      */

From 3470a6a140e7e87cbb53332d9a8b50e5693603ef Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Sat, 1 Feb 2020 16:11:56 +0000
Subject: [PATCH 3/9] Aligned SAML2 system with LDAP implementation in terms of
 guards and UI

---
 app/Auth/Access/Guards/Saml2SessionGuard.php  | 103 ++++++++++++++++++
 .../views/auth/forms/login/saml2.blade.php    |  30 +++++
 2 files changed, 133 insertions(+)
 create mode 100644 app/Auth/Access/Guards/Saml2SessionGuard.php
 create mode 100644 resources/views/auth/forms/login/saml2.blade.php

diff --git a/app/Auth/Access/Guards/Saml2SessionGuard.php b/app/Auth/Access/Guards/Saml2SessionGuard.php
new file mode 100644
index 000000000..1bdb59d51
--- /dev/null
+++ b/app/Auth/Access/Guards/Saml2SessionGuard.php
@@ -0,0 +1,103 @@
+<?php
+
+namespace BookStack\Auth\Access\Guards;
+
+use BookStack\Auth\Access\LdapService;
+use BookStack\Auth\User;
+use BookStack\Auth\UserRepo;
+use BookStack\Exceptions\LdapException;
+use BookStack\Exceptions\LoginAttemptException;
+use BookStack\Exceptions\LoginAttemptEmailNeededException;
+use Illuminate\Contracts\Auth\UserProvider;
+use Illuminate\Contracts\Session\Session;
+
+class LdapSessionGuard extends ExternalBaseSessionGuard
+{
+
+    protected $ldapService;
+
+    /**
+     * LdapSessionGuard constructor.
+     */
+    public function __construct($name,
+        UserProvider $provider,
+        Session $session,
+        LdapService $ldapService,
+        UserRepo $userRepo
+    )
+    {
+        $this->ldapService = $ldapService;
+        parent::__construct($name, $provider, $session, $userRepo);
+    }
+
+    /**
+     * Validate a user's credentials.
+     *
+     * @param array $credentials
+     * @return bool
+     * @throws LdapException
+     */
+    public function validate(array $credentials = [])
+    {
+        $userDetails = $this->ldapService->getUserDetails($credentials['username']);
+        $this->lastAttempted = $this->provider->retrieveByCredentials([
+            'external_auth_id' => $userDetails['uid']
+        ]);
+
+        return $this->ldapService->validateUserCredentials($userDetails, $credentials['username'], $credentials['password']);
+    }
+
+    /**
+     * Attempt to authenticate a user using the given credentials.
+     *
+     * @param array $credentials
+     * @param bool $remember
+     * @return bool
+     * @throws LoginAttemptEmailNeededException
+     * @throws LoginAttemptException
+     * @throws LdapException
+     */
+    public function attempt(array $credentials = [], $remember = false)
+    {
+        $username = $credentials['username'];
+        $userDetails = $this->ldapService->getUserDetails($username);
+        $this->lastAttempted = $user = $this->provider->retrieveByCredentials([
+            'external_auth_id' => $userDetails['uid']
+        ]);
+
+        if (!$this->ldapService->validateUserCredentials($userDetails, $username, $credentials['password'])) {
+            return false;
+        }
+
+        if (is_null($user)) {
+            $user = $this->freshUserInstanceFromLdapUserDetails($userDetails);
+        }
+
+        $this->checkForUserEmail($user, $credentials['email'] ?? '');
+        $this->saveIfNew($user);
+
+        // Sync LDAP groups if required
+        if ($this->ldapService->shouldSyncGroups()) {
+            $this->ldapService->syncGroups($user, $username);
+        }
+
+        $this->login($user, $remember);
+        return true;
+    }
+
+    /**
+     * Create a fresh user instance from details provided by a LDAP lookup.
+     */
+    protected function freshUserInstanceFromLdapUserDetails(array $ldapUserDetails): User
+    {
+        $user = new User();
+
+        $user->name = $ldapUserDetails['name'];
+        $user->external_auth_id = $ldapUserDetails['uid'];
+        $user->email = $ldapUserDetails['email'];
+        $user->email_confirmed = false;
+
+        return $user;
+    }
+
+}
diff --git a/resources/views/auth/forms/login/saml2.blade.php b/resources/views/auth/forms/login/saml2.blade.php
new file mode 100644
index 000000000..12592d492
--- /dev/null
+++ b/resources/views/auth/forms/login/saml2.blade.php
@@ -0,0 +1,30 @@
+<form action="{{ url('/login') }}" method="POST" id="login-form" class="mt-l">
+    {!! csrf_field() !!}
+
+    <div class="stretch-inputs">
+        <div class="form-group">
+            <label for="username">{{ trans('auth.username') }}</label>
+            @include('form.text', ['name' => 'username', 'autofocus' => true])
+        </div>
+
+        @if(session('request-email', false) === true)
+            <div class="form-group">
+                <label for="email">{{ trans('auth.email') }}</label>
+                @include('form.text', ['name' => 'email'])
+                <span class="text-neg">{{ trans('auth.ldap_email_hint') }}</span>
+            </div>
+        @endif
+
+        <div class="form-group">
+            <label for="password">{{ trans('auth.password') }}</label>
+            @include('form.password', ['name' => 'password'])
+        </div>
+    </div>
+
+    <div class="grid half collapse-xs gap-xl v-center">
+        <div class="text-right">
+            <button class="button">{{ Str::title(trans('auth.log_in')) }}</button>
+        </div>
+    </div>
+
+</form>
\ No newline at end of file

From e743cd3f606fb8a2e432813f7c84fed1093f68c4 Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Sun, 2 Feb 2020 10:59:03 +0000
Subject: [PATCH 4/9] Added files missed in previous commit

---
 .env.example.complete                         |  4 +-
 .../Guards/ExternalBaseSessionGuard.php       | 19 ++--
 app/Auth/Access/Guards/LdapSessionGuard.php   |  4 +-
 app/Auth/Access/Guards/Saml2SessionGuard.php  | 89 +++----------------
 app/Auth/Access/RegistrationService.php       |  5 +-
 app/Auth/Access/Saml2Service.php              | 16 ++--
 app/Config/auth.php                           |  6 +-
 app/Config/saml2.php                          |  4 -
 app/Http/Controllers/Auth/LoginController.php | 26 +-----
 .../Controllers/Auth/RegisterController.php   |  2 -
 app/Http/Controllers/Auth/Saml2Controller.php |  3 +-
 app/Providers/AuthServiceProvider.php         | 11 +++
 .../views/auth/forms/login/ldap.blade.php     | 43 +++++----
 .../views/auth/forms/login/saml2.blade.php    | 31 ++-----
 .../views/auth/forms/login/standard.blade.php | 46 +++++++---
 resources/views/auth/login.blade.php          | 36 +-------
 resources/views/auth/register.blade.php       |  9 --
 resources/views/common/header.blade.php       |  8 +-
 resources/views/settings/roles/form.blade.php |  2 +-
 resources/views/users/form.blade.php          |  2 +-
 routes/web.php                                |  2 +-
 21 files changed, 139 insertions(+), 229 deletions(-)

diff --git a/.env.example.complete b/.env.example.complete
index e44644f08..bc6b644aa 100644
--- a/.env.example.complete
+++ b/.env.example.complete
@@ -121,7 +121,7 @@ STORAGE_S3_ENDPOINT=https://my-custom-s3-compatible.service.com:8001
 STORAGE_URL=false
 
 # Authentication method to use
-# Can be 'standard' or 'ldap'
+# Can be 'standard', 'ldap' or 'saml2'
 AUTH_METHOD=standard
 
 # Social authentication configuration
@@ -205,8 +205,6 @@ LDAP_REMOVE_FROM_GROUPS=false
 # SAML authentication configuration
 # Refer to https://www.bookstackapp.com/docs/admin/saml2-auth/
 SAML2_NAME=SSO
-SAML2_ENABLED=false
-SAML2_AUTO_REGISTER=true
 SAML2_EMAIL_ATTRIBUTE=email
 SAML2_DISPLAY_NAME_ATTRIBUTES=username
 SAML2_EXTERNAL_ID_ATTRIBUTE=null
diff --git a/app/Auth/Access/Guards/ExternalBaseSessionGuard.php b/app/Auth/Access/Guards/ExternalBaseSessionGuard.php
index 3022b7f8e..d1fb0b606 100644
--- a/app/Auth/Access/Guards/ExternalBaseSessionGuard.php
+++ b/app/Auth/Access/Guards/ExternalBaseSessionGuard.php
@@ -2,6 +2,10 @@
 
 namespace BookStack\Auth\Access\Guards;
 
+use BookStack\Auth\User;
+use BookStack\Auth\UserRepo;
+use BookStack\Exceptions\LoginAttemptEmailNeededException;
+use BookStack\Exceptions\LoginAttemptException;
 use Illuminate\Auth\GuardHelpers;
 use Illuminate\Contracts\Auth\Authenticatable as AuthenticatableContract;
 use Illuminate\Contracts\Auth\StatefulGuard;
@@ -51,21 +55,24 @@ class ExternalBaseSessionGuard implements StatefulGuard
      */
     protected $loggedOut = false;
 
+    /**
+     * Repository to perform user-specific actions.
+     *
+     * @var UserRepo
+     */
+    protected $userRepo;
+
     /**
      * Create a new authentication guard.
      *
-     * @param  string  $name
-     * @param  \Illuminate\Contracts\Auth\UserProvider  $provider
-     * @param  \Illuminate\Contracts\Session\Session  $session
      * @return void
      */
-    public function __construct($name,
-        UserProvider $provider,
-        Session $session)
+    public function __construct(string $name, UserProvider $provider, Session $session, UserRepo $userRepo)
     {
         $this->name = $name;
         $this->session = $session;
         $this->provider = $provider;
+        $this->userRepo = $userRepo;
     }
 
     /**
diff --git a/app/Auth/Access/Guards/LdapSessionGuard.php b/app/Auth/Access/Guards/LdapSessionGuard.php
index 223088d05..6c416bf30 100644
--- a/app/Auth/Access/Guards/LdapSessionGuard.php
+++ b/app/Auth/Access/Guards/LdapSessionGuard.php
@@ -15,7 +15,6 @@ class LdapSessionGuard extends ExternalBaseSessionGuard
 {
 
     protected $ldapService;
-    protected $userRepo;
 
     /**
      * LdapSessionGuard constructor.
@@ -28,8 +27,7 @@ class LdapSessionGuard extends ExternalBaseSessionGuard
     )
     {
         $this->ldapService = $ldapService;
-        $this->userRepo = $userRepo;
-        parent::__construct($name, $provider, $session);
+        parent::__construct($name, $provider, $session, $userRepo);
     }
 
     /**
diff --git a/app/Auth/Access/Guards/Saml2SessionGuard.php b/app/Auth/Access/Guards/Saml2SessionGuard.php
index 1bdb59d51..4023913ed 100644
--- a/app/Auth/Access/Guards/Saml2SessionGuard.php
+++ b/app/Auth/Access/Guards/Saml2SessionGuard.php
@@ -2,49 +2,27 @@
 
 namespace BookStack\Auth\Access\Guards;
 
-use BookStack\Auth\Access\LdapService;
-use BookStack\Auth\User;
-use BookStack\Auth\UserRepo;
-use BookStack\Exceptions\LdapException;
-use BookStack\Exceptions\LoginAttemptException;
-use BookStack\Exceptions\LoginAttemptEmailNeededException;
-use Illuminate\Contracts\Auth\UserProvider;
-use Illuminate\Contracts\Session\Session;
-
-class LdapSessionGuard extends ExternalBaseSessionGuard
+/**
+ * Saml2 Session Guard
+ *
+ * The saml2 login process is async in nature meaning it does not fit very well
+ * into the default laravel 'Guard' auth flow. Instead most of the logic is done
+ * via the Saml2 controller & Saml2Service. This class provides a safer, thin
+ * version of SessionGuard.
+ *
+ * @package BookStack\Auth\Access\Guards
+ */
+class Saml2SessionGuard extends ExternalBaseSessionGuard
 {
-
-    protected $ldapService;
-
-    /**
-     * LdapSessionGuard constructor.
-     */
-    public function __construct($name,
-        UserProvider $provider,
-        Session $session,
-        LdapService $ldapService,
-        UserRepo $userRepo
-    )
-    {
-        $this->ldapService = $ldapService;
-        parent::__construct($name, $provider, $session, $userRepo);
-    }
-
     /**
      * Validate a user's credentials.
      *
      * @param array $credentials
      * @return bool
-     * @throws LdapException
      */
     public function validate(array $credentials = [])
     {
-        $userDetails = $this->ldapService->getUserDetails($credentials['username']);
-        $this->lastAttempted = $this->provider->retrieveByCredentials([
-            'external_auth_id' => $userDetails['uid']
-        ]);
-
-        return $this->ldapService->validateUserCredentials($userDetails, $credentials['username'], $credentials['password']);
+        return false;
     }
 
     /**
@@ -53,51 +31,10 @@ class LdapSessionGuard extends ExternalBaseSessionGuard
      * @param array $credentials
      * @param bool $remember
      * @return bool
-     * @throws LoginAttemptEmailNeededException
-     * @throws LoginAttemptException
-     * @throws LdapException
      */
     public function attempt(array $credentials = [], $remember = false)
     {
-        $username = $credentials['username'];
-        $userDetails = $this->ldapService->getUserDetails($username);
-        $this->lastAttempted = $user = $this->provider->retrieveByCredentials([
-            'external_auth_id' => $userDetails['uid']
-        ]);
-
-        if (!$this->ldapService->validateUserCredentials($userDetails, $username, $credentials['password'])) {
-            return false;
-        }
-
-        if (is_null($user)) {
-            $user = $this->freshUserInstanceFromLdapUserDetails($userDetails);
-        }
-
-        $this->checkForUserEmail($user, $credentials['email'] ?? '');
-        $this->saveIfNew($user);
-
-        // Sync LDAP groups if required
-        if ($this->ldapService->shouldSyncGroups()) {
-            $this->ldapService->syncGroups($user, $username);
-        }
-
-        $this->login($user, $remember);
-        return true;
-    }
-
-    /**
-     * Create a fresh user instance from details provided by a LDAP lookup.
-     */
-    protected function freshUserInstanceFromLdapUserDetails(array $ldapUserDetails): User
-    {
-        $user = new User();
-
-        $user->name = $ldapUserDetails['name'];
-        $user->external_auth_id = $ldapUserDetails['uid'];
-        $user->email = $ldapUserDetails['email'];
-        $user->email_confirmed = false;
-
-        return $user;
+        return false;
     }
 
 }
diff --git a/app/Auth/Access/RegistrationService.php b/app/Auth/Access/RegistrationService.php
index 7e8c7d5f5..74142301a 100644
--- a/app/Auth/Access/RegistrationService.php
+++ b/app/Auth/Access/RegistrationService.php
@@ -20,14 +20,15 @@ class RegistrationService
         $this->emailConfirmationService = $emailConfirmationService;
     }
 
-
     /**
      * Check whether or not registrations are allowed in the app settings.
      * @throws UserRegistrationException
      */
     public function checkRegistrationAllowed()
     {
-        if (!setting('registration-enabled') || config('auth.method') === 'ldap') {
+        $authMethod = config('auth.method');
+        $authMethodsWithRegistration = ['standard'];
+        if (!setting('registration-enabled') || !in_array($authMethod, $authMethodsWithRegistration)) {
             throw new UserRegistrationException(trans('auth.registrations_disabled'), '/login');
         }
     }
diff --git a/app/Auth/Access/Saml2Service.php b/app/Auth/Access/Saml2Service.php
index c1038e730..c52dc3a39 100644
--- a/app/Auth/Access/Saml2Service.php
+++ b/app/Auth/Access/Saml2Service.php
@@ -20,7 +20,6 @@ class Saml2Service extends ExternalAuthService
     protected $config;
     protected $userRepo;
     protected $user;
-    protected $enabled;
 
     /**
      * Saml2Service constructor.
@@ -30,7 +29,6 @@ class Saml2Service extends ExternalAuthService
         $this->config = config('saml2');
         $this->userRepo = $userRepo;
         $this->user = $user;
-        $this->enabled = config('saml2.enabled') === true;
     }
 
     /**
@@ -204,7 +202,7 @@ class Saml2Service extends ExternalAuthService
      */
     protected function shouldSyncGroups(): bool
     {
-        return $this->enabled && $this->config['user_to_groups'] !== false;
+        return $this->config['user_to_groups'] !== false;
     }
 
     /**
@@ -248,7 +246,7 @@ class Saml2Service extends ExternalAuthService
     /**
      * Extract the details of a user from a SAML response.
      */
-    public function getUserDetails(string $samlID, $samlAttributes): array
+    protected function getUserDetails(string $samlID, $samlAttributes): array
     {
         $emailAttr = $this->config['email_attribute'];
         $externalId = $this->getExternalId($samlAttributes, $samlID);
@@ -329,7 +327,7 @@ class Saml2Service extends ExternalAuthService
             throw new SamlException(trans('errors.saml_email_exists', ['email' => $userDetails['email']]));
         }
 
-        $user = $this->user->forceCreate($userData);
+        $user = $this->user->newQuery()->forceCreate($userData);
         $this->userRepo->attachDefaultRole($user);
         $this->userRepo->downloadAndAssignUserAvatar($user);
         return $user;
@@ -337,15 +335,15 @@ class Saml2Service extends ExternalAuthService
 
     /**
      * Get the user from the database for the specified details.
+     * @throws SamlException
      */
     protected function getOrRegisterUser(array $userDetails): ?User
     {
-        $isRegisterEnabled = $this->config['auto_register'] === true;
-        $user = $this->user
-          ->where('external_auth_id', $userDetails['external_id'])
+        $user = $this->user->newQuery()
+          ->where('external_auth_id', '=', $userDetails['external_id'])
           ->first();
 
-        if ($user === null && $isRegisterEnabled) {
+        if (is_null($user)) {
             $user = $this->registerUser($userDetails);
         }
 
diff --git a/app/Config/auth.php b/app/Config/auth.php
index 0be5aeee8..2afb10ec2 100644
--- a/app/Config/auth.php
+++ b/app/Config/auth.php
@@ -34,7 +34,11 @@ return [
         ],
         'ldap' => [
             'driver' => 'ldap-session',
-            'provider' => 'external'
+            'provider' => 'external',
+        ],
+        'saml2' => [
+            'driver' => 'saml2-session',
+            'provider' => 'external',
         ],
         'api' => [
             'driver' => 'api-token',
diff --git a/app/Config/saml2.php b/app/Config/saml2.php
index 2f2ad14f1..5f2c1395b 100644
--- a/app/Config/saml2.php
+++ b/app/Config/saml2.php
@@ -4,10 +4,6 @@ return [
 
     // Display name, shown to users, for SAML2 option
     'name' => env('SAML2_NAME', 'SSO'),
-    // Toggle whether the SAML2 option is active
-    'enabled' => env('SAML2_ENABLED', false),
-    // Enable registration via SAML2 authentication
-    'auto_register' => env('SAML2_AUTO_REGISTER', true),
 
     // Dump user details after a login request for debugging purposes
     'dump_user_details' => env('SAML2_DUMP_USER_DETAILS', false),
diff --git a/app/Http/Controllers/Auth/LoginController.php b/app/Http/Controllers/Auth/LoginController.php
index 2302937cb..8116288ad 100644
--- a/app/Http/Controllers/Auth/LoginController.php
+++ b/app/Http/Controllers/Auth/LoginController.php
@@ -65,7 +65,6 @@ class LoginController extends Controller
     {
         $socialDrivers = $this->socialAuthService->getActiveDrivers();
         $authMethod = config('auth.method');
-        $samlEnabled = config('saml2.enabled') === true;
 
         if ($request->has('email')) {
             session()->flashInput([
@@ -77,7 +76,6 @@ class LoginController extends Controller
         return view('auth.login', [
           'socialDrivers' => $socialDrivers,
           'authMethod' => $authMethod,
-          'samlEnabled' => $samlEnabled,
         ]);
     }
 
@@ -129,28 +127,16 @@ class LoginController extends Controller
      */
     protected function validateLogin(Request $request)
     {
-        $rules = [];
+        $rules = ['password' => 'required|string'];
         $authMethod = config('auth.method');
 
         if ($authMethod === 'standard') {
-            $rules = [
-                'email' => 'required|string|email',
-                'password' => 'required|string'
-            ];
+            $rules['email'] = 'required|email';
         }
 
         if ($authMethod === 'ldap') {
-            $rules = [
-                'username' => 'required|string',
-                'password' => 'required|string',
-                'email' => 'email',
-            ];
-        }
-
-        if ($authMethod === 'saml2') {
-            $rules = [
-                'email' => 'email',
-            ];
+            $rules['username'] = 'required|string';
+            $rules['email'] = 'email';
         }
 
         $request->validate($rules);
@@ -178,10 +164,6 @@ class LoginController extends Controller
      */
     public function logout(Request $request)
     {
-        if (config('saml2.enabled') && session()->get('last_login_type') === 'saml2') {
-            return redirect('/saml2/logout');
-        }
-
         $this->guard()->logout();
         $request->session()->invalidate();
 
diff --git a/app/Http/Controllers/Auth/RegisterController.php b/app/Http/Controllers/Auth/RegisterController.php
index c9c0c3ec5..8fb13d536 100644
--- a/app/Http/Controllers/Auth/RegisterController.php
+++ b/app/Http/Controllers/Auth/RegisterController.php
@@ -75,10 +75,8 @@ class RegisterController extends Controller
     {
         $this->registrationService->checkRegistrationAllowed();
         $socialDrivers = $this->socialAuthService->getActiveDrivers();
-        $samlEnabled = (config('saml2.enabled') === true) && (config('saml2.auto_register') === true);
         return view('auth.register', [
             'socialDrivers' => $socialDrivers,
-            'samlEnabled' => $samlEnabled,
         ]);
     }
 
diff --git a/app/Http/Controllers/Auth/Saml2Controller.php b/app/Http/Controllers/Auth/Saml2Controller.php
index 863894128..72cf0e019 100644
--- a/app/Http/Controllers/Auth/Saml2Controller.php
+++ b/app/Http/Controllers/Auth/Saml2Controller.php
@@ -20,7 +20,8 @@ class Saml2Controller extends Controller
 
         // SAML2 access middleware
         $this->middleware(function ($request, $next) {
-            if (!config('saml2.enabled')) {
+
+            if (config('auth.method') !== 'saml2') {
                 $this->showPermissionError();
             }
 
diff --git a/app/Providers/AuthServiceProvider.php b/app/Providers/AuthServiceProvider.php
index 0b299551a..a885628f3 100644
--- a/app/Providers/AuthServiceProvider.php
+++ b/app/Providers/AuthServiceProvider.php
@@ -6,6 +6,7 @@ use Auth;
 use BookStack\Api\ApiTokenGuard;
 use BookStack\Auth\Access\ExternalBaseUserProvider;
 use BookStack\Auth\Access\Guards\LdapSessionGuard;
+use BookStack\Auth\Access\Guards\Saml2SessionGuard;
 use BookStack\Auth\Access\LdapService;
 use BookStack\Auth\UserRepo;
 use Illuminate\Support\ServiceProvider;
@@ -33,6 +34,16 @@ class AuthServiceProvider extends ServiceProvider
                 $app[UserRepo::class]
             );
         });
+
+        Auth::extend('saml2-session', function ($app, $name, array $config) {
+            $provider = Auth::createUserProvider($config['provider']);
+            return new Saml2SessionGuard(
+                $name,
+                $provider,
+                $this->app['session.store'],
+                $app[UserRepo::class]
+            );
+        });
     }
 
     /**
diff --git a/resources/views/auth/forms/login/ldap.blade.php b/resources/views/auth/forms/login/ldap.blade.php
index 2699fda00..12592d492 100644
--- a/resources/views/auth/forms/login/ldap.blade.php
+++ b/resources/views/auth/forms/login/ldap.blade.php
@@ -1,19 +1,30 @@
-<div class="form-group">
-    <label for="username">{{ trans('auth.username') }}</label>
-    @include('form.text', ['name' => 'username', 'autofocus' => true])
-</div>
+<form action="{{ url('/login') }}" method="POST" id="login-form" class="mt-l">
+    {!! csrf_field() !!}
 
-@if(session('request-email', false) === true)
-    <div class="form-group">
-        <label for="email">{{ trans('auth.email') }}</label>
-        @include('form.text', ['name' => 'email'])
-        <span class="text-neg">
-            {{ trans('auth.ldap_email_hint') }}
-        </span>
+    <div class="stretch-inputs">
+        <div class="form-group">
+            <label for="username">{{ trans('auth.username') }}</label>
+            @include('form.text', ['name' => 'username', 'autofocus' => true])
+        </div>
+
+        @if(session('request-email', false) === true)
+            <div class="form-group">
+                <label for="email">{{ trans('auth.email') }}</label>
+                @include('form.text', ['name' => 'email'])
+                <span class="text-neg">{{ trans('auth.ldap_email_hint') }}</span>
+            </div>
+        @endif
+
+        <div class="form-group">
+            <label for="password">{{ trans('auth.password') }}</label>
+            @include('form.password', ['name' => 'password'])
+        </div>
     </div>
-@endif
 
-<div class="form-group">
-    <label for="password">{{ trans('auth.password') }}</label>
-    @include('form.password', ['name' => 'password'])
-</div>
\ No newline at end of file
+    <div class="grid half collapse-xs gap-xl v-center">
+        <div class="text-right">
+            <button class="button">{{ Str::title(trans('auth.log_in')) }}</button>
+        </div>
+    </div>
+
+</form>
\ No newline at end of file
diff --git a/resources/views/auth/forms/login/saml2.blade.php b/resources/views/auth/forms/login/saml2.blade.php
index 12592d492..aa1913e31 100644
--- a/resources/views/auth/forms/login/saml2.blade.php
+++ b/resources/views/auth/forms/login/saml2.blade.php
@@ -1,30 +1,11 @@
-<form action="{{ url('/login') }}" method="POST" id="login-form" class="mt-l">
+<form action="{{ url('/saml2/login') }}" method="POST" id="login-form" class="mt-l">
     {!! csrf_field() !!}
 
-    <div class="stretch-inputs">
-        <div class="form-group">
-            <label for="username">{{ trans('auth.username') }}</label>
-            @include('form.text', ['name' => 'username', 'autofocus' => true])
-        </div>
-
-        @if(session('request-email', false) === true)
-            <div class="form-group">
-                <label for="email">{{ trans('auth.email') }}</label>
-                @include('form.text', ['name' => 'email'])
-                <span class="text-neg">{{ trans('auth.ldap_email_hint') }}</span>
-            </div>
-        @endif
-
-        <div class="form-group">
-            <label for="password">{{ trans('auth.password') }}</label>
-            @include('form.password', ['name' => 'password'])
-        </div>
-    </div>
-
-    <div class="grid half collapse-xs gap-xl v-center">
-        <div class="text-right">
-            <button class="button">{{ Str::title(trans('auth.log_in')) }}</button>
-        </div>
+    <div>
+        <button id="saml-login" class="button outline block svg">
+            @icon('saml2')
+            {{ trans('auth.log_in_with', ['socialDriver' => config('saml2.name')]) }}
+        </button>
     </div>
 
 </form>
\ No newline at end of file
diff --git a/resources/views/auth/forms/login/standard.blade.php b/resources/views/auth/forms/login/standard.blade.php
index 52fae3750..87603e2cb 100644
--- a/resources/views/auth/forms/login/standard.blade.php
+++ b/resources/views/auth/forms/login/standard.blade.php
@@ -1,12 +1,36 @@
-<div class="form-group">
-    <label for="email">{{ trans('auth.email') }}</label>
-    @include('form.text', ['name' => 'email', 'autofocus' => true])
-</div>
+<form action="{{ url('/login') }}" method="POST" id="login-form" class="mt-l">
+    {!! csrf_field() !!}
+
+    <div class="stretch-inputs">
+        <div class="form-group">
+            <label for="email">{{ trans('auth.email') }}</label>
+            @include('form.text', ['name' => 'email', 'autofocus' => true])
+        </div>
+
+        <div class="form-group">
+            <label for="password">{{ trans('auth.password') }}</label>
+            @include('form.password', ['name' => 'password'])
+            <div class="small mt-s">
+                <a href="{{ url('/password/email') }}">{{ trans('auth.forgot_password') }}</a>
+            </div>
+        </div>
+    </div>
+
+    <div class="grid half collapse-xs gap-xl v-center">
+        <div class="text-left ml-xxs">
+            @include('components.custom-checkbox', [
+                'name' => 'remember',
+                'checked' => false,
+                'value' => 'on',
+                'label' => trans('auth.remember_me'),
+            ])
+        </div>
+
+        <div class="text-right">
+            <button class="button">{{ Str::title(trans('auth.log_in')) }}</button>
+        </div>
+    </div>
+
+</form>
+
 
-<div class="form-group">
-    <label for="password">{{ trans('auth.password') }}</label>
-    @include('form.password', ['name' => 'password'])
-    <span class="block small mt-s">
-        <a href="{{ url('/password/email') }}">{{ trans('auth.forgot_password') }}</a>
-    </span>
-</div>
diff --git a/resources/views/auth/login.blade.php b/resources/views/auth/login.blade.php
index 098ce2100..0a21a0f62 100644
--- a/resources/views/auth/login.blade.php
+++ b/resources/views/auth/login.blade.php
@@ -9,29 +9,7 @@
         <div class="card content-wrap auto-height">
             <h1 class="list-heading">{{ Str::title(trans('auth.log_in')) }}</h1>
 
-            <form action="{{ url('/login') }}" method="POST" id="login-form" class="mt-l">
-                {!! csrf_field() !!}
-
-                <div class="stretch-inputs">
-                    @include('auth.forms.login.' . $authMethod)
-                </div>
-
-                <div class="grid half collapse-xs gap-xl v-center">
-                    <div class="text-left ml-xxs">
-                        @include('components.custom-checkbox', [
-                            'name' => 'remember',
-                            'checked' => false,
-                            'value' => 'on',
-                            'label' => trans('auth.remember_me'),
-                        ])
-                    </div>
-
-                    <div class="text-right">
-                        <button class="button">{{ Str::title(trans('auth.log_in')) }}</button>
-                    </div>
-                </div>
-
-            </form>
+            @include('auth.forms.login.' . $authMethod)
 
             @if(count($socialDrivers) > 0)
                 <hr class="my-l">
@@ -45,17 +23,7 @@
                 @endforeach
             @endif
 
-            @if($samlEnabled)
-                <hr class="my-l">
-                <div>
-                    <a id="saml-login" class="button outline block svg" href="{{ url("/saml2/login") }}">
-                       @icon('saml2')
-                      {{ trans('auth.log_in_with', ['socialDriver' => config('saml2.name')]) }}
-                    </a>
-                </div>
-            @endif
-
-            @if(setting('registration-enabled') && config('auth.method') !== 'ldap')
+            @if(setting('registration-enabled') && config('auth.method') === 'standard')
                 <div class="text-center pb-s">
                     <hr class="my-l">
                     <a href="{{ url('/register') }}">{{ trans('auth.dont_have_account') }}</a>
diff --git a/resources/views/auth/register.blade.php b/resources/views/auth/register.blade.php
index 8dd6592c1..1625ebc4c 100644
--- a/resources/views/auth/register.blade.php
+++ b/resources/views/auth/register.blade.php
@@ -50,15 +50,6 @@
                 @endforeach
             @endif
 
-            @if($samlEnabled)
-                <hr class="my-l">
-                <div>
-                    <a id="saml-login" class="button outline block svg" href="{{ url("/saml2/login") }}">
-                        @icon('saml2')
-                        {{ trans('auth.log_in_with', ['socialDriver' => config('saml2.name')]) }}
-                    </a>
-                </div>
-            @endif
         </div>
     </div>
 @stop
diff --git a/resources/views/common/header.blade.php b/resources/views/common/header.blade.php
index b06036031..7c837ec29 100644
--- a/resources/views/common/header.blade.php
+++ b/resources/views/common/header.blade.php
@@ -42,7 +42,7 @@
                     @endif
 
                     @if(!signedInUser())
-                        @if(setting('registration-enabled') && config('auth.method') !== 'ldap')
+                        @if(setting('registration-enabled') && config('auth.method') === 'standard')
                             <a href="{{ url('/register') }}">@icon('new-user') {{ trans('auth.sign_up') }}</a>
                         @endif
                         <a href="{{ url('/login') }}">@icon('login') {{ trans('auth.log_in') }}</a>
@@ -64,7 +64,11 @@
                                 <a href="{{ url("/settings/users/{$currentUser->id}") }}">@icon('edit'){{ trans('common.edit_profile') }}</a>
                             </li>
                             <li>
-                                <a href="{{ url('/logout') }}">@icon('logout'){{ trans('auth.logout') }}</a>
+                                @if(config('auth.method') === 'saml2')
+                                    <a href="{{ url('/saml2/logout') }}">@icon('logout'){{ trans('auth.logout') }}</a>
+                                @else
+                                    <a href="{{ url('/logout') }}">@icon('logout'){{ trans('auth.logout') }}</a>
+                                @endif
                             </li>
                         </ul>
                     </div>
diff --git a/resources/views/settings/roles/form.blade.php b/resources/views/settings/roles/form.blade.php
index 1fbc80b1f..ed57ad944 100644
--- a/resources/views/settings/roles/form.blade.php
+++ b/resources/views/settings/roles/form.blade.php
@@ -19,7 +19,7 @@
                     @include('form.text', ['name' => 'description'])
                 </div>
 
-                @if(config('auth.method') === 'ldap' || config('saml2.enabled') === true)
+                @if(config('auth.method') === 'ldap' || config('auth.method') === 'saml2')
                     <div class="form-group">
                         <label for="name">{{ trans('settings.role_external_auth_id') }}</label>
                         @include('form.text', ['name' => 'external_auth_id'])
diff --git a/resources/views/users/form.blade.php b/resources/views/users/form.blade.php
index 6eafd43bc..df3d06c2f 100644
--- a/resources/views/users/form.blade.php
+++ b/resources/views/users/form.blade.php
@@ -25,7 +25,7 @@
     </div>
 </div>
 
-@if(($authMethod === 'ldap' || config('saml2.enabled') === true) && userCan('users-manage'))
+@if(($authMethod === 'ldap' || $authMethod === 'saml2') && userCan('users-manage'))
     <div class="grid half gap-xl v-center">
         <div>
             <label class="setting-list-label">{{ trans('settings.users_external_auth_id') }}</label>
diff --git a/routes/web.php b/routes/web.php
index eafad6489..3aa95dd68 100644
--- a/routes/web.php
+++ b/routes/web.php
@@ -225,7 +225,7 @@ Route::get('/register/confirm/{token}', 'Auth\ConfirmEmailController@confirm');
 Route::post('/register', 'Auth\RegisterController@postRegister');
 
 // SAML routes
-Route::get('/saml2/login', 'Auth\Saml2Controller@login');
+Route::post('/saml2/login', 'Auth\Saml2Controller@login');
 Route::get('/saml2/logout', 'Auth\Saml2Controller@logout');
 Route::get('/saml2/metadata', 'Auth\Saml2Controller@metadata');
 Route::get('/saml2/sls', 'Auth\Saml2Controller@sls');

From 5d08ec3cef1d9a2a1c96f47371f94f0762a49075 Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Sun, 2 Feb 2020 12:00:41 +0000
Subject: [PATCH 5/9] Fixed failing tests caused by auth changes

---
 app/Config/auth.php                           |   2 +-
 .../views/auth/forms/login/ldap.blade.php     |   4 +-
 tests/Auth/LdapTest.php                       | 111 +++++++++---------
 tests/Auth/Saml2Test.php                      |  42 ++-----
 4 files changed, 71 insertions(+), 88 deletions(-)

diff --git a/app/Config/auth.php b/app/Config/auth.php
index 2afb10ec2..66793308b 100644
--- a/app/Config/auth.php
+++ b/app/Config/auth.php
@@ -51,7 +51,7 @@ return [
     // mechanisms used by this application to persist your user's data.
     'providers' => [
         'users' => [
-            'driver' => env('AUTH_METHOD', 'standard') === 'standard' ? 'eloquent' : env('AUTH_METHOD'),
+            'driver' => 'eloquent',
             'model' => \BookStack\Auth\User::class,
         ],
         'external' => [
diff --git a/resources/views/auth/forms/login/ldap.blade.php b/resources/views/auth/forms/login/ldap.blade.php
index 12592d492..92eba80e8 100644
--- a/resources/views/auth/forms/login/ldap.blade.php
+++ b/resources/views/auth/forms/login/ldap.blade.php
@@ -19,10 +19,8 @@
             <label for="password">{{ trans('auth.password') }}</label>
             @include('form.password', ['name' => 'password'])
         </div>
-    </div>
 
-    <div class="grid half collapse-xs gap-xl v-center">
-        <div class="text-right">
+        <div class="form-group text-right pt-s">
             <button class="button">{{ Str::title(trans('auth.log_in')) }}</button>
         </div>
     </div>
diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php
index 5d7bbfb1e..8bf9475cc 100644
--- a/tests/Auth/LdapTest.php
+++ b/tests/Auth/LdapTest.php
@@ -1,4 +1,5 @@
 <?php namespace Tests;
+
 use BookStack\Auth\Role;
 use BookStack\Auth\Access\Ldap;
 use BookStack\Auth\User;
@@ -21,12 +22,16 @@ class LdapTest extends BrowserKitTest
         if (!defined('LDAP_OPT_REFERRALS')) define('LDAP_OPT_REFERRALS', 1);
         app('config')->set([
             'auth.method' => 'ldap',
+            'auth.defaults.guard' => 'ldap',
             'services.ldap.base_dn' => 'dc=ldap,dc=local',
             'services.ldap.email_attribute' => 'mail',
             'services.ldap.display_name_attribute' => 'cn',
             'services.ldap.id_attribute' => 'uid',
             'services.ldap.user_to_groups' => false,
-            'auth.providers.users.driver' => 'ldap',
+            'services.ldap.version' => '3',
+            'services.ldap.user_filter' => '(&(uid=${user}))',
+            'services.ldap.follow_referrals' => false,
+            'services.ldap.tls_insecure' => false,
         ]);
         $this->mockLdap = \Mockery::mock(Ldap::class);
         $this->app[Ldap::class] = $this->mockLdap;
@@ -60,16 +65,16 @@ class LdapTest extends BrowserKitTest
     {
         $this->mockLdap->shouldReceive('connect')->once()->andReturn($this->resourceId);
         $this->mockLdap->shouldReceive('setVersion')->once();
-        $this->mockLdap->shouldReceive('setOption')->times(4);
-        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(4)
+        $this->mockLdap->shouldReceive('setOption')->times(2);
+        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2)
             ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
             ->andReturn(['count' => 1, 0 => [
                 'uid' => [$this->mockUser->name],
                 'cn' => [$this->mockUser->name],
                 'dn' => ['dc=test' . config('services.ldap.base_dn')]
             ]]);
-        $this->mockLdap->shouldReceive('bind')->times(6)->andReturn(true);
-        $this->mockEscapes(4);
+        $this->mockLdap->shouldReceive('bind')->times(4)->andReturn(true);
+        $this->mockEscapes(2);
 
         $this->mockUserLogin()
             ->seePageIs('/login')->see('Please enter an email to use for this account.');
@@ -86,16 +91,16 @@ class LdapTest extends BrowserKitTest
         $this->mockLdap->shouldReceive('connect')->once()->andReturn($this->resourceId);
         $this->mockLdap->shouldReceive('setVersion')->once();
         $ldapDn = 'cn=test-user,dc=test' . config('services.ldap.base_dn');
-        $this->mockLdap->shouldReceive('setOption')->times(2);
-        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2)
+        $this->mockLdap->shouldReceive('setOption')->times(1);
+        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1)
             ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
             ->andReturn(['count' => 1, 0 => [
                 'cn' => [$this->mockUser->name],
                 'dn' => $ldapDn,
                 'mail' => [$this->mockUser->email]
             ]]);
-        $this->mockLdap->shouldReceive('bind')->times(3)->andReturn(true);
-        $this->mockEscapes(2);
+        $this->mockLdap->shouldReceive('bind')->times(2)->andReturn(true);
+        $this->mockEscapes(1);
 
         $this->mockUserLogin()
             ->seePageIs('/')
@@ -109,8 +114,8 @@ class LdapTest extends BrowserKitTest
         $this->mockLdap->shouldReceive('connect')->once()->andReturn($this->resourceId);
         $this->mockLdap->shouldReceive('setVersion')->once();
         $ldapDn = 'cn=test-user,dc=test' . config('services.ldap.base_dn');
-        $this->mockLdap->shouldReceive('setOption')->times(2);
-        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2)
+        $this->mockLdap->shouldReceive('setOption')->times(1);
+        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1)
             ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
             ->andReturn(['count' => 1, 0 => [
                 'cn' => [$this->mockUser->name],
@@ -120,8 +125,8 @@ class LdapTest extends BrowserKitTest
             ]]);
 
 
-        $this->mockLdap->shouldReceive('bind')->times(3)->andReturn(true);
-        $this->mockEscapes(2);
+        $this->mockLdap->shouldReceive('bind')->times(2)->andReturn(true);
+        $this->mockEscapes(1);
 
         $this->mockUserLogin()
             ->seePageIs('/')
@@ -133,16 +138,16 @@ class LdapTest extends BrowserKitTest
     {
         $this->mockLdap->shouldReceive('connect')->once()->andReturn($this->resourceId);
         $this->mockLdap->shouldReceive('setVersion')->once();
-        $this->mockLdap->shouldReceive('setOption')->times(2);
-        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2)
+        $this->mockLdap->shouldReceive('setOption')->times(1);
+        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1)
             ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
             ->andReturn(['count' => 1, 0 => [
                 'uid' => [$this->mockUser->name],
                 'cn' => [$this->mockUser->name],
                 'dn' => ['dc=test' . config('services.ldap.base_dn')]
             ]]);
-        $this->mockLdap->shouldReceive('bind')->times(3)->andReturn(true, true, false);
-        $this->mockEscapes(2);
+        $this->mockLdap->shouldReceive('bind')->times(2)->andReturn(true, false);
+        $this->mockEscapes(1);
 
         $this->mockUserLogin()
             ->seePageIs('/login')->see('These credentials do not match our records.')
@@ -201,10 +206,10 @@ class LdapTest extends BrowserKitTest
             'services.ldap.group_attribute' => 'memberOf',
             'services.ldap.remove_from_groups' => false,
         ]);
-        $this->mockLdap->shouldReceive('connect')->times(2)->andReturn($this->resourceId);
-        $this->mockLdap->shouldReceive('setVersion')->times(2);
-        $this->mockLdap->shouldReceive('setOption')->times(5);
-        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(5)
+        $this->mockLdap->shouldReceive('connect')->times(1)->andReturn($this->resourceId);
+        $this->mockLdap->shouldReceive('setVersion')->times(1);
+        $this->mockLdap->shouldReceive('setOption')->times(4);
+        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(4)
             ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
             ->andReturn(['count' => 1, 0 => [
                 'uid' => [$this->mockUser->name],
@@ -217,8 +222,8 @@ class LdapTest extends BrowserKitTest
                     1 => "cn=ldaptester-second,ou=groups,dc=example,dc=com",
                 ]
             ]]);
-        $this->mockLdap->shouldReceive('bind')->times(6)->andReturn(true);
-        $this->mockEscapes(5);
+        $this->mockLdap->shouldReceive('bind')->times(5)->andReturn(true);
+        $this->mockEscapes(4);
         $this->mockExplodes(6);
 
         $this->mockUserLogin()->seePageIs('/');
@@ -250,10 +255,10 @@ class LdapTest extends BrowserKitTest
             'services.ldap.group_attribute' => 'memberOf',
             'services.ldap.remove_from_groups' => true,
         ]);
-        $this->mockLdap->shouldReceive('connect')->times(2)->andReturn($this->resourceId);
-        $this->mockLdap->shouldReceive('setVersion')->times(2);
-        $this->mockLdap->shouldReceive('setOption')->times(4);
-        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(4)
+        $this->mockLdap->shouldReceive('connect')->times(1)->andReturn($this->resourceId);
+        $this->mockLdap->shouldReceive('setVersion')->times(1);
+        $this->mockLdap->shouldReceive('setOption')->times(3);
+        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(3)
             ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
             ->andReturn(['count' => 1, 0 => [
                 'uid' => [$this->mockUser->name],
@@ -265,8 +270,8 @@ class LdapTest extends BrowserKitTest
                     0 => "cn=ldaptester,ou=groups,dc=example,dc=com",
                 ]
             ]]);
-        $this->mockLdap->shouldReceive('bind')->times(5)->andReturn(true);
-        $this->mockEscapes(4);
+        $this->mockLdap->shouldReceive('bind')->times(4)->andReturn(true);
+        $this->mockEscapes(3);
         $this->mockExplodes(2);
 
         $this->mockUserLogin()->seePageIs('/');
@@ -299,10 +304,10 @@ class LdapTest extends BrowserKitTest
             'services.ldap.group_attribute' => 'memberOf',
             'services.ldap.remove_from_groups' => true,
         ]);
-        $this->mockLdap->shouldReceive('connect')->times(2)->andReturn($this->resourceId);
-        $this->mockLdap->shouldReceive('setVersion')->times(2);
-        $this->mockLdap->shouldReceive('setOption')->times(4);
-        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(4)
+        $this->mockLdap->shouldReceive('connect')->times(1)->andReturn($this->resourceId);
+        $this->mockLdap->shouldReceive('setVersion')->times(1);
+        $this->mockLdap->shouldReceive('setOption')->times(3);
+        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(3)
             ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
             ->andReturn(['count' => 1, 0 => [
                 'uid' => [$this->mockUser->name],
@@ -314,8 +319,8 @@ class LdapTest extends BrowserKitTest
                     0 => "cn=ex-auth-a,ou=groups,dc=example,dc=com",
                 ]
             ]]);
-        $this->mockLdap->shouldReceive('bind')->times(5)->andReturn(true);
-        $this->mockEscapes(4);
+        $this->mockLdap->shouldReceive('bind')->times(4)->andReturn(true);
+        $this->mockEscapes(3);
         $this->mockExplodes(2);
 
         $this->mockUserLogin()->seePageIs('/');
@@ -344,10 +349,10 @@ class LdapTest extends BrowserKitTest
             'services.ldap.group_attribute' => 'memberOf',
             'services.ldap.remove_from_groups' => true,
         ]);
-        $this->mockLdap->shouldReceive('connect')->times(2)->andReturn($this->resourceId);
-        $this->mockLdap->shouldReceive('setVersion')->times(2);
-        $this->mockLdap->shouldReceive('setOption')->times(5);
-        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(5)
+        $this->mockLdap->shouldReceive('connect')->times(1)->andReturn($this->resourceId);
+        $this->mockLdap->shouldReceive('setVersion')->times(1);
+        $this->mockLdap->shouldReceive('setOption')->times(4);
+        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(4)
             ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
             ->andReturn(['count' => 1, 0 => [
                 'uid' => [$this->mockUser->name],
@@ -360,8 +365,8 @@ class LdapTest extends BrowserKitTest
                     1 => "cn=ldaptester-second,ou=groups,dc=example,dc=com",
                 ]
             ]]);
-        $this->mockLdap->shouldReceive('bind')->times(6)->andReturn(true);
-        $this->mockEscapes(5);
+        $this->mockLdap->shouldReceive('bind')->times(5)->andReturn(true);
+        $this->mockEscapes(4);
         $this->mockExplodes(6);
 
         $this->mockUserLogin()->seePageIs('/');
@@ -385,8 +390,8 @@ class LdapTest extends BrowserKitTest
 
         $this->mockLdap->shouldReceive('connect')->once()->andReturn($this->resourceId);
         $this->mockLdap->shouldReceive('setVersion')->once();
-        $this->mockLdap->shouldReceive('setOption')->times(4);
-        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(4)
+        $this->mockLdap->shouldReceive('setOption')->times(2);
+        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2)
             ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
             ->andReturn(['count' => 1, 0 => [
                 'uid' => [$this->mockUser->name],
@@ -394,8 +399,8 @@ class LdapTest extends BrowserKitTest
                 'dn' => ['dc=test' . config('services.ldap.base_dn')],
                 'displayname' => 'displayNameAttribute'
             ]]);
-        $this->mockLdap->shouldReceive('bind')->times(6)->andReturn(true);
-        $this->mockEscapes(4);
+        $this->mockLdap->shouldReceive('bind')->times(4)->andReturn(true);
+        $this->mockEscapes(2);
 
         $this->mockUserLogin()
             ->seePageIs('/login')->see('Please enter an email to use for this account.');
@@ -415,16 +420,16 @@ class LdapTest extends BrowserKitTest
 
         $this->mockLdap->shouldReceive('connect')->once()->andReturn($this->resourceId);
         $this->mockLdap->shouldReceive('setVersion')->once();
-        $this->mockLdap->shouldReceive('setOption')->times(4);
-        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(4)
+        $this->mockLdap->shouldReceive('setOption')->times(2);
+        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2)
             ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
             ->andReturn(['count' => 1, 0 => [
                 'uid' => [$this->mockUser->name],
                 'cn' => [$this->mockUser->name],
                 'dn' => ['dc=test' . config('services.ldap.base_dn')]
             ]]);
-        $this->mockLdap->shouldReceive('bind')->times(6)->andReturn(true);
-        $this->mockEscapes(4);
+        $this->mockLdap->shouldReceive('bind')->times(4)->andReturn(true);
+        $this->mockEscapes(2);
 
         $this->mockUserLogin()
             ->seePageIs('/login')->see('Please enter an email to use for this account.');
@@ -444,14 +449,14 @@ class LdapTest extends BrowserKitTest
 
         // Standard mocks
         $this->mockLdap->shouldReceive('setVersion')->once();
-        $this->mockLdap->shouldReceive('setOption')->times(2);
-        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2)->andReturn(['count' => 1, 0 => [
+        $this->mockLdap->shouldReceive('setOption')->times(1);
+        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1)->andReturn(['count' => 1, 0 => [
             'uid' => [$this->mockUser->name],
             'cn' => [$this->mockUser->name],
             'dn' => ['dc=test' . config('services.ldap.base_dn')]
         ]]);
-        $this->mockLdap->shouldReceive('bind')->times(3)->andReturn(true);
-        $this->mockEscapes(2);
+        $this->mockLdap->shouldReceive('bind')->times(2)->andReturn(true);
+        $this->mockEscapes(1);
 
         $this->mockLdap->shouldReceive('connect')->once()
             ->with($expectedHost, $expectedPort)->andReturn($this->resourceId);
diff --git a/tests/Auth/Saml2Test.php b/tests/Auth/Saml2Test.php
index 45b6efa07..2cecad8bf 100644
--- a/tests/Auth/Saml2Test.php
+++ b/tests/Auth/Saml2Test.php
@@ -11,9 +11,9 @@ class Saml2Test extends TestCase
         parent::setUp();
         // Set default config for SAML2
         config()->set([
+            'auth.method' => 'saml2',
+            'auth.defaults.guard' => 'saml2',
             'saml2.name' => 'SingleSignOn-Testing',
-            'saml2.enabled' => true,
-            'saml2.auto_register' => true,
             'saml2.email_attribute' => 'email',
             'saml2.display_name_attributes' => ['first_name', 'last_name'],
             'saml2.external_id_attribute' => 'uid',
@@ -53,27 +53,12 @@ class Saml2Test extends TestCase
     {
         $req = $this->get('/login');
         $req->assertSeeText('SingleSignOn-Testing');
-        $req->assertElementExists('a[href$="/saml2/login"]');
-    }
-
-    public function test_login_option_shows_on_register_page_only_when_auto_register_enabled()
-    {
-        $this->setSettings(['app-public' => 'true', 'registration-enabled' => 'true']);
-
-        $req = $this->get('/register');
-        $req->assertSeeText('SingleSignOn-Testing');
-        $req->assertElementExists('a[href$="/saml2/login"]');
-
-        config()->set(['saml2.auto_register' => false]);
-
-        $req = $this->get('/register');
-        $req->assertDontSeeText('SingleSignOn-Testing');
-        $req->assertElementNotExists('a[href$="/saml2/login"]');
+        $req->assertElementExists('form[action$="/saml2/login"][method=POST] button');
     }
 
     public function test_login()
     {
-        $req = $this->get('/saml2/login');
+        $req = $this->post('/saml2/login');
         $redirect = $req->headers->get('location');
         $this->assertStringStartsWith('http://saml.local/saml2/idp/SSOService.php', $redirect, 'Login redirects to SSO location');
 
@@ -138,20 +123,15 @@ class Saml2Test extends TestCase
         });
     }
 
-    public function test_logout_redirects_to_saml_logout_when_active_saml_session()
+    public function test_logout_link_directs_to_saml_path()
     {
         config()->set([
             'saml2.onelogin.strict' => false,
         ]);
 
-        $this->withPost(['SAMLResponse' => $this->acsPostData], function () {
-            $acsPost = $this->post('/saml2/acs');
-            $lastLoginType = session()->get('last_login_type');
-            $this->assertEquals('saml2', $lastLoginType);
-
-            $req = $this->get('/logout');
-            $req->assertRedirect('/saml2/logout');
-        });
+        $resp = $this->actingAs($this->getEditor())->get('/');
+        $resp->assertElementExists('a[href$="/saml2/logout"]');
+        $resp->assertElementContains('a[href$="/saml2/logout"]', 'Logout');
     }
 
     public function test_logout_sls_flow()
@@ -235,8 +215,8 @@ class Saml2Test extends TestCase
 
     public function test_saml_routes_are_only_active_if_saml_enabled()
     {
-        config()->set(['saml2.enabled' => false]);
-        $getRoutes = ['/login', '/logout', '/metadata', '/sls'];
+        config()->set(['auth.method' => 'standard']);
+        $getRoutes = ['/logout', '/metadata', '/sls'];
         foreach ($getRoutes as $route) {
             $req = $this->get('/saml2' . $route);
             $req->assertRedirect('/');
@@ -245,7 +225,7 @@ class Saml2Test extends TestCase
             session()->flush();
         }
 
-        $postRoutes = ['/acs'];
+        $postRoutes = ['/login', '/acs'];
         foreach ($postRoutes as $route) {
             $req = $this->post('/saml2' . $route);
             $req->assertRedirect('/');

From e6c6de0848caab863f410bbf9a74312392db9dcd Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Sun, 2 Feb 2020 13:10:21 +0000
Subject: [PATCH 6/9] Simplified guard names and rolled out guard route checks

- Included tests to cover for LDAP and SAML
- Updated wording for external auth id option.
- Updated 'assertPermissionError' test case to be usable in BrowserKitTests
---
 app/Config/auth.php                           |  6 +--
 .../Auth/ForgotPasswordController.php         |  1 +
 app/Http/Controllers/Auth/LoginController.php | 14 ++---
 .../Controllers/Auth/RegisterController.php   |  3 +-
 .../Auth/ResetPasswordController.php          |  1 +
 app/Http/Controllers/Auth/Saml2Controller.php | 11 +---
 .../Controllers/Auth/UserInviteController.php | 16 ++----
 app/Http/Kernel.php                           |  5 +-
 app/Http/Middleware/CheckGuard.php            | 27 ++++++++++
 app/Http/Middleware/PermissionMiddleware.php  |  2 +-
 resources/lang/en/settings.php                |  2 +-
 routes/web.php                                |  4 +-
 tests/Api/ApiAuthTest.php                     |  8 +--
 tests/Auth/LdapTest.php                       | 33 ++++++++++++
 tests/Auth/Saml2Test.php                      | 52 ++++++++++++++++---
 tests/SharedTestHelpers.php                   | 15 ++++++
 tests/TestCase.php                            | 13 -----
 17 files changed, 146 insertions(+), 67 deletions(-)
 create mode 100644 app/Http/Middleware/CheckGuard.php

diff --git a/app/Config/auth.php b/app/Config/auth.php
index 66793308b..51b152ff1 100644
--- a/app/Config/auth.php
+++ b/app/Config/auth.php
@@ -11,14 +11,14 @@
 return [
 
     // Method of authentication to use
-    // Options: standard, ldap
+    // Options: standard, ldap, saml2
     'method' => env('AUTH_METHOD', 'standard'),
 
     // Authentication Defaults
     // This option controls the default authentication "guard" and password
     // reset options for your application.
     'defaults' => [
-        'guard' => env('AUTH_METHOD', 'standard') === 'standard' ? 'web' : env('AUTH_METHOD'),
+        'guard' => env('AUTH_METHOD', 'standard'),
         'passwords' => 'users',
     ],
 
@@ -28,7 +28,7 @@ return [
     // mechanisms used by this application to persist your user's data.
     // Supported drivers: "session", "api-token", "ldap-session"
     'guards' => [
-        'web' => [
+        'standard' => [
             'driver' => 'session',
             'provider' => 'users',
         ],
diff --git a/app/Http/Controllers/Auth/ForgotPasswordController.php b/app/Http/Controllers/Auth/ForgotPasswordController.php
index a3c0433a5..a60fcc1c9 100644
--- a/app/Http/Controllers/Auth/ForgotPasswordController.php
+++ b/app/Http/Controllers/Auth/ForgotPasswordController.php
@@ -30,6 +30,7 @@ class ForgotPasswordController extends Controller
     public function __construct()
     {
         $this->middleware('guest');
+        $this->middleware('guard:standard');
         parent::__construct();
     }
 
diff --git a/app/Http/Controllers/Auth/LoginController.php b/app/Http/Controllers/Auth/LoginController.php
index 8116288ad..4292ad6dd 100644
--- a/app/Http/Controllers/Auth/LoginController.php
+++ b/app/Http/Controllers/Auth/LoginController.php
@@ -38,7 +38,9 @@ class LoginController extends Controller
      */
     public function __construct(SocialAuthService $socialAuthService)
     {
-        $this->middleware('guest', ['only' => ['getLogin', 'postLogin']]);
+        $this->middleware('guest', ['only' => ['getLogin', 'login']]);
+        $this->middleware('guard:standard,ldap', ['only' => ['login', 'logout']]);
+
         $this->socialAuthService = $socialAuthService;
         $this->redirectPath = url('/');
         $this->redirectAfterLogout = url('/login');
@@ -159,14 +161,4 @@ class LoginController extends Controller
         return redirect('/login');
     }
 
-    /**
-     * Log the user out of the application.
-     */
-    public function logout(Request $request)
-    {
-        $this->guard()->logout();
-        $request->session()->invalidate();
-
-        return $this->loggedOut($request) ?: redirect('/');
-    }
 }
diff --git a/app/Http/Controllers/Auth/RegisterController.php b/app/Http/Controllers/Auth/RegisterController.php
index 8fb13d536..56ec66bff 100644
--- a/app/Http/Controllers/Auth/RegisterController.php
+++ b/app/Http/Controllers/Auth/RegisterController.php
@@ -43,7 +43,8 @@ class RegisterController extends Controller
      */
     public function __construct(SocialAuthService $socialAuthService, RegistrationService $registrationService)
     {
-        $this->middleware('guest')->only(['getRegister', 'postRegister']);
+        $this->middleware('guest');
+        $this->middleware('guard:standard');
 
         $this->socialAuthService = $socialAuthService;
         $this->registrationService = $registrationService;
diff --git a/app/Http/Controllers/Auth/ResetPasswordController.php b/app/Http/Controllers/Auth/ResetPasswordController.php
index 4d98eca59..214647cd5 100644
--- a/app/Http/Controllers/Auth/ResetPasswordController.php
+++ b/app/Http/Controllers/Auth/ResetPasswordController.php
@@ -31,6 +31,7 @@ class ResetPasswordController extends Controller
     public function __construct()
     {
         $this->middleware('guest');
+        $this->middleware('guard:standard');
         parent::__construct();
     }
 
diff --git a/app/Http/Controllers/Auth/Saml2Controller.php b/app/Http/Controllers/Auth/Saml2Controller.php
index 72cf0e019..8c0cb21d2 100644
--- a/app/Http/Controllers/Auth/Saml2Controller.php
+++ b/app/Http/Controllers/Auth/Saml2Controller.php
@@ -17,16 +17,7 @@ class Saml2Controller extends Controller
     {
         parent::__construct();
         $this->samlService = $samlService;
-
-        // SAML2 access middleware
-        $this->middleware(function ($request, $next) {
-
-            if (config('auth.method') !== 'saml2') {
-                $this->showPermissionError();
-            }
-
-            return $next($request);
-        });
+        $this->middleware('guard:saml2');
     }
 
     /**
diff --git a/app/Http/Controllers/Auth/UserInviteController.php b/app/Http/Controllers/Auth/UserInviteController.php
index c361b0a9b..c61b1c42b 100644
--- a/app/Http/Controllers/Auth/UserInviteController.php
+++ b/app/Http/Controllers/Auth/UserInviteController.php
@@ -8,11 +8,9 @@ use BookStack\Exceptions\UserTokenExpiredException;
 use BookStack\Exceptions\UserTokenNotFoundException;
 use BookStack\Http\Controllers\Controller;
 use Exception;
-use Illuminate\Contracts\View\Factory;
 use Illuminate\Http\RedirectResponse;
 use Illuminate\Http\Request;
 use Illuminate\Routing\Redirector;
-use Illuminate\View\View;
 
 class UserInviteController extends Controller
 {
@@ -21,22 +19,20 @@ class UserInviteController extends Controller
 
     /**
      * Create a new controller instance.
-     *
-     * @param UserInviteService $inviteService
-     * @param UserRepo $userRepo
      */
     public function __construct(UserInviteService $inviteService, UserRepo $userRepo)
     {
+        $this->middleware('guest');
+        $this->middleware('guard:standard');
+
         $this->inviteService = $inviteService;
         $this->userRepo = $userRepo;
-        $this->middleware('guest');
+
         parent::__construct();
     }
 
     /**
      * Show the page for the user to set the password for their account.
-     * @param string $token
-     * @return Factory|View|RedirectResponse
      * @throws Exception
      */
     public function showSetPassword(string $token)
@@ -54,9 +50,6 @@ class UserInviteController extends Controller
 
     /**
      * Sets the password for an invited user and then grants them access.
-     * @param Request $request
-     * @param string $token
-     * @return RedirectResponse|Redirector
      * @throws Exception
      */
     public function setPassword(Request $request, string $token)
@@ -85,7 +78,6 @@ class UserInviteController extends Controller
 
     /**
      * Check and validate the exception thrown when checking an invite token.
-     * @param Exception $exception
      * @return RedirectResponse|Redirector
      * @throws Exception
      */
diff --git a/app/Http/Kernel.php b/app/Http/Kernel.php
index c2016281a..4517deb90 100644
--- a/app/Http/Kernel.php
+++ b/app/Http/Kernel.php
@@ -48,7 +48,8 @@ class Kernel extends HttpKernel
         'auth'       => \BookStack\Http\Middleware\Authenticate::class,
         'can'        => \Illuminate\Auth\Middleware\Authorize::class,
         'guest'      => \BookStack\Http\Middleware\RedirectIfAuthenticated::class,
-        'throttle' => \Illuminate\Routing\Middleware\ThrottleRequests::class,
-        'perm'       => \BookStack\Http\Middleware\PermissionMiddleware::class
+        'throttle'   => \Illuminate\Routing\Middleware\ThrottleRequests::class,
+        'perm'       => \BookStack\Http\Middleware\PermissionMiddleware::class,
+        'guard'      => \BookStack\Http\Middleware\CheckGuard::class,
     ];
 }
diff --git a/app/Http/Middleware/CheckGuard.php b/app/Http/Middleware/CheckGuard.php
new file mode 100644
index 000000000..cc73ea68d
--- /dev/null
+++ b/app/Http/Middleware/CheckGuard.php
@@ -0,0 +1,27 @@
+<?php
+
+namespace BookStack\Http\Middleware;
+
+use Closure;
+
+class CheckGuard
+{
+    /**
+     * Handle an incoming request.
+     *
+     * @param  \Illuminate\Http\Request  $request
+     * @param  \Closure  $next
+     * @param string $allowedGuards
+     * @return mixed
+     */
+    public function handle($request, Closure $next, ...$allowedGuards)
+    {
+        $activeGuard = config('auth.method');
+        if (!in_array($activeGuard, $allowedGuards)) {
+            session()->flash('error', trans('errors.permission'));
+            return redirect('/');
+        }
+
+        return $next($request);
+    }
+}
diff --git a/app/Http/Middleware/PermissionMiddleware.php b/app/Http/Middleware/PermissionMiddleware.php
index 28ffff643..d0bb4f79e 100644
--- a/app/Http/Middleware/PermissionMiddleware.php
+++ b/app/Http/Middleware/PermissionMiddleware.php
@@ -19,7 +19,7 @@ class PermissionMiddleware
     {
 
         if (!$request->user() || !$request->user()->can($permission)) {
-            Session::flash('error', trans('errors.permission'));
+            session()->flash('error', trans('errors.permission'));
             return redirect()->back();
         }
 
diff --git a/resources/lang/en/settings.php b/resources/lang/en/settings.php
index b36fdda7a..8c9675f09 100755
--- a/resources/lang/en/settings.php
+++ b/resources/lang/en/settings.php
@@ -131,7 +131,7 @@ return [
     'users_send_invite_text' => 'You can choose to send this user an invitation email which allows them to set their own password otherwise you can set their password yourself.',
     'users_send_invite_option' => 'Send user invite email',
     'users_external_auth_id' => 'External Authentication ID',
-    'users_external_auth_id_desc' => 'This is the ID used to match this user when communicating with your LDAP system.',
+    'users_external_auth_id_desc' => 'This is the ID used to match this user when communicating with your external authentication system.',
     'users_password_warning' => 'Only fill the below if you would like to change your password.',
     'users_system_public' => 'This user represents any guest users that visit your instance. It cannot be used to log in but is assigned automatically.',
     'users_delete' => 'Delete User',
diff --git a/routes/web.php b/routes/web.php
index 3aa95dd68..90261e1ac 100644
--- a/routes/web.php
+++ b/routes/web.php
@@ -210,7 +210,9 @@ Route::group(['middleware' => 'auth'], function () {
 // Social auth routes
 Route::get('/login/service/{socialDriver}', 'Auth\SocialController@getSocialLogin');
 Route::get('/login/service/{socialDriver}/callback', 'Auth\SocialController@socialCallback');
-Route::get('/login/service/{socialDriver}/detach', 'Auth\SocialController@detachSocialAccount');
+Route::group(['middleware' => 'auth'], function () {
+    Route::get('/login/service/{socialDriver}/detach', 'Auth\SocialController@detachSocialAccount');
+});
 Route::get('/register/service/{socialDriver}', 'Auth\SocialController@socialRegister');
 
 // Login/Logout routes
diff --git a/tests/Api/ApiAuthTest.php b/tests/Api/ApiAuthTest.php
index 2ab81814b..1f283753a 100644
--- a/tests/Api/ApiAuthTest.php
+++ b/tests/Api/ApiAuthTest.php
@@ -20,7 +20,7 @@ class ApiAuthTest extends TestCase
         $resp = $this->get($this->endpoint);
         $resp->assertStatus(401);
 
-        $this->actingAs($viewer, 'web');
+        $this->actingAs($viewer, 'standard');
 
         $resp = $this->get($this->endpoint);
         $resp->assertStatus(200);
@@ -72,11 +72,11 @@ class ApiAuthTest extends TestCase
     public function test_api_access_permission_required_to_access_api_with_session_auth()
     {
         $editor = $this->getEditor();
-        $this->actingAs($editor, 'web');
+        $this->actingAs($editor, 'standard');
 
         $resp = $this->get($this->endpoint);
         $resp->assertStatus(200);
-        auth('web')->logout();
+        auth('standard')->logout();
 
         $accessApiPermission = RolePermission::getByName('access-api');
         $editorRole = $this->getEditor()->roles()->first();
@@ -84,7 +84,7 @@ class ApiAuthTest extends TestCase
 
         $editor = User::query()->where('id', '=', $editor->id)->first();
 
-        $this->actingAs($editor, 'web');
+        $this->actingAs($editor, 'standard');
         $resp = $this->get($this->endpoint);
         $resp->assertStatus(403);
         $resp->assertJson($this->errorResponse("The owner of the used API token does not have permission to make API calls", 403));
diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php
index 8bf9475cc..92ff4a829 100644
--- a/tests/Auth/LdapTest.php
+++ b/tests/Auth/LdapTest.php
@@ -478,4 +478,37 @@ class LdapTest extends BrowserKitTest
     {
         $this->checkLdapReceivesCorrectDetails('ldap.bookstack.com', 'ldap.bookstack.com', 389);
     }
+
+    public function test_forgot_password_routes_inaccessible()
+    {
+        $resp = $this->get('/password/email');
+        $this->assertPermissionError($resp);
+
+        $resp = $this->post('/password/email');
+        $this->assertPermissionError($resp);
+
+        $resp = $this->get('/password/reset/abc123');
+        $this->assertPermissionError($resp);
+
+        $resp = $this->post('/password/reset');
+        $this->assertPermissionError($resp);
+    }
+
+    public function test_user_invite_routes_inaccessible()
+    {
+        $resp = $this->get('/register/invite/abc123');
+        $this->assertPermissionError($resp);
+
+        $resp = $this->post('/register/invite/abc123');
+        $this->assertPermissionError($resp);
+    }
+
+    public function test_user_register_routes_inaccessible()
+    {
+        $resp = $this->get('/register');
+        $this->assertPermissionError($resp);
+
+        $resp = $this->post('/register');
+        $this->assertPermissionError($resp);
+    }
 }
diff --git a/tests/Auth/Saml2Test.php b/tests/Auth/Saml2Test.php
index 2cecad8bf..b3e6bd41b 100644
--- a/tests/Auth/Saml2Test.php
+++ b/tests/Auth/Saml2Test.php
@@ -219,22 +219,58 @@ class Saml2Test extends TestCase
         $getRoutes = ['/logout', '/metadata', '/sls'];
         foreach ($getRoutes as $route) {
             $req = $this->get('/saml2' . $route);
-            $req->assertRedirect('/');
-            $error = session()->get('error');
-            $this->assertStringStartsWith('You do not have permission to access', $error);
-            session()->flush();
+            $this->assertPermissionError($req);
         }
 
         $postRoutes = ['/login', '/acs'];
         foreach ($postRoutes as $route) {
             $req = $this->post('/saml2' . $route);
-            $req->assertRedirect('/');
-            $error = session()->get('error');
-            $this->assertStringStartsWith('You do not have permission to access', $error);
-            session()->flush();
+            $this->assertPermissionError($req);
         }
     }
 
+    public function test_forgot_password_routes_inaccessible()
+    {
+        $resp = $this->get('/password/email');
+        $this->assertPermissionError($resp);
+
+        $resp = $this->post('/password/email');
+        $this->assertPermissionError($resp);
+
+        $resp = $this->get('/password/reset/abc123');
+        $this->assertPermissionError($resp);
+
+        $resp = $this->post('/password/reset');
+        $this->assertPermissionError($resp);
+    }
+
+    public function test_standard_login_routes_inaccessible()
+    {
+        $resp = $this->post('/login');
+        $this->assertPermissionError($resp);
+
+        $resp = $this->get('/logout');
+        $this->assertPermissionError($resp);
+    }
+
+    public function test_user_invite_routes_inaccessible()
+    {
+        $resp = $this->get('/register/invite/abc123');
+        $this->assertPermissionError($resp);
+
+        $resp = $this->post('/register/invite/abc123');
+        $this->assertPermissionError($resp);
+    }
+
+    public function test_user_register_routes_inaccessible()
+    {
+        $resp = $this->get('/register');
+        $this->assertPermissionError($resp);
+
+        $resp = $this->post('/register');
+        $this->assertPermissionError($resp);
+    }
+
     protected function withGet(array $options, callable $callback)
     {
         return $this->withGlobal($_GET, $options, $callback);
diff --git a/tests/SharedTestHelpers.php b/tests/SharedTestHelpers.php
index 3433f3b83..f7b7d5edf 100644
--- a/tests/SharedTestHelpers.php
+++ b/tests/SharedTestHelpers.php
@@ -262,4 +262,19 @@ trait SharedTestHelpers
         self::assertThat($passed, self::isTrue(), "Failed asserting that given map:\n\n{$toCheckStr}\n\nincludes:\n\n{$toIncludeStr}");
     }
 
+    /**
+     * Assert a permission error has occurred.
+     */
+    protected function assertPermissionError($response)
+    {
+        if ($response instanceof BrowserKitTest) {
+            $response = \Illuminate\Foundation\Testing\TestResponse::fromBaseResponse($response->response);
+        }
+
+        $response->assertRedirect('/');
+        $this->assertSessionHas('error');
+        $error = session()->pull('error');
+        $this->assertStringStartsWith('You do not have permission to access', $error);
+    }
+
 }
\ No newline at end of file
diff --git a/tests/TestCase.php b/tests/TestCase.php
index f20b20fd8..1f1d5ece7 100644
--- a/tests/TestCase.php
+++ b/tests/TestCase.php
@@ -16,19 +16,6 @@ abstract class TestCase extends BaseTestCase
      */
     protected $baseUrl = 'http://localhost';
 
-    /**
-     * Assert a permission error has occurred.
-     * @param TestResponse $response
-     * @return TestCase
-     */
-    protected function assertPermissionError(TestResponse $response)
-    {
-        $response->assertRedirect('/');
-        $this->assertSessionHas('error');
-        session()->remove('error');
-        return $this;
-    }
-
     /**
      * Assert the session contains a specific entry.
      * @param string $key

From 3991fbe726c527b89d06cb0b2afdca705cafa494 Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Sun, 2 Feb 2020 17:31:00 +0000
Subject: [PATCH 7/9] Checked over and aligned registration option behavior
 across all auth options

- Added tests to cover
---
 app/Auth/Access/ExternalAuthService.php       |  6 +-
 .../Guards/ExternalBaseSessionGuard.php       | 15 ++--
 app/Auth/Access/Guards/LdapSessionGuard.php   | 73 ++++++------------
 app/Auth/Access/LdapService.php               |  6 +-
 app/Auth/Access/RegistrationService.php       | 74 +++++++++++++++----
 app/Auth/Access/Saml2Service.php              | 45 ++++-------
 app/Auth/Access/SocialAuthService.php         |  4 +-
 app/Auth/User.php                             | 22 +++---
 app/Auth/UserRepo.php                         | 34 ++-------
 app/Exceptions/Handler.php                    |  5 +-
 app/Http/Controllers/Auth/LoginController.php |  1 +
 .../Controllers/Auth/RegisterController.php   |  7 +-
 app/Http/Controllers/Auth/Saml2Controller.php |  1 -
 .../Controllers/Auth/SocialController.php     | 19 ++---
 app/Providers/AuthServiceProvider.php         |  5 +-
 resources/lang/en/errors.php                  |  1 -
 resources/lang/en/settings.php                |  2 +-
 resources/views/settings/index.blade.php      |  4 +-
 tests/Auth/LdapTest.php                       | 32 ++++++++
 tests/Auth/Saml2Test.php                      | 22 +++++-
 20 files changed, 200 insertions(+), 178 deletions(-)

diff --git a/app/Auth/Access/ExternalAuthService.php b/app/Auth/Access/ExternalAuthService.php
index 4bd8f8680..db8bd2dfb 100644
--- a/app/Auth/Access/ExternalAuthService.php
+++ b/app/Auth/Access/ExternalAuthService.php
@@ -64,10 +64,8 @@ class ExternalAuthService
 
     /**
      * Sync the groups to the user roles for the current user
-     * @param \BookStack\Auth\User $user
-     * @param array $userGroups
      */
-    public function syncWithGroups(User $user, array $userGroups)
+    public function syncWithGroups(User $user, array $userGroups): void
     {
         // Get the ids for the roles from the names
         $groupsAsRoles = $this->matchGroupsToSystemsRoles($userGroups);
@@ -75,7 +73,7 @@ class ExternalAuthService
         // Sync groups
         if ($this->config['remove_from_groups']) {
             $user->roles()->sync($groupsAsRoles);
-            $this->userRepo->attachDefaultRole($user);
+            $user->attachDefaultRole();
         } else {
             $user->roles()->syncWithoutDetaching($groupsAsRoles);
         }
diff --git a/app/Auth/Access/Guards/ExternalBaseSessionGuard.php b/app/Auth/Access/Guards/ExternalBaseSessionGuard.php
index d1fb0b606..f3d05366d 100644
--- a/app/Auth/Access/Guards/ExternalBaseSessionGuard.php
+++ b/app/Auth/Access/Guards/ExternalBaseSessionGuard.php
@@ -2,10 +2,7 @@
 
 namespace BookStack\Auth\Access\Guards;
 
-use BookStack\Auth\User;
-use BookStack\Auth\UserRepo;
-use BookStack\Exceptions\LoginAttemptEmailNeededException;
-use BookStack\Exceptions\LoginAttemptException;
+use BookStack\Auth\Access\RegistrationService;
 use Illuminate\Auth\GuardHelpers;
 use Illuminate\Contracts\Auth\Authenticatable as AuthenticatableContract;
 use Illuminate\Contracts\Auth\StatefulGuard;
@@ -56,23 +53,23 @@ class ExternalBaseSessionGuard implements StatefulGuard
     protected $loggedOut = false;
 
     /**
-     * Repository to perform user-specific actions.
+     * Service to handle common registration actions.
      *
-     * @var UserRepo
+     * @var RegistrationService
      */
-    protected $userRepo;
+    protected $registrationService;
 
     /**
      * Create a new authentication guard.
      *
      * @return void
      */
-    public function __construct(string $name, UserProvider $provider, Session $session, UserRepo $userRepo)
+    public function __construct(string $name, UserProvider $provider, Session $session, RegistrationService $registrationService)
     {
         $this->name = $name;
         $this->session = $session;
         $this->provider = $provider;
-        $this->userRepo = $userRepo;
+        $this->registrationService = $registrationService;
     }
 
     /**
diff --git a/app/Auth/Access/Guards/LdapSessionGuard.php b/app/Auth/Access/Guards/LdapSessionGuard.php
index 6c416bf30..3c98140f6 100644
--- a/app/Auth/Access/Guards/LdapSessionGuard.php
+++ b/app/Auth/Access/Guards/LdapSessionGuard.php
@@ -3,13 +3,17 @@
 namespace BookStack\Auth\Access\Guards;
 
 use BookStack\Auth\Access\LdapService;
+use BookStack\Auth\Access\RegistrationService;
 use BookStack\Auth\User;
 use BookStack\Auth\UserRepo;
 use BookStack\Exceptions\LdapException;
 use BookStack\Exceptions\LoginAttemptException;
 use BookStack\Exceptions\LoginAttemptEmailNeededException;
+use BookStack\Exceptions\UserRegistrationException;
 use Illuminate\Contracts\Auth\UserProvider;
 use Illuminate\Contracts\Session\Session;
+use Illuminate\Support\Facades\Hash;
+use Illuminate\Support\Str;
 
 class LdapSessionGuard extends ExternalBaseSessionGuard
 {
@@ -23,11 +27,11 @@ class LdapSessionGuard extends ExternalBaseSessionGuard
         UserProvider $provider,
         Session $session,
         LdapService $ldapService,
-        UserRepo $userRepo
+        RegistrationService $registrationService
     )
     {
         $this->ldapService = $ldapService;
-        parent::__construct($name, $provider, $session, $userRepo);
+        parent::__construct($name, $provider, $session, $registrationService);
     }
 
     /**
@@ -56,6 +60,7 @@ class LdapSessionGuard extends ExternalBaseSessionGuard
      * @throws LoginAttemptEmailNeededException
      * @throws LoginAttemptException
      * @throws LdapException
+     * @throws UserRegistrationException
      */
     public function attempt(array $credentials = [], $remember = false)
     {
@@ -70,12 +75,9 @@ class LdapSessionGuard extends ExternalBaseSessionGuard
         }
 
         if (is_null($user)) {
-            $user = $this->freshUserInstanceFromLdapUserDetails($userDetails);
+            $user = $this->createNewFromLdapAndCreds($userDetails, $credentials);
         }
 
-        $this->checkForUserEmail($user, $credentials['email'] ?? '');
-        $this->saveIfNew($user);
-
         // Sync LDAP groups if required
         if ($this->ldapService->shouldSyncGroups()) {
             $this->ldapService->syncGroups($user, $username);
@@ -86,58 +88,27 @@ class LdapSessionGuard extends ExternalBaseSessionGuard
     }
 
     /**
-     * Save the give user if they don't yet existing in the system.
-     * @throws LoginAttemptException
-     */
-    protected function saveIfNew(User $user)
-    {
-        if ($user->exists) {
-            return;
-        }
-
-        // Check for existing users with same email
-        $alreadyUser = $user->newQuery()->where('email', '=', $user->email)->count() > 0;
-        if ($alreadyUser) {
-            throw new LoginAttemptException(trans('errors.error_user_exists_different_creds', ['email' => $user->email]));
-        }
-
-        $user->save();
-        $this->userRepo->attachDefaultRole($user);
-        $this->userRepo->downloadAndAssignUserAvatar($user);
-    }
-
-    /**
-     * Ensure the given user has an email.
-     * Takes the provided email in the request if a value is provided
-     * and the user does not have an existing email.
+     * Create a new user from the given ldap credentials and login credentials
      * @throws LoginAttemptEmailNeededException
+     * @throws LoginAttemptException
+     * @throws UserRegistrationException
      */
-    protected function checkForUserEmail(User $user, string $providedEmail)
+    protected function createNewFromLdapAndCreds(array $ldapUserDetails, array $credentials): User
     {
-        // Request email if missing from user and missing from request
-        if (is_null($user->email) && !$providedEmail) {
+        $email = trim($ldapUserDetails['email'] ?: ($credentials['email'] ?? ''));
+
+        if (empty($email)) {
             throw new LoginAttemptEmailNeededException();
         }
 
-        // Add email to model if non-existing and email provided in request
-        if (!$user->exists && is_null($user->email) && $providedEmail) {
-            $user->email = $providedEmail;
-        }
-    }
+        $details = [
+            'name' => $ldapUserDetails['name'],
+            'email' => $ldapUserDetails['email'] ?: $credentials['email'],
+            'external_auth_id' => $ldapUserDetails['uid'],
+            'password' => Str::random(32),
+        ];
 
-    /**
-     * Create a fresh user instance from details provided by a LDAP lookup.
-     */
-    protected function freshUserInstanceFromLdapUserDetails(array $ldapUserDetails): User
-    {
-        $user = new User();
-
-        $user->name = $ldapUserDetails['name'];
-        $user->external_auth_id = $ldapUserDetails['uid'];
-        $user->email = $ldapUserDetails['email'];
-        $user->email_confirmed = false;
-
-        return $user;
+        return $this->registrationService->registerUser($details, null, false);
     }
 
 }
diff --git a/app/Auth/Access/LdapService.php b/app/Auth/Access/LdapService.php
index cc2890817..07e9f7b64 100644
--- a/app/Auth/Access/LdapService.php
+++ b/app/Auth/Access/LdapService.php
@@ -1,10 +1,8 @@
 <?php namespace BookStack\Auth\Access;
 
 use BookStack\Auth\User;
-use BookStack\Auth\UserRepo;
 use BookStack\Exceptions\LdapException;
 use ErrorException;
-use Illuminate\Contracts\Auth\Authenticatable;
 
 /**
  * Class LdapService
@@ -16,17 +14,15 @@ class LdapService extends ExternalAuthService
     protected $ldap;
     protected $ldapConnection;
     protected $config;
-    protected $userRepo;
     protected $enabled;
 
     /**
      * LdapService constructor.
      */
-    public function __construct(Ldap $ldap, UserRepo $userRepo)
+    public function __construct(Ldap $ldap)
     {
         $this->ldap = $ldap;
         $this->config = config('services.ldap');
-        $this->userRepo = $userRepo;
         $this->enabled = config('auth.method') === 'ldap';
     }
 
diff --git a/app/Auth/Access/RegistrationService.php b/app/Auth/Access/RegistrationService.php
index 74142301a..8cf76013a 100644
--- a/app/Auth/Access/RegistrationService.php
+++ b/app/Auth/Access/RegistrationService.php
@@ -1,6 +1,7 @@
 <?php namespace BookStack\Auth\Access;
 
 use BookStack\Auth\SocialAccount;
+use BookStack\Auth\User;
 use BookStack\Auth\UserRepo;
 use BookStack\Exceptions\UserRegistrationException;
 use Exception;
@@ -24,38 +25,51 @@ class RegistrationService
      * Check whether or not registrations are allowed in the app settings.
      * @throws UserRegistrationException
      */
-    public function checkRegistrationAllowed()
+    public function ensureRegistrationAllowed()
+    {
+        if (!$this->registrationAllowed()) {
+            throw new UserRegistrationException(trans('auth.registrations_disabled'), '/login');
+        }
+    }
+
+    /**
+     * Check if standard BookStack User registrations are currently allowed.
+     * Does not prevent external-auth based registration.
+     */
+    protected function registrationAllowed(): bool
     {
         $authMethod = config('auth.method');
         $authMethodsWithRegistration = ['standard'];
-        if (!setting('registration-enabled') || !in_array($authMethod, $authMethodsWithRegistration)) {
-            throw new UserRegistrationException(trans('auth.registrations_disabled'), '/login');
-        }
+        return in_array($authMethod, $authMethodsWithRegistration) && setting('registration-enabled');
     }
 
     /**
      * The registrations flow for all users.
      * @throws UserRegistrationException
      */
-    public function registerUser(array $userData, ?SocialAccount $socialAccount = null, bool $emailVerified = false)
+    public function registerUser(array $userData, ?SocialAccount $socialAccount = null, bool $emailConfirmed = false): User
     {
-        $registrationRestrict = setting('registration-restrict');
+        $userEmail = $userData['email'];
 
-        if ($registrationRestrict) {
-            $restrictedEmailDomains = explode(',', str_replace(' ', '', $registrationRestrict));
-            $userEmailDomain = $domain = mb_substr(mb_strrchr($userData['email'], "@"), 1);
-            if (!in_array($userEmailDomain, $restrictedEmailDomains)) {
-                throw new UserRegistrationException(trans('auth.registration_email_domain_invalid'), '/register');
-            }
+        // Email restriction
+        $this->ensureEmailDomainAllowed($userEmail);
+
+        // Ensure user does not already exist
+        $alreadyUser = !is_null($this->userRepo->getByEmail($userEmail));
+        if ($alreadyUser) {
+            throw new UserRegistrationException(trans('errors.error_user_exists_different_creds', ['email' => $userEmail]));
         }
 
-        $newUser = $this->userRepo->registerNew($userData, $emailVerified);
+        // Create the user
+        $newUser = $this->userRepo->registerNew($userData, $emailConfirmed);
 
+        // Assign social account if given
         if ($socialAccount) {
             $newUser->socialAccounts()->save($socialAccount);
         }
 
-        if ($this->emailConfirmationService->confirmationRequired() && !$emailVerified) {
+        // Start email confirmation flow if required
+        if ($this->emailConfirmationService->confirmationRequired() && !$emailConfirmed) {
             $newUser->save();
             $message = '';
 
@@ -68,7 +82,37 @@ class RegistrationService
             throw new UserRegistrationException($message, '/register/confirm');
         }
 
-        auth()->login($newUser);
+        return $newUser;
+    }
+
+    /**
+     * Ensure that the given email meets any active email domain registration restrictions.
+     * Throws if restrictions are active and the email does not match an allowed domain.
+     * @throws UserRegistrationException
+     */
+    protected function ensureEmailDomainAllowed(string $userEmail): void
+    {
+        $registrationRestrict = setting('registration-restrict');
+
+        if (!$registrationRestrict) {
+            return;
+        }
+
+        $restrictedEmailDomains = explode(',', str_replace(' ', '', $registrationRestrict));
+        $userEmailDomain = $domain = mb_substr(mb_strrchr($userEmail, "@"), 1);
+        if (!in_array($userEmailDomain, $restrictedEmailDomains)) {
+            $redirect = $this->registrationAllowed() ? '/register' : '/login';
+            throw new UserRegistrationException(trans('auth.registration_email_domain_invalid'), $redirect);
+        }
+    }
+
+    /**
+     * Alias to the UserRepo method of the same name.
+     * Attaches the default system role, if configured, to the given user.
+     */
+    public function attachDefaultRole(User $user): void
+    {
+        $this->userRepo->attachDefaultRole($user);
     }
 
 }
\ No newline at end of file
diff --git a/app/Auth/Access/Saml2Service.php b/app/Auth/Access/Saml2Service.php
index c52dc3a39..8f9a24cde 100644
--- a/app/Auth/Access/Saml2Service.php
+++ b/app/Auth/Access/Saml2Service.php
@@ -1,9 +1,9 @@
 <?php namespace BookStack\Auth\Access;
 
 use BookStack\Auth\User;
-use BookStack\Auth\UserRepo;
 use BookStack\Exceptions\JsonDebugException;
 use BookStack\Exceptions\SamlException;
+use BookStack\Exceptions\UserRegistrationException;
 use Exception;
 use Illuminate\Support\Str;
 use OneLogin\Saml2\Auth;
@@ -18,16 +18,16 @@ use OneLogin\Saml2\ValidationError;
 class Saml2Service extends ExternalAuthService
 {
     protected $config;
-    protected $userRepo;
+    protected $registrationService;
     protected $user;
 
     /**
      * Saml2Service constructor.
      */
-    public function __construct(UserRepo $userRepo, User $user)
+    public function __construct(RegistrationService $registrationService, User $user)
     {
         $this->config = config('saml2');
-        $this->userRepo = $userRepo;
+        $this->registrationService = $registrationService;
         $this->user = $user;
     }
 
@@ -78,6 +78,7 @@ class Saml2Service extends ExternalAuthService
      * @throws SamlException
      * @throws ValidationError
      * @throws JsonDebugException
+     * @throws UserRegistrationException
      */
     public function processAcsResponse(?string $requestId): ?User
     {
@@ -308,34 +309,10 @@ class Saml2Service extends ExternalAuthService
         return $defaultValue;
     }
 
-    /**
-     *  Register a user that is authenticated but not already registered.
-     */
-    protected function registerUser(array $userDetails): User
-    {
-        // Create an array of the user data to create a new user instance
-        $userData = [
-            'name' => $userDetails['name'],
-            'email' => $userDetails['email'],
-            'password' => Str::random(32),
-            'external_auth_id' => $userDetails['external_id'],
-            'email_confirmed' => true,
-        ];
-
-        $existingUser = $this->user->newQuery()->where('email', '=', $userDetails['email'])->first();
-        if ($existingUser) {
-            throw new SamlException(trans('errors.saml_email_exists', ['email' => $userDetails['email']]));
-        }
-
-        $user = $this->user->newQuery()->forceCreate($userData);
-        $this->userRepo->attachDefaultRole($user);
-        $this->userRepo->downloadAndAssignUserAvatar($user);
-        return $user;
-    }
-
     /**
      * Get the user from the database for the specified details.
      * @throws SamlException
+     * @throws UserRegistrationException
      */
     protected function getOrRegisterUser(array $userDetails): ?User
     {
@@ -344,7 +321,14 @@ class Saml2Service extends ExternalAuthService
           ->first();
 
         if (is_null($user)) {
-            $user = $this->registerUser($userDetails);
+            $userData = [
+                'name' => $userDetails['name'],
+                'email' => $userDetails['email'],
+                'password' => Str::random(32),
+                'external_auth_id' => $userDetails['external_id'],
+            ];
+
+            $user = $this->registrationService->registerUser($userData, null, false);
         }
 
         return $user;
@@ -355,6 +339,7 @@ class Saml2Service extends ExternalAuthService
      * they exist, optionally registering them automatically.
      * @throws SamlException
      * @throws JsonDebugException
+     * @throws UserRegistrationException
      */
     public function processLoginCallback(string $samlID, array $samlAttributes): User
     {
diff --git a/app/Auth/Access/SocialAuthService.php b/app/Auth/Access/SocialAuthService.php
index cf836618a..16815a8e1 100644
--- a/app/Auth/Access/SocialAuthService.php
+++ b/app/Auth/Access/SocialAuthService.php
@@ -64,7 +64,7 @@ class SocialAuthService
 
         if ($this->userRepo->getByEmail($socialUser->getEmail())) {
             $email = $socialUser->getEmail();
-            throw new UserRegistrationException(trans('errors.social_account_in_use', ['socialAccount'=>$socialDriver, 'email' => $email]), '/login');
+            throw new UserRegistrationException(trans('errors.error_user_exists_different_creds', ['email' => $email]), '/login');
         }
 
         return $socialUser;
@@ -124,7 +124,7 @@ class SocialAuthService
 
         // Otherwise let the user know this social account is not used by anyone.
         $message = trans('errors.social_account_not_used', ['socialAccount' => $titleCaseDriver]);
-        if (setting('registration-enabled') && config('auth.method') !== 'ldap') {
+        if (setting('registration-enabled') && config('auth.method') !== 'ldap' && config('auth.method') !== 'saml2') {
             $message .= trans('errors.social_account_register_instructions', ['socialAccount' => $titleCaseDriver]);
         }
         
diff --git a/app/Auth/User.php b/app/Auth/User.php
index 35b3cd54f..28fb9c7fc 100644
--- a/app/Auth/User.php
+++ b/app/Auth/User.php
@@ -116,6 +116,17 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon
         return $this->roles->pluck('system_name')->contains($role);
     }
 
+    /**
+     * Attach the default system role to this user.
+     */
+    public function attachDefaultRole(): void
+    {
+        $roleId = setting('registration-role');
+        if ($roleId && $this->roles()->where('id', '=', $roleId)->count() === 0) {
+            $this->roles()->attach($roleId);
+        }
+    }
+
     /**
      * Get all permissions belonging to a the current user.
      * @param bool $cache
@@ -153,16 +164,7 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon
      */
     public function attachRole(Role $role)
     {
-        $this->attachRoleId($role->id);
-    }
-
-    /**
-     * Attach a role id to this user.
-     * @param $id
-     */
-    public function attachRoleId($id)
-    {
-        $this->roles()->attach($id);
+        $this->roles()->attach($role->id);
     }
 
     /**
diff --git a/app/Auth/UserRepo.php b/app/Auth/UserRepo.php
index e082b2dd5..cfa7bfce1 100644
--- a/app/Auth/UserRepo.php
+++ b/app/Auth/UserRepo.php
@@ -29,10 +29,9 @@ class UserRepo
     }
 
     /**
-     * @param string $email
-     * @return User|null
+     * Get a user by their email address.
      */
-    public function getByEmail($email)
+    public function getByEmail(string $email): ?User
     {
         return $this->user->where('email', '=', $email)->first();
     }
@@ -78,31 +77,16 @@ class UserRepo
 
      /**
      * Creates a new user and attaches a role to them.
-     * @param array $data
-     * @param boolean $verifyEmail
-     * @return User
      */
-    public function registerNew(array $data, $verifyEmail = false)
+    public function registerNew(array $data, bool $emailConfirmed = false): User
     {
-        $user = $this->create($data, $verifyEmail);
-        $this->attachDefaultRole($user);
+        $user = $this->create($data, $emailConfirmed);
+        $user->attachDefaultRole();
         $this->downloadAndAssignUserAvatar($user);
 
         return $user;
     }
 
-    /**
-     * Give a user the default role. Used when creating a new user.
-     * @param User $user
-     */
-    public function attachDefaultRole(User $user)
-    {
-        $roleId = setting('registration-role');
-        if ($roleId !== false && $user->roles()->where('id', '=', $roleId)->count() === 0) {
-            $user->attachRoleId($roleId);
-        }
-    }
-
     /**
      * Assign a user to a system-level role.
      * @param User $user
@@ -172,17 +156,15 @@ class UserRepo
 
     /**
      * Create a new basic instance of user.
-     * @param array $data
-     * @param boolean $verifyEmail
-     * @return User
      */
-    public function create(array $data, $verifyEmail = false)
+    public function create(array $data, bool $emailConfirmed = false): User
     {
         return $this->user->forceCreate([
             'name'     => $data['name'],
             'email'    => $data['email'],
             'password' => bcrypt($data['password']),
-            'email_confirmed' => $verifyEmail
+            'email_confirmed' => $emailConfirmed,
+            'external_auth_id' => $data['external_auth_id'] ?? '',
         ]);
     }
 
diff --git a/app/Exceptions/Handler.php b/app/Exceptions/Handler.php
index 284d731d7..a3bc1e8b9 100644
--- a/app/Exceptions/Handler.php
+++ b/app/Exceptions/Handler.php
@@ -57,7 +57,10 @@ class Handler extends ExceptionHandler
         // Handle notify exceptions which will redirect to the
         // specified location then show a notification message.
         if ($this->isExceptionType($e, NotifyException::class)) {
-            session()->flash('error', $this->getOriginalMessage($e));
+            $message = $this->getOriginalMessage($e);
+            if (!empty($message)) {
+                session()->flash('error', $message);
+            }
             return redirect($e->redirectLocation);
         }
 
diff --git a/app/Http/Controllers/Auth/LoginController.php b/app/Http/Controllers/Auth/LoginController.php
index 4292ad6dd..ea584a3b6 100644
--- a/app/Http/Controllers/Auth/LoginController.php
+++ b/app/Http/Controllers/Auth/LoginController.php
@@ -5,6 +5,7 @@ namespace BookStack\Http\Controllers\Auth;
 use BookStack\Auth\Access\SocialAuthService;
 use BookStack\Exceptions\LoginAttemptEmailNeededException;
 use BookStack\Exceptions\LoginAttemptException;
+use BookStack\Exceptions\UserRegistrationException;
 use BookStack\Http\Controllers\Controller;
 use Illuminate\Foundation\Auth\AuthenticatesUsers;
 use Illuminate\Http\Request;
diff --git a/app/Http/Controllers/Auth/RegisterController.php b/app/Http/Controllers/Auth/RegisterController.php
index 56ec66bff..0bdeef9e6 100644
--- a/app/Http/Controllers/Auth/RegisterController.php
+++ b/app/Http/Controllers/Auth/RegisterController.php
@@ -74,7 +74,7 @@ class RegisterController extends Controller
      */
     public function getRegister()
     {
-        $this->registrationService->checkRegistrationAllowed();
+        $this->registrationService->ensureRegistrationAllowed();
         $socialDrivers = $this->socialAuthService->getActiveDrivers();
         return view('auth.register', [
             'socialDrivers' => $socialDrivers,
@@ -87,12 +87,13 @@ class RegisterController extends Controller
      */
     public function postRegister(Request $request)
     {
-        $this->registrationService->checkRegistrationAllowed();
+        $this->registrationService->ensureRegistrationAllowed();
         $this->validator($request->all())->validate();
         $userData = $request->all();
 
         try {
-            $this->registrationService->registerUser($userData);
+            $user = $this->registrationService->registerUser($userData);
+            auth()->login($user);
         } catch (UserRegistrationException $exception) {
             if ($exception->getMessage()) {
                 $this->showErrorNotification($exception->getMessage());
diff --git a/app/Http/Controllers/Auth/Saml2Controller.php b/app/Http/Controllers/Auth/Saml2Controller.php
index 8c0cb21d2..7ffcc572b 100644
--- a/app/Http/Controllers/Auth/Saml2Controller.php
+++ b/app/Http/Controllers/Auth/Saml2Controller.php
@@ -81,7 +81,6 @@ class Saml2Controller extends Controller
             return redirect('/login');
         }
 
-        session()->put('last_login_type', 'saml2');
         return redirect()->intended();
     }
 
diff --git a/app/Http/Controllers/Auth/SocialController.php b/app/Http/Controllers/Auth/SocialController.php
index bcd82a9c0..ae7a1bc06 100644
--- a/app/Http/Controllers/Auth/SocialController.php
+++ b/app/Http/Controllers/Auth/SocialController.php
@@ -49,7 +49,7 @@ class SocialController extends Controller
      */
     public function socialRegister(string $socialDriver)
     {
-        $this->registrationService->checkRegistrationAllowed();
+        $this->registrationService->ensureRegistrationAllowed();
         session()->put('social-callback', 'register');
         return $this->socialAuthService->startRegister($socialDriver);
     }
@@ -78,7 +78,7 @@ class SocialController extends Controller
 
         // Attempt login or fall-back to register if allowed.
         $socialUser = $this->socialAuthService->getSocialUser($socialDriver);
-        if ($action == 'login') {
+        if ($action === 'login') {
             try {
                 return $this->socialAuthService->handleLoginCallback($socialDriver, $socialUser);
             } catch (SocialSignInAccountNotUsed $exception) {
@@ -89,7 +89,7 @@ class SocialController extends Controller
             }
         }
 
-        if ($action == 'register') {
+        if ($action === 'register') {
             return $this->socialRegisterCallback($socialDriver, $socialUser);
         }
 
@@ -108,7 +108,6 @@ class SocialController extends Controller
 
     /**
      * Register a new user after a registration callback.
-     * @return RedirectResponse|Redirector
      * @throws UserRegistrationException
      */
     protected function socialRegisterCallback(string $socialDriver, SocialUser $socialUser)
@@ -121,17 +120,11 @@ class SocialController extends Controller
         $userData = [
             'name' => $socialUser->getName(),
             'email' => $socialUser->getEmail(),
-            'password' => Str::random(30)
+            'password' => Str::random(32)
         ];
 
-        try {
-            $this->registrationService->registerUser($userData, $socialAccount, $emailVerified);
-        } catch (UserRegistrationException $exception) {
-            if ($exception->getMessage()) {
-                $this->showErrorNotification($exception->getMessage());
-            }
-            return redirect($exception->redirectLocation);
-        }
+        $user = $this->registrationService->registerUser($userData, $socialAccount, $emailVerified);
+        auth()->login($user);
 
         $this->showSuccessNotification(trans('auth.register_success'));
         return redirect('/');
diff --git a/app/Providers/AuthServiceProvider.php b/app/Providers/AuthServiceProvider.php
index a885628f3..fe52df168 100644
--- a/app/Providers/AuthServiceProvider.php
+++ b/app/Providers/AuthServiceProvider.php
@@ -8,6 +8,7 @@ use BookStack\Auth\Access\ExternalBaseUserProvider;
 use BookStack\Auth\Access\Guards\LdapSessionGuard;
 use BookStack\Auth\Access\Guards\Saml2SessionGuard;
 use BookStack\Auth\Access\LdapService;
+use BookStack\Auth\Access\RegistrationService;
 use BookStack\Auth\UserRepo;
 use Illuminate\Support\ServiceProvider;
 
@@ -31,7 +32,7 @@ class AuthServiceProvider extends ServiceProvider
                 $provider,
                 $this->app['session.store'],
                 $app[LdapService::class],
-                $app[UserRepo::class]
+                $app[RegistrationService::class]
             );
         });
 
@@ -41,7 +42,7 @@ class AuthServiceProvider extends ServiceProvider
                 $name,
                 $provider,
                 $this->app['session.store'],
-                $app[UserRepo::class]
+                $app[RegistrationService::class]
             );
         });
     }
diff --git a/resources/lang/en/errors.php b/resources/lang/en/errors.php
index bb7b6148c..4752d8b0c 100644
--- a/resources/lang/en/errors.php
+++ b/resources/lang/en/errors.php
@@ -23,7 +23,6 @@ return [
     'saml_no_email_address' => 'Could not find an email address, for this user, in the data provided by the external authentication system',
     'saml_invalid_response_id' => 'The request from the external authentication system is not recognised by a process started by this application. Navigating back after a login could cause this issue.',
     'saml_fail_authed' => 'Login using :system failed, system did not provide successful authorization',
-    'saml_email_exists' => 'Registration unsuccessful since a user already exists with email address ":email"',
     'social_no_action_defined' => 'No action defined',
     'social_login_bad_response' => "Error received during :socialAccount login: \n:error",
     'social_account_in_use' => 'This :socialAccount account is already in use, Try logging in via the :socialAccount option.',
diff --git a/resources/lang/en/settings.php b/resources/lang/en/settings.php
index 8c9675f09..692489afd 100755
--- a/resources/lang/en/settings.php
+++ b/resources/lang/en/settings.php
@@ -56,7 +56,7 @@ return [
     'reg_enable_toggle' => 'Enable registration',
     'reg_enable_desc' => 'When registration is enabled user will be able to sign themselves up as an application user. Upon registration they are given a single, default user role.',
     'reg_default_role' => 'Default user role after registration',
-    'reg_enable_ldap_warning' => 'The option above is not used while LDAP authentication is active. User accounts for non-existing members will be auto-created if authentication, against the LDAP system in use, is successful.',
+    'reg_enable_external_warning' => 'The option above is ignore while external LDAP or SAML authentication is active. User accounts for non-existing members will be auto-created if authentication, against the external system in use, is successful.',
     'reg_email_confirmation' => 'Email Confirmation',
     'reg_email_confirmation_toggle' => 'Require email confirmation',
     'reg_confirm_email_desc' => 'If domain restriction is used then email confirmation will be required and this option will be ignored.',
diff --git a/resources/views/settings/index.blade.php b/resources/views/settings/index.blade.php
index 94605da6f..2585ec5e3 100644
--- a/resources/views/settings/index.blade.php
+++ b/resources/views/settings/index.blade.php
@@ -219,8 +219,8 @@
                                 'label' => trans('settings.reg_enable_toggle')
                             ])
 
-                            @if(config('auth.method') === 'ldap')
-                                <div class="text-warn text-small mb-l">{{ trans('settings.reg_enable_ldap_warning') }}</div>
+                            @if(in_array(config('auth.method'), ['ldap', 'saml2']))
+                                <div class="text-warn text-small mb-l">{{ trans('settings.reg_enable_external_warning') }}</div>
                             @endif
 
                             <label for="setting-registration-role">{{ trans('settings.reg_default_role') }}</label>
diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php
index 92ff4a829..cb1194e22 100644
--- a/tests/Auth/LdapTest.php
+++ b/tests/Auth/LdapTest.php
@@ -86,6 +86,38 @@ class LdapTest extends BrowserKitTest
             ->seeInDatabase('users', ['email' => $this->mockUser->email, 'email_confirmed' => false, 'external_auth_id' => $this->mockUser->name]);
     }
 
+    public function test_email_domain_restriction_active_on_new_ldap_login()
+    {
+        $this->setSettings([
+            'registration-restrict' => 'testing.com'
+        ]);
+
+        $this->mockLdap->shouldReceive('connect')->once()->andReturn($this->resourceId);
+        $this->mockLdap->shouldReceive('setVersion')->once();
+        $this->mockLdap->shouldReceive('setOption')->times(2);
+        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2)
+            ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
+            ->andReturn(['count' => 1, 0 => [
+                'uid' => [$this->mockUser->name],
+                'cn' => [$this->mockUser->name],
+                'dn' => ['dc=test' . config('services.ldap.base_dn')]
+            ]]);
+        $this->mockLdap->shouldReceive('bind')->times(4)->andReturn(true);
+        $this->mockEscapes(2);
+
+        $this->mockUserLogin()
+            ->seePageIs('/login')
+            ->see('Please enter an email to use for this account.');
+
+        $email = 'tester@invaliddomain.com';
+
+        $this->type($email, '#email')
+            ->press('Log In')
+            ->seePageIs('/login')
+            ->see('That email domain does not have access to this application')
+            ->dontSeeInDatabase('users', ['email' => $email]);
+    }
+
     public function test_login_works_when_no_uid_provided_by_ldap_server()
     {
         $this->mockLdap->shouldReceive('connect')->once()->andReturn($this->resourceId);
diff --git a/tests/Auth/Saml2Test.php b/tests/Auth/Saml2Test.php
index b3e6bd41b..9a3d6d8ec 100644
--- a/tests/Auth/Saml2Test.php
+++ b/tests/Auth/Saml2Test.php
@@ -73,7 +73,7 @@ class Saml2Test extends TestCase
             $this->assertDatabaseHas('users', [
                 'email' => 'user@example.com',
                 'external_auth_id' => 'user',
-                'email_confirmed' => true,
+                'email_confirmed' => false,
                 'name' => 'Barry Scott'
             ]);
 
@@ -209,7 +209,7 @@ class Saml2Test extends TestCase
             $acsPost = $this->post('/saml2/acs');
             $acsPost->assertRedirect('/');
             $errorMessage = session()->get('error');
-            $this->assertEquals('Registration unsuccessful since a user already exists with email address "user@example.com"', $errorMessage);
+            $this->assertEquals('A user with the email user@example.com already exists but with different credentials.', $errorMessage);
         });
     }
 
@@ -271,6 +271,24 @@ class Saml2Test extends TestCase
         $this->assertPermissionError($resp);
     }
 
+    public function test_email_domain_restriction_active_on_new_saml_login()
+    {
+        $this->setSettings([
+            'registration-restrict' => 'testing.com'
+        ]);
+        config()->set([
+            'saml2.onelogin.strict' => false,
+        ]);
+
+        $this->withPost(['SAMLResponse' => $this->acsPostData], function () {
+            $acsPost = $this->post('/saml2/acs');
+            $acsPost->assertRedirect('/login');
+            $errorMessage = session()->get('error');
+            $this->assertStringContainsString('That email domain does not have access to this application', $errorMessage);
+            $this->assertDatabaseMissing('users', ['email' => 'user@example.com']);
+        });
+    }
+
     protected function withGet(array $options, callable $callback)
     {
         return $this->withGlobal($_GET, $options, $callback);

From b4f2b735904169eb12a05cb0befc4ff38beaeaee Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Sun, 2 Feb 2020 17:35:16 +0000
Subject: [PATCH 8/9] Updated settings-save action to return to the same
 section

---
 app/Http/Controllers/SettingController.php | 18 ++----------------
 resources/views/settings/index.blade.php   |  9 ++++++---
 2 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/app/Http/Controllers/SettingController.php b/app/Http/Controllers/SettingController.php
index f0a078300..7f7724468 100644
--- a/app/Http/Controllers/SettingController.php
+++ b/app/Http/Controllers/SettingController.php
@@ -5,8 +5,6 @@ use BookStack\Notifications\TestEmail;
 use BookStack\Uploads\ImageRepo;
 use BookStack\Uploads\ImageService;
 use Illuminate\Http\Request;
-use Illuminate\Http\Response;
-use Setting;
 
 class SettingController extends Controller
 {
@@ -14,7 +12,6 @@ class SettingController extends Controller
 
     /**
      * SettingController constructor.
-     * @param $imageRepo
      */
     public function __construct(ImageRepo $imageRepo)
     {
@@ -22,10 +19,8 @@ class SettingController extends Controller
         parent::__construct();
     }
 
-
     /**
      * Display a listing of the settings.
-     * @return Response
      */
     public function index()
     {
@@ -43,8 +38,6 @@ class SettingController extends Controller
 
     /**
      * Update the specified settings in storage.
-     * @param  Request $request
-     * @return Response
      */
     public function update(Request $request)
     {
@@ -78,12 +71,11 @@ class SettingController extends Controller
         }
 
         $this->showSuccessNotification(trans('settings.settings_save_success'));
-        return redirect('/settings');
+        return redirect('/settings#' . $request->get('section', ''));
     }
 
     /**
      * Show the page for application maintenance.
-     * @return \Illuminate\Contracts\View\Factory|\Illuminate\View\View
      */
     public function showMaintenance()
     {
@@ -98,9 +90,6 @@ class SettingController extends Controller
 
     /**
      * Action to clean-up images in the system.
-     * @param Request $request
-     * @param ImageService $imageService
-     * @return \Illuminate\Http\RedirectResponse|\Illuminate\Routing\Redirector
      */
     public function cleanupImages(Request $request, ImageService $imageService)
     {
@@ -127,11 +116,8 @@ class SettingController extends Controller
 
     /**
      * Action to send a test e-mail to the current user.
-     * @param Request $request
-     * @param User $user
-     * @return \Illuminate\Http\RedirectResponse|\Illuminate\Routing\Redirector
      */
-    public function sendTestEmail(Request $request)
+    public function sendTestEmail()
     {
         $this->checkPermission('settings-manage');
 
diff --git a/resources/views/settings/index.blade.php b/resources/views/settings/index.blade.php
index 2585ec5e3..b3a11955d 100644
--- a/resources/views/settings/index.blade.php
+++ b/resources/views/settings/index.blade.php
@@ -15,9 +15,10 @@
         </div>
 
         <div class="card content-wrap auto-height">
-            <h2 class="list-heading">{{ trans('settings.app_features_security') }}</h2>
+            <h2 id="features" class="list-heading">{{ trans('settings.app_features_security') }}</h2>
             <form action="{{ url("/settings") }}" method="POST">
                 {!! csrf_field() !!}
+                <input type="hidden" name="section" value="features">
 
                 <div class="setting-list">
 
@@ -79,9 +80,10 @@
         </div>
 
         <div class="card content-wrap auto-height">
-            <h2 class="list-heading">{{ trans('settings.app_customization') }}</h2>
+            <h2 id="customization" class="list-heading">{{ trans('settings.app_customization') }}</h2>
             <form action="{{ url("/settings") }}" method="POST" enctype="multipart/form-data">
                 {!! csrf_field() !!}
+                <input type="hidden" name="section" value="customization">
 
                 <div class="setting-list">
 
@@ -202,9 +204,10 @@
         </div>
 
         <div class="card content-wrap auto-height">
-            <h2 class="list-heading">{{ trans('settings.reg_settings') }}</h2>
+            <h2 id="registration" class="list-heading">{{ trans('settings.reg_settings') }}</h2>
             <form action="{{ url("/settings") }}" method="POST">
                 {!! csrf_field() !!}
+                <input type="hidden" name="section" value="registration">
 
                 <div class="setting-list">
                     <div class="grid half gap-xl">

From 9d77cca7340dec4872fba4742e7aa7c6d25e6052 Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Sun, 2 Feb 2020 17:57:21 +0000
Subject: [PATCH 9/9] Cleaned setting section redirect path

---
 app/Http/Controllers/SettingController.php | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/app/Http/Controllers/SettingController.php b/app/Http/Controllers/SettingController.php
index 7f7724468..892b2d9cf 100644
--- a/app/Http/Controllers/SettingController.php
+++ b/app/Http/Controllers/SettingController.php
@@ -71,7 +71,8 @@ class SettingController extends Controller
         }
 
         $this->showSuccessNotification(trans('settings.settings_save_success'));
-        return redirect('/settings#' . $request->get('section', ''));
+        $redirectLocation = '/settings#' . $request->get('section', '');
+        return redirect(rtrim($redirectLocation, '#'));
     }
 
     /**