search

yunojuno / django-request-token

Django app that uses JWT to manage one-time and expiring tokens to protected URLs. Not related to DRF.
MIT License
47 stars 23 forks source link

Cant delete user assigned to RequestToken in view that uses that token #55

Closed ddoyaguez closed 1 year ago

ddoyaguez commented 2 years ago

Hi. I have been trying to implement RGPD Article 94 "right to be forgotten" using django-request-token. So when the user enters his email in a view, an email is sent to him and that email has a link with a request token that triggers the deletion of his user. So I have a view decorated by a use_request_token decorator, and inside that view I am trying to delete the user that is associated with the current request_token. The user deletion goes well, but the problem is that error 500 is always return by the decorator.

I have in my settings.py REQUEST_TOKEN_DISABLE_LOGS = True, but anyway in RequestToken's decorator.py the call to log_token_use is done always. So it calls increment_used_counts() always. The problem with that is that it calls the save method of the RequestToken, and it has the user associated that I have previously deleted, so it fails.

Would it be possible to make REQUEST_TOKEN_DISABLE_LOGS disable also the usage increment by moving the increment_used_count() call to be after the DISABLE_LOGS check?

Another option is to add another setting called maybe DISABLE_USAGE_INCREMENT so it can be controlled.

I can create a pull request with the option you prefer if it is possible.

hugorodgerbrown commented 2 years ago

@ddoyaguez thanks for this - it's a really interesting use case. I had a look at what was going on, and in the course of investigating it I've ended up with PR (#57) which would solve this issue. Take a look and see what you think.

I think that disabling the increment along with the logs is interesting, but ultimately they are different use cases - DISABLE_LOGS is really there to prevent table bloat from logging too many requests - which is not an issue for the increment. Or is it (too many updates?) Not sure 🤔

/cc @emab

ddoyaguez commented 2 years ago

I think that your solution is perfect, thanks for the quick reply and implementation. At first sight it is exactly what I needed. I will try to test PR #57 tomorrow with my code. Im pretty sure it will work though. Thank you very much!