jazzband / django-smart-selects

chained and grouped selects for django forms
https://django-smart-selects.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
1.12k stars 352 forks source link

Add optional SMART_SELECTS_CHECK_MODEL_PERMISSION setting. #307

Open cveilleux opened 4 years ago

cveilleux commented 4 years ago

If enabled, a standard django permission check is performed on the chained model in the API endpoint. This prevents information leaks to unauthorized users.

The extra check is disabled by default to not break backward compatibility.

manelclos commented 4 years ago

Hi @cveilleux, thanks for taking the time to prepare this PR. Adding permission checks would be really good.

In the Django Admin, you don't need permission on the Foreign Key model, but on the model you are editing. Django uses the base manager to get the related objects. I think we should do the same here.

Please let me know if this makes sense.

cveilleux commented 4 years ago

You are right. You need edit permission on the model containing the foreign key, not view permission on the referenced model.

So the permission check would have to check if you have edit permission on any model containing a foreign key referencing the auto-complete.

So if we have a model name, is there a way in django to find all the ChainedForeignKey fields referencing this model?

manelclos commented 4 years ago

Hi @cveilleux, in the filterchain function:

def filterchain(request, app, model, field, foreign_key_app_name, foreign_key_model_name,
                foreign_key_field_name, value, manager=None):

So when editing a Location like in http://127.0.0.1:8000/admin/test_app/location/1/change/, values are:

So your code is almost right, as you check for permission test_app.view_Location.

Please adjust your code to use lower() in the permission (though it works anyway), and also test for change permission, like:

    perm = '{0}.change_{1}'.format(foreign_key_app_name, foreign_key_model_name).lower()

I tested with those changes and everything seems to work ok.

Some considerations:

Let me know if you will take on those too.