schrockwell / bodyguard Goto Github PK

Hi!

I've been using policies so far and just tried to add scopes, however I get the following error:

protocol Ecto.Queryable not implemented for []

defmodule App.Case.Policy do
  import Ecto.Query

  def can?(_user, _action, _case), do: true

  def scope(_user, _action, scope), do: scope
end

All I've added is the following line (policies work fine for the same controller):

def index(conn, _params) do
  IO.inspect scope(conn, Case)
  # ...

I've looked at the documentation but couldn't find a difference, what's wrong with my code?

Thanks!

Need to do something smarter about the user option in the Bodyguard.Plug.Authorize plug module. Since the init/1 callback happens at compile-time, we can't provide functions as options because they can't be unwrapped.

The easiest solution is change it to a {module, function} tuple that gets the current user from the conn.

Use case: the :params options in the plug can't access functions that are defined in that module.

So, in addition to accepting params: &get_params/1, also allow params: {__MODULE__, :get_params} and use apply/3 at runtime.

Hi,

First, thanks for the great library !

I was wondering why the library doesn't return {:error, :forbidden} instead of {:error, :unauthorized}, when authorization is failing ?

After reading on the subject, :forbidden would be more appropriate... 401 Unauthorized seems to be for an error of authentication (token/session expired, not authenticated at all etc.). 403 Forbidden seems to be for authorization error.

Am I getting something wrong ?

Thanks in advance.

For example, in scenarios where you have a many-to-many association between, say, companies and users, where the role column is defined in the memberships schema.

In this scenario, the current user is not enough. We also need the membership record.

I have a nested resource as such:

resources "/companies", CompanyController, only: [:create, :index, :show, :update] do
  resources "/users", UserController, only: [:index]
end

I want the user to be able to view all of the users by the same company, but not any other company's users. The problem is, on a route like /companies/:id/users, the id parameter is ignored and not passed further.

Parameters: %{"company_id" => "e6cda375-72d8-4bd3-9437-112db20dc62e"}

defmodule App.User.Policy do
  # I can't really do validations here, because on index just the User module is passed in, with no other context.
  def can?(_user, :index, User), do: true
end

Should I separately be authorizing the company and then the user/index action? That means I'd have to separately load the company from the db... when in my case I could simply compare current_user.company_id == id

It would be nice if there would be a changelog file for bodyguard. I'm always looking what changed before updating dependencies and this would make the process quite a bit easier.

Currently it's suggested to use the app env for providing default values for the Authorize plug:

### Default Plug Options

Application-wide defaults for the above options can be specified in the application config. For
example, if you're using Phoenix with Pow for authentication, you might want to specify:

      config :bodyguard, Bodyguard.Plug.Authorize,
        action: {Phoenix.Controller, :action_name},
        user: {Pow.Plug, :current_user}

https://github.com/schrockwell/bodyguard/blob/develop/lib/bodyguard/plug/authorize.ex#L26-L33

I'm wondering if it would be better to suggest just wrapping it in another plug like so:

defmodule MyAppWeb.Authorize do
  def init(opts) do
    opts
    |> Keyword.put_new(:action, {Phoenix.Controller, :action_name})
    |> Keyword.put_new(:user, {Pow.Plug, :current_user})
    |> Bodyguard.Plug.Authorize.init()
  end

  def call(conn, opts) do
    Bodyguard.Plug.Authorize.call(conn, opts)
  end
end

It's more flexible and also isn't limited to a single set of defaults.

I see that the Bodyguard.scope/3 helper automagically infers the record name from the Ecto queryable. In order for that to work, our scope must actually be defined inside the Ecto record itself.

I feel like this is too restrictive and kind of a hack. It also leads to unnecessarily fattening Ecto records. Scoping doesn't belong there - for example, we'd like to define our scopes inside a dedicated Policy file for each record, along with it's authorization logic.

How do you feel about a Bodyguard.scope/4 helper that also takes as its argument the name of the module that defines the policy? I can submit a PR if you like.

'nuff said

With Phoenix 1.3 soon to be released, I was wondering how bodyguard should be used with it, as we will have more separation and structure (aka contexts) in the new version.

I'll now port my app over to the current 1.3.0-rc.0 and share my experience when I get to authorization, but I was wondering if you already thought about this :)

Something like:

plug :bodyguard_options, policy: MyOtherPolicy

def index(conn, params) do
  authorize!(conn, action: :foo) # => opts is [policy: MyOtherPolicy, action: :foo]
end

It would be useful if an action could be any type. There's no real reason to constrain it to atoms.

This might "just work" and only require typespec changes.

Relevant discussion here: https://elixirforum.com/t/authorizing-context-actions-where-in-each-method-or-the-controller/6676

Note: would almost certainly require updating docs to change @behaviour Bodyguard.Policy to use Bodyguard.Policy to inject the macro (should have just done that in the first place).

I am getting:

Running dependency resolution...

Failed to use "bodyguard" because
  mix.exs specifies ~> 2.0.0

** (Mix) Hex dependency resolution failed, relax the version requirements of your dependencies or unlock them (by using mix deps.update or mix deps.unlock). If you are unable to resolve the conflicts you can try overriding with {:dependency, "~> 1.0", override: true}

Only convert a keyword list to a map – otherwise, just pass the param straight through

"nil data will not defer to any policy module, and will fail authorization by default. If the :policy option is explicitly specified, then that policy module will be used, passing nil as the data."

But the actual code will ignore the explicitly specified policy and fail:

Seems like a no-brainer – just use conn instead of socket, and use pattern matching against channels and messages versus controller actions.

... and don't take a block argument.

Hey, thanks for this library. Currently using it to add authorization to my app, and the documentation has been mostly very clear.

I'm starting to replace my initial simplistic authorization flow with Bodyguard, and am having some issues using the Authorize plug with a fallback controller. First, I have a policy definition like so:

defmodule ScribeWeb.Policy do
  @behaviour Bodyguard.Policy
  alias __MODULE__

  alias Scribe.Auth.User

  def authorize(_, %User{role: :admin}, _), do: true

  def authorize(:new_document, %User{role: :customer}, _), do: true

  def authorize(:new_document, %User{role: :demo}, _), do: true

  def authorize(_, _, _), do: false
end

Next I have this as an administrative-only controller:

defmodule ScribeWeb.AdminUserController do
  use ScribeWeb, :controller

  alias Scribe.Auth
  alias Scribe.Auth.User

  action_fallback ScribeWeb.FallbackController

  plug Bodyguard.Plug.Authorize
  ...
end

And this as my fallback. Note that this throws a warning about function calls that won't match, because I'm trying to debug it:

defmodule ScribeWeb.FallbackController do
  use ScribeWeb, :controller

  def call(conn, params) do
    IO.inspect(params)
    conn
  end

  def call(conn, {:error, :unauthorized}) do
    conn
    |> put_status(:not_found)
    |> put_view(ScribeWeb.ErrorView)
    |> render(:not_found)
  end
end

Unfortunately this doesn't work, My FallbackController is never hit. I'm unclear as to whether my use of the Authorize plug is incompatible with Phoenix fallbacks, if I have to perform checks on each individual action I want to protect, etc. Ideally I'd like to replace my entire authorization flow with a central policy definition, and some smarter return codes like :requires_login if a user isn't authenticated, :requires_demo if a user needs to request a demo to access the specified URL, etc.

Thanks for any help.

Per Slack discussion with @jnicklas, it would be cool if a can?/3 policy function could return tuples to provide more information about why authorization failed.

• true and :ok would count as authorized
• false, :error, and {:error, reason} would count as unauthorized

Then the question remains – is the interrogative "can?" no longer semantic? Something like "permit" or "can" would be better. I'd hate to change the API at this point.

Something more terse than Authy.authorized?(@conn.assigns.current_user, :action, resource)

It would be great if we could just do:

  def create(conn, %{"post" => post_params}) do
    with :ok <- permit(conn, Posts, :create_post), # <-- note conn, not user
      {:ok, post} <- Posts.create_post(current_user, post_params)
    do
      render(conn, "show.json", post: post)
    end
  end

…and automatically pull in conn.assigns[:current_user] for the user param (key configurable via config)

Not sure if it should be permit(conn, Posts, :create_post) (more plug-like) or permit(Posts, :create_post, conn) (matches signature of Bodyguard.permit/4)

Schema scopes aren't in the released version. Would it be okay to cut a release?

Right now it's technically possible through the optional opts list, but it would be nice if there was a convention for it so it was more obvious.

On the Bodyguard.Plug.Authorize plug, the action and user options accept functions. We should do that for params, too.

A bunch of the deps have build warnings because of old syntax, etc.

Compiling bodyguard on Elixir 1.11 throws the following warning:

==> bodyguard
Compiling 9 files (.ex)
warning: function actions/1 required by protocol Plug.Exception is not implemented (in module Plug.Exception.Bodyguard.NotAuthorizedError)
  lib/bodyguard/not_authorized_error.ex:8: Plug.Exception.Bodyguard.NotAuthorizedError (module)

Generated bodyguard app

Hi @schrockwell, first of all thanks for creating bodyguard! We've been using it successfully, however looks like it's moving at a slow pace or near abandoned.

The lib is stable but issues like #70 are concerning to me because eventually that will become a problem to update the deps of a project. So I'd like to offer help to keep maintaining bodygard, taking care of current and future PRs and issues.

You can also contact me at https://twitter.com/leandrocesquini, DMs are open.

Bodyguard.permit/4's typespec does suggest that it can return any of the possible return values of policies, but in fact it does normalize to only :ok | {:error, …} | no_return. It would be nice if this can be reflected in the typespec.

Why only just a string or atom?
https://github.com/schrockwell/bodyguard/blob/main/lib/bodyguard/policy.ex#L40

Bodyguard.permit?(MyAppWeb.SomeModule, {:some_action, :some_extra_context}, @current_user, nil)}

I am currently using tuples and it's working aside from dialyzer crying.

It might be cool if policies could also provide a "starting point" changeset. Maybe include cast operations, but no validations necessarily. Basically it would just be used to whitelist parameters passed in to a changeset.

The rough idea:

defmodule Post.Policy do
  def changeset(user, _action, post, params, _opts) do
    cond do
      # Admins can change title and body
      user.role == :admin -> 
        cast(post, params, [:title, :body])

      # Owners can only change title
      user.role == :user && user.id == post.user_id -> 
        cast(post, params, [:title])

      # Nobody else can change anything
      true ->
        cast(post, params, [])
    end
  end
end

Hi!

I am thinking about about possibility to provide some extra context for permissions. Like not the just fact that user can do something but also why. Something like this:

def authorize(:edit_post, user, post) do
  cond do
    user.is_admin -> {:ok, :admin}
    post.user_id == user.id -> {:ok, :author}
    true -> {:error, "Only adminisrators and authors can edit posts."}
  end
end

I am thinking about preparing PR but does not make sense if you think it don't fit into lib scope etc.

Example:

config :bodyguard, Bodyguard.Plug.Authorize,
  action: {Phoenix.Controller, :current_action},
  user: {App, :get_current_user}

Note we probably shouldn't/can't use anonymous functions in config files.

These would be merged in with the plug options, which would take precedence.

When using the verify_authorized plug in Phoenix 1.3 with Bodyguard 1.x, if any error is thrown in a controller action, the 403 error is raised and the error page does not show.

This is due to the way controller actions were refactored in Phoenix 1.3 to support fallback controllers. See https://github.com/phoenixframework/phoenix/blob/v1.3.0-rc.2/lib/phoenix/controller/pipeline.ex#L86

Need to investigate if this is an easy fix. If not, Bodyguard 2.x is recommended for Phoenix 1.3+ anyway.

Could we technically make the action always default to action_name(conn)? https://hexdocs.pm/phoenix/Phoenix.Controller.html#action_name/1

That way it would be a smart default that we wouldn't need to then manually specify everywhere.

Twilio has a trademark for Authy, their iOS two-factor authentication app.

It could be a an issue in the future, and it's going to be infinitely easier to rename the package now before anybody is actually using it.

We can rename the GitHub repo with no problem, since it will redirect everything.

We should just remove the package from Hex and publish it under a new name. There is a mechanism in hex to point users to the new package, but I think it's young enough we can nip this in the bud without affecting anybody.

We'll also need a new name. Thoughts @bencates?

After discussion with @bencates at ElixirConf...

Primary change: additional controller helpers for authorize:

• authorize/3: new, returns {:ok, conn} or {:error, reason} - good for handling with statements (a bit of future-proofing for Phoenix 1.3, and nice and Elixiry)
• authorize!/3: new, returns conn on success, raises on failure
• authorize/4: tweak: it's still a macro, but let's explicitly pass in conn instead of hiding it

Also:

• Drop handler config, rely on MyApp.ErrorView to display unauthorized
• Don't care about 404 case – let Repo.get! raise and explain more complicated case in readme
• Add :authy_authorized marker to via put_private
• Add auth plug that utilizes register_before_send - call it verify_authorized
• Probably expose mark_authorized to explicitly skip authorization

I'm getting a different error than what I expect when I use permit!:

defmodule MyAppWeb.Admin.LinkController do
  action_fallback MyAppWeb.FallbackController
  plug :set_and_authorize_post when not action in [:scrape]

  def set_and_authorize_post(%{params: %{"post_id" => post_id}} = conn, _) do
    post = Media.get_post!(post_id)
    Bodyguard.permit!(MyApp.Media, :manage_links, conn.assigns.current_user, post)
    assign(conn, :post, post)
  end

The error is:

protocol Enumerable not implemented for %MyApp.Media.Post{__meta__: #Ecto.Schema.Metadata<:loaded, "posts">, ...

In the readme in the Plugs section I see the following code:

  defp get_post(conn, _) do
    assign(conn, :post, MyApp.Posts.get_post!(conn.params["id"]))
  end

  # Helper for the Authorize plug
  def extract_post(conn), do: conn.assigns.posts

My confusion is how does posts become an element of assigns?

The function mentioned in the subject does return a map, this is not allowed in Plug 1.0 (according to the callbacks specs).

The specs do allow a map only since 1.3.0.

The signature of scope/4 has user as the second argument and the description states:

Filter a query down to user-accessible items.

What about cases where we need to filter a query on a resource other than the current user, like a current account, for example?

I suppose we can pass in the account in place of the user:

  def scope(query, account) do
    from invitation in query,
      where: invitation.account_id == ^account.id
  end

  def list_invitations(%Account{} = account) do
    Invitation
    |> Bodyguard.scope(account)
    |> Repo.all()
  end

I haven't really tested this and it brings some additional mental overhead. I'm not sure if the scope of scope/4 (no pun intended) includes this use case. Thoughts?

I have migrated my app from phoenix 1.2 to 1.3. But in order to upgrade Bodyguard 1 to 2, my app uses the api of permitted attributes that was available in 1.0.0
I asume this enhancement is in your roadmap, this is only a reminder.
Thnx in advance.

• Map true -> :ok
• Map false -> {:error, :unauthorized}

Is it possible to render changeset errors when using with syntax? For example:

    with :ok <- Bodyguard.permit(Media, :manage_links, current_user, post),
      {:ok, _link} <- Media.create_post_link(post, link_params)
    do
      conn
      |> redirect(to: get_location(conn))
    else
      {:error, %Ecto.Changeset{} = changeset} ->
        render conn, "new.html", user: current_user, changeset: changeset
    end

This will fail with a WithClause error because there's no clause that matches {:error, :unauthorized}.

However, dropping down to action_fallback:

    with :ok <- Bodyguard.permit(Media, :manage_links, current_user, post),
      {:ok, _link} <- Media.create_post_link(post, link_params)
    do
      conn
      |> redirect(to: get_location(conn))
    end

Doesn't render the template along with the changeset to display the errors.

Is it possible to use a with syntax and still show validation errors?

In documentation, an example is given to make role checks inside authorize function. Kindly, provide more information how to implement the role field itself, datatype, schema properties etc.

  # Admin users can do anything
  def authorize(_, %Blog.User{role: :admin}, _), do: :ok

Given...

  plug Bodyguard.Plug.Authorize,
    policy: Subscriptions,
    action: :crud,
    user: {UserAuth, :current_user},
    params: & &1.assigns.subscription,
    fallback: MyApp.FallbackController

The following error results:

== Compilation error in file lib/my_app_web/controllers/subscription_controller.ex ==
** (ArgumentError) cannot escape #Function<0.125979929/1 in :elixir_compiler_1.__MODULE__/1>. The supported values are: lists, tuples, maps
, atoms, numbers, bitstrings, PIDs and remote functions in the format &Mod.fun/arity

This is relevant per https://github.com/schrockwell/bodyguard/blob/develop/README.md#plugs uses this.

Platform:

Erlang/OTP 22 [erts-10.7.1] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [hipe] [dtrace]

Elixir 1.10.3 (compiled with Erlang/OTP 22)

I'm passing in a query where the "schema can't be determined". I noticed I should be able to pass in schema: MySchema and be all set. It doesn't work, and when I look at the source code, it seems like it couldn't..

    {schema, params} =
      get_option("Bodyguard.scope/4", params, opts, :schema, resolve_schema(query))

When that line is run, it'll immediately run the last arg to get the default value and that bombs out because of this:

  # Unable to determine
  defp resolve_schema(unknown) do
    raise ArgumentError, "Cannot automatically determine the schema of
      #{inspect(unknown)} - specify the :schema option"
  end

I think what you need to do is pass in a fn that can be executed if the default is needed versus calculating the default ahead of time.

Are there tests for this and I am missing something?
Thanks!

Is in your plans add support for strong parameters as Pundit for Rails ?