From 086e44bdafd8213f34c539ab9b0d1f4713efc0e5 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Thu, 2 Jul 2026 12:36:43 +0800 Subject: [PATCH 1/2] security(oauth): restrict OAuth signing keys to system admins The module granted base.group_user read/write on res.config.settings, which exposes oauth_priv_key (the RS256 JWT signing key) and oauth_pub_key. Any internal user could read the private key via RPC and forge OAuth/JWT tokens, or overwrite the keypair to break/subvert authentication. - Remove the over-broad ir.model.access row. Odoo core already grants res.config.settings to base.group_system (read/write/create), so admins keep full access and can still save settings. - Override default_get to strip the signing-key fields for non-system-admins. default_get reads config_parameter values via sudo() with no model/field access check, so the ACL change alone would not stop key exfiltration over RPC. - Add regression tests asserting a plain internal user is denied model access and cannot read the keys via default_get, while a system admin retains both. Operators should treat the deployed keypair as compromised and rotate it. --- spp_oauth/__manifest__.py | 2 +- spp_oauth/models/res_config_settings.py | 16 +++++ spp_oauth/security/ir.model.access.csv | 1 - spp_oauth/tests/__init__.py | 1 + spp_oauth/tests/test_config_settings_acl.py | 76 +++++++++++++++++++++ 5 files changed, 94 insertions(+), 2 deletions(-) create mode 100644 spp_oauth/tests/test_config_settings_acl.py diff --git a/spp_oauth/__manifest__.py b/spp_oauth/__manifest__.py index e3b2530a..7cc8424c 100644 --- a/spp_oauth/__manifest__.py +++ b/spp_oauth/__manifest__.py @@ -3,7 +3,7 @@ "name": "OpenSPP API: Oauth", "summary": "The module establishes an OAuth 2.0 authentication framework, securing OpenSPP API communication for integrated systems and applications.", "category": "OpenSPP", - "version": "19.0.2.0.0", + "version": "19.0.2.0.1", "author": "OpenSPP.org", "development_status": "Production/Stable", "maintainers": ["jeremi", "gonzalesedwin1123", "reichie020212"], diff --git a/spp_oauth/models/res_config_settings.py b/spp_oauth/models/res_config_settings.py index c0ee50f9..2193b902 100644 --- a/spp_oauth/models/res_config_settings.py +++ b/spp_oauth/models/res_config_settings.py @@ -1,5 +1,11 @@ from odoo import fields, models +# The OAuth signing keys are only meaningful to system administrators. Access to +# res.config.settings is restricted to base.group_system via Odoo core's ACL (this +# module no longer widens it), but default_get() reads config_parameter fields with +# sudo() and performs no access check, so it is guarded explicitly below. +OAUTH_KEY_FIELDS = ("oauth_priv_key", "oauth_pub_key") + class RegistryConfig(models.TransientModel): _inherit = "res.config.settings" @@ -12,3 +18,13 @@ class RegistryConfig(models.TransientModel): string="OAuth Public Key", config_parameter="spp_oauth.oauth_pub_key", ) + + def default_get(self, fields_list): + res = super().default_get(fields_list) + # default_get sources config_parameter values via sudo(), bypassing model + # and field access checks. Never expose the OAuth signing keys to a user + # who is not a system administrator. + if not self.env.user.has_group("base.group_system"): + for field_name in OAUTH_KEY_FIELDS: + res.pop(field_name, None) + return res diff --git a/spp_oauth/security/ir.model.access.csv b/spp_oauth/security/ir.model.access.csv index fb353758..97dd8b91 100644 --- a/spp_oauth/security/ir.model.access.csv +++ b/spp_oauth/security/ir.model.access.csv @@ -1,2 +1 @@ id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink -access_res_config_settings_spp_oauth_user,res.config.settings spp_oauth user,base.model_res_config_settings,base.group_user,1,1,0,0 diff --git a/spp_oauth/tests/__init__.py b/spp_oauth/tests/__init__.py index 89149670..aba8cf61 100644 --- a/spp_oauth/tests/__init__.py +++ b/spp_oauth/tests/__init__.py @@ -1,2 +1,3 @@ from . import test_rsa_encode_decode from . import test_oauth_errors +from . import test_config_settings_acl diff --git a/spp_oauth/tests/test_config_settings_acl.py b/spp_oauth/tests/test_config_settings_acl.py new file mode 100644 index 00000000..3724ed4d --- /dev/null +++ b/spp_oauth/tests/test_config_settings_acl.py @@ -0,0 +1,76 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Security: OAuth signing keys must be restricted to system administrators. + +Regression test for "OAuth signing keys exposed to all internal users": the +module granted ``base.group_user`` read/write on ``res.config.settings``, which +exposes ``oauth_priv_key`` (the RS256 JWT signing key) and ``oauth_pub_key``. +Any internal user could therefore: + - read the model directly (``read``/``search``) via RPC, and + - retrieve the keys through ``res.config.settings.default_get()``, which reads + the backing ``ir.config_parameter`` values with ``sudo()`` and performs no + model-ACL or field-group check. + +Access must require a system administrator (``base.group_system``), and the +signing keys must not leak through ``default_get`` to non-admins. +""" + +from odoo import Command +from odoo.exceptions import AccessError +from odoo.tests import TransactionCase, tagged + +PRIV_KEY_SENTINEL = "TEST-OAUTH-PRIVATE-KEY-DO-NOT-LEAK" +PUB_KEY_SENTINEL = "TEST-OAUTH-PUBLIC-KEY" + + +@tagged("post_install", "-at_install") +class TestOAuthConfigSettingsAcl(TransactionCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + icp = cls.env["ir.config_parameter"].sudo() + icp.set_param("spp_oauth.oauth_priv_key", PRIV_KEY_SENTINEL) + icp.set_param("spp_oauth.oauth_pub_key", PUB_KEY_SENTINEL) + + # A plain internal user: base.group_user only, no system access. + cls.plain_user = cls.env["res.users"].create( + { + "name": "Plain Internal User", + "login": "plain_internal_oauth_test", + "group_ids": [Command.link(cls.env.ref("base.group_user").id)], + } + ) + # A system administrator. + cls.admin_user = cls.env["res.users"].create( + { + "name": "System Admin User", + "login": "system_admin_oauth_test", + "group_ids": [Command.link(cls.env.ref("base.group_system").id)], + } + ) + + def test_plain_user_cannot_access_settings_model(self): + """base.group_user (any internal user) must NOT have read access to + res.config.settings — that is the model exposing the signing keys.""" + with self.assertRaises(AccessError): + self.env["res.config.settings"].with_user(self.plain_user).check_access("read") + + def test_plain_user_cannot_read_keys_via_default_get(self): + """The signing keys must not leak to a non-admin through default_get(), + which reads the backing config parameters with sudo().""" + settings = self.env["res.config.settings"].with_user(self.plain_user) + values = settings.default_get(["oauth_priv_key", "oauth_pub_key"]) + self.assertNotIn("oauth_priv_key", values) + self.assertNotIn("oauth_pub_key", values) + + def test_admin_can_access_settings_model(self): + """A system administrator retains read access to res.config.settings.""" + # Raises AccessError only if admin access was wrongly removed. + self.env["res.config.settings"].with_user(self.admin_user).check_access("read") + + def test_admin_can_read_keys_via_default_get(self): + """A system administrator can still read the signing keys, so the + Settings UI keeps working for admins.""" + settings = self.env["res.config.settings"].with_user(self.admin_user) + values = settings.default_get(["oauth_priv_key", "oauth_pub_key"]) + self.assertEqual(values.get("oauth_priv_key"), PRIV_KEY_SENTINEL) + self.assertEqual(values.get("oauth_pub_key"), PUB_KEY_SENTINEL) From ba0b5f22d45c40817f6ef24baf3a9da5d931953f Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Thu, 2 Jul 2026 12:56:36 +0800 Subject: [PATCH 2/2] security(oauth): honour superuser mode in default_get key filter Per review: env.user stays the original (possibly non-admin) user under sudo() while env.su is True. Bypass the group check when env.su is set so trusted server-side sudo contexts still receive the keys. The RPC attack path is never in superuser mode, so the exposure stays closed. --- spp_oauth/models/res_config_settings.py | 6 ++++-- spp_oauth/tests/test_config_settings_acl.py | 11 +++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/spp_oauth/models/res_config_settings.py b/spp_oauth/models/res_config_settings.py index 2193b902..89121257 100644 --- a/spp_oauth/models/res_config_settings.py +++ b/spp_oauth/models/res_config_settings.py @@ -23,8 +23,10 @@ def default_get(self, fields_list): res = super().default_get(fields_list) # default_get sources config_parameter values via sudo(), bypassing model # and field access checks. Never expose the OAuth signing keys to a user - # who is not a system administrator. - if not self.env.user.has_group("base.group_system"): + # who is not a system administrator. Superuser mode (self.env.su) is a + # trusted server-side context that already bypasses ACLs, so honour it here + # too — env.user stays the original (possibly non-admin) user under sudo(). + if not self.env.su and not self.env.user.has_group("base.group_system"): for field_name in OAUTH_KEY_FIELDS: res.pop(field_name, None) return res diff --git a/spp_oauth/tests/test_config_settings_acl.py b/spp_oauth/tests/test_config_settings_acl.py index 3724ed4d..44ed60df 100644 --- a/spp_oauth/tests/test_config_settings_acl.py +++ b/spp_oauth/tests/test_config_settings_acl.py @@ -62,6 +62,17 @@ def test_plain_user_cannot_read_keys_via_default_get(self): self.assertNotIn("oauth_priv_key", values) self.assertNotIn("oauth_pub_key", values) + def test_superuser_mode_bypasses_key_filter(self): + """Superuser mode (sudo) is a trusted server-side context that already + bypasses ACLs; default_get must not strip the keys there, even though + env.user remains the original non-admin user. The RPC attack path is + never in superuser mode, so this does not reopen the exposure.""" + settings = self.env["res.config.settings"].with_user(self.plain_user).sudo() + self.assertTrue(settings.env.su) + values = settings.default_get(["oauth_priv_key", "oauth_pub_key"]) + self.assertEqual(values.get("oauth_priv_key"), PRIV_KEY_SENTINEL) + self.assertEqual(values.get("oauth_pub_key"), PUB_KEY_SENTINEL) + def test_admin_can_access_settings_model(self): """A system administrator retains read access to res.config.settings.""" # Raises AccessError only if admin access was wrongly removed.