Reset user's password fine

Question

I wrote a code to reset user password. I just want to ask if it is a good and secure way to do this? Can I make any improvements here?

I use send_password_resetting_message function to send an email with a link to reset password and when I go on this page I use there set_new_password to set new password using uidb64, token and new password.

I am using DRF.

(it is for situation when you forget your password and you want to reset it)

views.py

from .utils import password_reset_token

@api_view(['POST'])
def send_password_resetting_message(request):
    try:
        email = request.data['email']

        if User.objects.filter(email=email).exists():
            user = User.objects.get(email=email)

            email_subject = "..."
            email_body = render_to_string('password_resetting/index.html', {
                'user': user,
                'domain': settings.FRONTEND_APP_ADDRESS,
                'uid': urlsafe_base64_encode(force_bytes(user.pk)),
                'token': password_reset_token.make_token(user)
            })

            email=EmailMessage(subject=email_subject, body=email_body, from_email=settings.EMAIL_FROM_USER, to=[email])
            email.content_subtype='html'
            email.send()
        return Response(status=status.HTTP_200_OK)
    except:
        return Response(status=status.HTTP_500_INTERNAL_SERVER_ERROR)
        
@api_view(['POST'])
def set_new_password(request, uidb64, token):
    try:
        uid = force_text(urlsafe_b64decode(uidb64))
        user = User.object.get(pk=uid)
    except Exception as e:
        user=None
    
    if user and password_reset_token.check_token(user, token):
        user.set_password(request.data['password'])
        user.save()
        return Response(status=status.HTTP_200_OK)
    return Response(status=status.HTTP_403_FORBIDDEN)

utils.py

from django.contrib.auth.tokens import PasswordResetTokenGenerator
import six

class PasswordResetTokenGenerator(PasswordResetTokenGenerator):
    def _make_hash_value(self, user, timestamp):
        return (six.text_type(user.pk) + six.text_type(timestamp) + six.text_type(user.password))

password_reset_token = PasswordResetTokenGenerator()

Answer 1

I am not an active user of Django but I would make some remarks regarding error handling and HTTP status codes.

In function send_password_resetting_message you have this in case an exception (any exception) occurs:

return Response(status=status.HTTP_500_INTERNAL_SERVER_ERROR)

Quite probably, something pretty similar would already take place automatically even if you did nothing, so this looks redundant and unneeded. Try without it, and perhaps you can ditch this statement.

Likewise, HTTP/200 should be the default response already, thus it can be omitted.

In the function set_new_password you have this bit of code:

try:
    uid = force_text(urlsafe_b64decode(uidb64))
    user = User.object.get(pk=uid)
except Exception as e:
    user=None

I am not sure what kind of error you expect here, but probably you should stop at this point. If an exception occurs, and you proceed nonetheless, the routine is bound to behave unpredictably. Maybe this is where you should terminate with an appropriate HTTP status code.

Speaking of which, the response is not helpful because it does not provide the user with any hint about what went wrong. You could add some description as shown in the docs:

from rest_framework import status
from rest_framework.response import Response

def empty_view(self):
    content = {'please move along': 'nothing to see here'}
    return Response(content, status=status.HTTP_404_NOT_FOUND)

Even if it's an API, returning some meaningful description of the error doesn't hurt.

Also, always do log the traceback to a log file, so you can investigate and troubleshoot later. And then, think about implementing a log monitoring solution. I know I am asking a lot, but it's important to monitor your logs. It helps you understand what's going on on your website, what problems users encounter and you can also fix problems proactively vs waiting for a bug report from an angry user. And if the user complains, you don't even have a trace to investigate since you don't log the errors.

One tip: when an exception occurs, what I do is generate an event ID (a random string), which I add to the log file, and I return that event ID to the user. So if they contact you to report the issue and quote the "event ID", then you can easily retrieve the relevant log entries and investigate, even if the user was unable to provide an accurate time and clear description of the problem.

