chaps-io / access-granted

Multi-role and whitelist based authorization gem for Rails (and not only Rails!)
MIT License
775 stars 41 forks source link

Caching accesses. #30

Closed ZebThan closed 8 years ago

ZebThan commented 8 years ago

Hi.

We've been using this gem with rolify, and encountered a problem with performance. The thing was, we were doing a lot "can?" on one page. And by a lot I mean a LOT. It took 7 seconds to load everything every time we were reloading the page. The thing is, most of the queries where duplicated, so we could cache them, instead of loading them from DB (DB caching seems not very efficient). We have solved the problem by overriding the "can?" method like this:

def can?(action, subject = nil)     
    @user.can = {} unless @user.can
    key = "#{action}_#{subject.try(:to_s)}".to_sym
    return @user.can[key] unless @user.can[key].nil?
    roles.each do |role|
      next unless role.applies_to?(@user)
      permission = role.find_permission(action, subject)
      if permission
        @user.can[key] = true
        return permission.granted
      end
    end
    @user.can[key] = false
    false
  end

Caching feature would be a great thing. In our case, the code above increased performance on the given page from 7 sec load time, to 2 sec load time. Of course it depends on the amount of data, more data, bigger improvement, on the other pages it is not noticeable at all. It's downside is the fact, that we are caching it inside a user, which doesn't seem like the best idea. Changing accesses while user is logged in will have no effect, unless we force him to relog. Also it is a security danger, if user finds a way to somehow write something to the "can" attribute, he will gain access, and we will not be checking again if he really does have an access.

Anyways, that's what happened, I just wanted to share what we did. Great work by the way.

pokonski commented 8 years ago

Hi @ZebThan, thanks for sharing your findings! Agree that caching this way has some nasty side-effects.

One way to fix this would be caching only for the lifespan of a single request, much like ActiveRecord does it with its queries. Something for me to explore :+1:

pokonski commented 8 years ago

@ZebThan I just released a 1.1 version which brings caching. This results in 10x improvements in overall performance, do check it out :+1:

ZebThan commented 8 years ago

Wow, that was fast. Thanks for the info. I will check this out when I'll get a spare moment. I guess this issue can be closed now. Thanks again.

pokonski commented 8 years ago

Closing as resolved, if this is still an issue do reply here :)