Add ability to return failed authorization requests

[Cardosaum]

Is your feature request related to a problem? Please describe.

I see in the documentation that is stated as one of the main reasons one would not consider using LetMe the inability to provide more detailed information on why an authorization request failed:

You need to provide details on why an authorization request fails. Checks in LetMe currently return only a boolean value, meaning users receive a generic error without knowing which exact check failed. (Source)

I'm sure this is intentional, but I don't fully comprehend why the library present this design choice; Wouldn't the ability to return a {:error, reason} tuple instead of simply false be a sensible choice in order to provide more flexibility for end users?

My issue arises from the fact that I can't easily point why an authorization request failed - If I have a policy like:

object :object1 do
    action :create do
        allow [:requirement1, :requirement2, :requirement3, ...]
    end
end

It would be useful to have transparency on which exact check failed, but currently I'm only able to get {:error, :unauthorized}.

Describe the solution you'd like

It would be great if I could specify the reason for a failed test in the check itself.
For example something like:

defmodule MyApp.Policy.Checks do
  def requirement1(%User{id: id}, %{user_id: id}), do: true
  def requirement1(_subject, _object), do: {:error, :unsatisfied_requirement1}

  def requirement2(%Something{}, _), do: true
  def requirement2(subject, object), do: {:error, :unsatisfied_requirement2}
end

Describe alternatives you've considered

From what I could see, Bodyguard does have the ability to specify a custom error/reason - Which is great, but after seen how nicely LetMe handles things, I would really prefer sticking with it if possible.

Additional context

I know that the design choice for this behavior was deliberate, but I can't understand exactly why...

@woylie , would you be able to clarify the reasons for such an option?

---

Also, I'd like to say that the work done in LetMe is great, and I'm thankful for all your efforts and providing the Elixir ecosystem a great library! 🙇‍♂️

[woylie]

It's not a deliberate design choice, it just didn't make it into the initial release, and I currently don't need it. I'm definitely open for a PR to add this!

[Cardosaum]

Would you be open (and have bandwidth) to mentor me on how to implement this?
I'd be definitely interested in contributing, it's just that I think I might have some limited knowledge in Elixir yet.

From what I quickly skimmed through in the source code, authorize_functions seems to be the only place I need to change?

From what I understand, changing the default of using authorize? inside the other variants to authorize would do the trick I guess?

I'll open a proper PR for that, just looking for confirmation on my observations :)

[Cardosaum]

Okay, so looking at the source code it seems the hole is deeper 😅

There's all this fancy defp permit_function_clause macro and inner macros to play with as well - First time having fun with Elixir macros 💯

---

I'll be sharing my findings here, as I'm not sure if I'll be able to actually propose a PR given the code complexity - but at least I can help others on discoverability if they decide to contribute as well ^^

[Cardosaum]

Okay I think I'm starting to understand what is happening.

In permit_function_clause, we have a case that does the following:

source

 combined_condition =
      case {allow_condition, deny_condition} do
        {false, _} -> false
        {_, true} -> false
        {_, false} -> allow_condition
        {true, _} -> quote(do: !unquote(deny_condition))
        _ -> quote(do: !unquote(deny_condition) && unquote(allow_condition))
      end

Since we're using ! and &&, we end up "casting" the return value specified by the function (for example, {:error, :custom_error} into a false value, and considering an example where we specify deny false we actually have a bug it seems, because we're going to short-circuit and return the value of allow_condition - which in this case would be {:error, :custom_error}.

So, if we have Policy.authorize?(:test_create, %{}, %{}), it could return {:error, :custom_error} given the contents of the Checks.ex module being

defmodule MyApp.Checks do
  def custom_error(subject, object, opts) do
    {:error, :custom_error}
  end
end

and the test policy being:

    object :test do
      action :create do
        allow :custom_error
        deny false
      end
    end

Which contradicts the function definition of only returning true or false.

[woylie]

Changing the code so that checks can return error tuples would be feasible, but the question is, what do you return if you combine checks? Say we have this rule:

object :pet do
    action :feed do
        allow :owner
        allow :caretaker
    end
end

