Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete all sessions when calling Pow.Plug.delete_user/2 #404

Open
danschultzer opened this issue Feb 3, 2020 · 5 comments · May be fixed by #567
Open

Delete all sessions when calling Pow.Plug.delete_user/2 #404

danschultzer opened this issue Feb 3, 2020 · 5 comments · May be fixed by #567
Labels
enhancement New feature or request

Comments

@danschultzer
Copy link
Collaborator

danschultzer commented Feb 3, 2020

Prompted by #386 (comment)

Currently Pow.Plug.delete_user/2 calls do_delete/2 for the plug, which only deletes the current session. However it should clear all sessions for the user. Only Pow.Store.CredentialsCache knows about related sessions which makes this tricky. Might work if a config value is passed on e.g. delete_all_for: user.

PowPersistentSession might interfere with this. The way to resolve that is to delete all persistent session tokens that has a session fingerprint that's deleted. There could be a method in PowPersistentSession.Store.PersistentSessionCache to do that, but it might also be a good idea to be able to look up all persistent sessions for the user.

@danschultzer danschultzer added the enhancement New feature or request label Feb 3, 2020
@Kurisu3
Copy link

Kurisu3 commented Sep 14, 2020

May I ask a question about this?

I have a custom controller in an admin panel to manage users (list - delete). I will maybe add new - create actions later since with the Invitation extension it is not possible to set a role along the invited user email.

But let's come back to my question about user deletion. If I understand correctly Pow.Plug.delete_user/2 is not for the case when an admin want to delete another user....
So in that case would just calling something like Repo.delete(user) be enough? I mean can I just delete the user from the database without any issue?

@danschultzer
Copy link
Collaborator Author

If I understand correctly Pow.Plug.delete_user/2 is not for the case when an admin want to delete another user....

Correct, it's only when the user deletes the current user signed in, and deletes the associated session.

I mean can I just delete the user from the database without any issue?

The session will still be active, so if you don't check if the user exists in the DB the user may be able to continue using the app until their session expire. #563 would resolve this.

To remove all the sessions for the user it's necessary to call Pow.Store.CredentialsCache.sessions/2 and then Pow.Store.CredentialsCache.delete/2 for each of them.

I wonder if permitting a second argument with the user to be deleted in Pow.Plug.delete_user/1 makes sense.

@Kurisu3
Copy link

Kurisu3 commented Sep 20, 2020

Thanks for this clarification.

I wonder if permitting a second argument with the user to be deleted in Pow.Plug.delete_user/1 makes sense.

That would be great! ^^

@danschultzer danschultzer linked a pull request Sep 20, 2020 that will close this issue
5 tasks
@benonymus
Copy link

benonymus commented May 23, 2023

If I understand correctly Pow.Plug.delete_user/2 is not for the case when an admin want to delete another user....

Correct, it's only when the user deletes the current user signed in, and deletes the associated session.

I mean can I just delete the user from the database without any issue?

The session will still be active, so if you don't check if the user exists in the DB the user may be able to continue using the app until their session expire. #563 would resolve this.

To remove all the sessions for the user it's necessary to call Pow.Store.CredentialsCache.sessions/2 and then Pow.Store.CredentialsCache.delete/2 for each of them.

I wonder if permitting a second argument with the user to be deleted in Pow.Plug.delete_user/1 makes sense.

Hey @danschultzer ,

I ran into this comment while I was looking for a way to clear up sessions in case a user updates their password.
What config do I need to pass to these to query both the sessions and the credentials?

If I do CredentialsCache.sessions([backend: "backend here"], user)
and I delete, the persistent_session stays around and the user is not logged out.

We have a liveview page and getting the config from the socket was not working.

The use case is in case a user account is compromised, and they chnaged the password we want preferably the other sessions to be deleted.

Would it be possible to remove the reset_token as well?

Thanks

@sb8244
Copy link

sb8244 commented Oct 9, 2023

@benonymus I ended up doing something like this to clear out the session + the persistent session. Unfortunately, due to how persistent sessions are stored, there's no way except full enumeration to clear it out:

  def before_respond(PowResetPassword.Phoenix.ResetPasswordController, :update, {:ok, user = %{id: user_id}, conn}, _config) do
    session_key = :erlang.term_to_binary([UserService.User, :user, user.id])

    # Delete all active sessions
    from(
      sesh in UserService.PostgresPowStore,
      where: sesh.original_key == ^session_key
    )
    |> Super.Repo.delete_all()

    # Delete the persistent session entries
    from(
      pow in UserService.PostgresPowStore,
      where: pow.namespace == "persistent_session"
    )
    |> Super.Repo.all()
    |> Enum.each(fn entry ->
      case :erlang.binary_to_term(entry.value) do
        {%UserService.User{id: ^user_id}, _opts} ->
          from(
            pow in UserService.PostgresPowStore,
            where: pow.key == ^entry.key and pow.namespace == ^entry.namespace
          )
          |> Super.Repo.delete_all()

        _ ->
          nil
      end
    end)

    {:ok, user, conn}
  end

I think enumeration that doesn't load all (batch it) and using the actual store functions would make this better. I don't care about being generic though, so just winged it a bit :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants