Django

Code

Ticket #6083 (closed: fixed)

Opened 7 months ago

Last modified 2 months ago

contrib.auth still uses oldforms

Reported by: telenieko Assigned to: brosner
Milestone: Component: Contrib apps
Version: SVN Keywords: auth forms newforms oldforms
Cc: roppert Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 1

Description

Could not find a ticket stating it so here it is. the title says it all :)

Attachments

auth-uses-newforms.diff (20.5 kB) - added by cogat on 12/12/07 06:07:20.
auth-uses-newforms-2.diff (20.6 kB) - added by dstndstn@gmail.com on 01/17/08 18:17:20.
includes a one-liner update for delete_test_cookie() problem
auth-uses-newforms-3.diff (20.0 kB) - added by Michael Newman <newmaniese@gmail.com> on 02/14/08 10:43:00.
Fixed reference to comments.auth.py and added fix to admin.templates.admin.auth.user.change_password.html for new forms and slight change to the way contrib.auth.forms.AdminPasswordChangeForm? cleans the form, and changed auth.views.py to use AdminPasswordChangeForm? instead of the now nonexistent old form way
03-newforms-auth.diff (18.6 kB) - added by Petr Marhoun <petr.marhoun@gmail.com> on 02/14/08 13:39:20.
auth-uses-oldforms-3.diff (22.6 kB) - added by Michael Newman <newmaniese@gmail.com> on 02/20/08 13:28:03.
fixes a few issues and changes the user add view into newforms
6083_nfa_newforms_auth.diff (21.7 kB) - added by brosner on 03/01/08 16:09:02.
patch against newforms-admin.
6083_newforms_auth2.diff (33.5 kB) - added by brosner on 03/02/08 12:42:56.
more complete with tests and docs. looking for feedback.

Change History

12/01/07 19:23:01 changed by Simon G <dev@simon.net.nz>

  • needs_better_patch changed.
  • stage changed from Unreviewed to Accepted.
  • needs_tests changed.
  • needs_docs changed.

12/01/07 20:54:04 changed by Greg Turner <greg@gregturner.org>

  • owner changed from nobody to anonymous.
  • status changed from new to assigned.

12/01/07 20:54:36 changed by cogat

  • owner changed from anonymous to cogat.
  • status changed from assigned to new.

12/03/07 10:57:37 changed by cogat

  • keywords set to auth forms newforms oldforms.
  • needs_docs set to 1.
  • has_patch set to 1.
  • needs_tests set to 1.

I've made the changes to contrib.auth (my first Django patch!). The oldforms are used in contrib.admin and contrib.comments, and I have patched these too. Contrib.admin now uses the newforms in authentication forms. contrib.comments subclasses the oldform AuthenticationForm?, and has heavy reliance on validators so I made a copy of AuthenticationForm? and put it in comments.py.

I'll update the documentation if the approach seems sound. Shall I also update the newforms branch?

12/12/07 06:07:20 changed by cogat

  • attachment auth-uses-newforms.diff added.

01/05/08 02:27:04 changed by brosner

Marked #6318 as duplicate. It has a patch as well so perhaps they needed to be merged unless they really accomplished the same exact thing with little differences.

01/17/08 18:13:33 changed by dstndstn@gmail.com

I encountered the following error when using Django's test framework fake web client (django.test.client.Client):

----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/gmaps/test/tilecache/an/portal/test_login.py", line 72, in testLoginWorks
    resp = self.client.post(self.loginurl, { 'username': self.u1, 'password': self.p1 })
  File "/home/gmaps/django-test//lib/python2.4/site-packages/django/test/client.py", line 238, in post
    return self.request(**r)
  File "/home/gmaps/django-test//lib/python2.4/site-packages/django/core/handlers/base.py", line 82, in get_response
    response = callback(request, *callback_args, **callback_kwargs)
  File "/home/gmaps/django-test//lib/python2.4/site-packages/django/contrib/auth/views.py", line 23, in login
    request.session.delete_test_cookie()
  File "/home/gmaps/django-test//lib/python2.4/site-packages/django/contrib/sessions/backends/base.py", line 69, in delete_test_cookie
    del self[self.TEST_COOKIE_NAME]
  File "/home/gmaps/django-test//lib/python2.4/site-packages/django/contrib/sessions/backends/base.py", line 38, in __delitem__
    del self._session[key]
KeyError: 'testcookie'

----------------------------------------------------------------------

And this was happening because I was just POSTing to the login form with the "username" and "password" values without setting the test cookie. Then sessions.delete_test_cookie() tries to delete the cookie regardless of whether it exists or not (this seems like a bug to me...), and instead of getting an error message or anything, it just crashes. I added a one-liner fix:

