Accessing a reason from a nested allowed_to?

[brendon]

Discussed in #185

^{Originally posted by brendon August 29, 2021}
I have something like this:

class ComponentInstancePolicy < ApplicationPolicy
  pre_check :deny_deleted_component_instances

  def view?
    record.has_permission_to(:view, user)
  end

  private

  def deny_deleted_component_instances
    deny!(:deleted) if record.deleted?
  end
end

class InstancePolicy < ApplicationPolicy
  def view?
    allowed_to?(:view?, record.component_instance)
  end
end

When I call authorize! my_instance, to: :view?, with: InstancePolicy on a deleted component_instance and inspect the exception: exception.result.reasons I get:

#<ActionPolicy::Policy::FailureReasons:0x00007f89d3bc2c08 @reasons={ComponentInstancePolicy=>[:view?]}>

What I really want is :deleted for the reason so that I can provide a nice error message.

Is there a way to do this?

[brendon]

See: https://github.com/brendon/action_policy/tree/broken_reasons

EDIT: Sorry, the test was wrong. This correctly shows that a deny!(:reason) doesn't bubble up and the reason gets overwritten by the deferred to policy's entry method ('view?' in this case).

[palkan]

shows that a deny!(:reason) doesn't bubble up

That's a correct current behavior: allowed_to? acts like a barrier for bubbling, only the rule and the policy used in the check are added to reasons. Originally, we decided to no unwrap reasons: the idea was that policies shouldn't care about the underlying reasons in the nested policies.
Another reason is that we wanted to keep reasons flat, because introducing a reasons tree would make this feature a bit complicated.

Maybe, we can add an experimental option delegate_reasons: true (and its global counterpart) and see how it'll work out. WDYT?

[brendon]

Hi @palkan, I understand your reasoning. I guess there's a tension here between being able to delegate allowance logic and the granular reporting of failure reasons. delegate_reasons does look like one way to solve it but this could get messy too.

I had another case where I wanted to take action (redirect a user to a password entry screen when they tried to access something that was password protected (this can be indirectly by any of its ancestors so it's a check on another policy). I ended up with this

details[:passworded_parent] = passworded_parent
deny!

because the details do bubble up I was able to catch this at the exception layer and do a redirect to that passworded parent. I guess this is mildly inconsistent given the reason won't bubble.

There's kind of two situations here. One is just a failure of a check (boolean) when say view? returns false and an explicit deny! with a reason. Perhaps reasons from deny! should bubble up because they're fail-fast?

[palkan]

inconsistent given the reason won't bubble.

Yeah, probably; through details were added exactly to allow adding global metadata.

Perhaps reasons from deny! should bubble up because they're fail-fast?

That would make deny! too specific; not sure.

One of the reasons for the current reasons behavior is the following use-case I had: using e.result.reasons.full_messages to show a message to a user. If we decide on adding all reasons, even for nested policies, we would end up having more verbose and duplicating messages.

there's a tension here between being able to delegate allowance logic and the granular reporting of failure reasons.

Seems so.

delegate_reasons does look like one way to solve it but this could get messy too

What about adding one more helper (in addition to allowed_to? and check?) to delegate without introducing a new reasons scope? Less messy than adding an option; though, maybe, adds additional confusion when trying to choose the right method.

[brendon]

Thanks @palkan, that does sound like a good idea (a third helper). I think if it's documented carefully and named well then the confusion should hopefully be minimal. What the name should be? Hard to say. delegated_allowed_to??

[palkan]

What the name should be? Hard to say

Yeah, that's tricky; now I think that adding an option was a better idea 🙂 Maybe, more explicit one, say, merge_reasons: true.

And I have one more idea: ex.result.reasons.unwrapped to return all the leaf reasons (or, maybe, all the track reasons, not sure).

[brendon]

The option idea is probably better because it allows one to chose to delegate or not per call.

To be able to return the hierarchy of reasons might also be handy but also a bit convoluted to find a specific reason arbitrarily nested within others.

[palkan]

Hey @brendon! Please, take a look at the PR)

[brendon]

Thanks @palkan. Would it be possible to do a new gem release so I can give this and the bug fix a whirl? :)

[palkan]

Yes, I'm currently preparing the v0.6.0

[brendon]

Not sure if it was intentional, but this change obliterates details:

details[:title] = record.title
deny!(:my_reason)

would only return PolicyClass => :my_reason.

I found a workaround was to call:

deny!(:my_reason, { title: record.title })

Unsure if this requires more looking into though?

[palkan]

if this requires more looking into though?

Yeah, looks confusing (or not 🙂, the whole details feature craves a re-design).

[brendon]

Haha yea, now I have about 20 inline_reasons: true littered through my policies. Looks gross but does the trick. I suppose there could be a global toggle so it’s always on with the option to local_reasons: true.

[palkan]

I suppose there could be a global toggle so it’s always on with the option to local_reasons: true.

Yeah, I think we can add it, wouldn't hurt 🙂

[brendon]

Hi @palkan, as I've begun to use this more, and to catch the ActionPolicy::Unauthorized error with rescue_from it begins to become a bit of a mega-method and digging into reasons for authentication failure from deny! calls can be quite fragile. E.g.

if exception.result.reasons.details.fetch(:learning_area, []).include?(:logged_out) ...

Worse if I've passed up an object to take action upon from the deny! call. I sadly wrote this a while back and now the reasons are unclear lol!

passworded_parent = exception.result.reasons.details.fetch(:component_instance, []).lazy.map { |detail|
  detail.is_a?(Hash) && detail.dig(:passworded, :parent)
}.find(&:itself)

I think I'm making sure I have a deny!(passworded: { parent: object }) before continuing down that track.

All that is so say I'm wondering now whether it'd be cleaner for deny! to be able to raise domain specific error classes so that we could rescue_from these individually. It'd make that code cleaner because we wouldn't need one mega-method that digs into the reasons, and we could make more assumptions about the incoming error object.

Of course I could just raise myself at this point and not use deny! but I just wondered if you had any thoughts on this.

[brendon]

Just to follow this up, I've managed to simplify things with pattern matching:

  rescue_from ActionPolicy::Unauthorized do |exception|
    result = exception.result
    reasons = result.reasons
    details = reasons.details

    if reasons.present?
      case details
      in learning_area: [{ logged_out: learning_area }]
        flash[:fail] = 'Please log in to use the Learning Area.'
        redirect_to [learning_area]
      in component_instance: [{ passworded: passworded_parent }]
        save_original_url
        redirect_to [:prompt, passworded_parent.parent, passworded_parent.instance]
      else
        render inline: reasons.full_messages.to_sentence, status: 403, layout: 'error'
      end
    else
      render inline: result.message, status: 403, layout: 'error'
    end
  end

[palkan]

Oh, the pattern matching example looks good 👍 !

Have you considered all_details? I think, it's much closer to the "raise" idea (though you need to figure out how to name possible reasons).

[brendon]

Haha, I know right?! I always wondered what pattern matching would be good for and never really understood the hype, but I see the point now.

all_details seems to do some magic that's incompatible with passing actual objects as reasons (e.g. I pass the thing that I want to redirect to). When I try I get: no implicit conversion of LearningArea into Hash.

def deny_logged_out
  deny!(logged_out: record) if context != :admin && !record.public? && !logged_in?
end

I think I'm pretty happy with the state of things now. The only other nice-to-have would be the option to deny! with an alternative error class (e.g. an error that inherits off of ActionPolicy::Unauthorized) so that we can split up the rescue_from code. I use Rails engines extensively in this codebase and it would make sense to be able to put this rescue code with the particular engine it concerns.