Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#15032 closed (fixed)

Changeset 15031 breaks filtering to objects which are subclassed.

Reported by: rene Owned by: nobody
Component: contrib.admin Version: 1.2
Severity: Keywords: filtering subclassed object not allowed regression blocker
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

In changeset 15031 a security check is implemented which checks if the parameters in the URL query are indeed field names specified in the 'list_filter' attribute of the AdminModel.

This breaks the filtering of a inherintanced model object. See attached two files for a sample code.

I have a 'Employee' class in models.py which is a subclass of 'django.contrib.auth.models.User'.
I have a WorkHour class in models.py which has a foreign key to 'Employee'.
In admin.py I have WorkHourAdmin which defines a list_filter attribute which includes the field 'employee'. This field is the foreign key to Employee.

The employee filter on 'WorkHour' admin object will generate a lookup key like this: 'employee user_ptr exact'

In changeset 15031 this does not the pass the check in 'django/contrib/admin/options.py' line 243

The field 'employee user ptr' is not defined in the 'list_filter' attribute on class WorkHourAdmin. But according to me this is a valid filtering.

Attachments (9)

models.py (441 bytes) - added by rene 5 years ago.
Sample models.py file
admin.py (148 bytes) - added by rene 5 years ago.
Sample admin.py file
15032-tests.diff (3.0 KB) - added by ramiro 5 years ago.
Patch for the Django test suite
dtest18.tar.gz (3.0 KB) - added by ramiro 5 years ago.
Standalone project
Dtest.zip (13.9 KB) - added by rene 5 years ago.
Stand alone project with test-suite.
transcript.txt (3.4 KB) - added by rene 5 years ago.
Transscript of run of unit-test
explanation.txt (1.6 KB) - added by rene 5 years ago.
Explanation of source of error
15032-1.2.X.diff (3.7 KB) - added by ramiro 5 years ago.
Fix plus tests for 1.2.X branch
15032-tests-trunk.diff (2.6 KB) - added by ramiro 5 years ago.
Addition to regression tests for trunk

Download all attachments as: .zip

Change History (23)

Changed 5 years ago by rene

Sample models.py file

Changed 5 years ago by rene

Sample admin.py file

comment:1 Changed 5 years ago by rene

  • Component changed from Uncategorized to django.contrib.admin
  • Keywords filtering subclassed object not allowed added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 5 years ago by rene

  • Summary changed from Changeset 15031 breaks filtering to objects which are are subclassed. to Changeset 15031 breaks filtering to objects which are subclassed.

comment:3 Changed 5 years ago by russellm

  • Keywords regression blocker added
  • milestone set to 1.3
  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 5 years ago by ramiro

  • Resolution set to worksforme
  • Status changed from new to closed

I have added a simplified test case that instead of having Employee inheriting from User makes it inherit from a Person model and it shows the filtering of WorkHour instances based on Employee is working (it also works if I undo the change introduced in r15139.)

Unfortunately you don't tell us what is the error you see in the admin.

Find attached the modification for the Django test suite as a patch, I've also implemented this as a standalone test app so we can test this interactively, it is in the attached tarball.

I'm going to close this ticket, please reopen if you can give us more details about what's the error condition you see.

Changed 5 years ago by ramiro

Patch for the Django test suite

Changed 5 years ago by ramiro

Standalone project

Changed 5 years ago by rene

Stand alone project with test-suite.

comment:5 follow-up: Changed 5 years ago by rene

Hello,

Your testsuite contains an error according to me. On line 392 you should reassign the variable 'response'. On line 393 you are still testing the 'response' assigined on line 389 which returned a '200' status code.

Attached you will a modified version of the standalone project you provided. I added a unit test, based upon your unit-test. A transscript of this unit-test is also added.

Changed 5 years ago by rene

Transscript of run of unit-test

comment:6 Changed 5 years ago by rene

Please run the attached standalone project with Django 1.2.4. The included test-suite is not compatible with the current development version of Django.

--

For the source and explanation of the error, please see attached file 'explanation.txt'.

comment:7 Changed 5 years ago by rene

  • Resolution worksforme deleted
  • Status changed from closed to reopened

Changed 5 years ago by rene

Explanation of source of error

comment:8 in reply to: ↑ 5 Changed 5 years ago by ramiro

  • milestone 1.3 deleted

Replying to rene:

Hello,

Your testsuite contains an error according to me. On line 392 you should reassign the variable 'response'. On line 393 you are still testing the 'response' assigined on line 389 which returned a '200' status code.

You are completely right, thanks for noting this. I can reproduce what you report using 1.2.4 and with a checkout of the 1.2.X branch as of now.

And there lies the critical difference. I assumed this report was against the SVN development trunk (possibly because it references commit r15031 (trunk) instead of r15033 (1.2.X branch) and because the milestone for the ticket had been set to 1.3), but from a latter comment you posted I can see now you are reporting this against the 1.2.4 release.

My reports about not seeing this problem both with the automated test suite (even if I correct that response = assignment) and when playing interactively with the admin app with the attached project were also correct. Because I'm testing against the SVN trunk.

So, in conclusion, this an error that isn't present in trunk but it is in the 1.2.X bug-fix branch. Updating ticket fields accordingly.

comment:9 Changed 5 years ago by rene

Oke, sorry my mistake for wrong ticket headers. I am glad you could reproduce the error.

Do you have any idea about a solution? For now I just overwrite the method in my AdminClass and do some additional testing for this special case. But it is not a generic solution.

comment:10 Changed 5 years ago by russellm

  • milestone set to 1.3

Ramiro - this still needs to be on the 1.3 milestone because it needs to be addressed before we can release 1.3. If it isn't addressed, 1.2 will go into security-fix mode with this issue lingering.

Changed 5 years ago by ramiro

Fix plus tests for 1.2.X branch

Changed 5 years ago by ramiro

Addition to regression tests for trunk

comment:11 Changed 5 years ago by ramiro

  • Has patch set

comment:12 Changed 5 years ago by ramiro

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [15177]) [1.2.X] Fixed #15032 -- Replaced 1.2.X implementation of admin changelist filtering security fix (r15031/r15033) with the one from trunk so another valid filter usage scenario (using model inheritance) is still possible. Thanks rene for reporting this.

comment:13 Changed 5 years ago by ramiro

In [15555]:

[1.1.X] Fixed #15306 -- Replaced 1.1.X implementation of admin changelist filtering security fix (r15031/r15033) with the one from trunk so another valid filter usage scenario (using model inheritance) is still possible. Thanks dbenamy for reporting this. Refs #15032.

comment:14 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

Note: See TracTickets for help on using tickets.
Back to Top