Hi! I need to dynamically disable a ModelForm Field. I found some websites (not Django doc) recommending self.fields['myfield'].disabled = True. But testing here this seems to allow tampering. Is this expected behavior? Since this is very dangerous, maybe it should be stated in the docs.
class MyForm(forms.ModelForm):
# This works, but isn't dynamic.
# num = forms.IntegerField(disabled=True)
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
# This disables in HTML, but allows tampering through POST.
if should_disable_num:
self.fields['num'].disabled = True
# Something like this is also needed for dynamic behavior and avoid tampering.
def clean_num(self):
if should_disable_num:
return self.instance.num
else:
return self.cleaned_data['num']
Are you referring to the user being able to modify the html in the browser to re-enable the field? If so, there’s nothing you can do about that. The server has no control over the browser. You must never blindly trust what comes from the browser.
However, if you have properly disabled that form field (both on the GET and the POST actions), then Django will not accept the input being submitted from the browser for those disabled fields. There is no need to add the clean_num method if you’re disabling that field in both cases.
Thanks for the answer and sorry for not being more specific.
The behavior I expected is exactly what you said. But my tests showed otherwise. By “tampering” I mean a manipulated POST, using a valid CSRF token, but with the disabled field inserted manually. I tested using curl.
I’ve found the problem. The code in my first post is missing a crucial call to self.errors that is in my original code:
class MyForm(forms.ModelForm):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
# This line caches the fields...
for item in self.errors:
self.fields[item].widget.attrs.update({'autofocus': ''})
break
if should_disable_num:
# ...so this will be ignored.
self.fields['num'].disabled = True
To make it work, the self.fields['num'].disabled = True must be after the super().__init__, but before the call to self.errors.
Did I miss a warning about this somewhere? It seems a bit dangerous…
As I said in my last post, I think it’s a somewhat expected behavior, because self.errors caches the fields. But it’s easy to cause a security bug in this case.
I created a test project and app to reproduce the problem, not sure how to post all the files here, but the main ones are bellow. As you can see, and as I posted before, the view is just a generic UpdateView, and not very relevant to reproduce the problem. The crucial part is the self.errors before the self.fields['num'].disabled = True.
forms.py
from django import forms
from .models import MyModel
class MyForm(forms.ModelForm):
class Meta:
model = MyModel
fields = ['num']
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.errors
self.fields['num'].disabled = True
views.py
from django.views.generic.edit import CreateView, UpdateView
from .forms import MyForm
from .models import MyModel
class MyCreateView(CreateView):
model = MyModel
form_class = MyForm
success_url = 'create'
class MyUpdateView(UpdateView):
model = MyModel
form_class = MyForm
success_url = 'create'
models.py
from django.db import models
class MyModel(models.Model):
num = models.PositiveIntegerField(null=True, blank=True)
It is needed for my use case, which is similar to this code:
I need to call self.errors to add the autofocus, or at least that is the way I found to do it.
But if you do that, in that code order, you will open a security breach. And it’s quite dangerous because the disabled will work in the HTML (the field appears disabled in the browser), but if someone does a manipulated POST, the value will be accepted and stored in the database.
If you invert the order, and set the disabled before calling self.errors to set the autofocus, everything works fine.
The point I’m making is that it’s easy for someone to make the same mistake I did, maybe calling self.errors for other reasons unrelated to autofocus. Gladly I tested a manipulated POST, but others maybe won’t, and leave theirs systems unsecured.
That said, I’m not sure what to suggest. If there is a way to detect a disabled change and invalidate the cache done by self.errors. Or maybe display a warning when the disabled is edited after the field was cached. Or add a warning somewhere in the docs.
But I think trying to do things in this order is still a mistake. The behavior of the clean process is documented, and the results you’re seeing are a natural result of the order that you’re trying to do things.
(Side note: This is the first time I’ve ever seen someone reference the form’s error attribute - or even call clean - in the __init__ method. It seems to me that there would be a “cleaner” way to handle this - especially in a CBV, by an override in the form_invalid method. By allowing the form to be handled normally, it doesn’t matter what might be set as data values, because the entity won’t be saved.)