Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
367 changes: 367 additions & 0 deletions fixtures/github.py

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions src/sentry/features/temporary.py
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,9 @@ def register_temporary_features(manager: FeatureManager) -> None:
manager.add("organizations:integrations-deployment", OrganizationFeature, FeatureHandlerStrategy.INTERNAL, default=True, api_expose=True)
manager.add("organizations:integrations-feature-flag-integration", OrganizationFeature, FeatureHandlerStrategy.INTERNAL, api_expose=False)
manager.add("organizations:integrations-cursor", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True)
# Project Management Integrations Feature Parity Flags
# GitHub inbound assignee sync
manager.add("organizations:integrations-github-inbound-assignee-sync", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True)
# Enable inviting billing members to organizations at the member limit.
manager.add("organizations:invite-billing", OrganizationFeature, FeatureHandlerStrategy.INTERNAL, default=False, api_expose=False)
# Enable inviting members to organizations.
Expand Down
59 changes: 59 additions & 0 deletions src/sentry/integrations/github/webhook.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
from sentry.integrations.types import IntegrationProviderSlug
from sentry.integrations.utils.metrics import IntegrationWebhookEvent, IntegrationWebhookEventType
from sentry.integrations.utils.scope import clear_tags_and_context
from sentry.integrations.utils.sync import sync_group_assignee_inbound_by_external_actor
from sentry.models.commit import Commit
from sentry.models.commitauthor import CommitAuthor
from sentry.models.commitfilechange import CommitFileChange, post_bulk_create
Expand Down Expand Up @@ -497,6 +498,63 @@ def _handle(
repo.save()


class IssuesEventWebhook(GitHubWebhook):
"""https://developer.github.com/v3/activity/events/types/#issuesevent"""

@property
def event_type(self) -> IntegrationWebhookEventType:
return IntegrationWebhookEventType.INBOUND_SYNC

def _handle(self, integration: RpcIntegration, event: Mapping[str, Any], **kwargs: Any) -> None:
"""
Handle GitHub issue events, particularly assignment and status changes.
"""

action = event.get("action")
issue = event.get("issue", {})
repository = event.get("repository", {})
repo_full_name = repository.get("full_name")
issue_number = issue.get("number")
assignee_gh_name = event.get("assignee", {}).get("login")
Copy link
Contributor

Choose a reason for hiding this comment

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

Potential bug: The code will raise an AttributeError when the GitHub webhook payload contains "assignee": null, as it attempts to call .get("login") on a None object.
  • Description: The expression event.get("assignee", {}).get("login") will raise an AttributeError when processing a GitHub webhook for an unassigned issue. The GitHub API sends "assignee": null in this scenario. The event.get("assignee", {}) call correctly returns None instead of the default {}, but the subsequent chained call to .get("login") on this None value causes the exception. This crashes the webhook handler before it reaches the intended validation logic that checks for a falsy assignee_gh_name, preventing the synchronization of issue unassignments from GitHub.

  • Suggested fix: Safely access the nested login value. First, retrieve the assignee object using assignee = event.get("assignee"). Then, conditionally get the login value, for example: assignee_gh_name = assignee.get("login") if assignee else None.
    severity: 0.75, confidence: 0.95

Did we get this right? 👍 / 👎 to inform future reviews.


if not repo_full_name or not issue_number or not assignee_gh_name:
logger.warning(
"github.webhook.missing-data",
extra={
"integration_id": integration.id,
"repo": repo_full_name,
"issue_number": issue_number,
"action": action,
},
)
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: Webhook Fails on Null Assignee

The IssuesEventWebhook unconditionally extracts and validates assignee data. This leads to warnings and early exits for issue events not related to assignment. Additionally, the assignee extraction can raise an AttributeError if the GitHub payload includes an assignee field with a null value.

Fix in Cursor Fix in Web


external_issue_key = f"{repo_full_name}#{issue_number}"

# Handle issue assignment changes
if action in ["assigned", "unassigned"]:
# Sentry uses the @username format for assignees
assignee_name = "@" + assignee_gh_name

# Sync the assignment to Sentry
sync_group_assignee_inbound_by_external_actor(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

do we want to FF this? i am thinking through how we will FF all the features i am adding also per integration and can't think of a clean way to do this.

open to suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

Good call! yes, let's feature flag.

integration=integration,
external_user_name=assignee_name,
external_issue_key=external_issue_key,
assign=(action == "assigned"),
)

logger.info(
"github.webhook.assignment.synced",
extra={
"integration_id": integration.id,
"external_issue_key": external_issue_key,
"assignee_name": assignee_name,
"action": action,
},
)


class PullRequestEventWebhook(GitHubWebhook):
"""https://developer.github.com/v3/activity/events/types/#pullrequestevent"""

Expand Down Expand Up @@ -621,6 +679,7 @@ class GitHubIntegrationsWebhookEndpoint(Endpoint):
"push": PushEventWebhook,
"pull_request": PullRequestEventWebhook,
"installation": InstallationEventWebhook,
"issues": IssuesEventWebhook,
}

def get_handler(self, event_type: str) -> type[GitHubWebhook] | None:
Expand Down
17 changes: 17 additions & 0 deletions src/sentry/integrations/utils/sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

from django.db.models.query import QuerySet

from sentry import features
from sentry.integrations.mixins.issues import where_should_sync
from sentry.integrations.models.external_actor import ExternalActor
from sentry.integrations.models.integration import Integration
Expand All @@ -19,7 +20,9 @@
from sentry.integrations.types import EXTERNAL_PROVIDERS_REVERSE, ExternalProviderEnum
from sentry.models.group import Group
from sentry.models.groupassignee import GroupAssignee
from sentry.models.organization import Organization
from sentry.models.project import Project
from sentry.organizations.services.organization.model import RpcOrganization
from sentry.silo.base import region_silo_function
from sentry.users.services.user.model import RpcUser
from sentry.users.services.user.service import user_service
Expand All @@ -33,6 +36,14 @@ class AssigneeInboundSyncMethod(StrEnum):
EXTERNAL_ACTOR = "external_actor"


def should_sync_assignee_inbound(
organization: Organization | RpcOrganization, provider: str
) -> bool:
if provider == "github":
return features.has("organizations:integrations-github-inbound-assignee-sync", organization)
return True


def _get_user_id(projects_by_user: dict[int, set[int]], group: Group) -> int | None:
user_ids = [
user_id
Expand Down Expand Up @@ -60,6 +71,9 @@ def _handle_deassign(
groups: QuerySet[Group], integration: RpcIntegration | Integration
) -> QuerySet[Group]:
for group in groups:
if not should_sync_assignee_inbound(group.organization, integration.provider):
continue

GroupAssignee.objects.deassign(
group,
assignment_source=AssignmentSource.from_integration(integration),
Expand All @@ -79,6 +93,9 @@ def _handle_assign(
projects_by_user = Project.objects.get_by_users(users)

for group in affected_groups:
if not should_sync_assignee_inbound(group.organization, integration.provider):
continue

user_id = _get_user_id(projects_by_user, group)
user = users_by_id.get(user_id) if user_id is not None else None
if user:
Expand Down
141 changes: 141 additions & 0 deletions tests/sentry/integrations/github/test_webhooks.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
INSTALLATION_API_RESPONSE,
INSTALLATION_DELETE_EVENT_EXAMPLE,
INSTALLATION_EVENT_EXAMPLE,
ISSUES_ASSIGNED_EVENT_EXAMPLE,
ISSUES_UNASSIGNED_EVENT_EXAMPLE,
PULL_REQUEST_CLOSED_EVENT_EXAMPLE,
PULL_REQUEST_EDITED_EVENT_EXAMPLE,
PULL_REQUEST_OPENED_EVENT_EXAMPLE,
Expand All @@ -17,6 +19,7 @@
from sentry.constants import ObjectStatus
from sentry.integrations.models.integration import Integration
from sentry.integrations.models.organization_integration import OrganizationIntegration
from sentry.integrations.services.integration import integration_service
from sentry.middleware.integrations.parsers.github import GithubRequestParser
from sentry.models.commit import Commit
from sentry.models.commitauthor import CommitAuthor
Expand All @@ -29,7 +32,9 @@
from sentry.testutils.asserts import assert_failure_metric, assert_success_metric
from sentry.testutils.cases import APITestCase
from sentry.testutils.helpers import override_options
from sentry.testutils.helpers.features import with_feature
from sentry.testutils.silo import assume_test_silo_mode, control_silo_test
from sentry.utils import json


class WebhookTest(APITestCase):
Expand Down Expand Up @@ -994,3 +999,139 @@ def assert_group_link(self, group, pr):
assert link.group_id == group.id
assert link.linked_id == pr.id
assert link.linked_type == GroupLink.LinkedType.pull_request


@with_feature("organizations:integrations-github-inbound-assignee-sync")
class IssuesEventWebhookTest(APITestCase):
def setUp(self) -> None:
self.url = "/extensions/github/webhook/"
self.secret = "b3002c3e321d4b7880360d397db2ccfd"
options.set("github-app.webhook-secret", self.secret)

future_expires = datetime.now().replace(microsecond=0) + timedelta(minutes=5)

with assume_test_silo_mode(SiloMode.CONTROL):
integration = self.create_integration(
organization=self.organization,
external_id="12345",
provider="github",
metadata={"access_token": "1234", "expires_at": future_expires.isoformat()},
)
integration.add_organization(self.project.organization.id, self.user)
self.integration = integration

@patch("sentry.integrations.github.webhook.sync_group_assignee_inbound_by_external_actor")
@patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
def test_assigned_issue(self, mock_record: MagicMock, mock_sync: MagicMock) -> None:

Repository.objects.create(
organization_id=self.project.organization.id,
external_id="35129377",
provider="integrations:github",
name="baxterthehacker/public-repo",
)

response = self.client.post(
path=self.url,
data=ISSUES_ASSIGNED_EVENT_EXAMPLE,
content_type="application/json",
HTTP_X_GITHUB_EVENT="issues",
HTTP_X_HUB_SIGNATURE="sha1=e19d80f5a6e09e20d126e923464778bb4b601a7e",
HTTP_X_HUB_SIGNATURE_256="sha256=e91927e8d8e0db9cb1f5ead889bba2deb24aa2f8925a8eae85ba732424604489",
HTTP_X_GITHUB_DELIVERY=str(uuid4()),
)

assert response.status_code == 204

rpc_integration = integration_service.get_integration(integration_id=self.integration.id)

mock_sync.assert_called_once_with(
integration=rpc_integration,
external_user_name="@octocat",
external_issue_key="baxterthehacker/public-repo#2",
assign=True,
)

assert_success_metric(mock_record)

@patch("sentry.integrations.github.webhook.sync_group_assignee_inbound_by_external_actor")
@patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
def test_unassigned_issue(self, mock_record: MagicMock, mock_sync: MagicMock) -> None:

Repository.objects.create(
organization_id=self.project.organization.id,
external_id="35129377",
provider="integrations:github",
name="baxterthehacker/public-repo",
)

response = self.client.post(
path=self.url,
data=ISSUES_UNASSIGNED_EVENT_EXAMPLE,
content_type="application/json",
HTTP_X_GITHUB_EVENT="issues",
HTTP_X_HUB_SIGNATURE="sha1=8d2cf8bdfaae30fc619bfbfafee3681404a12d6b",
HTTP_X_HUB_SIGNATURE_256="sha256=19794c8575c58d0be5d447e08b50d7cc235e7f7e76b32a0c371988d4335fab21",
HTTP_X_GITHUB_DELIVERY=str(uuid4()),
)

assert response.status_code == 204

rpc_integration = integration_service.get_integration(integration_id=self.integration.id)

mock_sync.assert_called_once_with(
integration=rpc_integration,
external_user_name="@octocat",
external_issue_key="baxterthehacker/public-repo#2",
assign=False,
)

assert_success_metric(mock_record)

def test_missing_assignee_data(self) -> None:

Repository.objects.create(
organization_id=self.project.organization.id,
external_id="35129377",
provider="integrations:github",
name="baxterthehacker/public-repo",
)

event_data = json.loads(ISSUES_ASSIGNED_EVENT_EXAMPLE)
del event_data["assignee"]

response = self.client.post(
path=self.url,
data=json.dumps(event_data),
content_type="application/json",
HTTP_X_GITHUB_EVENT="issues",
HTTP_X_HUB_SIGNATURE="sha1=fake",
HTTP_X_HUB_SIGNATURE_256="sha256=fake",
HTTP_X_GITHUB_DELIVERY=str(uuid4()),
)

# Should fail due to invalid signature
assert response.status_code == 401

@patch("sentry.integrations.github.webhook.metrics")
def test_creates_missing_repo_for_issues(self, mock_metrics: MagicMock) -> None:

response = self.client.post(
path=self.url,
data=ISSUES_ASSIGNED_EVENT_EXAMPLE,
content_type="application/json",
HTTP_X_GITHUB_EVENT="issues",
HTTP_X_HUB_SIGNATURE="sha1=e19d80f5a6e09e20d126e923464778bb4b601a7e",
HTTP_X_HUB_SIGNATURE_256="sha256=e91927e8d8e0db9cb1f5ead889bba2deb24aa2f8925a8eae85ba732424604489",
HTTP_X_GITHUB_DELIVERY=str(uuid4()),
)

assert response.status_code == 204

repos = Repository.objects.all()
assert len(repos) == 1
assert repos[0].organization_id == self.project.organization.id
assert repos[0].external_id == "35129377"
assert repos[0].provider == "integrations:github"
assert repos[0].name == "baxterthehacker/public-repo"
mock_metrics.incr.assert_called_with("github.webhook.repository_created")
2 changes: 2 additions & 0 deletions tests/sentry/integrations/utils/test_sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
from sentry.models.groupassignee import GroupAssignee
from sentry.models.organization import Organization
from sentry.testutils.cases import TestCase
from sentry.testutils.helpers.features import with_feature
from sentry.testutils.silo import assume_test_silo_mode_of, region_silo_test
from sentry.users.models import User, UserEmail
from sentry.users.services.user import RpcUser
Expand Down Expand Up @@ -225,6 +226,7 @@ def raise_exception(*args, **kwargs):


@region_silo_test
@with_feature("organizations:integrations-github-inbound-assignee-sync")
class TestSyncAssigneeInboundByExternalActor(TestCase):

@pytest.fixture(autouse=True)
Expand Down
Loading