ModelChoiceField and admin readonly_fields #235

Closed
spookylukey opened this Issue Jan 16, 2014 · 18 comments

Projects

None yet

4 participants

@spookylukey
Contributor

Using "autocomplete_light.ModelChoiceField" in a form, and using that form in the admin with readonly fields doesn't seem to work.

In particular, it returns a validation error for the readonly field - "this field is required". This doesn't happen otherwise. The error message doesn't actually show, because the field is displayed in readonly mode, you just get "Please correct the errors below".

There are no docs for running tests, at least that I could find, so I'm afraid I ran out of energy in trying to create a test case for this.

Tested in 2.0.0a9

If the field is readonly, can you confirm that there is a pre-existing value for it when you load the record?
django validates all values on load. If no value is found for the required field then it raises that error.

Contributor

Yep, it loads and displays fine, it's just when I try to save it that it complains i.e. a POST.

I am also use 'get_readonly_fields' to dynamically change which fields are readonly, in case that affect things.

Owner
jpic commented Jan 16, 2014

Well you could run the tests from test_project: ./manage.py test autocomplete_light, also there is test.sh which bootstraps everything in a fresh (re-usable) environment for any specific django or python version.

Thanks a lot for your feedback, I'm going to check what has to be done to support readonly display.

@jpic jpic added a commit that referenced this issue Jan 18, 2014
@jpic jpic Created apps and data for #235 fd7497a
Owner
jpic commented Jan 18, 2014

Ok, I think there is a security issue in your implementation. Django should not parse POST values for readonly fields !

I created apps and data that attempt to reproduce the case you are talking about. Running the test_project from branch readonly_fields you can open http://localhost:8000/admin/readonly_autocomplete_field/

See the source here: https://github.com/yourlabs/django-autocomplete-light/tree/readonly_fields/autocomplete_light/example_apps/readonly_autocomplete_field

And I must say, I cannot reproduce your issue with normal readonly_fields (which means that Django's readonly_field implementation is secure). All I can get to crash is trying to create an FkModel or OtoModel because the relation is not null and I think that's completely normal and not autocomplete-light specific.

Could you please paste your get_readonly_fields implementation so that I could try to reproduce it and maybe fix the security issue we've found in your implementation, then see if there's any remaining autocomplete-light issue ?

Owner
jpic commented Jan 21, 2014

Any update on this ? It looks critical for your project.

Contributor

Thanks for your interest - I'm going to work on it this afternoon and try to work out if it is a bug in my code, Django's admin code, or autocomplete, and produce a test case if it is the latter. Thanks.

Owner
jpic commented Jan 21, 2014

I'm positive that it's a bug in your code: django admin should not parse
POST values for readonly fields.​

Contributor

OK, made some progress.

Your tests don't replicate my problem - they just use autocomplete_light.modelform_factory. I'm not doing that, I'm using autocomplete_light.ModelChoiceField:

class FooForm(ModelForm):
    my_field = autocomplete_light.ModelChoiceField('Foo')

    class Meta:
        model = Foo

    # other custom stuff here

class FooAdmin(admin.ModelAdmin):
    form = FooForm

The problem is that readonly fields in the admin are implemented by passing the readonly fields to modelform_factory as 'exclude' arguments, so the generated form does not include those fields, and everything works great normally.

However, if you create a ModelForm subclass with a "autocomplete_light.ModelChoiceField" field specifically added, that 'overrides' the fields that were missed out, and you have a problem.

So this is not a bug in my code, nor in Django, but an incompatibility between admin readonly fields and manually adding a field to a ModelForm.

Possible fixes are:

  1. Instead of adding the "autocomplete_light.ModelChoiceField", use "autocomplete_light.ModelForm" as a base class. When I do this, I get the following traceback (v 2.0.0a12):
File "/home/luke/.virtualenvs/wolfandbadger/local/lib/python2.7/site-packages/django/core/handlers/base.py" in get_response
  115.                         response = callback(request, *callback_args, **callback_kwargs)
File "/home/luke/.virtualenvs/wolfandbadger/local/lib/python2.7/site-packages/django/contrib/admin/options.py" in wrapper
  372.                 return self.admin_site.admin_view(view)(*args, **kwargs)
File "/home/luke/.virtualenvs/wolfandbadger/local/lib/python2.7/site-packages/django/utils/decorators.py" in _wrapped_view
  91.                     response = view_func(request, *args, **kwargs)
File "/home/luke/.virtualenvs/wolfandbadger/local/lib/python2.7/site-packages/django/views/decorators/cache.py" in _wrapped_view_func
  89.         response = view_func(request, *args, **kwargs)
File "/home/luke/.virtualenvs/wolfandbadger/local/lib/python2.7/site-packages/django/contrib/admin/sites.py" in inner
  202.             return view(request, *args, **kwargs)
File "/home/luke/.virtualenvs/wolfandbadger/local/lib/python2.7/site-packages/django/utils/decorators.py" in _wrapper
  25.             return bound_func(*args, **kwargs)
File "/home/luke/.virtualenvs/wolfandbadger/local/lib/python2.7/site-packages/django/utils/decorators.py" in _wrapped_view
  91.                     response = view_func(request, *args, **kwargs)
File "/home/luke/.virtualenvs/wolfandbadger/local/lib/python2.7/site-packages/django/utils/decorators.py" in bound_func
  21.                 return func(self, *args2, **kwargs2)
File "/home/luke/.virtualenvs/wolfandbadger/local/lib/python2.7/site-packages/django/db/transaction.py" in inner
  223.                 return func(*args, **kwargs)
File "/home/luke/.virtualenvs/wolfandbadger/local/lib/python2.7/site-packages/django/contrib/admin/options.py" in add_view
  984.         ModelForm = self.get_form(request)
File "/home/luke/.virtualenvs/wolfandbadger/local/lib/python2.7/site-packages/django/contrib/admin/options.py" in get_form
  465.         return modelform_factory(self.model, **defaults)
File "/home/luke/.virtualenvs/wolfandbadger/local/lib/python2.7/site-packages/django/forms/models.py" in modelform_factory
  424.     return type(form)(class_name, (form,), form_class_attrs)
File "/home/luke/.virtualenvs/wolfandbadger/local/lib/python2.7/site-packages/autocomplete_light/forms.py" in __new__
  266.                 attrs)
