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

[Andrey Gusev]

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

[Julien Phalip]

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

[r1chardj0n3s]

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

[Julien Phalip]

Absolutely. Some tests would also be needed. Thanks!

[r1chardj0n3s]

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

[Julien Phalip]

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.

[Timothy Schilling]

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

[Paul Garner]

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.

[Timothy Schilling]

@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?

[Paul Garner]

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

[Paul Garner]

@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

[Timothy Schilling]

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)

[Anssi Kääriäinen]

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.

[Paul Garner]

@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

[Paul Garner]

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

[Anssi Kääriäinen]

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

[Timothy Schilling]

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

[Paul Garner]

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."

[Paul Garner]

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.

[Tim Graham <timograham@…>]

In c4db7f075e269411287ff14b5518412250ba455f:

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

Thanks gauss for the report.

[Chris Wilson]

Update the summary to help people to find this ticket.

[Chris Wilson]

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.

[Tim Graham]

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).