Re: [Django] #15103: Django 1.2.4 breaks limit_choices_to for raw_id_fields
#15103: Django 1.2.4 breaks limit_choices_to for raw_id_fields ---+ Reporter: natrius | Owner: nobody Status: new | Milestone: 1.3 Component: django.contrib.admin | Version: 1.2 Resolution:| Keywords: blocker regression send_mail email Stage: Accepted | Has_patch: 0 Needs_docs: 0 | Needs_tests: 0 Needs_better_patch: 0 | ---+ Comment (by russellm): Ok - with @luke's help, I've discovered the flaw in my logic: just because you have a list of Tours and associated leaders doesn't *necessarily* mean that the status of each of those leaders is displayed publicly on the list of Tours -- and in the example provided, it isn't. The situation we have is an admin interface that allows me to know a list of all available leaders, but won't let me know their status unless I try to associate them with a Holiday -- in which case I am given the restricted subset that meets the 'expert' criterion. Which strikes me as a really bad (or at least unusual) piece of UX, but that's doesn't change the fact that it's possible, and may have practical application in some circumstances. Objection dropped; filtering on value appears to be required, and the patch seems fine to that end. -- Ticket URL: <http://code.djangoproject.com/ticket/15103#comment:9> Django <http://code.djangoproject.com/> The Web framework for perfectionists with deadlines. -- You received this message because you are subscribed to the Google Groups "Django updates" group. To post to this group, send email to django-updates@googlegroups.com. To unsubscribe from this group, send email to django-updates+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-updates?hl=en.
Re: [Django] #15103: Django 1.2.4 breaks limit_choices_to for raw_id_fields
#15103: Django 1.2.4 breaks limit_choices_to for raw_id_fields
---+
Reporter: natrius | Owner: nobody
Status: new | Milestone: 1.3
Component: django.contrib.admin | Version: 1.2
Resolution:| Keywords: blocker regression
send_mail email
Stage: Accepted | Has_patch: 0
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
---+
Comment (by russellm):
@luke,
Fair point about being explicit, but (using the Holiday/Tour/Person models
as an example):
This whole filtering issue arises because a HolidayAdmin with
raw_id_fields=('tour',) requires the ability to filter Tours based on
leader!__status. However, the existence of the raw_id_fields clause
requires that there is a registered TourAdmin anyway... which means that
the user that you're restricting access to can already see *all* the Tour
data. Ok, they may not be able to filter based on leader status, but data
access is hardly a concern.
I haven't been able to construct an example case where an implied
limit_choices_to filter is required, where the data visibility implied by
that filter exceeds that already required in order to make the admin work.
The only way I can see that the value *might* be important (and this is
looking longer term) is with per-object permissions -- especially if we
start looking at "view" permissions. However, my gut tells me that this is
something that should be handled as at the queryset level of the
ModelAdmin, not as a 'allowed filter' check.
--
Ticket URL: <http://code.djangoproject.com/ticket/15103#comment:8>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to
django-updates+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/django-updates?hl=en.
Re: [Django] #15103: Django 1.2.4 breaks limit_choices_to for raw_id_fields
#15103: Django 1.2.4 breaks limit_choices_to for raw_id_fields ---+ Reporter: natrius | Owner: nobody Status: new | Milestone: 1.3 Component: django.contrib.admin | Version: 1.2 Resolution:| Keywords: blocker regression send_mail email Stage: Accepted | Has_patch: 0 Needs_docs: 0 | Needs_tests: 0 Needs_better_patch: 0 | ---+ Comment (by lukeplant): Russell, I'd argue that with `limit_choices_to` we need to err on the side of caution, for the following reasons: * `list_filter` etc is an '''explicit''' control enabling filtering in the admin, while `limit_choices_to` is at best implicit. * `limit_choices_to` is actually a model/database level constraint, not application level, and would be useful if the admin didn't exist, so it is not obvious that its behaviour affects the filtering available in the admin changelist at all - and in fact only needs to do so because of raw id fields, which are not the default. * `list_filter` etc are defined in the `ModelAdmin` for the model in question, whereas `limit_choices_to` is defined in (any number of) completely separate models. So, suppose I am allowing `MyModel` to be viewed in the admin changelist by some users, and supposed I then add `some_other_app.SomeModel` to my project, which doesn't even have an admin interface, but has a foreign key to `MyModel`, with `limit_choices_to` set. It really isn't obvious that this addition could change the behaviour of the admin for `MyModel` '''at all''', since that isn't necessary, and it could be argued that any change would be a bug. It also isn't likely to be the kind of hole that anyone will catch, because it's unlikely that anyone will have sufficient overview of all the models to spot the potential information leaks. I therefore think we need to be as conservative as possible in what we allow here. Also, I think the example I have above was better than the example in the test. In the example above, if the Tour changelist showed the leader's name, then allowing filtering of `Holiday` on any `leader__status` would easily allow you to find the status of any leader that was associated with at least one Tour. Now, although the interface is fairly obviously exposing all those leaders who are `Expert` and those who are not, and that is difficult to avoid, you might not want the other distinctions to be visible. In fact, consider the case where one person can access the admin to view/edit `Tour` objects, but that is all. With my proposed patch, we already have the problem that they can find out which leaders have status=2. For some applications, this might be bad enough in itself, and I don't think we should open this up more than it has to be opened. -- Ticket URL: <http://code.djangoproject.com/ticket/15103#comment:7> Django <http://code.djangoproject.com/> The Web framework for perfectionists with deadlines. -- You received this message because you are subscribed to the Google Groups "Django updates" group. To post to this group, send email to django-updates@googlegroups.com. To unsubscribe from this group, send email to django-updates+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-updates?hl=en.
Re: [Django] #15103: Django 1.2.4 breaks limit_choices_to for raw_id_fields
#15103: Django 1.2.4 breaks limit_choices_to for raw_id_fields
---+
Reporter: natrius | Owner: nobody
Status: new | Milestone: 1.3
Component: django.contrib.admin | Version: 1.2
Resolution:| Keywords: blocker regression
send_mail email
Stage: Accepted | Has_patch: 0
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
---+
Comment (by russellm):
lookup_internal was quite deliberately undocumented so that it wouldn't be
official API, giving us the flexibility to change it if required. This was
because #5833 (and at the time, #3400) is still lingering, and we didn't
want to back ourself into a corner.
It's unfortunate that people are externally documenting the "fix" for the
security problem to be "remove the security", but there's not much we can
do beyond documenting the change.
That said, I'm not completely convinced a change in signature is required.
The patch you provide certainly works, and the broad thrust seems correct
to me. However, the original security issue was about allowing completely
arbitrary join combinations -- the absence of any security checks meant
you could set up a query to retrieve password details, or anything else of
interest in the database.
If you're defining limit_choices_to = {'leader__name="palin"'} , you're
pretty much saying that it's ok to inspect the name field of the leader
relation. Ok; this would allow you to find out the name of any leader in
the system, but only by a process of elimination, and you would only find
the leader's name, and only if you already had access to the admin.
--
Ticket URL: <http://code.djangoproject.com/ticket/15103#comment:6>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to
django-updates+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/django-updates?hl=en.
Re: [Django] #15103: Django 1.2.4 breaks limit_choices_to for raw_id_fields
#15103: Django 1.2.4 breaks limit_choices_to for raw_id_fields ---+ Reporter: natrius | Owner: nobody Status: new | Milestone: 1.3 Component: django.contrib.admin | Version: 1.2 Resolution:| Keywords: blocker regression send_mail email Stage: Accepted | Has_patch: 0 Needs_docs: 0 | Needs_tests: 0 Needs_better_patch: 0 | ---+ Comment (by carljm): Replying to [comment:4 lukeplant]: > I have attached a patch which fixes the issue, for another pair of eyes to review. For the reason given above, I have implemented it so that only the exact lookup specified in the limit_choices_to is allowed. The only problem is that this involves passing the value to the `ModelAdmin.lookup_allowed` method, thus changing its signature. Due to the breakage in 1.2.4, people are already using the lookup_allowed method (e.g. http://www.hoboes.com/Mimsy/hacks/fixing-django-124s- suspiciousoperation-filtering/ ), so we need to think what to do about that. I realize people are overriding this method because 1.2.4 already broke their code once, but nonetheless: if we can't change the signature of undocumented internal methods, we're really up a creek fixing anything in a sane way. Seems reasonable to me to go ahead and make this fix with a warning in the release notes for 1.2.5. Overall, fix looks reasonable to me. -- Ticket URL: <http://code.djangoproject.com/ticket/15103#comment:5> Django <http://code.djangoproject.com/> The Web framework for perfectionists with deadlines. -- You received this message because you are subscribed to the Google Groups "Django updates" group. To post to this group, send email to django-updates@googlegroups.com. To unsubscribe from this group, send email to django-updates+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-updates?hl=en.
Re: [Django] #15103: Django 1.2.4 breaks limit_choices_to for raw_id_fields
#15103: Django 1.2.4 breaks limit_choices_to for raw_id_fields ---+ Reporter: natrius | Owner: nobody Status: new | Milestone: 1.3 Component: django.contrib.admin | Version: 1.2 Resolution:| Keywords: blocker regression send_mail email Stage: Accepted | Has_patch: 0 Needs_docs: 0 | Needs_tests: 0 Needs_better_patch: 0 | ---+ Comment (by lukeplant): I have attached a patch which fixes the issue, for another pair of eyes to review. For the reason given above, I have implemented it so that only the exact lookup specified in the limit_choices_to is allowed. The only problem is that this involves passing the value to the `ModelAdmin.lookup_allowed` method, thus changing its signature. Due to the breakage in 1.2.4, people are already using the lookup_allowed method (e.g. http://www.hoboes.com/Mimsy/hacks/fixing-django-124s- suspiciousoperation-filtering/ ), so we need to think what to do about that. Just using a keyword argument won't make it compatible with the example linked, although it would reduce comeback - we can say that you should always use `**kwargs` when overriding a method that isn't documented. Technically `lookup_allowed` it isn't documented, so we are allowed to change the signature. But we should at least put a note in the release notes, so that people don't get yet more breakage with this. Alternatively we could add another method like `lookup_allowed_value`, or move the added code to the calling method. I did not yet fix #14880, but I'm pretty sure that the newly extracted 'url_params_from_lookup_dict' function is the place to do it. I did take the opportunity to fix various unicode bugs in the existing code (it produced !UnicodeDecodeError on template render if `limit_choices_to` contained non-ASCII chars, for example). -- Ticket URL: <http://code.djangoproject.com/ticket/15103#comment:4> Django <http://code.djangoproject.com/> The Web framework for perfectionists with deadlines. -- You received this message because you are subscribed to the Google Groups "Django updates" group. To post to this group, send email to django-updates@googlegroups.com. To unsubscribe from this group, send email to django-updates+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-updates?hl=en.
Re: [Django] #15103: Django 1.2.4 breaks limit_choices_to for raw_id_fields
#15103: Django 1.2.4 breaks limit_choices_to for raw_id_fields
---+
Reporter: natrius | Owner: nobody
Status: new | Milestone: 1.3
Component: django.contrib.admin | Version: 1.2
Resolution:| Keywords: blocker regression
send_mail email
Stage: Accepted | Has_patch: 0
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
---+
Comment (by lukeplant):
For this bug, we need to think about whether the value of the lookup, as
well as the field, needs to be checked. Suppose we have:
{{{
#!python
class Person(models.Model):
status = models.ChoiceField(choices=[('2', 'Expert'), ('1',
'Competent'), ('0', 'Noob')])
class Tour(models.Model):
leader = models.ForeignKey(Person)
class Holiday(models.Model):
tour = models.ForeignKey(Tour, limit_choices_to({'leader__status':
'2'}))
}}}
Now, either with raw_id_fields or without, the user will be able to see a
list of Tours that have an 'expert' leader. But that doesn't necessarily
mean we want them to be able to see which Tours have a 'noob' leader. I
think this means we need be checking the value somewhere.
Also, I'm not convinced we are always parsing the value from the query
string properly - see #14880 - so we may need to look at this carefully.
--
Ticket URL: <http://code.djangoproject.com/ticket/15103#comment:3>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to
django-updates+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/django-updates?hl=en.
Re: [Django] #15103: Django 1.2.4 breaks limit_choices_to for raw_id_fields
#15103: Django 1.2.4 breaks limit_choices_to for raw_id_fields ---+ Reporter: natrius | Owner: nobody Status: new | Milestone: 1.3 Component: django.contrib.admin | Version: 1.2 Resolution:| Keywords: blocker regression send_mail email Stage: Accepted | Has_patch: 0 Needs_docs: 0 | Needs_tests: 0 Needs_better_patch: 0 | ---+ Changes (by lrekucki): * component: django-admin.py => django.contrib.admin Comment: I'm pretty sure you meant contrib.admin. Also, dunno how is this related to email. -- Ticket URL: <http://code.djangoproject.com/ticket/15103#comment:2> Django <http://code.djangoproject.com/> The Web framework for perfectionists with deadlines. -- You received this message because you are subscribed to the Google Groups "Django updates" group. To post to this group, send email to django-updates@googlegroups.com. To unsubscribe from this group, send email to django-updates+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-updates?hl=en.
Re: [Django] #15103: Django 1.2.4 breaks limit_choices_to for raw_id_fields
#15103: Django 1.2.4 breaks limit_choices_to for raw_id_fields --+- Reporter: natrius | Owner: nobody Status: new | Milestone: 1.3 Component: django-admin.py | Version: 1.2 Resolution: | Keywords: blocker regression send_mail email Stage: Accepted | Has_patch: 0 Needs_docs: 0| Needs_tests: 0 Needs_better_patch: 0| --+- Changes (by russellm): * needs_better_patch: => 0 * component: Uncategorized => django-admin.py * needs_tests: => 0 * milestone: => 1.3 * keywords: => blocker regression send_mail email * needs_docs: => 0 * stage: Unreviewed => Accepted -- Ticket URL: <http://code.djangoproject.com/ticket/15103#comment:1> Django <http://code.djangoproject.com/> The Web framework for perfectionists with deadlines. -- You received this message because you are subscribed to the Google Groups "Django updates" group. To post to this group, send email to django-updates@googlegroups.com. To unsubscribe from this group, send email to django-updates+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-updates?hl=en.
[Django] #15103: Django 1.2.4 breaks limit_choices_to for raw_id_fields
#15103: Django 1.2.4 breaks limit_choices_to for raw_id_fields ---+ Reporter: natrius| Owner: nobody Status: new| Milestone: Component: Uncategorized | Version: 1.2 Keywords: | Stage: Unreviewed Has_patch: 0 | ---+ The security patch in Django 1.2.4 assumes that all the fields that should be filtered on have been chosen to display as filters in the sidebar of the list view for the model in the admin. However, filters can also result from using `limit_choices_to` on a field that is displayed as a `raw_id_field`. If any fields are present in `limit_choices_to` that aren't in `list_filters`, the admin will 500 on a `SuspiciousOperation` exception any time a user tries to open the window to select an item. -- Ticket URL: <http://code.djangoproject.com/ticket/15103> Django <http://code.djangoproject.com/> The Web framework for perfectionists with deadlines. -- You received this message because you are subscribed to the Google Groups "Django updates" group. To post to this group, send email to django-updates@googlegroups.com. To unsubscribe from this group, send email to django-updates+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-updates?hl=en.
