Hi all,
First, I want to say that complex things fail in complex ways, so there
it's probably a fallacy to look for a single root cause. I agree with
various other points about mistakes that were made, but not others.
Particularly:
On 22/11/16 12:41, Florian Apolloner wrote:
Hi,
On Monday, November 21, 2016 at 11:56:56 PM UTC+1, Tim Graham wrote:
… that the existing tests would catch this type of obviously
incorrect issue.
I think that is the main issue here. I was also really surprised to
discover that the tests were missing cases like this -- then again,
writing good tests is hard and I know that I am personally one not to
write good tests usually. Either way, I think the way forward is to
improve the testsuite in this case.
I disagree with this, there was no issue with the existing test suite
for the FBV version. *Tests are never close to exhaustive and we should
never think they are or can be.* Tests are spot checks of correctness in
an infinite universe of possible input values. When trying to work out
what you should test in this vast space, there are a few sensible ways:
1. Look at the requirements, and pick spots that match each requirement.
In this case:
* If a request has the required token, it should be allowed through
* If a request does not have the required token, it should not be
allowed through
2. Make some guesses about well known edge cases and test them (e.g the
empty string case etc.). However this could be a complete waste of time
3. Look at the implementation at work out where it might go wrong. This
one is really important in reality.
And this is what the existing tests did. Out of the infinite number of
HTTP requests to test under point 1, they happened to pick a GET
request, which was perfectly sensible. In the case of FBVs, the control
flow is so clear that all of the following are equally silly tests to add:
* Check that requests with query string parameter 'xyz' doesn't
introduce a back door
* Check that on leap years tokens are checked
* Check that the security is correct for requests that are
specifically GET, POST, HEAD, OPTIONS, DELETE....
The conclusion is this: in CBVs control flow is much, much less clear.
In the FBV the mistake would have been immediately obvious, the CBV base
classes obfuscated things to the point where it wasn't at all clear. To
write adequate tests for the CBV version, under point 3 above, you have
*much, much more work* to do.
I disagree that forcing people to write their own auth views is
*necessarily* worse than providing a CBV that people can customise.
Given this demonstration of how CBVs obfuscate control flow, it's quite
possible that writing an FBV from scratch will be less error prone than
using a CBV and subclassing, especially when we have encapsulated things
like the token checking. Personally I would be massively happier
auditing a custom FBV than a subclassed CBV, especially when you
consider that the CBV subclass is inheriting from a stack of classes
that could change in later versions of Django in some subtle way that
breaks the code. Subclassing is not necessarily safer, it just feels
safer ("I'm only changing this one little method"), and *that feeling of
greater safety is itself a huge danger*.
Going forward, I think we need to:
1. Recognise that CBVs are much harder to reason about, and therefore
require much more care.
2. Avoid using CBVs unless you really need them.
3. Recognise that tests for FBVs are inadequate against the CBV translation.
4. Not deprecate the FBVs. At the very least, they provide a sensible
and easy to understand starting point for people that want to copy the
logic to create their own, and do so in a way that they actually
understand and can audit.
I'd be +0 on reverting to an FBV only, but it's part of a much bigger
discussion
Regards,
Luke
--
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/e160c9a0-0af0-7767-4b35-cb02a92d6a4d%40cantab.net.
For more options, visit https://groups.google.com/d/optout.
