From 58835a1051e0aff95730cebcc9e6804f6c28152a Mon Sep 17 00:00:00 2001 From: nexxo Date: Tue, 28 Jul 2026 15:36:29 +0200 Subject: [PATCH] Check users directly for the reverse operator-identity collision, not just customers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three sites that refuse to create or rename an operator onto a customer's email (Admin\Settings::saveAccount(), ::inviteStaff(), and clupilot:create-operator) all checked Customer::where('email', ...) as a proxy for "does this address already have a portal login". A users row with no matching customers row — an email changed on one side only, or legacy/orphaned data — passed straight through: this dev database already had one. Extracted the three copies into Customer::emailTaken(), which checks both tables directly, so the three sites cannot drift from each other again. --- app/Console/Commands/CreateAdmin.php | 7 ++++--- app/Livewire/Admin/Settings.php | 6 ++++-- app/Models/Customer.php | 18 ++++++++++++++++ tests/Feature/Admin/AdminSettingsTest.php | 25 ++++++++++++++++++++++- tests/Feature/Console/CreateAdminTest.php | 15 ++++++++++++++ 5 files changed, 65 insertions(+), 6 deletions(-) create mode 100644 tests/Feature/Console/CreateAdminTest.php diff --git a/app/Console/Commands/CreateAdmin.php b/app/Console/Commands/CreateAdmin.php index 772a2e9..5ed80d2 100644 --- a/app/Console/Commands/CreateAdmin.php +++ b/app/Console/Commands/CreateAdmin.php @@ -6,8 +6,8 @@ use App\Models\Customer; use App\Models\Operator; use Illuminate\Console\Command; use Illuminate\Support\Facades\Hash; -use Illuminate\Validation\Rules\Password; use Illuminate\Support\Facades\Validator; +use Illuminate\Validation\Rules\Password; use Spatie\Permission\PermissionRegistrar; /** @@ -56,8 +56,9 @@ class CreateAdmin extends Command } // An operator address that also belongs to a customer would block that - // customer from ever getting a portal login. - if (Customer::query()->where('email', $email)->exists()) { + // customer from ever getting a portal login. Checked directly against + // `users`, not only `customers`: see Customer::emailTaken(). + if (Customer::emailTaken($email)) { $this->error('That address already belongs to a customer.'); return self::FAILURE; diff --git a/app/Livewire/Admin/Settings.php b/app/Livewire/Admin/Settings.php index fe83da6..39dfb11 100644 --- a/app/Livewire/Admin/Settings.php +++ b/app/Livewire/Admin/Settings.php @@ -97,7 +97,9 @@ class Settings extends Component // An operator email must never collide with a customer's — that would // block the customer from ever obtaining a portal login (ensureUser). - if (Customer::query()->where('email', $data['email'])->exists()) { + // Checked directly against `users`, not only `customers`: see + // Customer::emailTaken(). + if (Customer::emailTaken($data['email'])) { $this->addError('email', __('admin_settings.is_customer')); return; @@ -123,7 +125,7 @@ class Settings extends Component return; } // Never turn a customer's portal login into an operator. - if (Customer::query()->where('email', $data['staffEmail'])->exists()) { + if (Customer::emailTaken($data['staffEmail'])) { $this->addError('staffEmail', __('admin_settings.is_customer')); return; diff --git a/app/Models/Customer.php b/app/Models/Customer.php index b919f4e..bdfd646 100644 --- a/app/Models/Customer.php +++ b/app/Models/Customer.php @@ -97,6 +97,24 @@ class Customer extends Model return $this->hasMany(Instance::class); } + /** + * True if this address already belongs to a customer identity — R21's + * other direction, checked before an `operators` row is created or + * renamed onto it (Admin\Settings::saveAccount(), ::inviteStaff(), and + * the clupilot:create-operator command — one rule, three call sites). + * + * Checks `users` directly, not only `customers`: the two are usually in + * lockstep (see ensureUser()), but a `users` row can outlive or predate + * a matching `customers.email` — an email changed on one side only, or + * legacy/orphaned data — and a `customers`-only check is blind to + * exactly that row. + */ + public static function emailTaken(string $email): bool + { + return static::query()->where('email', $email)->exists() + || User::query()->where('email', $email)->exists(); + } + /** * Find or create the portal login account for this customer and link it. * Race-safe against the unique email index. Refuses by construction to diff --git a/tests/Feature/Admin/AdminSettingsTest.php b/tests/Feature/Admin/AdminSettingsTest.php index 4493326..34e9433 100644 --- a/tests/Feature/Admin/AdminSettingsTest.php +++ b/tests/Feature/Admin/AdminSettingsTest.php @@ -4,6 +4,7 @@ use App\Livewire\Admin\Settings; use App\Models\Customer; use App\Models\Operator; use App\Models\User; +use Illuminate\Support\Facades\Hash; use Livewire\Livewire; // operator() helper defined in RbacTest. @@ -17,6 +18,17 @@ it('updates the operator own account', function () { expect($owner->fresh()->name)->toBe('New Name')->and($owner->fresh()->email)->toBe('new@ops.test'); }); +it('will not save an account email that collides with an orphaned portal login', function () { + // A users row with no matching customers row — an email changed on one + // side only, or legacy data. Customer::where('email', ...) alone cannot + // see this; the identity still lives in `users`. + User::factory()->create(['email' => 'orphan-account@ops.test']); + + Livewire::actingAs(operator('Owner'), 'operator')->test(Settings::class) + ->set('email', 'orphan-account@ops.test')->call('saveAccount') + ->assertHasErrors(['email']); +}); + it('lets an Owner invite staff with a role', function () { Livewire::actingAs(operator('Owner'), 'operator')->test(Settings::class) ->set('staffName', 'Sam')->set('staffEmail', 'sam@ops.test')->set('staffRole', 'Support') @@ -34,7 +46,7 @@ it('surfaces a usable temporary password for the invited staff', function () { $temp = $c->get('invitedPassword'); expect($temp)->not->toBeNull(); $ivy = Operator::query()->where('email', 'ivy@ops.test')->first(); - expect(Illuminate\Support\Facades\Hash::check($temp, $ivy->password))->toBeTrue(); + expect(Hash::check($temp, $ivy->password))->toBeTrue(); }); it('forbids a non-Owner from managing staff', function () { @@ -52,6 +64,17 @@ it('will not create an operator from a customer email', function () { ->call('inviteStaff')->assertHasErrors(['staffEmail']); }); +it('will not create an operator from an orphaned portal login', function () { + // Same gap as the account-save case above, at the staff-invite site. + User::factory()->create(['email' => 'orphan-invite@ops.test']); + + Livewire::actingAs(operator('Owner'), 'operator')->test(Settings::class) + ->set('staffName', 'Orphan')->set('staffEmail', 'orphan-invite@ops.test')->set('staffRole', 'Support') + ->call('inviteStaff')->assertHasErrors(['staffEmail']); + + expect(Operator::query()->where('email', 'orphan-invite@ops.test')->exists())->toBeFalse(); +}); + it('will not escalate a non-staff user via a tampered id', function () { // The old risk was one table shared by both groups: a customer's numeric // id could be handed to setStaffRole() and land on that same row. That is diff --git a/tests/Feature/Console/CreateAdminTest.php b/tests/Feature/Console/CreateAdminTest.php new file mode 100644 index 0000000..a11bc3d --- /dev/null +++ b/tests/Feature/Console/CreateAdminTest.php @@ -0,0 +1,15 @@ +create(['email' => 'orphan-cli@ops.test']); + + $this->artisan('clupilot:create-operator --email=orphan-cli@ops.test --name=Orphan --password=a-strong-password-123') + ->assertFailed(); + + expect(Operator::query()->where('email', 'orphan-cli@ops.test')->exists())->toBeFalse(); +});