Check users directly for the reverse operator-identity collision, not just customers
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.
feat/operator-identity
parent
278c4b9953
commit
58835a1051
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -0,0 +1,15 @@
|
|||
<?php
|
||||
|
||||
use App\Models\Operator;
|
||||
use App\Models\User;
|
||||
|
||||
it('will not create an operator from an orphaned portal login', function () {
|
||||
// A users row with no matching customers row — Customer::where('email',
|
||||
// ...) alone cannot see this identity; it still lives in `users`.
|
||||
User::factory()->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();
|
||||
});
|
||||
Loading…
Reference in New Issue