fix(crons): honor alertsMemberWrite setting #104171
Conversation
github-actions
bot
added
the
Scope: Backend
|
|
||
| # team admins should be able to create alerts for the projects they have access to | ||
| # Verify that get_projects is available (requires OrganizationEndpoint) | ||
| if not hasattr(self, "get_projects"): |
There was a problem hiding this comment.
Should we record a sentry error here? Since presumably if this happens it means we’ve implemented things wrong (not extending organization endpoint)
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
src/sentry/incidents/endpoints/organization_alert_rule_index.py
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #104171 +/- ##
===========================================
+ Coverage 80.04% 80.71% +0.67%
===========================================
Files 9332 9369 +37
Lines 398425 402381 +3956
Branches 25480 25480
===========================================
+ Hits 318908 324787 +5879
+ Misses 79067 77144 -1923
Partials 450 450 |
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you add the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| """ | ||
| Create a new monitor. | ||
| """ | ||
| self.check_can_create_alert(request, organization) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| ) | ||
| # all() returns True for empty list, so include a check for it | ||
| if not team_admin_has_access or not projects: | ||
| raise PermissionDenied |
There was a problem hiding this comment.
Permission check validates all projects, not the requested one
Medium Severity
The check_can_create_alert method calls self.get_projects(request, organization) without passing the target project, which returns ALL accessible projects rather than the project specified in the POST body. The subsequent check verifies alerts:write permission on ALL returned projects using all(). This causes users with mixed team permissions (team admin on some projects, regular member on others) to be incorrectly denied when trying to create a monitor for a project they DO have admin access to, because the permission check fails on the other projects where they only have member access.
Additional Locations (1)
| return | ||
|
|
||
| # team admins should be able to create alerts for the projects they have access to | ||
| projects = self.get_projects(request, organization) |
There was a problem hiding this comment.
Does this get all projects the user has access to, or just the projects in the current url params selection?
There was a problem hiding this comment.
Returns all project ids that the user can access within this organization:
https://github.com/getsentry/sentry/blob/master/src/sentry/api/bases/organization.py#L355-L369
Btw this is not the new code, it was moved from OrganizationAlertRuleIndexEndpoint.check_can_create_alert
https://github.com/getsentry/sentry/pull/104171/changes#diff-7ce0c5c5a5e204143274734b7e9a288cd70a869fa5d460e99eee830e44cda682L627-L628
The previous fix (#86315) did not really fix the issue.
Here we're reusing
check_can_create_alertmethod first implemented in #79624Also, for the
OrganizationAlertRuleIndexEndpoint.postmethod I extracted the following logic because it is only related to metric detectors:https://github.com/getsentry/sentry/pull/97445/files#diff-7ce0c5c5a5e204143274734b7e9a288cd70a869fa5d460e99eee830e44cda682