From 92bc095f8f668cd47017673f3db5c19eb4720db8 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 24 Jul 2026 16:34:02 +0800 Subject: [PATCH 1/4] test(security): pin that storage credentials are hidden from base.group_user TDD red: spp.storage.backend grants base.group_user model read and the S3 access/secret keys + Azure connection string have no field-level groups, so any internal user can read the raw credentials via read/search_read. New tests assert base.group_user cannot read the three credential fields (read + search_read), that they carry a group restriction, while storage admins and the server-side client builders still resolve them. --- spp_storage_backend/tests/__init__.py | 1 + .../tests/test_credential_access.py | 158 ++++++++++++++++++ 2 files changed, 159 insertions(+) create mode 100644 spp_storage_backend/tests/test_credential_access.py diff --git a/spp_storage_backend/tests/__init__.py b/spp_storage_backend/tests/__init__.py index ee45687b6..928ff0bda 100644 --- a/spp_storage_backend/tests/__init__.py +++ b/spp_storage_backend/tests/__init__.py @@ -1,3 +1,4 @@ # Part of OpenSPP. See LICENSE file for full copyright and licensing details. from . import test_storage_backend +from . import test_credential_access diff --git a/spp_storage_backend/tests/test_credential_access.py b/spp_storage_backend/tests/test_credential_access.py new file mode 100644 index 000000000..e43b3674e --- /dev/null +++ b/spp_storage_backend/tests/test_credential_access.py @@ -0,0 +1,158 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Storage backend credentials must not be readable by ordinary internal users. + +The model stores S3 access/secret keys and Azure connection strings. Ordinary +`base.group_user` users get model read access (to resolve/use a backend), but +must never be able to read the raw credentials over RPC/`read`/`search_read`. +Only storage administrators (and trusted server-side `sudo()` flows, e.g. the +S3/Azure client builders) may resolve the credential values. +""" + +import sys +from unittest.mock import MagicMock + +from odoo.exceptions import AccessError +from odoo.tests.common import TransactionCase, tagged + +S3_SECRET = "super-secret-s3-key" +S3_ACCESS = "super-access-s3-id" +AZURE_CONN = "DefaultEndpointsProtocol=https;AccountName=x;AccountKey=SECRETKEY==;" +CRED_FIELDS = ("s3_access_key", "s3_secret_key", "azure_connection_string") + + +@tagged("post_install", "-at_install") +class TestStorageCredentialAccess(TransactionCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + Backend = cls.env["spp.storage.backend"] + cls.s3_backend = Backend.create( + { + "name": "S3 Cred Backend", + "backend_type": "s3", + "s3_bucket": "bucket", + "s3_access_key": S3_ACCESS, + "s3_secret_key": S3_SECRET, + "s3_region": "us-east-1", + } + ) + cls.azure_backend = Backend.create( + { + "name": "Azure Cred Backend", + "backend_type": "azure", + "azure_connection_string": AZURE_CONN, + "azure_container": "container", + } + ) + cls.base_user = cls.env["res.users"].create( + { + "name": "Plain Internal User", + "login": "storage_plain_user", + "group_ids": [(6, 0, [cls.env.ref("base.group_user").id])], + } + ) + cls.storage_admin = cls.env["res.users"].create( + { + "name": "Storage Admin User", + "login": "storage_admin_user", + "group_ids": [ + (6, 0, [ + cls.env.ref("base.group_user").id, + cls.env.ref("spp_storage_backend.group_storage_admin").id, + ]) + ], + } + ) + + def _read_field(self, backend, user, field): + """Return the field value as seen by `user`, or None if blocked.""" + try: + return backend.with_user(user).read([field])[0].get(field) + except AccessError: + return None + + # --- credentials hidden from ordinary users ----------------------- + + def test_base_user_cannot_read_s3_secret(self): + self.assertNotEqual( + self._read_field(self.s3_backend, self.base_user, "s3_secret_key"), + S3_SECRET, + "base.group_user must not be able to read s3_secret_key", + ) + + def test_base_user_cannot_read_s3_access_key(self): + self.assertNotEqual( + self._read_field(self.s3_backend, self.base_user, "s3_access_key"), + S3_ACCESS, + ) + + def test_base_user_cannot_read_azure_connection_string(self): + self.assertNotEqual( + self._read_field(self.azure_backend, self.base_user, "azure_connection_string"), + AZURE_CONN, + ) + + def test_base_user_search_read_excludes_credentials(self): + rows = self.env["spp.storage.backend"].with_user(self.base_user).search_read([], list(CRED_FIELDS)) + for row in rows: + for field in CRED_FIELDS: + self.assertFalse(row.get(field), f"{field} leaked via search_read") + + def test_credential_fields_are_group_restricted(self): + info = self.env["spp.storage.backend"].fields_get(list(CRED_FIELDS)) + for field in CRED_FIELDS: + self.assertTrue( + info[field].get("groups"), + f"{field} must carry a field-level groups restriction", + ) + + # --- admins and trusted flows can still resolve them -------------- + + def test_storage_admin_can_read_credentials(self): + self.assertEqual( + self._read_field(self.s3_backend, self.storage_admin, "s3_secret_key"), + S3_SECRET, + ) + self.assertEqual( + self._read_field(self.azure_backend, self.storage_admin, "azure_connection_string"), + AZURE_CONN, + ) + + def test_s3_client_builder_resolves_credentials_for_non_admin(self): + """The S3 client is built server-side with the real credentials even + when the operating user cannot read them (use-without-see via sudo).""" + fake_boto3 = MagicMock() + with patch_dict_modules({"boto3": fake_boto3}): + self.s3_backend.with_user(self.base_user)._get_s3_client() + _, kwargs = fake_boto3.client.call_args + self.assertEqual(kwargs.get("aws_secret_access_key"), S3_SECRET) + self.assertEqual(kwargs.get("aws_access_key_id"), S3_ACCESS) + + def test_azure_client_builder_resolves_credentials_for_non_admin(self): + fake_module = MagicMock() + with patch_dict_modules({"azure": MagicMock(), "azure.storage": MagicMock(), "azure.storage.blob": fake_module}): + self.azure_backend.with_user(self.base_user)._get_azure_client() + args, _ = fake_module.BlobServiceClient.from_connection_string.call_args + self.assertEqual(args[0], AZURE_CONN) + + +class patch_dict_modules: + """Context manager to temporarily inject modules into sys.modules.""" + + def __init__(self, mapping): + self.mapping = mapping + self._saved = {} + + def __enter__(self): + for name, mod in self.mapping.items(): + self._saved[name] = sys.modules.get(name) + sys.modules[name] = mod + return self + + def __exit__(self, *exc): + for name, prev in self._saved.items(): + if prev is None: + sys.modules.pop(name, None) + else: + sys.modules[name] = prev + return False From 70295eb6a54451c3ea14a79e2c088f0aa7f5ef10 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 24 Jul 2026 16:50:10 +0800 Subject: [PATCH 2/4] security(storage): restrict backend credentials to storage admins spp.storage.backend stored S3 access/secret keys and Azure connection strings as plain Char fields, and ir.model.access.csv grants base.group_user model read, so any internal user could read the raw credentials via read/search_read and use them to reach the configured bucket/container outside Odoo (password=True only masks the widget). - add groups="spp_storage_backend.group_storage_admin" to s3_access_key, s3_secret_key and azure_connection_string so Odoo strips them from read/search_read for non-admins (explicit requests raise AccessError) - resolve the credentials via self.sudo() inside _get_s3_client and _get_azure_client so a non-admin who is allowed to operate a backend can still store/retrieve without being able to read the secret (use-without-see) - keep base.group_user model read (non-secret fields stay usable) - bump to 19.0.2.0.1 with HISTORY fragment; no migration (field metadata, no schema/data change) --- spp_storage_backend/__manifest__.py | 2 +- spp_storage_backend/models/storage_backend.py | 19 +++++-- spp_storage_backend/readme/HISTORY.md | 10 ++++ .../tests/test_credential_access.py | 54 ++++++++----------- 4 files changed, 49 insertions(+), 36 deletions(-) diff --git a/spp_storage_backend/__manifest__.py b/spp_storage_backend/__manifest__.py index 2c76ef29d..c0b12340d 100644 --- a/spp_storage_backend/__manifest__.py +++ b/spp_storage_backend/__manifest__.py @@ -4,7 +4,7 @@ "name": "OpenSPP Storage Backend", "summary": "Pluggable storage backend configuration for OpenSPP file storage", "category": "OpenSPP/Core", - "version": "19.0.2.0.0", + "version": "19.0.2.0.1", "author": "OpenSPP.org", "website": "https://github.com/OpenSPP/OpenSPP2", "license": "LGPL-3", diff --git a/spp_storage_backend/models/storage_backend.py b/spp_storage_backend/models/storage_backend.py index 2404f5914..27b8944c2 100644 --- a/spp_storage_backend/models/storage_backend.py +++ b/spp_storage_backend/models/storage_backend.py @@ -58,10 +58,12 @@ class StorageBackend(models.Model): ) s3_access_key = fields.Char( string="Access Key", + groups="spp_storage_backend.group_storage_admin", help="AWS Access Key ID or S3-compatible access key", ) s3_secret_key = fields.Char( string="Secret Key", + groups="spp_storage_backend.group_storage_admin", help="AWS Secret Access Key or S3-compatible secret key", ) s3_region = fields.Char( @@ -78,6 +80,7 @@ class StorageBackend(models.Model): # Azure Configuration azure_connection_string = fields.Char( string="Connection String", + groups="spp_storage_backend.group_storage_admin", help="Azure Storage connection string", ) azure_container = fields.Char( @@ -363,9 +366,14 @@ def _get_s3_client(self): except ImportError as e: raise UserError(_("boto3 library is not installed. Please install it to use S3 storage.")) from e + # Credentials are group-restricted (admin-only readable); resolve them + # server-side so an operating user can use the backend without being + # able to read the secret. The value is only used to build the client. + # nosemgrep: odoo-sudo-without-context - read admin-only credential fields to build the client + creds = self.sudo() config = { - "aws_access_key_id": self.s3_access_key, - "aws_secret_access_key": self.s3_secret_key, + "aws_access_key_id": creds.s3_access_key, + "aws_secret_access_key": creds.s3_secret_key, "region_name": self.s3_region, } @@ -452,7 +460,12 @@ def _get_azure_client(self): _("azure-storage-blob library is not installed. Please install it to use Azure storage.") ) from e - service_client = BlobServiceClient.from_connection_string(self.azure_connection_string) + # Connection string is group-restricted (admin-only readable); resolve + # it server-side so an operating user can use the backend without being + # able to read the secret. + # nosemgrep: odoo-sudo-without-context - read admin-only credential field to build the client + connection_string = self.sudo().azure_connection_string + service_client = BlobServiceClient.from_connection_string(connection_string) return service_client.get_container_client(self.azure_container) def _store_azure(self, binary_data, path): diff --git a/spp_storage_backend/readme/HISTORY.md b/spp_storage_backend/readme/HISTORY.md index 4aaf9afef..efd87404b 100644 --- a/spp_storage_backend/readme/HISTORY.md +++ b/spp_storage_backend/readme/HISTORY.md @@ -1,3 +1,13 @@ +### 19.0.2.0.1 + +- fix(security): restrict storage backend credentials to storage administrators. + The S3 access/secret keys and Azure connection string were readable by any + internal user via RPC (`base.group_user` had model read and the fields had no + field-level protection). The three credential fields are now gated with + `groups="spp_storage_backend.group_storage_admin"`, and the S3/Azure client + builders resolve them via `sudo()` so a non-admin can still operate a backend + without being able to read the secret. + ### 19.0.2.0.0 - Initial migration to OpenSPP2 diff --git a/spp_storage_backend/tests/test_credential_access.py b/spp_storage_backend/tests/test_credential_access.py index e43b3674e..ec2b533ab 100644 --- a/spp_storage_backend/tests/test_credential_access.py +++ b/spp_storage_backend/tests/test_credential_access.py @@ -9,7 +9,7 @@ """ import sys -from unittest.mock import MagicMock +from unittest.mock import MagicMock, patch from odoo.exceptions import AccessError from odoo.tests.common import TransactionCase, tagged @@ -56,10 +56,14 @@ def setUpClass(cls): "name": "Storage Admin User", "login": "storage_admin_user", "group_ids": [ - (6, 0, [ - cls.env.ref("base.group_user").id, - cls.env.ref("spp_storage_backend.group_storage_admin").id, - ]) + ( + 6, + 0, + [ + cls.env.ref("base.group_user").id, + cls.env.ref("spp_storage_backend.group_storage_admin").id, + ], + ) ], } ) @@ -92,11 +96,18 @@ def test_base_user_cannot_read_azure_connection_string(self): AZURE_CONN, ) - def test_base_user_search_read_excludes_credentials(self): - rows = self.env["spp.storage.backend"].with_user(self.base_user).search_read([], list(CRED_FIELDS)) + def test_base_user_search_read_explicit_credentials_denied(self): + # Explicitly requesting a restricted field over RPC raises AccessError. + with self.assertRaises(AccessError): + self.env["spp.storage.backend"].with_user(self.base_user).search_read([], list(CRED_FIELDS)) + + def test_base_user_default_read_excludes_credentials(self): + # Reading the accessible fields must not surface the credentials. + rows = self.env["spp.storage.backend"].with_user(self.base_user).search_read([]) + self.assertTrue(rows) for row in rows: for field in CRED_FIELDS: - self.assertFalse(row.get(field), f"{field} leaked via search_read") + self.assertNotIn(field, row, f"{field} leaked via default search_read") def test_credential_fields_are_group_restricted(self): info = self.env["spp.storage.backend"].fields_get(list(CRED_FIELDS)) @@ -122,7 +133,7 @@ def test_s3_client_builder_resolves_credentials_for_non_admin(self): """The S3 client is built server-side with the real credentials even when the operating user cannot read them (use-without-see via sudo).""" fake_boto3 = MagicMock() - with patch_dict_modules({"boto3": fake_boto3}): + with patch.dict(sys.modules, {"boto3": fake_boto3}): self.s3_backend.with_user(self.base_user)._get_s3_client() _, kwargs = fake_boto3.client.call_args self.assertEqual(kwargs.get("aws_secret_access_key"), S3_SECRET) @@ -130,29 +141,8 @@ def test_s3_client_builder_resolves_credentials_for_non_admin(self): def test_azure_client_builder_resolves_credentials_for_non_admin(self): fake_module = MagicMock() - with patch_dict_modules({"azure": MagicMock(), "azure.storage": MagicMock(), "azure.storage.blob": fake_module}): + mods = {"azure": MagicMock(), "azure.storage": MagicMock(), "azure.storage.blob": fake_module} + with patch.dict(sys.modules, mods): self.azure_backend.with_user(self.base_user)._get_azure_client() args, _ = fake_module.BlobServiceClient.from_connection_string.call_args self.assertEqual(args[0], AZURE_CONN) - - -class patch_dict_modules: - """Context manager to temporarily inject modules into sys.modules.""" - - def __init__(self, mapping): - self.mapping = mapping - self._saved = {} - - def __enter__(self): - for name, mod in self.mapping.items(): - self._saved[name] = sys.modules.get(name) - sys.modules[name] = mod - return self - - def __exit__(self, *exc): - for name, prev in self._saved.items(): - if prev is None: - sys.modules.pop(name, None) - else: - sys.modules[name] = prev - return False From 42f281206e19f640eafa1ba6d4cea29122cb92e5 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 24 Jul 2026 17:01:53 +0800 Subject: [PATCH 3/4] security(storage): sudo the second Azure connection-string read Staff review found _get_azure_public_url read self.azure_connection_string directly (no sudo), so SAS-URL generation would fail with AccessError for the non-admin operating users the fix supports (fails closed; latent as nothing calls get_public_url yet). Extract a shared _get_azure_connection_string() sudo helper and use it in both _get_azure_client and _get_azure_public_url; add a regression test that the SAS path resolves the credential for a base.group_user. --- spp_storage_backend/models/storage_backend.py | 23 ++++++++++++------- .../tests/test_credential_access.py | 11 +++++++++ 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/spp_storage_backend/models/storage_backend.py b/spp_storage_backend/models/storage_backend.py index 27b8944c2..9aabeebcc 100644 --- a/spp_storage_backend/models/storage_backend.py +++ b/spp_storage_backend/models/storage_backend.py @@ -451,6 +451,16 @@ def _test_s3_connection(self): # AZURE BACKEND IMPLEMENTATION # ======================================================================== + def _get_azure_connection_string(self): + """Resolve the group-restricted Azure connection string server-side. + + The field is admin-only readable; an operating user may use the backend + without being able to read the secret, so it is read via ``sudo()`` and + only used to build clients / SAS URLs (never returned to the caller). + """ + # nosemgrep: odoo-sudo-without-context - read admin-only credential field to build the client + return self.sudo().azure_connection_string + def _get_azure_client(self): """Get configured Azure Blob client.""" try: @@ -460,12 +470,7 @@ def _get_azure_client(self): _("azure-storage-blob library is not installed. Please install it to use Azure storage.") ) from e - # Connection string is group-restricted (admin-only readable); resolve - # it server-side so an operating user can use the backend without being - # able to read the secret. - # nosemgrep: odoo-sudo-without-context - read admin-only credential field to build the client - connection_string = self.sudo().azure_connection_string - service_client = BlobServiceClient.from_connection_string(connection_string) + service_client = BlobServiceClient.from_connection_string(self._get_azure_connection_string()) return service_client.get_container_client(self.azure_container) def _store_azure(self, binary_data, path): @@ -510,8 +515,10 @@ def _get_azure_public_url(self, reference, expires_in): container_client = self._get_azure_client() blob_client = container_client.get_blob_client(reference) - # Extract account name and key from connection string - conn_parts = dict(item.split("=", 1) for item in self.azure_connection_string.split(";") if "=" in item) + # Extract account name and key from connection string (admin-only field + # resolved via sudo; used only to sign the SAS URL, not returned). + connection_string = self._get_azure_connection_string() + conn_parts = dict(item.split("=", 1) for item in connection_string.split(";") if "=" in item) account_name = conn_parts.get("AccountName") account_key = conn_parts.get("AccountKey") diff --git a/spp_storage_backend/tests/test_credential_access.py b/spp_storage_backend/tests/test_credential_access.py index ec2b533ab..41d682f20 100644 --- a/spp_storage_backend/tests/test_credential_access.py +++ b/spp_storage_backend/tests/test_credential_access.py @@ -146,3 +146,14 @@ def test_azure_client_builder_resolves_credentials_for_non_admin(self): self.azure_backend.with_user(self.base_user)._get_azure_client() args, _ = fake_module.BlobServiceClient.from_connection_string.call_args self.assertEqual(args[0], AZURE_CONN) + + def test_azure_public_url_resolves_credentials_for_non_admin(self): + """SAS-URL generation reads the connection string through the same sudo + helper, so it works for an operating non-admin user (regression: the + second Azure credential read site).""" + fake_module = MagicMock() + mods = {"azure": MagicMock(), "azure.storage": MagicMock(), "azure.storage.blob": fake_module} + with patch.dict(sys.modules, mods): + self.azure_backend.with_user(self.base_user)._get_azure_public_url("blob.txt", 3600) + _, kwargs = fake_module.generate_blob_sas.call_args + self.assertEqual(kwargs.get("account_key"), "SECRETKEY==") From 29f3112a086f9f870662fd8d03e710ad08d0ac85 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 24 Jul 2026 17:03:30 +0800 Subject: [PATCH 4/4] docs(spp_storage_backend): regenerate README from fragments (CI oca-gen output) --- spp_storage_backend/README.rst | 12 ++++++++++++ spp_storage_backend/static/description/index.html | 13 +++++++++++++ 2 files changed, 25 insertions(+) diff --git a/spp_storage_backend/README.rst b/spp_storage_backend/README.rst index 9307d00a2..2a118ba44 100644 --- a/spp_storage_backend/README.rst +++ b/spp_storage_backend/README.rst @@ -115,6 +115,18 @@ Dependencies Changelog ========= +19.0.2.0.1 +~~~~~~~~~~ + +- fix(security): restrict storage backend credentials to storage + administrators. The S3 access/secret keys and Azure connection string + were readable by any internal user via RPC (``base.group_user`` had + model read and the fields had no field-level protection). The three + credential fields are now gated with + ``groups="spp_storage_backend.group_storage_admin"``, and the S3/Azure + client builders resolve them via ``sudo()`` so a non-admin can still + operate a backend without being able to read the secret. + 19.0.2.0.0 ~~~~~~~~~~ diff --git a/spp_storage_backend/static/description/index.html b/spp_storage_backend/static/description/index.html index 34a3c642c..4d09de8fb 100644 --- a/spp_storage_backend/static/description/index.html +++ b/spp_storage_backend/static/description/index.html @@ -487,6 +487,19 @@

Changelog

+

19.0.2.0.1

+
    +
  • fix(security): restrict storage backend credentials to storage +administrators. The S3 access/secret keys and Azure connection string +were readable by any internal user via RPC (base.group_user had +model read and the fields had no field-level protection). The three +credential fields are now gated with +groups="spp_storage_backend.group_storage_admin", and the S3/Azure +client builders resolve them via sudo() so a non-admin can still +operate a backend without being able to read the secret.
    • +
+
+

19.0.2.0.0

  • Initial migration to OpenSPP2