File "/home/luke/.virtualenvs/wolfandbadger/local/lib/python2.7/site-packages/django/forms/models.py" in __new__
  212.                                       opts.exclude, opts.widgets, formfield_callback)
File "/home/luke/.virtualenvs/wolfandbadger/local/lib/python2.7/site-packages/django/forms/models.py" in fields_for_model
  170.             formfield = formfield_callback(f, **kwargs)
File "/home/luke/.virtualenvs/wolfandbadger/local/lib/python2.7/site-packages/autocomplete_light/forms.py" in __call__
  235.         return self.default(model_field, **kwargs)
File "/home/luke/.virtualenvs/wolfandbadger/local/lib/python2.7/site-packages/django/contrib/admin/options.py" in formfield_for_dbfield
  113.                 formfield = self.formfield_for_foreignkey(db_field, request, **kwargs)
File "/home/luke/.virtualenvs/wolfandbadger/local/lib/python2.7/site-packages/django/contrib/admin/options.py" in formfield_for_foreignkey
  174.         return db_field.formfield(**kwargs)
File "/home/luke/.virtualenvs/wolfandbadger/local/lib/python2.7/site-packages/django/db/models/fields/related.py" in formfield
  1092.         return super(ForeignKey, self).formfield(**defaults)
File "/home/luke/.virtualenvs/wolfandbadger/local/lib/python2.7/site-packages/django/db/models/fields/__init__.py" in formfield
  499.         return form_class(**defaults)
File "/home/luke/.virtualenvs/wolfandbadger/local/lib/python2.7/site-packages/autocomplete_light/fields.py" in __init__
  34.             kwargs['queryset'] = self.autocomplete.choices

Exception Type: AttributeError at /admin/product/productupdate/add/
Exception Value: 'NoneType' object has no attribute 'choices'

I don't know what is going on here, and I haven't been able to dig further due to the difficulty of running tests (filed in other bugs, including yourlabs/django-cities-light#40 )

However, this isn't an ideal solution anyway, because I don't want all the other behaviour from ModelForm

  1. Use formfield_callback

Doesn't work because Django's ModelAdmin.get_form passes it's own formfield_callback

Contributor

OK, I found out why my solution 1) (using ModelForm) didn't work:

I had put the item in 'raw_id_fields' (which is often a precursor to a more advanced solution like autocomplete), and that caused problems. Specifically:

Here:
https://github.com/yourlabs/django-autocomplete-light/blob/v2/autocomplete_light/fields.py#L41

'widget' was django.contrib.admin.widgets.ForeignKeyRawIdWidget, resulting in problems later.

Perhaps this is something to document, or even provide a better solution like an error if it can be detected?

Owner
jpic commented Jan 27, 2014

On Mon, Jan 27, 2014 at 7:31 PM, Luke Plant notifications@github.comwrote:

  1. Use formfield_callback

Doesn't work because Django's ModelAdmin.get_form passes it's own
formfield_callback

Completely supported:
https://github.com/yourlabs/django-autocomplete-light/blob/v2/autocomplete_light/tests/forms.py#L113

http://yourlabs.org http://blog.yourlabs.org
Customer is king - Le client est roi - El cliente es rey.

Owner
jpic commented Jan 27, 2014

It seems like I fail to understand your use case.

Please read the FAQ: How to get
help

Thanks !

