Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Implement GitHub Webhook #3906

Merged
merged 5 commits into from
May 9, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
3 changes: 3 additions & 0 deletions api/api/urls/v1.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from environments.identities.views import SDKIdentities
from environments.sdk.views import SDKEnvironmentAPIView
from features.views import SDKFeatureStates
from integrations.github.views import github_webhook
from organisations.views import chargebee_webhook

schema_view = get_schema_view(
Expand Down Expand Up @@ -44,6 +45,8 @@
url(r"^metadata/", include("metadata.urls")),
# Chargebee webhooks
url(r"cb-webhook/", chargebee_webhook, name="chargebee-webhook"),
# GitHub integration webhook
url(r"github-webhook/", github_webhook, name="github-webhook"),
# Client SDK urls
url(r"^flags/$", SDKFeatureStates.as_view(), name="flags"),
url(r"^identities/$", SDKIdentities.as_view(), name="sdk-identities"),
Expand Down
2 changes: 1 addition & 1 deletion api/app/settings/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -893,7 +893,7 @@
# GitHub integrations
GITHUB_PEM = env.str("GITHUB_PEM", default="")
GITHUB_APP_ID: int = env.int("GITHUB_APP_ID", default=0)

GITHUB_WEBHOOK_SECRET = env.str("GITHUB_WEBHOOK_SECRET", default="")

# MailerLite
MAILERLITE_BASE_URL = env.str(
Expand Down
45 changes: 44 additions & 1 deletion api/integrations/github/views.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
import hashlib
import hmac
import json
import re
from functools import wraps

Expand All @@ -8,7 +11,7 @@
from rest_framework import status, viewsets
from rest_framework.decorators import api_view, permission_classes
from rest_framework.exceptions import ValidationError
from rest_framework.permissions import IsAuthenticated
from rest_framework.permissions import AllowAny, IsAuthenticated
from rest_framework.response import Response

from integrations.github.client import generate_token
Expand All @@ -25,6 +28,22 @@
from organisations.permissions.permissions import GithubIsAdminOrganisation


def github_webhook_payload_is_valid(payload_body, secret_token, signature_header):
gagantrivedi marked this conversation as resolved.
Show resolved Hide resolved
"""Verify that the payload was sent from GitHub by validating SHA256.
Raise and return 403 if not authorized.
"""
if not signature_header:
return False
hash_object = hmac.new(
secret_token.encode("utf-8"), msg=payload_body, digestmod=hashlib.sha1
)
expected_signature = "sha1=" + hash_object.hexdigest()
if not hmac.compare_digest(expected_signature, signature_header):
return False

return True


def github_auth_required(func):

@wraps(func)
Expand Down Expand Up @@ -204,3 +223,27 @@ def fetch_repositories(request, organisation_pk: int):
return Response(data)
except requests.RequestException as e:
return JsonResponse({"error": str(e)}, status=500)


@api_view(["POST"])
@permission_classes([AllowAny])
def github_webhook(request) -> JsonResponse:
# Validate payload signature
gagantrivedi marked this conversation as resolved.
Show resolved Hide resolved
secret = settings.GITHUB_WEBHOOK_SECRET
signature = request.headers.get("X-Hub-Signature")
github_event = request.headers.get("x-github-event")
payload = request.body
if github_webhook_payload_is_valid(
payload_body=payload, secret_token=secret, signature_header=signature
):
data = json.loads(payload.decode("utf-8"))
# handle GitHub Webhook "installation" event with action type "deleted"
if github_event == "installation" and data["action"] == "deleted":
GithubConfiguration.objects.filter(
installation_id=data["installation"]["id"]
).delete()
return JsonResponse({"detail": "Event processed"}, status=200)
gagantrivedi marked this conversation as resolved.
Show resolved Hide resolved
else:
return JsonResponse({"detail": "Event bypassed"}, status=200)
else:
return JsonResponse({"error": "Invalid signature"}, status=400)
133 changes: 133 additions & 0 deletions api/tests/unit/integrations/github/test_unit_github_views.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import json

import pytest
from django.conf import settings
from django.urls import reverse
from pytest_lazyfixture import lazy_fixture
from pytest_mock import MockerFixture
Expand All @@ -7,9 +10,14 @@

from features.feature_external_resources.models import FeatureExternalResource
from integrations.github.models import GithubConfiguration, GithubRepository
from integrations.github.views import github_webhook_payload_is_valid
from organisations.models import Organisation
from projects.models import Project

WEBHOOK_PAYLOAD = json.dumps({"installation": {"id": 1234567}, "action": "deleted"})
WEBHOOK_SIGNATURE = "sha1=57a1426e19cdab55dd6d0c191743e2958e50ccaa"
WEBHOOK_SECRET = "secret-key"


def test_get_github_configuration(
admin_client_new: APIClient,
Expand Down Expand Up @@ -416,3 +424,128 @@ def test_cannot_fetch_issues_or_prs_when_does_not_have_permissions(

# Then
assert response.status_code == status.HTTP_403_FORBIDDEN


def test_verify_github_webhook_payload() -> None:
# When
result = github_webhook_payload_is_valid(
payload_body=WEBHOOK_PAYLOAD.encode("utf-8"),
secret_token=WEBHOOK_SECRET,
signature_header=WEBHOOK_SIGNATURE,
)

# Then
assert result is True


def test_verify_github_webhook_payload_returns_false_on_bad_signature() -> None:
# When
result = github_webhook_payload_is_valid(
payload_body=WEBHOOK_PAYLOAD.encode("utf-8"),
secret_token=WEBHOOK_SECRET,
signature_header="sha1=757107ea0eb2509fc211221cce984b8a37570b6d7586c22c46f4379c8b043e18",
)

# Then
assert result is False


def test_verify_github_webhook_payload_returns_false_on_no_signature_header() -> None:
# When
result = github_webhook_payload_is_valid(
payload_body=WEBHOOK_PAYLOAD.encode("utf-8"),
secret_token=WEBHOOK_SECRET,
signature_header=None,
)

# Then
assert result is False


def test_github_webhook_delete_installation(
github_configuration: GithubConfiguration,
) -> None:
# Given
settings.GITHUB_WEBHOOK_SECRET = WEBHOOK_SECRET
url = reverse("api-v1:github-webhook")

# When
client = APIClient()
response = client.post(
path=url,
data=WEBHOOK_PAYLOAD,
content_type="application/json",
HTTP_X_HUB_SIGNATURE=WEBHOOK_SIGNATURE,
HTTP_X_GITHUB_EVENT="installation",
)

# Then
assert response.status_code == 200
assert not GithubConfiguration.objects.filter(installation_id=1234567).exists()


def test_github_webhook_fails_on_signature_header_missing(
github_configuration: GithubConfiguration,
) -> None:
# Given
settings.GITHUB_WEBHOOK_SECRET = WEBHOOK_SECRET
url = reverse("api-v1:github-webhook")

# When
client = APIClient()
response = client.post(
path=url,
data=WEBHOOK_PAYLOAD,
content_type="application/json",
HTTP_X_GITHUB_EVENT="installation",
)

# Then
assert response.status_code == 400
assert response.json() == {"error": "Invalid signature"}
assert GithubConfiguration.objects.filter(installation_id=1234567).exists()


def test_github_webhook_fails_on_bad_signature_header_missing(
github_configuration: GithubConfiguration,
) -> None:
# Given
settings.GITHUB_WEBHOOK_SECRET = WEBHOOK_SECRET
url = reverse("api-v1:github-webhook")

# When
client = APIClient()
response = client.post(
path=url,
data=WEBHOOK_PAYLOAD,
content_type="application/json",
HTTP_X_HUB_SIGNATURE="sha1=757107ea0eb2509fc211221cce984b8a37570b6d7586c22c46f4379c8b043e18",
HTTP_X_GITHUB_EVENT="installation",
)

# Then
assert response.status_code == 400
assert GithubConfiguration.objects.filter(installation_id=1234567).exists()
assert response.json() == {"error": "Invalid signature"}


def test_github_webhook_bypass_event(
github_configuration: GithubConfiguration,
) -> None:
# Given
settings.GITHUB_WEBHOOK_SECRET = WEBHOOK_SECRET
url = reverse("api-v1:github-webhook")

# When
client = APIClient()
response = client.post(
path=url,
data=WEBHOOK_PAYLOAD,
content_type="application/json",
HTTP_X_HUB_SIGNATURE=WEBHOOK_SIGNATURE,
HTTP_X_GITHUB_EVENT="installation_repositories",
)

# Then
assert response.status_code == 200
assert GithubConfiguration.objects.filter(installation_id=1234567).exists()
4 changes: 4 additions & 0 deletions infrastructure/aws/production/ecs-task-definition-web.json
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,10 @@
"name": "SSE_AUTHENTICATION_TOKEN",
"valueFrom": "arn:aws:secretsmanager:eu-west-2:084060095745:secret:ECS-API-LxUiIQ:SSE_AUTHENTICATION_TOKEN::"
},
{
Copy link
Member

Choose a reason for hiding this comment

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

Have we added these secrete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yet, I don't have the credentials or authorization to do it. Could you?

Copy link
Member

Choose a reason for hiding this comment

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

Have we added it to staging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added instructions in the description on how to update the GitHub App once we have a secret defined and set.

Copy link
Member

Choose a reason for hiding this comment

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

But we can't merge it without it, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For another env var added @matthewelwell asked me to do first the PR declaring the env var in the infrastructure folder. And he set the value after that, however, I would say that he has the last word.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that makes sense, but we still can't merge it before the secrets are added because the deployment will fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@matthewelwell have set the env var secret.

"name": "GITHUB_WEBHOOK_SECRET",
"valueFrom": "arn:aws:secretsmanager:eu-west-2:084060095745:secret:ECS-API-LxUiIQ:GITHUB_WEBHOOK_SECRET::"
},
{
"name": "GITHUB_PEM",
"valueFrom": "arn:aws:secretsmanager:eu-west-2:084060095745:secret:GITHUB_PEM-E1Ot8p"
Expand Down
4 changes: 4 additions & 0 deletions infrastructure/aws/staging/ecs-task-definition-web.json
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,10 @@
"name": "SSE_AUTHENTICATION_TOKEN",
"valueFrom": "arn:aws:secretsmanager:eu-west-2:302456015006:secret:ECS-API-heAdoB:SSE_AUTHENTICATION_TOKEN::"
},
{
"name": "GITHUB_WEBHOOK_SECRET",
"valueFrom": "arn:aws:secretsmanager:eu-west-2:302456015006:secret:ECS-API-heAdoB:GITHUB_WEBHOOK_SECRET::"
},
{
"name": "GITHUB_PEM",
"valueFrom": "arn:aws:secretsmanager:eu-west-2:302456015006:secret:GITHUB_PEM-Bfoaql"
Expand Down