Code

Opened 6 years ago

Closed 14 months ago

Last modified 14 months ago

#8404 closed Bug (fixed)

Auth password reset tests are too restrictive about template requirements

Reported by: mtredinnick Owned by: nobody
Component: contrib.auth Version: master
Severity: Normal Keywords:
Cc: siddhartag@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The tests in django.contrib.auth.tests.views.PasswordResetTest check for a correct "failure to submit" with an invalid email address by looking for a particular error message string. The problem is that this string actually reveals that a particular email address isn't on the system. So if somebody writes a password reset template for their own sites that doesn't reveal the presence or absence of a user (an ITS requirement in some organisations, e.g. financial sites), there is no way to have that test pass.

So we need to come up with a better way to test for "success" (i.e. failure to submit the form) when the email address doesn't exist in the system. Possibly just easing back and checking for the existence of form.errors in the template rendering will be enough (or the existence of that error message in the context used for rendering), rather than checking the actual string output so carefully is enough. But maybe somebody has another idea.

Attachments (0)

Change History (15)

comment:1 Changed 6 years ago by jacob

  • milestone changed from 1.0 maybe to post-1.0
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:3 Changed 5 years ago by jacob

  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 5 years ago by fnl

An additional problem with the tests: self.assert_("Please enter your new password" in response.content) makes restrictions which content the page has to contain, and disregards the possible use of not only other phrases but also languages. This is a prevalent problem in tests.forms and tests.views. If such tests are to be included, they should either use strings the site's implementation defines somewhere or not provide test for content at all, only as some sort of test template the site implementation can use to test the content as the site will use it. The currently only "way" of dealing with this is changing the files in the Django distribution, a very messy way of handling this problem.

comment:5 Changed 5 years ago by fnl

OK, I went through all auth tests to see what creates these problems; auth.tests assumes the implementation uses the English language (directly or via i18n) and an exact pre-defined phrase set; all problems are in the file auth.tests.views:

 23:        settings.LANGUAGES = (('en', 'English'),)
 24:        settings.LANGUAGE_CODE = 'en'
 43:        settings.LANGUAGES = (('en', 'English'),)
 44:        settings.LANGUAGE_CODE = 'en'
 55:        self.assertContains(response, "That e-mail address doesn't have an associated user account")
 82:        self.assert_("Please enter your new password" in response.content)
 92:        self.assert_("The password reset link was invalid" in response.content)
119:        self.assert_("The password reset link was invalid" in response.content)
126:        self.assert_("The two password fields didn't match" in response.content)
146:        self.assert_("Please enter a correct username and password. Note that both fields are case-sensitive." in response.content)
160:        self.assert_("Your old password was entered incorrectly. Please enter it again." in response.content)

These lines are the fault that calling "./manage.py test" produces errors in any current django distribution (the lines are from SVN) when using the auth module and trying to run the global test procedure and the two artificial assumptions (english language and exactly those above phrases) don't hold. IMHO, this should not be part of a base distribution of any WDF!

comment:6 Changed 5 years ago by siddhi

  • Cc siddhartag@… added

Some situations I've encountered in the past -

  1. Using the auth app just for the models where I've implemented my own views and templates. The auth view unit tests fail complaining that it cant find the templates.
  1. Renaming the templates and setting the name in the urlconf. The test suite overrides the urlconf, so again it won't find the templates.
  1. Referencing urls to views in other apps in my templates. Again the urlconf is overwritten in the suite, so it wont be able to do a reverse lookup to other apps and template rendering fails.
  1. Then there are the phrasing issues mentioned in the above comment.

I think this ticket is too narrow in scope. There is a general problem in the unit tests that it does not setup an independent, isolated environment which can be setup by the suite for running the tests. Making any sort of change in the production configuration can impact unit test success. I can touch a url, template, add an auth backend, change the middleware and in all cases have unit tests from another app fail spectacularly. We should be able to run the unit tests independently of the settings in a specific project.

I also want to link this up to Ticket #7611 and the discussions at

http://groups.google.com/group/django-developers/browse_thread/thread/ac888ea8567ba466/63637d699383a1c6

and

http://groups.google.com/group/django-developers/browse_thread/thread/5d2e10405227b2fc/2fa326461ec87adf

comment:7 Changed 4 years ago by fnl

  • Cc fnl added

comment:8 Changed 3 years ago by gabrielhurley

  • Component changed from Contrib apps to contrib.auth

comment:9 Changed 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

comment:10 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:11 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:12 Changed 14 months ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 142ec8b2835c242339b930c47a70a3c7036df91d:

Fixed #8404 -- Isolated auth password-related tests from custom templates

comment:13 Changed 14 months ago by claudep

I'm aware that the committed fix does not resolve all isolation issues of auth tests, but please, open separate tickets for each identified issue.

comment:14 Changed 14 months ago by Claude Paroz <claude@…>

In f1029b308f3ea967a5d93aea2b730671898a56f5:

Fixed a misnamed variable introduced in commit 142ec8b283

Refs #8404.

comment:15 Changed 14 months ago by fnl

  • Cc fnl removed

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.