[model] cache user permissions #145 #153

NoumbissiValere · 2020-07-25T08:21:09Z

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

Fixes #145

coveralls · 2020-07-25T08:23:52Z

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

coveralls · 2020-07-25T08:23:52Z

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

nemesifier · 2020-07-25T23:39:03Z

openwisp_users/base/models.py

            self.phone_number = None

+    @cached_property
+    def get_permissions(self):


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 · 2020-07-25T23:39:50Z

openwisp_users/base/models.py

+        try:
+            del self.__dict__['get_permissions']
+        except KeyError:
+            pass


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

nemesifier · 2020-07-25T23:45:24Z

openwisp_users/tests/test_models.py

+
+        permission = Permission.objects.filter(codename='add_organization')
+        user.user_permissions.add(*permission)
+        self.assertEqual(user.get_all_permissions(), user.get_permissions)


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 · 2020-07-27T11:21:29Z

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 · 2020-07-27T23:18:56Z

README.rst

    http GET localhost:8000/api/v1/firmware/build/ "Authorization: Bearer $TOKEN"

+User permissions
+---------------


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 · 2020-07-27T23:43:10Z

openwisp_users/base/models.py

            self.phone_number = None

+    @property
+    def permissions(self):


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 · 2020-07-27T23:45:20Z

openwisp_users/base/models.py

+                and model.lower() == perm.split('_')[-1]
+            ):
+                return True
+        return False


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 · 2020-07-27T23:45:41Z

README.rst

+
+All user permissions are cached and can be accessed as follows ``obj.permissions``. This cache
+is updated each time the user's permissions or group is changed.
+


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'}

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

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

👍

Fixes #153

[bug] Organization field is missing when importing a CA or cert. openwisp#152

Related to openwisp#152

NoumbissiValere requested a review from nemesifier July 25, 2020 08:21

nemesifier requested changes Jul 25, 2020

View reviewed changes

nemesifier requested changes Jul 26, 2020

View reviewed changes

nemesifier reviewed Jul 26, 2020

View reviewed changes

nemesifier mentioned this pull request Jul 26, 2020

[admin-theme] menu doesn't respect assigned permissions #103 openwisp/openwisp-utils#123

Merged

NoumbissiValere force-pushed the issue-145-cache_user_permissions branch from ea3d339 to 7e92de7 Compare July 27, 2020 16:52

NoumbissiValere requested a review from nemesifier July 27, 2020 16:52

NoumbissiValere force-pushed the issue-145-cache_user_permissions branch 2 times, most recently from f9d1117 to 0429490 Compare July 27, 2020 18:48

nemesifier requested changes Jul 27, 2020

View reviewed changes

[model] cache user permissions #145

5952fea

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

NoumbissiValere force-pushed the issue-145-cache_user_permissions branch from 0429490 to 5952fea Compare July 28, 2020 00:09

[chores] Minor improvements + docs

1f3422e

nemesifier force-pushed the issue-145-cache_user_permissions branch from 0776a01 to 1f3422e Compare July 28, 2020 01:21

nemesifier requested changes Jul 28, 2020

View reviewed changes

[update] added minor changes

a0f46d8

NoumbissiValere force-pushed the issue-145-cache_user_permissions branch from 7d67d48 to a0f46d8 Compare July 29, 2020 05:30

Merge branch 'master' into issue-145-cache_user_permissions

0ea698f

nemesifier approved these changes Aug 13, 2020

View reviewed changes

nemesifier merged commit b4cb116 into master Aug 13, 2020

nemesifier deleted the issue-145-cache_user_permissions branch August 13, 2020 22:50

pandafy pushed a commit that referenced this pull request Jan 12, 2021

[Fix] Fixed shifting of html due to main-menu #153 (#154)

eff2770

Fixes #153

dee077 pushed a commit to dee077/openwisp-users that referenced this pull request Jan 6, 2026

Merge pull request openwisp#153 from Weirdo914/issue/152

c7e34d3

[bug] Organization field is missing when importing a CA or cert. openwisp#152

dee077 pushed a commit to dee077/openwisp-users that referenced this pull request Jan 6, 2026

[admin] Improved to PR openwisp#153

9eb7bf6

Related to openwisp#152


		All user permissions are cached and can be accessed as follows ``obj.permissions``. This cache
		is updated each time the user's permissions or group is changed.

Uh oh!

[model] cache user permissions #145 #153

[model] cache user permissions #145 #153

Uh oh!

Conversation

NoumbissiValere commented Jul 25, 2020

Uh oh!

coveralls commented Jul 25, 2020

Uh oh!

coveralls commented Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nemesifier Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemesifier Jul 25, 2020

Choose a reason for hiding this comment

Uh oh!

nemesifier Jul 25, 2020

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

NoumbissiValere commented Jul 27, 2020

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

nemesifier Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

nemesifier Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

nemesifier Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

nemesifier Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Jul 25, 2020 •

edited

Loading

nemesifier Jul 25, 2020 •

edited

Loading

nemesifier left a comment •

edited

Loading

nemesifier Jul 27, 2020 •

edited

Loading

nemesifier left a comment •

edited

Loading