From 95ddbf9ead9f6848436cdd5add256eb95286781a Mon Sep 17 00:00:00 2001 From: Gregor Vostrak Date: Wed, 16 Sep 2026 15:14:12 +0200 Subject: [PATCH] fix placeholder users being resolved by authentication flows --- app/Auth/ActiveUserProvider.php | 37 ++++++++ app/Models/User.php | 7 +- app/Providers/AuthServiceProvider.php | 10 +++ app/Service/MemberService.php | 11 ++- ...ove_credentials_from_placeholder_users.php | 53 +++++++++++ tests/Feature/PasswordConfirmationTest.php | 40 +++++++++ tests/Feature/PasswordResetTest.php | 59 ++++++++++++ tests/Unit/Auth/ActiveUserProviderTest.php | 90 +++++++++++++++++++ .../OrganizationInvitationEndpointTest.php | 3 + tests/Unit/Model/UserModelTest.php | 17 ++++ tests/Unit/Service/DeletionServiceTest.php | 5 +- tests/Unit/Service/MemberServiceTest.php | 43 +++++++++ 12 files changed, 371 insertions(+), 4 deletions(-) create mode 100644 app/Auth/ActiveUserProvider.php create mode 100644 database/migrations/2026_09_16_000001_remove_credentials_from_placeholder_users.php create mode 100644 tests/Unit/Auth/ActiveUserProviderTest.php diff --git a/app/Auth/ActiveUserProvider.php b/app/Auth/ActiveUserProvider.php new file mode 100644 index 00000000..4c72656c --- /dev/null +++ b/app/Auth/ActiveUserProvider.php @@ -0,0 +1,37 @@ + + */ + #[\Override] + protected function newModelQuery($model = null): Builder + { + $query = parent::newModelQuery($model); + $query->getQuery()->where('is_placeholder', '=', false); + + return $query; + } +} diff --git a/app/Models/User.php b/app/Models/User.php index 5d7fc298..9357dba9 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -38,7 +38,10 @@ use OwenIt\Auditing\Contracts\Auditable as AuditableContract; * @property string|null $pending_email * @property Carbon|null $email_verified_at * @property string|null $password + * @property string|null $remember_token * @property string|null $two_factor_secret + * @property string|null $two_factor_recovery_codes + * @property Carbon|null $two_factor_confirmed_at * @property string $timezone * @property bool $is_placeholder * @property Weekday $week_start @@ -150,7 +153,9 @@ class User extends Authenticatable implements AuditableContract, FilamentUser, M public function canAccessPanel(Panel $panel): bool { - return in_array($this->email, config('auth.super_admins', []), true) && $this->hasVerifiedEmail(); + return $this->is_placeholder === false + && in_array($this->email, config('auth.super_admins', []), true) + && $this->hasVerifiedEmail(); } public function isMemberOfOrganization(Organization $organization): bool diff --git a/app/Providers/AuthServiceProvider.php b/app/Providers/AuthServiceProvider.php index cb15494c..0ae25165 100644 --- a/app/Providers/AuthServiceProvider.php +++ b/app/Providers/AuthServiceProvider.php @@ -4,11 +4,14 @@ declare(strict_types=1); namespace App\Providers; +use App\Auth\ActiveUserProvider; use App\Models\Passport\AuthCode; use App\Models\Passport\Client; use App\Models\Passport\RefreshToken; use App\Models\Passport\Token; +use Illuminate\Contracts\Foundation\Application; use Illuminate\Foundation\Support\Providers\AuthServiceProvider as ServiceProvider; +use Illuminate\Support\Facades\Auth; use Laravel\Passport\Passport; class AuthServiceProvider extends ServiceProvider @@ -26,6 +29,13 @@ class AuthServiceProvider extends ServiceProvider */ public function boot(): void { + // Replaces the built-in eloquent user provider, so that no authentication flow can + // resolve a placeholder user. The driver name is kept, because Passport recognizes + // only providers that are configured with the driver "eloquent". + Auth::provider('eloquent', function (Application $app, array $config): ActiveUserProvider { + return new ActiveUserProvider($app->make('hash'), $config['model']); + }); + // define scopes for passport tokens Passport::tokensCan([ 'create' => 'Create resources', diff --git a/app/Service/MemberService.php b/app/Service/MemberService.php index 68e2a516..dc2df481 100644 --- a/app/Service/MemberService.php +++ b/app/Service/MemberService.php @@ -218,7 +218,16 @@ class MemberService $placeholderUser = $user->replicate(); $placeholderUser->is_placeholder = true; - $placeholderUser->current_team_id = $member->organization_id; + // Reset authentication relevant properties on the placeholder user + $placeholderUser->password = null; + $placeholderUser->remember_token = null; + $placeholderUser->two_factor_secret = null; + $placeholderUser->two_factor_recovery_codes = null; + $placeholderUser->two_factor_confirmed_at = null; + $placeholderUser->email_verified_at = null; + $placeholderUser->pending_email = null; + $placeholderUser->current_team_id = null; + $placeholderUser->profile_photo_path = null; $placeholderUser->save(); $member->user()->associate($placeholderUser); diff --git a/database/migrations/2026_09_16_000001_remove_credentials_from_placeholder_users.php b/database/migrations/2026_09_16_000001_remove_credentials_from_placeholder_users.php new file mode 100644 index 00000000..4a4a3e17 --- /dev/null +++ b/database/migrations/2026_09_16_000001_remove_credentials_from_placeholder_users.php @@ -0,0 +1,53 @@ +where('is_placeholder', '=', true) + ->where(function (Builder $builder): void { + $builder->whereNotNull('password') + ->orWhereNotNull('remember_token') + ->orWhereNotNull('two_factor_secret') + ->orWhereNotNull('two_factor_recovery_codes') + ->orWhereNotNull('two_factor_confirmed_at') + ->orWhereNotNull('email_verified_at') + ->orWhereNotNull('pending_email') + ->orWhereNotNull('current_team_id') + ->orWhereNotNull('profile_photo_path'); + }) + ->update([ + 'password' => null, + 'remember_token' => null, + 'two_factor_secret' => null, + 'two_factor_recovery_codes' => null, + 'two_factor_confirmed_at' => null, + 'email_verified_at' => null, + 'pending_email' => null, + 'current_team_id' => null, + 'profile_photo_path' => null, + ]); + } + + /** + * Reverse the migrations. + */ + public function down(): void + { + // + } +}; diff --git a/tests/Feature/PasswordConfirmationTest.php b/tests/Feature/PasswordConfirmationTest.php index 2f9ba1ed..db7df729 100644 --- a/tests/Feature/PasswordConfirmationTest.php +++ b/tests/Feature/PasswordConfirmationTest.php @@ -6,6 +6,7 @@ namespace Tests\Feature; use App\Models\User; use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Support\Facades\Hash; use Tests\TestCase; class PasswordConfirmationTest extends TestCase @@ -43,4 +44,43 @@ class PasswordConfirmationTest extends TestCase $response->assertSessionHasErrors(); } + + public function test_password_can_be_confirmed_if_a_placeholder_user_with_the_same_email_exists(): void + { + // Arrange + // Placeholders created by an import have no password at all. The placeholder is created + // first so that it would be returned by an unordered lookup by email. + $email = 'shared@example.com'; + User::factory()->placeholder()->create(['email' => $email, 'password' => null]); + $user = User::factory()->create(['email' => $email, 'password' => Hash::make('secret-password')]); + + // Act + $response = $this->actingAs($user)->post('/user/confirm-password', [ + 'password' => 'secret-password', + ]); + + // Assert + $response->assertRedirect(); + $response->assertSessionHasNoErrors(); + $this->assertTrue($this->app['session']->has('auth.password_confirmed_at')); + } + + public function test_password_confirmation_ignores_the_password_of_a_placeholder_user_with_the_same_email(): void + { + // Arrange + // Placeholders created by removing a member copy the password hash as of the removal, + // so the placeholder holds a password that the real user has since replaced. + $email = 'shared@example.com'; + User::factory()->placeholder()->create(['email' => $email, 'password' => Hash::make('outdated-password')]); + $user = User::factory()->create(['email' => $email, 'password' => Hash::make('current-password')]); + + // Act + $response = $this->actingAs($user)->post('/user/confirm-password', [ + 'password' => 'outdated-password', + ]); + + // Assert + $response->assertSessionHasErrors(); + $this->assertFalse($this->app['session']->has('auth.password_confirmed_at')); + } } diff --git a/tests/Feature/PasswordResetTest.php b/tests/Feature/PasswordResetTest.php index ee0a579b..34d4a7d7 100644 --- a/tests/Feature/PasswordResetTest.php +++ b/tests/Feature/PasswordResetTest.php @@ -7,6 +7,7 @@ namespace Tests\Feature; use App\Models\User; use Illuminate\Auth\Notifications\ResetPassword; use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Support\Facades\Hash; use Illuminate\Support\Facades\Notification; use Laravel\Fortify\Features; use Tests\TestCase; @@ -93,4 +94,62 @@ class PasswordResetTest extends TestCase return true; }); } + + public function test_password_reset_targets_the_real_user_when_a_placeholder_user_with_the_same_email_exists(): void + { + + Notification::fake(); + + // The placeholder is created first so that it would be returned by an unordered lookup by email + $email = 'shared@example.com'; + $placeholder = User::factory()->placeholder()->create(['email' => $email]); + $user = User::factory()->create(['email' => $email]); + $placeholderPasswordBefore = $placeholder->password; + + $response = $this->post('/forgot-password', [ + 'email' => $email, + ]); + + $response->assertSessionHasNoErrors(); + Notification::assertNotSentTo($placeholder, ResetPassword::class); + Notification::assertSentTo($user, ResetPassword::class, function (ResetPassword $notification) use ($email) { + $response = $this->post('/reset-password', [ + 'token' => $notification->token, + 'email' => $email, + 'password' => 'new-password-123', + 'password_confirmation' => 'new-password-123', + ]); + + $response->assertSessionHasNoErrors(); + + return true; + }); + + $placeholder->refresh(); + $user->refresh(); + $this->assertSame($placeholderPasswordBefore, $placeholder->password); + $this->assertTrue(Hash::check('new-password-123', $user->password)); + + $response = $this->post('/login', [ + 'email' => $email, + 'password' => 'new-password-123', + ]); + + $response->assertSessionHasNoErrors(); + $this->assertAuthenticatedAs($user); + } + + public function test_password_reset_link_is_not_sent_if_only_a_placeholder_user_with_the_email_exists(): void + { + Notification::fake(); + + $placeholder = User::factory()->placeholder()->create(); + + $response = $this->post('/forgot-password', [ + 'email' => $placeholder->email, + ]); + + $response->assertSessionHasErrors('email'); + Notification::assertNothingSent(); + } } diff --git a/tests/Unit/Auth/ActiveUserProviderTest.php b/tests/Unit/Auth/ActiveUserProviderTest.php new file mode 100644 index 00000000..dc6da3be --- /dev/null +++ b/tests/Unit/Auth/ActiveUserProviderTest.php @@ -0,0 +1,90 @@ +assertInstanceOf(ActiveUserProvider::class, $brokerProvider); + } + + public function test_api_guard_uses_the_active_user_provider(): void + { + // Act + $guardProvider = Auth::createUserProvider(config('auth.guards.api.provider')); + + // Assert + $this->assertInstanceOf(ActiveUserProvider::class, $guardProvider); + } + + public function test_web_guard_uses_the_active_user_provider(): void + { + // Act + $guardProvider = Auth::createUserProvider(config('auth.guards.web.provider')); + + // Assert + $this->assertInstanceOf(ActiveUserProvider::class, $guardProvider); + } + + public function test_retrieve_by_credentials_ignores_placeholder_users_with_the_same_email(): void + { + // Arrange + $email = 'shared@example.com'; + $placeholder = User::factory()->placeholder()->create(['email' => $email]); + $user = User::factory()->create(['email' => $email]); + $provider = Auth::createUserProvider('users'); + + // Act + $result = $provider->retrieveByCredentials(['email' => $email]); + + // Assert + $this->assertInstanceOf(User::class, $result); + $this->assertTrue($user->is($result)); + $this->assertFalse($placeholder->is($result)); + } + + public function test_retrieve_by_credentials_returns_null_if_only_a_placeholder_user_exists(): void + { + // Arrange + $email = 'placeholder-only@example.com'; + User::factory()->placeholder()->create(['email' => $email]); + $provider = Auth::createUserProvider('users'); + + // Act + $result = $provider->retrieveByCredentials(['email' => $email]); + + // Assert + $this->assertNull($result); + } + + public function test_retrieve_by_id_returns_null_for_placeholder_users(): void + { + // Arrange + $placeholder = User::factory()->placeholder()->create(); + $user = User::factory()->create(); + $provider = Auth::createUserProvider('users'); + + // Act + $placeholderResult = $provider->retrieveById($placeholder->getKey()); + $userResult = $provider->retrieveById($user->getKey()); + + // Assert + $this->assertNull($placeholderResult); + $this->assertInstanceOf(User::class, $userResult); + $this->assertTrue($user->is($userResult)); + } +} diff --git a/tests/Unit/Endpoint/Web/OrganizationInvitationEndpointTest.php b/tests/Unit/Endpoint/Web/OrganizationInvitationEndpointTest.php index fc978fc6..2fc4e3c2 100644 --- a/tests/Unit/Endpoint/Web/OrganizationInvitationEndpointTest.php +++ b/tests/Unit/Endpoint/Web/OrganizationInvitationEndpointTest.php @@ -105,6 +105,9 @@ class OrganizationInvitationEndpointTest extends EndpointTestAbstract $this->assertDatabaseMissing(OrganizationInvitation::class, [ 'id' => $invitation->getKey(), ]); + // Joining sets the organization as the current one for the user, independently of the + // placeholders that were merged into them + $this->assertSame($user->organization->getKey(), $user2->user->fresh()->current_team_id); } public function test_accepting_invitation_while_logged_out_redirects_to_login(): void diff --git a/tests/Unit/Model/UserModelTest.php b/tests/Unit/Model/UserModelTest.php index bbb99e35..af862420 100644 --- a/tests/Unit/Model/UserModelTest.php +++ b/tests/Unit/Model/UserModelTest.php @@ -53,6 +53,23 @@ class UserModelTest extends ModelTestAbstract $this->assertTrue($canAccess); } + public function test_placeholder_user_with_a_super_admin_email_can_not_access_admin_panel(): void + { + // Arrange + Config::set('auth.super_admins', ['some@email.test', 'other@email.test']); + $user = User::factory()->placeholder()->create([ + 'email' => 'some@email.test', + ]); + $panelProvider = new AdminPanelProvider(app()); + $mainPanel = $panelProvider->panel(Panel::make()); + + // Act + $canAccess = $user->canAccessPanel($mainPanel); + + // Assert + $this->assertFalse($canAccess); + } + public function test_scope_belongs_to_organization_returns_only_users_of_organization_including_owners(): void { // Arrange diff --git a/tests/Unit/Service/DeletionServiceTest.php b/tests/Unit/Service/DeletionServiceTest.php index 333b0a04..b9dd4d56 100644 --- a/tests/Unit/Service/DeletionServiceTest.php +++ b/tests/Unit/Service/DeletionServiceTest.php @@ -412,10 +412,11 @@ class DeletionServiceTest extends TestCaseWithDatabase $this->assertDatabaseHas(Organization::class, [ 'id' => $organizationOfA->getKey(), ]); - // The placeholder user should exist with current_team_id set to the org where they are a placeholder + // The placeholder user should exist and must not reference the deleted organization, + // which is what caused the foreign key violation in #989 $placeholderUser = User::query()->where('is_placeholder', true)->first(); $this->assertNotNull($placeholderUser); - $this->assertSame($organizationOfA->getKey(), $placeholderUser->current_team_id); + $this->assertNull($placeholderUser->current_team_id); $this->assertDatabaseHas(Member::class, [ 'id' => $memberBInOrgA->getKey(), 'user_id' => $placeholderUser->getKey(), diff --git a/tests/Unit/Service/MemberServiceTest.php b/tests/Unit/Service/MemberServiceTest.php index 26498485..d23ca33c 100644 --- a/tests/Unit/Service/MemberServiceTest.php +++ b/tests/Unit/Service/MemberServiceTest.php @@ -13,6 +13,7 @@ use App\Models\TimeEntry; use App\Models\User; use App\Service\MemberService; use App\Service\UserService; +use Illuminate\Support\Facades\Hash; use InvalidArgumentException; use PHPUnit\Framework\Attributes\CoversClass; use Tests\TestCaseWithDatabase; @@ -64,6 +65,48 @@ class MemberServiceTest extends TestCaseWithDatabase $this->assertSame(Role::Admin->value, $oldOwnerMember->refresh()->role); } + public function test_make_member_to_placeholder_does_not_copy_the_credentials_and_account_state_of_the_user(): void + { + // Arrange + $user = User::factory()->create([ + 'password' => Hash::make('secret-password'), + 'remember_token' => 'remember-me-token', + 'two_factor_secret' => 'two-factor-secret', + 'two_factor_recovery_codes' => 'two-factor-recovery-codes', + 'two_factor_confirmed_at' => '2026-09-16 10:00:00', + 'email_verified_at' => '2026-09-16 09:00:00', + 'pending_email' => 'pending@example.com', + 'profile_photo_path' => 'profile-photos/photo.png', + ]); + $organization = Organization::factory()->create(); + $member = Member::factory()->forOrganization($organization)->forUser($user)->role(Role::Employee)->create(); + + // Act + $this->memberService->makeMemberToPlaceholder($member); + + // Assert + $member->refresh(); + $placeholderUser = $member->user; + $this->assertTrue($placeholderUser->is_placeholder); + $this->assertSame($user->email, $placeholderUser->email); + $this->assertNull($placeholderUser->password); + $this->assertNull($placeholderUser->remember_token); + $this->assertNull($placeholderUser->two_factor_secret); + $this->assertNull($placeholderUser->two_factor_recovery_codes); + $this->assertNull($placeholderUser->two_factor_confirmed_at); + $this->assertNull($placeholderUser->email_verified_at); + $this->assertNull($placeholderUser->pending_email); + $this->assertNull($placeholderUser->current_team_id); + $this->assertNull($placeholderUser->profile_photo_path); + // the user the placeholder was created from keeps their own credentials and state + $user->refresh(); + $this->assertTrue(Hash::check('secret-password', (string) $user->password)); + $this->assertSame('two-factor-secret', $user->two_factor_secret); + $this->assertNotNull($user->email_verified_at); + $this->assertSame('pending@example.com', $user->pending_email); + $this->assertSame('profile-photos/photo.png', $user->profile_photo_path); + } + public function test_make_member_to_placeholder_creates_new_user_based_on_member_and_changes_member_to_placeholder(): void { // Arrange