Security checking for model-fields breaks use of form-fields only

optiz0r commented 7 years ago

Checklist

[x] This issue is not about installing previous versions of django-smart-selects older than 1.2.8. I understand that previous versions are insecure and will not receive any support whatsoever.
[x] I have verified that that issue exists against the master branch of django-smart-selects.
[x] I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
[x] I have debugged the issue to the smart_selects app.
[x] I have reduced the issue to the simplest possible case.
[x] I have included all relevant sections of models.py, forms.py, and views.py with problems.
[x] I have used GitHub Flavored Markdown to style all of my posted code.

Steps to reproduce

Update to 1.5.x

Actual behavior

403 prevents chaining from working

Expected behavior

Chaining to still work

Explanation

The changes in 051ea42 have completely broken my (perhaps niche?) use-case. My models only contain the foreign key that is at the end of the chain, rather than all the foreign keys for each step of the chain. This was a deliberate decision to avoid storing duplicate information in the database since my models are modified by both webui but also via REST API. As such I don't use the Chained model fields at all, only the Chained Form fields, which I've been manually adding to forms.

An example chain is Continent->Country->Address->Item->Child Item. My Child Item contains only an item_id field, not also continent_id, country_id, address_id fields.

The two potential database integrity issues I'm concerned about are:

When client wants to modify the last FK in the chain, not having to look up at reset all parent FKs in the chain too. E.g. if my Item is chained to a different Address, I don't want continent_id, country_id fields on the item to need to be modified.
If a lower item in the chain is modified, associating it with a different parent above it in the chain, not invalidating every single model in the database. E.g. if someone moves an Item to a different Address, every single Child Item model in the database would immediately have an invalid address_id.

I don't want to over-complicate my models by adding complex save() logic to check for and update the parents in the chain each time they are modified.

The security changes stop the form fields working completely because every single lookup is now running a 403 since there are no model fields. I'm not concerned about information leakage via this app. I would prefer the specific check for model-fields were optional, and that the security concerns were mitigated by checking for model and object-level permissions instead.

I'm hoping you'll agree that this is a valid use-case and worth supporting. If so, I'll see if I can help with a PR. In the meantime I'm probably going to have to revert back to 1.3.4 and cherry-pick in the specific bugfixes I need.

Edit (by @blag): Fixed checkboxes to actually be Markdown checkboxes.

blag commented 7 years ago

I'm hoping you'll agree that this is a valid use-case and worth supporting.

Yep.

and that the security concerns were mitigated by checking for model and object-level permissions instead

This is, AFAICT, difficult to do in a generic fashion to support all users of django-smart-selects. I think a global option to turn off the model field checking (and default to on) would be a sufficient situation. If users need more complicated permissions checks they can use django-autocomplete-light, which requires much more configuration and custom coding than django-smart-selects.

I'll see if I can help with a PR.

That would be great and very appreciated. Please namespace the global option with SMART_SELECTS_ and default to the more secure option.

blag commented 7 years ago

And if adding tests for the new option aren't too difficult, please add tests for it.

jazzband / django-smart-selects