pow-auth / pow_assent

Multi-provider authentication for your Pow enabled app
https://powauth.com
MIT License
325 stars 50 forks source link

Trying to preload user_identities by overriding function #125

Closed praveenperera closed 4 years ago

praveenperera commented 4 years ago

I am using MyApp.Accounts as the context for user and user_identities:

I want the users, user_identities to always be preloaded.

With the file below it preloads when logging in regularly (with email and password) but not when logging in via oauth.

defmodule MyApp.Accounts do
  alias MyApp.Accounts.{User, UserIdentity}
  alias MyApp.Repo

  use Pow.Ecto.Context,
    repo: Repo,
    user: User

  use PowAssent.Ecto.UserIdentities.Context,
    repo: Repo,
    user: User

  def authenticate(params) do
    pow_authenticate(params)
    |> load_identities()
  end

  def get_user_by_provider_uid(provider, uid, _conn) do
    pow_assent_get_user_by_provider_uid(provider, uid)
    |> load_identities()
  end

  def load_identities(%User{} = user), do: Repo.preload(user, :user_identities)
end

config

config :my_app, :pow,
  user: MyApp.Accounts.User,
  users_context: MyApp.Accounts,
  user_identities_context: MyApp.Accounts,
  repo: MyApp.Repo,
  web_module: MyAppWeb
praveenperera commented 4 years ago

Solved:

I had to change my config to:

config :my_app, :pow,
  user: MyApp.Accounts.User,
  users_context: MyApp.Accounts,
  repo: MyAppWeb.Repo,
  web_module: MyAppWebWeb

config :my_app, :pow_assent, user_identities_context: MyApp.Accounts

Again obvious in hindsight, but I'm wondering if its another opportunity to improve the docs?

danschultzer commented 4 years ago

Yeah, updated in https://github.com/pow-auth/pow_assent/commit/69964b1614d1540765cae883c6796fa8b5142af6.

FYI it's best to separate them into two contexts, since they work with two different tables.

