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

Update Django to 5.1 #1633

Closed
pauloxnet opened this issue Sep 26, 2024 · 7 comments · Fixed by #1792
Closed

Update Django to 5.1 #1633

pauloxnet opened this issue Sep 26, 2024 · 7 comments · Fixed by #1792
Labels
dependencies Pull requests that update a dependency file on hold

Comments

@pauloxnet
Copy link
Member

pauloxnet commented Sep 26, 2024

At the moment we are using Django 4.2 but Django 5.1 to be able to use all the latest features and bugfixes.

In particular can be useful to start using GeneratedField to improve how page are shown in the documentation form the metadata JSONfield.

We have to wait for the resolution of #1574

@ulgens
Copy link
Contributor

ulgens commented Dec 4, 2024

It looks like there is a blocking backward incompatible change:

The django.contrib.auth.hashers.SHA1PasswordHasher, django.contrib.auth.hashers.UnsaltedSHA1PasswordHasher, and django.contrib.auth.hashers.UnsaltedMD5PasswordHasher are removed.

https://docs.djangoproject.com/en/5.1/releases/5.1/#features-removed-in-5-1

SHA1PasswordHasher is being used by PBKDF2WrappedSHA1PasswordHasher, which added to the codebase in #634 .

I have two questions:

  • Is this still needed? I was thinking that the modern Django is handling this case itself.
  • Is there any user left with SHA1-hashed passwords?

@bmispelon
Copy link
Member

Great questions! 🎸

Is this still needed? I was thinking that the modern Django is handling this case itself.

Django will automatically upgrade the hashing algorithm of your password when you log in (using a different hashing algorithm requires access to the password in clear so it can't be done in a migration for example). That means that accounts that have no logged in in a long time are still using old hashers. This is not caused by an old version of Django, this is because the website has been around for a long time.

Is there any user left with SHA1-hashed passwords?

I took a look inside the database, and out of the ~50 000 active users that are in the table, there are ~8 000 still using the pbkdf2_wrapped_sha1 algorithm (the last one logged in in 2013). Out of all of these, there's a single one that does not have an email address set (which means they won't be able to use the "password reset" feature to regain access to their account if we disable the SHA1 hasher).

Given those numbers, I'm OK getting rid of the custom SHA1 hasher. I would just like to confirm that any user still using that hasher would be able to use the password reset feature (having a test for that would be ideal).

I hope that helps!

@pauloxnet
Copy link
Member Author

Is this still needed? I was thinking that the modern Django is handling this case itself.
Is there any user left with SHA1-hashed passwords?

I hope that helps!

@ulgens do you think you can continue working on that with the feedback received?

@ulgens
Copy link
Contributor

ulgens commented Jan 20, 2025

@ulgens do you think you can continue working on that with the feedback received?

I will check the comments again gonna add if there is any adiditional questions (very soon 🌻)

@ulgens
Copy link
Contributor

ulgens commented Jan 21, 2025

Okay, so we are getting rid of the old hasher and adding a test for password reset, right? Initially, I thought we needed to do something more complex, like automatically triggering the password reset flow for users with old hashes, but I don't think it's necessary.

I can handle them on the weekend.

@bmispelon
Copy link
Member

From what I understand we only use the plain PasswordResetView from Django for password reset, so we can assume it's well tested already.

All that's needed is to remove the PASSWORD_HASHERS setting (we can rely on the default one) and the associated code in accounts/hashers.py.

ulgens added a commit to ulgens/djangoproject.com that referenced this issue Jan 21, 2025
@ulgens
Copy link
Contributor

ulgens commented Jan 21, 2025

Pushed my changes with 2 questions on the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file on hold
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants