I haven't extended these views much, so I can't talk about the pain points of extending the function-based views compared to the ease of extending the classes. I'm certainly more confident about reasoning with function-based code. There was a draft patch [0] a few months ago that converted some admin views to class-based views. I found it much more difficult to follow to the point where I didn't feel it was an improvement.
When reviewing the patch for the auth class-based view additions, I made the mistake of assuming that the existing tests would catch this type of obviously incorrect issue. Perhaps with the function-based views, the code was "too obviously correct" that a test for token verification on POST wasn't considered. I'd like to think that future work on the class-based views would be more careful about testing, especially after we made this mistake. If you feel the class-based views aren't reliable enough to deprecate the function-based views immediately, then I would say we shouldn't ship those views in Django in the first place. contrib.auth isn't a good place to ship "experimental API." The class-based views would certainly benefit from further audit now that we realize this type of mistake is possible. [0] https://github.com/django/django/pull/6045 On Monday, November 21, 2016 at 2:11:34 PM UTC-5, Markus Holtermann wrote: > > Hi all, > > As it turned out [1], due to their complexity, using class-based generic > views for security-sensitive functionality can result in unintended > behavior. Essentially, the reset token was only checked on GET requests, > not on POST. This was due to the check being in `get_context_data()` (which > is only called on GET but not POST except for invalid forms) and not higher > up the stack. Validation could happen in the SetPasswordForm but doesn't > really belong there either. The form is being used by the admin to allow > superusers to change other users' password. Also, password resets could > probably happen via other ways that want to leverage a form that doesn't > require a token. In the end, from my perspective the check for the correct > token does belong in the view. > > While the reported issue was fixed [2] it raises the question if the added > functionality of class-based generic views is worth the danger of shooting > ourselves in the foot. I see the benefits of GCBVs. But given that the > reported issue stayed unnoticed for 4 months makes me think that those > views are not the best for these use cases and easily underpin the security > functionality. Hence I suggest to revert the patch (including all > additional features they gained) unless they are integrated in the > function-based views and add guidelines on how to use class-based generic > views for security sensitive feature. > > This is the thread to get the discussion about this started. > > One thing I want to suggest regardless if the class-based generic views > are removed again or not, is to hold off the deprecation of the > function-based views. This allows users who feel the same to not use > class-based generic views without having deprecation warnings. At least > until the next LTS release. > > Furthermore, myself and Florian Apolloner, who discovered the issue, are > leaning +0 to +1 on the revert of the class-based views. > > Cheers, > > Markus Holtermann > > [1] > https://www.djangoproject.com/weblog/2016/nov/21/passwordresetconfirmview-security-advisory/ > [2] https://github.com/django/django/pull/7591 > -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at https://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/491d8668-2024-4dd4-bf1e-5dfca54aab08%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