For the user context you can also just rely on get_by/1 (per https://github.com/danschultzer/pow/issues/369#issuecomment-570746283):

defmodule MyApp.Accounts do
  alias MyApp.Accounts.User
  alias MyApp.Repo

  use Pow.Ecto.Context,
    repo: Repo,
    user: User

  @impl true
  def get_by(clauses) do
    clauses
    |> pow_get_by()
    |> preload_credential()
  end

  defp preload_user_identities(nil), do: nil
  defp preload_user_identities(user), do: Repo.preload(user, :user_identities)
end

For the user identities what you did works, but you probably also want it for create_user/3:

defmodule MyApp.UserIdentities do
  alias MyApp.Accounts.User
  alias MyApp.Repo

  use PowAssent.Ecto.UserIdentities.Context,
    repo: Repo,
    user: User

  @impl true
  def get_user_by_provider_uid(provider, uid) do
    provider
    |> pow_assent_get_user_by_provider_uid(uid)
    |> preload_user_identities()
  end

  @impl true
  def create_user(user_identity_params, user_params, user_id_params) do
    provider
    |> pow_create_user(user_identity_params, user_params, user_id_params)
    |> case do
      {:ok, user}         -> {:ok, preload_user_identities(user)}
      {:error, changeset} -> {:error, changeset}
    end
  end

  defp preload_user_identities(nil), do: nil
  defp preload_user_identities(user), do: Repo.preload(user, :user_identities)
end

Here's an alternative way of setting up config:

config :my_app, :pow,
  user: MyApp.Accounts.User,
  users_context: MyApp.Accounts,
  repo: MyApp.Repo,
  web_module: MyAppWeb,
  pow_assent: [
    user_identities_context: MyApp.UserIdentities
  ]
praveenperera commented 4 years ago

Cool thanks for all your help!

Here is what I have:

  @impl true
  def authenticate(params) do
    pow_authenticate(params)
    |> preload_user_identities()
  end

  @impl true
  def get_by(clauses) do
    clauses
    |> pow_get_by()
    |> preload_user_identities
  end

  @impl true
  def create(params) do
    params
    |> pow_create()
    |> case do
      {:ok, user} -> {:ok, preload_user_identities(user)}
      error -> error
    end
  end

  @impl true
  def get_user_by_provider_uid(provider, uid) do
    pow_assent_get_user_by_provider_uid(provider, uid)
    |> preload_user_identities()
  end

  @impl true
  def create_user(user_identity_params, user_params, user_id_params) do
    pow_assent_create_user(user_identity_params, user_params, user_id_params)
    |> case do
      {:ok, user} -> {:ok, preload_user_identities(user)}
      {:error, changeset} -> {:error, changeset}
    end
  end

I decided to keep them in the same context, because even though they deal with different tables it feels like the same domain (accounts).

I needed the authenticate() or it doesn't add the preload user_identities when logging in with email and password.

Only path this doesn't account for is if the user signs up, and then connects a user_identity, doing so doesn't refresh the user in the cache

praveenperera commented 4 years ago

Update

I was able to very easily always preload user_identities by copying the entire Pow.Plug.Session file over to a new file and in the create() function I added:

user = MyApp.Accounts.get_user(user.id)

where get_user is:

  def get_user(user_id) do
    User
    |> Repo.get(user_id)
    |> preload_user_identities()
  end

Doing so I was able to get rid of all the context code above

@danschultzer Do you think we could add this to the library somehow? I needed the user_identities to always be available because my header changes depending on if an oauth account is connected or now.

danschultzer commented 4 years ago

In this case the best way to deal with it is in a plug as specified in: https://hexdocs.pm/pow/1.0.16/sync_user.html#reload-the-user

You add it to your routes or endpoint so the user identities are always preloaded. You won't have to deal with old cache user, and you will have a much cleaner flow without custom contexts/session plug/etc.

For PowAssent.Plug.providers_for_current_user/1 which is used for the view helpers, I load all user identities when I need it to prevent these kind of issues.

praveenperera commented 4 years ago

I saw the PowAssent.Plug.providers_for_current_user/1 I tired to use that initially but I I needed to make a custom button.

Also I realized using PowAssent.Plug.providers_for_current_user/1 hits the database every time to load the user_identities

danschultzer commented 4 years ago

Also I realized using PowAssent.Plug.providers_for_current_user/1 hits the database every time to load the user_identities

The query will be cached so it should only hit the DB once.

praveenperera commented 4 years ago

Also I realized using PowAssent.Plug.providers_for_current_user/1 hits the database every time to load the user_identities

The query will be cached so it should only hit the DB once.

In that case maybe I should just avoid all of this and fetch it from the DB in the view like PowAssent.Plug.providers_for_current_user/1

How is it cached? Can I use the same cache?

Edit

I decided I was being dumb by trying to prematurely optimize, so now I'm just going to use PowAssent.Plug.providers_for_current_user/1 to get the providers for the user.

As far as I can tell though this hits the DB everytime.

[debug] QUERY OK source="user_identities" db=0.4ms idle=9232.5ms
SELECT u0."id", u0."access_token", u0."access_token_secret", u0."refresh_token", u0."username", u0."user_id", u0."provider", u0."uid", u0."inserted_at", u0."updated_at" FROM "user_identities" AS u0 WHERE (u0."user_id" = $1) [1]
danschultzer commented 4 years ago

Ecto deals with it. Here's some info how Ecto v3 handles the cache: https://dockyard.com/blog/2018/11/20/caching-in-ecto-v3-0

I assume it would still spit out the query debug message, but the latency would be much shorter for subsequent calls.

praveenperera commented 4 years ago

Hey my mistake I thought you meant cached in the credentials cache. I was aware of the Ecto Query cache, which is another reason it was dumb of me to try and optimize prematurely, when ecto is so fast.

Any way thanks for all your help, its greatly appreciated.

And amazing job on Pow. In my opinion this was the one thing missing in the elixir ecosystem for a while. This really handles authentication so nicely. Thank you

danschultzer commented 4 years ago

Thanks 😄