Since you are sending E-mail, this is a good example of code that can fail, usually for transient reasons (DNS or routing issues etc) and it would be appropriate to return a warning if the mail could not be sent, whatever the reason. The docs tells us that the possible exceptions here are those defined in the smtplib module:

fail_silently: A boolean. When it’s False, send_mail() will raise an smtplib.SMTPException if an error occurs. See the smtplib docs for a list of possible exceptions, all of which are subclasses of SMTPException.

So you could handle SMTPException whenever you are sending mail. Plus, return a status code != 200 to signal there was a problem. HTTP/422 sounds appropriate. At least a warning was served and whomever sees it (or reads the log) knows why the mail won't arrive.

I would also log critical events, not just the errors. For instance, when a user changes password or some important details, I would log (with level INFO) the event to keep an audit trail. Unless you are already doing this at database level but I don't think this is the case here.

In case a user account is hijacked, you still have the webserver logs to investigate but it's far from certain that you will be able to correlate the log entries with a particular user. Of course it depends on how critical (or amateur) the project is.

But I can only emphasize the point: make it a habit to log errors, this will save you a lot of time (and headache) in your troubleshooting efforts.

Possible security considerations:

• Is the generated token single-use only and time-sensitive, meaning: invalidated after it's been consumed, and expires after a period of time? The point is to avoid a replay attack whereby a hostile party who comes into possession of this token would be able to set a new password later on
• At the risk of being spammy, I would then send a confirmation of password change by E-mail, after the said change has been carried out

Answer 2

Apart from Kate's great points, two points that needs to be said:

Extract methods

I would create separate functions that do your thing without any relation to HTTP connection. Not accessing request, not returning HTTP response.

Just pass it real parameters you are getting from the request and return something meaningful if viable or just nothing. As @Kate mentioned, drf is pretty intuitive in handling your errors and returning appropriate HTTP errors when something goes wrong. That will do a few things for you:

• You can re-use the code somewhere else, for example in non-API views, celery tasks or django managements commands.
• It will naturally separate and make visible problems with your code related to error handling as Kate mentioned.
• Make them unit-testable.

Make tests

Logging is great and helps you in production, but tests are extremely invaluable especially for critical parts of API.

Making tests for your specific usecases is just a few lines code really and they can help you so much in long run. Even when emails are involved, that is still easy to test in django

You can make both tests of extracted methods and tests for API itself.

I have been migrating old django project since django 1.6 until recent versions and early written tests are invaluable and have saved me ton of headaches especially when upgrading libraries and doing db migrations.

Answer 3

I think you did a great job, but I think is better to use serializers see on example:

class ChangePasswordSerializer(serializers.Serializer):
        model = User
    
        """
        Serializer for password change endpoint.
        """
        old_password = serializers.CharField(required=True)
        new_password = serializers.CharField(required=True) 
    
    
    
    class ChangePasswordView(generics.UpdateAPIView):
        """
        An endpoint for changing password inside of profile.
        """
        serializer_class = ChangePasswordSerializer
        model = User
        permission_classes = [permissions.IsAuthenticated]
    
        def get_object(self, queryset=None):
            obj = self.request.user
            return obj
    
        def update(self, request, *args, **kwargs):
            if self.request.method == 'PUT':
              self.object = self.get_object()
              serializer = self.get_serializer(data=request.data)
       
              if serializer.is_valid():
                  # Check old password
                  if not self.object.check_password(serializer.data.get("old_password")):
                      return Response({"old_password": ["Wrong password."]}, status=status.HTTP_400_BAD_REQUEST)
                  # set_password also hashes the password that the user will get
                  self.object.set_password(serializer.data.get("new_password"))
                  self.object.save()
                  response = {
                      'status': 'success',
                      'code': status.HTTP_200_OK,
                      'message': 'Password updated successfully',
                      'data': []
                  }
       
                  return Response(response)
       
              return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST
)