ueberauth / guardian

Elixir Authentication
MIT License
3.43k stars 382 forks source link

No way to delete `remember_me` cookie from client #595

Closed futpib closed 5 years ago

futpib commented 5 years ago

After I've done remember_me_from_token(conn, token, claims) in the login controller method, I'd like to do something in the logout method so that a set-cookie header with the token cookie removed is sent to the client.

Currently my best bet is to copy the private fetch_token_key to my code and use that together with put_resp_cookie to manually clear the cookie.

EDIT: this worked (in my controller):

  def logout(conn, _) do
    key = fetch_token_key(conn)

    conn
    |> Guardian.Plug.sign_out()
    |> Guardian.Plug.clear_remember_me()
    |> put_resp_cookie(key, "deleted", Accounts.Auth.Guardian.config(:cookie_options) ++ [ max_age: 0 ])
    |> render("logout.json")
  end

  # Workaround for https://github.com/ueberauth/guardian/issues/595
  defp fetch_token_key(conn, opts \\ []) do
    conn
    |> Pipeline.fetch_key(opts)
    |> Keys.token_key()
    |> Atom.to_string()
  end
doomspork commented 5 years ago

@futpib would you like to add something to the README about this?

Should we add a function to our public API to clear the cookie @ueberauth/developers?

futpib commented 5 years ago

Oh, I think README already offers a solution to this (:clear_remember_me option): https://github.com/ueberauth/guardian/blob/3fef64e08bdf3d4a18e531e7e78be269826ff1bc/README.md#L128-L130

Sorry, I did not notice this before :man_facepalming:

futpib commented 5 years ago

Ah, :clear_remember_me does not solve this issue. :clear_remember_me removes the cookie from the response

https://github.com/ueberauth/guardian/blob/5663cfa94bdabe33474accb7b49052ca12ea4ea5/lib/guardian/plug.ex#L235-L239

while I need to send a "deleted" cookie to the client, so that it is deleted/invalidated on the client.

Hanspagh commented 5 years ago

Hi @futpib. What is the reason you need the cookie to deleted. I think I would prefer to do deal with the issue with setting the cookie as expired instead. What do you think about this?

futpib commented 5 years ago

What do you mean by "setting the cookie as expired"? Do you suggest storing invalidated tokens in a database?

Hanspagh commented 5 years ago

No was thinking to ask the browser to expire the cookie. This post from stack overflow has quite a good explanation

futpib commented 5 years ago

That is what max_age: 0 in this line of my current workaround is about:

|> put_resp_cookie(key, "deleted", Accounts.Auth.Guardian.config(:cookie_options) ++ [ max_age: 0 ])
Hanspagh commented 5 years ago

Ahh sorry, I missed that. I think this is great and we should make it part of the clear_remember_me implementation. For the sense of understanding, I would recommend to just a empty string instead of "deleted". Would you be interested in doing the PR @futpib ?