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

Meta: Upgrade Django to 1.10 #4174

Closed
4 of 5 tasks
stsewd opened this issue May 31, 2018 · 9 comments · Fixed by #4319
Closed
4 of 5 tasks

Meta: Upgrade Django to 1.10 #4174

stsewd opened this issue May 31, 2018 · 9 comments · Fixed by #4319
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Milestone

Comments

@stsewd
Copy link
Member

stsewd commented May 31, 2018

There is a PR already to update Django to 2.0 (#3305), but as the official docs from django says

If you’re upgrading through more than one feature version (e.g. A.B to A.B+2), it’s usually easier to upgrade through each feature release incrementally (A.B to A.B+1 to A.B+2) rather than to make all the changes for each feature release at once. For each feature release, use the latest patch release (A.B.C).

https://docs.djangoproject.com/en/2.0/howto/upgrade-version/

Is better to upgrade incrementally, and there are some breaking changes related to very delicate parts of the code (like the middleware), so we need to make sure that those parts are well tested.

I'll be putting some of the parts of the code that we need to update before and after and the dependencies that support the new django version.

The changelog from the 1.10 version: https://docs.djangoproject.com/en/2.0/releases/1.10/

That's the most relevant information that I could extract from reading the changelog.

@stsewd stsewd added the Improvement Minor improvement to code label May 31, 2018
@agjohnson agjohnson added this to the 2.6 milestone May 31, 2018
@ericholscher
Copy link
Member

I'd love to get upgraded, so big +1 from me on this one.

@ericholscher ericholscher modified the milestones: 2.6, Cleanup Jun 5, 2018
@safwanrahman
Copy link
Member

@mashrikt Can you take a look into this? Maybe its interesting for you.

@agjohnson agjohnson added the Accepted Accepted issue on our roadmap label Jun 8, 2018
@agjohnson
Copy link
Contributor

+1 from me, but not on our immediate roadmap. If anyone non-core wants to take this on, go for it. Targeting 3.0 as this will be a backwards incompat version change.

@agjohnson agjohnson modified the milestones: 2.7, 3.0 Jun 8, 2018
@stsewd
Copy link
Member Author

stsewd commented Jul 2, 2018

I was playing around with this, I found https://docs.djangoproject.com/en/2.0/releases/1.10/#removed-weak-password-hashers-from-the-default-password-hashers-setting we are using a salted sha1 to hash the user passwords (at least in our tests fixtures), these hashes were removed in 1.10. We need to check if we have these hashes in the db (I'm not sure and the docs didn't mention, but what will happen to this users? the change is automatically? Are their required to change the password?)

@stsewd
Copy link
Member Author

stsewd commented Jul 2, 2018

Also, I found that we need to upgrade tastypie, because of https://docs.djangoproject.com/en/2.0/releases/1.10/#features-removed-in-1-10 (tastpie 0.13.1 uses get_all_field_names() we need to update to 0.13.2/3)

@stsewd stsewd mentioned this issue Jul 2, 2018
4 tasks
@ericholscher
Copy link
Member

ericholscher commented Jul 3, 2018

Yea, confirming that we have some salted sha1. We should probably do something like this and migrate everyone at once: https://docs.djangoproject.com/en/2.0/topics/auth/passwords/#password-upgrading-without-requiring-a-login

The current default is django.contrib.auth.hashers.PBKDF2PasswordHasher -- so the only ones left with SHA1 I believe are people who haven't logged in in a long time.

@davidfischer
Copy link
Contributor

I checked our database for users with old passwords. We have no unsalted passwords. We have a ~3k (out of ~100k) users who are using old salted sha1. This means they haven't logged in since RTD upgraded to Django 1.4.

One possible option rather than adding a wrapped password hasher is to set an unusable password for users who haven't logged in since the sha1 days. This will remove their hashed password from the DB and they will be required to request a password reset or login with social auth to access their account. This is probably fine since these users are probably never going to login to their accounts.

@ericholscher
Copy link
Member

I don't feel strongly here. I'm fine just unsetting their passwords and making them reset. I do wish there was a better way to show in the UI (eg. your password must be reset for security reasons) instead of just "wrong password" -- but I don't think it's worth the time to build that for the few users who might eventually login.

@davidfischer
Copy link
Contributor

I created a migration to set unusable passwords in #4750. It also upgrades to Django 1.11.16 (the latest LTS). I builds on @stsewd's excellent work. If needed, the 1 commit that makes the migration could be cherry picked from 9f2c9bd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants