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

Delete perms: must be staff and in group #82

Merged
merged 3 commits into from
Jul 24, 2019
Merged

Delete perms: must be staff and in group #82

merged 3 commits into from
Jul 24, 2019

Conversation

james1293
Copy link
Contributor

AFAICT, this change makes the code match the intended behavior described in the comment:

# Ensure user has permission to delete list. Get the group this list belongs to,
# and check whether current user is a member of that group AND a staffer.

before this change, a staffer could delete any list.

Example evaluation of the part after the and:

task_list.group not in request.user.groups.all() and not request.user.is_staff
# ...evaluates to...
task_list.group not in request.user.groups.all() and not True
# ...evaluates to...
task_list.group not in request.user.groups.all() and False
# ...evaluates to...
False

@james1293
Copy link
Contributor Author

Hmm, looks like a test failed. I'll look at that tomorrow.

@shacker
Copy link
Owner

shacker commented Jul 23, 2019

Well spotted, thanks! I think the right fix is just to replace the and with or:

if task_list.group not in request.user.groups.all() or not request.user.is_staff:

Or they could become two separate checks, which would actually be more readable. I'm fine with either. And thanks for making sure the change keeps the tests passing.

@james1293
Copy link
Contributor Author

True, those would all be equiv. Good old De Morgan's laws. I ended up doing separate checks.

Not sure if I should do some fancy git-fu to remove my original commit and condense the other two into one. I know that if I were just working locally, I could git reset HEAD^; feel free to suggest options if you prefer a cleaner commit history.

Copy link
Owner

@shacker shacker left a comment

Choose a reason for hiding this comment

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

Nice, thanks for the contribution!

@shacker shacker merged commit 7f576c9 into shacker:master Jul 24, 2019
@shacker
Copy link
Owner

shacker commented Jul 24, 2019

Not sure if I should do some fancy git-fu to remove my original commit and condense the other two into one.

No need - I always squash PRs during merge, so it all becomes one fresh commit in the end anyway.

@james1293 james1293 deleted the patch-3 branch July 24, 2019 14:48
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