Code

Opened 8 months ago

Last modified 6 months ago

#20917 assigned New feature

Change the password hashers when testing

Reported by: mjtamlyn Owned by: ashchristopher
Component: Testing framework Version: master
Severity: Normal Keywords:
Cc: tom@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Disclaimer: I'm not completely sure this is a good idea as a default.

The default password hasher is very secure, and very slow to create passwords. This is never an issue in production, but in testing it is *amazingly* slow. Most of the time using the unsalted MD5 hasher as settings.PASSWORD_HASHERS[0] has resulted in a six-fold increase in speed in my test suites. To be honest, I think we could use a "non-hashing" hasher in these cases.

I'd like to change the "default" to insert this new non-hashing hasher as settings.PASSWORD_HASHERS[0] during setup_test_environment(). For anyone who does not know about this trick, their test suits will automatically speed up. Any tests expecting a certain hasher to have been used when creating would fail in a backwardsly incompatible manner. Any fixtures or similar with passwords created using another hasher would still be valid, but would then update to be the raw password on success. Of course, to validate these passwords the password text would need to be included in plain text in the test suite (See #20916 for an alternative solution to this issue).

Other comparable setting changes done in this way include: turning off translations, using the console email backend, removing allowed_hosts checking, turning debug off.

Am I mental, or is this a sensible optimisation?

Attachments (0)

Change History (10)

comment:1 Changed 8 months ago by tomchristie

  • Cc tom@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 8 months ago by timo

  • Triage Stage changed from Unreviewed to Accepted

I think the idea is definitely worth exploring. There's a note in the docs, but it's rather buried: https://docs.djangoproject.com/en/dev/topics/testing/overview/#speeding-up-the-tests

comment:3 Changed 8 months ago by ashchristopher

  • Owner changed from nobody to ashchristopher
  • Status changed from new to assigned

comment:4 Changed 8 months ago by ashchristopher

I was thinking of adding a TEST_PASSWORD_HASHER to the settings which by default is set to None. If TEST_PASSWORD_HASHER is None, then we don't override the PASSWORD_HASHER tuple and password hashing works as normal.

This would allow backwards compatibility.

comment:5 Changed 8 months ago by anonymous

I'm not sure "backwards compatibility" needs to be kept in this instance. Nothing is really broken by the change, unless somebody is manually inspecting the hash. In this case, fast by default may be the best solution. (But maybe keep the MD5 hasher instead of a no-op hasher, just in case somebody is doing something incredibly stupid.)

comment:6 Changed 8 months ago by ashchristopher

If people have overridden the PASSWORD_HASHER with their own custom hasher, or if they have updated the PASSWORD_HASHER in say a test_settings.py file (which is how we solved this) then having an opinion on the password hasher in tests would break their tests.

I know that our test suite would break if we defaulted to a specific hasher in tests (though this is basically a data point of 1).

comment:7 Changed 8 months ago by ashchristopher

Discussed this issue with julienphalip on irc (#django-sprints) and we both think that maybe this isn't actually something Django should have an opinion on.

[10:40:54] <+julienphalip> ashc: So I'm feeling a bit divided about #20917.
[10:40:54] <ticketbot> https://code.djangoproject.com/ticket/20917
[10:41:07] <ashc> julienphalip: re: #20917, I am not even sure if it is a real code problem
[10:41:08] <ticketbot> https://code.djangoproject.com/ticket/20917
[10:41:21] <ashc> part of me thinks it is a documentation issue
[10:41:32] <+julienphalip> ashc: I think so too
[10:41:34] <ashc> or rather, a "this is how you should do tests" problem
[10:42:06] <ashc> some people just rely on settings.py + local_setting.py and others (like us) have a special testing_settings.py file we use
[10:42:17] <ashc> and that is where we override the PASSWORD_HASHERS
[10:42:30] <+julienphalip> ashc: Yep
[10:42:56] <+julienphalip> I think it's best to let users consciously change the hashers setting for testing
[10:43:28] <ashc> Is there anything we can do to make this more clear for people? Does Django have an opinion on test settings files?
[10:44:44] <+julienphalip> ashc: I'm all for speeding up tests' execution, but if we enforced a weak hasher then I'd like to be sure there can be no weird surprise and no discrepancy between the test and production environments.
[10:45:14] <+julienphalip> ashc: I don't think Django as a project has any particular recommendation. There are many different techniques....
[10:48:08] <ashc> julienphalip: I am thinking things are kind of ok as they are, and maybe we just close the ticket with an explanation that Django shouldn't have an opinion on this matter, and that there is documentation that will allow developers to apply the speed fixes themselves?
[10:49:32] <+julienphalip> ashc: I tend to agree. It'd be great to have a chat with mjtamlyn about this first to see what he thinks.
[10:51:43] <+julienphalip> ashc: This section could be beefed up with more tips as well: https://docs.djangoproject.com/en/dev/topics/testing/overview/#speeding-up-the-tests

comment:8 Changed 8 months ago by jcd

Apologies. Anonymous above (comment:5) is me. Is there a way to check if the hasher has been manually set, and use that if so, but to have it default to something fast if no hasher has been specified? I think a fast test suite is something we want people to be able to use by default. My concern is that requiring TEST_PASSWORD_HASHER to be set to get fast tests doesn't solve the problem, it just moves the workaround from supplying a separate settings file to supplying an extra setting. It's better, but we're still leaving new users with slow tests.

Or, if the solution requires TEST_PASSWORD_HASHER to be set, include it in the default settings file created by makeproject.

comment:9 Changed 7 months ago by mjtamlyn

I don't personally feel that this is any different to using an in memory database by default for SQLite rather than a file. The performance is different to in production (anyway, why are you using SQLite in production?), but this is what you expect from testing. I agree there are a number of ways to override settings, and there's the slight issue of what to do if someone wants to use something different, but in my experience I have *always* had to set this for tests to get any kind of performance. It's not a last-resort method to speed tests up, it's the first thing you should do, and as we continue to increase the iterations it will get worse.

There are a small number of users who may run into this problem, but for 90%+ of users it saves remembering to set it up, or not knowing about it at all, deciding the tests are too slow to run, getting bored and never writing any. It's not a marginal gain - on small examples where tests create users with passwords and log them in to the site I was seeing a 6x speed increase.

So in summary:

  • Most users won't notice
  • We need a mechanism so that those who do (and don't want it) can disable it
  • Will save most projects a few lines of code
  • Will help adoption of testing as it gives a better initial impression

comment:10 Changed 6 months ago by benoitbryon

I just released https://pypi.python.org/pypi/django-plainpasswordhasher, which is a no-op password hasher. Yes, very dangerous in production, but pretty quick for tests.

I have no benchmark actually, but I guess "no encoding" is the fastest encoding implementation, isn't it?

What about using it as default or recommended password hasher when running tests?

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as assigned
The owner will be changed from ashchristopher to anonymous. Next status will be 'assigned'
The ticket will be disowned. Next status will be 'new'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.