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/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..43bee473d6d 100644 --- a/geonode/security/registry.py +++ b/geonode/security/registry.py @@ -423,10 +423,11 @@ 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() - resource_pks_with_perms = [ - resource.pk for resource in get_objects_for_group(instance, ["base.view_resourcebase"], any_perm=True) - ] + # 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 = 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, @@ -437,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, @@ -595,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}" @@ -614,10 +614,12 @@ 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 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: - 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..89647edf83d 100644 --- a/geonode/security/tests.py +++ b/geonode/security/tests.py @@ -3663,6 +3663,37 @@ 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, 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) + + 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