fix(billing): reconciliation review fixes — per-subscription key, IDOR guard
- 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.
This commit is contained in:
@@ -4,6 +4,7 @@
|
|||||||
import logging
|
import logging
|
||||||
|
|
||||||
from odoo import api, fields, models
|
from odoo import api, fields, models
|
||||||
|
from odoo.exceptions import UserError
|
||||||
|
|
||||||
_logger = logging.getLogger(__name__)
|
_logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
@@ -25,6 +26,11 @@ class FusionBillingReconciliation(models.Model):
|
|||||||
)
|
)
|
||||||
partner_id = fields.Many2one("res.partner", required=True, ondelete="cascade", index=True)
|
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.")
|
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()
|
odoo_amount = fields.Monetary()
|
||||||
external_amount = fields.Monetary(string="App-actual Amount")
|
external_amount = fields.Monetary(string="App-actual Amount")
|
||||||
delta = fields.Monetary(help="odoo_amount - external_amount.")
|
delta = fields.Monetary(help="odoo_amount - external_amount.")
|
||||||
@@ -42,6 +48,11 @@ class FusionBillingReconciliation(models.Model):
|
|||||||
)
|
)
|
||||||
note = fields.Text()
|
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
|
@api.model
|
||||||
def _compute_reconciliation(self, flat_amount, charge, cpu_seconds, external_amount,
|
def _compute_reconciliation(self, flat_amount, charge, cpu_seconds, external_amount,
|
||||||
tolerance=0.01):
|
tolerance=0.01):
|
||||||
@@ -67,6 +78,10 @@ class FusionBillingReconciliation(models.Model):
|
|||||||
Charge = self.env['fusion.billing.charge']
|
Charge = self.env['fusion.billing.charge']
|
||||||
service = self.env['fusion.billing.service'].search(
|
service = self.env['fusion.billing.service'].search(
|
||||||
[('code', '=', 'nexacloud')], limit=1)
|
[('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': []}
|
summary = {'match': 0, 'delta': 0, 'skipped': [], 'failed': []}
|
||||||
for r in rows:
|
for r in rows:
|
||||||
sub_ext = str(r.get('subscription_external_id') or '')
|
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),
|
flat, charge, float(r.get('cpu_seconds') or 0.0),
|
||||||
external_amount, tolerance)
|
external_amount, tolerance)
|
||||||
vals = {
|
vals = {
|
||||||
'service_id': service.id if service else False,
|
'service_id': service.id,
|
||||||
'partner_id': sub.partner_id.id, 'period': period,
|
'partner_id': sub.partner_id.id, 'period': period,
|
||||||
|
'external_subscription_id': sub_ext,
|
||||||
'odoo_amount': odoo_amount, 'external_amount': external_amount,
|
'odoo_amount': odoo_amount, 'external_amount': external_amount,
|
||||||
'delta': delta, 'status': status,
|
'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([
|
existing = self.search([
|
||||||
('service_id', '=', vals['service_id']),
|
('service_id', '=', service.id),
|
||||||
('partner_id', '=', sub.partner_id.id),
|
('external_subscription_id', '=', sub_ext),
|
||||||
('period', '=', period)], limit=1)
|
('period', '=', period)], limit=1)
|
||||||
if existing:
|
if existing:
|
||||||
existing.write(vals)
|
existing.write(vals)
|
||||||
|
|||||||
@@ -126,9 +126,16 @@ class FusionBillingService(models.Model):
|
|||||||
if sub:
|
if sub:
|
||||||
return sub
|
return sub
|
||||||
try:
|
try:
|
||||||
return SaleOrder.browse(int(external_ref))
|
candidate = SaleOrder.browse(int(external_ref))
|
||||||
except (TypeError, ValueError):
|
except (TypeError, ValueError):
|
||||||
return SaleOrder
|
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):
|
def _api_record_usage(self, payload):
|
||||||
"""Ingest a batch of usage events.
|
"""Ingest a batch of usage events.
|
||||||
|
|||||||
@@ -89,3 +89,23 @@ class TestReconcileRows(TransactionCase):
|
|||||||
{'subscription_external_id': 'nope', 'period': '2026-05',
|
{'subscription_external_id': 'nope', 'period': '2026-05',
|
||||||
'cpu_seconds': 0.0, 'external_amount': 1.0}])
|
'cpu_seconds': 0.0, 'external_amount': 1.0}])
|
||||||
self.assertTrue(any(s['id'] == 'nope' for s in summary['skipped']))
|
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'})
|
||||||
|
|||||||
Reference in New Issue
Block a user