orgs: stripe data hygiene (pypi#17380)

* ensure we don't try to process checkout events unrelated to us * create _new_ subscription when existing subscriptions are cancelled Cancelled is a terminal state, we cannot "return" to re-enable a cancelled subscription. Must create a new one * translations * lint * implement Organization.manageable_subscription This is distinct from "active_subscription" as it determines if a user can self-service updating the subscription payment. Basically all states excepted Cancelled can be self-serviced.
miketheman · Jan 10, 2025 · e951ec1 · e951ec1
1 parent dedcee4
commit e951ec1
Show file tree

Hide file tree

Showing 11 changed files with 157 additions and 20 deletions.
diff --git a/dev/environment b/dev/environment
@@ -20,8 +20,8 @@ SESSION_SECRET="an insecure development secret"
 # Use Stripe billing service with test keys for development.
 # See Stripe Dashboard for keys. https://dashboard.stripe.com/apikeys
 # See Stripe Docs for more information. https://stripe.com/docs/keys
-BILLING_BACKEND=warehouse.subscriptions.services.MockStripeBillingService api_base=http://stripe:12111 api_version=2020-08-27
-# BILLING_BACKEND=warehouse.subscriptions.services.StripeBillingService api_version=2020-08-27 publishable_key=pk_test_123 secret_key=sk_test_123 webhook_key=whsec_123
+BILLING_BACKEND=warehouse.subscriptions.services.MockStripeBillingService api_base=http://stripe:12111 api_version=2020-08-27 domain=localhost
+# BILLING_BACKEND=warehouse.subscriptions.services.StripeBillingService api_version=2020-08-27 publishable_key=pk_test_123 secret_key=sk_test_123 webhook_key=whsec_123 domain=localhost
 
 CAMO_URL={request.scheme}://{request.domain}:9000/
 CAMO_KEY=insecurecamokey

diff --git a/tests/conftest.py b/tests/conftest.py
@@ -590,6 +590,7 @@ def billing_service(app_config):
         api=stripe,
         publishable_key="pk_test_123",
         webhook_secret="whsec_123",
+        domain="localhost",
     )
 
 

diff --git a/tests/unit/api/test_billing.py b/tests/unit/api/test_billing.py
@@ -30,6 +30,59 @@
 
 class TestHandleBillingWebhookEvent:
     # checkout.session.completed
+    def test_handle_billing_webhook_event_checkout_complete_not_us(
+        self, db_request, subscription_service, monkeypatch, billing_service
+    ):
+        organization = OrganizationFactory.create()
+        stripe_customer = StripeCustomerFactory.create()
+        OrganizationStripeCustomerFactory.create(
+            organization=organization, customer=stripe_customer
+        )
+        subscription = StripeSubscriptionFactory.create(customer=stripe_customer)
+        OrganizationStripeSubscriptionFactory.create(
+            organization=organization, subscription=subscription
+        )
+
+        event = {
+            "type": "checkout.session.completed",
+            "data": {
+                "object": {
+                    "id": "cs_test_12345",
+                    "customer": stripe_customer.customer_id,
+                    "status": "complete",
+                    "subscription": subscription.subscription_id,
+                    # Missing expected metadata tags (billing_service, domain)
+                    "metadata": {},
+                },
+            },
+        }
+
+        checkout_session = {
+            "id": "cs_test_12345",
+            "customer": {
+                "id": stripe_customer.customer_id,
+                "email": "[email protected]",
+            },
+            "status": "complete",
+            "subscription": {
+                "id": subscription.subscription_id,
+                "items": {
+                    "data": [{"id": "si_12345"}],
+                },
+            },
+        }
+
+        get_checkout_session = pretend.call_recorder(lambda *a, **kw: checkout_session)
+        monkeypatch.setattr(
+            billing_service, "get_checkout_session", get_checkout_session
+        )
+
+        billing.handle_billing_webhook_event(db_request, event)
+
+        assert (
+            get_checkout_session.calls == []
+        )  # Should have stopped immediately before this call
+
     def test_handle_billing_webhook_event_checkout_complete_update(
         self, db_request, subscription_service, monkeypatch, billing_service
     ):
@@ -51,6 +104,10 @@ def test_handle_billing_webhook_event_checkout_complete_update(
                     "customer": stripe_customer.customer_id,
                     "status": "complete",
                     "subscription": subscription.subscription_id,
+                    "metadata": {
+                        "billing_service": "pypi",
+                        "domain": "localhost",
+                    },
                 },
             },
         }
@@ -94,6 +151,10 @@ def test_handle_billing_webhook_event_checkout_complete_add(
                     "customer": stripe_customer.customer_id,
                     "status": "complete",
                     "subscription": "sub_12345",
+                    "metadata": {
+                        "billing_service": "pypi",
+                        "domain": "localhost",
+                    },
                 },
             },
         }
