2

I'm using Pundit gem for my authorization classes, where each controller action is checked against the model policy, to see if action is allowed by the user.

These methods are sometimes becoming quite bloated and unreadable, because I'm checking quite some stuff for some objects.

Now I'm thinking to refactor those methods, and place every "validation" in it's own method:

Previous:

class PostPolicy < ApplicationPolicy

 def update
   return true if @user.has_role? :admin
   return true if @object.owner == user
   return true if 'some other reason'
   
   false
 end
end

Now ideally, I want to refactor this in something like:

class PostPolicy < ApplicationPolicy

 def update
   allow_if_user_is_admin
   allow_if_user_owns_record
   allow_for_some_other_reason
   
   false
 end

 private

 def allow_if_user_is_admin
  # this would go in the parent class, as the logic is the same for other objects
  return true if @user.has_role? :admin
 end
end

The problem now is, that the mane update method will keep on going, even if the user is admin, as there's no return. If I would inlcude a return, then the other methods will never be evalutaed. Is there a way in ruby to do kind of a "superreturn", so that when the user is an admin, the main update method would stop evaluting?

Thanks!

bo-oz
  • 2,842
  • 2
  • 24
  • 44

4 Answers4

3

Given your example and this comment: "...no native way to do kind of a 'super return' in Ruby? It feels like kind of a "raise" but then with a positive outcome... could I use that perhaps?".

While there are usually other ways to solve the issue that could be considered "more idiomatic", ruby does have a Kernel#throw and Kernel#catch implementation that can be very useful for control flow when navigating through numerous and possibly disparate methods and operations.

The throw and corresponding catch will short circuit the result of the block which appears to be the syntax you are looking for.

VERY Basic Example:

class PostPolicy 
  def initialize(n) 
    @n = n 
  end
  def update
    catch(:fail) do
      stop_bad_actor!
      catch(:success) do 
        allow_if_user_is_admin
        allow_if_user_owns_record
        stop_bad_actor!(2)
        allow_for_some_other_reason
        false
      end 
    end
  end

 private

  def allow_if_user_is_admin
   puts "Is User Admin?"
   throw(:success, true) if @n == 1
  end

  def allow_if_user_owns_record
    puts "Is User Owner?"
    throw(:success,true) if @n == 2
  end 

  def allow_for_some_other_reason
    puts "Is User Special?"
    throw(:success,true) if @n == 3
  end 
  
  def stop_bad_actor!(m=1)
    puts "Is a Bad Actor?"
    throw(:fail, false) if @n == 6 || @n ** m == 64
  end
end

Example Output:

PostPolicy.new(1).update 
# Is a Bad Actor?
# Is User Admin?
#=> true
PostPolicy.new(2).update 
# Is a Bad Actor?
# Is User Admin?
# Is User Owner?
#=> true
PostPolicy.new(3).update 
# Is a Bad Actor?
# Is User Admin?
# Is User Owner?
# Is a Bad Actor?
# Is User Special?
#=> true
PostPolicy.new(4).update 
# Is a Bad Actor?
# Is User Admin?
# Is User Owner?
# Is a Bad Actor?
# Is User Special?
#=> false
PostPolicy.new(6).update 
# Is a Bad Actor?
#=> false
PostPolicy.new(8).update 
# Is a Bad Actor?
# Is User Admin?
# Is User Owner?
# Is a Bad Actor?
#=> false
engineersmnky
  • 25,495
  • 2
  • 36
  • 52
  • I think this is indeed more in the direction I'm looking for. What would be the difference from implementing this as "raise" versus "throw"? I'm not familair with the "throw" concept. – bo-oz Feb 11 '22 at 21:55
  • @bo-oz 2 Exceptions (`raise`) should be used for exceptional events (errors) and should not be used for control flow. Your implementation is intentional (not exceptional) and intended specifically for control flow. `catch` and `throw` are specific and communicative. I linked to the docs for each in the question but there are plenty of other user friendly explanations if you google "ruby throw catch" Like this one: http://phrogz.net/programmingruby/tut_exceptions.html – engineersmnky Feb 11 '22 at 22:01
  • This is the same logic as `allow_if_user_is_admin || allow_if_user_owns_record || allow_for_some_other_reason`, right? – David Aldridge Feb 12 '22 at 08:57
  • The only limitation that I still see is that I always have to have a succes thrown, while sometimes you'd like to specifically throw a failure in case there's some condition in which someone should specifically not get access. So basically each sub-method should have any of 3 outcomes --> specifically allow an action, specifically prevent an action, or go to the next sub method (so basically indecisive) – bo-oz Feb 12 '22 at 14:14
  • So something like 'return the first non-nil result' is the request here? – David Aldridge Feb 12 '22 at 22:03
1

What you can do is chain && operators.

As soon as one is false, ruby will not evaluate the others (And the update method will return false).

