search

TireSwingSoftware / openassign-server

OpenAssign server intended for use by a separate client via RPC
BSD 3-Clause "New" or "Revised" License
6 stars 2 forks source link

Authorizer check method cache coherency issues #113

Closed jc0n closed 12 years ago

jc0n commented 12 years ago

In my recent work with the authorizer I have encountered some issues with check method cache coherency for checks that do not use the actee (and are decorated with @does_not_use_actee) but make use of other model objects, or relationships exposed through auth_token (ie. user). actor_member_of_group is a check that is particularly vulnerable. Currently, if a user was removed from a group the change would not take affect until the app was restarted. After my cache expiry changes in #112 it would not take affect until after the auth token expired which is better but not ideal.

I believe actor_status_check is vulnerable as well. This leaves only 3 very trivial methods to be cached which will likely introduce more overhead than just making the calls. actor_is_anybody simply returns True, the other two are just a wrapper around a call to isinstance, hardly worth caching.

My suggestion would be to remove check method result caching for now. I was pondering whether or not it would be possible to cache all of the authorizer methods based on a session_id and their parameters and then register notify callbacks for model object change signals that we could use to expire the cache. This would be relatively complicated to get right. Then I realized that type of caching is really a job for the ORM (which is likely caching a lot already). As far as the performance impact of the check methods, I think the best thing to do is optimize the individual methods that exhibit sub-optimal performance. I noticed a few that could be improved a lot just glancing through.

mhrivnak commented 12 years ago

I don't think the ORM is actually doing much if any caching on its own.

You might be interested to look at 2791695f7cc6c26ef1e8dc45ed8fb9ff77dedf06 for how AuthTokens are cached, and how they are removed from the cache when saved or deleted with the ORM.

Keep in mind that the authorizer's cache features came about before django's cache framework existed.

On the topic of caching AuthTokens, it might make sense to start caching authorizer decisions on the AuthToken object itself.

However, all of this is fairly low-priority at the moment.

jc0n commented 12 years ago

The point of this issue isn't so much about the caching as it is about changes not propagating correctly through the system because of the caching.

jc0n commented 12 years ago

I don't think the ORM is actually doing much if any caching on its own.

Not in the same sense. But for example a common pattern that we have is checking objects after they are manipulated in some way be it create or update, etc. After the object has been retrieved the first time django does cache attributes and query sets so its likely that check methods will benefit from that.