As a big fan of GCBVs, this got me out of the chair :) I am -1 for removing GCBVs for authentication.
My reasons: 1) Security improvements also happen over time, finding a security hole in a component is not a reason to point the finger at the architecture. If you entirely remove something, you will have to rebuild it somewhere else, and new security holes will emerge. Thinking that users of builtin GCBVs will just opt for the FBVs is wrong, we won't, we'll write our own GCBVs or find some third-party that has less attentive eyes than the django.contrib.auth 2) In this case, having security checks in `get_context_data()` is far from the intentions of that method. If I override it, I would not expect security properties of the view to be removed, right? We shouldn't put security checks there, simple as that. If we assume the design pattern is (G)CBV, then this is a violation of the design principles and should have been rejected even before the security issue happened. In a GCBV, we should always ask "Is this logic to be expected by someone inheriting from this GCBV". 3) People have implemented and deployed GCBV from auth. The consequence of removing them because we don't want to deal with their security issues is alarming! I inherit from these classes, believing that we will be stronger together, because inheriting from a common codebase will have more eyes on the security aspects. This will be a bad example or bad signal to set for other usages of GCBVs. 4) SomeViewClass.as_view() generates an FBV, I'm sure we can bridge CBVs and FBVs if the concern is about having separate codebases as Baptiste expressed it. 5) I fully agree with Baptiste: If Django does not supply easily pluggable authentication patterns, someone else will (and not just 1 project, many will popup, they already have), and the effort will be fragmented, the security will be weakened. 6) I write CBVs in my own apps, I know there is a lot of afterthought and you often have to move logic between CBVs, mixins, and method decorators. I fully understand and sympathize with release notes that have entries about design changes and small refactors in GCBVs, that's awesome and you can learn from the considerations and decisions in Django's GCBVs. Thanks :) Ben On Monday, November 21, 2016 at 8:11:34 PM UTC+1, 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/057b4a9c-0939-4def-93e5-f1b192912c55%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
