riverrun / phauxth

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

Issue with how report/3 is called from login/base.ex:86 #49

Closed joshchernoff closed 6 years ago

joshchernoff commented 6 years ago

I'm looking at how I can add custom login logic as shown by the Phauxth.Confirm.Login.

I've implemented my own custom login module which pulls in the base via use Phauxth.Login.Base

defmodule Beffect.Accounts.Login do
  use Phauxth.Login.Base
  alias Beffect.Accounts.User

  # This is the custom logic path I'm running into problems with. 
  def check_pass(%User{active: false}, _, _, _) do
    {:error, "account locked"}
  end

  # this custom logic works fine because the report method to handle that is already defined via 
  # https://github.com/riverrun/phauxth/blob/a61a2f3ac2d9a9334de571561dc0371af2484b8c/lib/phauxth/login/base.ex#L130

  def check_pass(%User{invite_confirmed_at: nil}, _, _, _) do
    {:error, "account unconfirmed"}
  end

  def check_pass(user, password, crypto, opts) do
    super(user, password, crypto, opts)
  end
end

The problem I'm running into is that if I want to output a custom message for my active: false path I run into the problem that I can't easily define the report handler function like the ones already in the Phauxth.Login.Base ones.

IE: If I try to add a report method like this to my Beffect.Accounts.Login I get an conflict compile time error.

defmodule Beffect.Accounts.Login do
   ....
     alias Phauxth.Log
     def report({:error, "account locked"}, _, meta) do
        Log.warn(%Log{message: "account locked", meta: meta})
        {:error,  "Account Locked!"}
    end
  ...
end

Produces: (CompileError) lib/beffect/accounts/login.ex:33: imported Phauxth.Login.Base.report/3 conflicts with local function

My question is:

1: where should I be defining the custom report/3 functions. 2: is there a way we can decouple the Phauxth.Login.Base methods from custom logic paths like Confirm. Not sure having the report methods there is the best place for them.

riverrun commented 6 years ago

Thanks for reporting this issue. I'll look into it and get back to you soon.

riverrun commented 6 years ago

I've added some changes to the master branch. I've made the report function overridable and updated the Confirm.Login module to use it. As you can see from the Confirm.Login example, it is not necessary to redefine all the report clauses - you just need to add your additional entries and then add an additional clause calling super on the arguments.

Let me know how that works and / or if you need any further information.

joshchernoff commented 6 years ago

Thank you very much. I will give some feed back once I get a chance to look into it. :)

joshchernoff commented 6 years ago

Nice so allowing for the override was what looks like needed to happen.

I made a simple example app showing how I'm trying to use a custom login module. https://github.com/digitalcake/active_user_example_phauxth/blob/master/lib/active_user_example_phauxth/accounts/login.ex

Let me know if that is inline with your design you had for how people would extend that base login module. ("aside from extracting the reports & messages to its own module ect..")

So far it looks like it works. Thanks for the help again.

riverrun commented 6 years ago

I just updated the hex package with the new changes.

Thanks for showing me the example. I have just one comment to make. For the report fallback function, you just need one clause, like this:

def report(result, ok_message, meta) do
    super(result, ok_message, meta)
end

The pattern matching is then done by the default implementation. This is how the new version of the Confirm.Login module works.