Model attributes shouldn't override deferred fields

[Ryan Hiebert]

If you have a base model (whether concrete or abstract) with a generic property set, and it is overridden by a field in a subclass.

Consider the following models:

from django.db import models
class Super(models.Model):
    is_super = True
class Sub(Super):
    is_super = models.BooleanField(default=False)

Then, you would see this in a shell session:

>>> sub = Sub.objects.create()
>>> sub.is_super
False
>>> sub_defer = Sub.objects.defer('is_super').get()
>>> sub_defer.is_super
True
>>> sub_defer.refresh_from_db()
>>> sub_defer.is_super
True
>>> Sub.objects.get().is_super
False

The deferred field doesn't get fixed, even by calling refresh_from_db(). It also behaves this way if the Super model is an abstract model. A particular instance of this is with auth.User, which displays this behavior with the is_active field, which gets the value from AbstractBaseUser.

>>> from django.contrib.auth.models import User
>>> User.objects.create_user(username='spam', password='eggs', is_active=False)
<User: spam>
>>> User.objects.get().is_active
False
>>> User.objects.defer('is_active').get().is_active
True

---

I found this bug while trying to get creative, and loading a sparse User model with User.from_db(None, ['id'], [1]), to avoid a database access when I just wanted to use it as a filter argument and I already had the user id. It turned out, though, that it would affect any deferred access of this property, whether using from_db, defer, or only.

I found this issue in Django 1.11.1, and I've confirmed that it exists in master, all the way back through 1.8, which was the earliest version I could use with the manage.py that was created with master Django, since 1.7 was Python 2 only.

I first sent an email to the security list out of an abundance of caution. Because of the quite limited exposure this would cause, we decided to move it over to a public issue.

This was the response, with some helpful information as to the cause of the issue:

The general problem is that Django doesn't override existing class attributes with a deferred instance attribute: ​https://github.com/django/django/blob/a87189fc5e2e874219d20700cf811345138dd365/django/db/models/fields/__init__.py#L699-L703.

In the case of User.is_active, the class attribute defaulting to True was added in Django 1.6 by ​https://github.com/django/django/commit/4ea8105120121c7ef0d3dd6eb23f2bf5f55b496a in order to fix https://code.djangoproject.com/ticket/19061.

Unfortunately this looks like it isn't going to be trivial to fix.

[Aymeric Augustin]

Regarding the piece of code highlighted in the report above, I don't understand why fields shouldn't override classmethods.

If I look at this piece of code:

class A(models.Model):

@classmethod
def foo(self, bar):

raise FooError

class B(A):

foo = models.FooField()

and guess what B().foo(bar) does, I'm not guessing "FooError", I'm saying "TypeError: ... is not callable".

Simply removing the condition breaks the tests for multiple inheritance, for reasons I haven't quite figured out.

In django.db.models.base.ModelBase (the metaclass for Model) Django builds inherited models child-to-parent by tracking which fields are already defined, likely to avoid calling contribute_to_class on fields from parents that are overridden in fields from children. This code is a bit complicated and I'm having trouble figuring out how to modify it.

[Denis.Tarykin]

Pull Request:​https://github.com/django/django/pull/9126

[Ryan Hiebert]

I've tested the patch against the auth.User issue mentioned initially, and have reviewed the patch. It looks good to me, so I hope it's acceptable for me to mark it as Ready for Checkin, despite being the one who opened the issue. I anticipate that it would be appropriate to backport to supported versions of Django.

[Tim Graham]

I left some comments for improvement on the pull request. Please uncheck "Patch needs improvement" after updating.

[Carlton Gibson]

Feedback on patch:

• At the least, please break out the logic adjusting attributes on parent models and add tests for those.
• Can we not do this without actually altering the parent models? (e.g. one possibility, by noting duplicates and checking against that in Field.contribute_to_class)
• This should be helped by extracting the helper methods but, it wasn't clear what the code was doing and why. The comments should be clarified if necessary.

Please uncheck "Patch needs improvement" after updating.

[Carlton Gibson]

Whilst this was earlier, closing as a duplicate of #30427.

(Thanks all.)