Opened 3 years ago

Closed 21 months ago

Last modified 17 months ago

#19182 closed Bug (fixed)

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

Reported by: gauss Owned by: nobody
Component: contrib.admin Version: 1.4
Severity: Normal Keywords: modeladmin lookup_allowed
Cc: r1chardj0n3s, schillingt@…, anentropic, 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 Changed 3 years ago by julien

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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

comment:2 Changed 2 years ago by r1chardj0n3s

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

comment:3 Changed 2 years ago by r1chardj0n3s

  • Cc r1chardj0n3s added

comment:4 Changed 2 years ago by julien

Absolutely. Some tests would also be needed. Thanks!

comment:5 Changed 2 years ago by r1chardj0n3s

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

comment:6 Changed 2 years ago by julien

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 Changed 22 months ago by CodenameTim

  • 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 Changed 22 months ago by anentropic

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 Changed 22 months ago by CodenameTim

@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 Changed 22 months ago by anentropic

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 Changed 22 months ago by anentropic

@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 Changed 22 months ago by CodenameTim

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 Changed 22 months ago by akaariai

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 Changed 22 months ago by anentropic

@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 Changed 22 months ago by anentropic

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 Changed 22 months ago by akaariai

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

comment:17 Changed 22 months ago by CodenameTim

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

comment:18 Changed 22 months ago by anentropic

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 Changed 22 months ago by anentropic

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

Last edited 22 months ago by anentropic (previous) (diff)

comment:20 Changed 21 months ago by anentropic

  • Cc anentropic added
  • Has patch set

comment:21 Changed 21 months ago by Tim Graham <timograham@…>

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

In c4db7f075e269411287ff14b5518412250ba455f:

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

Thanks gauss for the report.

comment:22 Changed 17 months ago by gcc

  • Summary changed from ModelAdmin.lookup_allowed should also check for ('fieldname', ClassName) syntax to ModelAdmin filtering when list_filter = (('fieldname', ClassName),) throws SuspiciousOperation exception

Update the summary to help people to find this ticket.

comment:23 Changed 17 months ago by gcc

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 Changed 17 months ago by timo

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 Changed 17 months ago by gcc

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