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

Restrict dropdown choices #258

Merged
merged 13 commits into from
Mar 2, 2023
Merged

Restrict dropdown choices #258

merged 13 commits into from
Mar 2, 2023

Conversation

dandavies99
Copy link
Contributor

Restricts dropdown choices to those the user has permission for.

This ensures that no private device specifications are listed as options in:

  • The filter form when searching for batches
  • The form for creating or updating new batches

I can't see any other places there could be a risk of this happening.

Closes #72 (finally)

Copy link
Collaborator

@AdrianDAlessandro AdrianDAlessandro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is related to this PR, but in trying to test this I tried to make a public and a private batch and selected "Module" as the specification for one of them and got the error: "Select a valid choice. That choice is not one of the available choices."

Then when I went to select another choice, "Module" was no longer there ("Module" is an abstract specification, if that helps)

@AdrianDAlessandro
Copy link
Collaborator

This doesn't address the "Remove AnonymousUser from dropdown options." test in #72

Copy link
Collaborator

@AdrianDAlessandro AdrianDAlessandro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so hard to test... But I think I found a few places where it needs to be hidden. I made a user that is a contributor and I can see a couple of private things still.

Batches in dropdown (the "None" batch is private - it lost it's name at some point):
Screenshot 2023-02-20 at 17 58 25

Secret Experiments (ie a private experiment that uses a public batch):
Screenshot 2023-02-20 at 17 57 35

Private batches when looking at public experiments (although this one seems like user-error):
Screenshot 2023-02-20 at 17 57 11

AnonymousUser appearing in filters:
Screenshot 2023-02-20 at 17 56 51

@dandavies99
Copy link
Contributor Author

dandavies99 commented Feb 21, 2023

Not sure if this is related to this PR, but in trying to test this I tried to make a public and a private batch and selected "Module" as the specification for one of them and got the error: "Select a valid choice. That choice is not one of the available choices."

Then when I went to select another choice, "Module" was no longer there ("Module" is an abstract specification, if that helps)

Yeah this is a bit annoying.

This is unexpected because limit_choices_to={"abstract": False} is set on the specification field of the Batch() class.

I've tried explicitly setting allowed_device_specs = allowed_device_specs.filter(abstract=False) in the filter but it doesn't change the behaviour.

For now, at least the form refreshes with a sensible message and reduced list of actually allowed device specs, but I'll flag this in a separate issue.

UPDATE
allowed_device_specs = allowed_device_specs.filter(abstract=False) ⬅️ this line just needed to go in the __init__ method of the form.

@dandavies99
Copy link
Contributor Author

I had forgotten about the AnonymousUser appearing in the dropdowns issue, but that was an easy fix by modifying the BaseFilter ✅

@dandavies99
Copy link
Contributor Author

Batches in dropdown (the "None" batch is private - it lost it's name at some point)

Fixing this for inline forms was a bit fiddly but the solution seems to be working for the create/edit experiment page. Only batches the user has permission to view now appear.

@dandavies99
Copy link
Contributor Author

Secret Experiments (ie a private experiment that uses a public batch):

I don't think this is as much of an issue, but it depends how the experiment is named. The template has now been modified to check the experiment permissions before rendering each row.

Private batches when looking at public experiments (although this one seems like user-error)

Yeh we're getting into the boggy mires of how permissions of various objects should propagate. For simplicity, I've done the same as above and checked the permissions in the template for each batch when each row of the template is rendered.

@dandavies99
Copy link
Contributor Author

dandavies99 commented Feb 21, 2023

Thanks for the thorough checks, @AdrianDAlessandro. Ready for another look 😉

For clarity:

  • The edits to battDB.filters are to restrict the querysets that populate the dropdowns at the top of the table views.
  • The edits to common.filters are to restrict the querysets that populate the user_owner dropdown at the top of table views.
  • The edits to battDB.forms are to restrict the querysets that populate the dropdowns within create and edit views (where the forms are displayed).

Copy link
Collaborator

@AdrianDAlessandro AdrianDAlessandro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Looks like this covers all the issues I commented on

@dandavies99 dandavies99 merged commit 4b59d4e into develop Mar 2, 2023
@dandavies99 dandavies99 deleted the dropdown-privacy branch March 2, 2023 15:43
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.

Filter fields to limit dropdown options
2 participants