fix(slack): Ensure Slack user is member of org for Seer actions#118877
Conversation
| if group.organization.has_access(user): | ||
| return True | ||
|
|
||
| _logger.info( |
There was a problem hiding this comment.
Nit / Question
should we just raise an exception here or fail more loudly? (i like the ux of letting the user know that they don't have permission to do it, but not sure if a 403 would also return that to the slack interface or not)
we could also have this raise an exception, we catch it and send the org not a member, then refire it. eventually re-catching it and returning a 403 forbidden.
the way this would kinda work / help is when we're using this method right now we check to see if the user has access, then if they don't we return. instead, we could raise an exception here, just check to see if they have access, and if not it'll raise an exception that bubbles up to the call sites. If there's a centralized spot(s) that we could wrap with handlers, it might make the code flows a little cleaner.
in the end, this would probably only be a helpful thing to do if we can get back to a single callsite / activity handler from seer.
There was a problem hiding this comment.
any non 2xx wouldn't surface anything to the slack interface, I think it'd just show the "something went wrong" message.
| def _has_seer_org_access( | ||
| self, | ||
| *, | ||
| slack_request: SlackActionRequest, | ||
| entrypoint: SlackAutofixEntrypoint, |
There was a problem hiding this comment.
since the name of this method is just _has_seer_org_access i wouldn't expect it to also send the notification -- feels a little like a side effect, and the method signature here is adding to that.
I wonder if we could pass in the method that wraps send_not_org_message to it, and have the method be an input into this? handle_not_member=send_not_org_message kind of thing.
The end result might look something like:
def idk_the_caller_name():
def not_org_member_slack():
return send_not_org_member_message( .... )
if _has_seer_org_access(user, organization, handle_not_member=not_org_member_slack):
...
^ so the nice thing about this is, i don't have to think about why it's sending the slack request / entry point over to the org access check, i'd also filter the group down to just the organization, so the method signature reads like "i have a user and an org, if the user doesn't belong to this org, call this method".
all that could also be boiled back down to a single helper that's like validate_user_access or something, for whatever reason i feel like that has less of a connotation that it's just trying to do the boolean operation. naming is hard.
Add a check to make sure the acting Slack user is a member of the group's organization before performing Slack Seer autofix actions