@@ -131,6 +192,10 @@ def test_handle_billing_webhook_event_checkout_complete_invalid_status(
                     "customer": "cus_1234",
                     "status": "invalid_status",
                     "subscription": "sub_12345",
+                    "metadata": {
+                        "billing_service": "pypi",
+                        "domain": "localhost",
+                    },
                 },
             },
         }
@@ -149,6 +214,10 @@ def test_handle_billing_webhook_event_checkout_complete_invalid_customer(
                     "customer": "",
                     "status": "complete",
                     "subscription": "sub_12345",
+                    "metadata": {
+                        "billing_service": "pypi",
+                        "domain": "localhost",
+                    },
                 },
             },
         }
@@ -187,6 +256,10 @@ def test_handle_billing_webhook_event_checkout_complete_invalid_subscription(
                     "customer": "cus_1234",
                     "status": "complete",
                     "subscription": "",
+                    "metadata": {
+                        "billing_service": "pypi",
+                        "domain": "localhost",
+                    },
                 },
             },
         }

diff --git a/tests/unit/organizations/test_models.py b/tests/unit/organizations/test_models.py
@@ -445,8 +445,38 @@ def test_active_subscription(self, db_session):
             organization=organization, subscription=subscription
         )
         assert organization.active_subscription is not None
+        assert organization.manageable_subscription is not None
 
     def test_active_subscription_none(self, db_session):
+        organization = DBOrganizationFactory.create()
+        stripe_customer = DBStripeCustomerFactory.create()
+        DBOrganizationStripeCustomerFactory.create(
+            organization=organization, customer=stripe_customer
+        )
+        subscription = DBStripeSubscriptionFactory.create(
+            customer=stripe_customer,
+            status="unpaid",
+        )
+        DBOrganizationStripeSubscriptionFactory.create(
+            organization=organization, subscription=subscription
+        )
+        assert organization.active_subscription is None
+        assert organization.manageable_subscription is not None
+
+    def test_manageable_subscription(self, db_session):
+        organization = DBOrganizationFactory.create()
+        stripe_customer = DBStripeCustomerFactory.create()
+        DBOrganizationStripeCustomerFactory.create(
+            organization=organization, customer=stripe_customer
+        )
+        subscription = DBStripeSubscriptionFactory.create(customer=stripe_customer)
+        DBOrganizationStripeSubscriptionFactory.create(
+            organization=organization, subscription=subscription
+        )
+        assert organization.active_subscription is not None
+        assert organization.manageable_subscription is not None
+
+    def test_manageable_subscription_none(self, db_session):
         organization = DBOrganizationFactory.create()
         stripe_customer = DBStripeCustomerFactory.create()
         DBOrganizationStripeCustomerFactory.create(
@@ -460,3 +490,4 @@ def test_active_subscription_none(self, db_session):
             organization=organization, subscription=subscription
         )
         assert organization.active_subscription is None
+        assert organization.manageable_subscription is None
diff --git a/tests/unit/subscriptions/test_services.py b/tests/unit/subscriptions/test_services.py
@@ -59,11 +59,13 @@ def test_basic_init(self):
             api=api,
             publishable_key="secret_to_everybody",
             webhook_secret="keep_it_secret_keep_it_safe",
+            domain="tests",
         )
 
         assert billing_service.api is api
         assert billing_service.publishable_key == "secret_to_everybody"
         assert billing_service.webhook_secret == "keep_it_secret_keep_it_safe"
+        assert billing_service.domain == "tests"
 
     def test_create_service(self):
         # Reload stripe to reset the global stripe.api_key to default.
@@ -77,6 +79,7 @@ def test_create_service(self):
                     "billing.secret_key": "sk_test_123",
                     "billing.publishable_key": "pk_test_123",
                     "billing.webhook_key": "whsec_123",
+                    "billing.domain": "tests",
                 }
             )
         )
@@ -87,6 +90,7 @@ def test_create_service(self):
         assert billing_service.api.api_key == "sk_test_123"
         assert billing_service.publishable_key == "pk_test_123"
         assert billing_service.webhook_secret == "whsec_123"
+        assert billing_service.domain == "tests"
 
 
 class TestMockStripeBillingService:
@@ -100,11 +104,13 @@ def test_basic_init(self):
             api=api,
             publishable_key="secret_to_everybody",
             webhook_secret="keep_it_secret_keep_it_safe",
+            domain="tests",
         )
 
         assert billing_service.api is api
         assert billing_service.publishable_key == "secret_to_everybody"
         assert billing_service.webhook_secret == "keep_it_secret_keep_it_safe"