Owner
jpic commented Jan 28, 2014

Could you update autocomplete_light/example_apps/readonly_autocomplete_field from readonly_fields branch to reproduce your issue ?

Thanks !

Contributor

There is no issue now.

My problems were caused by a few things:

  1. Use of explicit fields in forms is not compatible with readonly fields in the admin:
class FooForm(ModelForm):
    my_field = autocomplete_light.ModelChoiceField('Bar')

    class Meta:
        model = Foo

    # other custom stuff here

class FooAdmin(admin.ModelAdmin):
    form = FooForm

    def get_readonly_fields(request, obj=None):
        if obj.id is not None:
            return ["my_field"]

This does not work, because readonly fields work by adding a field to the 'exclude' argument when calling modelform_factory, but the explicit addition of my_field in FooForm means that you always get 'my_field' in the form.

This is not an issue specific to django-autocomplete-light. There is no reason to update the test suite, because this cannot work - it is a limitation of Django's readonly fields in admin feature.

So, I tried to use autocomplete_light.ModelForm, and had some other issues:

  1. formfield_callback does not work in the context of the admin, and providing your own ModelForm with a formfield_callback attached because it is ignored by the admin, which provides its own. I traced this though the Django source code:

https://github.com/django/django/blob/master/django/contrib/admin/options.py#L616

There is nothing you can do about this, AFAICS.

  1. If the field to add the autocomplete to is also in raw_id_fields (my error), you get a very difficult to debug error.

Or rather, I did, and still do in my case, but I haven't been able to distil it down into a test case yet

Owner
jpic commented Jan 28, 2014
  1. As suprising as it seems, you have found a bug in Django. I could reproduce it:
class TestFkModelForm(forms.ModelForm):
    relation = autocomplete_light.ModelChoiceField('RelatedModelAutocomplete')

    class Meta:
        model = TestFk

This will cause relation to appear twice in the admin:

2014-01-28-154941_423x233_scrot

And will cause the bug you were talking about when trying to save. About the potential security issue, I've tried that as well, apparently Django will parse but won't save the value a malicious user could have set on input[name=relation] (which could be added in the page with firebug or crafted directly in the POST request). So it looks safe.

  1. Look at django's code you pointed, it uses the defaults which specifies a formfield_callback as such: defaults.update(kwargs). Doesn't that mean that kwargs['formfield_callback'] will override defaults['formfield_callback'] ? I have not tested setting ModelAdmin.formfield_callback yet.

  2. I have to try that too

Owner
jpic commented Jan 28, 2014

I don't know if that's exactly what you want to do, but this example demonstrates how to:

  • not use autocomplete_light.ModelForm,
  • get an autocomplete in the admin on create form,
  • get readonly field on edit form,
  • use default's admin formfield callback ie. for date/time widgets as well as add-another functionnality.

Example:

class ReadonlyFieldsModelAdmin(admin.ModelAdmin):
    def get_readonly_fields(self, request, obj=None):
        return ['relation'] if obj is not None and obj.pk else []

    def get_form(self, request, obj=None, **kwargs):
        kwargs['formfield_callback'] = autocomplete_light.FormfieldCallback(
            partial(self.formfield_for_dbfield, request=request))
        return super(ReadonlyFieldsModelAdmin, self).get_form(request, obj, **kwargs)
@jpic jpic closed this Apr 7, 2015
Hedde commented May 27, 2015

The example link is down, I have this exact issue using the following implementation (django 1.8);

class PurchaseForm(forms.ModelForm):
    grower = autocomplete_light.ModelChoiceField('GrowerAutocomplete', label=_(u"Grower"))

    class Meta:
        exclude = tuple()
        model = Purchase


class PurchaseAdmin(admin.ModelAdmin):
    readonly_fields = ('status', 'date_created', 'date_pushed',)

    form = PurchaseForm

    def get_readonly_fields(self, request, obj=None):
        if obj:
            return ('grower',) + super(PurchaseAdmin, self).get_readonly_fields(request, obj=obj)
        return super(PurchaseAdmin, self).get_readonly_fields(request, obj=obj)
Owner
jpic commented May 27, 2015

The link is still up: http://django-autocomplete-light.readthedocs.org/en/v2/faq.html#how-to-ask-for-help

How to report a bug effectively ?
Read How to Report Bugs Effectively and open an issue on django-autocomplete-light’s issue tracker on GitHub.

How to ask for help ?
The best way to ask for help is:

fork the repo,
add a simple way to reproduce your problem in a new app of test_project, try to keep it minimal,
open an issue on github and mention your fork.
Really, it takes quite some time for me to clean pasted code and put up an example app it would be really cool if you could help me with that !

If you don’t want to do the fork and the reproduce case, then you should better ask on StackOverflow and you might be lucky (just tag your question
with django-autocomplete-light to ensure that I find it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment