1
\$\begingroup\$

I wrote a code to resetting 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()
\$\endgroup\$
0

2 Answers 2

0
\$\begingroup\$

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
)
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Please edit your answer and explain why it would be better to use serializers. \$\endgroup\$
    – pacmaninbw
    Sep 8 at 22:21
  • 1
    \$\begingroup\$ Welcome to Code Review! While it seems that short answers are acceptable are you able to expand this answer, perhaps explaining reasons for the suggestion? \$\endgroup\$ Sep 8 at 22:29
0
\$\begingroup\$

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
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.