Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#19182 closed Bug (fixed)

ModelAdmin filtering when list_filter = (('fieldname', ClassName),) throws SuspiciousOperation exception

Reported by: Andrey Gusev Owned by: nobody
Component: contrib.admin Version: 1.4
Severity: Normal Keywords: modeladmin lookup_allowed
Cc: r1chardj0n3s, schillingt@…, Paul Garner, chris+django@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Admin raises SuspiciousOperation on using complex query like ghi__jkl__contains in custom list filter described by ('fieldpath', ClassName ) syntax.

class MyModelAdmin(ModelAdmin):
    ...
    list_filter = (
        'abc', 'def',
        ( 'ghi__jkl__contains', MyListFilter )
    )
    ...

Simple but ugly fix by replacing last line of ModelAdmin.lookup_allowed (around line 270 in contrib/admin/options.py):

        clean_lookup = LOOKUP_SEP.join(parts)
        --- return clean_lookup in self.list_filter or clean_lookup == self.date_hierarchy
        +++ return any([ ( clean_lookup == ( i[0] if isinstance(i, tuple) else i ) ) for i in self.list_filter ]) or clean_lookup == self.date_hierarchy

Change History (25)

comment:1 by Julien Phalip, 12 years ago

Triage Stage: UnreviewedAccepted

Accepted, although a clearer implementation than the one proposed in the ticket description would be preferable.

comment:2 by r1chardj0n3s, 11 years ago

Would a Pull Request help move acceptance of this change along?

comment:3 by r1chardj0n3s, 11 years ago

Cc: r1chardj0n3s added

comment:4 by Julien Phalip, 11 years ago

Absolutely. Some tests would also be needed. Thanks!

comment:5 by r1chardj0n3s, 11 years ago

Hm, there's no tests at all for lookup_allowed that I can see, unless I'm missing something.

comment:6 by Julien Phalip, 11 years ago

Most tests for list_filter should normally be in https://github.com/django/django/blob/master/tests/admin_filters/tests.py

After a quick look it seems there are also a few in admin_views, for example: https://github.com/django/django/blob/1b47508ac88c1b1ee6ae71f032abb268395ac00e/tests/admin_views/tests.py#L411-L480

It would be very helpful to write a failing test reproducing the SuspiciousOperation error.

comment:7 by Timothy Schilling, 11 years ago

Cc: schillingt@… added

I'm not sure this is a bug. The list_filter accepts tuples in it as long as the filter class passed in inherits from django.contrib.admin.FieldListFilter

Documentation: https://docs.djangoproject.com/en/dev/ref/contrib/admin/#django.contrib.admin.ModelAdmin.list_filter

comment:8 by Paul Garner, 11 years ago

I just hit this, I'm on 1.5

@CodenameTim I think you misunderstood, the bug is in admin.ModelAdmin ...it doesn't handle tuples in the list_filter attr. Everything works until you try to actually filter on that field

The code seems to only handle strings - it doesn't look like it would be able to handle the other form shown in the docs list_filter = (DecadeBornListFilter,) either but I haven't tried it.

comment:9 by Timothy Schilling, 11 years ago

@anentropic I must be misunderstanding the problem. I just pulled stable/1.5.x and implemented what is in the documentation and it worked fine. Can you post what you tried to use and what you expected it to do?

comment:10 by Paul Garner, 11 years ago

Yes I'm a bit puzzled, there's a case in the tests that uses the tuple form and appears to work fine. I haven't yet found the part of Django that makes it work - any pointers? (because reading the source of lookup_allowed method it seems obvious why it wouldn't work...)

Am trying to narrow down what part of my code my be causing it, to make a reproduceable case

comment:11 by Paul Garner, 11 years ago

@CodenameTim it seems to be related to use of AllValuesFieldListFilter

Here is a simple test case which fails:
https://github.com/anentropic/django/commit/5d32f4e09e68a101ccd4202438504296ac68a02b

DisallowedModelAdminLookup: Filtering by author__email not allowed

comment:12 by Timothy Schilling, 11 years ago

Cool, thanks for that. I was able to reproduce it now. Not sure if it matters, but a workaround for that case would be to just pass 'author__email' rather than ('author__email', AllValuesFieldListFilter)

