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

Password reset throws error with django 3.1 #118

Closed
IIFelix opened this issue Aug 7, 2020 · 14 comments
Closed

Password reset throws error with django 3.1 #118

IIFelix opened this issue Aug 7, 2020 · 14 comments
Assignees
Labels
bug Something isn't working

Comments

@IIFelix
Copy link

IIFelix commented Aug 7, 2020

Hello,

recently updated django to version 3.1 and the password reset view throws error:
django.urls.exceptions.NoReverseMatch: Reverse for 'password_reset_confirm' with keyword arguments '{'uidb64': 'MTA', 'token': 'a88hdr-5fb0f72acbe1e9d2b81b7810dee31037'}' not found. 1 pattern(s) tried: ['usermanagement\/password-reset/confirm/(?P[0-9A-Za-z_\-]+)/(?P[0-9A-Za-z]{1,13}-[0-9A-Za-z]{1,20})/$']

If i roll back to django 3.0 it works again.

@iMerica
Copy link
Owner

iMerica commented Aug 7, 2020

Hi @IIFelix, thanks for raising this issue. I'll investigate this evening.

@iMerica iMerica self-assigned this Aug 7, 2020
@iMerica iMerica added the bug Something isn't working label Aug 7, 2020
@mohmyo
Copy link
Contributor

mohmyo commented Aug 7, 2020

The problem here is that the resulted token length is longer than the length defined in the URL pattern.

I believe that the recent changes with the hashing algorithm are responsible for this

check,
https://github.com/django/django/blob/99abfe8f4d3caebcd73548f5bf9e4755bdfed318/django/contrib/auth/tokens.py#L64

@iMerica
Copy link
Owner

iMerica commented Aug 9, 2020

Can you show us all your URLS on your app @IIFelix ? In Django Rest Auth we have this URL for password_reset_confirm:

path('password/reset/confirm/', PasswordResetConfirmView.as_view(), name='rest_password_reset_confirm'

This URL pattern does not accept kwargs or patterns, so this suggest you might be having an issue with a custom URL that has overridden this one.

@iMerica
Copy link
Owner

iMerica commented Aug 9, 2020

All our tests currently pass in Django 3.1

Tox output

  python3.5-django22: commands succeeded
  python3.6-django22: commands succeeded
  python3.7-django22: commands succeeded
  python3.8-django22: commands succeeded
  python3.6-django31: commands succeeded
  python3.7-django31: commands succeeded
  python3.8-django31: commands succeeded

Passing build in Circle CI

@mohmyo
Copy link
Contributor

mohmyo commented Aug 10, 2020

Can you show us all your URLS on your app @IIFelix ? In Django Rest Auth we have this URL for password_reset_confirm:

path('password/reset/confirm/', PasswordResetConfirmView.as_view(), name='rest_password_reset_confirm'

This URL pattern does not accept kwargs or patterns, so this suggest you might be having an issue with a custom URL that has overridden this one.

I think he may followed the demo project, here is a past issue with an exmaple for what I did back then which is the same as the url in the issue.
#22 (comment)

@mohmyo
Copy link
Contributor

mohmyo commented Aug 10, 2020

If I'm right, you should follow Michael's suggestion and use the mentioned url pattern instead of the one in the demo project.
it should work as expected

@mohmyo
Copy link
Contributor

mohmyo commented Aug 10, 2020

But thanks for raising this, now, I know that password reset token length got increased which is better.
Also, this should be taken in considration with other packages which are using regex for this part.

@IIFelix
Copy link
Author

IIFelix commented Aug 10, 2020

My urls inside the usermanagement app look like this:

`from django.urls import path, include
from rest_framework.routers import url
from . import views
from django.views.generic import TemplateView

urlpatterns = [
url(r'^password-reset/confirm/(?P[0-9A-Za-z_-]+)/(?P[0-9A-Za-z]{1,13}-[0-9A-Za-z]{1,20})/$',
TemplateView.as_view(template_name="password_reset_confirm.html"),
name='password_reset_confirm'),
path(r'', include('dj_rest_auth.urls')),
path('registration/', include('dj_rest_auth.registration.urls'))
]
`

As @mohmyo commented i just copied the passwort reset confirm url used for generating emails from the demo project.
If i replace the url i got from the demo with the one from @iMerica i get this:

Reverse for 'password_reset_confirm' not found. 'password_reset_confirm' is not a valid view function or pattern name.

In your faq (https://dj-rest-auth.readthedocs.io/en/latest/faq.html) at point 2 it's mentioned i should check the urls from the demo project, that's why i copied it.

@mohmyo
Copy link
Contributor

mohmyo commented Aug 10, 2020

Sorry, I think we were confusing the path for dj-rest-auth itself and the path that is used internally by Django for a password reset template that is used to send the email.

I'm using this one at my project

path('password-reset/confirm/<uidb64>/<token>/', TemplateView.as_view(), name='password_reset_confirm'),

@IIFelix
Copy link
Author

IIFelix commented Aug 17, 2020

Works now with your path! tyvm
Also had to change the django rest framework 'url' module to the 'path' module from django urls.

@IIFelix IIFelix closed this as completed Aug 17, 2020
@mohmyo
Copy link
Contributor

mohmyo commented Aug 17, 2020

Glad it helped

@alexerhardt
Copy link

Hi, first of all thanks @mohmyo - your solution worked.

I don't understand however why this is not default behavior in dj-rest-auth - why do we have to wire in Django's password reset template manually?

As a newcomer I'd expect this to work by default in an auth package. There may be a design consideration here, I'm just trying to understand it.

Would it not be good to at least add it to the docs in installation? I'd be happy to amend it if it's suitable and someone can explain the rationale.

@mohmyo
Copy link
Contributor

mohmyo commented Dec 17, 2020

I think @iMerica can put more details on the table regarding design or to address this clearly.
I agree that this should be added to the docs's FAQ section at least considering the number of issues and questions triggered by this.

@lifenautjoe
Copy link

Thanks @mohmyo . Please consider putting this on the demo project.. The FAQ directs users to check the demo folder for fixing this issue, which does not work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants