From 858360f614bdc834f8474c62c8ced4dfa75d4590 Mon Sep 17 00:00:00 2001 From: Krstan Vjestica Date: Sat, 15 Aug 2026 15:10:00 +0200 Subject: [PATCH 1/3] fix: a failed tenant lookup ran the request with no RLS scope `_resolve_tenant_org` caught every exception from the organization query and returned None. `__call__` cannot tell that None apart from the legitimate one -- superuser, or a user with no tenant -- so it proceeded without installing a scope. Postgres RLS reads an unset `forail.current_tenant_id` as "every row", so a transient database error during the lookup turned a tenant's request into a global one. The outer try/except in `__call__` made it worse: it looked like a guard, but the exception had already been swallowed one level down, so it never fired. A failed lookup is not evidence that the user has no tenant. It is evidence that we do not know, and the only safe answer to that is to refuse -- which is already what a failed `set_tenant_id` does two lines below. `_resolve_tenant_org` now lets the error out and `__call__` fails closed with the same 500. Gated on tenancy and RLS both being on: with either off the resolver returns before touching the database, so a single-tenant install cannot start answering 500 because of this gate. Covered by tests_standalone/test_tenant_isolation.py, a new file -- the module had none. Against the pre-fix code two of them fail: the view runs anyway, and the resolver swallows the error. --- forail/main/tenancy/isolation.py | 38 +++-- tests_standalone/test_tenant_isolation.py | 182 ++++++++++++++++++++++ 2 files changed, 209 insertions(+), 11 deletions(-) create mode 100644 tests_standalone/test_tenant_isolation.py diff --git a/forail/main/tenancy/isolation.py b/forail/main/tenancy/isolation.py index ea2dd4f..f1608ef 100644 --- a/forail/main/tenancy/isolation.py +++ b/forail/main/tenancy/isolation.py @@ -73,9 +73,25 @@ def __call__(self, request): try: tenant_org = self._resolve_tenant_org(request) except Exception: - # Resolving the user's tenant org failed. A user we cannot place - # into a tenant is treated as unscoped (superuser / non-tenant), - # which is the pre-existing contract for a None result. + # Fail CLOSED, for the same reason set_tenant_id does below. + # + # This used to swallow the error and continue with tenant_org=None, + # which __call__ cannot tell apart from "superuser / not a tenant + # user" -- so a transient database error during the org lookup let + # the request run with NO RLS scope, and RLS reads an unset tenant + # id as "all rows". A failed lookup is not evidence that the user + # has no tenant; it is evidence that we do not know. + # + # Only meaningful when tenancy and RLS are on: with either off, + # _resolve_tenant_org returns before it touches the database, so an + # exception here cannot come from the lookup and a single-tenant + # install is not turned into a 500. + if getattr(settings, 'TENANCY_ENABLED', False) and getattr(settings, 'TENANCY_RLS_ENABLED', False): + logger.exception('TenantIsolationMiddleware: tenant org resolution failed — failing closed') + return JsonResponse( + {'detail': 'Tenant isolation could not be established.'}, + status=500, + ) logger.debug('TenantIsolationMiddleware: tenant org resolution failed', exc_info=True) tenant_org = None @@ -214,14 +230,14 @@ def _resolve_tenant_org(request): if getattr(user, 'is_superuser', False): return None - try: - orgs = list( - user.organizations.filter(is_tenant_root=True) - .only('pk', 'is_tenant_root', 'tenant_isolation_strict')[:1] - ) - except Exception: - logger.debug('_resolve_tenant_org: org lookup failed', exc_info=True) - return None + # Deliberately not wrapped in try/except. A database error here must + # reach __call__, which fails the request closed; returning None would + # make "the lookup broke" indistinguishable from "this user has no + # tenant", and the second answer runs the request unscoped. + orgs = list( + user.organizations.filter(is_tenant_root=True) + .only('pk', 'is_tenant_root', 'tenant_isolation_strict')[:1] + ) return orgs[0] if orgs else None diff --git a/tests_standalone/test_tenant_isolation.py b/tests_standalone/test_tenant_isolation.py new file mode 100644 index 0000000..47c29fc --- /dev/null +++ b/tests_standalone/test_tenant_isolation.py @@ -0,0 +1,182 @@ +"""Standalone tests for the tenant isolation middleware. + +Loads forail/main/tenancy/isolation.py directly with Django and the RLS module +stubbed -- no database, no settings module. The point of interest is what the +middleware does when it *cannot* determine a request's tenant: RLS reads an +unset tenant id as "every row", so the difference between "this user has no +tenant" and "the lookup failed" is the difference between a scoped request and +a global one. +""" + +import importlib.util +import os +import sys +import types +import unittest +from unittest.mock import MagicMock + + +def _load(mod_name, rel_path): + path = os.path.abspath(os.path.join(os.path.dirname(__file__), '..', rel_path)) + spec = importlib.util.spec_from_file_location(mod_name, path) + mod = importlib.util.module_from_spec(spec) + sys.modules[mod_name] = mod + spec.loader.exec_module(mod) + return mod + + +class _Settings: + """Stands in for django.conf.settings; only the flags read here matter.""" + + def __init__(self, **flags): + self.__dict__.update(flags) + + +class _JsonResponse: + """Records what the middleware answered instead of running the view.""" + + def __init__(self, payload, status=200): + self.payload = payload + self.status_code = status + + +settings_holder = _Settings() + +django = types.ModuleType('django') +django_conf = types.ModuleType('django.conf') +django_conf.settings = settings_holder +django_http = types.ModuleType('django.http') +django_http.JsonResponse = _JsonResponse +sys.modules.setdefault('django', django) +sys.modules['django.conf'] = django_conf +sys.modules['django.http'] = django_http + +# The RLS module talks to Postgres; the middleware only needs the two calls. +set_tenant_id = MagicMock() +clear_tenant_id = MagicMock() +rls = types.ModuleType('forail.main.tenancy.rls') +rls.set_tenant_id = set_tenant_id +rls.clear_tenant_id = clear_tenant_id + +for pkg in ('forail', 'forail.main', 'forail.main.tenancy'): + sys.modules.setdefault(pkg, types.ModuleType(pkg)) +sys.modules['forail.main.tenancy.rls'] = rls +_load('forail.main.tenancy.helpers', 'forail/main/tenancy/helpers.py') + +isolation = _load('forail.main.tenancy.isolation', 'forail/main/tenancy/isolation.py') +TenantIsolationMiddleware = isolation.TenantIsolationMiddleware + + +def tenant_user(is_superuser=False): + user = MagicMock() + user.is_authenticated = True + user.is_superuser = is_superuser + return user + + +def request_for(user): + request = MagicMock() + request.user = user + request.path = '/api/v2/job_templates/' + return request + + +class TenantResolutionFailure(unittest.TestCase): + """H3: a failed lookup must not read as 'this user has no tenant'.""" + + def setUp(self): + set_tenant_id.reset_mock() + clear_tenant_id.reset_mock() + settings_holder.__dict__.clear() + settings_holder.TENANCY_ENABLED = True + settings_holder.TENANCY_RLS_ENABLED = True + self.view = MagicMock(return_value='view-response') + self.mw = TenantIsolationMiddleware(self.view) + + def _fail_resolution(self): + self.mw._resolve_tenant_org = MagicMock(side_effect=RuntimeError('database is down')) + + def test_failed_lookup_does_not_run_the_view(self): + self._fail_resolution() + response = self.mw(request_for(tenant_user())) + self.assertEqual(response.status_code, 500) + self.view.assert_not_called() + + def test_failed_lookup_never_installs_a_scope(self): + self._fail_resolution() + self.mw(request_for(tenant_user())) + set_tenant_id.assert_not_called() + + def test_failed_lookup_is_ignored_when_tenancy_is_off(self): + # A single-tenant install has no scope to lose, and must not start + # answering 500 because of this gate. + settings_holder.TENANCY_ENABLED = False + self._fail_resolution() + self.assertEqual(self.mw(request_for(tenant_user())), 'view-response') + + def test_failed_lookup_is_ignored_when_rls_is_off(self): + settings_holder.TENANCY_RLS_ENABLED = False + self._fail_resolution() + self.assertEqual(self.mw(request_for(tenant_user())), 'view-response') + + def test_resolver_lets_a_database_error_out(self): + # The middleware can only fail closed if the lookup stops swallowing. + user = tenant_user() + user.organizations.filter.side_effect = RuntimeError('database is down') + request = request_for(user) + with self.assertRaises(RuntimeError): + TenantIsolationMiddleware._resolve_tenant_org(request) + + +class TenantResolutionSuccess(unittest.TestCase): + """The paths that must keep working unchanged.""" + + def setUp(self): + set_tenant_id.reset_mock() + clear_tenant_id.reset_mock() + settings_holder.__dict__.clear() + settings_holder.TENANCY_ENABLED = True + settings_holder.TENANCY_RLS_ENABLED = True + self.view = MagicMock(return_value='view-response') + self.mw = TenantIsolationMiddleware(self.view) + + def test_resolved_org_scopes_the_request(self): + org = MagicMock(pk=42) + self.mw._resolve_tenant_org = MagicMock(return_value=org) + request = request_for(tenant_user()) + self.assertEqual(self.mw(request), 'view-response') + set_tenant_id.assert_called_once_with(42) + clear_tenant_id.assert_called_once() + self.assertIs(request._tenant_org, org) + + def test_no_tenant_org_runs_unscoped(self): + # A superuser or a non-tenant user legitimately has no scope. + self.mw._resolve_tenant_org = MagicMock(return_value=None) + self.assertEqual(self.mw(request_for(tenant_user(is_superuser=True))), 'view-response') + set_tenant_id.assert_not_called() + + def test_set_tenant_id_failure_still_fails_closed(self): + # Pre-existing behaviour, kept under test alongside the new path. + self.mw._resolve_tenant_org = MagicMock(return_value=MagicMock(pk=7)) + set_tenant_id.side_effect = RuntimeError('cannot set session variable') + try: + response = self.mw(request_for(tenant_user())) + finally: + set_tenant_id.side_effect = None + self.assertEqual(response.status_code, 500) + self.view.assert_not_called() + + def test_superuser_resolver_returns_none_without_a_query(self): + user = tenant_user(is_superuser=True) + self.assertIsNone(TenantIsolationMiddleware._resolve_tenant_org(request_for(user))) + user.organizations.filter.assert_not_called() + + def test_resolver_returns_none_when_tenancy_is_off(self): + settings_holder.TENANCY_ENABLED = False + user = tenant_user() + self.assertIsNone(TenantIsolationMiddleware._resolve_tenant_org(request_for(user))) + user.organizations.filter.assert_not_called() + + +if __name__ == '__main__': + unittest.main() From beae609d473d4899664d1130cf0f76f703fcbc77 Mon Sep 17 00:00:00 2001 From: Krstan Vjestica Date: Mon, 17 Aug 2026 09:25:00 +0200 Subject: [PATCH 2/3] fix: RLS treated every NULL organization as a globally shared row Both policy builders emitted `OR organization_id IS NULL` for every table, on the reasoning that a NULL organization marks an AWX resource shared platform-wide. That is true of a handful of inherited tables and of none of ours: 20 of the 22 covered tables have a nullable organization, so a single row saved without one -- a scan result, a drift alert, an audit event, an inventory -- was readable by every tenant. One missed assignment was enough. Which tables mean it is now explicit. `RLS_GLOBAL_NULL_ORG_TABLES` lists the five where a NULL organization is a real sharing or ownership mechanism: credentials and OAuth applications owned by a user rather than an org, globally shared execution environments, and system job templates and their runs. Adding to that list is a decision that those rows may be read by any tenant. Everything else is scoped strictly, including every Forail-authored table -- that is where the organization is assigned by our own code, so a NULL is an unset field rather than a convention. The indirect policy follows the parent table, since the parent is the row that carries the organization; the orphan bypass (`fk IS NULL`) stays, because a row with no parent has no organization to compare against and hiding it would make it unreachable rather than unscoped. Behaviour change, stated plainly in migration 0211: on a tenant-scoped request, existing rows with no organization in a strictly-scoped table stop being returned. They are not deleted, and superusers and unscoped requests still see them. Hidden is the safe direction for an ambiguous row; visible-to-every-tenant is not. If a resource disappears for a tenant, assign its organization. --- .../migrations/0211_rls_scoped_null_org.py | 53 +++++++++++++++ forail/main/tenancy/helpers.py | 66 ++++++++++++++++--- tests_standalone/test_tenancy.py | 57 +++++++++++++++- 3 files changed, 164 insertions(+), 12 deletions(-) create mode 100644 forail/main/migrations/0211_rls_scoped_null_org.py diff --git a/forail/main/migrations/0211_rls_scoped_null_org.py b/forail/main/migrations/0211_rls_scoped_null_org.py new file mode 100644 index 0000000..3f5f329 --- /dev/null +++ b/forail/main/migrations/0211_rls_scoped_null_org.py @@ -0,0 +1,53 @@ +"""Multi-Tenancy v2: stop treating every NULL organization as globally shared. + +Codex M6 — the policies emitted ``OR organization_id IS NULL`` for every table, +so a row saved without an organization was readable by every tenant. Whether a +NULL organization means "shared with the platform" is now decided per table by +``RLS_GLOBAL_NULL_ORG_TABLES``; the rest are scoped strictly. + +This changes visibility for existing rows: on a tenant-scoped request, rows with +no organization in a strictly-scoped table stop being returned. They are not +deleted and remain visible to superusers and to any request without a tenant +context. If a resource "disappears" for a tenant after this migration, the fix +is to assign its organization -- not to add its table to the global list. + +Idempotent: each policy is dropped (IF EXISTS) then recreated, for both direct +and indirect tables, so this converges on fresh and existing databases. +""" + +from django.db import migrations + +from forail.main.tenancy.helpers import ( + RLS_TABLES_DIRECT, + RLS_TABLES_INDIRECT, + build_rls_policy_sql, + build_rls_policy_sql_indirect, +) + + +def _rebuild_sql(): + statements = [] + for table, org_col in RLS_TABLES_DIRECT: + create, drop = build_rls_policy_sql(table, org_col) + statements.append(f'ALTER TABLE {table} ENABLE ROW LEVEL SECURITY;') + statements.append(f'ALTER TABLE {table} FORCE ROW LEVEL SECURITY;') + statements.append(drop) + statements.append(create) + for table, fk_col, parent_table, parent_org_col in RLS_TABLES_INDIRECT: + create, drop = build_rls_policy_sql_indirect(table, fk_col, parent_table, parent_org_col) + statements.append(f'ALTER TABLE {table} ENABLE ROW LEVEL SECURITY;') + statements.append(f'ALTER TABLE {table} FORCE ROW LEVEL SECURITY;') + statements.append(drop) + statements.append(create) + return '\n'.join(statements) + + +class Migration(migrations.Migration): + + dependencies = [ + ('main', '0210_rls_nullif_cast'), + ] + + operations = [ + migrations.RunSQL(sql=_rebuild_sql(), reverse_sql=migrations.RunSQL.noop), + ] diff --git a/forail/main/tenancy/helpers.py b/forail/main/tenancy/helpers.py index 6175475..c9dadfe 100644 --- a/forail/main/tenancy/helpers.py +++ b/forail/main/tenancy/helpers.py @@ -232,12 +232,11 @@ def tenant_queue_name(org_id): # that references Organization.id — directly ('organization_id') for most # tables or via a subquery for indirect relationships. # -# Tables with nullable organization_id are included; the RLS policy handles -# NULLs by treating them as "visible to everyone" (no tenant scope). This is -# intentional for AWX's genuinely global, org-less resources (a credential or -# label created with organization=NULL is shared platform-wide). Do NOT add a -# table here whose rows are always tenant-owned unless it forbids NULL org, or -# those NULL rows would leak across every tenant (see needtofix M5). +# Tables with nullable organization_id are included. Whether a NULL organization +# means "shared with the whole platform" or "nobody set it" is decided per table +# by RLS_GLOBAL_NULL_ORG_TABLES below -- it used to be assumed to mean the first +# for every table, which is what made a single missed assignment enough to turn +# a tenant-owned row into cross-tenant data (see needtofix M5, Codex M6). # # Coverage note (needtofix M4): several models that *look* org-scoped # (Project, WorkflowJobTemplate, Schedule, workflow nodes, job events) do not @@ -287,12 +286,51 @@ def tenant_queue_name(org_id): ] +# Tables where `organization_id IS NULL` is a deliberate sharing or ownership +# mechanism inherited from AWX, and the row really is meant to be visible +# outside any one tenant: +# +# main_credential NULL org = a user-owned (personal) credential +# main_oauth2application NULL org = a user-owned application +# main_executionenvironment NULL org = a globally shared execution environment +# main_unifiedjobtemplate system job templates carry no organization +# main_unifiedjob ...and neither do their runs +# +# Everything else is scoped strictly: a NULL organization there is an unset +# field, not a shared resource, and the row stays invisible to a tenant-scoped +# request. Hidden is the safe direction for an ambiguous row; visible-to-all is +# not. An unscoped request (superuser, or no tenant context) still sees it, so +# nothing becomes unreachable -- and every Forail-authored table is in this +# group, which is where an org is assigned by our own code rather than by AWX's +# long-standing conventions. +# +# Adding a table here is a decision that its NULL rows may be read by every +# tenant. Do not add one to silence a "resource disappeared" report; assign the +# organization instead. +RLS_GLOBAL_NULL_ORG_TABLES = frozenset( + { + 'main_credential', + 'main_oauth2application', + 'main_executionenvironment', + 'main_unifiedjobtemplate', + 'main_unifiedjob', + } +) + + +def null_org_is_global(table): + """Whether rows of ``table`` with no organization are visible to every tenant.""" + return table in RLS_GLOBAL_NULL_ORG_TABLES + + def build_rls_policy_sql(table, org_column='organization_id'): """Return (create_sql, drop_sql) for a permissive RLS policy. The policy allows the row when: 1. ``organization_id`` matches ``forail.current_tenant_id``, OR - 2. ``organization_id`` IS NULL (global/shared resources), OR + 2. ``organization_id`` IS NULL **and** this table is listed in + ``RLS_GLOBAL_NULL_ORG_TABLES`` (a NULL org is a sharing mechanism there, + not an unset field), OR 3. The session variable is empty / unset (no tenant context — backwards compatible for non-tenant requests and superusers). """ @@ -303,12 +341,13 @@ def build_rls_policy_sql(table, org_column='organization_id'): # before the cast is evaluated. NULLIF collapses '' to NULL, which casts # cleanly and is caught by the IS NULL branch (→ all rows visible). tenant = "NULLIF(current_setting('forail.current_tenant_id', true), '')" + null_branch = f'OR {org_column} IS NULL ' if null_org_is_global(table) else '' create = ( f'CREATE POLICY {policy_name} ON {table} ' f'AS PERMISSIVE FOR ALL ' f'USING (' f'{org_column} = {tenant}::int ' - f'OR {org_column} IS NULL ' + f'{null_branch}' f'OR {tenant} IS NULL' f');' ) @@ -319,10 +358,17 @@ def build_rls_policy_sql(table, org_column='organization_id'): def build_rls_policy_sql_indirect(table, fk_column, parent_table, parent_org_column): """Return (create_sql, drop_sql) for an indirect RLS policy. - Uses a subquery to resolve the organization from a parent table. + Uses a subquery to resolve the organization from a parent table. Whether a + NULL organization on the *parent* is global is decided by the parent's entry + in ``RLS_GLOBAL_NULL_ORG_TABLES``, since that is the row that carries it. + + ``{fk_column} IS NULL`` stays unconditionally: a row with no parent at all + has no organization to compare against, and hiding it would make orphans + unreachable rather than merely unscoped. """ policy_name = f'tenant_isolation_{table}' tenant = "NULLIF(current_setting('forail.current_tenant_id', true), '')" + parent_null_branch = f'OR {parent_org_column} IS NULL' if null_org_is_global(parent_table) else '' create = ( f'CREATE POLICY {policy_name} ON {table} ' f'AS PERMISSIVE FOR ALL ' @@ -330,7 +376,7 @@ def build_rls_policy_sql_indirect(table, fk_column, parent_table, parent_org_col f'{fk_column} IN (' f'SELECT id FROM {parent_table} WHERE ' f'{parent_org_column} = {tenant}::int ' - f'OR {parent_org_column} IS NULL' + f'{parent_null_branch}' f') ' f'OR {fk_column} IS NULL ' f'OR {tenant} IS NULL' diff --git a/tests_standalone/test_tenancy.py b/tests_standalone/test_tenancy.py index 2943e03..0f1cb97 100644 --- a/tests_standalone/test_tenancy.py +++ b/tests_standalone/test_tenancy.py @@ -197,6 +197,8 @@ def test_non_dict(self): build_rls_policy_sql_indirect = helpers.build_rls_policy_sql_indirect RLS_TABLES_DIRECT = helpers.RLS_TABLES_DIRECT RLS_TABLES_INDIRECT = helpers.RLS_TABLES_INDIRECT +RLS_GLOBAL_NULL_ORG_TABLES = helpers.RLS_GLOBAL_NULL_ORG_TABLES +null_org_is_global = helpers.null_org_is_global class TestBuildRlsPolicySql(unittest.TestCase): @@ -221,8 +223,46 @@ def test_create_contains_bypass_clauses(self): create, _ = build_rls_policy_sql('main_inventory', 'organization_id') # Bypass when session var is unset/empty — NULLIF('') collapses to NULL. self.assertIn("NULLIF(current_setting('forail.current_tenant_id', true), '') IS NULL", create) - # Bypass when org column is NULL (shared resources) - self.assertIn('organization_id IS NULL', create) + + def test_null_org_bypass_only_for_globally_shared_tables(self): + # Codex M6: this branch used to be emitted for every table, so one row + # saved without an organization was readable by every tenant. + shared, _ = build_rls_policy_sql('main_credential', 'organization_id') + scoped, _ = build_rls_policy_sql('main_scanner', 'organization_id') + self.assertIn('OR organization_id IS NULL', shared) + self.assertNotIn('OR organization_id IS NULL', scoped) + + def test_every_forail_table_is_scoped_strictly(self): + # The Forail-authored tables assign the organization in our own code, + # so a NULL there is an unset field rather than AWX's sharing mechanism. + forail_tables = [ + 'main_eventrule', 'main_outboundwebhook', 'main_eventlog', + 'main_hostfactsnapshot', 'main_driftdetection', 'main_driftalertrule', + 'main_driftalert', 'main_policy', 'main_policydecision', 'main_scanner', + 'main_scanresult', 'main_servicecatalogitem', 'main_auditevent', + ] + for table in forail_tables: + self.assertFalse(null_org_is_global(table), f'{table} must not expose NULL-org rows') + create, _ = build_rls_policy_sql(table, 'organization_id') + self.assertNotIn('OR organization_id IS NULL', create) + + def test_global_tables_are_a_closed_list(self): + # Every entry is a deliberate decision that NULL-org rows may be read by + # any tenant; the assertion is here so growing the list is not quiet. + self.assertEqual( + RLS_GLOBAL_NULL_ORG_TABLES, + frozenset({ + 'main_credential', + 'main_oauth2application', + 'main_executionenvironment', + 'main_unifiedjobtemplate', + 'main_unifiedjob', + }), + ) + + def test_every_global_table_is_actually_under_rls(self): + covered = {table for table, _ in RLS_TABLES_DIRECT} + self.assertTrue(RLS_GLOBAL_NULL_ORG_TABLES.issubset(covered)) def test_create_is_permissive(self): create, _ = build_rls_policy_sql('main_inventory', 'organization_id') @@ -265,6 +305,19 @@ def test_create_contains_bypass(self): # needtofix L6: NULLIF-guarded empty/unset sentinel (was `= ''`). self.assertIn("NULLIF(current_setting('forail.current_tenant_id', true), '') IS NULL", create) + def test_parent_null_org_follows_the_parent_table(self): + # A host is visible through its inventory, so whether a NULL org is + # global is the inventory's question, not the host's. main_inventory is + # not globally shared, so hosts of an org-less inventory stay hidden + # from a scoped tenant. + create, _ = build_rls_policy_sql_indirect( + 'main_host', 'inventory_id', 'main_inventory', 'organization_id' + ) + self.assertNotIn('OR organization_id IS NULL)', create) + # An orphan row keeps its bypass: no parent means no organization to + # compare, and hiding it would make it unreachable rather than unscoped. + self.assertIn('OR inventory_id IS NULL', create) + def test_drop_contains_policy_name(self): _, drop = build_rls_policy_sql_indirect( 'main_host', 'inventory_id', 'main_inventory', 'organization_id' From 0578fc62302551392ad65fee5fc6f2be5e547f60 Mon Sep 17 00:00:00 2001 From: Krstan Vjestica Date: Tue, 18 Aug 2026 13:05:00 +0200 Subject: [PATCH 3/3] fix: only one of the four tenancy controls could be set from the environment `_ENV_SYNC_KEYS` mirrored `TENANCY_ENABLED` into the Setting registry and nothing else. `TENANCY_RLS_ENABLED`, `TENANCY_STRICT_ISOLATION_ENABLED` and `TENANCY_RATE_LIMITING_ENABLED` all default to False and are the flags that actually enforce anything -- so a deployment could set the one switch it was offered, reasonably expect isolation, and get the tenancy features running with no row-level security behind them and no way to turn it on short of the Settings UI. All four now sync, and all four are parsed as booleans: without that the Setting row would hold the string "false", which is truthy at every point it is read. Pairs with the chart change that exposes them as Helm values and refuses an install that enables tenancy without RLS. --- forail/main/observability/bootstrap.py | 15 +++++++++++- tests_standalone/test_observability.py | 33 ++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/forail/main/observability/bootstrap.py b/forail/main/observability/bootstrap.py index 96f5a32..a068d4b 100644 --- a/forail/main/observability/bootstrap.py +++ b/forail/main/observability/bootstrap.py @@ -23,9 +23,22 @@ 'OTEL_TRACES_SAMPLER', 'OTEL_TRACES_SAMPLER_ARG', 'TENANCY_ENABLED', + # Codex M2: TENANCY_ENABLED synced but the controls that actually enforce + # isolation did not, so a deployment could set the one flag it was offered + # and get tenancy features with no row-level security behind them. Turning + # tenancy on through the environment now reaches the whole set. + 'TENANCY_RLS_ENABLED', + 'TENANCY_STRICT_ISOLATION_ENABLED', + 'TENANCY_RATE_LIMITING_ENABLED', ) -_BOOL_KEYS = ('OTEL_ENABLED', 'TENANCY_ENABLED') +_BOOL_KEYS = ( + 'OTEL_ENABLED', + 'TENANCY_ENABLED', + 'TENANCY_RLS_ENABLED', + 'TENANCY_STRICT_ISOLATION_ENABLED', + 'TENANCY_RATE_LIMITING_ENABLED', +) def _coerce_env_value(key, raw): diff --git a/tests_standalone/test_observability.py b/tests_standalone/test_observability.py index 9d37f42..b7f54c6 100644 --- a/tests_standalone/test_observability.py +++ b/tests_standalone/test_observability.py @@ -22,6 +22,7 @@ def _load(mod_name, rel_path): helpers = _load('obs_helpers', 'forail/main/observability/helpers.py') +bootstrap = _load('obs_bootstrap', 'forail/main/observability/bootstrap.py') parse_resource_attributes = helpers.parse_resource_attributes parse_endpoint = helpers.parse_endpoint @@ -152,3 +153,35 @@ def test_should_recheck_is_inverse(self): if __name__ == '__main__': unittest.main() + + +class TestEnvSyncKeys(unittest.TestCase): + """Which settings a deployment can drive from the environment. + + Codex M2: the chart offered one tenancy switch, and only that one was + mirrored into the Setting registry -- so an operator who turned tenancy on + got the tenancy features with no row-level security behind them, because + TENANCY_RLS_ENABLED stayed at its False default with no way to reach it. + """ + + TENANCY_KEYS = ( + 'TENANCY_ENABLED', + 'TENANCY_RLS_ENABLED', + 'TENANCY_STRICT_ISOLATION_ENABLED', + 'TENANCY_RATE_LIMITING_ENABLED', + ) + + def test_every_tenancy_control_is_syncable(self): + for key in self.TENANCY_KEYS: + self.assertIn(key, bootstrap._ENV_SYNC_KEYS, f'{key} cannot be set from the environment') + + def test_every_tenancy_control_is_parsed_as_a_boolean(self): + # Without this the Setting row stores the string "false", which is + # truthy everywhere it is read. + for key in self.TENANCY_KEYS: + self.assertIn(key, bootstrap._BOOL_KEYS) + self.assertIs(bootstrap._coerce_env_value(key, 'false'), False) + self.assertIs(bootstrap._coerce_env_value(key, 'true'), True) + + def test_non_bool_keys_stay_strings(self): + self.assertEqual(bootstrap._coerce_env_value('OTEL_SERVICE_NAME', 'forail'), 'forail')