laravel / jetstream

Tailwind scaffolding for the Laravel framework.
https://jetstream.laravel.com
MIT License
3.95k stars 810 forks source link

Recreating an account after account deletion doesn't work properly #730

Closed Loots-it closed 3 years ago

Loots-it commented 3 years ago

Description:

When testing my application I found a small bug, nothing serious, mostly annoying when testing/debugging. If you create an account and delete it, there is a small bug when you try to recreate the same account (although I think it will also happen if you try to create another account). When you register, you won't be logged in immediately, but you will be redirected to the login page. I am pretty sure this is because the login cookie of the old account isn't deleted yet. If I delete all the cookies from the site before I try to recreate the account, everything works fine.

Steps To Reproduce:

  1. register a new account 1.1. as expected, you are now logged in
  2. go to profile and delete this new account
  3. try to recreate the account you just deleted 3.1. this time, you are not logged in directly, but are redirected to the login page -> bug
driesvints commented 3 years ago

I can confirm this. I'll pick this up internally to see what the best solution would be.

driesvints commented 3 years ago

We'd appreciate a PR for this, otherwise we'll pick this up eventually.

driesvints commented 3 years ago

I'm going to mark this as an enhancement since nothing is really broken here.

Loots-it commented 3 years ago

If I find some time, I will look into it! It shouldn't be too difficult I think/hope.

Loots-it commented 3 years ago

I quickly checked the differences between the logout action and the delete user action. As expected the logout action (destroy method on AuthenticatedSessionController class) also invalidates the session:

$request->session()->invalidate(); $request->session()->regenerateToken();

The easiest way to fix this is to use dependency injection to inject the AuthenticatedSessionController class into the deleteUser method on the DeleteUserForm class and call the destroy method from there instead of just calling logout(). But I'm not sure this is an accepted approach.

This also has the benefit that the LogoutResponse is used instead of redirecting to '/'. Which makes sense because when you're deleting your account, you're also logging out.

It's also possible to add both lines:

$request->session()->invalidate(); $request->session()->regenerateToken();

to the deleteUser method on the DeleteUserForm class which gives almost the same result, but then you are not using LogoutResponse. (and are duplicating some code)

On which branch should I create a pull request? And which method do you prefer? I think the first approach is best.

I'm not sure if Inertia has the same problem or not. I still need to check this when I have some time (Inertia doesn't use the DeleteUserForm class).

driesvints commented 3 years ago

I think the latter and I think the 2.x branch is fine. The first one re-uses too much a bit there I believe.

Loots-it commented 3 years ago

Inertia has the same bug. And ok, I will use the second approach, I personally think the first one is better though. We just want to destroy the authenticated session. With the second approach, this code will be duplicated across 3 places (logout, inertia delete account, livewire delete account). I will implement the fix today or tomorrow.