Skip to content

fix(api_views): remove throttles and add auth classes to webhook api #7453

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.

Sign up for GitHub

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

Merged
albertisfu merged 4 commits into from
Jun 18, 2026
Merged

fix(api_views): remove throttles and add auth classes to webhook api #7453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
95d7e89
fix(api_views): remove throttles and add auth classes to webhook endp…
nadahlberg Jun 16, 2026
2573e93
feat(tests): add session client helper for webhook tests
nadahlberg Jun 16, 2026
d86334e
Merge branch 'main' into remove-webhooks-api-from-rate-limits
albertisfu Jun 17, 2026
35003ce
Merge branch 'main' into remove-webhooks-api-from-rate-limits
nadahlberg Jun 18, 2026
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
8 changes: 8 additions & 0 deletions cl/tests/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,14 @@ def make_client(user_pk: int) -> AsyncAPIClient:
return client


def make_session_client(user_pk: int) -> AsyncAPIClient:
# Use for endpoints that only accept session authentication
user = User.objects.get(pk=user_pk)
client = AsyncAPIClient()
client.force_login(user)
return client


def get_with_wait(
wait: WebDriverWait,
locator: tuple[str, str],
Expand Down
5 changes: 5 additions & 0 deletions cl/users/api_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from rest_framework.viewsets import ModelViewSet

from cl.api.api_permissions import IsOwner
from cl.api.authentication import ReplicaRoutingSessionAuthentication
from cl.api.models import (
Webhook,
WebhookEvent,
Expand All @@ -29,6 +30,8 @@ class WebhooksViewSet(ModelViewSet):

permission_classes = [IsAuthenticated, IsOwner]
renderer_classes = [JSONRenderer, TemplateHTMLRenderer]
authentication_classes = [ReplicaRoutingSessionAuthentication]
throttle_classes = []
Comment on lines +33 to +34

@claude claude Bot Jun 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 Restricting these viewsets to [ReplicaRoutingSessionAuthentication] replaces (not extends) DRF's default auth classes, so Token auth is no longer accepted. The existing WebhooksHTMXTests suite in cl/users/tests.py (line 3546) uses make_client() from cl/tests/utils.py:107-113, which authenticates exclusively via the Authorization: Token <token> header and never establishes a Django session — so all ~9 tests in that class (test_make_an_webhook, test_make_an_http_webhook_fails, test_list_users_webhooks, test_delete_webhook, test_webhook_detail, test_webhook_update, test_send_webhook_test, test_send_webhook_test_all_types, test_list_webhook_events) will return 401/403 and CI will break. Fix by migrating those tests to a session-authenticated client (e.g. client.force_login / APIClient.force_authenticate) alongside this PR.

Extended reasoning...

What the bug is

DRF's authentication_classes on a view replaces DEFAULT_AUTHENTICATION_CLASSES rather than appending to them. Before this PR, WebhooksViewSet and WebhookEventViewSet inherited the project defaults from cl/settings/third_party/rest_framework.py (Token, OAuth2, Basic, Session). After this PR they accept only ReplicaRoutingSessionAuthentication, which extends DRF SessionAuthentication (cl/api/authentication.py:100) and only authenticates via the Django session cookie — it ignores Authorization: Token … headers entirely.

The test path that breaks

WebhooksHTMXTests (cl/users/tests.py:3546) builds its clients in setUp:

self.client = make_client(self.user_1.pk)
self.client_2 = make_client(self.user_2.pk)

make_client (cl/tests/utils.py:107-113) is the only authentication step:

def make_client(user_pk: int) -> AsyncAPIClient:
    user = User.objects.get(pk=user_pk)
    token, created = Token.objects.get_or_create(user=user)
    token_header = f"Token {token}"
    client = AsyncAPIClient()
    client.credentials(HTTP_AUTHORIZATION=token_header)
    return client

No login, no force_login, no session cookie — just the Token header.

Step-by-step proof that test_make_an_webhook will fail

  1. setUp runs self.client = make_client(self.user_1.pk) → AsyncAPIClient with only HTTP_AUTHORIZATION: Token <token> set.
  2. test_make_an_webhook calls self.make_a_webhook(self.client) → POST /…webhooks-list….
  3. DRF dispatches to WebhooksViewSet; with this PR, authentication_classes = [ReplicaRoutingSessionAuthentication] is the only class consulted.
  4. ReplicaRoutingSessionAuthentication (subclass of SessionAuthentication) reads request._request.user from the session middleware. No session cookie is present, so request.user resolves to AnonymousUser and authenticate() returns None. The Authorization: Token <token> header is never inspected because TokenAuthentication is no longer in the list.
  5. IsAuthenticated rejects → DRF returns 401 Unauthorized.
  6. The test asserts response.status_code == HTTPStatus.CREATED (201) → AssertionError, CI fails.

Why the existing code doesn't protect against this

The PR diff only touches cl/users/api_views.py; no companion update to cl/users/tests.py and no override of setUp to establish a session. All nine listed tests (test_make_an_webhook, test_make_an_http_webhook_fails, test_list_users_webhooks, test_delete_webhook, test_webhook_detail, test_webhook_update, test_send_webhook_test, test_send_webhook_test_all_types, test_list_webhook_events) follow the same pattern: they assert HTTPStatus.OK / CREATED / NO_CONTENT / NOT_FOUND and will all instead receive 401/403.

Impact and fix

This is a hard CI break on merge — every test in WebhooksHTMXTests flips red. The fix is to switch the test client to session auth. Either replace make_client(user.pk) with an AsyncAPIClient() and call await sync_to_async(client.force_login)(user) in setUp, or use client.force_authenticate(user=user). (The product intent — that these UI viewsets are only callable via the logged-in browser session — is correct; only the tests need updating.)


def get_queryset(self):
"""
Expand Down Expand Up @@ -274,6 +277,8 @@ class WebhookEventViewSet(ModelViewSet):
permission_classes = [IsAuthenticated, IsOwner]
renderer_classes = [TemplateHTMLRenderer]
filterset_class = WebhookEventViewFilter
authentication_classes = [ReplicaRoutingSessionAuthentication]
throttle_classes = []

def get_queryset(self):
"""
Expand Down
6 changes: 3 additions & 3 deletions cl/users/tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@
TestCase,
)
from cl.tests.utils import MockResponse as MockPostResponse
from cl.tests.utils import make_client
from cl.tests.utils import make_session_client
from cl.users.admin import UserAdmin
from cl.users.email_handlers import (
add_bcc_random,
Expand Down Expand Up @@ -3553,8 +3553,8 @@ def setUpTestData(cls):

def setUp(self) -> None:
self.webhook_path = reverse("webhooks-list")
self.client = make_client(self.user_1.pk)
self.client_2 = make_client(self.user_2.pk)
self.client = make_session_client(self.user_1.pk)
self.client_2 = make_session_client(self.user_2.pk)

def tearDown(cls):
Webhook.objects.all().delete()
Expand Down
Loading