From c5d173f45c3e7aacb33e1e5e36f27d08ac0b3b9b Mon Sep 17 00:00:00 2001 From: Bubka <858858+Bubka@users.noreply.github.com> Date: Thu, 14 Dec 2023 15:39:14 +0100 Subject: [PATCH] Fix user registration via SSO with existing email and name --- .../Controllers/Auth/SocialiteController.php | 15 +++-- resources/lang/en/errors.php | 3 +- .../Http/Auth/SocialiteControllerTest.php | 57 +++++++++++++++++-- 3 files changed, 66 insertions(+), 9 deletions(-) diff --git a/app/Http/Controllers/Auth/SocialiteController.php b/app/Http/Controllers/Auth/SocialiteController.php index a6705c2a..cf446fdc 100644 --- a/app/Http/Controllers/Auth/SocialiteController.php +++ b/app/Http/Controllers/Auth/SocialiteController.php @@ -42,6 +42,10 @@ public function callback(Request $request, string $driver) return redirect('/error?err=sso_failed'); } + $uniqueName = $socialiteUser->getId() . '@' . $driver; + $socialiteEmail = $socialiteUser->getEmail() ?? $uniqueName; + $socialiteName = ($socialiteUser->getNickname() ?? $socialiteUser->getName()) . ' (' . $uniqueName . ')'; + /** @var User|null $user */ $user = User::firstOrNew([ 'oauth_id' => $socialiteUser->getId(), @@ -49,17 +53,20 @@ public function callback(Request $request, string $driver) ]); if (! $user->exists) { - if (User::count() === 0) { + if (User::where('email', $socialiteEmail)->exists()) { + return redirect('/error?err=sso_email_already_used'); + } + else if (User::count() === 0) { $user->is_admin = true; } else if (Settings::get('disableRegistration')) { - return redirect('/error?err=no_register'); + return redirect('/error?err=sso_no_register'); } $user->password = bcrypt(Str::random()); } - $user->email = $socialiteUser->getEmail() ?? $socialiteUser->getId() . '@' . $driver; - $user->name = $socialiteUser->getNickname() ?? $socialiteUser->getName() ?? $driver . ' #' . $socialiteUser->getId(); + $user->email = $socialiteEmail; + $user->name = $socialiteName; $user->last_seen_at = Carbon::now()->format('Y-m-d H:i:s'); $user->save(); diff --git a/resources/lang/en/errors.php b/resources/lang/en/errors.php index e0edd203..907dd4e1 100644 --- a/resources/lang/en/errors.php +++ b/resources/lang/en/errors.php @@ -62,6 +62,7 @@ 'sso_disabled' => 'SSO is disabled', 'sso_bad_provider_setup' => 'This SSO provider is not fully setup in your .env file', 'sso_failed' => 'Authentication via SSO rejected', - 'no_register' => 'Registrations are disabled', + 'sso_no_register' => 'Registrations are disabled', + 'sso_email_already_used' => 'A user account with the same email address already exists but it does not match your external account ID. Do not use SSO if you are already registered on 2FAuth with this email.', 'account_managed_by_external_provider' => 'Account managed by an external provider', ]; \ No newline at end of file diff --git a/tests/Feature/Http/Auth/SocialiteControllerTest.php b/tests/Feature/Http/Auth/SocialiteControllerTest.php index 31aff07d..f21c3603 100644 --- a/tests/Feature/Http/Auth/SocialiteControllerTest.php +++ b/tests/Feature/Http/Auth/SocialiteControllerTest.php @@ -129,7 +129,6 @@ public function test_callback_updates_user_informations() $this->assertDatabaseHas('users', [ 'oauth_id' => self::USER_OAUTH_ID, 'oauth_provider' => self::USER_OAUTH_PROVIDER, - 'name' => 'new_nickname', 'email' => 'new_email', ]); } @@ -152,7 +151,6 @@ public function test_callback_updates_username_with_fallback_value() $this->assertDatabaseHas('users', [ 'oauth_id' => self::USER_OAUTH_ID, 'oauth_provider' => self::USER_OAUTH_PROVIDER, - 'name' => 'new_name', 'email' => 'new_email', ]); } @@ -175,12 +173,34 @@ public function test_callback_registers_new_user() $this->assertDatabaseHas('users', [ 'oauth_id' => 'new_id', 'oauth_provider' => self::USER_OAUTH_PROVIDER, - 'name' => 'jane', 'email' => 'jane@provider.com', 'is_admin' => 0, ]); } + /** + * @test + */ + public function test_callback_registers_new_user_with_existing_name() + { + $socialiteUserWithSameName = new \Laravel\Socialite\Two\User; + $socialiteUserWithSameName->id = 'socialiteUserWithSameNameId'; + $socialiteUserWithSameName->name = self::USER_NAME; + $socialiteUserWithSameName->email = 'socialiteuserwithsamename@example.com'; + $socialiteUserWithSameName->nickname = self::USER_NICKNAME; + + Socialite::shouldReceive('driver->user') + ->andReturn($socialiteUserWithSameName); + + $response = $this->get('/socialite/callback/github', ['driver' => 'github']); + + $this->assertDatabaseHas('users', [ + 'oauth_id' => 'socialiteUserWithSameNameId', + 'oauth_provider' => self::USER_OAUTH_PROVIDER, + 'email' => 'socialiteuserwithsamename@example.com', + ]); + } + /** * @test */ @@ -202,6 +222,35 @@ public function test_callback_always_registers_first_user_as_admin() ]); } + /** + * @test + */ + public function test_callback_returns_error_when_email_is_already_used() + { + $userWithSameEmail = User::factory()->create([ + 'name' => 'userWithSameEmail', + 'email' => 'other@example.com', + 'password' => 'password', + ]); + + $socialiteUserWithSameEmail = new \Laravel\Socialite\Two\User; + $socialiteUserWithSameEmail->id = '666'; + $socialiteUserWithSameEmail->name = 'socialiteUserWithSameEmail'; + $socialiteUserWithSameEmail->email = 'other@example.com'; + $socialiteUserWithSameEmail->nickname = self::USER_NICKNAME; + + Socialite::shouldReceive('driver->user') + ->andReturn($socialiteUserWithSameEmail); + + $response = $this->get('/socialite/callback/github', ['driver' => 'github']); + + $response->assertRedirect('/error?err=sso_email_already_used'); + $this->assertDatabaseMissing('users', [ + 'oauth_id' => '666', + 'oauth_provider' => self::USER_OAUTH_PROVIDER, + ]); + } + /** * @test */ @@ -219,7 +268,7 @@ public function test_callback_returns_error_when_registrations_are_closed() $response = $this->get('/socialite/callback/github', ['driver' => 'github']); - $response->assertRedirect('/error?err=no_register'); + $response->assertRedirect('/error?err=sso_no_register'); } /**