And the two checks return {:error, :not_an_owner} and {:error, :not_a_caretaker}, respectively. If one of the checks evaluates to true, feeding the pet is allowed. The action is only forbidden if both checks return an error. So what do you want to return in that case? Should the errors be combined in a list, like {:error, [:not_an_owner, :not_a_caretaker]}? That doesn't seem very ergonomic. Maybe {:error, :unauthorized, [:not_an_owner, :not_a_caretaker]} or {:error, %LetMe.UnauthorizedError{errors: [:not_an_owner, :not_a_caretaker]}} would be better. Let's change the example and combine rules with and and add a deny rule:

object :pet do
    action :feed do
        # returns {:error, :not_an_owner}
        allow :owner
        # returns {:error, :not_a_caretaker} and {:error, :not_in_care}
        allow [:caretaker, :in_care]
         # returns {:error, :hungry}
        deny :already_ate
    end
end

Now you might get an error like {:error, %LetMe.UnauthorizedError{errors: [:not_an_owner, :not_in_care]}}. The deny rule, though, fails if the check returns true (or maybe :ok in the future). We wouldn't have an error reason to return here.

I'm not sure about the ergonomics here. The difference between Bodyguard and LetMe is that in Bodyguard, you define a single function, and within that function, you can do any number of checks. But in LetMe, you define more granular checks that are then combined with the DSL. I feel like it might make more sense to make the failed checks available, instead of introducing checks with error reasons. Then you'd get something like: {:error, %LetMe.UnauthorizedError{failed_checks: [:owner, :caretaker]}} or {:error, %LetMe.UnauthorizedError{failed_checks: [:already_ate]}}. Or maybe failed_allow_checks and failed_deny_checks should be separated. If a check uses options, you'd see something like {:error, %LetMe.UnauthorizedError{failed_checks: [role: :owner]}}.

What do you think?

[Cardosaum]

Thank you for your time and sharing your thoughts, @woylie!

I think the idea of returning a struct with the failed checks is a good one. It would be more ergonomic and would allow for more flexibility in the future. I would suggest adding the feature of allowing the user to define their own error messages for each check, though. I think this would still reason well with the philosophy of LetMe about providing more granular control over the checks.

What I have in mind is something like this:

# lib/my_app/policies/pet.ex
defmodule MyApp.Policies.Pet do
  use LetMe.Policy

    object :pet do
        action :feed do
            allow :owner
            allow [:caretaker, :in_care]
            deny :already_ate
        end
    end
end

# lib/my_app/policies/pet/checks.ex
defmodule MyApp.Policies.Pet.Checks do

  def owner(%User{id: id}, %Pet{owner_id: id}), do: true
  def owner(_, _), do: %LetMe.FailedCheck{reason: "You are not the owner of the pet"}

  def caretaker(%User{id: id}, %Pet{caretaker_ids: ids}) when id in ids, do: true
  def caretaker(_, _), do: %LetMe.FailedCheck{reason: "You are not a caretaker of the pet"}
  
  def in_care(%User{id: id}, %Pet{in_care: id}), do: true
  def in_care(_, _), do: %LetMe.FailedCheck{reason: "You are not in care of the pet"}
  
  def already_ate(_, %Pet{ate_at: timestamp}) do
    if DateTime.diff(DateTime.utc_now(), timestamp, :hours) < 6 do
      %LetMe.FailedCheck{reason: "The pet already ate in the last 6 hours"}
    else
      true
    end
  end
end

By doing this, we could then return a tuple with the failed checks and their respective error messages. I think this would allow for more flexibility while still offering good ergonomics.

For example, when checking for a specific action, we could have something like:

{:error, %LetMe.UnauthorizedError{failed_checks: [
  %LetMe.FailedCheck{kind: :allow, check: :owner, reason: "You are not the owner of the pet"},
  %LetMe.FailedCheck{kind: :allow, check: :in_care, reason: "You are not in care of the pet"},
  %LetMe.FailedCheck{kind: :deny, check: :already_ate, reason: "The pet already ate in the last 6 hours"}
]}} = MyApp.Policies.Pet.authorize(:pet_feed, user, pet)

Note that in this example, I'm assuming that LetMe would automatically add the kind and check fields to the FailedCheck struct since it would have enough information to do so. This would allow the user to have more control over the error messages while still providing a good default behavior. (It might not be the right approach, but it's an idea of how it could be done.)

I know you mentioned that it might not be so good to return checks with errors, but I couldn't think of a easy way to provide custom error messages for each check without doing so. I think this approach would still provide a good balance between flexibility and ergonomics.

What do you think about this approach?