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.

Sign up for GitHub

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

#260 add check password when delete profile #261

Merged
merged 4 commits into from
Oct 10, 2023
Merged

#260 add check password when delete profile #261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
36b0c06
check password added
Lvyshnevska Oct 9, 2023
55d6656
delete tests updated
Lvyshnevska Oct 9, 2023
b5e14c6
redundant import removed
Lvyshnevska Oct 9, 2023
5e4d9de
update code
Lvyshnevska Oct 9, 2023
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
34 changes: 31 additions & 3 deletions profiles/tests/test_crud_profile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,14 +360,16 @@ def test_delete_profile_unauthorized(self):
)
self.assertEqual(status.HTTP_401_UNAUTHORIZED, response.status_code)

def test_delete_profile_authorized(self):
def test_delete_profile_authorized_with_correct_password(self):
self.user.set_password("Test1234")
self.client.force_authenticate(self.user)

# del profile
response = self.client.delete(
path="/api/profiles/{profile_id}".format(
profile_id=self.profile.id
)
),
data={"password": "Test1234"},
)
self.assertEqual(204, response.status_code)

Expand All @@ -383,12 +385,38 @@ def test_delete_profile_authorized(self):
)
self.assertEqual(status.HTTP_404_NOT_FOUND, response.status_code)

def test_delete_profile_authorized_with_wrong_password(self):
self.user.set_password("Test1234")
self.client.force_authenticate(self.user)

# del profile
response = self.client.delete(
path="/api/profiles/{profile_id}".format(
profile_id=self.profile.id
),
data={"password": "Test5678"},
)
self.assertEqual(status.HTTP_400_BAD_REQUEST, response.status_code)

def test_delete_profile_authorized_without_password(self):
self.client.force_authenticate(self.user)

# del profile
response = self.client.delete(
path="/api/profiles/{profile_id}".format(
profile_id=self.profile.id
)
)
self.assertEqual(status.HTTP_400_BAD_REQUEST, response.status_code)

def test_delete_profile_of_other_user_authorized(self):
self.user.set_password("Test1234")
profile2 = ProfileStartupFactory()
self.client.force_authenticate(self.user)

response = self.client.delete(
path="/api/profiles/{profile_id}".format(profile_id=profile2.id)
path="/api/profiles/{profile_id}".format(profile_id=profile2.id),
data={"password": "Test1234"},
)
self.assertEqual(403, response.status_code)

Expand Down
12 changes: 12 additions & 0 deletions profiles/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import stat
import django_filters
from django.shortcuts import get_object_or_404
from django.contrib.auth.hashers import check_password
from rest_framework import status
from rest_framework.generics import (
CreateAPIView,
Expand Down Expand Up @@ -174,6 +176,16 @@ def get_serializer_class(self):
else:
return ProfileOwnerDetailEditSerializer

def destroy(self, request, *args, **kwargs):
instance = self.get_object()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's better to use get_object_or_404 here?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. get_object_or_404() may be used in get_object() (if needed), but not here.

user = self.request.user
password = self.request.data.get("password")
if not password or not check_password(password, user.password):
return Response(status=status.HTTP_400_BAD_REQUEST)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 400 BAD REQUEST must have a body, with explanation what exactly is wrong with the request. So, I guess this logic (together with password check) should be in serializer validator.
  2. Don't, don't do return Response().

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to do it in serializer, but there is no delete method in serializers, so I don't know how to implement it. I remember that it's not okay to return Response. But I just override original destroy method and it contains Response.

else:
self.perform_destroy(instance)
return Response(status=status.HTTP_204_NO_CONTENT)

def perform_destroy(self, instance):
instance.is_deleted = True
instance.save()
Expand Down
Loading