Opened 13 years ago

Closed 13 years ago

Last modified 12 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: no UI/UX: no

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

Download all attachments as: .zip

Change History (23)

by rene, 13 years ago

Attachment: models.py added

Sample models.py file

by rene, 13 years ago

Attachment: admin.py added

Sample admin.py file

comment:1 by rene, 13 years ago

Component: Uncategorizeddjango.contrib.admin
Keywords: filtering subclassed object not allowed added

comment:2 by rene, 13 years ago

Summary: Changeset 15031 breaks filtering to objects which are are subclassed.Changeset 15031 breaks filtering to objects which are subclassed.

comment:3 by Russell Keith-Magee, 13 years ago

Keywords: regression blocker added
milestone: 1.3
Triage Stage: UnreviewedAccepted

comment:4 by Ramiro Morales, 13 years ago

Resolution: worksforme
Status: newclosed

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.

by Ramiro Morales, 13 years ago

Attachment: 15032-tests.diff added

Patch for the Django test suite

by Ramiro Morales, 13 years ago

Attachment: dtest18.tar.gz added

Standalone project

by rene, 13 years ago

Attachment: Dtest.zip added

Stand alone project with test-suite.

comment:5 by rene, 13 years ago

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.

by rene, 13 years ago

Attachment: transcript.txt added

Transscript of run of unit-test

comment:6 by rene, 13 years ago

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 by rene, 13 years ago

Resolution: worksforme
Status: closedreopened

by rene, 13 years ago

Attachment: explanation.txt added

Explanation of source of error

in reply to:  5 comment:8 by Ramiro Morales, 13 years ago

milestone: 1.3

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 by rene, 13 years ago

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 by Russell Keith-Magee, 13 years ago

milestone: 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.

by Ramiro Morales, 13 years ago

Attachment: 15032-1.2.X.diff added

Fix plus tests for 1.2.X branch

by Ramiro Morales, 13 years ago

Attachment: 15032-tests-trunk.diff added

Addition to regression tests for trunk

comment:11 by Ramiro Morales, 13 years ago

Has patch: set

comment:12 by Ramiro Morales, 13 years ago

Resolution: fixed
Status: reopenedclosed

(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 by Ramiro Morales, 13 years ago

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 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

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