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

Configurable confirmation URL + IPs stored in plain format #328

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

sashman
Copy link

@sashman sashman commented Feb 6, 2018

To configure confirmation URL

config :coherence,
  external_hostname: {:system, "EXTERNAL_HOSTNAME"}, # Set external hostname, e.g.: http://localhost:4000
  confimarion_url: ":external_hostname/#/user/confirm/:token", # Can use either hardcoded hostname or :external_hostname, and use :token to embed the generated token

@smpallen99
Copy link
Owner

Can you please explain the use case for using the "EXTERNAL_HOSTNAME" env variable? The system already creates the url based on the Endpoint's http: [host: "..."] and https: [host: "..."] configuration. So I don't see the need to have a separate env variable. Am I missing something?

@sashman
Copy link
Author

sashman commented Apr 15, 2018

Hey @smpallen99, thanks for replying!

This env variable might not have the best name, but in my use case, this arises from using the coherence in a microservice. So what I end up with is http: [host: "..."] and https: [host: "..."] pointing to the internal hostname of the microservice, which the user cannot navigate from the outside, only the front end service is publicly accessible, This is why I had to add a separate host variable to point to this public endpoint.

Hope this makes sense. I'm more than willing to change this to a more sensible solution in case I have misunderstood something.

@sashman
Copy link
Author

sashman commented May 11, 2018

Hey @smpallen99, any thoughts on this?

@smpallen99
Copy link
Owner

Sorry for the delay getting back to you on this @sashman. I've reviewed the PR and have some comments:

  • Why have you have only implemented this for the confirmation_url? If I understand the feature correctly, wouldn't you want all email links to be translated?
  • Adding a extra xxx_url's to the config just seems a little off.
  • There is a fairly elegant way (IMHO) to implement this in a project without any changes to coherence. See my example below.
  • The default should taken from the existing project. See the external_url/1 function from my example below.
  • This is a very specific case, and if there was community demand for such a feature, I would like to see it done a little more generically.

I just tried this in a test project with my master branch. The only file that needs changing is lib/coh_mass_assign_web/emails/coherence/user_email.ex.

Code.ensure_loaded Phoenix.Swoosh

defmodule CohMassAssignWeb.Coherence.UserEmail do
  @moduledoc false
  use Phoenix.Swoosh, view: CohMassAssignWeb.Coherence.EmailView, layout: {CohMassAssignWeb.Coherence.LayoutView, :email}
  alias Swoosh.Email
  require Logger
  alias Coherence.Config
  import CohMassAssignWeb.Gettext

  defp site_name, do: Config.site_name(inspect Config.module)

  def password(user, url) do
    Logger.info "reset url: #{inspect url}"
    url = external_url(url)  # add this line to each of the mail functions
    Logger.info "reset external url: #{inspect url}"
    %Email{}
    |> from(from_email())
    |> to(user_email(user))
    |> add_reply_to()
    |> subject(dgettext("coherence", "%{site_name} - Reset password instructions", site_name: site_name()))
    |> render_body("password.html", %{url: url, name: first_name(user.name)})
  end

  def confirmation(user, url) do
    url = external_url(url) 
   # ...
  end

  # ...

  defp external_url(url) do
    base_url = CohMassAssignWeb.Router.Helpers.url(CohMassAssignWeb.Endpoint)
    String.replace(url, base_url, Application.get_env(:coh_mass_assign, :external_url, base_url))
  end
end

By adding a :external_url config item like "https://my.external.domain", or "https://10.10.10.10:4000" to your projects field, I believe you can get the behaviour you're looking for.

What do you think?
Steve

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants