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

delete.auth/users/me returns Internal server error #218

Closed
Monstarrrr opened this issue Jun 23, 2024 · 18 comments
Closed

delete.auth/users/me returns Internal server error #218

Monstarrrr opened this issue Jun 23, 2024 · 18 comments
Assignees
Labels
[ backend / blocking This Issue interrupted the progress of an other bug Something isn't working

Comments

@Monstarrrr
Copy link
Owner

Monstarrrr commented Jun 23, 2024

image

image

The endpoint is also not documented as there is no mention of current_password being required which I only found out (within the 401 response) after trying to call the endpoint without it at first :
image

@Monstarrrr Monstarrrr added bug Something isn't working [ backend / blocking This Issue interrupted the progress of an other labels Jun 23, 2024
@Monstarrrr Monstarrrr mentioned this issue Jun 23, 2024
3 tasks
@seporterfield
Copy link
Collaborator

It's an issue with drf-spectacular. They're opinionated about not allowing request bodies for DELETE requests.
tfranzel/drf-spectacular#809
tfranzel/drf-spectacular#280 (More links here too)

I'd propose a change to CONTRIBUTING.md that notes this quirk about DELETE requests possibly missing documentation, and a list of DELETE routes with links to their documentation.

@seporterfield
Copy link
Collaborator

Can you post the logs of the error you get when you provide the correct request body as in screenshot 1?

@Monstarrrr
Copy link
Owner Author

Can you post the logs of the error you get when you provide the correct request body as in screenshot 1?

Internal Server Error: /auth/users/me/
Traceback (most recent call last):
  File "C:\Python310\lib\site-packages\django\core\handlers\exception.py", line 55, in inner
    response = get_response(request)
  File "C:\Python310\lib\site-packages\django\core\handlers\base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "C:\Python310\lib\site-packages\django\views\decorators\csrf.py", line 65, in _view_wrapper
    return view_func(request, *args, **kwargs)
  File "C:\Python310\lib\site-packages\rest_framework\viewsets.py", line 124, in view
    return self.dispatch(request, *args, **kwargs)        
  File "C:\Python310\lib\site-packages\rest_framework\views.py", line 509, in dispatch
    response = self.handle_exception(exc)
  File "C:\Python310\lib\site-packages\rest_framework\views.py", line 469, in handle_exception
    self.raise_uncaught_exception(exc)
  File "C:\Python310\lib\site-packages\rest_framework\views.py", line 480, in raise_uncaught_exception
    raise exc
  File "C:\Python310\lib\site-packages\rest_framework\views.py", line 506, in dispatch
    response = handler(request, *args, **kwargs)
  File "C:\Python310\lib\site-packages\djoser\views.py", line 177, in me
    return self.destroy(request, *args, **kwargs)
  File "C:\Python310\lib\site-packages\djoser\views.py", line 163, in destroy
    utils.logout_user(self.request)
  File "C:\Python310\lib\site-packages\djoser\utils.py", line 26, in logout_user
cts'
[25/Jun/2024 17:01:41] "DELETE /auth/users/me/ HTTP/1.1" 500 112250

@seporterfield
Copy link
Collaborator

Call the endpoint with query params and it works
example:
curl -X 'DELETE' \ 'http://localhost:8000/auth/users/me?current_password=password123!/'

@Monstarrrr
Copy link
Owner Author

Monstarrrr commented Jun 28, 2024

Call the endpoint with query params and it works example: curl -X 'DELETE' \ 'http://localhost:8000/auth/users/me?current_password=password123!/'

http://localhost:8000/auth/users/me?current_password=Password123
returns 404 unless APPEND_SLASH is kept on true in which case it creates this link instead :
http://localhost:8000/auth/users/me/?current_password=Password123
(with a / after /me)
But the server returns a 400 bad request error

Image

Also putting passwords as query params is not safe:
https://www.fullcontact.com/blog/2016/04/29/never-put-secrets-urls-query-parameters/

@seporterfield
Copy link
Collaborator

seporterfield commented Jun 28, 2024

Screenshot has me?/...
Try me?current_password.../

I don't understand the comment about APPEND_SLASH. It is set to true.

@seporterfield
Copy link
Collaborator

Want to go with this unsafe practice for the MVP? The accounts get deleted anyway- still not great to have links with passwords in them clientside though

Alternative is a custom View/APIView for account deletion

@Monstarrrr
Copy link
Owner Author

Monstarrrr commented Jun 28, 2024

Screenshot has me?/...
Try me?current_password.../

I don't understand the comment about APPEND_SLASH. It is set to true.

If I want to remove it I need to disable APPEND_SLASH else django replaces it automatically in the URL.
Even if I remove APPEND_SLASH I then get 404 error (django page doesn't show at all because it can't find a route without the /)

@Monstarrrr
Copy link
Owner Author

Monstarrrr commented Jun 28, 2024

Want to go with this unsafe practice for the MVP? The accounts get deleted anyway- still not great to have links with passwords in them clientside though

Alternative is a custom View/APIView for account deletion

I mention this because we may not want to spend too much time trying to fix something we're not going to keep, creating a custom APIView would be nice assuming that's easier

@Monstarrrr Monstarrrr self-assigned this Jun 28, 2024
@seporterfield
Copy link
Collaborator

seporterfield commented Jun 28, 2024

If I want to remove it I need to disable APPEND_SLASH else django replaces it automatically in the URL.

By "it" do you mean the final slash? Why remove it at all?

I can't reproduce any issue with me?.../

@seporterfield
Copy link
Collaborator

http://localhost:8000/auth/users/me?current_password=Password123 returns 404 ...

Wait, you may be confusing two different things.

What method are you using? GET or DELETE?

Use the correct method to make a call to
http://localhost:8000/auth/users/me?current_password=Password123/ and post the error

@seporterfield
Copy link
Collaborator

Since the request is a DELETE method, do the objections about the password being in the params still make sense?

@Monstarrrr
Copy link
Owner Author

Monstarrrr commented Jun 29, 2024

If I want to remove it I need to disable APPEND_SLASH else django replaces it automatically in the URL.

By "it" do you mean the final slash? Why remove it at all?

I can't reproduce any issue with me?.../

I tried with removing it because i've never seen query strings in URLs being used without a path next to it (I've seen this: domain.com/path?queryString=whatever but never domain.com/path/?queryString=whatever)
https://www.rfc-editor.org/rfc/rfc1738#section-3.3
So I gave this a shot because I don't have an other explanation

