diff --git a/spp_cel_domain/README.rst b/spp_cel_domain/README.rst index 9a72fa2b4..db53c5060 100644 --- a/spp_cel_domain/README.rst +++ b/spp_cel_domain/README.rst @@ -142,6 +142,31 @@ Dependencies Changelog ========= +19.0.2.1.2 +~~~~~~~~~~ + +- fix: recognise ``me`` as a CEL context identifier. The resolver + rewrites cached variables (and the DCI override rewrites dotted + accessors) into ``metric('', me)`` before identifiers are + extracted; because ``me`` was missing from + ``CEL_CONTEXT_IDENTIFIERS``, ``validate_expression`` / + ``validate_formula_expression`` wrongly reported valid expressions as + ``Undefined variables: me``. ``me`` is the individual record proxy in + the eval context, so it is now a recognised context identifier. + +19.0.2.1.1 +~~~~~~~~~~ + +- fix(security): key metric cache lookups strictly by the requested + params. The provider clause used to fall back to param-agnostic cache + rows (``(provider, "")`` and ``("", "")``), so a parameterized + ``metric(..., arg=…)`` predicate could be satisfied by an + unparameterized/legacy cached value — silently selecting subjects by a + less-specific value in eligibility/targeting/DCI-search flows. Reads + are now keyed by the exact ``params_hash`` (both the freshness + preflight and the SQL fast path), and the compute/refresh path + re-caches under the correct params key. + 19.0.2.1.0 ~~~~~~~~~~ diff --git a/spp_cel_domain/__manifest__.py b/spp_cel_domain/__manifest__.py index 550c3d0f3..0fe8b9919 100644 --- a/spp_cel_domain/__manifest__.py +++ b/spp_cel_domain/__manifest__.py @@ -2,7 +2,7 @@ { "name": "CEL Domain Query Builder", "summary": "Write simple CEL-like expressions to filter records (OpenSPP/OpenG2P friendly)", - "version": "19.0.2.1.0", + "version": "19.0.2.1.2", "license": "LGPL-3", "development_status": "Production/Stable", "author": "OpenSPP.org, OpenSPP Community", diff --git a/spp_cel_domain/models/cel_executor.py b/spp_cel_domain/models/cel_executor.py index b9fb36003..a0241e02e 100644 --- a/spp_cel_domain/models/cel_executor.py +++ b/spp_cel_domain/models/cel_executor.py @@ -1,5 +1,6 @@ from __future__ import annotations +import inspect import logging from collections.abc import Iterable, Iterator from typing import Any @@ -1218,7 +1219,9 @@ def _exec_metric( if not batch_ids: continue total_requested += len(batch_ids) - batch_values, batch_stats = svc.evaluate(p.metric, subject_model, batch_ids, period_key, mode=eval_mode) + batch_values, batch_stats = self._svc_evaluate_batch( # pragma: no cover + svc, p, subject_model, batch_ids, period_key, eval_mode + ) aggregated_values.update(batch_values) if batch_stats: stats_total["cache_hits"] += int(batch_stats.get("cache_hits") or 0) @@ -1463,6 +1466,39 @@ def _feature_value_subquery( ) return SQL("(%s)", SQL(sql, *args)) + @staticmethod + def _evaluate_accepts_params(svc) -> bool: + """Whether ``svc.evaluate`` accepts a ``params`` keyword argument. + + The evaluation service (``spp.indicator``) is provided by a legacy/external + module whose signature we do not control and which may predate the + ``params`` kwarg. Returns True when ``evaluate`` declares an explicit + ``params`` parameter or a ``**kwargs`` catch-all; False otherwise (so the + caller degrades to an unparameterized call instead of raising ``TypeError``). + """ + try: + sig = inspect.signature(svc.evaluate) + except (TypeError, ValueError): + return False + return any(prm.name == "params" or prm.kind is inspect.Parameter.VAR_KEYWORD for prm in sig.parameters.values()) + + def _svc_evaluate_batch(self, svc, p, subject_model, batch_ids, period_key, eval_mode): # pragma: no cover + """Call the legacy/external evaluation service for one batch. + + Threads the metric's params through so parameterized refreshes are computed + with the right params — but only when ``evaluate`` accepts a ``params`` kwarg + (see ``_evaluate_accepts_params``), degrading gracefully on older services. + + Not covered by tests: ``spp.indicator`` is not in this repo's dependency + closure, so this path is unreachable here; the params-compat decision is + unit-tested via ``_evaluate_accepts_params``. + """ + eval_kwargs = {"mode": eval_mode} + metric_params = getattr(p, "params", None) + if metric_params and self._evaluate_accepts_params(svc): + eval_kwargs["params"] = metric_params + return svc.evaluate(p.metric, subject_model, batch_ids, period_key, **eval_kwargs) + def _provider_clause(self, provider: str, params_hash: str, allow_any_provider: bool) -> tuple[str, list[Any]]: provider = provider or "" params_hash = params_hash or "" @@ -1470,10 +1506,12 @@ def _provider_clause(self, provider: str, params_hash: str, allow_any_provider: (provider, params_hash), ] if provider: + # Relax the provider (a routing/registry detail) but keep the requested + # params_hash. Params are a semantic filter, not a provider detail: a + # non-empty params_hash must never fall back to params_hash "" rows, or a + # parameterized metric would match unparameterized/legacy cache rows. When + # params_hash == "" this combo already covers the unparameterized rows. combos.append(("", params_hash)) - if params_hash: - combos.append((provider, "")) - combos.append(("", "")) # Deduplicate while preserving order seen = set() uniq_combos: list[tuple[str, str]] = [] diff --git a/spp_cel_domain/models/cel_variable_resolver.py b/spp_cel_domain/models/cel_variable_resolver.py index be60996d4..eb2672e4a 100644 --- a/spp_cel_domain/models/cel_variable_resolver.py +++ b/spp_cel_domain/models/cel_variable_resolver.py @@ -109,6 +109,7 @@ def _get_reserved_words(self): "or", "r", "m", + "me", "members", "enrollments", "entitlements", diff --git a/spp_cel_domain/readme/HISTORY.md b/spp_cel_domain/readme/HISTORY.md index 1b4fd10d3..294498269 100644 --- a/spp_cel_domain/readme/HISTORY.md +++ b/spp_cel_domain/readme/HISTORY.md @@ -1,3 +1,17 @@ +### 19.0.2.1.2 + +- fix: recognise `me` as a CEL context identifier. The resolver rewrites cached + variables (and the DCI override rewrites dotted accessors) into + `metric('', me)` before identifiers are extracted; because `me` was + missing from `CEL_CONTEXT_IDENTIFIERS`, `validate_expression` / + `validate_formula_expression` wrongly reported valid expressions as + `Undefined variables: me`. `me` is the individual record proxy in the eval + context, so it is now a recognised context identifier. + +### 19.0.2.1.1 + +- fix(security): key metric cache lookups strictly by the requested params. The provider clause used to fall back to param-agnostic cache rows (`(provider, "")` and `("", "")`), so a parameterized `metric(..., arg=…)` predicate could be satisfied by an unparameterized/legacy cached value — silently selecting subjects by a less-specific value in eligibility/targeting/DCI-search flows. Reads are now keyed by the exact `params_hash` (both the freshness preflight and the SQL fast path), and the compute/refresh path re-caches under the correct params key. + ### 19.0.2.1.0 - feat(sql): compile CEL ternary expressions to SQL CASE via `to_sql_case`, with `case_when`/`comparison` builders and a right-associative ternary parsing fix diff --git a/spp_cel_domain/services/cel_parser.py b/spp_cel_domain/services/cel_parser.py index 8222781bd..6d3e4b397 100644 --- a/spp_cel_domain/services/cel_parser.py +++ b/spp_cel_domain/services/cel_parser.py @@ -227,6 +227,7 @@ def __init__(self, kind: str, value: Any, pos: int): # ADR-008: Added 'r' as the standard prefix for current record access CEL_CONTEXT_IDENTIFIERS = { "m", + "me", "e", "r", "members", diff --git a/spp_cel_domain/static/description/index.html b/spp_cel_domain/static/description/index.html index f280ce2a1..d8db7a63b 100644 --- a/spp_cel_domain/static/description/index.html +++ b/spp_cel_domain/static/description/index.html @@ -522,6 +522,33 @@

Changelog

+

19.0.2.1.2

+
    +
  • fix: recognise me as a CEL context identifier. The resolver +rewrites cached variables (and the DCI override rewrites dotted +accessors) into metric('<accessor>', me) before identifiers are +extracted; because me was missing from +CEL_CONTEXT_IDENTIFIERS, validate_expression / +validate_formula_expression wrongly reported valid expressions as +Undefined variables: me. me is the individual record proxy in +the eval context, so it is now a recognised context identifier.
    • +
+
+
+

19.0.2.1.1

+
    +
  • fix(security): key metric cache lookups strictly by the requested +params. The provider clause used to fall back to param-agnostic cache +rows ((provider, "") and ("", "")), so a parameterized +metric(..., arg=…) predicate could be satisfied by an +unparameterized/legacy cached value — silently selecting subjects by a +less-specific value in eligibility/targeting/DCI-search flows. Reads +are now keyed by the exact params_hash (both the freshness +preflight and the SQL fast path), and the compute/refresh path +re-caches under the correct params key.
    • +
+
+

19.0.2.1.0

  • feat(sql): compile CEL ternary expressions to SQL CASE via @@ -533,7 +560,7 @@

    19.0.2.1.0

  • test(translator): add coverage for the CEL translation cache helpers
-
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • diff --git a/spp_cel_domain/tests/__init__.py b/spp_cel_domain/tests/__init__.py index f9eb6bff3..3388d4be2 100644 --- a/spp_cel_domain/tests/__init__.py +++ b/spp_cel_domain/tests/__init__.py @@ -32,3 +32,5 @@ from . import test_cel_relational_predicate from . import test_cel_smart_op_lookup from . import test_cel_translator_cache +from . import test_cel_me_identifier +from . import test_evaluate_accepts_params diff --git a/spp_cel_domain/tests/test_cel_me_identifier.py b/spp_cel_domain/tests/test_cel_me_identifier.py new file mode 100644 index 000000000..594bd0145 --- /dev/null +++ b/spp_cel_domain/tests/test_cel_me_identifier.py @@ -0,0 +1,46 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""`me` is a record-root context identifier and must not be flagged as an +undefined variable during validation. + +The resolver rewrites cached variables into ``metric('', me)`` and +the DCI override rewrites dotted accessors the same way *before* the base +resolver extracts identifiers. ``me`` then appears as a bare identifier in the +scanned expression; unless it is a recognized context identifier, +``validate_expression`` / ``validate_formula_expression`` wrongly report +``Undefined variables: me``. +""" + +from odoo.tests import TransactionCase, tagged + + +@tagged("post_install", "-at_install") +class TestMeContextIdentifier(TransactionCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.resolver = cls.env["spp.cel.variable.resolver"] + cls.service = cls.env["spp.cel.service"] + + def test_me_is_a_context_identifier(self): + from odoo.addons.spp_cel_domain.services.cel_parser import CEL_CONTEXT_IDENTIFIERS + + self.assertIn("me", CEL_CONTEXT_IDENTIFIERS) + + def test_expand_does_not_flag_me_as_missing(self): + result = self.resolver.expand_expression("metric('foo', me) == true") + self.assertNotIn("me", result["missing_variables"]) + + def test_validate_expression_accepts_bare_me(self): + result = self.resolver.validate_expression("metric('foo', me) == true") + self.assertTrue( + result["valid"], + f"expression with bare me should validate; errors: {result['errors']}", + ) + self.assertNotIn( + "Undefined variables: me", + " ".join(result["errors"]), + ) + + def test_validate_formula_expression_accepts_bare_me(self): + result = self.service.validate_formula_expression("metric('foo', me)", "individual") + self.assertNotIn("Missing variables: me", result.get("error") or "") diff --git a/spp_cel_domain/tests/test_evaluate_accepts_params.py b/spp_cel_domain/tests/test_evaluate_accepts_params.py new file mode 100644 index 000000000..62d47230a --- /dev/null +++ b/spp_cel_domain/tests/test_evaluate_accepts_params.py @@ -0,0 +1,51 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Unit tests for the params-compatibility guard used before calling the +legacy/external evaluation service (spp.indicator.evaluate). + +The metric compute path passes the metric's params to svc.evaluate only when +that method accepts a `params` kwarg, so a parameterized refresh is computed +with the right params without risking a TypeError on an older service that +predates the kwarg. See _evaluate_accepts_params / _svc_evaluate_batch in +cel_executor.py. +""" + +from odoo.tests.common import TransactionCase, tagged + + +class _EvalWithParams: + def evaluate(self, metric, model, ids, period_key, mode="fallback", params=None): + return {}, {} + + +class _EvalWithKwargs: + def evaluate(self, metric, model, ids, period_key, **kwargs): + return {}, {} + + +class _EvalNoParams: + def evaluate(self, metric, model, ids, period_key, mode="fallback"): + return {}, {} + + +class _NonCallableEvaluate: + evaluate = 42 # inspect.signature() raises TypeError -> degrade to False + + +@tagged("post_install", "-at_install") +class TestEvaluateAcceptsParams(TransactionCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.executor = cls.env["spp.cel.executor"] + + def test_explicit_params_arg_supported(self): + self.assertTrue(self.executor._evaluate_accepts_params(_EvalWithParams())) + + def test_var_keyword_supported(self): + self.assertTrue(self.executor._evaluate_accepts_params(_EvalWithKwargs())) + + def test_no_params_not_supported(self): + self.assertFalse(self.executor._evaluate_accepts_params(_EvalNoParams())) + + def test_uninspectable_evaluate_degrades_to_false(self): + self.assertFalse(self.executor._evaluate_accepts_params(_NonCallableEvaluate())) diff --git a/spp_dci_client/README.rst b/spp_dci_client/README.rst index e3ecc9a56..c27197ec1 100644 --- a/spp_dci_client/README.rst +++ b/spp_dci_client/README.rst @@ -140,6 +140,17 @@ Dependencies Changelog ========= +19.0.2.0.2 +~~~~~~~~~~ + +- fix(security): make the OAuth2 token/header methods private so they + are no longer callable over RPC — a low-privilege internal user can no + longer mint a DCI access token or obtain a Bearer header via + ``get_oauth2_token()`` / ``get_headers()``. Restrict the token cache + fields (``_oauth2_access_token`` / ``_oauth2_token_expires_at``) to + system administrators, and require write access to run a connection + test. + 19.0.2.0.0 ~~~~~~~~~~ diff --git a/spp_dci_client/__manifest__.py b/spp_dci_client/__manifest__.py index 457a40662..84f9afa46 100644 --- a/spp_dci_client/__manifest__.py +++ b/spp_dci_client/__manifest__.py @@ -2,7 +2,7 @@ { "name": "OpenSPP DCI Client", "summary": "Base DCI client infrastructure with OAuth2 and data source management", - "version": "19.0.2.0.1", + "version": "19.0.2.0.2", "category": "OpenSPP/Integration", "author": "OpenSPP.org", "website": "https://github.com/OpenSPP/OpenSPP2", diff --git a/spp_dci_client/models/data_source.py b/spp_dci_client/models/data_source.py index 71ff54fb5..454347cac 100644 --- a/spp_dci_client/models/data_source.py +++ b/spp_dci_client/models/data_source.py @@ -176,13 +176,17 @@ class DCIDataSource(models.Model): help="Last connection error message", ) - # OAuth2 token cache fields (transient storage) + # OAuth2 token cache fields (transient storage). Restricted to system + # administrators: the cached access token is a credential and must not be + # readable by ordinary internal users (who have read on this model). _oauth2_access_token = fields.Char( string="Cached Access Token", + groups="base.group_system", help="Cached OAuth2 access token (internal use only)", ) _oauth2_token_expires_at = fields.Datetime( string="Token Expiry", + groups="base.group_system", help="Cached token expiration timestamp (internal use only)", ) @@ -306,14 +310,18 @@ def _check_sender_id(self): if record.auth_type != "none" and not record.our_sender_id: raise ValidationError(_("Sender ID is required for authenticated connections.")) - def clear_oauth2_token_cache(self): + def _clear_oauth2_token_cache(self): """Clear cached OAuth2 token, forcing a fresh token request on next use. - This can be useful when the cached token becomes invalid or when - troubleshooting authentication issues. + Internal (underscore-prefixed) so it is NOT callable over RPC — otherwise + a low-privilege user could force repeated re-minting. It is invoked from + trusted server-side code (e.g. DCIClient on a 401 retry). The cache fields + are admin-restricted, so write via sudo: clearing the cache must work + regardless of the current user's privilege. """ self.ensure_one() - self.write( + # nosemgrep: odoo-sudo-without-context + self.sudo().write( { "_oauth2_access_token": False, "_oauth2_token_expires_at": False, @@ -321,9 +329,13 @@ def clear_oauth2_token_cache(self): ) _logger.info("Cleared OAuth2 token cache for data source: %s", self.code) - def get_oauth2_token(self, force_refresh=False): + def _get_oauth2_token(self, force_refresh=False): """Get or refresh OAuth2 access token. + Internal (underscore-prefixed) so it is NOT callable over RPC: it mints a + token from the administrator-only OAuth2 client secret, so it must only be + reachable from trusted server-side code (e.g. the DCIClient service). + Args: force_refresh: If True, skip cache and fetch a new token @@ -338,24 +350,26 @@ def get_oauth2_token(self, force_refresh=False): if self.auth_type != "oauth2": raise UserError(_("This data source does not use OAuth2 authentication.")) + # sudo(): the OAuth2 client secret and the token cache fields are + # restricted to administrators. This method is internal and only reached + # from trusted server-side code, so reading/writing them via sudo is safe. + sudo_self = self.sudo() # nosemgrep: odoo-sudo-without-context + # Check if cached token is still valid (with 60 second buffer) now = fields.Datetime.now() - if not force_refresh and self._oauth2_access_token and self._oauth2_token_expires_at: - expiry_with_buffer = self._oauth2_token_expires_at - timedelta(seconds=60) + if not force_refresh and sudo_self._oauth2_access_token and sudo_self._oauth2_token_expires_at: + expiry_with_buffer = sudo_self._oauth2_token_expires_at - timedelta(seconds=60) if now < expiry_with_buffer: - _logger.info( - "Using cached OAuth2 token for data source: %s (expires at %s)", - self.code, - self._oauth2_token_expires_at, - ) - return self._oauth2_access_token + # Do not log the token expiry field (it is a credential-adjacent + # cache field); log only the data source code. No secret reaches + # the log: the word "token" in the message alone trips the rule. + _logger.info("Using cached OAuth2 token for data source: %s", self.code) # nosemgrep: python.lang.security.audit.logging.logger-credential-leak.python-logger-credential-disclosure # noqa: E501 # fmt: skip + return sudo_self._oauth2_access_token # Request new token _logger.info("Requesting new OAuth2 token for data source: %s", self.code) try: - # Use sudo() to access OAuth2 credentials which are restricted to administrators - sudo_self = self.sudo() # nosemgrep: odoo-sudo-without-context token_data = { "grant_type": "client_credentials", "client_id": sudo_self.oauth2_client_id, @@ -446,9 +460,13 @@ def get_oauth2_token(self, force_refresh=False): # Show generic user-friendly message raise UserError(_("An unexpected error occurred. Please contact your administrator.")) from e - def get_headers(self, force_refresh_token=False): + def _get_headers(self, force_refresh_token=False): """Get HTTP headers for API requests including authentication. + Internal (underscore-prefixed) so it is NOT callable over RPC: it returns + an Authorization header carrying credentials minted from administrator-only + fields. Call it only from trusted server-side code (e.g. DCIClient). + Args: force_refresh_token: If True, force refresh OAuth2 token (skip cache) @@ -466,7 +484,7 @@ def get_headers(self, force_refresh_token=False): } _logger.debug( - "get_headers() called for data source %s, auth_type=%s, force_refresh=%s", + "_get_headers() called for data source %s, auth_type=%s, force_refresh=%s", self.code, self.auth_type, force_refresh_token, @@ -474,7 +492,7 @@ def get_headers(self, force_refresh_token=False): if self.auth_type == "oauth2": _logger.info("Fetching OAuth2 token for data source %s", self.code) - token = self.get_oauth2_token(force_refresh=force_refresh_token) + token = self._get_oauth2_token(force_refresh=force_refresh_token) headers["Authorization"] = f"Bearer {token}" _logger.info( "Added OAuth2 Authorization header for data source %s (token length: %d)", @@ -482,9 +500,14 @@ def get_headers(self, force_refresh_token=False): len(token) if token else 0, ) elif self.auth_type == "bearer": - if not self.bearer_token: + # bearer_token is admin-restricted (groups=base.group_system); read it + # via sudo so a non-admin internal caller (this method is not + # RPC-exposed) can build the header, matching the OAuth2 branch. + # nosemgrep: odoo-sudo-without-context + bearer_token = self.sudo().bearer_token + if not bearer_token: raise UserError(_("Bearer token is not configured for this data source.")) - headers["Authorization"] = f"Bearer {self.bearer_token}" + headers["Authorization"] = f"Bearer {bearer_token}" return headers @@ -499,10 +522,15 @@ def test_connection(self): """ self.ensure_one() + # Testing a connection mints/uses the data source's administrator-only + # credentials and makes an outbound call, so require management (write) + # access on the record — a read-only user must not trigger it. + self.check_access("write") + _logger.info("Testing connection to data source: %s (%s)", self.name, self.code) try: - headers = self.get_headers() + headers = self._get_headers() # Probe the authenticated ping endpoint. A 200 confirms both # reachability *and* that our credentials are accepted; a 401/403 diff --git a/spp_dci_client/readme/HISTORY.md b/spp_dci_client/readme/HISTORY.md index 4aaf9afef..9d07ae7c7 100644 --- a/spp_dci_client/readme/HISTORY.md +++ b/spp_dci_client/readme/HISTORY.md @@ -1,3 +1,7 @@ +### 19.0.2.0.2 + +- fix(security): make the OAuth2 token/header methods private so they are no longer callable over RPC — a low-privilege internal user can no longer mint a DCI access token or obtain a Bearer header via `get_oauth2_token()` / `get_headers()`. Restrict the token cache fields (`_oauth2_access_token` / `_oauth2_token_expires_at`) to system administrators, and require write access to run a connection test. + ### 19.0.2.0.0 - Initial migration to OpenSPP2 diff --git a/spp_dci_client/services/client.py b/spp_dci_client/services/client.py index 522f65f4b..e314493a5 100644 --- a/spp_dci_client/services/client.py +++ b/spp_dci_client/services/client.py @@ -956,8 +956,10 @@ def _make_request(self, endpoint: str, envelope: dict, _retry_auth: bool = True) """ url = f"{self.data_source.base_url.rstrip('/')}{endpoint}" - # Get headers from data source (includes auth) - headers = self.data_source.get_headers() + # Get headers from data source (includes auth). Internal method — the + # DCIClient service is the trusted server-side entry point for outbound + # DCI calls (get_headers/get_oauth2_token are not RPC-exposed). + headers = self.data_source._get_headers() # Track timing and result for outgoing log start_time = time.monotonic() @@ -999,7 +1001,7 @@ def _make_request(self, endpoint: str, envelope: dict, _retry_auth: bool = True) log_response_data = response.json() except json.JSONDecodeError: log_response_data = None - self.data_source.clear_oauth2_token_cache() + self.data_source._clear_oauth2_token_cache() return self._make_request(endpoint, envelope, _retry_auth=False) # Check for HTTP errors diff --git a/spp_dci_client/static/description/index.html b/spp_dci_client/static/description/index.html index 11e9ca70c..9c7558b5b 100644 --- a/spp_dci_client/static/description/index.html +++ b/spp_dci_client/static/description/index.html @@ -514,6 +514,18 @@

    Changelog

+

19.0.2.0.2

+
    +
  • fix(security): make the OAuth2 token/header methods private so they +are no longer callable over RPC — a low-privilege internal user can no +longer mint a DCI access token or obtain a Bearer header via +get_oauth2_token() / get_headers(). Restrict the token cache +fields (_oauth2_access_token / _oauth2_token_expires_at) to +system administrators, and require write access to run a connection +test.
    • +
+
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • diff --git a/spp_dci_client/tests/__init__.py b/spp_dci_client/tests/__init__.py index abee02b19..a6f1258b5 100644 --- a/spp_dci_client/tests/__init__.py +++ b/spp_dci_client/tests/__init__.py @@ -8,3 +8,5 @@ from . import test_data_source_validators from . import test_data_source_http from . import test_client_convenience + +from . import test_data_source_security diff --git a/spp_dci_client/tests/test_client_convenience.py b/spp_dci_client/tests/test_client_convenience.py index af64a25ef..e62dff642 100644 --- a/spp_dci_client/tests/test_client_convenience.py +++ b/spp_dci_client/tests/test_client_convenience.py @@ -211,7 +211,7 @@ def setUp(self): "message": {"q": 1}, } # Avoid auth HTTP in get_headers - p = patch.object(type(self.ds), "get_headers", return_value={"Content-Type": "application/json"}) + p = patch.object(type(self.ds), "_get_headers", return_value={"Content-Type": "application/json"}) p.start() self.addCleanup(p.stop) @@ -261,7 +261,7 @@ def test_make_request_oauth_401_retry(self): resp.raise_for_status.side_effect = httpx.HTTPStatusError( "401", request=MagicMock(), response=MagicMock(status_code=401, text="unauthorized") ) - with patch(HTTPX, return_value=_cm(resp)), patch.object(type(oauth_ds), "clear_oauth2_token_cache") as clear: + with patch(HTTPX, return_value=_cm(resp)), patch.object(type(oauth_ds), "_clear_oauth2_token_cache") as clear: with self.assertRaises(UserError): client._make_request("/sync/search", self.envelope) clear.assert_called_once() diff --git a/spp_dci_client/tests/test_data_source.py b/spp_dci_client/tests/test_data_source.py index 118ea632d..e88e68dc6 100644 --- a/spp_dci_client/tests/test_data_source.py +++ b/spp_dci_client/tests/test_data_source.py @@ -476,7 +476,7 @@ def test_get_oauth2_token_success(self, mock_client_class): mock_client_class.return_value = mock_client # Get token - token = ds.get_oauth2_token() + token = ds._get_oauth2_token() self.assertEqual(token, "test_token_12345") self.assertTrue(ds._oauth2_access_token) @@ -513,7 +513,7 @@ def test_get_oauth2_token_cached(self, mock_client_class): ) # Get token - should use cache - token = ds.get_oauth2_token() + token = ds._get_oauth2_token() self.assertEqual(token, "cached_token") # HTTP client should not be called @@ -531,7 +531,7 @@ def test_get_oauth2_token_wrong_auth_type(self): ) with self.assertRaises(UserError) as cm: - ds.get_oauth2_token() + ds._get_oauth2_token() self.assertIn("oauth2", str(cm.exception).lower()) @patch("httpx.Client") @@ -615,7 +615,7 @@ def test_get_headers_none_auth(self): } ) - headers = ds.get_headers() + headers = ds._get_headers() self.assertEqual(headers["Content-Type"], "application/json") self.assertEqual(headers["Accept"], "application/json") @@ -651,7 +651,7 @@ def test_get_headers_oauth2(self, mock_client_class): mock_client.__exit__.return_value = None mock_client_class.return_value = mock_client - headers = ds.get_headers() + headers = ds._get_headers() self.assertEqual(headers["Authorization"], "Bearer test_token_12345") diff --git a/spp_dci_client/tests/test_data_source_http.py b/spp_dci_client/tests/test_data_source_http.py index e93203fb1..3d7e5889c 100644 --- a/spp_dci_client/tests/test_data_source_http.py +++ b/spp_dci_client/tests/test_data_source_http.py @@ -56,7 +56,7 @@ def test_get_token_rejects_non_oauth2(self): } ) with self.assertRaises(UserError): - ds.get_oauth2_token() + ds._get_oauth2_token() def test_get_token_uses_valid_cache(self): ds = self._oauth_ds() @@ -67,7 +67,7 @@ def test_get_token_uses_valid_cache(self): } ) # No HTTP mock needed; cached token returned without a request. - self.assertEqual(ds.get_oauth2_token(), "cached-tok") + self.assertEqual(ds._get_oauth2_token(), "cached-tok") def test_get_token_fetches_new_via_body(self): ds = self._oauth_ds() @@ -75,7 +75,7 @@ def test_get_token_fetches_new_via_body(self): resp.raise_for_status = MagicMock() resp.json.return_value = {"access_token": "fresh-tok", "expires_in": 1800} with patch(HTTPX_CLIENT, return_value=_client_cm(resp)): - token = ds.get_oauth2_token() + token = ds._get_oauth2_token() self.assertEqual(token, "fresh-tok") self.assertEqual(ds.sudo()._oauth2_access_token, "fresh-tok") @@ -86,7 +86,7 @@ def test_get_token_query_credential_location(self): resp.json.return_value = {"access_token": "q-tok"} cm = _client_cm(resp) with patch(HTTPX_CLIENT, return_value=cm): - self.assertEqual(ds.get_oauth2_token(), "q-tok") + self.assertEqual(ds._get_oauth2_token(), "q-tok") # query mode posts with params=, not data= client = cm.__enter__.return_value _, kwargs = client.post.call_args @@ -99,7 +99,7 @@ def test_get_token_missing_access_token_raises(self): resp.json.return_value = {"no_token": "here"} with patch(HTTPX_CLIENT, return_value=_client_cm(resp)): with self.assertRaises(UserError): - ds.get_oauth2_token() + ds._get_oauth2_token() def test_get_token_http_status_error(self): ds = self._oauth_ds() @@ -108,7 +108,7 @@ def test_get_token_http_status_error(self): resp.raise_for_status.side_effect = httpx.HTTPStatusError("401", request=MagicMock(), response=err_resp) with patch(HTTPX_CLIENT, return_value=_client_cm(resp)): with self.assertRaises(UserError) as ctx: - ds.get_oauth2_token() + ds._get_oauth2_token() self.assertIn("Authentication failed", str(ctx.exception)) def test_get_token_request_error_timeout(self): @@ -118,15 +118,15 @@ def test_get_token_request_error_timeout(self): cm.__exit__.return_value = False with patch(HTTPX_CLIENT, return_value=cm): with self.assertRaises(UserError) as ctx: - ds.get_oauth2_token() + ds._get_oauth2_token() self.assertIn("timed out", str(ctx.exception).lower()) # --- get_headers --------------------------------------------------------- def test_get_headers_oauth2(self): ds = self._oauth_ds() - with patch.object(type(ds), "get_oauth2_token", return_value="abc"): - headers = ds.get_headers() + with patch.object(type(ds), "_get_oauth2_token", return_value="abc"): + headers = ds._get_headers() self.assertEqual(headers["Authorization"], "Bearer abc") def test_get_headers_bearer(self): @@ -140,7 +140,7 @@ def test_get_headers_bearer(self): "bearer_token": "btok", } ) - self.assertEqual(ds.get_headers()["Authorization"], "Bearer btok") + self.assertEqual(ds._get_headers()["Authorization"], "Bearer btok") # --- test_connection ----------------------------------------------------- diff --git a/spp_dci_client/tests/test_data_source_security.py b/spp_dci_client/tests/test_data_source_security.py new file mode 100644 index 000000000..fbae6f889 --- /dev/null +++ b/spp_dci_client/tests/test_data_source_security.py @@ -0,0 +1,101 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Security tests: low-privilege users must not be able to mint or read DCI +OAuth tokens, nor trigger credentialed connection tests. +""" + +from odoo import Command +from odoo.exceptions import AccessError +from odoo.tests import TransactionCase, tagged + + +@tagged("post_install", "-at_install") +class TestDataSourceCredentialAccess(TransactionCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.DataSource = cls.env["spp.dci.data.source"] + cls.user = cls.env["res.users"].create( + { + "name": "DCI Low-Priv User", + "login": "dci_lowpriv_user", + "group_ids": [Command.link(cls.env.ref("base.group_user").id)], + } + ) + cls.ds = cls.DataSource.create( + { + "name": "OAuth DS", + "code": "oauth_ds_sec", + "base_url": "https://dci.example.org/api", + "auth_type": "oauth2", + "our_sender_id": "openspp.test", + "oauth2_token_url": "https://auth.example.org/token", + "oauth2_client_id": "cid", + "oauth2_client_secret": "secret", + } + ) + + def test_token_methods_are_private(self): + """The credential methods must be private (underscore-prefixed). + + Odoo blocks RPC ``call_kw`` to underscore-prefixed methods (framework + guarantee), so making them private removes them as an RPC entry point. + This asserts the public names are gone (catching a re-added public alias) + and the private ones exist; the RPC-dispatch block itself is a framework + property of ``_``-prefixed names. + """ + model = self.env["spp.dci.data.source"] + self.assertFalse(hasattr(model, "get_oauth2_token"), "public get_oauth2_token must be removed") + self.assertFalse(hasattr(model, "get_headers"), "public get_headers must be removed") + self.assertTrue(hasattr(model, "_get_oauth2_token")) + self.assertTrue(hasattr(model, "_get_headers")) + + def test_cached_token_field_hidden_from_regular_user(self): + """The cached access token is a credential; it must not be visible to an + ordinary internal user (who has read on the model).""" + fields_for_user = self.ds.with_user(self.user).fields_get() + self.assertNotIn("_oauth2_access_token", fields_for_user) + self.assertNotIn("_oauth2_token_expires_at", fields_for_user) + # An administrator can see them (control). + fields_for_admin = self.ds.fields_get() + self.assertIn("_oauth2_access_token", fields_for_admin) + + def test_regular_user_cannot_read_cached_token(self): + """Even with a token cached, a regular user cannot read it back.""" + self.ds.sudo().write({"_oauth2_access_token": "super-secret-token"}) + with self.assertRaises(AccessError): + self.ds.with_user(self.user).read(["_oauth2_access_token"]) + + def test_test_connection_requires_write_access(self): + """test_connection mints/uses admin-only credentials and makes an + outbound call; a read-only user must not be able to trigger it.""" + with self.assertRaises(AccessError): + self.ds.with_user(self.user).test_connection() + + def test_action_test_connection_requires_write_access(self): + """The public button alias must inherit the same write gate.""" + with self.assertRaises(AccessError): + self.ds.with_user(self.user).action_test_connection() + + def test_regular_user_context_can_clear_token_cache(self): + """The internal cache-clear (used on a 401 retry) must work regardless of + the current user's privilege — it writes admin-restricted fields via sudo.""" + self.ds.sudo().write({"_oauth2_access_token": "some-token"}) + # Called from server-side code running in a non-admin user context. + self.ds.with_user(self.user)._clear_oauth2_token_cache() + self.assertFalse(self.ds.sudo()._oauth2_access_token) + + def test_regular_user_context_can_get_bearer_headers(self): + """A bearer-auth header must build in a non-admin user context: the + admin-only bearer_token is read via sudo inside the internal method.""" + bearer_ds = self.DataSource.create( + { + "name": "Bearer DS", + "code": "bearer_ds_sec", + "base_url": "https://dci.example.org/api", + "auth_type": "bearer", + "our_sender_id": "openspp.test", + "bearer_token": "secret-bearer-token", + } + ) + headers = bearer_ds.with_user(self.user)._get_headers() + self.assertEqual(headers.get("Authorization"), "Bearer secret-bearer-token") diff --git a/spp_dci_client/tests/test_outgoing_log_integration.py b/spp_dci_client/tests/test_outgoing_log_integration.py index 7cefec242..17dd66f17 100644 --- a/spp_dci_client/tests/test_outgoing_log_integration.py +++ b/spp_dci_client/tests/test_outgoing_log_integration.py @@ -381,7 +381,7 @@ def test_401_retry_creates_two_log_entries(self, mock_client_class): envelope = self._build_test_envelope(client) - with patch.object(ds, "clear_oauth2_token_cache"): + with patch.object(ds, "_clear_oauth2_token_cache"): client._make_request("/registry/sync/search", envelope) # Should have two log entries: one for 401, one for retry success diff --git a/spp_dci_client_compliance/README.rst b/spp_dci_client_compliance/README.rst index 32a137a5a..480aa009b 100644 --- a/spp_dci_client_compliance/README.rst +++ b/spp_dci_client_compliance/README.rst @@ -59,8 +59,10 @@ After installing: 1. Set system parameter ``dci.client_compliance.mock_registry_url`` to point to your mock registry (default: ``http://mock_registry:3335``) -2. Set system parameter ``dci.client_compliance.bearer_token`` for - authentication (default: ``compliance-test-api-key-12345``) +2. Set system parameter ``dci.client_compliance.bearer_token`` to a + **private** token for authentication. There is no default, and the + well-known value ``compliance-test-api-key-12345`` is rejected; the + trigger endpoints refuse to run until a private token is configured. 3. Verify test data source exists under **Settings > Technical > DCI > Configuration > Data Sources** (auto-created if missing) @@ -114,6 +116,18 @@ Dependencies .. contents:: :local: +Changelog +========= + +19.0.1.0.2 +~~~~~~~~~~ + +- fix(security): remove compliance data sources that still hold the old + shared bearer token on upgrade, refuse to serve any such record from + the trigger controller, and reject the well-known default token when + configured, so upgraded or freshly configured databases cannot use the + shared credential over the unauthenticated trigger routes. + Bug Tracker =========== diff --git a/spp_dci_client_compliance/__manifest__.py b/spp_dci_client_compliance/__manifest__.py index 0c1814cc7..3a0a993b7 100644 --- a/spp_dci_client_compliance/__manifest__.py +++ b/spp_dci_client_compliance/__manifest__.py @@ -2,7 +2,7 @@ { "name": "OpenSPP DCI Client Compliance Tests", "category": "OpenSPP", - "version": "19.0.1.0.1", + "version": "19.0.1.0.2", "sequence": 1, "author": "OpenSPP.org", "website": "https://github.com/OpenSPP/OpenSPP2", diff --git a/spp_dci_client_compliance/controllers/trigger.py b/spp_dci_client_compliance/controllers/trigger.py index 0c6712b6b..fcdd931fe 100644 --- a/spp_dci_client_compliance/controllers/trigger.py +++ b/spp_dci_client_compliance/controllers/trigger.py @@ -15,6 +15,8 @@ from odoo.exceptions import UserError from odoo.http import request +from ..models.data_source import DEFAULT_COMPLIANCE_BEARER_TOKEN + _logger = logging.getLogger(__name__) COMPLIANCE_ENABLED_PARAM = "dci.client_compliance.enabled" @@ -70,6 +72,18 @@ def _get_compliance_bearer_token(env): f"Set the system parameter {BEARER_TOKEN_PARAM!r} before " f"using the trigger endpoints." ) + # Refuse the well-known default token: it is public, so accepting it + # would recreate the exact exposure the 19.0.1.0.2 migration purges - + # a data source authenticating outbound requests with a shared secret. + # Plain equality against a PUBLIC sentinel - not a secret comparison, so + # timing analysis leaks nothing. + # nosemgrep: odoo-timing-attack-password + if token == DEFAULT_COMPLIANCE_BEARER_TOKEN: + raise UserError( + f"The DCI client compliance bearer token is set to the well-known " + f"default value, which is not allowed. Set {BEARER_TOKEN_PARAM!r} to a " + f"private token before using the trigger endpoints." + ) return token def _disabled_response(self): @@ -95,16 +109,24 @@ def _get_test_data_source(self): # nosemgrep: odoo-sudo-without-context DataSource = request.env["spp.dci.data.source"].sudo() + # Exclude any record still holding the well-known default token directly + # in the domain. Upgraded databases may retain such a record (see the + # 19.0.1.0.2 migration); serving it would re-expose the shared secret + # over the ``auth='none'`` routes. Filtering in the domain rather than + # post-search means a legitimately re-keyed record is still found even + # when a stale record sorts ahead of it under limit=1. + not_default = ("bearer_token", "!=", DEFAULT_COMPLIANCE_BEARER_TOKEN) + # First try to find one marked for compliance testing test_ds = DataSource.search( - [("is_compliance_test", "=", True)], + [("is_compliance_test", "=", True), not_default], limit=1, ) if not test_ds: # Fall back to one named "DCI Compliance Test" test_ds = DataSource.search( - [("name", "=", "DCI Compliance Test")], + [("name", "=", "DCI Compliance Test"), not_default], limit=1, ) diff --git a/spp_dci_client_compliance/migrations/19.0.1.0.2/post-migration.py b/spp_dci_client_compliance/migrations/19.0.1.0.2/post-migration.py new file mode 100644 index 000000000..613882f83 --- /dev/null +++ b/spp_dci_client_compliance/migrations/19.0.1.0.2/post-migration.py @@ -0,0 +1,34 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Remove compliance data sources that still hold the old shared bearer token. + +The 19.0.1.0.1 migration cleared only the ``ir.config_parameter`` copy of the +well-known token ``compliance-test-api-key-12345``. Earlier versions also +created an ``spp.dci.data.source`` record carrying that token in its own +``bearer_token`` column, which the trigger controller would keep using once the +compliance gate was re-enabled - re-exposing the shared secret over the +``auth='none'`` routes on upgraded databases. + +Delete any such retained record so the controller falls back to its fail-closed +create path (which requires an operator-configured token). Records an operator +has re-keyed with a real token are left untouched. +""" + +import logging + +from odoo import SUPERUSER_ID, api + +_logger = logging.getLogger(__name__) + + +def migrate(cr, version): + if not version: + return + + env = api.Environment(cr, SUPERUSER_ID, {}) + removed = env["spp.dci.data.source"]._purge_default_compliance_bearer_token() + if removed: + _logger.warning( + "Removed %d DCI compliance data source(s) that still held the default bearer token. " + "Set 'dci.client_compliance.bearer_token' before re-enabling the trigger endpoints.", + removed, + ) diff --git a/spp_dci_client_compliance/models/data_source.py b/spp_dci_client_compliance/models/data_source.py index d152b5e08..c574999b3 100644 --- a/spp_dci_client_compliance/models/data_source.py +++ b/spp_dci_client_compliance/models/data_source.py @@ -1,7 +1,16 @@ # Part of OpenSPP. See LICENSE file for full copyright and licensing details. """Extension to spp.dci.data.source for compliance testing.""" -from odoo import fields, models +import logging + +from odoo import api, fields, models + +_logger = logging.getLogger(__name__) + +# Well-known bearer token that earlier module versions (19.0.1.0.0) shipped as +# the default for the compliance test data source. It is public, so any data +# source still holding it must never be used to authenticate outbound requests. +DEFAULT_COMPLIANCE_BEARER_TOKEN = "compliance-test-api-key-12345" class DCIDataSourceCompliance(models.Model): @@ -15,3 +24,36 @@ class DCIDataSourceCompliance(models.Model): help="Mark this data source as used for DCI compliance testing. " "Only one data source should have this flag enabled.", ) + + @api.model + def _purge_default_compliance_bearer_token(self): + """Delete data sources that still hold the well-known default token. + + Earlier module versions created a compliance data source carrying + ``DEFAULT_COMPLIANCE_BEARER_TOKEN``. The 19.0.1.0.2 post-migration + calls this to remove any such retained record so the trigger + controller falls back to its fail-closed create path (which requires + an operator-configured token). Records an operator has re-keyed with a + real token are matched on the token value alone, so they are left + untouched. + + Returns: + int: number of data source records removed. + """ + # sudo(): bearer_token is field-level restricted to base.group_system; + # this maintenance sweep must see and remove records regardless of the + # calling user. Scope is limited to the exact known public secret. + # active_test=False: archived records still hold the token in their + # bearer_token column and remain usable by non-controller consumers + # (and readable at rest), so they must be purged too. + # nosemgrep: odoo-sudo-without-context + records = self.sudo().with_context(active_test=False) + stale = records.search([("bearer_token", "=", DEFAULT_COMPLIANCE_BEARER_TOKEN)]) + for record in stale: + _logger.warning( + "Removing DCI compliance data source %r that still held the default bearer token.", + record.code, + ) + count = len(stale) + stale.unlink() + return count diff --git a/spp_dci_client_compliance/readme/DESCRIPTION.md b/spp_dci_client_compliance/readme/DESCRIPTION.md index 26872e9bf..cba6557c1 100644 --- a/spp_dci_client_compliance/readme/DESCRIPTION.md +++ b/spp_dci_client_compliance/readme/DESCRIPTION.md @@ -20,7 +20,7 @@ Testing infrastructure for SPDCI protocol compliance validation. Exposes HTTP en After installing: 1. Set system parameter `dci.client_compliance.mock_registry_url` to point to your mock registry (default: `http://mock_registry:3335`) -2. Set system parameter `dci.client_compliance.bearer_token` for authentication (default: `compliance-test-api-key-12345`) +2. Set system parameter `dci.client_compliance.bearer_token` to a **private** token for authentication. There is no default, and the well-known value `compliance-test-api-key-12345` is rejected; the trigger endpoints refuse to run until a private token is configured. 3. Verify test data source exists under **Settings > Technical > DCI > Configuration > Data Sources** (auto-created if missing) ### Controller Endpoints diff --git a/spp_dci_client_compliance/readme/HISTORY.md b/spp_dci_client_compliance/readme/HISTORY.md new file mode 100644 index 000000000..b2a72ab87 --- /dev/null +++ b/spp_dci_client_compliance/readme/HISTORY.md @@ -0,0 +1,7 @@ +### 19.0.1.0.2 + +- fix(security): remove compliance data sources that still hold the old shared + bearer token on upgrade, refuse to serve any such record from the trigger + controller, and reject the well-known default token when configured, so + upgraded or freshly configured databases cannot use the shared credential + over the unauthenticated trigger routes. diff --git a/spp_dci_client_compliance/static/description/index.html b/spp_dci_client_compliance/static/description/index.html index fd1df3adf..0c887f2df 100644 --- a/spp_dci_client_compliance/static/description/index.html +++ b/spp_dci_client_compliance/static/description/index.html @@ -416,8 +416,10 @@

    Configuration

    1. Set system parameter dci.client_compliance.mock_registry_url to point to your mock registry (default: http://mock_registry:3335)
      2. -
    2. Set system parameter dci.client_compliance.bearer_token for -authentication (default: compliance-test-api-key-12345)
      3. +
    3. Set system parameter dci.client_compliance.bearer_token to a +private token for authentication. There is no default, and the +well-known value compliance-test-api-key-12345 is rejected; the +trigger endpoints refuse to run until a private token is configured.
    4. Verify test data source exists under Settings > Technical > DCI > Configuration > Data Sources (auto-created if missing)
    @@ -489,16 +491,24 @@

    Dependencies

    Table of contents

    + +
+
+

19.0.1.0.2

+
    +
  • fix(security): remove compliance data sources that still hold the old +shared bearer token on upgrade, refuse to serve any such record from +the trigger controller, and reject the well-known default token when +configured, so upgraded or freshly configured databases cannot use the +shared credential over the unauthenticated trigger routes.
    • +
-

Bug Tracker

+

Bug Tracker

Bugs are tracked on GitHub Issues. In case of trouble, please check there if your issue has already been reported. If you spotted it first, help us to smash it by providing a detailed and welcomed @@ -506,15 +516,15 @@

Bug Tracker

Do not contact contributors directly about support or help with technical issues.

-

Credits

+

Credits

-

Authors

+

Authors

  • OpenSPP.org
-

Maintainers

+

Maintainers

Current maintainers:

jeremi gonzalesedwin1123

This module is part of the OpenSPP/OpenSPP2 project on GitHub.

diff --git a/spp_dci_client_compliance/tests/__init__.py b/spp_dci_client_compliance/tests/__init__.py index 9f149bcbe..011bacac7 100644 --- a/spp_dci_client_compliance/tests/__init__.py +++ b/spp_dci_client_compliance/tests/__init__.py @@ -2,3 +2,4 @@ from . import test_trigger_controller from . import test_trigger_edge_cases +from . import test_stale_bearer_token diff --git a/spp_dci_client_compliance/tests/test_stale_bearer_token.py b/spp_dci_client_compliance/tests/test_stale_bearer_token.py new file mode 100644 index 000000000..91338cabe --- /dev/null +++ b/spp_dci_client_compliance/tests/test_stale_bearer_token.py @@ -0,0 +1,250 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Regression tests: an upgraded database must not keep using the old shared +compliance bearer token. + +Earlier module versions (19.0.1.0.0) created an ``spp.dci.data.source`` record +carrying the well-known token ``compliance-test-api-key-12345``. The 19.0.1.0.1 +post-migration only removed the ``ir.config_parameter`` copy, leaving the data +source usable through the ``auth='none'`` trigger routes. These tests cover the +two safeguards added in 19.0.1.0.2: + +- ``_purge_default_compliance_bearer_token`` (used by the migration) deletes + data sources that still hold the known default token, and leaves re-keyed + records untouched. +- ``_get_test_data_source`` in the trigger controller refuses to serve a record + still holding the default token, so a skipped migration cannot re-expose it. +- ``_get_compliance_bearer_token`` rejects the well-known default token when it + is configured, so the create path cannot mint a fresh default-token record. +""" + +from odoo.exceptions import UserError +from odoo.tests import TransactionCase, tagged + +from odoo.addons.spp_dci_client_compliance.models.data_source import ( + DEFAULT_COMPLIANCE_BEARER_TOKEN, +) + +BEARER_TOKEN_PARAM = "dci.client_compliance.bearer_token" +MOCK_URL_PARAM = "dci.client_compliance.mock_registry_url" + + +def _ds_vals(code, token, **overrides): + vals = { + "name": "DCI Compliance Test", + "code": code, + "base_url": "http://mock_registry:3335", + "registry_type": "social", + "our_sender_id": "spmis.compliance.test", + "our_callback_uri": "http://openspp.dci.local:8069/dci/callback", + "is_compliance_test": True, + "state": "active", + "auth_type": "bearer", + "bearer_token": token, + } + vals.update(overrides) + return vals + + +@tagged("post_install", "-at_install") +class TestPurgeDefaultBearerToken(TransactionCase): + """Unit-test the cleanup helper the 19.0.1.0.2 migration invokes.""" + + def test_purge_removes_record_with_default_token(self): + DataSource = self.env["spp.dci.data.source"].sudo() + stale = DataSource.create(_ds_vals("dci_compliance_stale", DEFAULT_COMPLIANCE_BEARER_TOKEN)) + + removed = DataSource._purge_default_compliance_bearer_token() + + self.assertEqual(removed, 1) + self.assertFalse(stale.exists(), "Data source holding the default token must be deleted") + + def test_purge_keeps_record_with_operator_token(self): + DataSource = self.env["spp.dci.data.source"].sudo() + rekeyed = DataSource.create(_ds_vals("dci_compliance_rekeyed", "operator-real-token")) + + removed = DataSource._purge_default_compliance_bearer_token() + + self.assertEqual(removed, 0) + self.assertTrue(rekeyed.exists(), "A record re-keyed with a real token must be left untouched") + + def test_purge_only_removes_default_token_records(self): + DataSource = self.env["spp.dci.data.source"].sudo() + stale = DataSource.create(_ds_vals("dci_compliance_stale", DEFAULT_COMPLIANCE_BEARER_TOKEN)) + rekeyed = DataSource.create(_ds_vals("dci_compliance_rekeyed", "operator-real-token")) + + removed = DataSource._purge_default_compliance_bearer_token() + + self.assertEqual(removed, 1) + self.assertFalse(stale.exists()) + self.assertTrue(rekeyed.exists()) + + def test_purge_removes_archived_default_token_record(self): + # Archived records still hold the token at rest and stay usable by + # non-controller consumers, so the purge must reach them too. + DataSource = self.env["spp.dci.data.source"].sudo() + archived = DataSource.create(_ds_vals("dci_compliance_archived", DEFAULT_COMPLIANCE_BEARER_TOKEN, active=False)) + + removed = DataSource._purge_default_compliance_bearer_token() + + self.assertEqual(removed, 1) + self.assertFalse(archived.exists(), "Archived default-token record must be deleted") + + def test_purge_matches_on_token_only_not_flag_or_name(self): + # A default-token record that is neither flagged nor named as a + # compliance record must still be purged - the match is token-only. + DataSource = self.env["spp.dci.data.source"].sudo() + unflagged = DataSource.create( + _ds_vals( + "dci_compliance_unflagged", + DEFAULT_COMPLIANCE_BEARER_TOKEN, + name="Some Other Data Source", + is_compliance_test=False, + ) + ) + + removed = DataSource._purge_default_compliance_bearer_token() + + self.assertEqual(removed, 1) + self.assertFalse(unflagged.exists()) + + +@tagged("post_install", "-at_install") +class TestControllerRejectsDefaultToken(TransactionCase): + """The controller must not serve a retained default-token data source. + + Even if the migration is skipped, ``_get_test_data_source`` must treat a + record still holding the known default token as absent and fall through to + the fail-closed create path (which requires an operator-configured token). + """ + + def setUp(self): + super().setUp() + from odoo.addons.spp_dci_client_compliance.controllers.trigger import ( + DCIClientTriggerController, + ) + + self.controller = DCIClientTriggerController() + ICP = self.env["ir.config_parameter"].sudo() + ICP.set_param(BEARER_TOKEN_PARAM, "operator-real-token") + ICP.set_param(MOCK_URL_PARAM, "http://mock_registry:3335") + + def _run_with_request(self): + import odoo.addons.spp_dci_client_compliance.controllers.trigger as mod + + class FakeRequest: + env = self.env + + original = mod.__dict__["request"] + mod.request = FakeRequest() + try: + return self.controller._get_test_data_source() + finally: + mod.request = original + + def test_default_token_record_is_not_served(self): + DataSource = self.env["spp.dci.data.source"].sudo() + DataSource.create(_ds_vals("dci_compliance_stale", DEFAULT_COMPLIANCE_BEARER_TOKEN)) + + result = self._run_with_request() + + self.assertNotEqual( + result.bearer_token, + DEFAULT_COMPLIANCE_BEARER_TOKEN, + "Controller must not return a data source still holding the default token", + ) + self.assertEqual(result.bearer_token, "operator-real-token") + + def test_default_token_record_matched_by_name_is_not_served(self): + # is_compliance_test=False so only the by-name lookup could match it - + # but the default-token exclusion in the search domain drops it there too. + DataSource = self.env["spp.dci.data.source"].sudo() + DataSource.create( + _ds_vals( + "dci_compliance_named", + DEFAULT_COMPLIANCE_BEARER_TOKEN, + is_compliance_test=False, + ) + ) + + result = self._run_with_request() + + self.assertNotEqual(result.bearer_token, DEFAULT_COMPLIANCE_BEARER_TOKEN) + self.assertEqual(result.bearer_token, "operator-real-token") + + def test_operator_token_record_is_served(self): + DataSource = self.env["spp.dci.data.source"].sudo() + rekeyed = DataSource.create(_ds_vals("dci_compliance_rekeyed", "operator-real-token")) + + result = self._run_with_request() + + self.assertEqual(result.id, rekeyed.id, "A properly configured record must still be used") + + def test_rekeyed_record_served_even_when_stale_record_coexists(self): + # Both records share the name/flag; a stale record can sort ahead of the + # valid one. The good record must still be served (not masked by limit=1). + DataSource = self.env["spp.dci.data.source"].sudo() + DataSource.create(_ds_vals("dci_compliance_stale", DEFAULT_COMPLIANCE_BEARER_TOKEN)) + rekeyed = DataSource.create(_ds_vals("dci_compliance_rekeyed", "operator-real-token")) + + result = self._run_with_request() + + self.assertEqual(result.id, rekeyed.id, "Valid record must not be masked by a stale one") + self.assertEqual(result.bearer_token, "operator-real-token") + + +@tagged("post_install", "-at_install") +class TestCreatePathRejectsDefaultToken(TransactionCase): + """The create path must refuse the well-known default token when configured. + + Guarding only the search paths is not enough: if an operator sets + ``dci.client_compliance.bearer_token`` to the public default (as the module + docs previously suggested), the create path would mint a fresh data source + carrying it - recreating the exposure through the front door. + ``_get_compliance_bearer_token`` therefore rejects the default value. + """ + + def setUp(self): + super().setUp() + from odoo.addons.spp_dci_client_compliance.controllers.trigger import ( + DCIClientTriggerController, + ) + + self.controller = DCIClientTriggerController() + self.ICP = self.env["ir.config_parameter"].sudo() + self.ICP.set_param(MOCK_URL_PARAM, "http://mock_registry:3335") + + def _run(self, method_name): + import odoo.addons.spp_dci_client_compliance.controllers.trigger as mod + + class FakeRequest: + env = self.env + + original = mod.__dict__["request"] + mod.request = FakeRequest() + try: + return getattr(self.controller, method_name)() + finally: + mod.request = original + + def test_get_bearer_token_rejects_default(self): + self.ICP.set_param(BEARER_TOKEN_PARAM, DEFAULT_COMPLIANCE_BEARER_TOKEN) + with self.assertRaises(UserError): + self.controller._get_compliance_bearer_token(self.env) + + def test_create_path_refuses_to_mint_default_token_record(self): + self.ICP.set_param(BEARER_TOKEN_PARAM, DEFAULT_COMPLIANCE_BEARER_TOKEN) + with self.assertRaises(UserError): + self._run("_create_test_data_source") + # Nothing was created carrying the default token. + count = ( + self.env["spp.dci.data.source"] + .sudo() + .search_count([("bearer_token", "=", DEFAULT_COMPLIANCE_BEARER_TOKEN)]) + ) + self.assertEqual(count, 0) + + def test_create_path_succeeds_with_private_token(self): + self.ICP.set_param(BEARER_TOKEN_PARAM, "operator-real-token") + result = self._run("_create_test_data_source") + self.assertTrue(result) + self.assertEqual(result.bearer_token, "operator-real-token") diff --git a/spp_dci_indicators/tests/__init__.py b/spp_dci_indicators/tests/__init__.py index 15c858873..4f9558b07 100644 --- a/spp_dci_indicators/tests/__init__.py +++ b/spp_dci_indicators/tests/__init__.py @@ -10,3 +10,4 @@ from . import test_dci_cel_params from . import test_dci_cel_methods from . import test_dci_cel_fetcher_errors +from . import test_dci_cel_validation diff --git a/spp_dci_indicators/tests/test_dci_cel_params.py b/spp_dci_indicators/tests/test_dci_cel_params.py index fada96298..5f495618a 100644 --- a/spp_dci_indicators/tests/test_dci_cel_params.py +++ b/spp_dci_indicators/tests/test_dci_cel_params.py @@ -64,3 +64,50 @@ def test_param_selects_the_matching_row(self): def test_param_discriminates_by_params_hash(self): # Hearing=1 >= 3 -> excluded (proves the lookup keyed on params, not just name) self.assertNotIn(self.partner, self._match("metric('zz.test.severity', me, arg='Hearing') >= 3")) + + def test_param_query_ignores_unparameterized_row(self): + """A parameterized query must NOT fall back to an unparameterized cache + row for the same metric. Seed a legacy/default row (no params -> params_hash + "") whose value would satisfy the comparison; querying with a param whose + own row does NOT satisfy it must still exclude the subject. + + Before the fix, _provider_clause appended (provider, "") / ("", "") + combos, so the unparameterized value=4 row matched arg='Hearing' >= 3. + """ + # Unparameterized row (params_hash "") with a value that satisfies >= 3. + self.DV.upsert_values( + [ + { + "variable_name": "zz.test.severity", + "subject_model": "res.partner", + "subject_id": self.partner.id, + "period_key": "current", + "value_json": {"value": 4}, + "value_type": "number", + "source_type": "external", + # no "params" -> params_hash == "" + "ttl_seconds": 3600, + }, + ] + ) + # Hearing's own value is 1 (< 3); the unparameterized 4 must not leak in. + self.assertNotIn(self.partner, self._match("metric('zz.test.severity', me, arg='Hearing') >= 3")) + + def test_unparameterized_query_still_matches_unparameterized_row(self): + """No regression for legacy metrics: an unparameterized query still + matches an unparameterized cache row.""" + self.DV.upsert_values( + [ + { + "variable_name": "zz.test.plain", + "subject_model": "res.partner", + "subject_id": self.partner.id, + "period_key": "current", + "value_json": {"value": 4}, + "value_type": "number", + "source_type": "external", + "ttl_seconds": 3600, + }, + ] + ) + self.assertIn(self.partner, self._match("metric('zz.test.plain', me) >= 3")) diff --git a/spp_dci_indicators/tests/test_dci_cel_validation.py b/spp_dci_indicators/tests/test_dci_cel_validation.py new file mode 100644 index 000000000..96b042bba --- /dev/null +++ b/spp_dci_indicators/tests/test_dci_cel_validation.py @@ -0,0 +1,35 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Validation must accept dotted DCI accessors. + +The DCI resolver rewrites a dotted cached accessor like ``r.dci.crvs.is_alive`` +into ``metric('r.dci.crvs.is_alive', me)`` before the base resolver extracts +identifiers, so ``me`` appears as a bare identifier in the scanned expression. +``validate_expression`` must not report it as an undefined variable. +""" + +from odoo.tests import TransactionCase, tagged + + +@tagged("post_install", "-at_install") +class TestDCIDottedValidation(TransactionCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.resolver = cls.env["spp.cel.variable.resolver"] + # Seeded active ttl variable with a dotted accessor. + cls.var = cls.env.ref("spp_dci_indicators.var_dci_crvs_is_alive") + + def test_expand_dotted_accessor_does_not_flag_me(self): + result = self.resolver.expand_expression(f"{self.var.cel_accessor} == true", context_type="individual") + # The accessor was rewritten to a metric() call ... + self.assertIn(f"metric('{self.var.cel_accessor}', me)", result["expression"]) + # ... and me is not treated as an undefined variable. + self.assertNotIn("me", result["missing_variables"]) + + def test_validate_dotted_accessor_expression_is_valid(self): + result = self.resolver.validate_expression(f"{self.var.cel_accessor} == true", context_type="individual") + self.assertTrue( + result["valid"], + f"dotted DCI accessor should validate; errors: {result['errors']}", + ) + self.assertNotIn("Undefined variables: me", " ".join(result["errors"])) diff --git a/spp_dci_server/README.rst b/spp_dci_server/README.rst index 639118900..ce72c3ca5 100644 --- a/spp_dci_server/README.rst +++ b/spp_dci_server/README.rst @@ -159,6 +159,16 @@ Dependencies Changelog ========= +19.0.2.0.5 +~~~~~~~~~~ + +- fix(security): reject non-ASCII Bearer tokens with a 401 instead of + raising an unhandled error. A Bearer token carrying non-ASCII header + bytes reached ``hmac.compare_digest`` as a non-ASCII string, which + raises ``TypeError`` and surfaced as a generic 500 (with a stack + trace) on public DCI endpoints. Such tokens are now rejected before + the constant-time comparison. + 19.0.2.0.4 ~~~~~~~~~~ diff --git a/spp_dci_server/__manifest__.py b/spp_dci_server/__manifest__.py index e07533adc..2dcfafaa2 100644 --- a/spp_dci_server/__manifest__.py +++ b/spp_dci_server/__manifest__.py @@ -1,7 +1,7 @@ { # pylint: disable=pointless-statement "name": "OpenSPP DCI Server", "summary": "DCI API server infrastructure with FastAPI routers", - "version": "19.0.2.0.4", + "version": "19.0.2.0.5", "category": "OpenSPP/Integration", "author": "OpenSPP.org", "website": "https://github.com/OpenSPP/OpenSPP2", diff --git a/spp_dci_server/middleware/signature.py b/spp_dci_server/middleware/signature.py index 82032fefe..dac3e476f 100644 --- a/spp_dci_server/middleware/signature.py +++ b/spp_dci_server/middleware/signature.py @@ -350,6 +350,23 @@ async def verify_bearer_token( headers={"WWW-Authenticate": "Bearer"}, ) + # Reject non-ASCII credentials before any comparison. HTTP headers are + # decoded as latin-1, so a non-ASCII byte reaches us as a non-ASCII str; + # hmac.compare_digest raises TypeError on non-ASCII str operands, and that + # would escape this dependency as an unhandled 500. No legitimate bearer + # credential is ever non-ASCII (OAuth2 JWTs are base64url; configured + # static tokens are ASCII), so a non-ASCII token is simply invalid. This + # guard sits before the constant-time loop, the OAuth2 path, and the + # opt-out return so every branch is covered by one check. + if not token.isascii(): + _logger.warning("DCI request has non-ASCII Bearer token") + raise DCIHTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + error_message="Invalid Bearer token", + error_code="err.auth.invalid_token", + headers={"WWW-Authenticate": "Bearer"}, + ) + # Get accepted tokens from config (comma-separated list). An empty # config used to mean "accept any non-empty token" - a fail-open # default that exposed every bearer-authenticated route the moment diff --git a/spp_dci_server/readme/HISTORY.md b/spp_dci_server/readme/HISTORY.md index a2439ea8c..cad92c614 100644 --- a/spp_dci_server/readme/HISTORY.md +++ b/spp_dci_server/readme/HISTORY.md @@ -1,3 +1,11 @@ +### 19.0.2.0.5 + +- fix(security): reject non-ASCII Bearer tokens with a 401 instead of raising an + unhandled error. A Bearer token carrying non-ASCII header bytes reached + ``hmac.compare_digest`` as a non-ASCII string, which raises ``TypeError`` and + surfaced as a generic 500 (with a stack trace) on public DCI endpoints. Such + tokens are now rejected before the constant-time comparison. + ### 19.0.2.0.4 - Return signed DCI ``on-search`` envelopes from registry alias stubs (disability, crvs, farmer) instead of HTTP 501; per-item ``rjct`` with ``ACTION_NOT_SUPPORTED``. diff --git a/spp_dci_server/static/description/index.html b/spp_dci_server/static/description/index.html index 02b56ebfa..3c89192a9 100644 --- a/spp_dci_server/static/description/index.html +++ b/spp_dci_server/static/description/index.html @@ -534,6 +534,17 @@

Changelog

+

19.0.2.0.5

+
    +
  • fix(security): reject non-ASCII Bearer tokens with a 401 instead of +raising an unhandled error. A Bearer token carrying non-ASCII header +bytes reached hmac.compare_digest as a non-ASCII string, which +raises TypeError and surfaced as a generic 500 (with a stack +trace) on public DCI endpoints. Such tokens are now rejected before +the constant-time comparison.
    • +
+
+

19.0.2.0.4

  • Return signed DCI on-search envelopes from registry alias stubs @@ -541,7 +552,7 @@

    19.0.2.0.4

    ACTION_NOT_SUPPORTED.
-
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • diff --git a/spp_dci_server/tests/test_bearer_middleware.py b/spp_dci_server/tests/test_bearer_middleware.py index ecccfc9c3..81cf7746e 100644 --- a/spp_dci_server/tests/test_bearer_middleware.py +++ b/spp_dci_server/tests/test_bearer_middleware.py @@ -146,6 +146,44 @@ def test_empty_bearer_token_rejects(self): self._call("Bearer ") self.assertEqual(ctx.exception.status_code, 401) + # --- Non-ASCII / malformed credentials (regression) ----------------------- + + def test_non_ascii_bearer_token_rejected_with_401(self): + """A Bearer token carrying non-ASCII characters must be rejected as a + 401, not crash the dependency. HTTP headers are latin-1-decoded, so a + non-ASCII header byte reaches this code as a non-ASCII str; passing it + to hmac.compare_digest raises TypeError, which - being neither an + HTTPException nor caught here - escaped as a generic 500 (with a stack + trace in the log) on every bearer-authenticated DCI endpoint.""" + self.ICP.set_param("dci.api_tokens", "alpha,beta") + + with self.assertRaises(HTTPException) as ctx: + self._call("Bearer café-ÿ") + self.assertEqual(ctx.exception.status_code, 401) + + def test_non_ascii_bearer_token_rejected_even_with_empty_list(self): + """The non-ASCII guard is a single choke point: it rejects before the + opt-out 'accept any non-empty token' path too, so a non-ASCII token is + never returned as a valid credential regardless of configuration.""" + self.ICP.set_param("dci.api_tokens", "") + self.ICP.set_param("dci.api_tokens_required", "false") + + with self.assertRaises(HTTPException) as ctx: + self._call("Bearer café-ÿ") + self.assertEqual(ctx.exception.status_code, 401) + + def test_control_char_token_treated_as_normal_invalid(self): + """Control characters are ASCII, so a control-char token passes the + non-ASCII guard and reaches hmac.compare_digest, which handles it + without raising. It is simply an ordinary non-match -> 401. This pins + that the guard deliberately rejects only non-ASCII input, not every + odd byte, and that control chars do not crash the compare.""" + self.ICP.set_param("dci.api_tokens", "alpha") + + with self.assertRaises(HTTPException) as ctx: + self._call("Bearer \x00\x01") + self.assertEqual(ctx.exception.status_code, 401) + @tagged("post_install", "-at_install") class TestSecurityDefaults(DCIServerCommon): diff --git a/spp_hazard/README.rst b/spp_hazard/README.rst index a77de2406..ea9bb06ea 100644 --- a/spp_hazard/README.rst +++ b/spp_hazard/README.rst @@ -1186,6 +1186,33 @@ encounter unexpected behavior, please report it as a new issue. Changelog ========= +19.0.2.1.1 +~~~~~~~~~~ + +- fix(security): remove the ``base.group_user`` read grant on + ``spp.hazard.impact`` so registrant-linked impact records (name, + damage level, verification, notes) are readable only by hazard roles, + ``registry_viewer``, and admins — not every internal user via RPC. + Gate the impact UI on the registrant and incident forms (stat buttons, + Emergency Response / Impacts pages, list columns, search filters) to + users with impact read. +- fix(security): guard the stored registrant indicators + ``res.partner.hazard_impact_count`` / ``has_active_impact`` with the + same field-level ``groups=`` as the impact ACL. Gating only the + registrant list/search views left both columns readable over RPC + (``search_read``, ``read_group``, export, search domains) by any + internal user — a per-registrant victim list. Stored computes run as + superuser, so partner creation by users without impact read is + unaffected (pinned by test). +- fix(security): guard ``spp.hazard.incident.affected_registrant_count`` + with field-level ``groups=``. ``spp.hazard.incident`` stays broadly + readable (sibling modules read incidents), but this aggregate is + derived from the sensitive impact table via raw ACL-bypassing SQL, so + a plain internal user could read the affected-registrant count over + RPC even without impact read. The field is now restricted to hazard + read / ``registry_viewer`` / admin, which also strips it from the + incident list column for other users. + 19.0.2.1.0 ~~~~~~~~~~ diff --git a/spp_hazard/__manifest__.py b/spp_hazard/__manifest__.py index 5e960c7d4..78fc17587 100644 --- a/spp_hazard/__manifest__.py +++ b/spp_hazard/__manifest__.py @@ -8,7 +8,7 @@ "for emergency response. Links registrants to disaster events with geographic scope " "and severity tracking to enable targeted humanitarian assistance.", "category": "OpenSPP/Targeting", - "version": "19.0.2.1.0", + "version": "19.0.2.1.1", "sequence": 1, "author": "OpenSPP.org", "website": "https://github.com/OpenSPP/OpenSPP2", diff --git a/spp_hazard/models/hazard_incident.py b/spp_hazard/models/hazard_incident.py index 642d52928..e06edbcc3 100644 --- a/spp_hazard/models/hazard_incident.py +++ b/spp_hazard/models/hazard_incident.py @@ -124,9 +124,16 @@ class HazardIncident(models.Model): ) # Computed metrics + # This aggregate is derived from the sensitive spp.hazard.impact table + # (registrant-linked) and is computed via raw SQL that bypasses record + # ACLs. spp.hazard.incident itself stays broadly readable (sibling modules + # such as spp_drims read incidents), so this field must carry its own + # group guard: without it, any internal user could read the affected- + # registrant aggregate over RPC even though they cannot read impact rows. affected_registrant_count = fields.Integer( compute="_compute_affected_registrant_count", string="Affected Registrants", + groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin", ) _code_unique = models.Constraint( diff --git a/spp_hazard/models/registrant.py b/spp_hazard/models/registrant.py index 639120386..52328dd78 100644 --- a/spp_hazard/models/registrant.py +++ b/spp_hazard/models/registrant.py @@ -17,15 +17,26 @@ class ResPartner(models.Model): "spp.hazard.impact", "registrant_id", string="Hazard Impacts", + # Reading the O2M searches spp.hazard.impact in the user's env; gate it + # like the impact ACL so a bare read() of a partner by a user without + # impact read does not fail on this field. + groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin", ) + # Both indicators are derived from the sensitive impact table and stored on + # the partner, so they are readable through the ORM (search_read, read_group, + # export, search domains) independently of the view-level gating. Field-level + # groups= mirrors the impact model's read ACL so a plain internal user cannot + # enumerate which registrants are disaster victims over RPC. hazard_impact_count = fields.Integer( compute="_compute_hazard_impact_count", string="Impact Count", store=True, + groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin", ) has_active_impact = fields.Boolean( compute="_compute_has_active_impact", store=True, + groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin", help="Whether the registrant has an impact from an active incident", ) diff --git a/spp_hazard/readme/HISTORY.md b/spp_hazard/readme/HISTORY.md index 0496cdc96..e95d9a1c7 100644 --- a/spp_hazard/readme/HISTORY.md +++ b/spp_hazard/readme/HISTORY.md @@ -1,3 +1,9 @@ +### 19.0.2.1.1 + +- fix(security): remove the `base.group_user` read grant on `spp.hazard.impact` so registrant-linked impact records (name, damage level, verification, notes) are readable only by hazard roles, `registry_viewer`, and admins — not every internal user via RPC. Gate the impact UI on the registrant and incident forms (stat buttons, Emergency Response / Impacts pages, list columns, search filters) to users with impact read. +- fix(security): guard the stored registrant indicators `res.partner.hazard_impact_count` / `has_active_impact` with the same field-level `groups=` as the impact ACL. Gating only the registrant list/search views left both columns readable over RPC (`search_read`, `read_group`, export, search domains) by any internal user — a per-registrant victim list. Stored computes run as superuser, so partner creation by users without impact read is unaffected (pinned by test). +- fix(security): guard `spp.hazard.incident.affected_registrant_count` with field-level `groups=`. `spp.hazard.incident` stays broadly readable (sibling modules read incidents), but this aggregate is derived from the sensitive impact table via raw ACL-bypassing SQL, so a plain internal user could read the affected-registrant count over RPC even without impact read. The field is now restricted to hazard read / `registry_viewer` / admin, which also strips it from the incident list column for other users. + ### 19.0.2.1.0 - feat(hazard): incidents start as a **Draft** and reach Alert or Active deliberately, rather than being assumed active on entry. Lifecycle moves are now refused server-side as well as hidden in the form: a draft cannot be closed (delete it instead) and a closed incident cannot be reopened (#1157, #1158) diff --git a/spp_hazard/security/ir.model.access.csv b/spp_hazard/security/ir.model.access.csv index 36c1b5f6f..826732791 100644 --- a/spp_hazard/security/ir.model.access.csv +++ b/spp_hazard/security/ir.model.access.csv @@ -3,7 +3,6 @@ access_spp_hazard_category_user,spp.hazard.category user,model_spp_hazard_catego access_spp_hazard_incident_user,spp.hazard.incident user,model_spp_hazard_incident,base.group_user,1,0,0,0 access_spp_hazard_incident_area_user,spp.hazard.incident.area user,model_spp_hazard_incident_area,base.group_user,1,0,0,0 access_spp_hazard_impact_type_user,spp.hazard.impact.type user,model_spp_hazard_impact_type,base.group_user,1,0,0,0 -access_spp_hazard_impact_user,spp.hazard.impact user,model_spp_hazard_impact,base.group_user,1,0,0,0 access_spp_hazard_category_sysadmin,Hazard Category System Admin,model_spp_hazard_category,base.group_system,1,1,1,1 access_spp_hazard_incident_sysadmin,Hazard Incident System Admin,model_spp_hazard_incident,base.group_system,1,1,1,1 access_spp_hazard_incident_area_sysadmin,Hazard Incident Area System Admin,model_spp_hazard_incident_area,base.group_system,1,1,1,1 diff --git a/spp_hazard/static/description/index.html b/spp_hazard/static/description/index.html index 06b97519e..8db1d981d 100644 --- a/spp_hazard/static/description/index.html +++ b/spp_hazard/static/description/index.html @@ -2454,6 +2454,34 @@

    Changelog

+

19.0.2.1.1

+
    +
  • fix(security): remove the base.group_user read grant on +spp.hazard.impact so registrant-linked impact records (name, +damage level, verification, notes) are readable only by hazard roles, +registry_viewer, and admins — not every internal user via RPC. +Gate the impact UI on the registrant and incident forms (stat buttons, +Emergency Response / Impacts pages, list columns, search filters) to +users with impact read.
    • +
  • fix(security): guard the stored registrant indicators +res.partner.hazard_impact_count / has_active_impact with the +same field-level groups= as the impact ACL. Gating only the +registrant list/search views left both columns readable over RPC +(search_read, read_group, export, search domains) by any +internal user — a per-registrant victim list. Stored computes run as +superuser, so partner creation by users without impact read is +unaffected (pinned by test).
    • +
  • fix(security): guard spp.hazard.incident.affected_registrant_count +with field-level groups=. spp.hazard.incident stays broadly +readable (sibling modules read incidents), but this aggregate is +derived from the sensitive impact table via raw ACL-bypassing SQL, so +a plain internal user could read the affected-registrant count over +RPC even without impact read. The field is now restricted to hazard +read / registry_viewer / admin, which also strips it from the +incident list column for other users.
    • +
+
+

19.0.2.1.0

  • feat(hazard): incidents start as a Draft and reach Alert or Active @@ -2463,7 +2491,7 @@

    19.0.2.1.0

    cannot be reopened (#1157, #1158)
-
+

19.0.2.0.2

  • fix(security): grant group_hazard_viewer to spp_user_roles roles @@ -2479,7 +2507,7 @@

    19.0.2.0.2

    Support).
-
+

19.0.2.0.1

  • fix(views): apply spp_registry.x2many_no_padding widget to the @@ -2487,7 +2515,7 @@

    19.0.2.0.1

    (showing a muted info line instead) (#943).
-
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • diff --git a/spp_hazard/tests/__init__.py b/spp_hazard/tests/__init__.py index 9956de3b9..700f0a64e 100644 --- a/spp_hazard/tests/__init__.py +++ b/spp_hazard/tests/__init__.py @@ -6,3 +6,5 @@ from . import test_hazard_impact_type from . import test_geofence from . import test_registrant + +from . import test_acl_group_user diff --git a/spp_hazard/tests/test_acl_group_user.py b/spp_hazard/tests/test_acl_group_user.py new file mode 100644 index 000000000..1fa2960d3 --- /dev/null +++ b/spp_hazard/tests/test_acl_group_user.py @@ -0,0 +1,241 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Security: hazard models must not be readable by every internal user. + +Regression test for "Broad internal read access exposes hazard impact records": +the ACL granted ``base.group_user`` read on the hazard models, so any internal +user could read hazard data (including registrant-linked impact records) via RPC, +even without a hazard role. Access must require a dedicated hazard group (or +``registry_viewer``/admin), not merely being an internal user. +""" + +from odoo import Command +from odoo.exceptions import AccessError +from odoo.tests import tagged + +from .common import HazardTestCase + +# The registrant-linked impact model is sensitive and must NOT be readable by +# every internal user. The other hazard models are non-PII reference/operational +# data that sibling modules (e.g. spp_drims) legitimately read broadly. +SENSITIVE_MODEL = "spp.hazard.impact" +NON_SENSITIVE_MODELS = [ + "spp.hazard.category", + "spp.hazard.incident", + "spp.hazard.incident.area", + "spp.hazard.impact.type", +] +ALL_HAZARD_MODELS = [SENSITIVE_MODEL, *NON_SENSITIVE_MODELS] + + +@tagged("post_install", "-at_install") +class TestHazardBaseUserNoAccess(HazardTestCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.plain_user = cls.env["res.users"].create( + { + "name": "Plain Internal User", + "login": "plain_internal_hazard_test", + "group_ids": [Command.link(cls.env.ref("base.group_user").id)], + } + ) + + def test_plain_internal_user_cannot_read_impact(self): + """base.group_user (any internal user) must NOT read the sensitive impact model.""" + with self.assertRaises(AccessError): + self.env[SENSITIVE_MODEL].with_user(self.plain_user).check_access("read") + + def test_plain_internal_user_can_read_non_sensitive_models(self): + """Non-PII hazard reference/operational models remain internally readable + (sibling modules such as spp_drims depend on reading incidents).""" + for model in NON_SENSITIVE_MODELS: + # Raises AccessError only if broad read was wrongly removed here. + self.env[model].with_user(self.plain_user).check_access("read") + + def test_hazard_viewer_retains_read(self): + """A hazard-group user must keep read access to all hazard models.""" + for model in ALL_HAZARD_MODELS: + self.env[model].with_user(self.hazard_viewer).check_access("read") + + def test_registry_user_can_still_read_registrant_hazard_fields(self): + """Regression: the registrant form's hazard indicator fields read + spp.hazard.impact in their compute. A registry user (Officer implies + Registry Viewer, which retains hazard read) must still be able to load + them after the ACL tightening — i.e. the fix must not break the form.""" + officer = self.env["res.users"].create( + { + "name": "Registry Officer (no hazard group)", + "login": "registry_officer_hazard_test", + "group_ids": [Command.link(self.env.ref("spp_registry.group_registry_officer").id)], + } + ) + # Sanity: this user is NOT in any hazard group. + self.assertFalse(officer.has_group("spp_hazard.group_hazard_read")) + + incident = self.env["spp.hazard.incident"].create( + { + "name": "Registry Officer Incident", + "code": "ROI-HAZ-001", + "category_id": self.category_typhoon.id, + "start_date": "2024-01-01", + } + ) + self.env["spp.hazard.impact"].create( + { + "incident_id": incident.id, + "registrant_id": self.registrant.id, + "impact_type_id": self.impact_type_displacement.id, + "damage_level": "moderate", + "impact_date": "2024-01-02", + } + ) + registrant_as_officer = self.registrant.with_user(officer) + # Force a live read through the impact O2M (not just the stored count), + # which must not raise AccessError for a registry user. + self.assertEqual(registrant_as_officer.hazard_impact_ids.mapped("damage_level"), ["moderate"]) + + def test_plain_internal_user_cannot_read_affected_registrant_count(self): + """spp.hazard.incident stays broadly readable, but its + ``affected_registrant_count`` aggregate is derived from the sensitive + impact table via raw ACL-bypassing SQL. A plain internal user must be + able to read the incident yet be denied that field over RPC.""" + incident = self.env["spp.hazard.incident"].create( + { + "name": "Aggregate Leak Incident", + "code": "ALI-HAZ-001", + "category_id": self.category_typhoon.id, + "start_date": "2024-01-01", + } + ) + self.env["spp.hazard.impact"].create( + { + "incident_id": incident.id, + "registrant_id": self.registrant.id, + "impact_type_id": self.impact_type_displacement.id, + "damage_level": "moderate", + "impact_date": "2024-01-02", + } + ) + incident_as_plain = incident.with_user(self.plain_user) + # The incident itself remains readable (non-sensitive model)... + incident_as_plain.read(["name"]) + # ...but the impact-derived aggregate must be denied. + with self.assertRaises(AccessError): + incident_as_plain.read(["affected_registrant_count"]) + with self.assertRaises(AccessError): + # Attribute access goes through Field.__get__, which enforces the + # field-level group guard independently of read(). + _ = incident_as_plain.affected_registrant_count + + def test_hazard_viewer_can_read_affected_registrant_count(self): + """A hazard-group user must still read the affected-registrant aggregate.""" + incident = self.env["spp.hazard.incident"].create( + { + "name": "Aggregate Visible Incident", + "code": "AVI-HAZ-001", + "category_id": self.category_typhoon.id, + "start_date": "2024-01-01", + } + ) + self.env["spp.hazard.impact"].create( + { + "incident_id": incident.id, + "registrant_id": self.registrant.id, + "impact_type_id": self.impact_type_displacement.id, + "damage_level": "moderate", + "impact_date": "2024-01-02", + } + ) + self.assertEqual(incident.with_user(self.hazard_viewer).affected_registrant_count, 1) + + def test_affected_registrant_count_column_hidden_from_non_hazard_user(self): + """The incident list column reads the gated aggregate; it must be stripped + from the arch for a plain internal user.""" + arch = self.env["spp.hazard.incident"].with_user(self.plain_user).get_view(view_type="list")["arch"] + self.assertNotIn("affected_registrant_count", arch) + + def test_incident_form_hides_impacts_from_non_hazard_user(self): + """The incident form's Impacts O2M reads spp.hazard.impact; it must be + stripped from the arch for a user without impact read (e.g. a DRIMS-only + user), so opening an incident does not raise AccessError.""" + arch = self.env["spp.hazard.incident"].with_user(self.plain_user).get_view(view_type="form")["arch"] + self.assertNotIn("impact_ids", arch) + + def test_incident_form_shows_impacts_to_hazard_user(self): + """A hazard user still gets the Impacts O2M on the incident form.""" + arch = self.env["spp.hazard.incident"].with_user(self.hazard_viewer).get_view(view_type="form")["arch"] + self.assertIn("impact_ids", arch) + + def test_plain_internal_user_cannot_read_registrant_impact_fields(self): + """``res.partner.hazard_impact_count`` / ``has_active_impact`` are stored + columns derived from the sensitive impact table. Gating only the views is + not enough: a plain internal user could still ``search_read`` them over RPC + and enumerate which registrants are disaster victims. The fields must carry + field-level ``groups=`` so the ORM refuses the read.""" + partner_as_plain = self.env["res.partner"].with_user(self.plain_user) + with self.assertRaises(AccessError): + partner_as_plain.search_read( + [("id", "=", self.registrant.id)], + ["name", "hazard_impact_count", "has_active_impact"], + ) + # The headline attack is the domain, not the field list: filtering or + # ordering on the gated columns must be refused too (presence oracle). + with self.assertRaises(AccessError): + partner_as_plain.search([("has_active_impact", "=", True)]) + with self.assertRaises(AccessError): + partner_as_plain.search([("hazard_impact_count", ">", 0)]) + with self.assertRaises(AccessError): + partner_as_plain.search([("id", "=", self.registrant.id)], order="hazard_impact_count desc") + with self.assertRaises(AccessError): + partner_as_plain.read_group([], ["hazard_impact_count:sum"], ["has_active_impact"]) + # The O2M itself must not be reachable either, and all three must be + # hidden from fields_get(): that is what makes a bare read() with no + # field list (generic RPC clients) skip them instead of failing on them. + with self.assertRaises(AccessError): + partner_as_plain.search_read([("id", "=", self.registrant.id)], ["hazard_impact_ids"]) + visible = partner_as_plain.fields_get(["hazard_impact_ids", "hazard_impact_count", "has_active_impact"]) + self.assertEqual(visible, {}) + # And the gated columns/filters are stripped from the list/search arch. + arch = partner_as_plain.get_view(view_type="list")["arch"] + self.assertNotIn("hazard_impact_count", arch) + self.assertNotIn("has_active_impact", arch) + + def test_hazard_viewer_can_read_registrant_impact_fields(self): + """A hazard-group user keeps read on the registrant impact indicator fields.""" + rows = ( + self.env["res.partner"] + .with_user(self.hazard_viewer) + .search_read([("id", "=", self.registrant.id)], ["hazard_impact_count", "has_active_impact"]) + ) + self.assertEqual(len(rows), 1) + + def test_contact_creator_without_impact_read_can_create_partner(self): + """Guard: the two stored impact indicators are computed on every + ``res.partner`` create by querying ``spp.hazard.impact``. Stored computes + run as superuser (``compute_sudo`` defaults to True for stored fields), so + removing ``base.group_user`` read on impacts must NOT break partner + creation for an internal user who may create contacts but holds no + hazard/registry role (e.g. Contact Creation only). Pins that behaviour, + including the flush that actually runs the compute.""" + creator = self.env["res.users"].create( + { + "name": "Contact Creator (no hazard/registry group)", + "login": "contact_creator_hazard_test", + "group_ids": [ + Command.link(self.env.ref("base.group_user").id), + Command.link(self.env.ref("base.group_partner_manager").id), + ], + } + ) + self.assertFalse(creator.has_group("spp_hazard.group_hazard_read")) + self.assertFalse(creator.has_group("spp_registry.group_registry_viewer")) + with self.assertRaises(AccessError): + self.env[SENSITIVE_MODEL].with_user(creator).check_access("read") + + partner = self.env["res.partner"].with_user(creator).create({"name": "Created by contact creator"}) + # Stored computes are deferred to flush time; flush in the CREATOR's env + # (as a real request does at its end) so the compute runs as that user. + partner.env.flush_all() + self.assertTrue(partner.exists()) + self.assertEqual(partner.sudo().hazard_impact_count, 0) + self.assertFalse(partner.sudo().has_active_impact) diff --git a/spp_hazard/views/hazard_incident_views.xml b/spp_hazard/views/hazard_incident_views.xml index 90b0ede2e..69d1c3551 100644 --- a/spp_hazard/views/hazard_incident_views.xml +++ b/spp_hazard/views/hazard_incident_views.xml @@ -32,7 +32,11 @@ decoration-danger="severity == '5'" /> - + @@ -92,6 +96,7 @@ type="object" class="oe_stat_button" icon="fa-users" + groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin" > - + diff --git a/spp_hazard/views/registrant_views.xml b/spp_hazard/views/registrant_views.xml index c7a0f7c7b..574fe5148 100644 --- a/spp_hazard/views/registrant_views.xml +++ b/spp_hazard/views/registrant_views.xml @@ -16,6 +16,7 @@ class="oe_stat_button" icon="fa-bolt" invisible="hazard_impact_count == 0" + groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin" >
    50 - + @@ -121,16 +129,20 @@ 50 - + diff --git a/spp_hazard_programs/README.rst b/spp_hazard_programs/README.rst index 0b6fdee90..384162d78 100644 --- a/spp_hazard_programs/README.rst +++ b/spp_hazard_programs/README.rst @@ -87,7 +87,7 @@ access from: Extension Points ~~~~~~~~~~~~~~~~ -- Override ``get_emergency_eligible_registrants()`` to customize +- Override ``_get_emergency_eligible_registrants()`` to customize eligibility logic beyond damage levels - Override ``_get_damage_level_domain()`` to add custom damage filtering rules @@ -324,6 +324,22 @@ Test Scenario 9: Incident List View Column Changelog ========= +19.0.2.0.1 +~~~~~~~~~~ + +- fix(security): read ``spp.hazard.impact`` via ``sudo`` in the + emergency-eligibility computes (``affected_registrant_count``, + ``get_emergency_eligible_registrants``), so they keep working for + non-hazard program users after impact read access was restricted to + hazard/registry roles. Only aggregate counts are surfaced to program + users without impact read; the list of eligible (impacted) registrants + is the identity linkage the impact ACL protects, so + ``action_view_affected_registrants`` now checks impact read access + server-side, its stat button is gated in the form, and + ``get_emergency_eligible_registrants()`` is renamed + ``_get_emergency_eligible_registrants()`` so it is no longer callable + over RPC (Python callers and overrides are unaffected). + 19.0.2.0.0 ~~~~~~~~~~ diff --git a/spp_hazard_programs/__manifest__.py b/spp_hazard_programs/__manifest__.py index 1871df01a..dfb115f96 100644 --- a/spp_hazard_programs/__manifest__.py +++ b/spp_hazard_programs/__manifest__.py @@ -7,7 +7,7 @@ "summary": "Links hazard impacts to program eligibility and entitlements. " "Enables emergency programs to use hazard data for targeting and benefit calculation.", "category": "OpenSPP/Targeting", - "version": "19.0.2.0.0", + "version": "19.0.2.0.1", "sequence": 1, "author": "OpenSPP.org", "website": "https://github.com/OpenSPP/OpenSPP2", diff --git a/spp_hazard_programs/models/program.py b/spp_hazard_programs/models/program.py index 4d48edc0e..fb1f2791e 100644 --- a/spp_hazard_programs/models/program.py +++ b/spp_hazard_programs/models/program.py @@ -84,15 +84,21 @@ def _compute_affected_registrant_count(self): # Build domain for qualifying damage levels damage_domain = rec._get_damage_level_domain() - # Count unique registrants with qualifying impacts - impacts = self.env["spp.hazard.impact"].search( + # Count unique registrants with qualifying impacts, aggregated in + # SQL so no impact rows are loaded into memory. + # sudo: emergency-program eligibility must consider all qualifying + # impacts regardless of the viewing user's hazard access; impact rows + # are not exposed, only the aggregate count. + impact_sudo = self.env["spp.hazard.impact"].sudo() # nosemgrep: odoo-sudo-without-context + [(count,)] = impact_sudo._read_group( [ ("incident_id", "in", rec.target_incident_ids.ids), ("verification_status", "=", "verified"), ] - + damage_domain + + damage_domain, + aggregates=["registrant_id:count_distinct"], ) - rec.affected_registrant_count = len(impacts.mapped("registrant_id")) + rec.affected_registrant_count = count def _get_damage_level_domain(self): """Get the domain filter for qualifying damage levels.""" @@ -107,10 +113,16 @@ def _get_damage_level_domain(self): return [("damage_level", "in", ("critical", "totally_damaged"))] return [] - def get_emergency_eligible_registrants(self): + def _get_emergency_eligible_registrants(self): """ Get registrants eligible for this emergency program based on hazard impacts. + Private on purpose: the result is the list of registrants affected by a + hazard, i.e. the identity linkage the impact ACL protects. It is meant + for Python callers (eligibility logic, overrides), not for RPC. The UI + entry point is ``action_view_affected_registrants``, which checks impact + read access before exposing the list. + Returns registrants who: - Have verified impact from one of the target incidents - Meet the qualifying damage level threshold @@ -123,16 +135,22 @@ def get_emergency_eligible_registrants(self): damage_domain = self._get_damage_level_domain() - # Find qualifying impacts - impacts = self.env["spp.hazard.impact"].search( + # Find the unique registrants of qualifying impacts, grouped in SQL so + # no impact rows are loaded into memory. + # sudo: eligibility must consider all qualifying impacts regardless of the + # viewing user's hazard access; only the resulting registrants are returned. + impact_sudo = self.env["spp.hazard.impact"].sudo() # nosemgrep: odoo-sudo-without-context + groups = impact_sudo._read_group( [ ("incident_id", "in", self.target_incident_ids.ids), ("verification_status", "=", "verified"), ] - + damage_domain + + damage_domain, + groupby=["registrant_id"], ) - return impacts.mapped("registrant_id") + # Return the registrants in the caller's env (not the sudo one). + return self.env["res.partner"].browse([registrant.id for (registrant,) in groups if registrant]) def action_view_target_incidents(self): """Open a list view of target incidents.""" @@ -147,9 +165,15 @@ def action_view_target_incidents(self): } def action_view_affected_registrants(self): - """Open a list view of potentially affected registrants.""" + """Open a list view of potentially affected registrants. + + The aggregate count stays visible to every program user, but the list + names the impacted registrants, so it requires impact read access. The + stat button is gated in the view; this check covers RPC callers. + """ self.ensure_one() - registrants = self.get_emergency_eligible_registrants() + self.env["spp.hazard.impact"].check_access("read") + registrants = self._get_emergency_eligible_registrants() return { "name": _("Affected Registrants - %s", self.name), "type": "ir.actions.act_window", diff --git a/spp_hazard_programs/readme/DESCRIPTION.md b/spp_hazard_programs/readme/DESCRIPTION.md index 29f7ab3bd..a2bff17c5 100644 --- a/spp_hazard_programs/readme/DESCRIPTION.md +++ b/spp_hazard_programs/readme/DESCRIPTION.md @@ -39,7 +39,7 @@ No new models or ACL entries. Fields added to existing models inherit access fro ### Extension Points -- Override `get_emergency_eligible_registrants()` to customize eligibility logic beyond damage levels +- Override `_get_emergency_eligible_registrants()` to customize eligibility logic beyond damage levels - Override `_get_damage_level_domain()` to add custom damage filtering rules - Inherit `spp.program` to add fields used in emergency calculations - Use `is_emergency_program` and `is_emergency_mode` flags in downstream program logic diff --git a/spp_hazard_programs/readme/HISTORY.md b/spp_hazard_programs/readme/HISTORY.md index 4aaf9afef..e96a173ff 100644 --- a/spp_hazard_programs/readme/HISTORY.md +++ b/spp_hazard_programs/readme/HISTORY.md @@ -1,3 +1,7 @@ +### 19.0.2.0.1 + +- fix(security): read `spp.hazard.impact` via `sudo` in the emergency-eligibility computes (`affected_registrant_count`, `get_emergency_eligible_registrants`), so they keep working for non-hazard program users after impact read access was restricted to hazard/registry roles. Only aggregate counts are surfaced to program users without impact read; the list of eligible (impacted) registrants is the identity linkage the impact ACL protects, so `action_view_affected_registrants` now checks impact read access server-side, its stat button is gated in the form, and `get_emergency_eligible_registrants()` is renamed `_get_emergency_eligible_registrants()` so it is no longer callable over RPC (Python callers and overrides are unaffected). + ### 19.0.2.0.0 - Initial migration to OpenSPP2 diff --git a/spp_hazard_programs/static/description/index.html b/spp_hazard_programs/static/description/index.html index 8a1fc0b68..9cbb32734 100644 --- a/spp_hazard_programs/static/description/index.html +++ b/spp_hazard_programs/static/description/index.html @@ -447,7 +447,7 @@

    Security

    Extension Points

      -
    • Override get_emergency_eligible_registrants() to customize +
    • Override _get_emergency_eligible_registrants() to customize eligibility logic beyond damage levels
    • Override _get_damage_level_domain() to add custom damage filtering rules
      • @@ -698,6 +698,23 @@

      Changelog

    +

    19.0.2.0.1

    +
      +
    • fix(security): read spp.hazard.impact via sudo in the +emergency-eligibility computes (affected_registrant_count, +get_emergency_eligible_registrants), so they keep working for +non-hazard program users after impact read access was restricted to +hazard/registry roles. Only aggregate counts are surfaced to program +users without impact read; the list of eligible (impacted) registrants +is the identity linkage the impact ACL protects, so +action_view_affected_registrants now checks impact read access +server-side, its stat button is gated in the form, and +get_emergency_eligible_registrants() is renamed +_get_emergency_eligible_registrants() so it is no longer callable +over RPC (Python callers and overrides are unaffected).
      • +
    +
    +

    19.0.2.0.0

    • Initial migration to OpenSPP2
      • diff --git a/spp_hazard_programs/tests/__init__.py b/spp_hazard_programs/tests/__init__.py index 66fbca0e0..669012f1b 100644 --- a/spp_hazard_programs/tests/__init__.py +++ b/spp_hazard_programs/tests/__init__.py @@ -1,3 +1,5 @@ # Part of OpenSPP. See LICENSE file for full copyright and licensing details. from . import test_hazard_programs + +from . import test_program_user_access diff --git a/spp_hazard_programs/tests/test_hazard_programs.py b/spp_hazard_programs/tests/test_hazard_programs.py index 42ccefcbe..bda984b5a 100644 --- a/spp_hazard_programs/tests/test_hazard_programs.py +++ b/spp_hazard_programs/tests/test_hazard_programs.py @@ -137,7 +137,7 @@ def test_get_emergency_eligible_registrants(self): "qualifying_damage_levels": "any", } ) - registrants = self.program.get_emergency_eligible_registrants() + registrants = self.program._get_emergency_eligible_registrants() self.assertEqual(len(registrants), 2) self.assertIn(self.registrant_1, registrants) self.assertIn(self.registrant_2, registrants) @@ -146,7 +146,7 @@ def test_get_emergency_eligible_registrants(self): def test_get_emergency_eligible_registrants_no_incidents(self): """Test eligible registrants returns empty when no incidents linked.""" - registrants = self.program.get_emergency_eligible_registrants() + registrants = self.program._get_emergency_eligible_registrants() self.assertEqual(len(registrants), 0) def test_get_emergency_eligible_registrants_with_filter(self): @@ -157,7 +157,7 @@ def test_get_emergency_eligible_registrants_with_filter(self): "qualifying_damage_levels": "critical_only", } ) - registrants = self.program.get_emergency_eligible_registrants() + registrants = self.program._get_emergency_eligible_registrants() self.assertEqual(len(registrants), 1) self.assertIn(self.registrant_1, registrants) diff --git a/spp_hazard_programs/tests/test_program_user_access.py b/spp_hazard_programs/tests/test_program_user_access.py new file mode 100644 index 000000000..3be50bcc3 --- /dev/null +++ b/spp_hazard_programs/tests/test_program_user_access.py @@ -0,0 +1,89 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Regression: emergency-eligibility logic must work for non-hazard program users. + +After tightening the hazard-impact ACL (removing the broad ``base.group_user`` +read grant), the program eligibility computes read ``spp.hazard.impact`` via +``sudo`` so a program user without any hazard group can still use them. Without +that sudo, ``affected_registrant_count`` / ``get_emergency_eligible_registrants`` +would raise ``AccessError`` for such users. +""" + +from odoo import Command +from odoo.tests import tagged + +from .common import HazardProgramsTestCase + + +@tagged("post_install", "-at_install") +class TestProgramUserHazardAccess(HazardProgramsTestCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.program_user = cls.env["res.users"].create( + { + "name": "Program Manager (no hazard group)", + "login": "program_mgr_no_hazard_test", + "group_ids": [ + Command.link(cls.env.ref("base.group_user").id), + Command.link(cls.env.ref("spp_programs.group_programs_manager").id), + ], + } + ) + cls.program.write( + { + "target_incident_ids": [Command.link(cls.incident_active.id)], + "qualifying_damage_levels": "any", + } + ) + + def test_program_user_without_hazard_group_can_compute_eligibility(self): + """A program user with no hazard group must still compute emergency + eligibility (the impact reads are sudo'd).""" + self.assertFalse(self.program_user.has_group("spp_hazard.group_hazard_read")) + program = self.program.with_user(self.program_user) + # Non-stored compute -> runs live as this user; reads impact via sudo. + self.assertEqual(program.affected_registrant_count, 2) + # Method -> runs live as this user; reads impact via sudo. Private so it + # is reachable from Python (eligibility, overrides) but not over RPC. + eligible = program._get_emergency_eligible_registrants() + self.assertIn(self.registrant_1, eligible) + self.assertIn(self.registrant_2, eligible) + + def test_eligible_registrants_method_is_not_rpc_callable(self): + """The eligible-registrant list is the identity linkage the impact ACL + protects. It must not be exposed as a public (call_kw-reachable) method.""" + self.assertFalse(hasattr(type(self.program), "get_emergency_eligible_registrants")) + self.assertTrue(hasattr(type(self.program), "_get_emergency_eligible_registrants")) + + def test_program_user_without_impact_read_cannot_open_affected_registrants(self): + """The 'Affected' stat button opens the list of impacted registrants. A + program user without impact read keeps the aggregate count but must be + refused the list, server-side (buttons are RPC-callable) and in the arch.""" + from odoo.exceptions import AccessError + + program = self.program.with_user(self.program_user) + self.assertEqual(program.affected_registrant_count, 2) + with self.assertRaises(AccessError): + program.action_view_affected_registrants() + arch = self.env["spp.program"].with_user(self.program_user).get_view(view_type="form")["arch"] + self.assertNotIn("action_view_affected_registrants", arch) + + def test_hazard_user_can_open_affected_registrants(self): + """A user with impact read (hazard viewer + program manager) keeps the list.""" + hazard_program_user = self.env["res.users"].create( + { + "name": "Program Manager with hazard read", + "login": "program_mgr_hazard_read_test", + "group_ids": [ + Command.link(self.env.ref("base.group_user").id), + Command.link(self.env.ref("spp_programs.group_programs_manager").id), + Command.link(self.env.ref("spp_hazard.group_hazard_viewer").id), + ], + } + ) + program = self.program.with_user(hazard_program_user) + action = program.action_view_affected_registrants() + self.assertEqual(action["res_model"], "res.partner") + self.assertEqual(set(action["domain"][0][2]), {self.registrant_1.id, self.registrant_2.id}) + arch = self.env["spp.program"].with_user(hazard_program_user).get_view(view_type="form")["arch"] + self.assertIn("action_view_affected_registrants", arch) diff --git a/spp_hazard_programs/views/program_views.xml b/spp_hazard_programs/views/program_views.xml index 910715cd6..5453049ed 100644 --- a/spp_hazard_programs/views/program_views.xml +++ b/spp_hazard_programs/views/program_views.xml @@ -28,6 +28,7 @@ class="oe_stat_button" icon="fa-users" invisible="affected_registrant_count == 0" + groups="spp_hazard.group_hazard_read,spp_registry.group_registry_viewer,spp_security.group_spp_admin" > 0 else config_limit search_mode = get_param("spp_registry_search.search_mode", "unified") - # If targeted mode and a field is specified, only search that field - if search_mode == "targeted" and search_field: + if search_mode == "targeted": + # Targeted mode never widens to unified search: a missing or + # unknown field falls back to the configured default field. + if search_field not in ("name", "id_number", "phone", "email"): + search_field = get_param("spp_registry_search.target_field", "name") partner_ids = self._search_by_field(search_term, search_field, limit) else: # Unified mode: run separate queries per field and merge @@ -119,7 +139,7 @@ def search_registrants(self, search_term, search_type="all", search_field=None, elif search_type == "groups": domain.append(("is_group", "=", True)) - if advanced_filters: + if advanced_filters and isinstance(advanced_filters, dict): if advanced_filters.get("registrationDateFrom"): domain.append(("registration_date", ">=", advanced_filters["registrationDateFrom"])) if advanced_filters.get("registrationDateTo"): diff --git a/spp_registry_search/readme/HISTORY.md b/spp_registry_search/readme/HISTORY.md index 1de70f102..d4fb6e3d9 100644 --- a/spp_registry_search/readme/HISTORY.md +++ b/spp_registry_search/readme/HISTORY.md @@ -1,3 +1,12 @@ +### 19.0.2.1.2 + +- fix(security): enforce the configured Registry Search controls server-side in the + `search_registrants` RPC method. The minimum-character requirement (counting effective, + non-wildcard characters), the administrator's maximum result limit, and the targeted + search mode (no fallback to unified search when the field is missing or invalid) are now + applied on the server instead of only in the JavaScript client, and malformed RPC inputs + are rejected instead of raising errors. + ### 19.0.2.1.1 - fix(security): gate the Registry Search "New Individual/Group" buttons on the registry create-permission roles instead of generic `res.partner` create access, so roles without registrant-create rights (e.g. validators) can no longer initiate creation (#1124) diff --git a/spp_registry_search/static/description/index.html b/spp_registry_search/static/description/index.html index 25e1fcec2..8758867b0 100644 --- a/spp_registry_search/static/description/index.html +++ b/spp_registry_search/static/description/index.html @@ -497,6 +497,19 @@

      Changelog

+

19.0.2.1.2

+
    +
  • fix(security): enforce the configured Registry Search controls +server-side in the search_registrants RPC method. The +minimum-character requirement (counting effective, non-wildcard +characters), the administrator’s maximum result limit, and the +targeted search mode (no fallback to unified search when the field is +missing or invalid) are now applied on the server instead of only in +the JavaScript client, and malformed RPC inputs are rejected instead +of raising errors.
    • +
+
+

19.0.2.1.1

  • fix(security): gate the Registry Search “New Individual/Group” buttons @@ -505,7 +518,7 @@

    19.0.2.1.1

    rights (e.g. validators) can no longer initiate creation (#1124)
-
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • diff --git a/spp_registry_search/tests/__init__.py b/spp_registry_search/tests/__init__.py index 049ae4fb9..72460818a 100644 --- a/spp_registry_search/tests/__init__.py +++ b/spp_registry_search/tests/__init__.py @@ -1,4 +1,5 @@ # Part of OpenSPP. See LICENSE file for full copyright and licensing details. from . import test_registrant_create_permission +from . import test_search_registrants from . import test_registry_view_history from . import test_registry_view_history_security diff --git a/spp_registry_search/tests/test_search_registrants.py b/spp_registry_search/tests/test_search_registrants.py new file mode 100644 index 000000000..9779f19a1 --- /dev/null +++ b/spp_registry_search/tests/test_search_registrants.py @@ -0,0 +1,149 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Tests for the ``search_registrants`` RPC method. + +The administrator-configurable Registry Search controls (``min_chars``, +``result_limit``, targeted search mode) were previously enforced only in the +JavaScript client. These tests pin the server-side enforcement: a caller +invoking the RPC directly must be subject to the same governance as the +portal UI, because the method searches sensitive registrant PII fields +(name, ID number, phone, email). +""" + +from odoo.tests import TransactionCase, tagged + + +@tagged("post_install", "-at_install") +class TestSearchRegistrants(TransactionCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.Partner = cls.env["res.partner"] + cls.ICP = cls.env["ir.config_parameter"].sudo() + + cls.alice = cls.Partner.create( + { + "name": "Alicia Registrant", + "is_registrant": True, + "is_group": False, + } + ) + cls.alice_phone = cls.env["spp.phone.number"].create( + { + "partner_id": cls.alice.id, + "phone_no": "09171234567", + } + ) + # A pool larger than the admin limit used in the limit tests. + cls.bulk = cls.Partner.create( + [ + { + "name": f"Bulklimit Person {i:02d}", + "is_registrant": True, + "is_group": False, + } + for i in range(1, 13) + ] + ) + + def setUp(self): + super().setUp() + # Explicit baseline so tests never depend on deployment defaults. + self.ICP.set_param("spp_registry_search.search_mode", "unified") + self.ICP.set_param("spp_registry_search.target_field", "name") + self.ICP.set_param("spp_registry_search.result_limit", "50") + self.ICP.set_param("spp_registry_search.min_chars", "3") + + def _search(self, term, **kwargs): + return self.Partner.search_registrants(term, **kwargs) + + # --- min_chars enforcement -------------------------------------------------- + + def test_min_chars_enforced_server_side(self): + """A term shorter than the configured minimum must return nothing, + even when the RPC is called directly (bypassing the JS check).""" + self.assertEqual(self._search("Al"), []) + + def test_wildcard_only_term_rejected(self): + """SQL LIKE wildcards must not count toward min_chars: '%%%' is three + characters but zero effective characters and would otherwise match + every registrant.""" + self.assertEqual(self._search("%%%"), []) + + def test_wildcard_padding_does_not_satisfy_min_chars(self): + """Wildcards mixed with too few literal characters are rejected.""" + self.assertEqual(self._search("Al%"), []) + + def test_min_chars_met_returns_results(self): + """Regression: a compliant term still finds the registrant.""" + results = self._search("Alicia") + self.assertIn(self.alice.id, [r["id"] for r in results]) + + # --- result limit enforcement ---------------------------------------------- + + def test_limit_capped_by_admin_config(self): + """A caller-supplied limit must never exceed the configured maximum.""" + self.ICP.set_param("spp_registry_search.result_limit", "10") + results = self._search("Bulklimit", limit=200) + self.assertLessEqual(len(results), 10) + + def test_lower_caller_limit_honored(self): + """A caller may request fewer results than the configured maximum.""" + results = self._search("Bulklimit", limit=5) + self.assertLessEqual(len(results), 5) + + def test_non_numeric_limit_does_not_crash(self): + """A malformed limit must fall back to the configured limit, not + raise TypeError (which would surface as a generic 500).""" + results = self._search("Alicia", limit="not-a-number") + self.assertIn(self.alice.id, [r["id"] for r in results]) + + # --- targeted mode enforcement ---------------------------------------------- + + def test_targeted_mode_missing_field_stays_targeted(self): + """In targeted mode, omitting search_field must NOT fall back to + unified search over all PII fields; it must use the configured + default field instead.""" + self.ICP.set_param("spp_registry_search.search_mode", "targeted") + # Configured field is 'name'; the term only matches via phone. + self.assertEqual(self._search("09171234567", search_field=None), []) + + def test_targeted_mode_invalid_field_uses_configured_default(self): + """An unknown search_field falls back to the configured default + field rather than widening or silently returning nothing.""" + self.ICP.set_param("spp_registry_search.search_mode", "targeted") + results = self._search("Alicia", search_field="bogus") + self.assertIn(self.alice.id, [r["id"] for r in results]) + + def test_targeted_mode_caller_field_honored(self): + """Regression: the portal lets users pick a valid field in targeted + mode; a valid caller-supplied field keeps working.""" + self.ICP.set_param("spp_registry_search.search_mode", "targeted") + results = self._search("0917123", search_field="phone") + self.assertIn(self.alice.id, [r["id"] for r in results]) + + # --- misc RPC-surface hardening ---------------------------------------------- + + def test_non_string_search_term_returns_empty(self): + """A non-string term must be rejected, not flow into ilike.""" + self.assertEqual(self._search({"weird": "input"}), []) + + def test_non_dict_advanced_filters_does_not_crash(self): + """A malformed advanced_filters value must be ignored, not raise + AttributeError (which would surface as a generic 500).""" + results = self._search("Alicia", advanced_filters="not-a-dict") + self.assertIn(self.alice.id, [r["id"] for r in results]) + + def test_targeted_mode_invalid_admin_target_field_fails_closed(self): + """If the admin-configured target_field is itself invalid, targeted + mode must fail closed (no results) rather than widen to unified.""" + self.ICP.set_param("spp_registry_search.search_mode", "targeted") + self.ICP.set_param("spp_registry_search.target_field", "bogus") + # Term matches via phone; a fallback to unified would find it. + self.assertEqual(self._search("09171234567", search_field=None), []) + + def test_search_type_filter_regression(self): + """Regression: search_type filtering still works.""" + results = self._search("Alicia", search_type="groups") + self.assertEqual(results, []) + results = self._search("Alicia", search_type="individuals") + self.assertIn(self.alice.id, [r["id"] for r in results])