On Tue, Sep 22, 2009 at 10:34 AM, Luke Plant <l.plant...@cantab.net> wrote: > > OK, you convinced me. I really would rather this wasn't baked in, but given > the migration issues and the fact that it is security related, I guess I can > stomach it. > > I've updated the patch [1] to move things to builtin functionality. I also > had to fix some bugs to get the csrf_protect decorator working for methods, > which are in trunk already. > > I've left most of the code itself under django/contrib/csrf because: > > 1) backwards compatibility with people importing the middleware > means we have to leave django/contrib/csrf for some things > anyway. > 2) In this case, I don't see any great advantage in having stub modules > which just import other stuff for backwards compatibility
This isn't just a "moving deckchairs on the Titantic" thing - I can think of at least three good reasons we should be moving the code into core modules: * Maintaining the basic contract of django.contrib - that you should be able to delete the contrib directory and have Django still work. If all the CSRF code is in contrib, this won't be the case. * Making it clear that this really is a core feature - it's a core template tag, a core middleware, and a core context. Just like the URL is part of the UI for a web app, the module namespace is part of the UI for a library like Django. * For deprecation purposes, we can say that the whole contrib.csrf module will be deprecated in 1.4 (at which point, CSRF will be an entirely core feature). > 3) I really can't be bothered to change all the imports > at this point in time! I can understand this as interim measure while we get consensus with other core developers, but I'd be -1 to leaving the code as is for the final commit. > I moved the template tag itself to core code, because it was causing import > cycles otherwise, and there are no backwards compatibility issues, nor does it > add any actual imports of contrib code to core. Which reinforces the point. Some of the code is going to be in core. Do you want to write the documentation explaining why some code is in contrib and some isn't? > I think the patch now addresses all your/our concerns: Agreed. > I fixed the tests by a custom attribute on request objects that tells the > middleware/decorator to not actually reject requests. This is better than > disabling completely, because it means that the middleware will still send > cookies etc., and it's always good to have tests as close as possible to the > real code. The test client adds the custom attribute to HttpRequests after it > has created them. I had to add the attribute in one other place in the code > as well - a test that was manually calling a view function that had > csrf_protect applied. > > This method of fixing tests was also the best for testing the CSRF middleware > - globally mocking the middleware out would have made it hard to test the > middleware itself! I'm reasonably happy with the testing approach you've taken - or, at least, I can't think of anything substantially better. My only comment is that we might want to put an underscore on the magic variable (i.e., request._dont_enforce_csrf_checks) to reinforce the fact that this really isn't public API. > Docs are all updated, all tests passing etc. > > If people are happy for this to go in, it would be very helpful if other > people could have a go updating their apps and give the general docs/upgrade > instructions/tutorials a good check after I commit it. I can't easily do > checks like that, because I just won't spot the holes after having the code in > my head for so long. Whew. Well, that was a year well spent :-) At this point, I'm convinced, mod the minor things I've flagged. However, I'd like to see Jacob and Malcolm chime in before this is committed. > The only thing left is a nicer render_to_response shortcut for using > RequestContext, which is a refinement we can add later. Agreed, and it's a mostly orthogonal change anyway. Russ %-) --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~----------~----~----~----~------~----~------~--~---
