Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fixes #11995] Implement the DELETE method for the User API #12028

Merged
merged 3 commits into from
Mar 11, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions geonode/base/api/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -464,15 +464,15 @@ def test_delete_user_profile(self):
# Bob can't delete user
self.assertTrue(self.client.login(username="bobby", password="bob"))
response = self.client.delete(url, format="json")
self.assertEqual(response.status_code, 405)
self.assertEqual(response.status_code, 403)
# User can not delete self profile
self.assertTrue(self.client.login(username="user_test_delete", password="user"))
response = self.client.delete(url, format="json")
self.assertEqual(response.status_code, 405)
self.assertEqual(response.status_code, 403)
mattiagiupponi marked this conversation as resolved.
Show resolved Hide resolved
# Admin can delete user
self.assertTrue(self.client.login(username="admin", password="admin"))
response = self.client.delete(url, format="json")
self.assertEqual(response.status_code, 405)
self.assertEqual(response.status_code, 204)
mattiagiupponi marked this conversation as resolved.
Show resolved Hide resolved
finally:
user.delete()

Expand Down
118 changes: 118 additions & 0 deletions geonode/people/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import django
from django.test.utils import override_settings
from mock import MagicMock, PropertyMock, patch
from geonode.base.models import ResourceBase
from geonode.groups.models import GroupMember, GroupProfile
from geonode.tests.base import GeoNodeBaseTestSupport

from django.core import mail
Expand Down Expand Up @@ -58,6 +60,7 @@ def setUp(self):
self.permission_type = ("view", "download", "edit")
self.groups = Group.objects.all()[:3]
self.group_ids = ",".join(str(element.pk) for element in self.groups)
self.bar = GroupProfile.objects.get(slug="bar")

def test_redirect_on_get_request(self):
"""
Expand Down Expand Up @@ -786,3 +789,118 @@ def test_users_api_patch_username(self):
# username cannot be updated
self.assertEqual(response.status_code, 400)
self.assertTrue("username cannot be updated" in response.json()["errors"])

@override_settings(USER_DELETION_RULES=["geonode.people.utils.has_resources", "geonode.people.utils.is_manager"])
def test_valid_delete(self):
# create a new user
tim = get_user_model().objects.create(username="tim")

admin = get_user_model().objects.get(username="admin")

self.assertTrue(self.client.login(username="admin", password="admin"))

# admin wants to delete tim
# Admin is superuser or staff
self.assertTrue(admin.is_superuser or admin.is_staff)
# check that tim is not manager
# nor has any resources
self.assertFalse(ResourceBase.objects.filter(owner_id=tim.pk).exists())
self.assertFalse(GroupMember.objects.filter(user_id=tim.pk, role="manager").exists())

url = f"{reverse('users-list')}/{tim.pk}"
response = self.client.delete(url, content_type="application/json")

# admin is permitted to delete
self.assertEqual(response.status_code, 204)
mattiagiupponi marked this conversation as resolved.
Show resolved Hide resolved
# tim has been deleted
self.assertEqual(get_user_model().objects.filter(username="tim").first(), None)

@override_settings(USER_DELETION_RULES=[])
@patch("geonode.people.utils.user_deletion_modules", [])
def test_delete_without_validators(self):

norman = get_user_model().objects.get(username="norman")
admin = get_user_model().objects.get(username="admin")

self.assertTrue(self.client.login(username="admin", password="admin"))

# admin wants to delete norman but norman is already promoted
# Admin is superuser or staff
self.assertTrue(admin.is_superuser or admin.is_staff)

# Make sure norman is not a member
self.assertFalse(self.bar.user_is_member(norman))

# Add norman to the self.bar group
self.bar.join(norman)

# Ensure norman is now a member
self.assertTrue(self.bar.user_is_member(norman))

# p romote norman to a manager
self.bar.promote(norman)
# Ensure norman is in the managers queryset
self.assertTrue(norman in self.bar.get_managers())

url = f"{reverse('users-list')}/{norman.pk}"
response = self.client.delete(url, content_type="application/json")

# norman can be deleted because validator rules are not applied
self.assertEqual(response.status_code, 204)
self.assertEqual(get_user_model().objects.filter(username="norman").first(), None)

@override_settings(USER_DELETION_RULES=["geonode.people.utils.has_resources", "geonode.people.utils.is_manager"])
def test_delete_a_manger(self):
norman = get_user_model().objects.get(username="norman")
admin = get_user_model().objects.get(username="admin")

self.assertTrue(self.client.login(username="admin", password="admin"))

# admin wants to delete norman but norman is already promoted
# Admin is superuser or staff
self.assertTrue(admin.is_superuser or admin.is_staff)

# Make sure norman is not a member
self.assertFalse(self.bar.user_is_member(norman))

# Add norman to the self.bar group
self.bar.join(norman)

# Ensure norman is now a member
self.assertTrue(self.bar.user_is_member(norman))

# promote norman to a manager
self.bar.promote(norman)
# Ensure norman is in the managers queryset
self.assertTrue(norman in self.bar.get_managers())

url = f"{reverse('users-list')}/{norman.pk}"
response = self.client.delete(url, content_type="application/json")

# norman cant be deleted
self.assertEqual(response.status_code, 403)
mattiagiupponi marked this conversation as resolved.
Show resolved Hide resolved
self.assertNotEqual(get_user_model().objects.filter(username="norman").first(), None)

@override_settings(USER_DELETION_RULES=["geonode.people.utils.has_resources", "geonode.people.utils.is_manager"])
def test_delete_a_user_with_resource(self):
# create a new user
bobby = get_user_model().objects.get(username="bobby")
admin = get_user_model().objects.get(username="admin")

self.assertTrue(self.client.login(username="admin", password="admin"))

# admin wants to delete bobby
# Admin is superuser or staff
self.assertTrue(admin.is_superuser or admin.is_staff)
# check that bobby is not manager
# but he has resources already assigned
self.assertTrue(ResourceBase.objects.filter(owner_id=bobby.pk).exists())
self.assertFalse(GroupMember.objects.filter(user_id=bobby.pk, role="manager").exists())

url = f"{reverse('users-list')}/{bobby.pk}"
response = self.client.delete(url, content_type="application/json")

# admin is permitted to delete
self.assertEqual(response.status_code, 403)
# bobby cant be deleted
self.assertNotEqual(get_user_model().objects.filter(username="bobby").first(), None)
53 changes: 53 additions & 0 deletions geonode/people/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,14 @@
from django.contrib.auth.models import Group

from geonode import GeoNodeException
from geonode.base.models import ResourceBase
from geonode.groups.models import GroupProfile, GroupMember
from geonode.groups.conf import settings as groups_settings
from rest_framework.exceptions import PermissionDenied
from django.conf import settings
from django.utils.module_loading import import_string

# from geonode.people.models import Profile


def get_default_user():
Expand Down Expand Up @@ -140,3 +146,50 @@ def get_available_users(user):
member_ids.extend(users_ids)

return get_user_model().objects.filter(id__in=member_ids)


def has_resources(profile) -> bool:
"""
checks if user has any resource in ownership

Args:
profile (Profile) : accepts a userprofile instance.

Returns:
bool: profile is the owner of any resources
"""
return ResourceBase.objects.filter(owner_id=profile.pk).exists()


def is_manager(profile) -> bool:
"""
Checks if user is the manager of any group

Args:
profile (Profile) : accepts a userprofile instance.

Returns:
bool: profile is mangager or not

"""
return GroupMember.objects.filter(user_id=profile.pk, role=GroupMember.MANAGER).exists()


def call_user_deletion_rules(profile) -> None:
"""
calls a set of defined rules specific to the deletion of a user
which are read from settings.USER_DELETION_RULES
new rules can be added as long as they take as parameter the userprofile
and return a boolean
Args:
profile (Profile) : accepts a userprofile instance.

Returns:
None : Raises PermissionDenied Exception in case any of the deletion rule Fail
"""
if not globals().get("user_deletion_modules"):
rule_path = settings.USER_DELETION_RULES if hasattr(settings, "USER_DELETION_RULES") else []
globals()["user_deletion_modules"] = [import_string(deletion_rule) for deletion_rule in rule_path]
for not_valid in globals().get("user_deletion_modules", []):
if not_valid(profile):
raise PermissionDenied()
10 changes: 9 additions & 1 deletion geonode/people/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
from geonode.security.utils import get_visible_resources
from guardian.shortcuts import get_objects_for_user
from rest_framework.exceptions import PermissionDenied
from geonode.people.utils import call_user_deletion_rules


class SetUserLayerPermission(View):
Expand Down Expand Up @@ -166,7 +167,7 @@ class UserViewSet(DynamicModelViewSet):
API endpoint that allows users to be viewed or edited.
"""

http_method_names = ["get", "post", "patch"]
http_method_names = ["get", "post", "patch", "delete"]
authentication_classes = [SessionAuthentication, BasicAuthentication, OAuth2Authentication]
permission_classes = [
IsAuthenticated,
Expand Down Expand Up @@ -204,6 +205,13 @@ def update(self, request, *args, **kwargs):
serializer.save()
return Response(serializer.data)

def perform_destroy(self, instance):
# self delete check
if self.request.user.pk == int(self.kwargs["pk"]):
raise PermissionDenied()
call_user_deletion_rules(instance)
instance.delete()

@extend_schema(
methods=["get"],
responses={200: ResourceBaseSerializer(many=True)},
Expand Down