aaronrenner / phx_gen_auth

An authentication system generator for Phoenix 1.5 applications.
774 stars 56 forks source link

hashed_password assigned to Conn #103

Closed njwest closed 3 years ago

njwest commented 3 years ago

Thanks for this really great, robust auth solution generator! Am loving using it for Phoenix session auth.

I am a bit uneasy with how the default auth behavior assigns hashed_password to conn in UserAuth's fetch_current_user function, via verify_session_token_query/1 (~/lib/my_app/accounts/user_token.ex):

## ..._web/controllers/user_auth.ex

  def fetch_current_user(conn, _opts) do
    {user_token, conn} = ensure_user_token(conn)
    user = user_token && Accounts.get_user_by_session_token(user_token)
    assign(conn, :current_user, user)
  end

## .../accounts.ex

  def get_user_by_session_token(token) do
    {:ok, query} = UserToken.verify_session_token_query(token)
    Repo.one(query)
  end

## .../accounts/user_token.ex

  def verify_session_token_query(token) do
    query =
      from token in token_and_context_query(token, "session"),
        join: user in assoc(token, :user),
        where: token.inserted_at > ago(@session_validity_in_days, "day"),
        select: user

    {:ok, query}
  end

I'm removing this in my own codebase by just selecting what I need from the user record in verify_session_token_query:

        select: %{id: user.id, confirmed_at: user.confirmed_at}
josevalim commented 3 years ago

Hi @njwest! It is not a problem to put it in the connection. After all, the connection has other confidential information, such as secret_key_base and similar. The reason that's ok is because the connection only lives during the scope of the request and is discarded afterwards. It is not shared with the client nor anything else.

However, there is nothing wrong with your changes either. So it is up to you!

njwest commented 3 years ago

Thanks, @josevalim ! My main concern is a developer not realizing that there is sensitive info in conn.assigns[:current_user] and irresponsibly passing the entire current_user record to the frontend, but I suppose it is up to the developer to code responsibly