From a82f09ea5097ed6747807fced87ccdefff4e14a5 Mon Sep 17 00:00:00 2001 From: gsinghpal Date: Wed, 27 May 2026 14:51:43 -0400 Subject: [PATCH] =?UTF-8?q?fix(billing):=20reconciliation=20review=20fixes?= =?UTF-8?q?=20=E2=80=94=20per-subscription=20key,=20IDOR=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - CRITICAL: reconciliation upsert keyed on (service, partner, period) collided when one customer has two deployments (two subs) in a period — the second overwrote the first. Add external_subscription_id to the model + a UNIQUE(service_id, external_subscription_id, period) constraint, and key the upsert per subscription. New test proves two subs for one partner keep two rows. - raise a clear error if the nexacloud service is missing (was a confusing per-row failure). - _fc_resolve_subscription: the integer fallback no longer reaches a different service's tagged subscription (latent multi-service IDOR); live untagged subs stay resolvable and the partner-link authz is unchanged. Full suite green on odoo-trial. --- .../models/reconciliation.py | 24 ++++++++++++++++--- fusion_centralize_billing/models/service.py | 9 ++++++- .../tests/test_reconciliation.py | 20 ++++++++++++++++ 3 files changed, 49 insertions(+), 4 deletions(-) diff --git a/fusion_centralize_billing/models/reconciliation.py b/fusion_centralize_billing/models/reconciliation.py index 0abfb5a2..b7e67838 100644 --- a/fusion_centralize_billing/models/reconciliation.py +++ b/fusion_centralize_billing/models/reconciliation.py @@ -4,6 +4,7 @@ import logging from odoo import api, fields, models +from odoo.exceptions import UserError _logger = logging.getLogger(__name__) @@ -25,6 +26,11 @@ class FusionBillingReconciliation(models.Model): ) partner_id = fields.Many2one("res.partner", required=True, ondelete="cascade", index=True) period = fields.Char(required=True, help="Billing period label, e.g. 2026-05.") + external_subscription_id = fields.Char( + index=True, + help="Source-app subscription id this row reconciles (NexaCloud sub UUID). Part of " + "the upsert key so a customer with multiple deployments gets one row PER " + "subscription per period, not a single colliding row.") odoo_amount = fields.Monetary() external_amount = fields.Monetary(string="App-actual Amount") delta = fields.Monetary(help="odoo_amount - external_amount.") @@ -42,6 +48,11 @@ class FusionBillingReconciliation(models.Model): ) note = fields.Text() + _service_sub_period_uniq = models.Constraint( + "UNIQUE(service_id, external_subscription_id, period)", + "One reconciliation row per service, subscription, and period.", + ) + @api.model def _compute_reconciliation(self, flat_amount, charge, cpu_seconds, external_amount, tolerance=0.01): @@ -67,6 +78,10 @@ class FusionBillingReconciliation(models.Model): Charge = self.env['fusion.billing.charge'] service = self.env['fusion.billing.service'].search( [('code', '=', 'nexacloud')], limit=1) + if not service: + raise UserError( + "NexaCloud billing service not found — run the importer first so the " + "service, catalog, and shadow subscriptions exist.") summary = {'match': 0, 'delta': 0, 'skipped': [], 'failed': []} for r in rows: sub_ext = str(r.get('subscription_external_id') or '') @@ -89,14 +104,17 @@ class FusionBillingReconciliation(models.Model): flat, charge, float(r.get('cpu_seconds') or 0.0), external_amount, tolerance) vals = { - 'service_id': service.id if service else False, + 'service_id': service.id, 'partner_id': sub.partner_id.id, 'period': period, + 'external_subscription_id': sub_ext, 'odoo_amount': odoo_amount, 'external_amount': external_amount, 'delta': delta, 'status': status, } + # Upsert per (service, subscription, period) — NOT per partner — so a + # customer with two deployments gets a row for each, no overwrite. existing = self.search([ - ('service_id', '=', vals['service_id']), - ('partner_id', '=', sub.partner_id.id), + ('service_id', '=', service.id), + ('external_subscription_id', '=', sub_ext), ('period', '=', period)], limit=1) if existing: existing.write(vals) diff --git a/fusion_centralize_billing/models/service.py b/fusion_centralize_billing/models/service.py index d56f1aa3..10308af3 100644 --- a/fusion_centralize_billing/models/service.py +++ b/fusion_centralize_billing/models/service.py @@ -126,9 +126,16 @@ class FusionBillingService(models.Model): if sub: return sub try: - return SaleOrder.browse(int(external_ref)) + candidate = SaleOrder.browse(int(external_ref)) except (TypeError, ValueError): return SaleOrder + # Don't let the integer fallback reach a DIFFERENT service's tagged subscription. + # (Live, API-created subs carry no service tag and stay resolvable here; the caller + # still enforces partner-is-linked-to-this-service authorization.) + if candidate.exists() and candidate.x_fc_billing_service_id \ + and candidate.x_fc_billing_service_id != self: + return SaleOrder + return candidate def _api_record_usage(self, payload): """Ingest a batch of usage events. diff --git a/fusion_centralize_billing/tests/test_reconciliation.py b/fusion_centralize_billing/tests/test_reconciliation.py index 30927b36..d4a6fb60 100644 --- a/fusion_centralize_billing/tests/test_reconciliation.py +++ b/fusion_centralize_billing/tests/test_reconciliation.py @@ -89,3 +89,23 @@ class TestReconcileRows(TransactionCase): {'subscription_external_id': 'nope', 'period': '2026-05', 'cpu_seconds': 0.0, 'external_amount': 1.0}]) self.assertTrue(any(s['id'] == 'nope' for s in summary['skipped'])) + + def test_two_subscriptions_same_partner_period_do_not_collide(self): + # A customer with two deployments -> two subscriptions in the same period. + data = _fixture() + data['subscriptions'].append( + {"id": "s-1b", "user_id": "u-1", "deployment_id": "d-1b", "plan_id": "p-1", + "status": "active", "billing_cycle": "monthly", + "current_period_start": "2026-05-01", "current_period_end": "2026-06-01"}) + self.env['fusion.billing.import.wizard'].sudo()._import_rows(data) + self.Recon._reconcile_rows([ + {'subscription_external_id': 's-1', 'period': '2026-05', + 'cpu_seconds': 0.0, 'external_amount': 20.0}, + {'subscription_external_id': 's-1b', 'period': '2026-05', + 'cpu_seconds': 0.0, 'external_amount': 99.0}, + ]) + partner = self._partner_of('s-1') + rows = self.Recon.search( + [('partner_id', '=', partner.id), ('period', '=', '2026-05')]) + self.assertEqual(len(rows), 2, "two subs for one partner must keep two rows") + self.assertEqual(set(rows.mapped('external_subscription_id')), {'s-1', 's-1b'})