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');