Extend web controller macro

I am trying to move login check from router to controller level and the idea is

  1. Add a compile-time variable to a controller
def MyAppWeb.FooBarController do
  @public_actions [:index, :show]
  use MyAppWeb, :controller
  ... 
  def index(conn, paras) do
    ... 
  end
end
  1. Add the check to macro like this
def controller do
  quote do
    use Phoenix.Controller, namespace: MyAppWeb
    import Plug.Conn
    import MyAppWeb.Router.Helpers
    import MyAppWeb.Gettext
    plug MyAppWeb.Plug.Authorize when action not in Module.get_attribute(__MODULE__, :public_actions, [])
 end
end

I am getting

* (ArgumentError) invalid right argument for operator "in", it expects a compile-time proper list or compile-time range on the right side when used in guard expressions, got: Module.get_attribute(__MODULE__, :public_actions, [])

And can’t figure out what I am missing. The @ variable defined in controller is compile time, so it should be accessible.
I tried to specify it directly like

@pub_right_here []
plug MyAppWeb.Plug.Authorize when action not in @pub_right_here

getting even worse error

cannot find or invoke local action/0 inside guard. Only macros can be invoked in a guard and they must be defined before their invocation. Called as: action()

Any meaningful suggestions are welcome.
Please, do not suggest use pipe_through.

First error says that you can use only functions allowed in guards (look for https://hexdocs.pm/elixir/guards.html#list-of-allowed-expressions)
And I don’t see why second variant is not working.

But you can achieve the same behaviour using init options instead of guards:

defmodule AuthPlug do
  def init(opts), do: opts

  def call(conn, [actions: actions]) do
    if conn.private.phoenix_action not in actions do
      conn
      |> put_status(:forbidden)
      |> Plug.Conn.halt()
    else
      conn
    end
  end
end

@pub_right_here [:index]
plug AuthPlug, actions: @pub_right_here
1 Like

The problem underneath is a bit more complex. I am trying to avoid extra explicitness by macros. I have only 30 from barely 500 routes that I have to open, so by default it should be closed. I think that it would be better to control plugs behaviour from the controller level rather than put plug call everywhere. I mentioned do not suggest the pipe_through because it is a messy and difficult to read the routes file when it is looks like public/private mom’s spaghetti :sweat_smile:

Regarding the guards, it has some tricky part that I didn’t figure out because of Marco complexity inside the framework itself which is not that obvious :slight_smile: Inside the pipeline there is some compilation algorithm which allows to use action as macro inside the guard in your controller, but at compile time it is not available yet. I believe only the main contributors could answer my question and tell is it possible to achieve at all.

The current workaround that I “invented” is

def MyAppWeb.FooBarController do
  @public_actions [:index, :show]
  use MyAppWeb, :controller
  ... 
  def index(conn, paras) do
    ... 
  end
end

Plus

def controller do
  quote do
    ...
    # it is here to access the value if was set otherwise compiler creates warnings. A lot.
    @internal_public_actions Module.get_attribute(__MODULE__, :public_actions, [])
    plug :do_the_validate_check
    defp do_the_validate_check(conn, opts) do
      conn = if action_name(conn) not in @internal_public_actions do
        conn = Authorize.call(conn, [])
      end || conn
      conn
    end
  end
end

I didn’t suggest the usage of pipe_through. I suggested instead of using some compilation algorithm which allows to use action to pass options to plug as is and check action inside the plug.

Snipped from my comment earlier goes to controller definition too:

def controller do
  quote do
    ...
    @pub_right_here [:index]
    plug AuthPlug, actions: @pub_right_here
 end
end

And it’s roughly the same as you did, but allows you to extract code from controller definition somewhere else.

The full story is something like this:

Controller:

def MyAppWeb.FooBarController do
  @public_actions [:index, :show]
  use MyAppWeb, :controller
  ... 
  def index(conn, paras) do
    ... 
  end
end

Controller definition:

def controller do
  quote do
    ...
    plug AuthPlug, actions: @public_actions
 end
end

Plug:

defmodule AuthPlug do
  def init(opts), do: opts

  def call(conn, [actions: actions]) do
    if conn.private.phoenix_action not in actions do
      conn
      |> put_status(:forbidden)
      |> Plug.Conn.halt()
    else
      conn
    end
  end
end

I assume it’s something like this: warning: undefined module attribute @some, please remove access to @some or explicitly set it before access

I think this link can be useful - https://hexdocs.pm/elixir/Module.html#has_attribute?/2

You surely could move the with logic into the controllers but I would like to ask a question: where would you look if wanted to know which routes require auth and which do not?

Moving auth into controllers would mean that you would have to open each and every controller to get this information. With 5 controllers that might be doable, but what about 10, 20, 30?

Pipelines definitely are the idiomatic approach to do this, and to combat the “spaghetti” routes you might want to consider using nested scopes (we used that approach quite successfully in a big project):

pipeline :browser do
  # ...
end

pipeline :authenticated do
  # ...
end

scope "/", YourWebApp do
  pipe_through :browser

  # public routes here

  scope "/" do
    pipe_through :authenticated

    # private routes here
  end
end

Another alternative is to pass some kind of “requires auth” field to the private assigns in the route and use that in an auth plug.

get "/requires-auth", YourController, :index, private: [requires_auth: true]
1 Like

The scoping is exactly what I am trying to avoid because it leads for errors in my case that I always forget that there is a route somewhere at the bottom in MyAppWeb.Router.

The requirement is to open specific routes only, like :index and :show. The router file uses resources macro everywhere and passing route opts for each get/post will require to rewrite the router file. Here I am raising both hands for if it works then don’t touch anything.

get "/requires-auth", YourController, :index, private: [requires_auth: true]

I am not sure that it is possible. Currently using Phoenix 1.3 and it gives a compilation error. May be things changed but AFAIK the whole ecosystem restricts options lists to be the correct list or simply ignores non matching atoms. The same is applicable to Ecto.Changeset and I think would be cool to be able to pass some private options instead of creating libs for filters like filtrex with similar DSL :man_shrugging:

where would you look if wanted to know which routes require auth and which do not

I understand the point that better look into a single place instead of having auth smashed between controllers. But shouldn’t the problem just to search for @public_actions.

Sorry, didn’t mean to heart anybody. It was kind of extra information to avoid multiple plugs calls. And I didn’t understand fully where you intended to place the plug call.

True. But the plug call is runtime so if I specify @public_actions directly it gives this warning. To omit it I used a “proxy” compile-time variable

@internal_public_actions Module.get_attribute(__MODULE__, :public_actions, [])
1 Like

Did a little research on

 get "/requires-auth", YourController, :index, private: [requires_auth: true]

Seems it takes a map to merge existing :private with custom map.

get "/requires-auth", YourController, :index, private: %{requires_auth: true}

Also tried with resources and got my map in :private

resources "/pages", PageController, [only: [:index, :show], private: %{foobar: :not_today1}]

This is definitely solves my problem. Thanks

2 Likes

Glad that I could be of help.

AFAIK the private fields will be merged before the plugs in the pipeline are executed, that means you can put the auth plug into your usual pipeline instead of having to “hide” it in the controller.

I’ve used the same approach to assign JSON schemas to specific routes.