Cache user's Account securely and threadsafe - is that possible?

[anatoly techtonik]

Hey,

I am looking at our middleware that adds .user to request:

http://code.google.com/p/rietveld/source/browse/codereview/middleware.py

It caches Account inside class attribute `current_user_account`:

http://code.google.com/p/rietveld/source/browse/codereview/models.py

And we've already discussed that it is not threadsafe.

So I thought that alternative would be to:

1. memcache  - insecure, because any key can be requested anytime by any piece of code

2. Account.__cache[username]  - may grow up to be extra long and it might still be possible

                                                 to misuse that to get access to other user's records

I want to get rid of request.user in favor of request.account to make the code more clear.

So the proper place for this cache to me is middleware itself.

Any thoughts?

-- 

anatoly t.

[Andi Albrecht]

On Fri, Feb 17, 2012 at 11:32 AM, anatoly techtonik <tech...@gmail.com> wrote:
> Hey,
>
> I am looking at our middleware that adds .user to request:
> http://code.google.com/p/rietveld/source/browse/codereview/middleware.py
> It caches Account inside class attribute `current_user_account`:
> http://code.google.com/p/rietveld/source/browse/codereview/models.py
> And we've already discussed that it is not threadsafe.

Maybe we can get more explicit and add an "account" (or "user")
argument to each function that uses Account.current_user_account.

>
> So I thought that alternative would be to:
> 1. memcache  - insecure, because any key can be requested anytime by any
> piece of code
> 2. Account.__cache[username]  - may grow up to be extra long and
> it might still be possible
>                                                  to misuse that to get
> access to other user's records

Another option would be to use a threading.local - but then we should
be very careful to reset it correctly. This could be done by using a
middleware - except for some rare cases (I'm thinking of
DeadlineExceeded). At least when a new request is started we should
reset it.

--
Andi

>
> I want to get rid of request.user in favor of request.account to make the
> code more clear.
> So the proper place for this cache to me is middleware itself.
>
> Any thoughts?
> --
> anatoly t.
>

> --
> You received this message because you are subscribed to the Google Groups
> "codereview-discuss" group.
> To post to this group, send email to coderevie...@googlegroups.com.
> To unsubscribe from this group, send email to
> codereview-disc...@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/codereview-discuss?hl=en.

[Guido van Rossum]

On Fri, Feb 17, 2012 at 3:10 AM, Andi Albrecht
<albrec...@googlemail.com> wrote:
> On Fri, Feb 17, 2012 at 11:32 AM, anatoly techtonik <tech...@gmail.com> wrote:
>> Hey,
>>
>> I am looking at our middleware that adds .user to request:
>> http://code.google.com/p/rietveld/source/browse/codereview/middleware.py
>> It caches Account inside class attribute `current_user_account`:
>> http://code.google.com/p/rietveld/source/browse/codereview/models.py
>> And we've already discussed that it is not threadsafe.
>
> Maybe we can get more explicit and add an "account" (or "user")
> argument to each function that uses Account.current_user_account.
>
>>
>> So I thought that alternative would be to:
>> 1. memcache  - insecure, because any key can be requested anytime by any
>> piece of code
>> 2. Account.__cache[username]  - may grow up to be extra long and
>> it might still be possible
>>                                                  to misuse that to get
>> access to other user's records
>
> Another option would be to use a threading.local - but then we should
> be very careful to reset it correctly. This could be done by using a
> middleware - except for some rare cases (I'm thinking of
> DeadlineExceeded). At least when a new request is started we should
> reset it.

+1 on using thread-local.

(If we were to switch to NDB we'd get thread-local per-request caching
like this for free. But that's a major undertaking.)

--Guido van Rossum (python.org/~guido)

[anatoly techtonik]

I am looking at get_account_for_user() which implements caching.

http://code.google.com/p/rietveld/source/browse/codereview/models.py#633

It caches only the last user by checking Account.current_user_account and I wonder if such caching between requests makes any sense at all?

Right now we have about 2-3 requests per minute on average and these come from different users. ISTM that with single user the cache miss is above 95%. With multi-threading it will be above 99%. So the only viable solution I see to reduce DB reads is to use memcache (and remove caching between requests for now).

-- 
anatoly t. 

[Guido van Rossum]

I am sure the caching was only meant to matter within the current
request, which requests the same Account (i.e. for the current user)
several times in different, unrelated places.

If the Django request object is available in all those places we could
indeed use middleware that sticks the account on the request object
(like it's already done for the user).

It's possible to mix and match NDB and old db for different model
classes, as long as they aren't linked via ReferenceProperty (or
KeyProperty on the NDB side). So in theory we could use NDB for
Accounts only (at first). Then NDB's caching would make all these
hacks unnecessary. NDB's the future anyway.

--
--Guido van Rossum (python.org/~guido)

[Guido van Rossum]

Oops, this is already done. Perhaps much of the account caching code
is no longer relevant? Try cutting it out and observe with Appstats if
more Account records are being read.