class PostPolicy < ApplicationPolicy

 def update
   allow_if_user_is_admin &&
     allow_if_user_owns_record &&
     allow_for_some_other_reason &&
 end

 private

 def allow_if_user_is_admin
  # this would go in the parent class, as the logic is the same for other objects
  return true if @user.has_role? :admin
 end
end
BTL
  • 4,311
  • 3
  • 24
  • 29
  • Yes indeed going in right direction, but no native way to do kind of a "super return" in Ruby? It feels like kind of a "raise" but then with a positive outcome... could I use that perhaps? – bo-oz Feb 11 '22 at 18:53
  • @bo-oz not sure I understand what you mean with this comment, can you clarify? – lacostenycoder Feb 11 '22 at 19:49
  • @BTL why do you need explicit return inside `allow_if_user_is_admin` ? – lacostenycoder Feb 11 '22 at 19:54
  • well, I don't really want any of the methods to be decisive, it could depend on the next criteria. So using && would need any method to return true, otherwise when the first method is false, it would also stop the evaluation of the other methods. In this case, if you're not an admin the first method would be `false`, so the other methods would not be evaluated. So this would not work. – bo-oz Feb 11 '22 at 20:45
  • so basicallty, when calling a submethod, there can be only 3 outcomes: return true & stop the rest of the calls, return false and sotp the rest of the calls, or continue with the next evaluation. In the example you put above, each one needs to be true, to get " true" as the final implicit return≥ – bo-oz Feb 11 '22 at 20:47
1

It seems that this would achieve your aim and be more idiomatic:

class PostPolicy < ApplicationPolicy

  def update
    user_has_admin_role? ||
      user_owns_object? ||
      some_other_reason?
  end

  private
  
  def user_has_admin_role?
    @user.has_role? :admin
  end

  def user_owns_object?
    @object.owner == user
  end

  def some_other_reason?
    'some other reason'
  end
end
David Aldridge
  • 51,479
  • 8
  • 68
  • 96
  • Yes close, but sometimes it can be that a submethod should explicitly return true or false. See this comment: so basicallty, when calling a submethod, there can be only 3 outcomes: return true & stop the rest of the calls, return false and sotp the rest of the calls, or continue with the next evaluation. In the example you put above, each one needs to be true, to get " true" as the final implicit return≥ – bo-oz Feb 11 '22 at 20:48
  • Aside from `some_other_reason?`, those methods do explicitly return true or false. `return true if (boolean expression)` is surely an anti-pattern when you can just write `boolean expressions`. These execute in turn until one returns true, and if none are true the result of the last method is returned, which ought to be a Boolean. But if you're looking for three valued logic, that isn't what your original code does? – David Aldridge Feb 12 '22 at 08:41
1

You can short-circuit boolean syntax, there me be cases where long chaining would look like bad style, here's an alternative, but basically same idea using Enumerable#all? See this answer for how it short-circuits

class PostPolicy < ApplicationPolicy

  def update
    deny?
  end
  
  private
  
  def deny?
    [ 
      user_is_admin?,
      user_owns_record,
      allow_for_some_other_reason?,
      thing1?,
      thing2? 
    ].any?
  end

  def user_is_admin?
    @user.has_role? :admin
  end

  def user_owns_record?
   @user.owns_record?
  end

  def allow_for_some_other_reason?
    @user.has_cheezebuerger?
  end

  def thing1?
    @user.thing1
  end

  def thing2?
    @user.thing2
  end
end
lacostenycoder
  • 10,623
  • 4
  • 31
  • 48
  • Tricky part is that sometimes a method needs to enforce true (admin) and sometimes it can be dependent on multiple criteria.. not the record owner, but authorized by the owner. So using [all] wouldn't work in thid case I think. – bo-oz Feb 11 '22 at 20:37
  • See comments on other suggestions. Couldn't I consider usin raise for both positive & negative outcome? Or should `raise` be exclusively kept for negative outcome? – bo-oz Feb 11 '22 at 20:49
  • 1
    @bo-oz `raise` will raise a runtime exception error, generally not a good idea. Better to handle your logic gracefully. You can also perhaps use the opposite of `.any?` and fail if any return `false` and perhaps use `deny?` instead of `permit?` – lacostenycoder Feb 11 '22 at 23:39
  • 1
    But neither `all?` not `any?` short-circuits the evaluation. All of those boolean methods are evaluated before `#all?` or `any?` evaluates the first result. – David Aldridge Feb 12 '22 at 08:53
  • While @DavidAldridge is correct although you could change the `Array` method calls to `Symbol`s (e.g `[:user_is_admin?]`) then use `any?(&method(:send))` – engineersmnky Feb 12 '22 at 17:34
  • You could, but it's starting to get very non-idiomatic compared to a set of method calls separated by `||` – David Aldridge Feb 12 '22 at 21:47