Should user permissions / data correctness logic live in my Ecto validations or Context logic?

Questions / Help
1

This is a best practice / idiomatic phoenix question. I’m building a demo app but I’m looking to understand good practice for more rigorous apps that control personal data and financial information.

Most of the actions in my contexts require a user. I.e. creating a transaction, updating it, etc. Either the record is going to be created belonging to the user, or permissions will be checked / logged. I know I can use a library like Canary to simplify the role management, but at this point I’m purely thinking about how to pass these user records around.

Approach A — Let Ecto handle it implicitly — treat the user as a required part of the transaction schema. Inject the user into the transactions attributes in my controllers before passing to MyApp.Ledger.create_transaction(attrs). Use put_assoc in the schema and use Ecto to validate the users presence.

Approach B — Handle it in my contexts explicitly — alter the interface of my context functions, make them all require a user too. Do any checks I need to before handing over to Ecto.

It feels like options A is less work, but is more “magic” and hidden logic. B feels like a lot of repetitive boilerplate - but a more explicit way of doing things.

I see this as a boundary issue - I want the knowledge and handling of users to live in my core application, not mixed up into the controllers of my web wrapper.

Does anyone have a strong opinion on either way? Or something else?
Have you done anything like this yourself? My goal is to have robust checks on which user is requesting which actions.

2 Likes
2

I’m trying to do it like you describe in approach B. I define “policies” for protected resources and call them from the context.

@spec create_transaction(map, by: %User{}) :: {:ok, %Transaction{}} | {:error, Ecto.Changeset.t} | no_return
def create_transaction(attrs, by: %User{id: uid} = user) do
  Policy.authorize!(:create_transaction, user)

  %Transaction{user_id: uid}
  |> Transaction.changeset(attrs)
  |> Repo.insert()
end

and then in ledger/policy.ex I have something like this

defmodule Ledger.Policy do
  alias MyApp.Authorization.UnauthorizedError
  
  @typep action :: :create_transaction
  @typep actor :: %User{}

  @spec authorized?(action, actor) :: boolean
  defp authorized?(:create_transaction, %User{role: :admin}), do: true
  defp authorized?(:create_transaction, _user}), do: false

  # usually I have authorize!(action, target, actor) though
  @spec authorize!(action, actor) :: nil | no_return
  def authorize!(action, actor) do
    unless authorized?(action, actor) do
      raise(UnauthorizedError, message: "nah ah")
    end
  end
end

And the exception has a plug_status to be rendered correctly on raise

defmodule MyApp.Authorization.UnauthorizedError do
  defexception message: "you are not authorized to access this resource :(",
               plug_status: 403
end

A lot of boilerplate though … It also causes a stacktrace to be generated which might hurt performance.

Can you show how Approach A would work? I don’t quite get it.

I’m also thinking about how I could move some of the above to the database. Because I don’t know how to handle get queries like

def get(resource_id, by: _user) do
  # authorize somehow
  Repo.get(Resource, resource_id)
end

since the result of my authorization depends on the resource being fetched.

btw, the controllers end up looking the same as if there was no authorization

def create(conn, %{"transaction_params"= > params}, current_user) do
  case Ledger.create_transaction(params, by: current_user) do
    {:ok, %Ledger.Transaction{} = transaction} ->
      render(conn, "show.html", transaction: transaction)
    {:error, %Ecto.Changeset{} = changeset} ->
      render(conn, "new.html", changeset: changeset)
  end
end
4 Likes
3

Thanks for the examples! I think you’re right, that’s the better way to proceed. <re-write incoming!>

My approach A has been for data correctness (i.e. validating that a transaction belongs to exactly 1 or 2 users), not for permissions.

As for your get example, what about scoping the query?

query = from t in "transactions",
          where: t.user_id = 1,
          where: t.id = 123

(sorry if this isn’t quite right!)

1 Like
4

Hey @idi527, @alexslade, what approach did you end up going with?

I’m currently struggling to find the best place for this logic to live. It seems like the Ledger policy approach checks for the existence of a user struct but doesn’t actually check if that user has permission to get/edit the resource.

My current train of thought for the different options is as follows:

For queries

  • Add the check inside the ecto query being run so we are essentially checking and querying at the same time.
  • Have a seperate context method that checks if the user has permission to query this resource

For mutations

  • Run a query beforehand (should be kept in seperate context method for reusability) that checks if the user has permission

For queries you can get away with running the check at the same time you query and it means one less request to the DB, but it feels wrong because it’s not consistent with mutations and the permission logic is super coupled with our query logic.

It feels more correct to have a seperate context method that ran the check query but then I’m not sure where that should be invoked. Do you invoke it in the context method in question, or back at the controller/resolver level? It feels like the controller/resolver level would be better since it decouples the permission checking from the action being performed. Unfortunately this seems like it leaves a lot of room for screw ups since you could easily forget to run the check…

1 Like