From 01ce3a70af10157714e058dab1e8fe9c54e56aab Mon Sep 17 00:00:00 2001 From: Mattia Giupponi Date: Tue, 29 Sep 2026 15:20:37 +0200 Subject: [PATCH 1/3] Creating a user or a group times out and kills the worker --- geonode/groups/tests.py | 22 ++++++++++++++++++++++ geonode/people/signals.py | 14 ++++++++------ geonode/security/registry.py | 7 ++++--- geonode/security/tests.py | 12 ++++++++++++ 4 files changed, 46 insertions(+), 9 deletions(-) diff --git a/geonode/groups/tests.py b/geonode/groups/tests.py index b17993098e3..5b08c75a5b5 100644 --- a/geonode/groups/tests.py +++ b/geonode/groups/tests.py @@ -19,6 +19,8 @@ import json import logging +from unittest.mock import patch + from django.urls import reverse from django.conf import settings from django.test import override_settings @@ -838,6 +840,26 @@ def test_removing_user_from_auth_group_deletes_member(self): member_exists = GroupMember.objects.filter(user=self.test_user, group=self.bar).exists() self.assertFalse(member_exists) + def test_adding_user_to_auth_group_does_not_recompute_member_perms(self): + """Syncing GroupMember from auth.Group must not run the per-resource perms recomputation.""" + with patch("geonode.groups.models.GroupMember._handle_perms") as _handle_perms: + self.test_user.groups.add(self.bar.group) + + self.assertTrue(GroupMember.objects.filter(user=self.test_user, group=self.bar).exists()) + _handle_perms.assert_not_called() + + def test_adding_user_to_multiple_auth_groups_does_not_duplicate_member(self): + """Each m2m sync must not create a second GroupMember for a group already synced.""" + foo = GroupProfile.objects.create(slug="sync_foo", title="sync_foo") + try: + self.test_user.groups.add(self.bar.group) + self.test_user.groups.add(foo.group) + + self.assertEqual(GroupMember.objects.filter(user=self.test_user, group=self.bar).count(), 1) + self.assertEqual(GroupMember.objects.filter(user=self.test_user, group=foo).count(), 1) + finally: + foo.delete() + def test_join_syncs_to_auth_group(self): self.bar.join(self.test_user, role=GroupMember.MEMBER) diff --git a/geonode/people/signals.py b/geonode/people/signals.py index 0b438bb6ef6..43de05aed36 100644 --- a/geonode/people/signals.py +++ b/geonode/people/signals.py @@ -193,12 +193,14 @@ def sync_group_members(sender, instance, action, reverse, **kwargs): user_groups_ids = set(instance.groups.values_list("id", flat=True)) group_profiles = GroupProfile.objects.filter(group_id__in=user_groups_ids) - for group_profile in group_profiles: - GroupMember.objects.get_or_create( - group=group_profile, - user=instance, - defaults={"role": GroupMember.MEMBER}, - ) + # bulk_create skips GroupMember.save(), the auth group is already assigned and + # _handle_perms would recompute perms for every resource/user of the group + GroupMember.objects.bulk_create( + [ + GroupMember(group=group_profile, user=instance, role=GroupMember.MEMBER) + for group_profile in group_profiles.exclude(groupmember__user=instance) + ] + ) stale_members = GroupMember.objects.filter(user=instance).exclude(group__group_id__in=user_groups_ids) for member in stale_members: diff --git a/geonode/security/registry.py b/geonode/security/registry.py index 35c5c6ce22b..cd0952a3c71 100644 --- a/geonode/security/registry.py +++ b/geonode/security/registry.py @@ -614,10 +614,11 @@ def _get_cache_key(self, resource_pks, users=None, groups=None, remove_all_cache remove_all_cache: If True, includes the __ALL__ cache key """ cache_keys = [] + # resolve identifiers once, _user_identifier hits the DB for each call + user_identifiers = [self._user_identifier(user) for user in users] if users else [] for pk in resource_pks: - if users: - for user in users: - cache_keys.append(f"resource_perms:{pk}:{self._user_identifier(user)}") + for identifier in user_identifiers: + cache_keys.append(f"resource_perms:{pk}:{identifier}") if groups: for group in groups: diff --git a/geonode/security/tests.py b/geonode/security/tests.py index 3428127eff3..8c086b19ddd 100644 --- a/geonode/security/tests.py +++ b/geonode/security/tests.py @@ -3663,6 +3663,18 @@ def test_multiple_resources_independent_caching(self): self.assertIsNotNone(cache.get(cache_key_r1)) self.assertIsNotNone(cache.get(cache_key_r2)) + def test_cache_key_resolves_user_identifier_once_per_user(self): + """Cache key generation must not query the anonymous user for every resource/user pair.""" + resource_pks = [resource.pk for resource in self.resources] + users = [self.admin_user, self.test_user, self.test_user_owner] + + with patch("geonode.security.registry.get_anonymous_user", wraps=get_anonymous_user) as _get_anonymous_user: + cache_keys = permissions_registry._get_cache_key(resource_pks, users=users) + + self.assertEqual(_get_anonymous_user.call_count, len(users)) + self.assertEqual(len(cache_keys), len(resource_pks) * len(users)) + self.assertIn(f"resource_perms:{resource_pks[-1]}:user:{self.test_user.pk}", cache_keys) + def test_cache_key_generation_consistency(self): """ Test that the _get_cache_key method generates consistent and correct cache keys From 073b3736c3e3a2bb55781697454281af30066cd4 Mon Sep 17 00:00:00 2001 From: Mattia Giupponi Date: Tue, 29 Sep 2026 15:26:00 +0200 Subject: [PATCH 2/3] Creating a user or a group times out and kills the worker --- geonode/groups/models.py | 4 +++- geonode/security/registry.py | 3 ++- geonode/security/tests.py | 19 +++++++++++++++++++ 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/geonode/groups/models.py b/geonode/groups/models.py index b70511ef0e4..76099170d35 100644 --- a/geonode/groups/models.py +++ b/geonode/groups/models.py @@ -302,7 +302,9 @@ def _handle_perms(self, role=None): from geonode.security.utils import AdvancedSecurityWorkflowManager from geonode.security.registry import permissions_registry - permissions_registry.delete_resource_permissions_cache(instance=self.group.group) + permissions_registry.delete_resource_permissions_cache( + instance=self.group.group, users=[self.user], group_clear_cache=False + ) if not AdvancedSecurityWorkflowManager.is_auto_publishing_workflow(): AdvancedSecurityWorkflowManager.set_group_member_permissions(self.user, self.group, role) diff --git a/geonode/security/registry.py b/geonode/security/registry.py index cd0952a3c71..8d0d0b7414a 100644 --- a/geonode/security/registry.py +++ b/geonode/security/registry.py @@ -423,7 +423,8 @@ def delete_resource_permissions_cache(self, instance, user_clear_cache=True, gro self._clear_cache_keys(cache_keys if cache_keys else []) elif isinstance(instance, Group): - group_users = instance.user_set.all() + # membership changes only affect the given users, not the whole group + group_users = kwargs.get("users") or instance.user_set.all() resource_pks_with_perms = [ resource.pk for resource in get_objects_for_group(instance, ["base.view_resourcebase"], any_perm=True) ] diff --git a/geonode/security/tests.py b/geonode/security/tests.py index 8c086b19ddd..cc919bf1736 100644 --- a/geonode/security/tests.py +++ b/geonode/security/tests.py @@ -3675,6 +3675,25 @@ def test_cache_key_resolves_user_identifier_once_per_user(self): self.assertEqual(len(cache_keys), len(resource_pks) * len(users)) self.assertIn(f"resource_perms:{resource_pks[-1]}:user:{self.test_user.pk}", cache_keys) + def test_joining_group_clears_only_member_cache(self): + """Joining a group must clear only the joining user's cache, not the other members' or the group's.""" + test_resource = self.resources[0] + member_key = f"resource_perms:{test_resource.pk}:user:{self.admin_user.pk}" + joining_key = f"resource_perms:{test_resource.pk}:user:{self.test_user.pk}" + group_key = f"resource_perms:{test_resource.pk}:group:{self.test_group.pk}" + + permissions_registry.get_perms(instance=test_resource, user=self.admin_user, use_cache=True) + permissions_registry.get_perms(instance=test_resource, user=self.test_user, use_cache=True) + permissions_registry.get_perms(instance=test_resource, group=self.test_group, use_cache=True) + try: + self.test_group_profile.join(self.test_user, role=GroupMember.MEMBER) + + self.assertIsNone(cache.get(joining_key)) + self.assertIsNotNone(cache.get(member_key)) + self.assertIsNotNone(cache.get(group_key)) + finally: + self.test_group_profile.leave(self.test_user) + def test_cache_key_generation_consistency(self): """ Test that the _get_cache_key method generates consistent and correct cache keys From 31b14deb635bf8ce94f4e13727efff44297dc2b1 Mon Sep 17 00:00:00 2001 From: Mattia Giupponi Date: Tue, 29 Sep 2026 15:43:49 +0200 Subject: [PATCH 3/3] Deleting a user is slow --- geonode/security/registry.py | 18 +++++++++--------- geonode/security/tests.py | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/geonode/security/registry.py b/geonode/security/registry.py index 8d0d0b7414a..43bee473d6d 100644 --- a/geonode/security/registry.py +++ b/geonode/security/registry.py @@ -425,9 +425,9 @@ def delete_resource_permissions_cache(self, instance, user_clear_cache=True, gro elif isinstance(instance, Group): # membership changes only affect the given users, not the whole group group_users = kwargs.get("users") or instance.user_set.all() - resource_pks_with_perms = [ - resource.pk for resource in get_objects_for_group(instance, ["base.view_resourcebase"], any_perm=True) - ] + resource_pks_with_perms = list( + get_objects_for_group(instance, ["base.view_resourcebase"], any_perm=True).values_list("pk", flat=True) + ) cache_keys = self._get_cache_key( resource_pks=resource_pks_with_perms, @@ -438,8 +438,7 @@ def delete_resource_permissions_cache(self, instance, user_clear_cache=True, gro self._clear_cache_keys(cache_keys) elif isinstance(instance, Profile): - resources = get_objects_for_user(instance, "base.view_resourcebase") - resource_pks = [resource.pk for resource in resources] + resource_pks = list(get_objects_for_user(instance, "base.view_resourcebase").values_list("pk", flat=True)) cache_keys = self._get_cache_key( resource_pks=resource_pks, @@ -596,8 +595,8 @@ def _clear_cache_keys(self, cache_keys): else: raise TypeError(f"Expected str or list, got {type(cache_keys)}") - def _user_identifier(self, user): - if user.is_anonymous or user.username == "AnonymousUser" or user == get_anonymous_user(): + def _user_identifier(self, user, anonymous_user=None): + if user.is_anonymous or user.username == "AnonymousUser" or user == (anonymous_user or get_anonymous_user()): return "anonymous" return f"user:{user.pk}" @@ -615,8 +614,9 @@ def _get_cache_key(self, resource_pks, users=None, groups=None, remove_all_cache remove_all_cache: If True, includes the __ALL__ cache key """ cache_keys = [] - # resolve identifiers once, _user_identifier hits the DB for each call - user_identifiers = [self._user_identifier(user) for user in users] if users else [] + # resolve the anonymous user once, get_anonymous_user hits the DB for each call + anonymous_user = get_anonymous_user() if users else None + user_identifiers = [self._user_identifier(user, anonymous_user) for user in users or []] for pk in resource_pks: for identifier in user_identifiers: cache_keys.append(f"resource_perms:{pk}:{identifier}") diff --git a/geonode/security/tests.py b/geonode/security/tests.py index cc919bf1736..89647edf83d 100644 --- a/geonode/security/tests.py +++ b/geonode/security/tests.py @@ -3671,7 +3671,7 @@ def test_cache_key_resolves_user_identifier_once_per_user(self): with patch("geonode.security.registry.get_anonymous_user", wraps=get_anonymous_user) as _get_anonymous_user: cache_keys = permissions_registry._get_cache_key(resource_pks, users=users) - self.assertEqual(_get_anonymous_user.call_count, len(users)) + self.assertEqual(_get_anonymous_user.call_count, 1) self.assertEqual(len(cache_keys), len(resource_pks) * len(users)) self.assertIn(f"resource_perms:{resource_pks[-1]}:user:{self.test_user.pk}", cache_keys)