Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add optional SMART_SELECTS_CHECK_MODEL_PERMISSION setting. #307

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cveilleux
Copy link

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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):
  • app, model: the ones you are requesting information for
  • field, foreign_key_app_name, foreign_key_field_name: the source model and field

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

  • app: test_app
  • model: Area
  • field: country
  • foreign_key_app_name: test_app
  • foreign_key_field_name: Location

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:

  • Default for SMART_SELECTS_CHECK_MODEL_PERMISSION should be True, this is an important change, so I prefer security over backwards compatibility, people would only need to adjust this setting.
  • We need tests for 200 and 403 responses checking this new functionality
  • The javascript code should be handling the response and informing the user, otherwise you would end up with a bad selection and no warnings.

Let me know if you will take on those too.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants