Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Igniters generate invalid content for non-Phoenix sender modules #814

Open
sevenseacat opened this issue Oct 24, 2024 · 2 comments
Open
Labels
bug Something isn't working

Comments

@sevenseacat
Copy link
Member

When not using Phoenix, igniters such as ash_authentication.add_strategy password will generate email sender modules like this:

defmodule TestAaEmails.Accounts.User.Senders.SendPasswordResetEmail do
  @moduledoc """
  Sends a password reset email
  """

  use AshAuthentication.Sender

  @impl true
  def send(_user, token, _) do
    IO.puts("""
    Click this link to reset your password:

    #{url(~p"/password-reset/#{token}")}
    """)
  end
end

This is invalid because the url function and p sigil are both provided by Phoenix.VerifiedRoutes.

The p sigil can be removed for the non-Phoenix version of the module, leaving a plain string, but how to fully-qualify the URL properly?

@sevenseacat sevenseacat added the bug Something isn't working label Oct 24, 2024
@zachdaniel
Copy link
Collaborator

zachdaniel commented Oct 24, 2024

So, I've been thinking about this, and I think there is a design flaw. This was actually pointed out by a customer of Alembic's as well. These senders are "web-aware". In phx.gen.auth, that is not the case. They pass down a url which the internals then use to send.

The reason this isn't great for us is because we define the controllers. Certain setups get factually more difficult because of this choice (like if the URL is not static but dependent on a subdomain for tenancy, for example).

@jimsynz had originally used route helpers for this, which allowed it to be derived from the routes in the router. But that was soft deprecated in Phoenix in favor of verified routes, but in this case verified routes is actually crossing the boundary of app -> web in the wrong direction.

We probably need to come up with a backwards compatible idiomatic solution. Probably some kind of additional callback in the auth controller, and if that is defined we pass that down to the senders. like

def url_for(conn, :password, {:reset, token}) do
    ~p"/reset_password/#{token}"
end

def url_for(_, _, _), do: :default

Where default will just be the current host + a standard route for that thing.

Then, when calling actions with senders, they look for a context that is set called ash_authentication: %{url: <url>}, then pass that into the senders.

It sounds a bit involved, but I think phx.gen.auth has the right of it in going through the hoops that avoid this dependency.

@jimsynz what do you think?

@zachdaniel
Copy link
Collaborator

zachdaniel commented Oct 25, 2024

I think the simplest answer here is to actually generate the actions with optional url arguments, which then get passed to the senders as opts. Old implementations will continue to work, but can upgrade to use the provided URL. Then, we add def url_for/3 for generating URLs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants