Validation during widget rendering. Why?

[iperelivskiy]

I just wanted to optimize db queries for my form that contains ~50 fields with generic autocomplete widgets invoking ~200 queries on render. Looking at source I've noticed that validation occurs right in render method. Why does it? Shouldn't it take place in form validation flow and invoked as needed on POST request? May be I am missing something but it seems to me like a design flaw, moreover it prevents to optimize db queries on render. Any thoughts how to workaround this? Thanks!

[jpic]

I do remember taking this decision for security reason. A user could craft values and get to see representation of objects he would not be allowed to see.

No time right now but I'll take a closer look at it (maybe you could too ?)

Thanks for your feedback

[iperelivskiy]

Ok, I'll take a closer look at the source soon. Thanks!

[jpic]

I found more about the security issue becaus on which that this design decision covers while answering a couple of questions about django-autocomplete-light on SO.

But I understand your point so feedback would be so very welcome on this.

I think we should ship a ModelChoiceField subclass for security because it would match Django's design better, also allow a bit of refactoring in user projects.

[iperelivskiy]

Hi,

Could you provide more details on security considerations regarding this issue please?

I'm not a security expert, but I convinced that when you're GETing data you're just reading it, hence no data is about to be changed, so I can't see any possible vulnerability here. If you are talking about permission system like "if a user can read this chunk of data" I think it's another layer of abstraction and should be implemented via api hooks depending on user needs.

English is not my mother language, so if you're having problems reading my message, please let me know.

[jpic]

Well a user could craft a pk in the hidden select of the HTML form and then he could be able to see the text representation of that object if we weren't validating before rendering.

[jpic]

But still I'm not sure about how to fix that, because in Django theory we never pass the request or user to the form: they are isolated - in reality we do it often I know ...

[iperelivskiy]

Well a user could craft a pk in the hidden select of the HTML form and then he could be able to see the text representation of that object if we weren't validating before rendering.

I still don't understand how a user could craft a pk without POST request. How is it possible?

[jpic]

We provide the opportunity to override choices_for_request(). Here, you might want to secure choices that are displayed, based on request.user. But because validation is done without request nor request.user, there can't be any relevant validation from within neither the form, field or widget. So, potentially a user could craft a ctype-pk and the form will validate, then the user can see objects which he's not supposed to see.

Do I make any sense ?

Now, I don't like this either, any suggestion is appreciated :)

I want to ship a Form and ModelForm base in the next major version (which will probably be a different project to be able to maintain BC of autocomplete-light in a sane way) which will help for some things. But still we suffer from Django's design decision, that no request nor request.user can be used in validation logic.

[spookylukey]

I agree with the security concerns, but the current behaviour of raising an exception has other problems.

Suppose in an admin page you have an autocomplete field that does filtering on a Django model to limit the items that can be selected. A user selects a valid item, and saves the form.

Then a database change occurs so that the item is no longer part of the filtered queryset. If the user comes back to that admin page, they will get an error due to the item no longer being valid. The error is a crasher, stopping them from even viewing the page. (I'm getting this currently on a site).

It's tricky to fix. The options are:

1. preserve the stored values, without displaying any potentially problematic data - just display the PK, perhaps. Raise an error when they try to save.

2. remove just the offending values (not all, in the case of multi-select). When the form is saved, that means that the old data gets over-written with new (valid) data.

3. if any values are invalid, replace with empty list. When the form is saved, the old data gets replaced with an empty list (assuming the user didn't select any new items).

4. is the only one that it is easy to do.

5. and 3) are potential data loss scenarios, especially 3), because it would be easy for a user to load a form and save it again, without noticing that the selected items had been reduced or truncated.

An enhancement would be some kind of warning, indicating that invalid values had been removed. That would make 2) or 3) more palatable.

[spookylukey]

The most basic fix, option 3, without any enhancement, looks like this:

https://github.com/spookylukey/django-autocomplete-light/compare/fix_validation_crash_on_render?expand=1

[jpic]

First note that in v2 branch, validation is not done in the widget anymore but in the form field.

Concerning your fix, I think you could implement your fix in the autocomplete class, overriding choices_for_values(). Wouldn't that work ?

Concerning your other options, i think 1) is the less dirty. I'll find a better way to address this issue in v2.

[jpic]

I have pre-released 2.0.0 which fixes the problem of "validation during widget rendering".

About your issue @spookylukey, I'm going to make sure that the behavior of our widget matches the one of django's Select widget, but it should be a separate issue I think.

So I'm leaving this one open until another issue is created (which eventually I will do if you don't ;) ).

[jpic]

@spookylukey I have checked what happens with django Select.

If a selection is not part of the queryset anymore, then it will not be represented as an <option>. So when I submit the form again the invisible, previously selected model will be disconnected from the model.

I will make django-autocomplete-light match this behavior (if not already the case in v2, since validation is gone from the widget.render()).

[jpic]

Please re-open if the problem persists.

Thanks again for your feedback.

[jpic]

FAQ added : http://django-autocomplete-light.readthedocs.org/en/v2/faq.html#fields-bound-on-values-which-are-not-in-the-queryset-anymore-raise-a-validationerror

[jpic]

If anyone has the opportunity to try v2, I'd like to have some feedback. I expect it to solve every issue mentioned in this thread, it's sort of frustrating not to be able to test it myself on your projects and see if it works as I expect :D

[spookylukey]

Just upgraded, and v2 is working fine for me, thanks, although I had some difficulties upgrading initially - see my comments on issue #229