From 20329b176bffa79e4852f7826aeb7f57ca4f900c Mon Sep 17 00:00:00 2001 From: nexxo Date: Tue, 28 Jul 2026 14:42:16 +0200 Subject: [PATCH] Refuse a portal login for an address that already belongs to an operator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Customer::ensureUser() enforced uniqueness only within users, so a customer provisioned (or registering publicly) with an operator's email got a second, colliding identity for the same address — the exact thing R21 exists to prevent. Checks Operator by address now, not by session (the deleted assertNotAdmin() checked who was signed in, which could never be the right question). Public registration gets the same guard via a plain unique() validation rule, reusing Laravel's stock "already taken" message rather than a dedicated one — naming the collision specifically would tell a public visitor an address belongs to staff, a worse leak than the generic refusal every other collision here already gives them. Impersonation answers 409 instead of crashing into it. The Stripe webhook keeps recording the paid order (same principle as openContract()'s own comment: the order is proof money changed hands) and withholds only the colliding login, logged for an operator to resolve by hand. Verified the reverse direction Codex flagged as already covered: two Admin\Settings actions and the clupilot:create-operator command all already refuse an operator email that collides with a customer's, and that check is sound today because every users row is created in lockstep with a customers row of the same email (CreateNewUser and ensureUser are the only two creation sites, both confirmed by grep) and nothing in this app can change a user's email afterwards (Fortify's updateProfileInformation feature is disabled, and no other code calls that action). --- app/Actions/Fortify/CreateNewUser.php | 9 ++++++ app/Actions/StartCustomerProvisioning.php | 17 ++++++++++- app/Exceptions/IdentityCollisionException.php | 21 ++++++++++++++ .../Controllers/ImpersonationController.php | 13 +++++++-- app/Models/Customer.php | 20 +++++++++---- lang/de/errors.php | 5 ++++ lang/en/errors.php | 5 ++++ resources/views/errors/409.blade.php | 1 + tests/Feature/Auth/RegisterTest.php | 19 ++++++++++-- tests/Feature/ImpersonationTest.php | 29 +++++++++++++++++++ .../Provisioning/StripeWebhookTest.php | 21 ++++++++++++++ 11 files changed, 150 insertions(+), 10 deletions(-) create mode 100644 app/Exceptions/IdentityCollisionException.php create mode 100644 resources/views/errors/409.blade.php diff --git a/app/Actions/Fortify/CreateNewUser.php b/app/Actions/Fortify/CreateNewUser.php index d4dfdf7..71700cc 100644 --- a/app/Actions/Fortify/CreateNewUser.php +++ b/app/Actions/Fortify/CreateNewUser.php @@ -3,6 +3,7 @@ namespace App\Actions\Fortify; use App\Models\Customer; +use App\Models\Operator; use App\Models\User; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Hash; @@ -36,6 +37,14 @@ class CreateNewUser implements CreatesNewUsers // provisioned) customer's email — ensureUser() would later link // that account and hand over the customer's portal. Rule::unique(Customer::class, 'email'), + // Nor an operator's — R21: one address must not represent both + // identities. No dedicated message: Laravel's stock "already + // taken" line (translated in both languages already) is what + // the two rules above fall back to as well, and telling a + // public visitor specifically that an address belongs to + // STAFF would be a worse leak than the generic refusal every + // other collision here already gives them. + Rule::unique(Operator::class, 'email'), ], 'password' => $this->passwordRules(), ])->validate(); diff --git a/app/Actions/StartCustomerProvisioning.php b/app/Actions/StartCustomerProvisioning.php index 95af926..89e06a9 100644 --- a/app/Actions/StartCustomerProvisioning.php +++ b/app/Actions/StartCustomerProvisioning.php @@ -2,6 +2,7 @@ namespace App\Actions; +use App\Exceptions\IdentityCollisionException; use App\Models\Customer; use App\Models\Order; use App\Models\ProvisioningRun; @@ -39,7 +40,21 @@ class StartCustomerProvisioning } $customer = $this->resolveCustomer($event); - $customer->ensureUser(); // portal login (also enables admin impersonation) + + try { + $customer->ensureUser(); // portal login (also enables admin impersonation) + } catch (IdentityCollisionException) { + // Same principle as openContract() below: the order is the record + // that money changed hands, so a paid order still gets recorded + // and provisioned. Only the colliding portal LOGIN is withheld — + // R21 — until an operator resolves the address conflict by hand + // (rename the operator, or the customer). Until then this + // customer has no self-service sign-in and cannot be + // impersonated either; both call ensureUser() the same way. + Log::error('Portal login withheld: this customer email already belongs to an operator account.', [ + 'customer_id' => $customer->id, 'email' => $customer->email, + ]); + } try { [$order, $run] = DB::transaction(function () use ($event, $customer) { diff --git a/app/Exceptions/IdentityCollisionException.php b/app/Exceptions/IdentityCollisionException.php new file mode 100644 index 0000000..297eff5 --- /dev/null +++ b/app/Exceptions/IdentityCollisionException.php @@ -0,0 +1,21 @@ +firstOrFail()->ensureUser(); + // Stale data, not a normal outcome: a customer row sharing its email + // with an operator should never have been possible to reach this far + // (see Customer::ensureUser() and CreateNewUser, R21) — refuse rather + // than hand the operator's own address a portal session. + try { + $target = Customer::where('uuid', $customer)->firstOrFail()->ensureUser(); + } catch (IdentityCollisionException) { + abort(409); + } $who = Operator::where('uuid', $operator)->firstOrFail(); Auth::guard('web')->login($target); diff --git a/app/Models/Customer.php b/app/Models/Customer.php index 2687655..b919f4e 100644 --- a/app/Models/Customer.php +++ b/app/Models/Customer.php @@ -2,7 +2,9 @@ namespace App\Models; +use App\Exceptions\IdentityCollisionException; use App\Models\Concerns\HasUuid; +use Database\Factories\CustomerFactory; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\BelongsTo; @@ -38,7 +40,7 @@ class Customer extends Model && $this->normalisedVatId() === self::normaliseVatId($this->vat_id_verified_value); } - /** @use HasFactory<\Database\Factories\CustomerFactory> */ + /** @use HasFactory */ use HasFactory, HasUuid; protected $fillable = [ @@ -97,10 +99,14 @@ class Customer extends Model /** * Find or create the portal login account for this customer and link it. - * Race-safe against the unique email index. Never links an operator - * account by construction, not by a runtime check: operators live in - * `operators`, on their own guard, and are never rows in `users` at all - * (see R21) — there is no longer an admin `users` row this could find. + * Race-safe against the unique email index. Refuses by construction to + * create or link a `users` row for an address that already belongs to an + * `operators` row (R21) — that table split killed the old "an operator + * account never has an is_admin users row" guarantee this method used to + * lean on, and left this one open: nothing stops the SAME address from + * being provisioned as a customer independently of its operator row. + * + * @throws IdentityCollisionException when the address is already an operator's. */ public function ensureUser(): User { @@ -108,6 +114,10 @@ class Customer extends Model return $this->user; } + if (Operator::query()->where('email', $this->email)->exists()) { + throw new IdentityCollisionException($this->email); + } + if (($existing = User::query()->where('email', $this->email)->first()) !== null) { $this->update(['user_id' => $existing->id]); diff --git a/lang/de/errors.php b/lang/de/errors.php index 71fee3e..406ee5e 100644 --- a/lang/de/errors.php +++ b/lang/de/errors.php @@ -27,6 +27,11 @@ return [ 'body' => 'Die Anfrage passt nicht zu dieser Adresse.', 'hint' => null, ], + '409' => [ + 'title' => 'Dieses Konto lässt sich nicht verbinden.', + 'body' => 'Diese E-Mail-Adresse des Kunden gehört bereits zu einem Mitarbeiterkonto. Ein Inhaber muss diesen Konflikt auflösen, bevor dieses Portal erreichbar ist.', + 'hint' => null, + ], '419' => [ 'title' => 'Die Sitzung ist abgelaufen.', 'body' => 'Aus Sicherheitsgründen wird ein Formular nach längerer Zeit ungültig.', diff --git a/lang/en/errors.php b/lang/en/errors.php index 1d7ae45..e071bb5 100644 --- a/lang/en/errors.php +++ b/lang/en/errors.php @@ -27,6 +27,11 @@ return [ 'body' => 'The request does not match this address.', 'hint' => null, ], + '409' => [ + 'title' => 'This account cannot be connected.', + 'body' => 'This customer\'s email address already belongs to a staff account. An owner needs to resolve that conflict before this portal can be reached.', + 'hint' => null, + ], '419' => [ 'title' => 'The session has expired.', 'body' => 'For security, a form stops being valid after a while.', diff --git a/resources/views/errors/409.blade.php b/resources/views/errors/409.blade.php new file mode 100644 index 0000000..7b7778a --- /dev/null +++ b/resources/views/errors/409.blade.php @@ -0,0 +1 @@ +@include('errors.layout', ['code' => 409]) diff --git a/tests/Feature/Auth/RegisterTest.php b/tests/Feature/Auth/RegisterTest.php index 5b06ab4..69b0b59 100644 --- a/tests/Feature/Auth/RegisterTest.php +++ b/tests/Feature/Auth/RegisterTest.php @@ -1,5 +1,7 @@ assertAuthenticatedAs($user); // A linked customer is created so the portal has something to work with. - $customer = \App\Models\Customer::query()->where('user_id', $user->id)->first(); + $customer = Customer::query()->where('user_id', $user->id)->first(); expect($customer)->not->toBeNull()->and($customer->email)->toBe('new@signup.test'); }); it('refuses to register with an existing customer email (account-claim guard)', function () { - \App\Models\Customer::factory()->create(['email' => 'claim@signup.test']); + Customer::factory()->create(['email' => 'claim@signup.test']); $this->post(route('register.store'), [ 'name' => 'Claimer', 'email' => 'claim@signup.test', @@ -41,6 +43,19 @@ it('refuses to register with an existing customer email (account-claim guard)', expect(User::query()->where('email', 'claim@signup.test')->exists())->toBeFalse(); }); +it('refuses to register with an existing operator email', function () { + // R21: the operator side of the same account-claim guard above — an + // address that already belongs to staff must not become a customer too. + Operator::factory()->create(['email' => 'staff@signup.test']); + + $this->post(route('register.store'), [ + 'name' => 'Claimer', 'email' => 'staff@signup.test', + 'password' => 'password1234', 'password_confirmation' => 'password1234', + ])->assertSessionHasErrors('email'); + + expect(User::query()->where('email', 'staff@signup.test')->exists())->toBeFalse(); +}); + it('throttles repeated registration attempts', function () { $hit429 = false; for ($i = 0; $i < 24; $i++) { diff --git a/tests/Feature/ImpersonationTest.php b/tests/Feature/ImpersonationTest.php index 325b5ee..bf2b82e 100644 --- a/tests/Feature/ImpersonationTest.php +++ b/tests/Feature/ImpersonationTest.php @@ -1,5 +1,6 @@ and(Auth::guard('web')->user()->email)->toBe($customer->email); }); +it('refuses to create or link a portal login when the email already belongs to an operator', function () { + // R21: one address must not represent both identities. This is the + // provisioning/impersonation-time half of the rule; CreateNewUser enforces + // the same thing at public registration (see RegisterTest). + Operator::factory()->create(['email' => 'shared@example.test']); + $customer = Customer::factory()->create(['email' => 'shared@example.test']); + + expect(fn () => $customer->ensureUser()) + ->toThrow(IdentityCollisionException::class); + + expect(User::query()->where('email', 'shared@example.test')->exists())->toBeFalse() + ->and($customer->fresh()->user_id)->toBeNull(); +}); + +it('refuses to impersonate into an email that already belongs to an operator, instead of crashing', function () { + $operator = Operator::factory()->role('Owner')->create(); + Operator::factory()->create(['email' => 'shared@example.test']); + $customer = Customer::factory()->create(['email' => 'shared@example.test']); + + $url = URL::temporarySignedRoute('impersonate.enter', now()->addSeconds(60), [ + 'customer' => $customer->uuid, 'operator' => $operator->uuid, + ]); + + $this->get($url)->assertStatus(409); + + expect(Auth::guard('web')->check())->toBeFalse(); +}); + it('signs the customer in on the web guard when the link is followed', function () { $operator = Operator::factory()->role('Owner')->create(); $customer = Customer::factory()->create(); diff --git a/tests/Feature/Provisioning/StripeWebhookTest.php b/tests/Feature/Provisioning/StripeWebhookTest.php index 6a2e603..5cb452d 100644 --- a/tests/Feature/Provisioning/StripeWebhookTest.php +++ b/tests/Feature/Provisioning/StripeWebhookTest.php @@ -1,7 +1,9 @@ create(['email' => 'kunde@example.com']); + + $this->postJson(route('webhooks.stripe'), stripeEvent('evt_opcollide'))->assertOk(); + + $order = Order::query()->where('stripe_event_id', 'cs_evt_opcollide')->first(); + expect($order)->not->toBeNull() + ->and(ProvisioningRun::query()->where('subject_id', $order->id)->where('pipeline', 'customer')->exists())->toBeTrue() + ->and(User::query()->where('email', 'kunde@example.com')->exists())->toBeFalse(); + + Queue::assertPushed(AdvanceRunJob::class, 1); +}); + it('rejects an invalid signature when a secret is configured', function () { config()->set('services.stripe.webhook_secret', 'whsec_test');