ueberauth / ueberauth

An Elixir Authentication System for Plug-based Web Applications
MIT License
1.62k stars 120 forks source link

Multiple paths per strategy no longer works #31

Closed r-gr closed 8 years ago

r-gr commented 8 years ago

I don't know if this was previously intentional behaviour but with v0.2.0 it was possible to configure a single provider multiple times to allow authentication at different paths:

config :ueberauth, Ueberauth,
  providers: [
    facebook: {Ueberauth.Strategy.Facebook, [
      request_path: "/auth/facebook",
      callback_path: "/auth/facebook/callback"
    ]},
    facebook: {Ueberauth.Strategy.Facebook, [
      request_path: "/signup/facebook",
      callback_path: "/signup/facebook/callback"
    ]}
  ]

Updating to v0.3.0, this no longer works. The provider defined first appears to be overridden by the latter. This results in an UndefinedFunctionError when accessing /auth/facebook, but /signup/facebook works as expected.

So as I said, I'm not sure if this was previously an intentional design choice, making this new behaviour a regression or if the config above worked in v0.2.0 purely by chance. Either way, I have found it to be important in order to logically separate different parts of the application. The signup flow can be very different to that of logging in and so it makes sense to handle them separately.

Looking at the commit history, it appears this change in behaviour was introduced at 22e072898991697b699acc382168fe40500dcbc5. Specifically on line 179 of lib/ueberauth.ex, where the use of all_providers = Enum.into(all_providers, %{}) removes all duplicate keys from the keyword list of providers by conversion to a map. (https://github.com/ueberauth/ueberauth/commit/22e072898991697b699acc382168fe40500dcbc5#diff-3f81f8ab5bb0951f0fa5db7604b4b9b3R179).

It looks like this is done so that Map.split/2 can be used (https://github.com/ueberauth/ueberauth/commit/22e072898991697b699acc382168fe40500dcbc5#diff-3f81f8ab5bb0951f0fa5db7604b4b9b3R184) but in this case, Keyword.split/2 accomplishes the same thing while preserving the duplicate keys.

What are your thoughts on this?

doomspork commented 8 years ago

@hassox was this change intentional? I'm assuming this is a bug.