From 6b6248c90933901006f500e1108a17006093c491 Mon Sep 17 00:00:00 2001 From: Dan Schultzer Date: Sat, 25 May 2019 10:30:42 -0700 Subject: [PATCH 1/5] Filter duplicate routes --- CHANGELOG.md | 1 + .../email_confirmation/phoenix/router.ex | 4 +- lib/extensions/invitation/phoenix/router.ex | 4 +- .../reset_password/phoenix/router.ex | 6 ++- lib/pow/phoenix/router.ex | 40 ++++++++++++++++++- test/pow/extension/phoenix/router_test.exs | 14 ++++++- test/pow/phoenix/router_test.exs | 22 ++++++++++ 7 files changed, 83 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 201d014c..31d8c784 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## v1.0.9 (TBA) +* `Pow.Phoenix.Router` will now only add specific routes if there is no matching route already defined * Removed call to `Pow.Ecto.Context.repo/1` ## v1.0.8 (2019-05-24) diff --git a/lib/extensions/email_confirmation/phoenix/router.ex b/lib/extensions/email_confirmation/phoenix/router.ex index afec8055..8a0a501b 100644 --- a/lib/extensions/email_confirmation/phoenix/router.ex +++ b/lib/extensions/email_confirmation/phoenix/router.ex @@ -2,9 +2,11 @@ defmodule PowEmailConfirmation.Phoenix.Router do @moduledoc false use Pow.Extension.Phoenix.Router.Base + alias Pow.Phoenix.Router + defmacro routes(_config) do quote location: :keep do - resources "/confirm-email", ConfirmationController, only: [:show] + Router.pow_resources "/confirm-email", ConfirmationController, only: [:show] end end end diff --git a/lib/extensions/invitation/phoenix/router.ex b/lib/extensions/invitation/phoenix/router.ex index 4d88712c..cfcfb045 100644 --- a/lib/extensions/invitation/phoenix/router.ex +++ b/lib/extensions/invitation/phoenix/router.ex @@ -2,9 +2,11 @@ defmodule PowInvitation.Phoenix.Router do @moduledoc false use Pow.Extension.Phoenix.Router.Base + alias Pow.Phoenix.Router + defmacro routes(_config) do quote location: :keep do - resources "/invitations", InvitationController, only: [:new, :create, :show, :edit, :update] + Router.pow_resources "/invitations", InvitationController, only: [:new, :create, :show, :edit, :update] end end end diff --git a/lib/extensions/reset_password/phoenix/router.ex b/lib/extensions/reset_password/phoenix/router.ex index ac38bf14..2d086803 100644 --- a/lib/extensions/reset_password/phoenix/router.ex +++ b/lib/extensions/reset_password/phoenix/router.ex @@ -2,10 +2,12 @@ defmodule PowResetPassword.Phoenix.Router do @moduledoc false use Pow.Extension.Phoenix.Router.Base + alias Pow.Phoenix.Router + defmacro routes(_config) do quote location: :keep do - resources "/reset-password", ResetPasswordController, only: [:new, :create, :update] - get "/reset-password/:id", ResetPasswordController, :edit + Router.pow_resources "/reset-password", ResetPasswordController, only: [:new, :create, :update] + Router.pow_route :get, "/reset-password/:id", ResetPasswordController, :edit end end end diff --git a/lib/pow/phoenix/router.ex b/lib/pow/phoenix/router.ex index 6719694d..0c4c98b4 100644 --- a/lib/pow/phoenix/router.ex +++ b/lib/pow/phoenix/router.ex @@ -68,7 +68,7 @@ defmodule Pow.Phoenix.Router do defmacro pow_session_routes do quote location: :keep do pow_scope do - resources "/session", SessionController, singleton: true, only: [:new, :create, :delete] + unquote(__MODULE__).pow_resources "/session", SessionController, singleton: true, only: [:new, :create, :delete] end end end @@ -77,7 +77,43 @@ defmodule Pow.Phoenix.Router do defmacro pow_registration_routes do quote location: :keep do pow_scope do - resources "/registration", RegistrationController, singleton: true, only: [:new, :create, :edit, :update, :delete] + unquote(__MODULE__).pow_resources "/registration", RegistrationController, singleton: true, only: [:new, :create, :edit, :update, :delete] + end + end + end + + @doc false + defmacro pow_resources(path, controller, opts) do + quote location: :keep do + opts = unquote(__MODULE__).__filter_resource_actions__(@phoenix_routes, __ENV__.line, __ENV__.module, unquote(path), unquote(controller), unquote(opts)) + + resources unquote(path), unquote(controller), unquote(opts) + end + end + + @doc false + def __filter_resource_actions__(phoenix_routes, line, module, path, controller, options) do + resource = Phoenix.Router.Resource.build(path, controller, options) + action_verbs = [index: :get, new: :get, create: :post, show: :get, edit: :get, update: :patch] + only = Enum.reject(resource.actions, &__route_defined__(phoenix_routes, line, module, action_verbs[&1], path, controller, &1, options)) + + Keyword.put(options, :only, only) + end + + @doc false + def __route_defined__(phoenix_routes, line, module, verb, path, plug, plug_opts, options) do + matching_params = + line + |> Phoenix.Router.Scope.route(module, :match, verb, path, plug, plug_opts, options) + |> Map.take([:opts, :helper]) + + Enum.any?(phoenix_routes, &Map.take(&1, [:opts, :helper]) == matching_params) + end + + defmacro pow_route(verb, path, plug, plug_opts, options \\ []) do + quote location: :keep do + unless unquote(__MODULE__).__route_defined__(@phoenix_routes, __ENV__.line, __ENV__.module, unquote(verb), unquote(path), unquote(plug), unquote(plug_opts), unquote(options)) do + unquote(verb)(unquote(path), unquote(plug), unquote(plug_opts), unquote(options)) end end end diff --git a/test/pow/extension/phoenix/router_test.exs b/test/pow/extension/phoenix/router_test.exs index 236f2d02..4b55dd06 100644 --- a/test/pow/extension/phoenix/router_test.exs +++ b/test/pow/extension/phoenix/router_test.exs @@ -1,9 +1,11 @@ defmodule Pow.Test.Extension.Phoenix.Router.Phoenix.Router do use Pow.Extension.Phoenix.Router.Base + alias Pow.Phoenix.Router + defmacro routes(_config) do quote location: :keep do - resources "/test", TestController, only: [:new, :create, :edit, :update] + Router.pow_resources "/test", TestController, only: [:new, :create, :edit, :update] end end end @@ -14,6 +16,10 @@ defmodule Pow.Test.Extension.Phoenix.Router do use Pow.Extension.Phoenix.Router, extensions: [Pow.Test.Extension.Phoenix.Router] + scope "/", as: "pow_test_extension_phoenix_router" do + get "/test/:id/overidden", TestController, :edit + end + scope "/" do pow_routes() pow_extension_routes() @@ -51,11 +57,15 @@ defmodule Pow.Extension.Phoenix.RouterTest do test "has routes" do assert unquote(Routes.pow_session_path(@conn, :new)) == "/session/new" - assert unquote(Routes.pow_test_extension_phoenix_router_test_path(@conn, :new)) = "/test/new" + assert unquote(Routes.pow_test_extension_phoenix_router_test_path(@conn, :new)) == "/test/new" end test "validates no aliases" do assert unquote(module_raised_with) =~ "Pow routes should not be defined inside scopes with aliases: Test" assert unquote(module_raised_with) =~ "scope \"/\", Test do" end + + test "can override routes" do + assert unquote(Routes.pow_test_extension_phoenix_router_test_path(@conn, :edit, 1)) == "/test/1/overidden" + end end diff --git a/test/pow/phoenix/router_test.exs b/test/pow/phoenix/router_test.exs index 1f4d4174..f44ff192 100644 --- a/test/pow/phoenix/router_test.exs +++ b/test/pow/phoenix/router_test.exs @@ -15,12 +15,34 @@ module_raised_with = _ -> raise "Scope with alias didn't throw any error" end +defmodule Pow.Test.Phoenix.OverriddenRouteRouter do + use Phoenix.Router + use Pow.Phoenix.Router + + scope "/", Pow.Phoenix, as: "pow" do + get "/registration/overidden", RegistrationController, :new + end + + scope "/" do + pow_routes() + end +end + defmodule Pow.Phoenix.RouterTest do use ExUnit.Case doctest Pow.Phoenix.Router + alias Pow.Test.Phoenix.OverriddenRouteRouter.Helpers, as: OverriddenRoutes + alias Phoenix.ConnTest + + @conn ConnTest.build_conn() + test "validates no aliases" do assert unquote(module_raised_with) =~ "Pow routes should not be defined inside scopes with aliases: Test" assert unquote(module_raised_with) =~ "scope \"/\", Test do" end + + test "can override routes" do + assert unquote(OverriddenRoutes.pow_registration_path(@conn, :new)) == "/registration/overidden" + end end From 1b20c90900f514716b1461983d9e964c82375227 Mon Sep 17 00:00:00 2001 From: humancopy Date: Wed, 29 May 2019 17:34:45 +0200 Subject: [PATCH 2/5] No need to unquote opts --- lib/pow/phoenix/router.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pow/phoenix/router.ex b/lib/pow/phoenix/router.ex index 0c4c98b4..cb41477a 100644 --- a/lib/pow/phoenix/router.ex +++ b/lib/pow/phoenix/router.ex @@ -87,7 +87,7 @@ defmodule Pow.Phoenix.Router do quote location: :keep do opts = unquote(__MODULE__).__filter_resource_actions__(@phoenix_routes, __ENV__.line, __ENV__.module, unquote(path), unquote(controller), unquote(opts)) - resources unquote(path), unquote(controller), unquote(opts) + resources unquote(path), unquote(controller), opts end end From b108689f00af5a4ae3359b9a4eeae12510759d0d Mon Sep 17 00:00:00 2001 From: Dan Schultzer Date: Wed, 29 May 2019 04:21:35 -0700 Subject: [PATCH 3/5] Test for filtered routes --- test/pow/phoenix/router_test.exs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/pow/phoenix/router_test.exs b/test/pow/phoenix/router_test.exs index f44ff192..4d55e497 100644 --- a/test/pow/phoenix/router_test.exs +++ b/test/pow/phoenix/router_test.exs @@ -16,6 +16,8 @@ module_raised_with = end defmodule Pow.Test.Phoenix.OverriddenRouteRouter do + @moduledoc false + use Phoenix.Router use Pow.Phoenix.Router @@ -26,6 +28,8 @@ defmodule Pow.Test.Phoenix.OverriddenRouteRouter do scope "/" do pow_routes() end + + def phoenix_routes(), do: @phoenix_routes end defmodule Pow.Phoenix.RouterTest do @@ -44,5 +48,6 @@ defmodule Pow.Phoenix.RouterTest do test "can override routes" do assert unquote(OverriddenRoutes.pow_registration_path(@conn, :new)) == "/registration/overidden" + assert Enum.count(Pow.Test.Phoenix.OverriddenRouteRouter.phoenix_routes(), &(&1.plug == Pow.Phoenix.RegistrationController && &1.opts == :new)) == 1 end end From d88e9fbfc672cacc3fe0a6a4760cb6eb7941be0b Mon Sep 17 00:00:00 2001 From: Dan Schultzer Date: Wed, 29 May 2019 19:44:27 -0700 Subject: [PATCH 4/5] Update docs --- lib/pow/phoenix/router.ex | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/pow/phoenix/router.ex b/lib/pow/phoenix/router.ex index cb41477a..cd8ee35f 100644 --- a/lib/pow/phoenix/router.ex +++ b/lib/pow/phoenix/router.ex @@ -2,6 +2,14 @@ defmodule Pow.Phoenix.Router do @moduledoc """ Handles Phoenix routing for Pow. + Resources are build with `pow_resources/3` and individual routes are build + with `pow_route/5`. The Pow routes will be filtered if a route has already + been defined with the same action and router helper alias. This makes it easy + to override pow routes with no conflicts. + + The scope will be validated to ensure that there is no aliases. An exception + will be raised if an alias was defined in any scope around the pow routes. + ## Usage Configure `lib/my_project_web/router.ex` the following way: @@ -36,9 +44,10 @@ defmodule Pow.Phoenix.Router do end @doc """ - Pow router macro. + Pow routes macro. - Use this macro to define the Pow routes. + Use this macro to define the Pow routes. This will call + `pow_session_routes/0` and `pow_registration_routes/0`. ## Example From f4256e6aa6d02a6c36879616935854ab135dc631 Mon Sep 17 00:00:00 2001 From: Dan Schultzer Date: Thu, 30 May 2019 07:20:57 -0700 Subject: [PATCH 5/5] Test that the extension router only has one route when overridden --- test/pow/extension/phoenix/router_test.exs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/pow/extension/phoenix/router_test.exs b/test/pow/extension/phoenix/router_test.exs index 4b55dd06..667b0e9b 100644 --- a/test/pow/extension/phoenix/router_test.exs +++ b/test/pow/extension/phoenix/router_test.exs @@ -24,6 +24,8 @@ defmodule Pow.Test.Extension.Phoenix.Router do pow_routes() pow_extension_routes() end + + def phoenix_routes(), do: @phoenix_routes end module_raised_with = @@ -67,5 +69,6 @@ defmodule Pow.Extension.Phoenix.RouterTest do test "can override routes" do assert unquote(Routes.pow_test_extension_phoenix_router_test_path(@conn, :edit, 1)) == "/test/1/overidden" + assert Enum.count(Pow.Test.Extension.Phoenix.Router.phoenix_routes(), &(&1.plug == TestController && &1.opts == :edit)) == 1 end end