-
Notifications
You must be signed in to change notification settings - Fork 25
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
Rework org perms again #5657
base: main
Are you sure you want to change the base?
Rework org perms again #5657
Conversation
06f920c
to
fd83f23
Compare
@@ -48,7 +48,7 @@ def test_has_permission(self): | |||
self.assertEqual(200, self.client.get(create_url).status_code) | |||
|
|||
# staff still can't POST | |||
self.assertLoginRedirect(self.client.post(create_url, {"name": "Sales"})) | |||
self.assertEqual(403, self.client.post(create_url, {"name": "Sales"}).status_code) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericnewcomer ok so this is changing again.. but I think this is safer. You POST as staff, the request just fails. No redirecting to login and then maybe back and wondering what happened with the original form submission.
@@ -972,8 +972,7 @@ def test_configuration(self): | |||
config_url = reverse("channels.channel_configuration", args=[self.ex_channel.uuid]) | |||
|
|||
# can't view configuration if not logged in | |||
response = self.client.get(config_url) | |||
self.assertLoginRedirect(response) | |||
self.assertRequestDisallowed(config_url, [None, self.agent]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really gotta figure out a consistent behavior for when you hit an object specific URL for an object you don't have access to (but might by switching orgs). Here we're changing to a 404 from a login redirect.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5657 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 561 561
Lines 25798 25799 +1
=========================================
+ Hits 25798 25799 +1 ☔ View full report in Codecov by Sentry. |
fd83f23
to
65dd6cc
Compare
@@ -37,7 +37,7 @@ def derive_queryset(self, **kwargs): | |||
|
|||
# don't filter by org for staff users but let OrgObjPermsMixin provide a redirect | |||
if not self.request.user.is_staff: | |||
qs = qs.filter(org=self.request.org) | |||
qs = qs.filter(org__users=self.request.user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we were brave we could just not filter by org at all and trust OrgObjPerms
to do its job
Reworked to make it easier to redirect regular users to a org-switcher page if they access a URL for an object that belongs to an org they also belong to (like what we do for staff).