Skip to content

fix(slack): Ensure Slack user is member of org for Seer actions#118877

Merged
ceorourke merged 2 commits into
masterfrom
ceorourke/ISWF-2861
Jul 2, 2026
Merged

fix(slack): Ensure Slack user is member of org for Seer actions#118877
ceorourke merged 2 commits into
masterfrom
ceorourke/ISWF-2861

Conversation

@ceorourke

Copy link
Copy Markdown
Member

Add a check to make sure the acting Slack user is a member of the group's organization before performing Slack Seer autofix actions

@ceorourke ceorourke requested review from a team as code owners July 1, 2026 21:13
@linear-code

linear-code Bot commented Jul 1, 2026

Copy link
Copy Markdown

ISWF-2861

@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 1, 2026
@ceorourke ceorourke requested a review from a team July 1, 2026 21:14
if group.organization.has_access(user):
return True

_logger.info(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any non 2xx wouldn't surface anything to the slack interface, I think it'd just show the "something went wrong" message.

Comment on lines +575 to +579
def _has_seer_org_access(
self,
*,
slack_request: SlackActionRequest,
entrypoint: SlackAutofixEntrypoint,

@saponifi3d saponifi3d Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ceorourke ceorourke requested a review from saponifi3d July 2, 2026 20:31

@saponifi3d saponifi3d left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! 🎉

@ceorourke ceorourke merged commit a0e4baf into master Jul 2, 2026
66 checks passed
@ceorourke ceorourke deleted the ceorourke/ISWF-2861 branch July 2, 2026 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants