diff --git a/app/Modules/Clients/Http/Controllers/InvitationRedemptionController.php b/app/Modules/Clients/Http/Controllers/InvitationRedemptionController.php index a24bd63a..aa34f039 100644 --- a/app/Modules/Clients/Http/Controllers/InvitationRedemptionController.php +++ b/app/Modules/Clients/Http/Controllers/InvitationRedemptionController.php @@ -69,6 +69,24 @@ class InvitationRedemptionController extends Controller ]); } + // An invitation is live for days, and the address it names can be + // taken in the meantime — staff got impatient and made the account + // by hand, or the person registered through the public form. The + // unique index on users.email spans trashed rows, so provision() + // would raise a QueryException here rather than refusing: a 500 on + // the screen of somebody who has just typed a password. Every other + // caller with no form to validate asks this first, for this reason + // — see ClientProvisioning::addressIsFree(). + if (! $this->provisioning->addressIsFree($invitation->email)) { + throw ValidationException::withMessages([ + // Says what happened, because the person holding this link + // already knows the address is theirs — it is the one the + // invitation was sent to. There is nothing here to disclose + // that the invitation itself did not. + 'token' => [__('An account already exists for this email address. Try signing in instead, or reset your password.')], + ]); + } + $client = $this->provisioning->provision( name: $validated['name'], email: $invitation->email, diff --git a/tests/Feature/Clients/ClientInvitationTest.php b/tests/Feature/Clients/ClientInvitationTest.php index 2c952676..86d3a969 100644 --- a/tests/Feature/Clients/ClientInvitationTest.php +++ b/tests/Feature/Clients/ClientInvitationTest.php @@ -238,3 +238,35 @@ test('resending an expired invitation issues a fresh token and emails it, withou $this->post('/invite/not-a-real-token/resend')->assertRedirect(); Notification::assertNothingSent(); }); + +test('an invitation whose address was taken while the link was live refuses rather than failing on the unique index', function () { + $invitation = Invitation::issue('invited@example.com', null, null, $this->admin, now()->addDay()); + + // Staff got impatient, or the person used the public form instead. + User::factory()->client()->create(['email' => 'invited@example.com']); + + $this->post("/invite/{$invitation->token}", [ + 'token' => $invitation->token, + 'name' => 'Invited Person', + 'password' => 'super-secret-password', + 'password_confirmation' => 'super-secret-password', + ])->assertSessionHasErrors('token'); + + expect(User::query()->where('email', 'invited@example.com')->count())->toBe(1) + ->and($invitation->fresh()->status)->toBe(Invitation::STATUS_PENDING); +}); + +test('an address held by a deleted account is still taken, the same as it is everywhere else', function () { + $invitation = Invitation::issue('invited@example.com', null, null, $this->admin, now()->addDay()); + + User::factory()->client()->create(['email' => 'invited@example.com'])->delete(); + + $this->post("/invite/{$invitation->token}", [ + 'token' => $invitation->token, + 'name' => 'Invited Person', + 'password' => 'super-secret-password', + 'password_confirmation' => 'super-secret-password', + ])->assertSessionHasErrors('token'); + + expect(User::query()->where('email', 'invited@example.com')->exists())->toBeFalse(); +});