Checked over and aligned registration option behavior across all auth options

- Added tests to cover

diff --git a/app/Auth/Access/ExternalAuthService.php b/app/Auth/Access/ExternalAuthService.php
index 4bd8f868014e337e8a6b79f9b9e85033a68092dd..db8bd2dfb7c2f83cd855d4622b818f17581045f8 100644 (file)
--- 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
 
     /**
      * 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);
     {
         // 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);
         // Sync groups
         if ($this->config['remove_from_groups']) {
             $user->roles()->sync($groupsAsRoles);

-            $this->userRepo->attachDefaultRole($user);
+            $user->attachDefaultRole();

         } else {
             $user->roles()->syncWithoutDetaching($groupsAsRoles);
         }
         } else {
             $user->roles()->syncWithoutDetaching($groupsAsRoles);
         }

diff --git a/app/Auth/Access/Guards/ExternalBaseSessionGuard.php b/app/Auth/Access/Guards/ExternalBaseSessionGuard.php
index d1fb0b606a3d04e8820fe73ff54bf3564879ba11..f3d05366d9544ee5dde9c1a17304199b83f834c7 100644 (file)
--- a/app/Auth/Access/Guards/ExternalBaseSessionGuard.php
+++ b/app/Auth/Access/Guards/ExternalBaseSessionGuard.php
@@ -2,10 +2,7 @@
 
 namespace BookStack\Auth\Access\Guards;
 
 
 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;
 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;
 
     /**
     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
      */
 
     /**
      * 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->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 6c416bf301621c31a8c887883b7470b2b0553557..3c98140f621b560f7f370d5fa0dab08eed29b5d6 100644 (file)
--- 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;
 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\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\Contracts\Auth\UserProvider;
 use Illuminate\Contracts\Session\Session;

+use Illuminate\Support\Facades\Hash;
+use Illuminate\Support\Str;

 
 class LdapSessionGuard extends ExternalBaseSessionGuard
 {
 
 class LdapSessionGuard extends ExternalBaseSessionGuard
 {


@@ -23,11 +27,11 @@ class LdapSessionGuard extends ExternalBaseSessionGuard
         UserProvider $provider,
         Session $session,
         LdapService $ldapService,
         UserProvider $provider,
         Session $session,
         LdapService $ldapService,

-        UserRepo $userRepo
+        RegistrationService $registrationService

     )
     {
         $this->ldapService = $ldapService;
     )
     {
         $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 LoginAttemptEmailNeededException
      * @throws LoginAttemptException
      * @throws LdapException

+     * @throws UserRegistrationException

      */
     public function attempt(array $credentials = [], $remember = false)
     {
      */
     public function attempt(array $credentials = [], $remember = false)
     {


@@ -70,12 +75,9 @@ class LdapSessionGuard extends ExternalBaseSessionGuard
         }
 
         if (is_null($user)) {
         }
 
         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);
         // 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.
+     * Create a new user from the given ldap credentials and login credentials
+     * @throws LoginAttemptEmailNeededException

      * @throws LoginAttemptException
      * @throws LoginAttemptException

+     * @throws UserRegistrationException

      */
      */

-    protected function saveIfNew(User $user)
+    protected function createNewFromLdapAndCreds(array $ldapUserDetails, array $credentials): 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]));
-        }
+        $email = trim($ldapUserDetails['email'] ?: ($credentials['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) {
+        if (empty($email)) {

             throw new LoginAttemptEmailNeededException();
         }
 
             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.
-     */
-    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;
+        $details = [
+            'name' => $ldapUserDetails['name'],
+            'email' => $ldapUserDetails['email'] ?: $credentials['email'],
+            'external_auth_id' => $ldapUserDetails['uid'],
+            'password' => Str::random(32),
+        ];

 
 

-        return $user;
+        return $this->registrationService->registerUser($details, null, false);

     }
 
 }
     }
 
 }

diff --git a/app/Auth/Access/LdapService.php b/app/Auth/Access/LdapService.php
index cc28908170206c4e9cb6b4f31134c1d5670ffd05..07e9f7b64e6b4516c53a4aa00f211a56d90598fb 100644 (file)
--- 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;
 <?php namespace BookStack\Auth\Access;
 
 use BookStack\Auth\User;

-use BookStack\Auth\UserRepo;

 use BookStack\Exceptions\LdapException;
 use ErrorException;
 use BookStack\Exceptions\LdapException;
 use ErrorException;

-use Illuminate\Contracts\Auth\Authenticatable;

 
 /**
  * Class LdapService
 
 /**
  * Class LdapService


@@ -16,17 +14,15 @@ class LdapService extends ExternalAuthService
     protected $ldap;
     protected $ldapConnection;
     protected $config;
     protected $ldap;
     protected $ldapConnection;
     protected $config;

-    protected $userRepo;

     protected $enabled;
 
     /**
      * LdapService constructor.
      */
     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->ldap = $ldap;
         $this->config = config('services.ldap');

-        $this->userRepo = $userRepo;

         $this->enabled = config('auth.method') === 'ldap';
     }
 
         $this->enabled = config('auth.method') === 'ldap';
     }
 

diff --git a/app/Auth/Access/RegistrationService.php b/app/Auth/Access/RegistrationService.php
index 74142301a288d07126e4980c0afa133ab759b15f..8cf76013a40c1ed04254ca07bb3443ef709e2db4 100644 (file)
--- 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;
 <?php namespace BookStack\Auth\Access;
 
 use BookStack\Auth\SocialAccount;

+use BookStack\Auth\User;

 use BookStack\Auth\UserRepo;
 use BookStack\Exceptions\UserRegistrationException;
 use Exception;
 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
      */
      * Check whether or not registrations are allowed in the app settings.
      * @throws UserRegistrationException
      */

-    public function checkRegistrationAllowed()
+    public function ensureRegistrationAllowed()

     {
     {

-        $authMethod = config('auth.method');
-        $authMethodsWithRegistration = ['standard'];
-        if (!setting('registration-enabled') || !in_array($authMethod, $authMethodsWithRegistration)) {
+        if (!$this->registrationAllowed()) {

             throw new UserRegistrationException(trans('auth.registrations_disabled'), '/login');
         }
     }
 
             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'];
+        return in_array($authMethod, $authMethodsWithRegistration) && setting('registration-enabled');
+    }
+

     /**
      * The registrations flow for all users.
      * @throws UserRegistrationException
      */
     /**
      * 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 ($socialAccount) {
             $newUser->socialAccounts()->save($socialAccount);
         }
 

-        if ($this->emailConfirmationService->confirmationRequired() && !$emailVerified) {
+        // Start email confirmation flow if required
+        if ($this->emailConfirmationService->confirmationRequired() && !$emailConfirmed) {

             $newUser->save();
             $message = '';
 
             $newUser->save();
             $message = '';
 


@@ -68,7 +82,37 @@ class RegistrationService
             throw new UserRegistrationException($message, '/register/confirm');
         }
 
             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
     }
 
 }
\ No newline at end of file

diff --git a/app/Auth/Access/Saml2Service.php b/app/Auth/Access/Saml2Service.php
index c52dc3a398aea2cba9ead90f55d0ed40bbbae9d3..8f9a24cdeacd142106c0a92411962bd59de877c5 100644 (file)
--- 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;
 <?php namespace BookStack\Auth\Access;
 
 use BookStack\Auth\User;

-use BookStack\Auth\UserRepo;

 use BookStack\Exceptions\JsonDebugException;
 use BookStack\Exceptions\SamlException;
 use BookStack\Exceptions\JsonDebugException;
 use BookStack\Exceptions\SamlException;

+use BookStack\Exceptions\UserRegistrationException;

 use Exception;
 use Illuminate\Support\Str;
 use OneLogin\Saml2\Auth;
 use Exception;
 use Illuminate\Support\Str;
 use OneLogin\Saml2\Auth;


@@ -18,16 +18,16 @@ use OneLogin\Saml2\ValidationError;
 class Saml2Service extends ExternalAuthService
 {
     protected $config;
 class Saml2Service extends ExternalAuthService
 {
     protected $config;

-    protected $userRepo;
+    protected $registrationService;

     protected $user;
 
     /**
      * Saml2Service constructor.
      */
     protected $user;
 
     /**
      * Saml2Service constructor.
      */

-    public function __construct(UserRepo $userRepo, User $user)
+    public function __construct(RegistrationService $registrationService, User $user)

     {
         $this->config = config('saml2');
     {
         $this->config = config('saml2');

-        $this->userRepo = $userRepo;
+        $this->registrationService = $registrationService;

         $this->user = $user;
     }
 
         $this->user = $user;
     }
 


@@ -78,6 +78,7 @@ class Saml2Service extends ExternalAuthService
      * @throws SamlException
      * @throws ValidationError
      * @throws JsonDebugException
      * @throws SamlException
      * @throws ValidationError
      * @throws JsonDebugException

+     * @throws UserRegistrationException

      */
     public function processAcsResponse(?string $requestId): ?User
     {
      */
     public function processAcsResponse(?string $requestId): ?User
     {


@@ -308,34 +309,10 @@ class Saml2Service extends ExternalAuthService
         return $defaultValue;
     }
 
         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
     /**
      * Get the user from the database for the specified details.
      * @throws SamlException

+     * @throws UserRegistrationException

      */
     protected function getOrRegisterUser(array $userDetails): ?User
     {
      */
     protected function getOrRegisterUser(array $userDetails): ?User
     {


@@ -344,7 +321,14 @@ class Saml2Service extends ExternalAuthService
           ->first();
 
         if (is_null($user)) {
           ->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;
         }
 
         return $user;


@@ -355,6 +339,7 @@ class Saml2Service extends ExternalAuthService
      * they exist, optionally registering them automatically.
      * @throws SamlException
      * @throws JsonDebugException
      * they exist, optionally registering them automatically.
      * @throws SamlException
      * @throws JsonDebugException

+     * @throws UserRegistrationException

      */
     public function processLoginCallback(string $samlID, array $samlAttributes): User
     {
      */
     public function processLoginCallback(string $samlID, array $samlAttributes): User
     {

diff --git a/app/Auth/Access/SocialAuthService.php b/app/Auth/Access/SocialAuthService.php
index cf836618a0fc3093e6660365ad7174374e8efcf6..16815a8e1b72d9f021e573975a473c94ab9d9f47 100644 (file)
--- 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();
 
         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;
         }
 
         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]);
 
         // 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]);
         }
         
             $message .= trans('errors.social_account_register_instructions', ['socialAccount' => $titleCaseDriver]);
         }
         

diff --git a/app/Auth/User.php b/app/Auth/User.php
index 35b3cd54f064e4a8615b801e32d6f7aa5cad82e8..28fb9c7fc2e17482518b8eee7886f151a5342be5 100644 (file)
--- 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);
     }
 
         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
     /**
      * 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)
     {
      */
     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 e082b2dd50bbbad68bea82bfed57fd19a3af857d..cfa7bfce1f1c836ffde1beddd0f58e66d07bd99a 100644 (file)
