SELC-9099: multitenant Step 1 — tenant data mapping registry, tenantId backfill and strict isolation switch - #694
Open
andrea-putzu wants to merge 10 commits into
Open
Conversation
Step_1 EPIC sub-task 9. The mapping tenant -> {Cosmos account, Storage
naming, personal data vault, email sender domain} reuses the Step 0
registry rather than a second one: local.tenant_data_isolation now sits
next to tenant_frontend_origins/tenant_ids in _modules/local-env, so one
file declares everything a tenant is. Two registries could have disagreed
about which tenants exist.
Values re-declare what the -ar/-pnpg stacks already name today, since
local.mongo_db picks a Cosmos account by deployment folder and a
consolidated deployment serving both tenants can no longer do that.
Connection strings appear as Key Vault secret NAMES, never values; PNPG's
is suffixed because after consolidation both tenants' secrets share the
surviving stack's vault.
personal_data_vault_tenant and email_sender_domain stay null: the vault
provider (SELC-10.3) and the sender domain table (SELC-11.3) are open, and
inventing a value would be exactly the silent default SELC-10.2/11.2
forbid. Reading a null dimension throws.
TenantDataIsolationRegistry parses the map from one env var and is
fail-closed on every path: unknown tenant, missing entry, null tenant or
undecided dimension all raise UnresolvedTenantMappingException. It takes
the tenant already validated by TenantContext and never derives one.
Unknown JSON properties are ignored so a dimension can be rolled out
service by service; a malformed payload or unknown tenant key fails at
startup, where a disagreement between config and the TenantId enum belongs.
container_app_microservice gains tenant_data_isolation_json so a consuming
stack adds one line instead of a literal map. Not yet wired into the ~40
existing stacks: shipping the variable to services that do not read it
would roll every container app revision for nothing.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Step_1 EPIC still read as if nothing had been built, while sub-tasks 1-4 were decided and largely implemented under Step_0's sub-task 6, which ran ahead of this document. Same drift that was fixed once already for Step_0; an epic that under-reports its own state is how work gets done twice. Recorded per sub-task: Cosmos discriminator chosen for all 15 Mongo services and rolled out, but still not enforcement while the migration-window "or tenantId is null" read branch stands (which makes the backfill, sub-task 10, a gate rather than a tail task); database-per-tenant selected by nobody, with the product-driven routing built in onboarding-ms described for what it is - an orthogonal axis, not this sub-task; storage inventory done except dashboard-bff. Two blockers found while scoping sub-task 5, now written down instead of being rediscovered later: the blob containers are not declared in this repository (the app stacks only read the storage accounts through data blocks), so per-tenant containers cannot be provisioned here; and the SYSTEM blob client serves both tenant data and shared templates, so suffixing its call sites wholesale would relocate templates into a container that does not hold them. Also noted that the six Spring apps can consume the sub-task 9 env var but have no parser for it yet. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Every tenant-scoped read still matches `tenantId = <tenant> OR tenantId IS NULL`. That branch was necessary the day the discriminator shipped — a strict filter would have hidden every pre-existing document from both tenants — but it also means an untagged document is readable by *both* tenants. Until it is gone the Cosmos isolation is a convention, not a boundary, and it cannot be removed while untagged documents exist. This adds the tool that makes their removal possible. The tenant is an explicit argument rather than something inferred from the data. Each tenant has its own Cosmos account, so it is the connection string that decides which tenant is being migrated; requiring the caller to say which one turns a wrong MONGO_HOST into a mismatch they notice instead of a silently mislabelled database. Two properties make it safe to run in production. It only writes to documents that have no `tenantId`, so a document already attributed to a tenant is never touched: replaying the script, or running it for the wrong tenant after a correct run, cannot move data across tenants, and an interrupted run simply resumes. And it writes nothing at all unless --apply is passed. The collection list is derived from the entities that actually declare the field, not from the set of databases that exist. The product catalogue is excluded on purpose: product definitions are global and shared, so tagging them would be wrong rather than merely unnecessary. Collections that are absent from a given host are reported as skipped, since one connection string reaches only the databases that deployment owns. --verify exists so the follow-up has an objective gate: it exits non-zero while untagged documents remain, and only when it is clean for both tenants in an environment may that environment's reads be made strict. Verified end to end against a MongoDB container: dry run reports without writing, --apply tags only the untagged documents while leaving pre-tagged ones on their original tenant, a second run for the other tenant is a no-op, and --verify flips from non-zero to zero. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The registry already refused to guess a mapping, but it refused silently: the only trace was the exception itself. A refusal means a request reached the data layer without a usable tenant mapping, which is exactly the security-relevant event sub-task 11 asks to be able to audit — and it is the kind of event a caller can erase, by retrying or by mapping the exception to a generic 500, before anyone sees it. The log line is emitted inside the registry rather than at its call sites for that reason: it is the only place that cannot be bypassed. It mirrors what TenantValidationFilter already does at the request boundary, so both halves of the tenant enforcement now leave the same kind of trail. The messages name the tenant and the missing dimension and nothing else: no connection string, no secret value, no user data. The registry only ever holds Key Vault secret *names*, so there is nothing sensitive to leak in the first place, and keeping the messages to routing metadata makes that property hold as the vault and email dimensions get filled in. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Every tenant-scoped read matches `tenantId = <tenant> OR tenantId IS NULL`, and the comment at each of those eight places said the null branch must be deleted once the backfill has run. Taken literally that is a single commit that flips isolation on everywhere at once, and it can only be merged after the last environment has been migrated: the same artifact is promoted DEV to UAT to PROD, so a hardcoded switch would hold every unrelated change behind a data migration in PROD. The branch is now controlled by selfcare.tenant.strict-data-isolation, default false. Deploying this changes nothing; an environment turns isolation strict the moment its own --verify comes back clean, and reverts by unsetting a variable rather than by rolling back a release. One variable name works for both stacks, since Spring's relaxed binding and MicroProfile Config resolve SELFCARE_TENANT_STRICT_DATA_ISOLATION to the same property. All eight chokepoints are covered, so no service can be left behind when an environment flips: document-ms (both the Panache and the native-syntax query), iam (the entity helper and the aggregation pipeline), user-ms, onboarding-ms, user-group-ms, institution-ms and delegation-cdc's two repositories. In iam's entity helper the flag is read through the static config API for the same reason the tenant itself is read through CDI.current() there: the predicate is built statically. The strict path is the one that has never run anywhere, so it gets its own tests rather than inheriting the existing ones: a Quarkus profile-based suite in document-ms and a Spring property-based one in institution-ms, both asserting on real MongoDB that an untagged document is now invisible while the current tenant's own data still is not. Without them the first execution of this code would have been in whichever environment flipped the flag first. The flag is deliberately scoped to the tenant predicate. Callers with no resolvable tenant - schedulers, event consumers - still read unscoped; that concession is a separate decision from the backfill and is left untouched here. It is also temporary, and both the README and the EPIC say so: once every environment runs strict, the flag and the null branch must both be deleted, so that isolation holds by construction rather than by configuration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous commit missed four chokepoints: auth's OtpFlowRepository, user-cdc's UserInstitutionRepository, and the two service-level lookups in user-ms that bypass QueryUtils (UserInfoServiceDefault and UserInstitutionServiceDefault). They kept matching untagged documents unconditionally. Partial coverage is worse than none here, because the flag is what an environment uses to declare itself strict. An operator who has run the backfill, flipped the variable and seen the other services go strict would reasonably conclude the environment is isolated, while otp flows, the CDC mirror and two user lookups still served untagged documents to whichever tenant asked. The value of a single switch is that it means the same thing everywhere. auth's helper had to stop being static to read injected configuration; it is only ever called from instance methods, so this is not a change in how it is used. Verified with clean builds: user-ms 366 tests, auth 133, user-cdc 50, all green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The flag shipped as code only. The runbook said "set SELFCARE_TENANT_STRICT_DATA_ISOLATION", but there was no place to set it: an operator would have had to hand-edit app_settings in each affected stack, which is exactly the kind of per-service edit that ends with an environment half strict. It is declared once per environment, in local-env, next to the tenant registry, because it describes the state of that environment's data rather than the configuration of a service. Every stack that carries tenant-scoped data reads the same local, so an environment goes strict in a single edit and cannot drift between services. The module variable stays null by default and is omitted from the container env block entirely when unset, rather than being rendered as "false". That keeps an unwired stack on the application default and, more importantly, makes the eventual deletion of the flag a no-op for those stacks instead of a removal of an env var they were pinned to. Wiring the individual app stacks is deliberately left out: passing the value to a service only matters when that environment is ready to flip, and injecting it now would roll every container app revision to deliver a variable whose value equals the default. terraform fmt and validate pass on both modules. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
andrea-putzu
requested review from
a team,
fabiosalamonenttdata,
giampiero-ferrara and
giulia-tremolada
as code owners
July 31, 2026 21:14
Sub-task 5 is blocked on blob containers this repository does not declare, so the code cannot ship. The scoping work behind it can, and leaving it unwritten means the next person re-derives it — or, worse, guesses. The status said only that the SYSTEM client mixes tenant data with shared templates; it did not say which of the 21 call sites is which. The classification is recorded in full: 14 tenant-owned, 7 reading shared templates. The point worth carrying forward is that the split does not follow StorageOrigin — clientFor(SYSTEM) serves both categories — it follows the storage path. Routing therefore belongs beside the path construction, and a change that keyed off StorageOrigin alone would suffix the mail and contract templates into per-tenant containers that do not hold them. Classifying also surfaced something that outlives this sub-task: every shared-template path is supplied by the caller. The container will be derived from the validated tenant as the acceptance criteria require, but the path inside it is not, and an implementation tempted to classify by inspecting the supplied path would let a crafted path pick the container too. Recorded as a finding with the mitigation, since it is a separate concern from provisioning the containers. No behaviour change; the isolation itself remains blocked on whoever owns the containers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Added Two points worth a reviewer's attention, both documentation-only (no behaviour change):
Sub-task 5 itself stays blocked: the per-tenant blob containers are not declared in this repository. |
The strict-mode switch existed at the application and module layers but stopped at the module boundary: no service stack passed it, so changing the per-environment local could never change a running service. Pass both the registry and the switch from local-env into every one of the 46 Container App module invocations that owns one of the fourteen tenant query chokepoints. This makes the documented one-edit environment transition real rather than aspirational. The backfill verification also treated an inaccessible or simply wrong host as clean when none of the expected collections could be listed. Make discovery fail closed, require every absent collection to be explicitly acknowledged, and reject a run that inspects no collection. The expected Cosmos account is now mandatory and checked both against MONGO_HOST and the requested tenant before connecting. Scan every collection before the first write and abort the entire run if an existing tenantId is not exactly the requested scalar string. Missing and BSON null values remain backfillable, while arrays, numbers, foreign tenant strings and other malformed values are evidence to investigate, not values to overwrite. The predicates use expression-level BSON type checks so arrays containing null or both tenant names cannot bypass the preflight or verification. Seven unit tests cover the failure modes and the exact query shapes. The predicates were also exercised against MongoDB 7 with missing, null, scalar, array and numeric tenantId values. Terraform validation passes for both shared modules and a representative root of every affected service. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Resolved all high-severity findings from the branch review in
Validation: 7 unit tests pass; MongoDB 7 predicate integration passes; Terraform |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the data-layer groundwork of
apps/docs/Multitenant/Step_1/EPIC.md, the epic that makes theAR(selfcare.pagopa.it) andPNPG(imprese.notifichedigitali.it) tenants stop sharing data stores. It targetsfeature/SELC-9099, notmain.List of Changes
A single source of truth for the per-tenant data mapping (sub-task 9)
infra/resources/_modules/local-env: newtenant_data_isolationlocal describing, per tenant, which Cosmos account/resource group/Key Vault secret name, which storage account infix and container suffix, which personal data vault tenant and which outbound email sender domain that tenant uses. Exported through theconfigoutput together with its JSON encoding.infra/resources/_modules/container_app_microservice: opt-intenant_data_isolation_jsonvariable, injected as theSELFCARE_TENANT_DATA_ISOLATIONenv var.libs/selfcare-sdk-security: newit.pagopa.selfcare.security.tenantpackage —TenantDataMapping,TenantDataIsolationRegistry,TenantDataIsolationRegistryProducer,UnresolvedTenantMappingException.Only routing data crosses this channel. Connection strings are carried as Key Vault secret names, never values. The two dimensions that have no decision yet (personal data vault tenant, email sender domain) are explicitly
null, and reading anulldimension throws instead of falling back to a default.Fail-closed behaviour, and evidence of it (sub-task 11, data layer)
The registry refuses to answer rather than guessing: unknown tenant, unmapped dimension and undecided dimension all raise
UnresolvedTenantMappingException. Every refusal is logged atWARNbefore the throw, inside the registry rather than at the call sites, so that a caller which recovers from the exception cannot silence a tenant-isolation failure. Log messages carry the tenant name and the missing dimension only — no PII, no secrets.Backfilling the tenant discriminator (sub-task 10)
apps/docs/Multitenant/Step_1/scripts/backfill_tenant_id.pyplus a runbook README. It tags pre-existing documents across the 15 tenant-scoped collections in 8 databases (selcAuth,selcDocument,selcIam,selcMsCore,selcOnboarding,selcUser,selcUserGroup,selcWebhook). The product catalogue is deliberately excluded, being cross-tenant by design. Dry-run by default,--applyto write,--verifyto assert that nothing untagged is left. It only ever touches documents with notenantId, so it is idempotent and cannot move a document from one tenant to the other.Turning the migration branch into real isolation (sub-task 2)
Every tenant-scoped query currently matches
tenantId = <caller> OR tenantId IS NULL, so pre-existing untagged data stays visible during the migration — which also means it is visible to both tenants. The newselfcare.tenant.strict-data-isolationflag (defaultfalse) drops that second branch at all fourteen chokepoints across ten services:auth,document-ms,iam,user-ms,onboarding-ms,user-group-ms,institution-ms,delegation-cdc,user-cdc.It is a flag rather than a code deletion because the same artifact is promoted DEV → UAT → PROD: a hardcoded deletion could only merge after the last environment had been backfilled, holding every unrelated change behind a data migration. It is declared once per environment in
local-env, not per service, because it describes the state of that environment's data — flipping it for some services and not others yields an environment that reports isolation it does not have. The flag is temporary and both the EPIC and the README say so: once every environment is strict, the flag and the branch are deleted so isolation holds by construction.Documentation
Step_1/EPIC.mdhad no statuses at all while much of the work already existed on the parent branch. Each sub-task now carries a grounded status block, and the blockers list reflects what is actually blocked and on whom.Motivation and Context
ARandPNPGare operated as separate services with separate privacy notices and separate data controllers, but they currently share Cosmos accounts, storage accounts, a personal data vault and an outbound mail domain. Tenant separation is enforced by an application-leveltenantIdpredicate that, during the migration, also matches untagged documents — so any document written before the discriminator existed is readable from both tenants.This PR closes the data-layer half of that gap in the order the dependencies require:
The remaining sub-tasks are blocked on decisions outside this repository (personal data vault provider, per-tenant email sender domains) or on infrastructure not declared here (blob containers for
document-ms); they are documented as such in the EPIC rather than half-implemented.How Has This Been Tested?
Infrastructure —
terraform fmt -checkandterraform validatepass on both_modules/local-envand_modules/container_app_microservice. Module documentation regenerated by theterraform_docspre-commit hook.Unit tests — 13 new tests for the registry (
TenantDataIsolationRegistryTest,TenantDataIsolationRegistryProducerTest) covering resolution, unknown tenants, unmapped and undecided dimensions, the empty-string vsnulldistinction for the AR container suffix, and malformed configuration. Fullselfcare-sdk-securitysuite: 54/54.Strict-mode tests — the flag is read at startup, so strict behaviour needs its own test classes:
DocumentRepositoryStrictTenantTest(9 tests, QuarkusQuarkusTestProfilewith config overrides) andTenantDataIsolationStrictRepositoryTest(4 tests, Spring@DataMongoTest(properties = ...)). Both run against a real MongoDB and assert that an untagged document is invisible under strict mode while remaining visible under the default.Regression — clean builds of every touched service, no failures:
user-ms366,auth133,user-cdc50,iam67,user-group-ms106,document-ms20 lenient + 9 strict,institution-msdao 4 lenient + 4 strict,delegation-cdc23, plusonboarding-ms. The lenient suites are unchanged and still pass, which is the point: deploying this changes no behaviour until an environment opts in.Backfill script — exercised end to end against a MongoDB 7 container. Verified that a dry run writes nothing; that
--applytags only untagged documents; that a document pre-taggedPNPGis left asPNPGwhen running asAR; that re-running for the other tenant is a no-op; and that--verifyflips from exit 1 to exit 0 after the backfill. Container and virtualenv removed afterwards.Screenshots (if appropriate):
N/A — no user-facing surface changes.
Types of changes
Non-breaking as merged:
selfcare.tenant.strict-data-isolationdefaults tofalseand the Terraform variables are opt-in, so nothing changes until an environment is explicitly switched. Enabling it in an environment beforebackfill_tenant_id.py --verifyexits0there is breaking — untagged documents become invisible to everyone rather than visible to both tenants. The runbook states the ordering.Checklist:
Step_1/EPIC.mdstatuses and blockers, a new operational README for the backfill script including the "after the backfill" procedure, and the regeneratedcontainer_app_microservicemodule README.