comment:13 by Anssi Kääriäinen, 11 years ago

I think the whole idea that admin code should know what a lookup targets is wrong. Instead methods for checking if lookup exists at all, and what models the lookup traverses should be added to QuerySet's public API, then admin should use those methods.

As is, the code is very fragile. In fact, to me it seems that if you are using GIS it will be possible to query non-allowed models by gis lookups. Why? Use a lookup that isn't in default QUERY_TERMS, but is in GeoQuery's lookups. The lookup part will not be popped in L335 in admin/options.py. Later on we get to FieldDoesNotExist on L346 for the GIS lookup. But the comment's logic doesn't apply here - the lookup isn't actually going to produce field error later on. End result is lookup allowed, even if it targets non-allowed related field. (I haven't tested this.)

I don't object to improving lookup_allowed for this specific cases - but trying to repeat the logic of how QuerySet treats lookups is non-DRY for a complex piece of code. This means that lookup_allowed method *will* fail when things are changed in QuerySet.

comment:14 by Paul Garner, 11 years ago

@akaariai My problem is the opposite one: lookups being disallowed when they shouldn't be

@CodenameTime in my real code I have a more complex filter class that inherits from AllValuesFieldListFilter which I need to cause to be used

This is what I've done to get things working for me:
https://github.com/anentropic/django/commit/1cc56634b729fe498c4ecf237c3c322c2fead919

comment:15 by Paul Garner, 11 years ago

Actually it has nothing to do with AllValuesFieldListFilter, test also fails if you substitute BooleanFieldListFilter

The problem is with relation-spanning filters such as 'author__email', in conjunction with the tuple form

I'm not sure whether my lookup is even supposed to fall through this check:

        if len(parts) == 1:
            return True

(which is why the existing test case containing ('is_best_seller', BooleanFieldListFilter) filter does not fail)

but I think it is supposed to, as there is a comment that the code above is just a special case for trimming foo__id

comment:16 by Anssi Kääriäinen, 11 years ago

OK, lets deal with the more generic lookup_allowed() refactoring problem separately.

comment:17 by Timothy Schilling, 11 years ago

I'm sorry, I'm a little bit lost. What issue are we trying to solve with this ticket?

comment:18 by Paul Garner, 11 years ago

The problem is as described by the OP:

"Admin raises SuspiciousOperation on using complex query like ghi__jkl__contains in custom list filter described by ('fieldpath', ClassName ) syntax."

comment:19 by Paul Garner, 11 years ago

After understanding the code a bit better last night I think my fix above is basically correct.

I have added another test case, for (SimpleListFilter,) class-only syntax... if the parameter_name string included double-underscores it was also failing. Have also tidied up the naming of previous test case and put some comments in.
https://github.com/anentropic/django/commit/91a6cd4895b269b01b872799001587ee34a25855

Is this enough to make a pull request? Do you want the new tests and the fix separately, at the moment they're in the same branch on my fork.

Version 0, edited 11 years ago by Paul Garner (next)

comment:20 by Paul Garner, 11 years ago

Cc: Paul Garner added
Has patch: set

comment:21 by Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: newclosed

In c4db7f075e269411287ff14b5518412250ba455f:

Fixed #19182 -- Fixed ModelAdmin.lookup_allowed to work with ('fieldname', SimpleListFilter) syntax.

Thanks gauss for the report.

comment:22 by Chris Wilson, 11 years ago

Summary: ModelAdmin.lookup_allowed should also check for ('fieldname', ClassName) syntaxModelAdmin filtering when list_filter = (('fieldname', ClassName),) throws SuspiciousOperation exception

Update the summary to help people to find this ticket.

comment:23 by Chris Wilson, 11 years ago

I'd like to request that this fix be backported to 1.6.x, since the use of ('fieldname', SimpleListFilter) is documented to work for 1.6.x but doesn't.

comment:24 by Tim Graham, 11 years ago

This doesn't qualify for a backport to 1.6.x since it's not a new regression (the bug has been present since 1.4 according to the ticket).

comment:25 by Chris Wilson, 11 years ago

Cc: chris+django@… added
Note: See TracTickets for help on using tickets.
Back to Top