--- 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();
     }
     {
         return $this->user->where('email', '=', $email)->first();
     }


@@ -78,31 +77,16 @@ class UserRepo
 
      /**
      * Creates a new user and attaches a role to them.
 
      /**
      * 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;
     }
 
         $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
     /**
      * Assign a user to a system-level role.
      * @param User $user


@@ -172,17 +156,15 @@ class UserRepo
 
     /**
      * Create a new basic instance of user.
 
     /**
      * 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']),
     {
         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 284d731d7194a434b3cdc69dea3e5a9d1ad3efd6..a3bc1e8b9e222e102fe7a8c93b1d2161362a0e8b 100644 (file)
--- 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)) {
         // 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);
         }
 
             return redirect($e->redirectLocation);
         }
 

diff --git a/app/Http/Controllers/Auth/LoginController.php b/app/Http/Controllers/Auth/LoginController.php
index 4292ad6dd97a4bd75a18599f14e11fba025a1e4e..ea584a3b6f0427f92c271fa8924bff4a5796edb9 100644 (file)
--- 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\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;
 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 56ec66bfff3a65948ada1101cb4d161780fd56a7..0bdeef9e6855c1337c34ff934bece9ab5d42d45d 100644 (file)
--- 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()
     {
      */
     public function getRegister()
     {

-        $this->registrationService->checkRegistrationAllowed();
+        $this->registrationService->ensureRegistrationAllowed();

         $socialDrivers = $this->socialAuthService->getActiveDrivers();
         return view('auth.register', [
             'socialDrivers' => $socialDrivers,
         $socialDrivers = $this->socialAuthService->getActiveDrivers();
         return view('auth.register', [
             'socialDrivers' => $socialDrivers,


@@ -87,12 +87,13 @@ class RegisterController extends Controller
      */
     public function postRegister(Request $request)
     {
      */
     public function postRegister(Request $request)
     {

-        $this->registrationService->checkRegistrationAllowed();
+        $this->registrationService->ensureRegistrationAllowed();

         $this->validator($request->all())->validate();
         $userData = $request->all();
 
         try {
         $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());
         } 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 8c0cb21d2821f6377dacd07b2ff20c2625c2a450..7ffcc572bcd06dc43f003df6edb9f8c05d84720e 100644 (file)
--- 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');
         }
 
             return redirect('/login');
         }
 

-        session()->put('last_login_type', 'saml2');

         return redirect()->intended();
     }
 
         return redirect()->intended();
     }
 

diff --git a/app/Http/Controllers/Auth/SocialController.php b/app/Http/Controllers/Auth/SocialController.php
index bcd82a9c03567a56ae885257b8697d2411574bbe..ae7a1bc069cc3a504404cf06d36a1a7da478036f 100644 (file)
--- 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)
     {
      */
     public function socialRegister(string $socialDriver)
     {

-        $this->registrationService->checkRegistrationAllowed();
+        $this->registrationService->ensureRegistrationAllowed();

         session()->put('social-callback', 'register');
         return $this->socialAuthService->startRegister($socialDriver);
     }
         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);
 
         // 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) {
             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);
         }
 
             return $this->socialRegisterCallback($socialDriver, $socialUser);
         }
 


