[model] cache user permissions #145

[NoumbissiValere]

Closed issue by adding a helper to
User model which caches all
permissions for a particular user.

Fixes #145

[coveralls]

Coverage increased (+0.03%) to 97.231% when pulling ea3d339 on issue-145-cache_user_permissions into 7024383 on master.

[coveralls]

Coverage increased (+0.1%) to 97.497% when pulling 0ea698f on issue-145-cache_user_permissions into 2826943 on master.

[nemesifier]

if it's a property, we don't need to name it get_permissions, we can name it just permissions.

The other problem is that cached_property does not cache using the cache storage, it caches only in memory, so the query will be executed again at subsequent requests.

We have to do something like organizations_dict: cache it in the cache storage (one key for each user) and invalidate the cache whenever the permissions or the groups of a user are changed.

Please study this PR and try to replicate what has been done there: https://github.com/openwisp/openwisp-users/pull/134/files.

[nemesifier]

why this? Looks like something that shouldn't be here.

[nemesifier]

please use assertNumQueries to ensure the query is executed only at the first call

[nemesifier]

I sent this to help you: 9d55c31
It will log queries to the console when you debug in ./manage shell_plus.

Look at this:

# flush cache
redis-cli
127.0.0.1:6379> FLUSHALL
OK
127.0.0.1:6379> quit

Launch shell and get user:

./manage.py shell_plus
In [1]: user = User.objects.first()                                                                                                                                                                                
(0.001) SELECT "openwisp_users_user"."password", "openwisp_users_user"."last_login", "openwisp_users_user"."is_superuser", "openwisp_users_user"."username", "openwisp_users_user"."first_name", "openwisp_users_user"."last_name", "openwisp_users_user"."is_staff", "openwisp_users_user"."is_active", "openwisp_users_user"."date_joined", "openwisp_users_user"."id", "openwisp_users_user"."email", "openwisp_users_user"."bio", "openwisp_users_user"."url", "openwisp_users_user"."company", "openwisp_users_user"."location", "openwisp_users_user"."phone_number" FROM "openwisp_users_user" ORDER BY "openwisp_users_user"."id" ASC LIMIT 1; args=()

Calling user.organizations_dict the first time generates a query:

In [2]: user.organizations_dict                                                                                                                                                                                    
(0.000) SELECT "openwisp_users_organizationuser"."created", "openwisp_users_organizationuser"."modified", "openwisp_users_organizationuser"."is_admin", "openwisp_users_organizationuser"."id", "openwisp_users_organizationuser"."user_id", "openwisp_users_organizationuser"."organization_id", "openwisp_users_organization"."name", "openwisp_users_organization"."is_active", "openwisp_users_organization"."created", "openwisp_users_organization"."modified", "openwisp_users_organization"."slug", "openwisp_users_organization"."id", "openwisp_users_organization"."description", "openwisp_users_organization"."email", "openwisp_users_organization"."url", "openwisp_users_organizationowner"."created", "openwisp_users_organizationowner"."modified", "openwisp_users_organizationowner"."id", "openwisp_users_organizationowner"."organization_user_id", "openwisp_users_organizationowner"."organization_id" FROM "openwisp_users_organizationuser" INNER JOIN "openwisp_users_organization" ON ("openwisp_users_organizationuser"."organization_id" = "openwisp_users_organization"."id") LEFT OUTER JOIN "openwisp_users_organizationowner" ON ("openwisp_users_organizationuser"."id" = "openwisp_users_organizationowner"."organization_user_id") WHERE ("openwisp_users_organization"."is_active" = 1 AND "openwisp_users_organizationuser"."user_id" = '07e322d84afb40cd899b04904287160c') ORDER BY "openwisp_users_organization"."name" ASC, "openwisp_users_organizationuser"."user_id" ASC; args=(True, '07e322d84afb40cd899b04904287160c')
Out[2]: {'20135c30-d486-4d68-993f-322b8acb51c4': {'is_admin': True, 'is_owner': False}}

Create a new user instance for the same user and access organizations_dict again, see how no query is generated (retrieved from cache):

In [3]: testuser = User.objects.first()                                                                                                                                                                            
(0.000) SELECT "openwisp_users_user"."password", "openwisp_users_user"."last_login", "openwisp_users_user"."is_superuser", "openwisp_users_user"."username", "openwisp_users_user"."first_name", "openwisp_users_user"."last_name", "openwisp_users_user"."is_staff", "openwisp_users_user"."is_active", "openwisp_users_user"."date_joined", "openwisp_users_user"."id", "openwisp_users_user"."email", "openwisp_users_user"."bio", "openwisp_users_user"."url", "openwisp_users_user"."company", "openwisp_users_user"."location", "openwisp_users_user"."phone_number" FROM "openwisp_users_user" ORDER BY "openwisp_users_user"."id" ASC LIMIT 1; args=()

In [4]: testuser.organizations_dict                                                                                                                                                                                
Out[4]: {'20135c30-d486-4d68-993f-322b8acb51c4': {'is_admin': True, 'is_owner': False}}

The same happens if you close shell_plus and launch it again. The cache during manual testing resides in redis (according to our test/settings.py). This means the cache survives requests and is shared among different processes (eg: application server, websockets server, celery workers, management commands).

We have to achieve a similar result with this patch for user.permissions.

[nemesifier]

Can you create an alternative has_perm method (see the one in django) to make it use the new cached permissions?

Let's not override the one from django to avoid causing issues, we add a new one for openwisp internal usage only, we'll use it here: openwisp/openwisp-utils#123 (review)

[NoumbissiValere]

ok @nemesisdesign thanks for the feedback. let me adjust them 😄

[nemesifier]

Great progress @NoumbissiValere, it's almost ready, see below for suggestions to make this even better!

[nemesifier]

the length of this must be equal to the length of the heading.
I would also move this section after the organization membership helpers and call it User permissions helper for consistency.

[nemesifier]

let's add a docstring:

"""
Returns the user permissions from the cache, if the cache is
empty it will call self.get_all_permissions() and cache the result 
"""

[nemesifier]

Why don't we just do:

def has_permission(self, permission):
    return permission in self.permissions

Since in openwisp-utils, we get the permission like {app_label}.{model}.

This method needs a brief mention in the README after User permissions

[nemesifier]

The ``User`` model of openwisp-users provides a ``permissions`` helper which returns the user's permissions from the cache, cache invalidation is handled automatically.

.. code-block:: python

    >>> user.permissions
    ... {'account.add_emailaddress',
         'account.change_emailaddress',
         'account.delete_emailaddress',
         'account.view_emailaddress',
         'openwisp_users.add_organizationuser',
         'openwisp_users.add_user',
         'openwisp_users.change_organizationuser',
         'openwisp_users.change_user',
         'openwisp_users.delete_organizationuser',
         'openwisp_users.delete_user'}

[nemesifier]

forgot one important detail, make sure has_permission method checks if the user is superuser, if it is, it should return True regardless (needs test and docs).

[nemesifier]

👍