Fix validators using ActorField, replace with ActorOwnerField#106074
Fix validators using ActorField, replace with ActorOwnerField#106074geoffg-sentry merged 8 commits intomasterfrom
Conversation
github-actions
bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
|
||
|
|
||
| @extend_schema_field(field=OpenApiTypes.STR) | ||
| class OwnerActorField(ActorField): |
There was a problem hiding this comment.
Should this just be a configuration on the ActorField?
I'm always weary of inheritence.
There was a problem hiding this comment.
I considered that, make it opt in, but then we have the same problem of people forgetting to opt-in to the auth checks. That forgetfulness is pretty common across alert related endpoints and I want to avoid it
There was a problem hiding this comment.
I think maybe the better approach here is to merge it, then refactor OwnerActorField as its own without extending ActorField. Make it so opt-out is the default, add a ignore_membership_checks=true param, and then refactor any use of ActorField into the more secure default
There was a problem hiding this comment.
forgetting to opt-in to the auth
Don't we have that same problem with having two actor fields?
There's not a huge difference between having to remember what the difference between the two actor fields are, and remebering to toggle some option right?
There was a problem hiding this comment.
Yarp, hence my suggesting of refactoring any use of ActorField after the fact.
There was a problem hiding this comment.
Ah so are you suggesting getting rid of ActorField completely?
There was a problem hiding this comment.
Converting all the current usage to a opt-in by default usage of OwnerActorField instead is ideal, yeah. Initially I was going to do that to ActorField but it became a much larger refactor then the problem I was trying to solve now so it'll be better to stage them separately
There was a problem hiding this comment.
@evanpurkhiser, @michelletran-sentry is the nicest and is going to take over the bigger explicit-opt-out refactor afterward so we can get it in pretty soon. Sounds good?
…gnment Create OwnerActorField that extends ActorField with team membership validation - users can only assign teams they belong to (or have team:admin scope). Replace ActorField with OwnerActorField in: - UptimeMonitorValidator - MonitorValidator - AlertRuleSerializer - BaseDetectorTypeValidator Prevents IDORs where org members could create alerts targeting unauthorized teams when Open Team Membership is disabled.
geoffg-sentry
force-pushed
the
fix-validators-using-actorfield
branch
from
13066df to
d581d90
Compare
michelletran-sentry
left a comment
michelletran-sentry
left a comment
There was a problem hiding this comment.
Generally LGTM except for the error messages. I think that we could probably try to consolidate these actors into one class (with an opt-out option), but that would be a larger refactor. So I'm fine with having it in a separate class for simplicity for now.
Co-authored-by: michelletran-sentry <167130096+michelletran-sentry@users.noreply.github.com>
…107333) #106074 wasn't properly handling for open team membership despite the intention. This intends to resolve #107226 & #107326 by: - Accounting for Open Team Membership being enabled OwnerActorField._validate_team_assignment() and validate_bulk_assignment() in the group index helpers - Improving the UI messaging when the behavior is prevented by the access control - Updating existing tests and adding new ones to ensure they're verified Error messaging isn't perfect here because the OwnerActorField spans Issues, Monitors, Alerts, and Detectors but "You can only assign teams you are a member of" is still an improvement. Front end change to follow it
3 participants
ActorField didn't always require ownership validation, created OwnerActorField that extends ActorField with team membership validation - users can only assign teams they belong to (or have team:admin scope). ActorField can still be about identity and OwnerActorField extends it for authorization.
Replace ActorField with OwnerActorField in:
Prevents several IDORs where org members could create alerts targeting unauthorized teams when Open Team Membership is disabled.