It could be unrelated though

@Monstarrrr
Copy link
Owner Author

http://localhost:8000/auth/users/me?current_password=Password123 returns 404 ...

Wait, you may be confusing two different things.

What method are you using? GET or DELETE?

Use the correct method to make a call to
http://localhost:8000/auth/users/me?current_password=Password123/ and post the error

I'm using delete

@Monstarrrr
Copy link
Owner Author

Since the request is a DELETE method, do the objections about the password being in the params still make sense?

It seems so, even though I don't care about it for the MVP, I still think it implies spending time looking for a solution we won't keep for long anyways so maybe we should look at alternatives

@seporterfield
Copy link
Collaborator

I don't see anything in that thread answer related to security or the objections mentioned in the previous article.

@seporterfield
Copy link
Collaborator

I still think it implies spending time looking for a solution we won't keep for long anyways so maybe we should look at alternatives

👍 👍 👍 Yes

@seporterfield
Copy link
Collaborator

By default djoser has the setting
"user_delete": "djoser.serializers.UserDeleteSerializer", in the SERIALIZERS sub-setting
We can make a new serializer and keep the same route, but with an actual sensible way of passing values to the API
And one that drf-spectacular will be able to detect and add to the documentation.

@Monstarrrr Monstarrrr closed this as completed by moving to Done in Rebutify's MVP Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[ backend / blocking This Issue interrupted the progress of an other bug Something isn't working
Projects
Status: Done
Development

No branches or pull requests

2 participants