security(issue-alerts): Validate the issue alert owner is a member of the organization #29962
Conversation
… the organization Followup to #29939, this fixes the same problem in issue alerts. Also noticed some tests weren't running since they were accidentally nested. One could be removed since it was no longer valid, put the other back in place.
| ) | ||
|
|
||
| assert response.status_code == 400, response.content | ||
| assert str(response.data["owner"][0]) == "User is not a member of this organization" |
There was a problem hiding this comment.
@wedamija shouldn't we give a 404 or something as to not give away that this organization exists if the user isn't part of it?
There was a problem hiding this comment.
The organization will already be validated at this point, so that's not a concern since the user will only hit this point if they're a member of the org. It might give away that a given user id or team id is a valid id, but it's easy enough to create a new user or team and assume that every id between 0 -> <new_id> is likely used, so we're not giving much away there either.
There was a problem hiding this comment.
@wedamija I'm confused. If the user is not a member of the organization and they hit this endpoint, are they going to get a 400 or a 404?
There was a problem hiding this comment.
oh duh...I understand now. The owner isn't necessarily the user making the request.
There was a problem hiding this comment.
This pr isn't changing that. There are two users involved here:
- The user making the request. This user has to be authenticated and authorized to access the organization and project to create/modify alert rules on the project. I'm not changing this validation at all.
- The owner of the rule. This is a parameter that the user can provide, and can be a team or a user. Previously this could be any sentry user/team, and now it's restricted to only users and teams that belong to the org. If the passed owner is from another org, then we will return a 400 here.
I don't think that will affect it? The |
Followup to #29939, this fixes the same problem in issue
alerts.
Also noticed some tests weren't running since they were accidentally nested. One could be removed
since it was no longer valid, put the other back in place.