ueberauth / guardian

Elixir Authentication
MIT License
3.44k stars 381 forks source link

"Remember Me" with a different ttl #405

Closed mitkins closed 6 years ago

mitkins commented 7 years ago

I'm trying to utilise the new remember_me function. I would like my "refresh" token to last twice as long as my "access" token. In my config I've added a refresh_ttl key/value pair:

config :zoinks, Zoinks.Guardian,
  issuer: "Zoinks",
  ttl: { 30, :days },
  refresh_ttl: { 60, :days }

In my SessionController, I've created the following private function to create the refresh token using the setting (above):

defp remember_me( conn, user, remember_me ) do
  if ( remember_me ) do
    claims = Zoinks.Guardian.Plug.current_claims(conn)
    refresh_ttl = Zoinks.Guardian.config( :refresh_ttl )

    Zoinks.Guardian.Plug.remember_me( conn, user, claims, [ttl: refresh_ttl, token_type: "refresh"] )
  else
    conn
  end
end

Is the above code a reasonable approach?

The remember_me function is called right after the sign in:

conn
|> Zoinks.Guardian.Plug.sign_in(user)
|> remember_me( user, user_params["remember_me"] == "true" )

Should I see a third cookie in the browser after the sign in is successful (I currently have 2 - default and _zoinks_key?

The other thing is that when Guardian.Plug.VerifyCookie calls Guardian.exchange - will this keep the ttl of the refresh token, instead of using the default ttl?

hassox commented 7 years ago

Hey @mitkins For the TTL question, you can specify the TTL of different token types in your config or as options to use Guardian.

The option for this would be:

  token_ttl: %{
    "refresh" => {14, :days},
    "access" => {7, :days},
  }

Or you can set a default with the ttl option and just overwrite it.

  ttl: {7, :days},
  token_ttl: %{
    "refresh" => {14, :days}
  }

For the remember me cookie... Pretty sure that's a bug. They're all getting the same key which is not the intention :(

Thankyou for raising that!

mitkins commented 7 years ago

After doing some digging, I found out the following additional information:

In the auth_error call, I redirect to my login page - but only for :unauthenticated. Redirecting for another type/reason (in addition to :unauthenticated) ends up causing a circular redirect loop. So I think it would be nice (in the case of Guardian.Plug.VerifyCookie) if it didn't consider :token_expired to be an error.

I made a copy of Guardian.Plug.VerifyCookie and tested this theory by added the following match in the else part of the with clause:

else
  :no_token_found ->
    conn

  {:error, :token_expired} ->
    conn

  {:error, reason} ->
    conn
    |> Pipeline.fetch_error_handler!(opts)
    |> apply(:auth_error, [conn, {:invalid_token, reason}, opts])
    |> halt()

  _ ->
    conn
end

When the refresh token expires, my redirect now works as expected

mitkins commented 7 years ago

The other thing that I just noticed, is that the cookie does not have a lifespan and is not persistent over browser sessions

hassox commented 7 years ago

I'm wondering if we should pull this for now. This seems to be a little buggy atm and I don't really want to hold up 1.0 anymore.

@mitkins @doomspork @scrogson thoughts?

mitkins commented 7 years ago

I say pull it out, release 1.0, then give it the treatment it deserves. I'm happy to workaround for now.

Hanspagh commented 7 years ago

Hi, I started working on this, any suggesting to the lifespan of the cookie?

mitkins commented 7 years ago

I've been giving it the same lifespan as the ttl for the refresh token

Hanspagh commented 6 years ago

fixed by #419