From e60d11ee04161d16875d706110876c31f9064565 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Fri, 21 Sep 2018 18:05:06 +0100 Subject: [PATCH] Altered social auto-reg to be configurable per service - Added {$service}_AUTO_REGISTER and {$service}_AUTO_CONFIRM_EMAIL env options for each social auth system. - Auto-register will allow registration from login, even if registration is disabled. - Auto-confirm-email indicates trust and will mark new registrants as 'email_confirmed' and skip 'confirmation email' flow. - Also added covering tests. --- app/Exceptions/SocialSignInAccountNotUsed.php | 6 ++ .../Controllers/Auth/RegisterController.php | 44 +++++--- app/Repos/UserRepo.php | 12 +-- app/Services/SocialAuthService.php | 101 ++++++++--------- config/services.php | 21 ++++ phpunit.xml | 4 + resources/lang/en/settings.php | 2 - resources/views/settings/index.blade.php | 5 - tests/Auth/SocialAuthTest.php | 102 +++++++++++++++--- 9 files changed, 200 insertions(+), 97 deletions(-) create mode 100644 app/Exceptions/SocialSignInAccountNotUsed.php diff --git a/app/Exceptions/SocialSignInAccountNotUsed.php b/app/Exceptions/SocialSignInAccountNotUsed.php new file mode 100644 index 000000000..7eaa72bd5 --- /dev/null +++ b/app/Exceptions/SocialSignInAccountNotUsed.php @@ -0,0 +1,6 @@ +userRepo->registerNew($userData); + $newUser = $this->userRepo->registerNew($userData, $emailVerified); if ($socialAccount) { $newUser->socialAccounts()->save($socialAccount); } - if (setting('registration-confirmation') || setting('registration-restrict')) { + if ((setting('registration-confirmation') || $registrationRestrict) && !$emailVerified) { $newUser->save(); try { @@ -250,7 +254,6 @@ class RegisterController extends Controller * @throws SocialSignInException * @throws UserRegistrationException * @throws \BookStack\Exceptions\SocialDriverNotConfigured - * @throws ConfirmationEmailException */ public function socialCallback($socialDriver, Request $request) { @@ -267,12 +270,24 @@ class RegisterController extends Controller } $action = session()->pull('social-callback'); + + // Attempt login or fall-back to register if allowed. + $socialUser = $this->socialAuthService->getSocialUser($socialDriver); if ($action == 'login') { - return $this->socialAuthService->handleLoginCallback($socialDriver); + try { + return $this->socialAuthService->handleLoginCallback($socialDriver, $socialUser); + } catch (SocialSignInAccountNotUsed $exception) { + if ($this->socialAuthService->driverAutoRegisterEnabled($socialDriver)) { + return $this->socialRegisterCallback($socialDriver, $socialUser); + } + throw $exception; + } } + if ($action == 'register') { - return $this->socialRegisterCallback($socialDriver); + return $this->socialRegisterCallback($socialDriver, $socialUser); } + return redirect()->back(); } @@ -288,15 +303,16 @@ class RegisterController extends Controller /** * Register a new user after a registration callback. - * @param $socialDriver + * @param string $socialDriver + * @param SocialUser $socialUser * @return \Illuminate\Http\RedirectResponse|\Illuminate\Routing\Redirector * @throws UserRegistrationException - * @throws \BookStack\Exceptions\SocialDriverNotConfigured */ - protected function socialRegisterCallback($socialDriver) + protected function socialRegisterCallback(string $socialDriver, SocialUser $socialUser) { - $socialUser = $this->socialAuthService->handleRegistrationCallback($socialDriver); + $socialUser = $this->socialAuthService->handleRegistrationCallback($socialDriver, $socialUser); $socialAccount = $this->socialAuthService->fillSocialAccount($socialDriver, $socialUser); + $emailVerified = $this->socialAuthService->driverAutoConfirmEmailEnabled($socialDriver); // Create an array of the user data to create a new user instance $userData = [ @@ -304,6 +320,6 @@ class RegisterController extends Controller 'email' => $socialUser->getEmail(), 'password' => str_random(30) ]; - return $this->registerUser($userData, $socialAccount); + return $this->registerUser($userData, $socialAccount, $emailVerified); } } diff --git a/app/Repos/UserRepo.php b/app/Repos/UserRepo.php index 6defe8aa5..6fe8d2619 100644 --- a/app/Repos/UserRepo.php +++ b/app/Repos/UserRepo.php @@ -79,12 +79,12 @@ class UserRepo /** * Creates a new user and attaches a role to them. * @param array $data - * @param boolean autoVerifyEmail + * @param boolean $verifyEmail * @return User */ - public function registerNew(array $data, $autoVerifyEmail=false) + public function registerNew(array $data, $verifyEmail = false) { - $user = $this->create($data, $autoVerifyEmail); + $user = $this->create($data, $verifyEmail); $this->attachDefaultRole($user); // Get avatar from gravatar and save @@ -142,17 +142,17 @@ class UserRepo /** * Create a new basic instance of user. * @param array $data - * @param boolean $autoVerifyEmail + * @param boolean $verifyEmail * @return User */ - public function create(array $data, $autoVerifyEmail=false) + public function create(array $data, $verifyEmail = false) { return $this->user->forceCreate([ 'name' => $data['name'], 'email' => $data['email'], 'password' => bcrypt($data['password']), - 'email_confirmed' => $autoVerifyEmail + 'email_confirmed' => $verifyEmail ]); } diff --git a/app/Services/SocialAuthService.php b/app/Services/SocialAuthService.php index 6b4c221b0..9017dfebe 100644 --- a/app/Services/SocialAuthService.php +++ b/app/Services/SocialAuthService.php @@ -1,13 +1,12 @@ validateDriver($socialDriver); - - // Get user details from social driver - $socialUser = $this->socialite->driver($driver)->user(); - // Check social account has not already been used if ($this->socialAccount->where('driver_id', '=', $socialUser->getId())->exists()) { throw new UserRegistrationException(trans('errors.social_account_in_use', ['socialAccount'=>$socialDriver]), '/login'); @@ -84,17 +78,26 @@ class SocialAuthService } /** - * Handle the login process on a oAuth callback. - * @param $socialDriver - * @return \Illuminate\Http\RedirectResponse|\Illuminate\Routing\Redirector + * Get the social user details via the social driver. + * @param string $socialDriver + * @return SocialUser * @throws SocialDriverNotConfigured - * @throws SocialSignInException */ - public function handleLoginCallback($socialDriver) + public function getSocialUser(string $socialDriver) { $driver = $this->validateDriver($socialDriver); - // Get user details from social driver - $socialUser = $this->socialite->driver($driver)->user(); + return $this->socialite->driver($driver)->user(); + } + + /** + * Handle the login process on a oAuth callback. + * @param $socialDriver + * @param SocialUser $socialUser + * @return \Illuminate\Http\RedirectResponse|\Illuminate\Routing\Redirector + * @throws SocialSignInAccountNotUsed + */ + public function handleLoginCallback($socialDriver, SocialUser $socialUser) + { $socialId = $socialUser->getId(); // Get any attached social accounts or users @@ -109,40 +112,6 @@ class SocialAuthService return redirect()->intended('/'); } - // When a user is not logged in and no matching SocialAccount exists, - // If the auto social registration is enabled, attach the social account, create new user and log him in. - if (!$isLoggedIn && $socialAccount === null && setting('autosocialregistration-confirmation')) { - - // Fill social account - $socialAccount = $this->fillSocialAccount($socialDriver, $socialUser); - - // Create an array of the user data to create a new user instance - $userData = [ - 'name' => $socialUser->getName(), - 'email' => $socialUser->getEmail(), - 'password' => str_random(30) - ]; - - // Check domain if domain restriction setting is set - if (setting('registration-restrict')) { - $restrictedEmailDomains = explode(',', str_replace(' ', '', setting('registration-restrict'))); - $userEmailDomain = $domain = substr(strrchr($socialUser->getEmail(), "@"), 1); - if (!in_array($userEmailDomain, $restrictedEmailDomains)) { - throw new SocialSignInException(trans('auth.registration_email_domain_invalid'), '/login'); - } - } - - // Register new user with autoVerifyEmail set to true and attach the social account - $newUser = $this->userRepo->registerNew($userData, true); - $newUser->socialAccounts()->save($socialAccount); - $newUser->save(); - - // Log him in - auth()->login($newUser); - - return redirect()->intended('/'); - } - // When a user is logged in but the social account does not exist, // Create the social account and attach it to the user & redirect to the profile page. if ($isLoggedIn && $socialAccount === null) { @@ -170,7 +139,7 @@ class SocialAuthService $message .= trans('errors.social_account_register_instructions', ['socialAccount' => title_case($socialDriver)]); } - throw new SocialSignInException($message, '/login'); + throw new SocialSignInAccountNotUsed($message, '/login'); } /** @@ -233,8 +202,28 @@ class SocialAuthService } /** - * @param string $socialDriver - * @param \Laravel\Socialite\Contracts\User $socialUser + * Check if the current config for the given driver allows auto-registration. + * @param string $driver + * @return bool + */ + public function driverAutoRegisterEnabled(string $driver) + { + return config('services.' . strtolower($driver) . '.auto_register') === true; + } + + /** + * Check if the current config for the given driver allow email address auto-confirmation. + * @param string $driver + * @return bool + */ + public function driverAutoConfirmEmailEnabled(string $driver) + { + return config('services.' . strtolower($driver) . '.auto_confirm') === true; + } + + /** + * @param string $socialDriver + * @param SocialUser $socialUser * @return SocialAccount */ public function fillSocialAccount($socialDriver, $socialUser) diff --git a/config/services.php b/config/services.php index fab0c1d75..2b0f260cd 100644 --- a/config/services.php +++ b/config/services.php @@ -48,6 +48,8 @@ return [ 'client_secret' => env('GITHUB_APP_SECRET', false), 'redirect' => env('APP_URL') . '/login/service/github/callback', 'name' => 'GitHub', + 'auto_register' => env('GITHUB_AUTO_REGISTER', false), + 'auto_confirm' => env('GITHUB_AUTO_CONFIRM_EMAIL', false), ], 'google' => [ @@ -55,6 +57,8 @@ return [ 'client_secret' => env('GOOGLE_APP_SECRET', false), 'redirect' => env('APP_URL') . '/login/service/google/callback', 'name' => 'Google', + 'auto_register' => env('GOOGLE_AUTO_REGISTER', false), + 'auto_confirm' => env('GOOGLE_AUTO_CONFIRM_EMAIL', false), ], 'slack' => [ @@ -62,6 +66,8 @@ return [ 'client_secret' => env('SLACK_APP_SECRET', false), 'redirect' => env('APP_URL') . '/login/service/slack/callback', 'name' => 'Slack', + 'auto_register' => env('SLACK_AUTO_REGISTER', false), + 'auto_confirm' => env('SLACK_AUTO_CONFIRM_EMAIL', false), ], 'facebook' => [ @@ -69,6 +75,8 @@ return [ 'client_secret' => env('FACEBOOK_APP_SECRET', false), 'redirect' => env('APP_URL') . '/login/service/facebook/callback', 'name' => 'Facebook', + 'auto_register' => env('FACEBOOK_AUTO_REGISTER', false), + 'auto_confirm' => env('FACEBOOK_AUTO_CONFIRM_EMAIL', false), ], 'twitter' => [ @@ -76,6 +84,8 @@ return [ 'client_secret' => env('TWITTER_APP_SECRET', false), 'redirect' => env('APP_URL') . '/login/service/twitter/callback', 'name' => 'Twitter', + 'auto_register' => env('TWITTER_AUTO_REGISTER', false), + 'auto_confirm' => env('TWITTER_AUTO_CONFIRM_EMAIL', false), ], 'azure' => [ @@ -84,6 +94,8 @@ return [ 'tenant' => env('AZURE_TENANT', false), 'redirect' => env('APP_URL') . '/login/service/azure/callback', 'name' => 'Microsoft Azure', + 'auto_register' => env('AZURE_AUTO_REGISTER', false), + 'auto_confirm' => env('AZURE_AUTO_CONFIRM_EMAIL', false), ], 'okta' => [ @@ -92,6 +104,8 @@ return [ 'redirect' => env('APP_URL') . '/login/service/okta/callback', 'base_url' => env('OKTA_BASE_URL'), 'name' => 'Okta', + 'auto_register' => env('OKTA_AUTO_REGISTER', false), + 'auto_confirm' => env('OKTA_AUTO_CONFIRM_EMAIL', false), ], 'gitlab' => [ @@ -100,6 +114,8 @@ return [ 'redirect' => env('APP_URL') . '/login/service/gitlab/callback', 'instance_uri' => env('GITLAB_BASE_URI'), // Needed only for self hosted instances 'name' => 'GitLab', + 'auto_register' => env('GITLAB_AUTO_REGISTER', false), + 'auto_confirm' => env('GITLAB_AUTO_CONFIRM_EMAIL', false), ], 'twitch' => [ @@ -107,12 +123,17 @@ return [ 'client_secret' => env('TWITCH_APP_SECRET'), 'redirect' => env('APP_URL') . '/login/service/twitch/callback', 'name' => 'Twitch', + 'auto_register' => env('TWITCH_AUTO_REGISTER', false), + 'auto_confirm' => env('TWITCH_AUTO_CONFIRM_EMAIL', false), ], + 'discord' => [ 'client_id' => env('DISCORD_APP_ID'), 'client_secret' => env('DISCORD_APP_SECRET'), 'redirect' => env('APP_URL') . '/login/service/discord/callback', 'name' => 'Discord', + 'auto_register' => env('DISCORD_AUTO_REGISTER', false), + 'auto_confirm' => env('DISCORD_AUTO_CONFIRM_EMAIL', false), ], 'ldap' => [ diff --git a/phpunit.xml b/phpunit.xml index 9434b710f..4c1e4f66c 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -35,8 +35,12 @@ + + + + diff --git a/resources/lang/en/settings.php b/resources/lang/en/settings.php index 7e33c73dd..839199b1f 100755 --- a/resources/lang/en/settings.php +++ b/resources/lang/en/settings.php @@ -43,8 +43,6 @@ return [ 'reg_settings' => 'Registration Settings', 'reg_allow' => 'Allow registration?', - 'reg_auto_social_allow' => 'Allow auto social registration?', - 'reg_auto_social_allow_desc' => 'If the social user doesn\'t exist, automatically sign him up. Domain restriction is respected if set. Email is also automatically validated for this kind of social registration.', 'reg_default_role' => 'Default user role after registration', 'reg_confirm_email' => 'Require email confirmation?', 'reg_confirm_email_desc' => 'If domain restriction is used then email confirmation will be required and the below value will be ignored.', diff --git a/resources/views/settings/index.blade.php b/resources/views/settings/index.blade.php index 8ad27c5f3..3c563a61c 100644 --- a/resources/views/settings/index.blade.php +++ b/resources/views/settings/index.blade.php @@ -136,11 +136,6 @@ @endforeach -
- -

{{ trans('settings.reg_auto_social_allow_desc') }}

- @include('components.toggle-switch', ['name' => 'setting-autosocialregistration-confirmation', 'value' => setting('autosocialregistration-confirmation')]) -

{{ trans('settings.reg_confirm_email_desc') }}

diff --git a/tests/Auth/SocialAuthTest.php b/tests/Auth/SocialAuthTest.php index e3494d073..5bfe0c222 100644 --- a/tests/Auth/SocialAuthTest.php +++ b/tests/Auth/SocialAuthTest.php @@ -1,6 +1,6 @@ shouldReceive('getName')->once()->andReturn($user->name); $mockSocialUser->shouldReceive('getAvatar')->once()->andReturn('avatar_placeholder'); - $this->visit('/register/service/google'); - $this->visit('/login/service/google/callback'); - $this->seeInDatabase('users', ['name' => $user->name, 'email' => $user->email]); + $this->get('/register/service/google'); + $this->get('/login/service/google/callback'); + $this->assertDatabaseHas('users', ['name' => $user->name, 'email' => $user->email]); $user = $user->whereEmail($user->email)->first(); - $this->seeInDatabase('social_accounts', ['user_id' => $user->id]); + $this->assertDatabaseHas('social_accounts', ['user_id' => $user->id]); } public function test_social_login() @@ -53,17 +53,21 @@ class SocialAuthTest extends BrowserKitTest $mockSocialDriver->shouldReceive('redirect')->twice()->andReturn(redirect('/')); // Test login routes - $this->visit('/login')->seeElement('#social-login-google') - ->click('#social-login-google') - ->seePageIs('/login'); + $resp = $this->get('/login'); + $resp->assertElementExists('a#social-login-google[href$="/login/service/google"]'); + $resp = $this->followingRedirects()->get("/login/service/google"); + $resp->assertSee('login-form'); // Test social callback - $this->visit('/login/service/google/callback')->seePageIs('/login') - ->see(trans('errors.social_account_not_used', ['socialAccount' => 'Google'])); + $resp = $this->followingRedirects()->get('/login/service/google/callback'); + $resp->assertSee('login-form'); + $resp->assertSee(trans('errors.social_account_not_used', ['socialAccount' => 'Google'])); + + $resp = $this->get('/login'); + $resp->assertElementExists('a#social-login-github[href$="/login/service/github"]'); + $resp = $this->followingRedirects()->get("/login/service/github"); + $resp->assertSee('login-form'); - $this->visit('/login')->seeElement('#social-login-github') - ->click('#social-login-github') - ->seePageIs('/login'); // Test social callback with matching social account \DB::table('social_accounts')->insert([ @@ -71,7 +75,77 @@ class SocialAuthTest extends BrowserKitTest 'driver' => 'github', 'driver_id' => 'logintest123' ]); - $this->visit('/login/service/github/callback')->seePageIs('/'); + $resp = $this->followingRedirects()->get('/login/service/github/callback'); + $resp->assertDontSee("login-form"); + } + + public function test_social_autoregister() + { + config([ + 'services.google.client_id' => 'abc123', 'services.google.client_secret' => '123abc', + 'APP_URL' => 'http://localhost' + ]); + + $user = factory(\BookStack\User::class)->make(); + $mockSocialite = \Mockery::mock('Laravel\Socialite\Contracts\Factory'); + $this->app['Laravel\Socialite\Contracts\Factory'] = $mockSocialite; + $mockSocialDriver = \Mockery::mock('Laravel\Socialite\Contracts\Provider'); + $mockSocialUser = \Mockery::mock('\Laravel\Socialite\Contracts\User'); + + $mockSocialUser->shouldReceive('getId')->times(4)->andReturn(1); + $mockSocialUser->shouldReceive('getEmail')->times(2)->andReturn($user->email); + $mockSocialUser->shouldReceive('getName')->once()->andReturn($user->name); + $mockSocialUser->shouldReceive('getAvatar')->once()->andReturn('avatar_placeholder'); + + $mockSocialDriver->shouldReceive('user')->times(2)->andReturn($mockSocialUser); + $mockSocialite->shouldReceive('driver')->times(4)->with('google')->andReturn($mockSocialDriver); + $mockSocialDriver->shouldReceive('redirect')->twice()->andReturn(redirect('/')); + + $googleAccountNotUsedMessage = trans('errors.social_account_not_used', ['socialAccount' => 'Google']); + + $this->get('/login/service/google'); + $resp = $this->followingRedirects()->get('/login/service/google/callback'); + $resp->assertSee($googleAccountNotUsedMessage); + + config(['services.google.auto_register' => true]); + + $this->get('/login/service/google'); + $resp = $this->followingRedirects()->get('/login/service/google/callback'); + $resp->assertDontSee($googleAccountNotUsedMessage); + + $this->assertDatabaseHas('users', ['name' => $user->name, 'email' => $user->email, 'email_confirmed' => false]); + $user = $user->whereEmail($user->email)->first(); + $this->assertDatabaseHas('social_accounts', ['user_id' => $user->id]); + } + + public function test_social_auto_email_confirm() + { + config([ + 'services.google.client_id' => 'abc123', 'services.google.client_secret' => '123abc', + 'APP_URL' => 'http://localhost', 'services.google.auto_register' => true, 'services.google.auto_confirm' => true + ]); + + $user = factory(\BookStack\User::class)->make(); + $mockSocialite = \Mockery::mock('Laravel\Socialite\Contracts\Factory'); + $this->app['Laravel\Socialite\Contracts\Factory'] = $mockSocialite; + $mockSocialDriver = \Mockery::mock('Laravel\Socialite\Contracts\Provider'); + $mockSocialUser = \Mockery::mock('\Laravel\Socialite\Contracts\User'); + + $mockSocialUser->shouldReceive('getId')->times(3)->andReturn(1); + $mockSocialUser->shouldReceive('getEmail')->times(2)->andReturn($user->email); + $mockSocialUser->shouldReceive('getName')->once()->andReturn($user->name); + $mockSocialUser->shouldReceive('getAvatar')->once()->andReturn('avatar_placeholder'); + + $mockSocialDriver->shouldReceive('user')->times(1)->andReturn($mockSocialUser); + $mockSocialite->shouldReceive('driver')->times(2)->with('google')->andReturn($mockSocialDriver); + $mockSocialDriver->shouldReceive('redirect')->once()->andReturn(redirect('/')); + + $this->get('/login/service/google'); + $this->get('/login/service/google/callback'); + + $this->assertDatabaseHas('users', ['name' => $user->name, 'email' => $user->email, 'email_confirmed' => true]); + $user = $user->whereEmail($user->email)->first(); + $this->assertDatabaseHas('social_accounts', ['user_id' => $user->id]); } }