riverrun / phauxth

Not actively maintained - Authentication library for Phoenix, and other Plug-based, web applications
409 stars 20 forks source link

Phauxth and Umbrella Projects #22

Closed guidotripaldi closed 6 years ago

guidotripaldi commented 7 years ago

Hi David, I would like to use Phauxth in an advanced project that will have both html (for web users) and api (for Rest client App) authentication, and I'm wondering which is the best workflow/approach for setting up the Phauxth stuff.

Ideally, the umbrella project should have this kind of structure:

my_umbrella
|__ apps
    |__ core
    |__ core_web
    |__ phauxth
    |__ phauxth_api_web
    |__ phauxth_html_web

where core and core_web will contains the app specific stuff, phauxth will contain the shared Phauxth Ecto Context, phauxth_api_web contains the rest api authentication server, and phauxth_html_web contains the html frontend app server for accounts administration. I think that having two separated Phauxth app for html and api it should be easier to mantain than having a single hybrid app.

I would like to be able to use the Phoauxth installer to set up the Context, Api and Html CRUD and Tests stuff rather than doing it by hand, doing something similar to:

$ mix new my_umbrella --umbrella && cd my_umbrella/apps

$ mix phx.new.ecto core
$ mix phx.new.web core_web

$ mix phx.new.ecto phauxth
$ mix phx.new.web  phauxth_api_web --no-html --no-brunch
$ mix phx.new.web  phauxth_html_web

$ cd phauxth          && mix phauxth.new --ecto --confirm
$ cd phauxth_api_web  && mix phauxth.new --api  --confirm --context phauxth
$ cd phauxth_html_web && mix phauxth.new --html --confirm --context phauxth

But currently, the Phoauxth installer cannot be used neither for setting up an hybrid html+api app (as it hasn't a switch for specifying a desidered namespace) nor for setting up an umbrella app (if used with an umbrella app seems that it mess up its files).

Do you think that a) in a future the installer will support a sort of --namespace foo switch for namespacing the generated files, and b) that there will be a support for umbrella projects?

In the mean time, can you give us some hints for which can be the best workflow to setting up Phauxth things with the current version of the installer in an umbrella project in general and in particular like the one above?

riverrun commented 7 years ago

At the moment, I don't know much about how umbrella projects are structured, so it's something I need to look into more. I'll try to look into it some time over the next week.

riverrun commented 7 years ago

Just a note to say I haven't forgotten this. I'm just very busy at the moment.

guidotripaldi commented 7 years ago

Thank you David, in the mean time I'll try to arrange things manually

guidotripaldi commented 7 years ago

Hi David, working on adapting Phauxth in an Umbrella project, I noticed an incompatibility with the current way the configuration is retrieved.

Currently Phauxth.Config.endpoint and Phauxth.Config.token_salt retrieve the configuration by calling Application.get_env(:phauxth, :key)but since in an Umbrella project the apps configurations are merged and conflicting keys overridden, the configuration retrieved is not the correct one.

I've worked on a new branch resolving this issue by namespacing the configuration, can I open a PR so you can take a look at the proposed code?

riverrun commented 7 years ago

Yes, please :)

guidotripaldi commented 7 years ago

I've found another little incompatibility: currently, the generator produces the context module accounts/accounts.ex that contains the function

  def create_password_reset(attrs) do
    with %User{} = user <- get_by(attrs) do
      change(user, %{reset_sent_at: DateTime.utc_now}) |> Repo.update
      Log.info(%Log{user: user.id, message: "password reset requested"})
      Phauxth.Token.sign(MyAppWeb.Endpoint, attrs)
    end
  end

this is called in the PasswordResetController module:

  def create(conn, %{"password_reset" => %{"email" => email}}) do
    key = Accounts.create_password_reset(%{"email" => email})
    Message.reset_request(email, key)
    message = "Check your inbox for instructions on how to reset your password"
    success(conn, message, page_path(conn, :index))
  end

but in Umbrella projects the context is shared between all the applications, so we cannot hard code the endpoint in the context, as the line Phauxth.Token.sign(MyAppWeb.Endpoint, attrs) does.

I suggest these modifications:

in module accounts/accounts.ex we accept current_endpoint as a new input parameter, passing it to Phauxth.Token.sign instead of the hard coded value:

  def create_password_reset(current_endpoint, attrs) do
    with %User{} = user <- get_by(attrs) do
      change(user, %{reset_sent_at: DateTime.utc_now}) |> Repo.update
      Log.info(%Log{user: user.id, message: "password reset requested"})
      Phauxth.Token.sign(current_endpoint, attrs)
    end
  end

in module PasswordResetController pass the hard coded endpoint for our app to Accounts.create_password_reset as its first parameter :

  def create(conn, %{"password_reset" => %{"email" => email}}) do
    key = Accounts.create_password_reset( MyAppWeb.Endpoint, %{"email" => email} )
    Message.reset_request(email, key)
    message = "Check your inbox for instructions on how to reset your password"
    success(conn, message, page_path(conn, :index))
  end

PS Why we have to tell to Phauxth.Token.sign which the endpoint is, if Phauxth in other of its internal modules get it from the configuration? But maybe this approach could be followed also to tell the Plug which is the current endpoint, so this could be the solution for the namespaced config issue: instead to try to retrieve the parameters from the configuration, just pass them explicitly to the plug. What do you think?

riverrun commented 7 years ago

I've just added a pull request #31 which addresses the endpoint and token_salt issue. It also makes sure that the key generator options are properly passed on to the Phauxth.Token module.

If you get time, please take a look and let me know what you think.

riverrun commented 7 years ago

I have just added the changes you suggested to the installer templates to the raining branch.

guidotripaldi commented 7 years ago

Thanks David, I'll check the new branch this afternoon

riverrun commented 7 years ago

Well, the changes are in master now :)

guidotripaldi commented 7 years ago

Sorry for the delay but I was very busy these last days. I've just check the new version, but the Endpoint problem seems unresolved: without the namespacing, Phauxth gets the wrong endpoint because of the overriding of the conflicting keys in the config/config.exs files of the children apps, but maybe I've just not understand which kind of configuration have to be made in an umbrella project after PR#31 (I had non time to take a good look at its new code).

riverrun commented 7 years ago

I notice I didn't explain the changes very well -- I understood them :)

First of all, the config endpoint should only be referenced in the main config/config.exs file (or not referenced at all).

When you call Confirm.verify, you need to add the endpoint option to it:

Confirm.verify(params, MyApp.Accounts, endpoint: LittleWeb.Endpoint)

As for the token salt, all the modules that use tokens accept the token_salt option. If that is not present, the Token module will use the salt from the config.

Let me know if that works.

guidotripaldi commented 7 years ago

oh yes, now it is clear, thanks!! It works :)

riverrun commented 6 years ago

I think this can be closed now.