@@ -108,7 +108,6 @@ class SocialController extends Controller
 
     /**
      * Register a new user after a registration callback.
 
     /**
      * Register a new user after a registration callback.

-     * @return RedirectResponse|Redirector

      * @throws UserRegistrationException
      */
     protected function socialRegisterCallback(string $socialDriver, SocialUser $socialUser)
      * @throws UserRegistrationException
      */
     protected function socialRegisterCallback(string $socialDriver, SocialUser $socialUser)


@@ -121,17 +120,11 @@ class SocialController extends Controller
         $userData = [
             'name' => $socialUser->getName(),
             'email' => $socialUser->getEmail(),
         $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('/');
 
         $this->showSuccessNotification(trans('auth.register_success'));
         return redirect('/');

diff --git a/app/Providers/AuthServiceProvider.php b/app/Providers/AuthServiceProvider.php
index a885628f3ac3114b1ff563cfb39625ac3044389b..fe52df1686cec7ac00bcd82ba14a1c74b0b5f3dd 100644 (file)
--- 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\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;
 
 use BookStack\Auth\UserRepo;
 use Illuminate\Support\ServiceProvider;
 


@@ -31,7 +32,7 @@ class AuthServiceProvider extends ServiceProvider
                 $provider,
                 $this->app['session.store'],
                 $app[LdapService::class],
                 $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'],
                 $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 bb7b6148c9e5c14eb6ba42862e175c58bd512047..4752d8b0c0ffaa532c74d4610ec24aad4c6ccbeb 100644 (file)
--- 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_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.',
     '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 8c9675f095b6535f900176ed7bdc3b13f9d506b8..692489afd57a371a34d5af5bbcbfc6399e2a1a0b 100755 (executable)
--- 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_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.',
     '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 94605da6f11029c946c25032949a14ffea971ba2..2585ec5e33238d213c84474c70639ee7b53d0705 100644 (file)
--- a/resources/views/settings/index.blade.php
+++ b/resources/views/settings/index.blade.php
@@ -219,8 +219,8 @@
                                 'label' => trans('settings.reg_enable_toggle')
                             ])
 
                                 '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>
                             @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 92ff4a829bb40e8ea3c8e65c28cf6d1d5637775b..cb1194e22bc3a5c8515ce7acd19c80cba01fa6c1 100644 (file)
--- 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]);
     }
 
             ->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 = '[email protected]';
+
+        $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);
     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 b3e6bd41b94aae6a95d31447b7e03c20f86bd161..9a3d6d8ecabb27acedb25957d2bef56920913009 100644 (file)
--- a/tests/Auth/Saml2Test.php
+++ b/tests/Auth/Saml2Test.php
@@ -73,7 +73,7 @@ class Saml2Test extends TestCase
             $this->assertDatabaseHas('users', [
                 'email' => '[email protected]',
                 'external_auth_id' => 'user',
             $this->assertDatabaseHas('users', [
                 'email' => '[email protected]',
                 'external_auth_id' => 'user',

-                'email_confirmed' => true,
+                'email_confirmed' => false,

                 'name' => 'Barry Scott'
             ]);
 
                 'name' => 'Barry Scott'
             ]);
 


@@ -209,7 +209,7 @@ class Saml2Test extends TestCase
             $acsPost = $this->post('/saml2/acs');
             $acsPost->assertRedirect('/');
             $errorMessage = session()->get('error');
             $acsPost = $this->post('/saml2/acs');
             $acsPost->assertRedirect('/');
             $errorMessage = session()->get('error');

-            $this->assertEquals('Registration unsuccessful since a user already exists with email address "[email protected]"', $errorMessage);
+            $this->assertEquals('A user with the email [email protected] already exists but with different credentials.', $errorMessage);

         });
     }
 
         });
     }
 


@@ -271,6 +271,24 @@ class Saml2Test extends TestCase
         $this->assertPermissionError($resp);
     }
 
         $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' => '[email protected]']);
+        });
+    }
+

     protected function withGet(array $options, callable $callback)
     {
         return $this->withGlobal($_GET, $options, $callback);
     protected function withGet(array $options, callable $callback)
     {
         return $this->withGlobal($_GET, $options, $callback);