While I agree that authorisation is part of the business logic, I disagree that authorisation is part of create_user function. While this indeed prevents (in most cases) accidental omission of the check, it makes API less flexible, which mean that we need additional functions to create 1st user or create users in tests.

Totally agreed. My app supports multiple accounts and I let the contexts function handle authorization. Here’s a sample controller code:

def show(conn, %{"id" => id}) do
  case Groups.get_group(conn.assigns.current_account, id) do
    {:ok, group} ->
      # render the group

    {:error, reason} when reason in [:not_found, :unauthorized] ->
      # render not found
  end
end

My context actually filters by account+group id, so it never returns :unauthorized (so the controller code is a bit different).

it makes API less flexible

I tend to think about it from a reliability perspective. I want flexibility, but not at the cost of reliability. If I were to create_user anywhere without the proper authorization than the result is not accurate to the way it “should” work.

which mean that we need additional functions to create 1st user or…

I think having “seed data” in applications is normal. Seeds commonly skirt lots of constraints. Like perhaps setting up an Admin account that doesn’t need a UI. A one-time task.

or create users in tests.

I’d want to trust my tests to be reflective of the real world scenario. If I’m allowed to create resources, like users, but without the proper constraints on the application it does leave a hole that hopefully is covered by an integration test.

I think at the very least authorization should live IN the context and not in the controller. There’s the idea of inter-context communication but then suddenly the Authorization module knows all types of things about the rest of the app. If the authorization function is already in the context, then it is only logical to put it even closer to the risky code, hence me putting it INTO the create_user function.

Just thinking this through and I’m still not comfortable with having auth far away (another module) from the code it cares about.

Yeah, I totally get this. Curious why lots of other people prefer the authorization part far away (another module) from the code it cares about.

Also, having authorization far from code it cares about can be a performance risk in scenarios where the authorization has to make a DB call (or similar).

If this is related to comments I made in the RBAC thread, I’ll clarify a bit here.

In my head I feel like authorization IS business logic

I agree with this. What I was warning about was making all business logic authorization logic. That is to say, it’s reasonable to ask “is this user authorized to create a user”. It’s also reasonable to say “Is it valid to create a user with no username? No”. But it’d be weird to say “You are authorized to create a user with a valid user name, and not authorized to create a user without a username”.

All of it is business logic, but within the domain of business logic there are some questions that make sense rendered in terms of authorization, and other questions that make more sense rendered in terms of validation. If you treat auth as merely one of a dozen different properties that must be true about an entity when it is being created then it blurs that line in a way that I think is unhelpful.

If this is related to comments I made in the RBAC thread, I’ll clarify a bit here.

I don’t think so, but you have me interested

I agree with this. What I was warning about was making all business logic authorization logic. That is to say, it’s reasonable to ask “is this user authorized to create a user”. It’s also reasonable to say “Is it valid to create a user with no username? No”. But it’d be weird to say “You are authorized to create a user with a valid user name, and not authorized to create a user without a username”.

All of it is business logic, but within the domain of business logic there are some questions that make sense rendered in terms of authorization, and other questions that make more sense rendered in terms of validation. If you treat auth as merely one of a dozen different properties that must be true about an entity when it is being created then it blurs that line in a way that I think is unhelpful.

Agreed.

I’m trying to come up with rules about applying authorization. I don’t think having all authorization in the specific functions (create_x, list_x, update_x, etc…) makes sense in many cases. I also don’t think having an Authorization module makes sense since it cares about every other (many?) contexts.

Now I’m leaning towards having auth in context, but implemented at the highest level of the entry point (Controller, Absinthe Resolver) so the auth lives close to the code it cares about, but is sufficiently high enough in the call stack to prevent multiple auth calls. Example:

# No idea why I wrote it with a with, but used a resolver cause...
# I appreciate your work 😊 
defmodule MyAppWeb.Resolvers.Blog do
  def update_post(author, args, %{context: %{current_user: _}}) do
    with :ok <- Blogs.authorize(:update_post, author, args["post_id"]),
      post = Blog.get_post(args["post_id"]),
      {:ok, post} <- Blogs.update_post(post, args) do
        {:ok, post}
    else
      error -> {:error, error}
  end
end

In this scenario every context would have authorize functions that are called, most likely, up the call stack, but may well be implemented in the context if the scenario makes sense. That’s all I got right now.

I have another thread running: Why was equal sign used in with statement?

And given all the feedback I now really like the implementation with the caveat that Authorization can be used in the context under the right circumstances.

Two comments about this, see the reference number in the code block for each:

1. It depends what Blogs.get_post returns here, but if it returns like Map.get then it will either return a blog post or nil, and since you’re using = there either will be a valid return value.

   This means that you could be passing nil to Blogs.update_post which would probably result in an error if you’re not checking for nil in Blogs.update_post.

   A cleaner solution would be either to use a version of Blogs.get_post! that raises or a Blogs.fetch_post that returns either {:ok, post} or and error that you can match on during the with.

2. error -> {:error, error} may not behave how you expect- if Blogs.update_post returns {:error, error} then you’re capturing that value as error, which means you’re returning {:error, {:error, error}} from the with.

Thank you @bennelsonweiss

Much appreciated

Wow, great insight! I always struggled whether my context should return the schema or an ok/error tuple. Using get/fetch approach makes total sense and it follows the std lib.