fix(certificates): scope the live certificate uniquely per tenant, not just per branch_ref
IMPORTANT (F2 review): certificate is per-tenant, matching series (already tenant-scoped) and the GET/DELETE anti-oracle boundary. Emission's _get_live_certificate and the upload-replace pre-check (certificates. service, renamed _get_live_certificate_by_branch -> _get_live_certificate_by_tenant_branch) both omitted tenant_ref -- two tenants of one product reusing branch_ref="matriz" collapsed onto the same slot: B's upload soft-deleted A's still-live certificate, and A's emission went on to sign with B's certificate. Migration 8f1a2c9d4b6e replaces the partial-unique index ix_fiscal_certificates_product_branch_live with ix_fiscal_certificates_product_tenant_branch_live on (product_id, tenant_ref, branch_ref) WHERE deleted_at IS NULL, with a working downgrade. Upload's two-layer defense (pre-check + IntegrityError -> CertificateUploadConflictError) still holds against the new index. Tests: - tests/emission/test_emissao.py:: test_dois_tenants_do_mesmo_produto_reusando_branch_ref_tem_certificados_isolados -- two tenants upload for the same product/branch_ref, both stay live; emission for each signs with its OWN certificate (observable via FIX 1's CNPJ check: without FIX 2, tenant A's emission would 409 emitente_certificate_cnpj_mismatch because the "live" cert would actually be B's). - tests/migrations/test_fiscal_documents_schema.py:: test_two_tenants_can_both_hold_a_live_certificate_for_the_same_branch_ref_on_real_migration -- real alembic upgrade head, raw INSERTs proving both tenants' certs land live. - tests/migrations/test_fiscal_documents_schema.py:: test_two_live_certificates_for_same_product_tenant_branch_violate_unique_index_on_real_migration (renamed from ..._product_branch_...) -- same (product, tenant, branch) still rejects a second live certificate on the real migration. - tests/certificates/test_certificates.py:: test_concurrent_uploads_for_same_product_branch_only_one_wins_the_other_gets_409 updated for the renamed/re-scoped precheck function (still same-tenant race, still 1 winner + 1 CertificateUploadConflictError). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
jonatanritter
co-authored by
Claude Opus 4.8
Claude Opus 4.8
parent
3e3a1abc6f
commit
72bb089222
@@ -73,12 +73,15 @@ class CertificateNotYetValidError(Exception):
|
||||
|
||||
class CertificateUploadConflictError(Exception):
|
||||
"""Traduz o `IntegrityError` da violação do índice parcial único
|
||||
`ix_fiscal_certificates_product_branch_live` (`(product_id, branch_ref)
|
||||
WHERE deleted_at IS NULL`, Task 3's `FiscalCertificate.__table_args__`)
|
||||
-- disparado quando DUAS chamadas genuinamente concorrentes de
|
||||
`upload_certificate` para o MESMO `(product_id, branch_ref)` ambas leem
|
||||
`_get_live_certificate_by_branch() -> None` antes de qualquer uma
|
||||
commitar. A PRIMEIRA a commitar vence; a segunda recebe este erro (409
|
||||
`ix_fiscal_certificates_product_tenant_branch_live` (`(product_id,
|
||||
tenant_ref, branch_ref) WHERE deleted_at IS NULL`, Task 3's
|
||||
`FiscalCertificate.__table_args__`, escopo estendido a `tenant_ref` na
|
||||
correção do FIX 2/F2 review -- ver o docstring de `_get_live_
|
||||
certificate_by_tenant_branch`) -- disparado quando DUAS chamadas
|
||||
genuinamente concorrentes de `upload_certificate` para o MESMO
|
||||
`(product_id, tenant_ref, branch_ref)` ambas leem `_get_live_
|
||||
certificate_by_tenant_branch() -> None` antes de qualquer uma commitar.
|
||||
A PRIMEIRA a commitar vence; a segunda recebe este erro (409
|
||||
`certificate_upload_conflict` no router) em vez de silenciosamente
|
||||
criar um segundo certificado vivo."""
|
||||
|
||||
@@ -92,28 +95,35 @@ class CertificateUploadConflictError(Exception):
|
||||
|
||||
|
||||
def _is_fiscal_certificate_branch_live_violation(exc: IntegrityError) -> bool:
|
||||
"""True iff `exc` violates `ix_fiscal_certificates_product_branch_live`
|
||||
-- substring match on the index name, which Task 3's migration names
|
||||
explicitly (unlike the auto's un-named-in-create_all equivalent, this
|
||||
one is identical across `Base.metadata.create_all` and Alembic, so a
|
||||
plain substring check suffices, no "unique constraint" reinforcement
|
||||
"""True iff `exc` violates `ix_fiscal_certificates_product_tenant_branch_
|
||||
live` -- substring match on the index name, which Task 3's migration
|
||||
names explicitly (unlike the auto's un-named-in-create_all equivalent,
|
||||
this one is identical across `Base.metadata.create_all` and Alembic, so
|
||||
a plain substring check suffices, no "unique constraint" reinforcement
|
||||
needed)."""
|
||||
detail = str(getattr(exc.orig, "args", [""])[0]) if exc.orig else str(exc)
|
||||
return "ix_fiscal_certificates_product_branch_live" in detail
|
||||
return "ix_fiscal_certificates_product_tenant_branch_live" in detail
|
||||
|
||||
|
||||
async def _get_live_certificate_by_branch(
|
||||
session: AsyncSession, product_id: uuid.UUID, branch_ref: str
|
||||
async def _get_live_certificate_by_tenant_branch(
|
||||
session: AsyncSession, product_id: uuid.UUID, tenant_ref: str, branch_ref: str
|
||||
) -> FiscalCertificate | None:
|
||||
"""Scoped to `(product_id, branch_ref)` ONLY -- matches exactly the
|
||||
scope of the partial unique index this function's callers (`upload_
|
||||
certificate`) need to pre-check against. NOT the public lookup for GET/
|
||||
DELETE (see `get_certificate`/`deactivate_certificate` below, which
|
||||
additionally scope by `tenant_ref` for the anti-oracle boundary, design
|
||||
spec decision #4: "anti-oracle 404 por (product_id, tenant_ref)")."""
|
||||
"""Scoped to `(product_id, tenant_ref, branch_ref)` -- matches exactly
|
||||
the scope of the partial unique index this function's callers (`upload_
|
||||
certificate`) need to pre-check against. FIX 2 (F2 review): renamed
|
||||
from `_get_live_certificate_by_branch` (which omitted `tenant_ref`) --
|
||||
without it, two DIFFERENT tenants of the SAME product reusing an
|
||||
identical opaque `branch_ref` (e.g. both "matriz") collapsed onto the
|
||||
SAME slot: the second tenant's upload soft-deleted the first tenant's
|
||||
still-live certificate as a "replace", not a conflict. Same public
|
||||
lookup shape as `_get_live_certificate` below (GET/DELETE, anti-oracle
|
||||
boundary, design spec decision #4) -- kept as a SEPARATE function
|
||||
because this one's caller (`upload_certificate`'s pre-check) needs
|
||||
`None` on "no live cert for this slot", not a 404-worthy distinction."""
|
||||
result = await session.execute(
|
||||
select(FiscalCertificate).where(
|
||||
FiscalCertificate.product_id == product_id,
|
||||
FiscalCertificate.tenant_ref == tenant_ref,
|
||||
FiscalCertificate.branch_ref == branch_ref,
|
||||
FiscalCertificate.deleted_at.is_(None),
|
||||
)
|
||||
@@ -159,9 +169,11 @@ async def upload_certificate(
|
||||
(`CertificateExpiredError`) e já vigente (`CertificateNotYetValidError`).
|
||||
Só depois de TODAS as checagens passarem é que qualquer escrita
|
||||
acontece: o certificado anterior (se houver, para o MESMO
|
||||
`(product_id, branch_ref)`) é soft-deletado e o novo é inserido -- um
|
||||
único VIVO por `(product_id, branch_ref)`, nunca dois, nunca um
|
||||
update-in-place.
|
||||
`(product_id, tenant_ref, branch_ref)`) é soft-deletado e o novo é
|
||||
inserido -- um único VIVO por `(product_id, tenant_ref, branch_ref)`,
|
||||
nunca dois, nunca um update-in-place (FIX 2/F2 review: `tenant_ref`
|
||||
entrou no escopo -- ver `_get_live_certificate_by_tenant_branch`'s
|
||||
docstring para o porquê).
|
||||
|
||||
O INSERT final é protegido pelo índice parcial único (Task 3) contra a
|
||||
corrida de dois uploads genuinamente concorrentes -- o `commit()` do
|
||||
@@ -178,7 +190,7 @@ async def upload_certificate(
|
||||
if info.not_valid_before > now:
|
||||
raise CertificateNotYetValidError(info.not_valid_before)
|
||||
|
||||
previous = await _get_live_certificate_by_branch(session, product_id, branch_ref)
|
||||
previous = await _get_live_certificate_by_tenant_branch(session, product_id, tenant_ref, branch_ref)
|
||||
if previous is not None:
|
||||
previous.deleted_at = now
|
||||
|
||||
|
||||
@@ -109,27 +109,33 @@ class FiscalSeries(Base, UUIDPKMixin, TimestampMixin, SoftDeleteMixin):
|
||||
class FiscalCertificate(Base, UUIDPKMixin, TimestampMixin, SoftDeleteMixin):
|
||||
"""Ported from the auto's `FiscalCertificate` (`app/modules/fiscal/
|
||||
models.py`) -- A1 certificate (.pfx) for a `branch_ref`. Per the porte
|
||||
table, the emitter is `(product_id, branch_ref)`, with `cnpj` carried
|
||||
on the row itself (the "vínculo forte", design spec decision #4: the
|
||||
CNPJ is what's validated against the certificate at upload and against
|
||||
the emitente at emission -- `branch_ref` alone is an opaque string this
|
||||
service never interprets). No fallback to a tenant-level or
|
||||
table, the emitter is `(product_id, tenant_ref, branch_ref)`, with
|
||||
`cnpj` carried on the row itself (the "vínculo forte", design spec
|
||||
decision #4: the CNPJ is what's validated against the certificate at
|
||||
upload and against the emitente at emission -- `branch_ref` alone is an
|
||||
opaque string this service never interprets). No fallback to a
|
||||
product-level certificate -- fail-closed by construction, same as the
|
||||
auto: a branch_ref without its own live certificate cannot emit.
|
||||
auto: a `(tenant_ref, branch_ref)` without its own live certificate
|
||||
cannot emit.
|
||||
|
||||
One VIVO (`deleted_at IS NULL`) row per `(product_id, branch_ref)`: a
|
||||
second upload soft-deletes the previous live row (Task 4's
|
||||
`certificates.service`). `ix_fiscal_certificates_product_branch_live`
|
||||
is a PARTIAL UNIQUE index enforcing that at the database level (same
|
||||
"two concurrent uploads must not both land a live row" reasoning as the
|
||||
auto's own migration `a1b2c3d4e5f6`'s fix) -- scoped by `product_id` in
|
||||
ADDITION to `branch_ref` (the auto's version only needed `branch_id`,
|
||||
already product-scoped by being a real FK; here `branch_ref` is an
|
||||
OPAQUE string owned by the calling product, so two DIFFERENT products
|
||||
could coincidentally pick the identical string for two DIFFERENT real
|
||||
branches -- scoping the uniqueness by `product_id` too is what keeps
|
||||
that from cross-contaminating one product's certificate slot with
|
||||
another's).
|
||||
One VIVO (`deleted_at IS NULL`) row per `(product_id, tenant_ref,
|
||||
branch_ref)`: a second upload soft-deletes the previous live row
|
||||
(Task 4's `certificates.service`). `ix_fiscal_certificates_product_
|
||||
tenant_branch_live` is a PARTIAL UNIQUE index enforcing that at the
|
||||
database level (same "two concurrent uploads must not both land a live
|
||||
row" reasoning as the auto's own migration `a1b2c3d4e5f6`'s fix) --
|
||||
scoped by `product_id` AND `tenant_ref` in ADDITION to `branch_ref`
|
||||
(the auto's version only needed `branch_id`, already product-scoped by
|
||||
being a real FK; here `branch_ref` is an OPAQUE string owned by the
|
||||
calling product, so two DIFFERENT products -- or two DIFFERENT tenants
|
||||
of the SAME product -- could coincidentally pick the identical string
|
||||
for two DIFFERENT real branches -- scoping the uniqueness by
|
||||
`product_id`+`tenant_ref` too is what keeps that from
|
||||
cross-contaminating one tenant's certificate slot with another's; FIX 2,
|
||||
F2 review, 2026-07-17-sowai-fiscal-svc-design.md decisão #4 turned this
|
||||
into a shipped bug otherwise: two tenants both using `branch_ref=
|
||||
"matriz"` would collapse onto one slot, the second tenant's upload
|
||||
silently soft-deleting the first's still-live certificate).
|
||||
|
||||
`pfx_encrypted`/`password_encrypted` are Fernet ciphertext (Task 4,
|
||||
`FISCAL_CERT_ENCRYPTION_KEY` env var, own key -- never
|
||||
@@ -139,8 +145,9 @@ class FiscalCertificate(Base, UUIDPKMixin, TimestampMixin, SoftDeleteMixin):
|
||||
__tablename__ = "fiscal_certificates"
|
||||
__table_args__ = (
|
||||
Index(
|
||||
"ix_fiscal_certificates_product_branch_live",
|
||||
"ix_fiscal_certificates_product_tenant_branch_live",
|
||||
"product_id",
|
||||
"tenant_ref",
|
||||
"branch_ref",
|
||||
unique=True,
|
||||
postgresql_where=text("deleted_at IS NULL"),
|
||||
|
||||
@@ -239,11 +239,23 @@ def _normalize_cnpj(value: str) -> str:
|
||||
|
||||
|
||||
async def _get_live_certificate(
|
||||
session: AsyncSession, product_id: uuid.UUID, branch_ref: str
|
||||
session: AsyncSession, product_id: uuid.UUID, tenant_ref: str, branch_ref: str
|
||||
) -> FiscalCertificate | None:
|
||||
"""Escopado por `(product_id, tenant_ref, branch_ref)` -- FIX 2 (F2
|
||||
review): o certificado é POR TENANT (mesma escala que `_get_series`, já
|
||||
tenant-scoped, e o GET/DELETE de `certificates.service._get_live_
|
||||
certificate`). Antes desta correção este lookup omitia `tenant_ref`, e
|
||||
dois tenants do MESMO produto reusando um `branch_ref` idêntico (ex.:
|
||||
ambos "matriz", nomes opacos que o serviço nunca interpreta) resolviam
|
||||
para o MESMO certificado -- o segundo upload de um tenant B para
|
||||
"matriz" soft-deletava o certificado vivo do tenant A (ver o índice
|
||||
parcial único em `documents.models.FiscalCertificate`, também corrigido
|
||||
nesta mesma revisão) e a emissão de A passava a assinar com o
|
||||
certificado de B."""
|
||||
result = await session.execute(
|
||||
select(FiscalCertificate).where(
|
||||
FiscalCertificate.product_id == product_id,
|
||||
FiscalCertificate.tenant_ref == tenant_ref,
|
||||
FiscalCertificate.branch_ref == branch_ref,
|
||||
FiscalCertificate.deleted_at.is_(None),
|
||||
)
|
||||
@@ -303,7 +315,7 @@ async def emitir_documento(
|
||||
# 2. completude estrutural -- ANTES de alocar número.
|
||||
missing: list[str] = []
|
||||
|
||||
certificate = await _get_live_certificate(session, product.id, payload.branch_ref)
|
||||
certificate = await _get_live_certificate(session, product.id, payload.tenant_ref, payload.branch_ref)
|
||||
if certificate is None:
|
||||
missing.append("certificado A1 do branch_ref")
|
||||
elif certificate.not_valid_after < datetime.now(timezone.utc):
|
||||
|
||||
Reference in New Issue
Block a user