From b3a8ccde77f24ec362a09d879c0cfd7e6d3ce63f Mon Sep 17 00:00:00 2001 From: Rechner Fox <659028+rechner@users.noreply.github.com> Date: Fri, 25 Sep 2026 14:20:46 -0700 Subject: [PATCH 1/2] refactor(billing): Refactor Stripe webhook, add tests - Refactor Stripe webhook view to reduce cognitive complexity --- .../api_billing/tests/test_webhook.py | 311 ++++++++ memberportal/api_billing/views.py | 714 ++++++++---------- 2 files changed, 630 insertions(+), 395 deletions(-) create mode 100644 memberportal/api_billing/tests/test_webhook.py diff --git a/memberportal/api_billing/tests/test_webhook.py b/memberportal/api_billing/tests/test_webhook.py new file mode 100644 index 00000000..53f48034 --- /dev/null +++ b/memberportal/api_billing/tests/test_webhook.py @@ -0,0 +1,311 @@ +from types import SimpleNamespace +from unittest.mock import Mock, patch + +from constance.test.unittest import override_config +from django.test import TestCase +from rest_framework import status +from rest_framework.test import APIRequestFactory + +from api_billing import views +from api_billing.models import ProcessedStripeEvent +from profile.models import Profile, User + + +class StripeWebhookTests(TestCase): + webhook_config = { + "STRIPE_WEBHOOK_SECRET": "whsec_test", + "ENABLE_STRIPE_MEMBERSHIP_PAYMENTS": True, + "TERMS_ACCEPTANCE_CARDS": "[]", + "REQUIRE_ACCESS_CARD": False, + "CANVAS_INDUCTION_ENABLED": False, + "MOODLE_INDUCTION_ENABLED": False, + "ENABLE_DOCUSEAL_INTEGRATION": False, + } + + def make_profile( + self, + *, + state="noob", + subscription_status="pending", + customer_id="cus_test", + subscription_id="sub_test", + state_locked=False, + ): + suffix = Profile.objects.count() + 1 + user = User.objects.create_user( + f"stripe-webhook-{suffix}@example.test", + password="test-password", + ) + return Profile.objects.create( + user=user, + first_name="Stripe", + last_name=str(suffix), + state=state, + state_locked=state_locked, + subscription_status=subscription_status, + stripe_customer_id=customer_id, + stripe_subscription_id=subscription_id, + ) + + def make_event(self, event_type="invoice.paid", **data): + return { + "id": data.pop("event_id", "evt_test"), + "type": event_type, + "data": { + "object": { + "customer": "cus_test", + "subscription": "sub_test", + "status": "paid", + **data, + } + }, + } + + def post_event(self, event): + request = APIRequestFactory().post( + "/api/billing/stripe-webhook/", + data=b"signed-payload", + content_type="application/json", + HTTP_STRIPE_SIGNATURE="sig_test", + ) + with patch.object( + views.stripe.Webhook, + "construct_event", + return_value=event, + ): + return views.StripeWebhook.as_view()(request) + + def test_rejects_events_without_a_signing_secret(self): + with override_config(STRIPE_WEBHOOK_SECRET=""): + with patch.object(views.stripe.Webhook, "construct_event") as construct: + response = self.post_event(self.make_event()) + + self.assertEqual(response.status_code, status.HTTP_503_SERVICE_UNAVAILABLE) + self.assertEqual(response.data, {"error": "Webhook signing not configured."}) + construct.assert_not_called() + + def test_reports_signature_validation_failures(self): + error = ValueError("bad signature") + request = APIRequestFactory().post( + "/api/billing/stripe-webhook/", + data=b"invalid-payload", + content_type="application/json", + HTTP_STRIPE_SIGNATURE="sig_test", + ) + with override_config(**self.webhook_config): + with patch.object( + views.stripe.Webhook, + "construct_event", + side_effect=error, + ), patch.object(views, "capture_exception") as capture: + response = views.StripeWebhook.as_view()(request) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual( + response.data, + {"error": "Error validating Stripe signature."}, + ) + capture.assert_called_once_with(error) + + def test_ignores_events_without_a_customer(self): + event = self.make_event(customer=None) + + with override_config(**self.webhook_config): + with patch.object(views.Profile.objects, "get") as profile_get: + response = self.post_event(event) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + profile_get.assert_not_called() + + def test_accepts_new_invoice_subscription_schema_and_deduplicates(self): + self.make_profile() + event = self.make_event( + parent={"subscription_details": {"subscription": "sub_test"}}, + subscription=None, + event_id="evt_nested_subscription", + ) + + with override_config(**self.webhook_config): + with patch.object(views.StripeWebhook, "_handle_event") as handle: + first_response = self.post_event(event) + second_response = self.post_event(event) + + self.assertEqual(first_response.status_code, status.HTTP_200_OK) + self.assertEqual(second_response.status_code, status.HTTP_200_OK) + handle.assert_called_once() + self.assertEqual( + ProcessedStripeEvent.objects.filter( + event_id="evt_nested_subscription" + ).count(), + 1, + ) + + def test_ignores_out_of_scope_invoice_without_claiming_event(self): + self.make_profile() + event = self.make_event( + subscription="sub_other", + event_id="evt_out_of_scope", + ) + + with override_config(**self.webhook_config): + with patch.object(views.StripeWebhook, "_handle_event") as handle: + response = self.post_event(event) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + handle.assert_not_called() + self.assertFalse( + ProcessedStripeEvent.objects.filter(event_id="evt_out_of_scope").exists() + ) + + def test_paid_invoice_activates_member_and_continues_after_email_failure(self): + profile = self.make_profile() + event = self.make_event(event_id="evt_paid_activation") + + with override_config(**self.webhook_config): + with patch.object( + Profile, + "can_signup", + return_value={"success": True}, + ), patch.object( + Profile, "complete_signup" + ) as complete_signup, patch.object( + User, + "log_event", + ), patch.object( + User, + "email_notification", + side_effect=RuntimeError("email unavailable"), + ), patch.object( + views, "send_email_to_admin" + ) as send_admin, patch.object( + views, + "capture_exception", + ) as capture: + with self.captureOnCommitCallbacks(execute=True): + response = self.post_event(event) + + profile.refresh_from_db() + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(profile.subscription_status, "active") + self.assertIsNotNone(profile.subscription_first_created) + complete_signup.assert_called_once() + send_admin.assert_not_called() + capture.assert_called_once() + + def test_paid_invoice_holds_state_locked_member(self): + profile = self.make_profile(state_locked=True) + event = self.make_event(event_id="evt_locked_paid") + + with override_config(**self.webhook_config): + with patch.object(User, "log_event"), patch.object( + views, + "send_email_to_admin", + ) as send_admin: + with self.captureOnCommitCallbacks(execute=True): + response = self.post_event(event) + + profile.refresh_from_db() + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(profile.subscription_status, "pending") + send_admin.assert_called_once() + self.assertIn("locked member", send_admin.call_args.kwargs["subject"]) + + def test_paid_invoice_marks_ineligible_member_active_and_notifies_admin(self): + profile = self.make_profile(state="accountonly") + event = self.make_event(event_id="evt_paid_ineligible") + + with override_config(**self.webhook_config): + with patch.object( + Profile, + "can_signup", + return_value={"success": False}, + ), patch.object(User, "log_event"), patch.object( + User, + "email_notification", + ) as email_notification, patch.object( + views, + "send_email_to_admin", + ) as send_admin: + with self.captureOnCommitCallbacks(execute=True): + response = self.post_event(event) + + profile.refresh_from_db() + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(profile.subscription_status, "active") + email_notification.assert_called_once() + send_admin.assert_called_once_with( + "Action Required: Verify returning member", + template_vars={ + "title": "Action Required: Verify returning member", + "message": ( + "An existing member (or someone who clicked 'skip signup I " + "just want an account') has setup a membership subscription. " + "You must now decide whether to enable their site access." + ), + }, + reply_to=profile.user.email, + ) + + def test_payment_failed_notifies_member_after_commit(self): + profile = self.make_profile() + event = self.make_event( + event_type="invoice.payment_failed", + event_id="evt_payment_failed", + ) + + with override_config(**self.webhook_config): + with patch.object(User, "log_event"), patch.object( + User, + "email_notification", + ) as email_notification: + with self.captureOnCommitCallbacks(execute=True): + response = self.post_event(event) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + email_notification.assert_called_once() + self.assertEqual( + email_notification.call_args.args[0], + "Your membership payment failed", + ) + + def test_subscription_deleted_clears_billing_and_preserves_callback_order(self): + profile = self.make_profile(state="active", subscription_status="active") + event = self.make_event( + event_type="customer.subscription.deleted", + event_id="evt_subscription_deleted", + id="sub_test", + subscription=None, + ) + order = [] + invoices = SimpleNamespace( + auto_paging_iter=Mock(return_value=[SimpleNamespace(id="in_open")]) + ) + + with override_config(**self.webhook_config): + with patch.object( + views.stripe.Invoice, + "list", + side_effect=lambda **kwargs: (order.append("list"), invoices)[1], + ), patch.object( + views.stripe.Invoice, + "void_invoice", + side_effect=lambda invoice_id: order.append("void"), + ), patch.object( + views, + "send_email_to_admin", + side_effect=lambda *args, **kwargs: order.append("admin"), + ), patch.object( + Profile, + "complete_cancel", + autospec=True, + side_effect=lambda self, triggered_by: order.append("cancel"), + ): + with self.captureOnCommitCallbacks(execute=True): + response = self.post_event(event) + + profile.refresh_from_db() + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertIsNone(profile.membership_plan) + self.assertIsNone(profile.stripe_subscription_id) + self.assertEqual(profile.subscription_status, "inactive") + self.assertEqual(order, ["list", "void", "admin", "cancel"]) diff --git a/memberportal/api_billing/views.py b/memberportal/api_billing/views.py index 3a0d128b..d0d2e337 100644 --- a/memberportal/api_billing/views.py +++ b/memberportal/api_billing/views.py @@ -1377,14 +1377,7 @@ class StripeWebhook(StripeAPIView): permission_classes = (permissions.AllowAny,) def post(self, request): - # Fail closed when no signing secret is configured. The endpoint is - # publicly reachable and unauthenticated by design (Stripe can't - # present a session/JWT), so signature verification is the *only* - # gate. Without it, anyone who guesses a stripe_customer_id can - # forge invoice.paid / customer.subscription.deleted events and - # activate or cancel arbitrary members. - webhook_secret = config.STRIPE_WEBHOOK_SECRET - if not webhook_secret: + if not config.STRIPE_WEBHOOK_SECRET: logger.error( "STRIPE_WEBHOOK_SECRET is not configured; rejecting webhook event." ) @@ -1393,422 +1386,353 @@ def post(self, request): status=status.HTTP_503_SERVICE_UNAVAILABLE, ) - signature = request.headers.get("stripe-signature") - try: - event = stripe.Webhook.construct_event( - payload=request.body, sig_header=signature, secret=webhook_secret - ) - except Exception as e: - logger.exception("Error validating Stripe signature.") - capture_exception(e) - return Response({"error": "Error validating Stripe signature."}) + event = self._construct_event(request) + if isinstance(event, Response): + return event - data = event["data"] + data = event["data"]["object"] event_type = event["type"] - - data = data["object"] - - # Some Stripe events (e.g. account-level ones) don't carry a customer - # field — we can't do anything useful with those. customer_id = data.get("customer") if not customer_id: return Response() - try: - member_profile = Profile.objects.get(stripe_customer_id=customer_id) - - except Profile.DoesNotExist: - # Stripe sends events for customers we don't track (e.g. one-off - # charges, deleted profiles). Don't sentry-spam on these — info - # log only, so we still have a trail without paging anyone. - logger.info("Webhook event for unknown stripe_customer_id; ignoring.") + profile = self._get_profile(customer_id) + if profile is None or not self._is_supported_event(event_type): return Response() - - except Profile.MultipleObjectsReturned as e: - # stripe_customer_id is not unique at the DB level on this branch, - # so a fixture import or manual edit can leave duplicates. Bail - # loudly — acting on either profile would corrupt their state. - capture_exception(e) + if not self._is_scoped_event(event_type, data, profile): + return Response() + if not self._claim_event(event): return Response() - # Both invoice events must be scoped to the membership subscription — - # the customer can have unrelated invoices (admin-created one-offs, - # memberbucks-related charges, etc.) and acting on those would falsely - # activate the member or send misleading "membership payment failed" - # emails. The admin "mark paid out-of-band" tool has the same guard. - if event_type in (INVOICE_PAID, INVOICE_PAYMENT_FAILED): - invoice_subscription = data.get("subscription") - if ( - not invoice_subscription - or invoice_subscription != member_profile.stripe_subscription_id - ): - return Response() - - # Scope events to the member's current sub — the customer may - # have unrelated invoices/subs (admin one-offs, memberbucks, - # replayed cancelled subs) we must not act on. Run the scope - # check BEFORE the dedup insert so an out-of-scope event doesn't - # poison its own retries — fix it, redeliver, and processing - # picks up cleanly. - if event_type in (INVOICE_PAID, INVOICE_PAYMENT_FAILED): - invoice_subscription = _invoice_subscription_id(data) - if ( - not invoice_subscription - or invoice_subscription != locked_profile.stripe_subscription_id - ): - return Response() - elif event_type == "customer.subscription.deleted": - if data.get("id") != locked_profile.stripe_subscription_id: - return Response() - - # Idempotency: Stripe retries deliveries for up to ~3 days on non-2xx - # responses or timeouts. Skip any event id we've already processed so - # side effects (emails, SMS, state changes) don't fire twice. - if event_id: - _, created = ProcessedStripeEvent.objects.get_or_create( - event_id=event_id, - defaults={"event_type": event_type}, - ) - if not created: - return Response() - - if event_type == "invoice.paid": - invoice_status = data["status"] - - locked_profile.user.log_event("Membership payment received.", "stripe") - - if ( - invoice_status == "paid" - and not locked_profile.subscription_first_created - ): - locked_profile.subscription_first_created = timezone.now() - locked_profile.save(update_fields=["subscription_first_created"]) - - # A state_locked member is by invariant subscription_status=inactive. - # If an invoice.paid arrives anyway (late/out-of-order delivery, or - # an admin manually marked an old invoice paid in Stripe), preserve - # the lock — do NOT flip subscription_status to "active" and do - # NOT auto-activate. Notify the admin so they can investigate. - if ( - locked_profile.state_locked - and locked_profile.state != "active" - and invoice_status == "paid" - ): - locked_profile.user.log_event( - "Invoice paid for a state_locked member — held; " - "admin must unlock + reconcile.", - "stripe", - ) - - held_full_name = locked_profile.get_full_name() - held_user_email = locked_profile.user.email - - def _on_commit_locked_paid_admin( - full_name=held_full_name, - user_email=held_user_email, - user=locked_profile.user, - ): - admin_subject = ( - f"Action Required: locked member {full_name} " - "had an invoice paid" - ) - admin_message = ( - f"{full_name} ({user_email}) is currently " - "state-locked, but Stripe just reported a paid " - "invoice on their subscription. The portal has " - "NOT activated them. Investigate whether to " - "unlock + activate, or to void the Stripe " - "subscription." - ) - try: - send_email_to_admin( - subject=admin_subject, - template_vars={ - "title": admin_subject, - "message": admin_message, - }, - user=user, - reply_to=user.email, - ) - except Exception as e: - capture_exception(e) - - transaction.on_commit(_on_commit_locked_paid_admin) - - # If they aren't an active member, are allowed to signup, and have paid the invoice - # then lets activate their account (this could be a new OR returning member) - elif ( - locked_profile.state != "active" - and locked_profile.can_signup()["success"] - and invoice_status == "paid" - ): - locked_profile.subscription_status = "active" - locked_profile.save(update_fields=["subscription_status"]) - - locked_profile.user.log_event( - "Activated membership because member met all requirements.", - "stripe", - ) + self._handle_event(event_type, data, profile) + return Response() - # Both callbacks deferred to on_commit so the I/O can't - # extend the row lock past Stripe's 30s webhook timeout. - # The paid-confirmation email is registered first so it - # arrives before activate()'s welcome email — the body - # references "another email message confirming this was - # successful" which is the welcome that follows. - paid_subject = "Your payment was successful." - paid_message = ( - "Thanks for making a membership payment using our " - "online payment system. You've already met all of " - "the requirements for activating your site access. " - "Please check for another email message confirming " - "this was successful." - ) + def _construct_event(self, request): + try: + return stripe.Webhook.construct_event( + payload=request.body, + sig_header=request.headers.get("stripe-signature"), + secret=config.STRIPE_WEBHOOK_SECRET, + ) + except Exception as error: + logger.exception("Error validating Stripe signature.") + capture_exception(error) + return Response({"error": "Error validating Stripe signature."}) - def _on_commit_paid_email( - user=locked_profile.user, - subject=paid_subject, - message=paid_message, - ): - try: - user.email_notification(subject, message) - user.log_event( - "Payment-received email sent.", - "email", - ) - except Exception as e: - capture_exception(e) + def _get_profile(self, customer_id): + try: + return Profile.objects.get(stripe_customer_id=customer_id) + except Profile.DoesNotExist: + logger.info("Webhook event for unknown stripe_customer_id; ignoring.") + return None + except Profile.MultipleObjectsReturned as error: + capture_exception(error) + return None + + @staticmethod + def _is_supported_event(event_type): + return event_type in { + INVOICE_PAID, + INVOICE_PAYMENT_FAILED, + "customer.subscription.deleted", + } - transaction.on_commit(_on_commit_paid_email) + @staticmethod + def _is_scoped_event(event_type, data, profile): + if event_type in (INVOICE_PAID, INVOICE_PAYMENT_FAILED): + subscription_id = _invoice_subscription_id(data) + return subscription_id and subscription_id == profile.stripe_subscription_id + return data.get("id") == profile.stripe_subscription_id + + @staticmethod + def _claim_event(event): + event_id = event.get("id") + if not event_id: + return True + + _, created = ProcessedStripeEvent.objects.get_or_create( + event_id=event_id, + defaults={"event_type": event["type"]}, + ) + return created - def _on_commit_paid_activate(profile=locked_profile): - try: - profile.complete_signup(SignupTriggeredBy.INVOICE_PAID) - except Exception as e: - capture_exception(e) + def _handle_event(self, event_type, data, profile): + handlers = { + INVOICE_PAID: self._handle_invoice_paid, + INVOICE_PAYMENT_FAILED: self._handle_invoice_payment_failed, + "customer.subscription.deleted": self._handle_subscription_deleted, + } + handlers[event_type](data, profile) + + def _handle_invoice_paid(self, data, profile): + invoice_status = data["status"] + profile.user.log_event("Membership payment received.", "stripe") + self._record_first_paid_invoice(profile, invoice_status) + + if self._is_locked_paid_invoice(profile, invoice_status): + self._hold_locked_paid_invoice(profile) + elif self._can_activate_paid_invoice(profile, invoice_status): + self._activate_paid_invoice(profile) + elif self._needs_paid_invoice_steps(profile, invoice_status): + self._mark_paid_invoice_active(profile) + + @staticmethod + def _record_first_paid_invoice(profile, invoice_status): + if invoice_status == "paid" and not profile.subscription_first_created: + profile.subscription_first_created = timezone.now() + profile.save(update_fields=["subscription_first_created"]) + + @staticmethod + def _is_locked_paid_invoice(profile, invoice_status): + return ( + profile.state_locked + and profile.state != "active" + and invoice_status == "paid" + ) - transaction.on_commit(_on_commit_paid_activate) + def _hold_locked_paid_invoice(self, profile): + profile.user.log_event( + "Invoice paid for a state_locked member — held; " + "admin must unlock + reconcile.", + "stripe", + ) + full_name = profile.get_full_name() + user_email = profile.user.email + transaction.on_commit( + lambda: self._notify_locked_paid_invoice( + profile.user, full_name, user_email + ) + ) - # If they aren't an active member, are NOT allowed to signup, and have paid the invoice - # then we need to let them know and mark the subscription as active - # (this could be a new OR returning member that's been too long since induction etc.) - elif locked_profile.state != "active" and invoice_status == "paid": - locked_profile.subscription_status = "active" - locked_profile.save(update_fields=["subscription_status"]) + @staticmethod + def _notify_locked_paid_invoice(user, full_name, user_email): + subject = f"Action Required: locked member {full_name} had an invoice paid" + message = ( + f"{full_name} ({user_email}) is currently state-locked, but Stripe " + "just reported a paid invoice on their subscription. The portal " + "has NOT activated them. Investigate whether to unlock + activate, " + "or to void the Stripe subscription." + ) + try: + send_email_to_admin( + subject=subject, + template_vars={"title": subject, "message": message}, + user=user, + reply_to=user.email, + ) + except Exception as error: + capture_exception(error) - locked_profile.user.log_event( - "Did not activate membership because member did not meet all requirements.", - "stripe", - ) + @staticmethod + def _can_activate_paid_invoice(profile, invoice_status): + return ( + profile.state != "active" + and profile.can_signup()["success"] + and invoice_status == "paid" + ) - paid_subject = "Your payment was received — additional steps needed" - paid_message = ( - "Thanks for making a membership payment using our " - "online payment system. Your access isn't enabled yet " - "because you still need to complete your induction. " - f"Please log in to {config.SITE_URL} and finish the " - "induction step to activate your membership." - ) - # Capture at decision time — state may shift before on_commit fires. - notify_admin = locked_profile.state != "noob" - - def _on_commit_paid_no_activate( - profile=locked_profile, - subject=paid_subject, - message=paid_message, - notify_admin=notify_admin, - ): - # See _on_commit_paid_activate for why each call - # is wrapped independently. - try: - profile.user.email_notification(subject, message) - except Exception as e: - capture_exception(e) - if notify_admin: - admin_subject = "Action Required: Verify returning member" - admin_message = ( - "An existing member (or someone who clicked 'skip signup I just want an account') " - "has setup a membership subscription. You must now decide whether to enable their site access." - ) - try: - send_email_to_admin( - admin_subject, - template_vars={ - "title": admin_subject, - "message": admin_message, - }, - reply_to=profile.user.email, - ) - except Exception as e: - capture_exception(e) + def _activate_paid_invoice(self, profile): + profile.subscription_status = "active" + profile.save(update_fields=["subscription_status"]) + profile.user.log_event( + "Activated membership because member met all requirements.", + "stripe", + ) + subject = "Your payment was successful." + message = ( + "Thanks for making a membership payment using our online payment " + "system. You've already met all of the requirements for activating " + "your site access. Please check for another email message confirming " + "this was successful." + ) + transaction.on_commit( + lambda: self._send_paid_confirmation(profile.user, subject, message) + ) + transaction.on_commit(lambda: self._complete_paid_signup(profile)) - transaction.on_commit(_on_commit_paid_no_activate) + @staticmethod + def _send_paid_confirmation(user, subject, message): + try: + user.email_notification(subject, message) + user.log_event("Payment-received email sent.", "email") + except Exception as error: + capture_exception(error) - # in all other instances, we don't care about a paid invoice and can ignore it + @staticmethod + def _complete_paid_signup(profile): + try: + profile.complete_signup(SignupTriggeredBy.INVOICE_PAID) + except Exception as error: + capture_exception(error) - if event_type == "invoice.payment_failed": - locked_profile.user.log_event("Membership payment failed", "stripe") + @staticmethod + def _needs_paid_invoice_steps(profile, invoice_status): + return profile.state != "active" and invoice_status == "paid" - failed_subject = "Your membership payment failed" - failed_message = ( - "Hi there, we tried to collect your membership payment but " - "weren't successful. Please update your billing method or contact " - "us if you need more time. We'll try again a few times, but if we're unable to " - "collect your payment soon, your membership may be cancelled." - ) - - def _on_commit_payment_failed( - profile=locked_profile, - subject=failed_subject, - message=failed_message, - ): - try: - profile.user.email_notification(subject, message) - except Exception as e: - capture_exception(e) + def _mark_paid_invoice_active(self, profile): + profile.subscription_status = "active" + profile.save(update_fields=["subscription_status"]) + profile.user.log_event( + "Did not activate membership because member did not meet all requirements.", + "stripe", + ) + subject = "Your payment was received — additional steps needed" + message = ( + "Thanks for making a membership payment using our online payment " + "system. Your access isn't enabled yet because you still need to " + "complete your induction. Please log in to " + f"{config.SITE_URL} and finish the induction step to activate your " + "membership." + ) + notify_admin = profile.state != "noob" + transaction.on_commit( + lambda: self._send_paid_steps_notification( + profile, subject, message, notify_admin + ) + ) - transaction.on_commit(_on_commit_payment_failed) + @staticmethod + def _send_paid_steps_notification(profile, subject, message, notify_admin): + try: + profile.user.email_notification(subject, message) + except Exception as error: + capture_exception(error) - if event_type == "customer.subscription.deleted": - deleted_subscription_id = data["id"] - full_name = locked_profile.get_full_name() + if not notify_admin: + return - locked_profile.membership_plan = None - locked_profile.stripe_subscription_id = None - locked_profile.subscription_status = "inactive" - locked_profile.save( - update_fields=[ - "membership_plan", - "stripe_subscription_id", - "subscription_status", - ] - ) + admin_subject = "Action Required: Verify returning member" + admin_message = ( + "An existing member (or someone who clicked 'skip signup I just want " + "an account') has setup a membership subscription. You must now " + "decide whether to enable their site access." + ) + try: + send_email_to_admin( + admin_subject, + template_vars={"title": admin_subject, "message": admin_message}, + reply_to=profile.user.email, + ) + except Exception as error: + capture_exception(error) - # Void open invoices — Stripe doesn't auto-void on cancel. - # On on_commit so the Stripe call can't extend the row lock. - # If voiding fails, the deleted subscription's open invoices - # may still be visible to the customer in Stripe — email - # admin so they can void manually. - def _on_commit_void_open_invoices( - subscription_id=deleted_subscription_id, - user=locked_profile.user, - full_name=full_name, - ): - try: - open_invoices = stripe.Invoice.list( - subscription=subscription_id, status="open" - ) - for invoice in open_invoices.auto_paging_iter(): - try: - stripe.Invoice.void_invoice(invoice.id) - except stripe.error.StripeError as e: - capture_exception(e) - user.log_event( - f"Failed to void open invoice " - f"{invoice.id} after subscription cancel.", - "stripe", - str(e), - ) - failure_subject = ( - f"Action Required: void Stripe invoice " - f"{invoice.id} for {full_name}" - ) - failure_message = ( - f"The Stripe subscription " - f"{subscription_id} for {full_name} " - "was cancelled, but voiding open " - f"invoice {invoice.id} failed. Please " - "void it manually in Stripe so the " - "customer isn't shown an unpaid " - "invoice." - ) - try: - send_email_to_admin( - subject=failure_subject, - template_vars={ - "title": failure_subject, - "message": failure_message, - }, - user=user, - reply_to=user.email, - ) - except Exception as email_err: - capture_exception(email_err) - except stripe.error.StripeError as e: - # Couldn't even list invoices — don't know which - # are open, so ask admin to audit the cancelled - # sub. - capture_exception(e) - user.log_event( - f"Failed to list open invoices for cancelled " - f"subscription {subscription_id}; admin must " - "audit Stripe manually.", - "stripe", - str(e), - ) - failure_subject = ( - f"Action Required: audit cancelled Stripe " - f"subscription {subscription_id} for {full_name}" - ) - failure_message = ( - f"The Stripe subscription {subscription_id} " - f"for {full_name} was cancelled, but we " - "couldn't list its open invoices to void " - "them. Please check Stripe and void any " - "open invoices manually." - ) - try: - send_email_to_admin( - subject=failure_subject, - template_vars={ - "title": failure_subject, - "message": failure_message, - }, - user=user, - reply_to=user.email, - ) - except Exception as email_err: - capture_exception(email_err) + @staticmethod + def _handle_invoice_payment_failed(data, profile): + profile.user.log_event("Membership payment failed", "stripe") + subject = "Your membership payment failed" + message = ( + "Hi there, we tried to collect your membership payment but weren't " + "successful. Please update your billing method or contact us if you " + "need more time. We'll try again a few times, but if we're unable to " + "collect your payment soon, your membership may be cancelled." + ) + transaction.on_commit( + lambda: StripeWebhook._send_payment_failed_notification( + profile, subject, message + ) + ) - transaction.on_commit(_on_commit_void_open_invoices) + @staticmethod + def _send_payment_failed_notification(profile, subject, message): + try: + profile.user.email_notification(subject, message) + except Exception as error: + capture_exception(error) - # Notify the operator that this member's Stripe sub ended out - # of band. Stripe-specific messaging stays here, not in - # complete_cancel. Registered before the complete_cancel - # callback so it lands before the member-facing access- - # disabled email that deactivate() sends. - admin_cancel_subject = ( - f"The membership for {full_name} was just cancelled" - ) - admin_cancel_message = ( - f"The Stripe subscription for {full_name} ended, so " - "their membership has been cancelled. Their site " - "access has been turned off." - ) + def _handle_subscription_deleted(self, data, profile): + subscription_id = data["id"] + full_name = profile.get_full_name() + profile.membership_plan = None + profile.stripe_subscription_id = None + profile.subscription_status = "inactive" + profile.save( + update_fields=[ + "membership_plan", + "stripe_subscription_id", + "subscription_status", + ] + ) + transaction.on_commit( + lambda: self._void_open_invoices(subscription_id, profile.user, full_name) + ) + subject = f"The membership for {full_name} was just cancelled" + message = ( + f"The Stripe subscription for {full_name} ended, so their membership " + "has been cancelled. Their site access has been turned off." + ) + transaction.on_commit( + lambda: self._send_cancellation_admin_notification( + profile.user, subject, message + ) + ) + transaction.on_commit(lambda: self._complete_subscription_cancel(profile)) - def _on_commit_admin_cancel_email( - user=locked_profile.user, - subject=admin_cancel_subject, - message=admin_cancel_message, - ): - try: - send_email_to_admin( - subject=subject, - template_vars={"title": subject, "message": message}, - user=user, - reply_to=user.email, - ) - except Exception as e: - capture_exception(e) + @staticmethod + def _void_open_invoices(subscription_id, user, full_name): + try: + invoices = stripe.Invoice.list(subscription=subscription_id, status="open") + for invoice in invoices.auto_paging_iter(): + StripeWebhook._void_invoice(invoice, subscription_id, user, full_name) + except stripe.error.StripeError as error: + capture_exception(error) + user.log_event( + f"Failed to list open invoices for cancelled subscription " + f"{subscription_id}; admin must audit Stripe manually.", + "stripe", + str(error), + ) + subject = ( + f"Action Required: audit cancelled Stripe subscription " + f"{subscription_id} for {full_name}" + ) + message = ( + f"The Stripe subscription {subscription_id} for {full_name} was " + "cancelled, but we couldn't list its open invoices to void them. " + "Please check Stripe and void any open invoices manually." + ) + StripeWebhook._send_admin_notification(user, subject, message) - transaction.on_commit(_on_commit_admin_cancel_email) + @staticmethod + def _void_invoice(invoice, subscription_id, user, full_name): + try: + stripe.Invoice.void_invoice(invoice.id) + except stripe.error.StripeError as error: + capture_exception(error) + user.log_event( + f"Failed to void open invoice {invoice.id} after subscription cancel.", + "stripe", + str(error), + ) + subject = ( + f"Action Required: void Stripe invoice {invoice.id} for {full_name}" + ) + message = ( + f"The Stripe subscription {subscription_id} for {full_name} was " + f"cancelled, but voiding open invoice {invoice.id} failed. Please " + "void it manually in Stripe so the customer isn't shown an unpaid " + "invoice." + ) + StripeWebhook._send_admin_notification(user, subject, message) - def _on_commit_complete_cancel(profile=locked_profile): - try: - profile.complete_cancel(CancelTriggeredBy.SUBSCRIPTION_DELETED) - except Exception as e: - capture_exception(e) + @staticmethod + def _send_admin_notification(user, subject, message): + try: + send_email_to_admin( + subject=subject, + template_vars={"title": subject, "message": message}, + user=user, + reply_to=user.email, + ) + except Exception as error: + capture_exception(error) - transaction.on_commit(_on_commit_complete_cancel) + @staticmethod + def _send_cancellation_admin_notification(user, subject, message): + StripeWebhook._send_admin_notification(user, subject, message) - return Response() + @staticmethod + def _complete_subscription_cancel(profile): + try: + profile.complete_cancel(CancelTriggeredBy.SUBSCRIPTION_DELETED) + except Exception as error: + capture_exception(error) From ec3a4111440a6afcf6bd54c0fd8348021ac9fafe Mon Sep 17 00:00:00 2001 From: Rechner Fox <659028+rechner@users.noreply.github.com> Date: Fri, 25 Sep 2026 17:48:06 -0700 Subject: [PATCH 2/2] More specific signature verification error handling, more test coverage --- memberportal/api_billing/tests/__init__.py | 0 .../api_billing/tests/test_webhook.py | 305 +++++++++++++++++- memberportal/api_billing/views.py | 10 +- 3 files changed, 311 insertions(+), 4 deletions(-) create mode 100644 memberportal/api_billing/tests/__init__.py diff --git a/memberportal/api_billing/tests/__init__.py b/memberportal/api_billing/tests/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/memberportal/api_billing/tests/test_webhook.py b/memberportal/api_billing/tests/test_webhook.py index 53f48034..43418096 100644 --- a/memberportal/api_billing/tests/test_webhook.py +++ b/memberportal/api_billing/tests/test_webhook.py @@ -100,7 +100,7 @@ def test_reports_signature_validation_failures(self): ), patch.object(views, "capture_exception") as capture: response = views.StripeWebhook.as_view()(request) - self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) self.assertEqual( response.data, {"error": "Error validating Stripe signature."}, @@ -309,3 +309,306 @@ def test_subscription_deleted_clears_billing_and_preserves_callback_order(self): self.assertIsNone(profile.stripe_subscription_id) self.assertEqual(profile.subscription_status, "inactive") self.assertEqual(order, ["list", "void", "admin", "cancel"]) + + def test_ignores_unknown_customer_without_claiming_event(self): + event = self.make_event(event_id="evt_unknown_customer") + + with override_config(**self.webhook_config): + with patch.object( + views.Profile.objects, + "get", + side_effect=Profile.DoesNotExist, + ), patch.object(views.StripeWebhook, "_claim_event") as claim, patch.object( + views.StripeWebhook, + "_handle_event", + ) as handle: + response = self.post_event(event) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + claim.assert_not_called() + handle.assert_not_called() + self.assertFalse( + ProcessedStripeEvent.objects.filter( + event_id="evt_unknown_customer" + ).exists() + ) + + def test_reports_duplicate_customer_without_claiming_event(self): + event = self.make_event(event_id="evt_duplicate_customer") + error = Profile.MultipleObjectsReturned() + + with override_config(**self.webhook_config): + with patch.object( + views.Profile.objects, + "get", + side_effect=error, + ), patch.object(views, "capture_exception") as capture, patch.object( + views.StripeWebhook, + "_claim_event", + ) as claim, patch.object( + views.StripeWebhook, "_handle_event" + ) as handle: + response = self.post_event(event) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + capture.assert_called_once_with(error) + claim.assert_not_called() + handle.assert_not_called() + self.assertFalse( + ProcessedStripeEvent.objects.filter( + event_id="evt_duplicate_customer" + ).exists() + ) + + def test_ignores_unsupported_event_without_claiming_it(self): + self.make_profile() + event = self.make_event( + event_type="customer.updated", + event_id="evt_unsupported", + ) + + with override_config(**self.webhook_config): + with patch.object( + views.StripeWebhook, "_claim_event" + ) as claim, patch.object( + views.StripeWebhook, + "_handle_event", + ) as handle: + response = self.post_event(event) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + claim.assert_not_called() + handle.assert_not_called() + self.assertFalse( + ProcessedStripeEvent.objects.filter(event_id="evt_unsupported").exists() + ) + + def test_explicit_null_new_schema_subscription_does_not_fall_back_to_legacy(self): + self.make_profile() + event = self.make_event( + event_id="evt_explicit_null_subscription", + parent={"subscription_details": {"subscription": None}}, + ) + + with override_config(**self.webhook_config): + with patch.object(views.StripeWebhook, "_handle_event") as handle: + response = self.post_event(event) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + handle.assert_not_called() + self.assertFalse( + ProcessedStripeEvent.objects.filter( + event_id="evt_explicit_null_subscription" + ).exists() + ) + + def test_ignores_deleted_subscription_for_a_different_membership(self): + self.make_profile() + event = self.make_event( + event_type="customer.subscription.deleted", + event_id="evt_other_deleted_subscription", + id="sub_other", + subscription=None, + ) + + with override_config(**self.webhook_config): + with patch.object(views.StripeWebhook, "_handle_event") as handle: + response = self.post_event(event) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + handle.assert_not_called() + self.assertFalse( + ProcessedStripeEvent.objects.filter( + event_id="evt_other_deleted_subscription" + ).exists() + ) + + def test_event_without_an_id_is_processed_for_every_delivery(self): + self.make_profile() + event = self.make_event() + event.pop("id") + + with override_config(**self.webhook_config): + with patch.object(views.StripeWebhook, "_handle_event") as handle: + first_response = self.post_event(event) + second_response = self.post_event(event) + + self.assertEqual(first_response.status_code, status.HTTP_200_OK) + self.assertEqual(second_response.status_code, status.HTTP_200_OK) + self.assertEqual(handle.call_count, 2) + self.assertEqual(ProcessedStripeEvent.objects.count(), 0) + + def test_non_paid_invoice_does_not_change_membership_or_schedule_callbacks(self): + profile = self.make_profile() + event = self.make_event(event_id="evt_not_paid", status="open") + + with override_config(**self.webhook_config): + with patch.object(Profile, "can_signup") as can_signup, patch.object( + User, + "log_event", + ) as log_event, patch.object( + User, + "email_notification", + ) as email_notification, patch.object( + views, + "send_email_to_admin", + ) as send_admin, patch.object( + Profile, "complete_signup" + ) as complete_signup: + with self.captureOnCommitCallbacks(execute=True) as callbacks: + response = self.post_event(event) + + profile.refresh_from_db() + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(profile.subscription_status, "pending") + self.assertIsNone(profile.subscription_first_created) + self.assertEqual(callbacks, []) + can_signup.assert_called_once() + email_notification.assert_not_called() + send_admin.assert_not_called() + complete_signup.assert_not_called() + log_event.assert_called_once_with("Membership payment received.", "stripe") + + def test_ineligible_noob_member_receives_steps_email_without_admin_notice(self): + profile = self.make_profile() + event = self.make_event(event_id="evt_noob_steps") + + with override_config(**self.webhook_config): + with patch.object( + Profile, + "can_signup", + return_value={"success": False}, + ), patch.object(User, "log_event"), patch.object( + User, + "email_notification", + ) as email_notification, patch.object( + views, + "send_email_to_admin", + ) as send_admin: + with self.captureOnCommitCallbacks(execute=True): + response = self.post_event(event) + + profile.refresh_from_db() + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(profile.subscription_status, "active") + email_notification.assert_called_once() + send_admin.assert_not_called() + + def test_failed_payment_email_error_is_captured(self): + self.make_profile() + event = self.make_event( + event_type="invoice.payment_failed", + event_id="evt_failed_payment_email_error", + ) + error = RuntimeError("email unavailable") + + with override_config(**self.webhook_config): + with patch.object(User, "log_event"), patch.object( + User, + "email_notification", + side_effect=error, + ), patch.object(views, "capture_exception") as capture: + with self.captureOnCommitCallbacks(execute=True): + response = self.post_event(event) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + capture.assert_called_once_with(error) + + def test_subscription_deleted_reports_invoice_list_failure_and_still_cancels(self): + profile = self.make_profile(state="active", subscription_status="active") + event = self.make_event( + event_type="customer.subscription.deleted", + event_id="evt_invoice_list_failure", + id="sub_test", + subscription=None, + ) + error = views.stripe.error.APIError("invoice list unavailable") + + with override_config(**self.webhook_config): + with patch.object( + views.stripe.Invoice, + "list", + side_effect=error, + ), patch.object( + views.stripe.Invoice, + "void_invoice", + ) as void_invoice, patch.object( + User, "log_event" + ) as log_event, patch.object( + views, + "send_email_to_admin", + ) as send_admin, patch.object( + Profile, + "complete_cancel", + ) as complete_cancel, patch.object( + views, + "capture_exception", + ) as capture: + with self.captureOnCommitCallbacks(execute=True): + response = self.post_event(event) + + profile.refresh_from_db() + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(profile.subscription_status, "inactive") + capture.assert_called_once_with(error) + void_invoice.assert_not_called() + self.assertEqual(send_admin.call_count, 2) + self.assertIn( + "audit cancelled Stripe subscription", + send_admin.call_args_list[0].kwargs["subject"], + ) + log_event.assert_called_once() + complete_cancel.assert_called_once() + + def test_subscription_deleted_continues_after_one_invoice_void_failure(self): + profile = self.make_profile(state="active", subscription_status="active") + event = self.make_event( + event_type="customer.subscription.deleted", + event_id="evt_invoice_void_failure", + id="sub_test", + subscription=None, + ) + failed_invoice = SimpleNamespace(id="in_failed") + succeeding_invoice = SimpleNamespace(id="in_succeeds") + invoices = SimpleNamespace( + auto_paging_iter=Mock(return_value=[failed_invoice, succeeding_invoice]) + ) + error = views.stripe.error.APIError("invoice void unavailable") + + def void_invoice(invoice_id): + if invoice_id == failed_invoice.id: + raise error + + with override_config(**self.webhook_config): + with patch.object( + views.stripe.Invoice, + "list", + return_value=invoices, + ), patch.object( + views.stripe.Invoice, + "void_invoice", + side_effect=void_invoice, + ) as void, patch.object( + User, "log_event" + ) as log_event, patch.object( + views, + "send_email_to_admin", + ) as send_admin, patch.object( + Profile, "complete_cancel" + ), patch.object( + views, + "capture_exception", + ) as capture: + with self.captureOnCommitCallbacks(execute=True): + response = self.post_event(event) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(void.call_args_list[0].args, ("in_failed",)) + self.assertEqual(void.call_args_list[1].args, ("in_succeeds",)) + capture.assert_called_once_with(error) + log_event.assert_called_once() + self.assertEqual(send_admin.call_count, 2) + self.assertIn( + "void Stripe invoice in_failed", + send_admin.call_args_list[0].kwargs["subject"], + ) diff --git a/memberportal/api_billing/views.py b/memberportal/api_billing/views.py index d0d2e337..6b795a89 100644 --- a/memberportal/api_billing/views.py +++ b/memberportal/api_billing/views.py @@ -18,7 +18,9 @@ from rest_framework.response import Response from rest_framework.views import APIView + import stripe +from stripe import SignatureVerificationError import logging import uuid from enum import Enum @@ -1414,10 +1416,12 @@ def _construct_event(self, request): sig_header=request.headers.get("stripe-signature"), secret=config.STRIPE_WEBHOOK_SECRET, ) - except Exception as error: - logger.exception("Error validating Stripe signature.") + except (ValueError, SignatureVerificationError) as error: capture_exception(error) - return Response({"error": "Error validating Stripe signature."}) + return Response( + {"error": "Error validating Stripe signature."}, + status=status.HTTP_400_BAD_REQUEST, + ) def _get_profile(self, customer_id): try: