fix(api_views): remove throttles and add auth classes to webhook api#7453
Conversation
| authentication_classes = [ReplicaRoutingSessionAuthentication] | ||
| throttle_classes = [] |
There was a problem hiding this comment.
🔴 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 clientNo login, no force_login, no session cookie — just the Token header.
Step-by-step proof that test_make_an_webhook will fail
setUprunsself.client = make_client(self.user_1.pk)→AsyncAPIClientwith onlyHTTP_AUTHORIZATION: Token <token>set.test_make_an_webhookcallsself.make_a_webhook(self.client)→POST /…webhooks-list….- DRF dispatches to
WebhooksViewSet; with this PR,authentication_classes = [ReplicaRoutingSessionAuthentication]is the only class consulted. ReplicaRoutingSessionAuthentication(subclass ofSessionAuthentication) readsrequest._request.userfrom the session middleware. No session cookie is present, sorequest.userresolves toAnonymousUserandauthenticate()returnsNone. TheAuthorization: Token <token>header is never inspected becauseTokenAuthenticationis no longer in the list.IsAuthenticatedrejects → DRF returns 401 Unauthorized.- 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.)
|
@albertisfu this implements a fix to the webhooks issue based on your suggestions in the (now-closed) PR #7340. That PR is blocked for unrelated reasons, so I'm breaking this out as a separate issue. This adds the |
albertisfu
left a comment
There was a problem hiding this comment.
Thanks @nadahlberg this is working as expected. Merging...
Fixes
Fixes #7452
Summary
Disable throttling on the webhook UI viewsets (WebhooksViewSet, WebhookEventViewSet) so visiting the webhooks tab no longer counts against or gets blocked by the user's API rate limit. To offset the removed throttle, restrict these endpoints to session-only authentication (ReplicaRoutingSessionAuthentication) so they can't be hit by token-based clients.
Deployment
This PR should:
skip-deploy(skips everything below)skip-web-deployskip-celery-deployskip-cronjob-deployskip-daemon-deploy