Should config be passed to custom users context?

[danschultzer]

Currently Pow.Operations only passes the config to the Pow.Ecto.Context module, but not any custom context module:

  @spec authenticate(map(), Config.t()) :: map() | nil
  def authenticate(params, config) do
    case context_module(config) do
      Context -> Context.authenticate(params, config)
      module  -> module.authenticate(params)
    end
  end

The reasoning for this was that normally ecto modules don't have any additional params, and I would like it to be clear for developers that this is just default ecto (and not dependent on Pow syntax).

However, this may be problematic as seen here: https://elixirforum.com/t/pow-robust-modular-extendable-user-authentication-and-management-system/15807/155?u=danschultzer

Would you be open to a PR that conditionally supplies the config to a custom user_context if the functions support the extra argument? It seems strange to me that Pow.Ecto.Context receives the argument but an implementation of it doesn’t.

I've to think about this one to see whether this is necessary or not. An overridable entry method could be created in the macro, so a custom context would look like this (looks ugly though):

  @callback do_authenticate(map(), Config.t()) :: user() | nil

  defmacro __using__(config) do
    quote do
      @behaviour unquote(__MODULE__)

      @pow_config unquote(config)

     def do_authenticate(params, config), do: authenticate(params)

      def authenticate(params), do: pow_authenticate(params)
      # ...

      def pow_authenticate(params) do
        unquote(__MODULE__).authenticate(params, @pow_config)
      end

      # ...

      defoverridable unquote(__MODULE__)
    end
  end

And then call it in the operations:

  @spec authenticate(map(), Config.t()) :: map() | nil
  def authenticate(params, config) do
    case context_module(config) do
      Context -> Context.authenticate(params, config)
      module  -> module.do_authenticate(params, config)
    end
  end

Alternatively a conditional check could be handled (kinda ugly too, but it's nice that it's contained to just the operations module):

  @spec authenticate(map(), Config.t()) :: map() | nil
  def authenticate(params, config) do
    case context_module(config) do
      Context -> Context.authenticate(params, config)
      module  -> if Kernel.function_exported?(module, :authenticate, 2),
        do:  module.authenticate(params, config),
        else: module.authenticate(params)
    end
  end

[jgwmaxwell]

Sometimes I do wish you could just slice *args by arity and be done with it :)

The advantage of the former, despite it being ugly within the macro, is that it allows strict implementation checks, whereas the latter approach is a bit more implicit and hacky.

The other option here would be to refactor the functions to use Ecto.Query, this would then allow a similar approach to the current repo_opts of passing in a default scope if you like. This would be the least disruptive approach to current implementations and IMHO would be the most elegant.

  @doc """
  Retrieves an user by the provided clauses.

  User schema module and repo module will be fetched from the config.
  """
  @spec get_by(Keyword.t() | map(), Config.t()) :: user() | nil
  def get_by(clauses, config) do
    user_mod = Config.user!(config)
    clauses  = user_mod
      |> normalize_user_id_field_value(clauses)
      |> Keyword.merge(config |> Config.get(:scope_opts, []))
    opts     = repo_opts(config, [:prefix])

    from(u in user_mod, where: ^clauses) |> Config.repo!(config).one(opts)
  end

It would need similar approaches for the other methods, but this would prevent needing any change to the API and allow some nice customisation. Obviously scope_opts or whatever name would be good could accommodate multiple namespaces if needed.

If you're ok with this approach then I'd be very happy to submit a PR for it.

[jgwmaxwell]

I think probably it would be ok just to do the above - update/create as you mentioned on the forum could easily be handled by the form submissions I think.

[danschultzer]

Yeah, I think the first option is the best of the bunch. Will have to give it some more thought though as I think there may be a better alternative.

As for the config example with Ecto.Query, I try to stay away from config options as much possible. I prefer configuration to be explicit, which means configuration should be done in the code rather than config. It may introduce a bit more code, but it's much easier to keep track and understand the flow of your app, and enforces testing that your app works in the configured way 😄

[danschultzer]

Oh, and just to document this, if form inputs is a good option, then something like this should work:

defmodule MyApp.Users do
  @behavior Pow.Ecto.Context

  alias MyApp.{Accounts, Users.User}

  @impl true
  def authenticate(params) do
    params["account"]
    |> Accounts.get()
    |> account_authenticate(params)
  end

  defp account_authenticate(nil, _params), do: nil
  defp account_authenticate(account, params) do
    User
    |> Repo.get_by(email: params["email"], account_id: account.id),
    |> case do
      nil  -> %User{password_hash: nil} # Prevent timing attack
      user -> user
    end
    |> User.verify_password(params["password"])
  end

  @impl true
  def create(params) do
    params["account"]
    |> Accounts.get()
    |> Accounts.create_user(params)
  end

  # ...
end

<%= form_for @changeset, Routes.pow_registration_path(@conn, :create), fn f -> %>
  <%= hidden_input f, :account, @account.id %>

  <%= label f, Pow.Ecto.Schema.user_id_field(@changeset) %>
  <%= text_input f, Pow.Ecto.Schema.user_id_field(@changeset) %>
  <%= error_tag f, Pow.Ecto.Schema.user_id_field(@changeset) %>

  <%= label f, :password, "password" %>
  <%= password_input f, :password %>
  <%= error_tag f, :password %>

  <%= label f, :confirm_password, "confirm_password" %>
  <%= password_input f, :confirm_password %>
  <%= error_tag f, :confirm_password %>

  <%= submit "Register" %>
<% end %>

[jgwmaxwell]

There's a lot to be said for explicit configuration, I definitely agree with you there. Thanks for the code example re: the form, I definitely agree that would work for those interactions (and could be validated for non-change using the existing Controller hooks).

I suppose this does really just come down to ensuring that retrieving that Pow picks the right user record from the available options. Let me know if you'd like any code based around your first option!

[danschultzer]

Let me know if you'd like any code based around your first option!

Sure, go ahead! I'll just warn you that there may be some pitfalls I haven't thought of yet and it may be tricky to set up. Having a working version will give us a much better idea whether it is a good solution.

[danschultzer]

I think I got an alternative that will keep things cleaner since it'll just be added on top of the current setup rather than requiring any changes to the current flow. It also requires you to be explicit.

The :users_context will be changed so it is permitted to use tuple value like {MyApp.Users, opts}. The second value will be a keyword list with custom options. In the Pow.Operations config will be pushed to that list:

  @spec authenticate(map(), Config.t()) :: map() | nil
  def authenticate(params, config) do
    case context_module(config) do
      Context        -> Context.authenticate(params, config)
      {module, opts} -> module.authenticate(params, Keyword.put_new(opts, :config, config))
      module         -> module.authenticate(params)
    end
  end

I'll detail something like this in the docs:

For complex configurations you may need to access the configuration from the operation rather than using the one from the macro, or you may need to pass additional options. You can do this the following way:

  defmodule MyApp.Users do
    @behaviour Pow.Ecto.Context.Opts

    @impl true
    def create(params, opts) do
      config = opts[:config]

      Pow.Ecto.Context.create(params, config)
    end
  end

And there will be an explicit behaviour for it:

defmodule Pow.Ecto.Context.Opts do
  @doc false

  @callback authenticate(map(), keyword()) :: user() | nil
  @callback create(map(), keyword()) :: {:ok, user()} | {:error, changeset()}
  @callback update(user(), map(), keyword()) :: {:ok, user()} | {:error, changeset()}
  @callback delete(user(), keyword()) :: {:ok, user()} | {:error, changeset()}
  @callback get_by(Keyword.t() | map(), keyword()) :: user() | nil
end

You can also have multiple :users_context settings with different opts this way if needed.

[escobera]

@danschultzer this would be really usefull for me.

I'm using multitenancy without database schemas, I may have the same email for different users on different tenants (differentiator being a foreign_key field on my accounts table).

Right now this seems impossible to do in Pow as I have no way of merging the tenant clause to get_by based on config.