--- views.py-old        2008-01-18 00:08:10.000000000 +0000
+++ views.py    2008-01-18 00:08:19.000000000 +0000
@@ -20,7 +20,8 @@
                 redirect_to = settings.LOGIN_REDIRECT_URL
             from django.contrib.auth import login
             login(request, form.get_user())
-            request.session.delete_test_cookie()
+            if request.session.test_cookie_worked():
+                request.session.delete_test_cookie()
             return HttpResponseRedirect(redirect_to)
     else:
         form = AuthenticationForm(request=request)

The updated patch I will attach contains this tweak and is against a more recent SVN rev so the patch applies cleanly.

01/17/08 18:17:20 changed by dstndstn@gmail.com

  • attachment auth-uses-newforms-2.diff added.

includes a one-liner update for delete_test_cookie() problem

02/14/08 10:43:00 changed by Michael Newman <newmaniese@gmail.com>

  • attachment auth-uses-newforms-3.diff added.

Fixed reference to comments.auth.py and added fix to admin.templates.admin.auth.user.change_password.html for new forms and slight change to the way contrib.auth.forms.AdminPasswordChangeForm? cleans the form, and changed auth.views.py to use AdminPasswordChangeForm? instead of the now nonexistent old form way

02/14/08 13:09:00 changed by roppert

  • cc set to roppert.

02/14/08 13:39:20 changed by Petr Marhoun <petr.marhoun@gmail.com>

  • attachment 03-newforms-auth.diff added.

02/14/08 13:40:17 changed by Petr Marhoun <petr.marhoun@gmail.com>

I tried another approach for this problem - not to remove oldforms versions, but to add newforms version only. I think it is not necessary to force everybody to port authorization code at once.

Another idea is to port "post data" logins (without redirections, with saving of post data) from oldforms-admin and newforms-admin to auth - so newforms-admin wouldn't need any authorization code. New parameter extra_context (#5298) is necessary for it - newforms-admin needs more context variables.

I am not sure that this patch works fully - I have checked only some forms. (It should be part of bigger series of tickets which is not ready.) I mean my patch mainly as a comment now. (I read about this ticket today on django-developers.)

02/20/08 13:28:03 changed by Michael Newman <newmaniese@gmail.com>

  • attachment auth-uses-oldforms-3.diff added.

fixes a few issues and changes the user add view into newforms

02/20/08 13:38:11 changed by Michael Newman <newmaniese@gmail.com>

  • needs_better_patch set to 1.

This new patch does not have an import of django.contrib.auth.admin in it, because while using auth multiple calls need to be made to models, auth (init), forms, etc. I never received a design decision from https://groups.google.com/group/django-developers/browse_thread/thread/ba8b115b74d6e502 . I would like to take Petr's patch to Django Devs as well, it is my opinion that as new forms merges into the trunk switches need to be made to all of admin and, in all reality, the switch of Auth would be relatively automatic.

03/01/08 16:09:02 changed by brosner

  • attachment 6083_nfa_newforms_auth.diff added.

patch against newforms-admin.

03/01/08 16:12:59 changed by brosner

  • owner changed from cogat to brosner.

I have added a patch against newforms-admin. I will eventually be committing the final patch there. My patch current breaks comments since it had dependancy on the oldforms AuthenticationForm?. I will be looking more into whether just moving the oldforms version over will be correct way.

03/01/08 16:13:27 changed by brosner

  • status changed from new to assigned.

03/01/08 18:37:30 changed by brosner

Development of this patch can be found at http://github.com/brosner/django/tree/newforms_auth

03/02/08 12:42:56 changed by brosner

  • attachment 6083_newforms_auth2.diff added.

more complete with tests and docs. looking for feedback.

03/02/08 12:44:08 changed by brosner

  • needs_docs deleted.
  • needs_tests deleted.

I have attached a more complete version of the patch. I have moved some files in django.contrib.auth.tests to make it easier for other parts of the app.

03/03/08 14:37:42 changed by brosner

  • status changed from assigned to closed.
  • resolution set to fixed.

(In [7191]) newforms-admin: Fixed #6083 -- Updated django.contrib.auth to use newforms. A big thanks to Greg Turner, Michael Newman and Petr Marhoun.

05/13/08 09:50:45 changed by gwilson

(In [7525]) newforms-admin: Cleaned up imports, refs #6083.


Add/Change #6083 (contrib.auth still uses oldforms)




Change Properties
Action