+        assert billing_service.domain == "tests"
 
     def test_create_service(self):
         request = pretend.stub(
@@ -420,11 +426,13 @@ def test_basic_init(self):
             api=api,
             publishable_key="secret_to_everybody",
             webhook_secret="keep_it_secret_keep_it_safe",
+            domain="tests",
         )
 
         assert billing_service.api is api
         assert billing_service.publishable_key == "secret_to_everybody"
         assert billing_service.webhook_secret == "keep_it_secret_keep_it_safe"
+        assert billing_service.domain == "tests"
 
     def test_notimplementederror(self):
         with pytest.raises(NotImplementedError):

diff --git a/warehouse/api/billing.py b/warehouse/api/billing.py
@@ -27,6 +27,11 @@ def handle_billing_webhook_event(request, event):
         # Occurs when a Checkout Session has been successfully completed.
         case "checkout.session.completed":
             checkout_session = event["data"]["object"]
+            if not (
+                checkout_session["metadata"].get("billing_service") == "pypi"
+                and checkout_session["metadata"].get("domain") == billing_service.domain
+            ):
+                return
             # Get expanded checkout session object
             checkout_session = billing_service.get_checkout_session(
                 checkout_session["id"],

diff --git a/warehouse/locale/messages.pot b/warehouse/locale/messages.pot
@@ -579,12 +579,12 @@ msgid ""
 msgstr ""
 
 #: warehouse/manage/views/__init__.py:2817
-#: warehouse/manage/views/organizations.py:887
+#: warehouse/manage/views/organizations.py:889
 msgid "User '${username}' already has an active invite. Please try again later."
 msgstr ""
 
 #: warehouse/manage/views/__init__.py:2882
-#: warehouse/manage/views/organizations.py:952
+#: warehouse/manage/views/organizations.py:954
 msgid "Invitation sent to '${username}'"
 msgstr ""
 
@@ -597,30 +597,30 @@ msgid "Invitation already expired."
 msgstr ""
 
 #: warehouse/manage/views/__init__.py:2958
-#: warehouse/manage/views/organizations.py:1139
+#: warehouse/manage/views/organizations.py:1141
 msgid "Invitation revoked from '${username}'."
 msgstr ""
 
-#: warehouse/manage/views/organizations.py:863
+#: warehouse/manage/views/organizations.py:865
 msgid "User '${username}' already has ${role_name} role for organization"
 msgstr ""
 
-#: warehouse/manage/views/organizations.py:874
+#: warehouse/manage/views/organizations.py:876
 msgid ""
 "User '${username}' does not have a verified primary email address and "
 "cannot be added as a ${role_name} for organization"
 msgstr ""
 
-#: warehouse/manage/views/organizations.py:1035
-#: warehouse/manage/views/organizations.py:1077
+#: warehouse/manage/views/organizations.py:1037
+#: warehouse/manage/views/organizations.py:1079
 msgid "Could not find organization invitation."
 msgstr ""
 
-#: warehouse/manage/views/organizations.py:1045
+#: warehouse/manage/views/organizations.py:1047
 msgid "Organization invitation could not be re-sent."
 msgstr ""
 
-#: warehouse/manage/views/organizations.py:1092
+#: warehouse/manage/views/organizations.py:1094
 msgid "Expired invitation for '${username}' deleted."
 msgstr ""
 

diff --git a/warehouse/manage/views/organizations.py b/warehouse/manage/views/organizations.py
@@ -573,8 +573,10 @@ def create_or_manage_subscription(self):
         if not self.request.organization_access:
             raise HTTPNotFound()
 
-        if not self.organization.subscriptions:
-            # Create subscription if there are no existing subscription.
+        if not self.organization.manageable_subscription:
+            # Create subscription if there are no manageable subscription.
+            # This occurs if no subscription exists, or all subscriptions have reached
+            # a terminal state of Canceled.
             return self.create_subscription()
         else:
             # Manage subscription if there is an existing subscription.

diff --git a/warehouse/organizations/models.py b/warehouse/organizations/models.py
@@ -490,6 +490,14 @@ def active_subscription(self):
         else:
             return None
 
+    @property
+    def manageable_subscription(self):
+        for subscription in self.subscriptions:
+            if subscription.is_manageable:
+                return subscription
+        else:
+            return None
+
     def customer_name(self, site_name="PyPI"):
         return f"{site_name} Organization - {self.display_name} ({self.name})"
 

diff --git a/warehouse/subscriptions/models.py b/warehouse/subscriptions/models.py
@@ -125,6 +125,12 @@ def is_restricted(self):
             StripeSubscriptionStatus.Trialing.value,
         ]
 
+    @property
+    def is_manageable(self):
+        return self.status not in [
+            StripeSubscriptionStatus.Canceled.value,
+        ]
+
 
 class StripeSubscriptionProduct(db.Model):
     __tablename__ = "stripe_subscription_products"