From e2417d0510dcad8b143d90913ade6d4d33cf8f6a Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Thu, 23 Jul 2026 11:35:00 +0800 Subject: [PATCH 1/6] test(security): pin that DCI admin group must not grant system admin TDD red: the group currently implies base.group_system, escalating any DCI Administrator to full Settings/System admin. New tests assert the implication is gone (direct and transitive), that spp_admin implies the group instead, and that the 19.0.2.0.2 migration strips the link from existing databases. --- spp_dci/tests/test_dci_admin_group.py | 70 ++++++++++++++++++++++++++- 1 file changed, 68 insertions(+), 2 deletions(-) diff --git a/spp_dci/tests/test_dci_admin_group.py b/spp_dci/tests/test_dci_admin_group.py index b02024371..a6476a055 100644 --- a/spp_dci/tests/test_dci_admin_group.py +++ b/spp_dci/tests/test_dci_admin_group.py @@ -1,6 +1,10 @@ # Part of OpenSPP. See LICENSE file for full copyright and licensing details. """Tests for the DCI Administrator security group.""" +import importlib.util +from pathlib import Path + +from odoo import Command from odoo.tests import TransactionCase, tagged @@ -10,6 +14,10 @@ class TestDCIAdminGroup(TransactionCase): raw payloads or PII. If the xml_id disappears or the group is replaced silently, those view gates degrade to "visible to everyone" - this pinning test catches that. + + The group must NEVER imply base.group_system: implied_ids grants the + implied groups to members, so such a link would turn a scoped PII + visibility role into full Settings/System administration. """ def test_dci_admin_group_exists(self): @@ -17,8 +25,66 @@ def test_dci_admin_group_exists(self): self.assertTrue(group, "spp_dci.group_dci_admin must exist") self.assertEqual(group.name, "DCI Administrator") - def test_dci_admin_group_implies_system(self): - """Members must already be system administrators.""" + def test_dci_admin_does_not_grant_system(self): + """Granting DCI Administrator must not escalate to system admin.""" + group = self.env.ref("spp_dci.group_dci_admin") + system = self.env.ref("base.group_system") + self.assertNotIn(system, group.implied_ids) + # Guard the transitive closure too, so the escalation cannot come + # back through an intermediate group. + self.assertNotIn(system, group.all_implied_ids) + + user = self.env["res.users"].create( + { + "name": "DCI Admin Only", + "login": "dci_admin_only", + "group_ids": [ + Command.link(self.env.ref("base.group_user").id), + Command.link(group.id), + ], + } + ) + self.assertTrue(user.has_group("spp_dci.group_dci_admin")) + self.assertFalse( + user.has_group("base.group_system"), + "DCI Administrator membership must not confer system administration", + ) + + def test_spp_admin_implies_dci_admin(self): + """OpenSPP admins (and thus system admins) keep PII visibility.""" + group = self.env.ref("spp_dci.group_dci_admin") + spp_admin = self.env.ref("spp_security.group_spp_admin") + self.assertIn(group, spp_admin.all_implied_ids) + + admin_user = self.env["res.users"].create( + { + "name": "DCI System Admin", + "login": "dci_system_admin", + "group_ids": [Command.link(self.env.ref("base.group_system").id)], + } + ) + self.assertTrue(admin_user.has_group("spp_dci.group_dci_admin")) + + def test_migration_removes_escalation(self): + """The 19.0.2.0.2 migration strips base.group_system from the + noupdate'd group record on databases installed before the fix.""" + migration_path = ( + Path(__file__).parent.parent / "migrations" / "19.0.2.0.2" / "post-migration.py" + ) + spec = importlib.util.spec_from_file_location( + "spp_dci_migration_19_0_2_0_2", migration_path + ) + migration = importlib.util.module_from_spec(spec) + spec.loader.exec_module(migration) + group = self.env.ref("spp_dci.group_dci_admin") system = self.env.ref("base.group_system") + # Recreate the released (vulnerable) state. + group.write({"implied_ids": [Command.link(system.id)]}) self.assertIn(system, group.implied_ids) + + migration.migrate(self.env.cr, "19.0.2.0.1") + + group.invalidate_recordset() + self.assertNotIn(system, group.implied_ids) + self.assertNotIn(system, group.all_implied_ids) From a5f10002a38f342f36396adf4b84ce399adb92ed Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Thu, 23 Jul 2026 11:37:10 +0800 Subject: [PATCH 2/6] security(dci): stop DCI Administrator group from granting system admin spp_dci.group_dci_admin listed base.group_system in implied_ids. In Odoo, implied_ids grants the implied groups to members - it is not a membership precondition - so assigning the scoped DCI/PII visibility role silently made the user a full Settings/System administrator. - drop the base.group_system link from the group (noupdate record) - link the group into spp_security.group_spp_admin.implied_ids instead, per the extension pattern documented in groups_admin.xml, so OpenSPP and system admins keep visibility of the gated PII fields - ship migrations/19.0.2.0.2/post-migration.py to strip the unsafe link from databases installed from the 2026.07 release, where the noupdate flag blocks the XML fix; membership is a computed transitive closure, so removal revokes the escalation immediately - bump to 19.0.2.0.2 with HISTORY fragment --- spp_dci/__manifest__.py | 2 +- .../migrations/19.0.2.0.2/post-migration.py | 40 +++++++++++++++++++ spp_dci/readme/HISTORY.md | 9 +++++ spp_dci/security/dci_groups.xml | 38 ++++++++++++------ 4 files changed, 76 insertions(+), 13 deletions(-) create mode 100644 spp_dci/migrations/19.0.2.0.2/post-migration.py diff --git a/spp_dci/__manifest__.py b/spp_dci/__manifest__.py index f4922840f..d388e6970 100644 --- a/spp_dci/__manifest__.py +++ b/spp_dci/__manifest__.py @@ -2,7 +2,7 @@ "name": "OpenSPP DCI Core", "summary": "Core DCI (Digital Convergence Initiative) API components", "category": "OpenSPP/Integration", - "version": "19.0.2.0.1", + "version": "19.0.2.0.2", "author": "OpenSPP.org", "website": "https://github.com/OpenSPP/OpenSPP2", "license": "LGPL-3", diff --git a/spp_dci/migrations/19.0.2.0.2/post-migration.py b/spp_dci/migrations/19.0.2.0.2/post-migration.py new file mode 100644 index 000000000..89a16fd85 --- /dev/null +++ b/spp_dci/migrations/19.0.2.0.2/post-migration.py @@ -0,0 +1,40 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Remove the base.group_system implication from the DCI Administrator group. + +Earlier versions shipped ``spp_dci.group_dci_admin`` with ``implied_ids`` +linking ``base.group_system``. ``implied_ids`` GRANTS the implied groups to +members, so any user given the DCI PII-visibility role silently became a +full Settings/System administrator. The group record is ``noupdate``, so +the corrected XML never reaches already-installed databases - strip the +link here. Odoo computes effective membership as a transitive closure of +``implied_ids``, so removing the link immediately revokes the escalated +privileges from affected users. +""" + +import logging + +from odoo import SUPERUSER_ID, Command, api + +_logger = logging.getLogger(__name__) + + +def migrate(cr, version): + if not version: + return + + env = api.Environment(cr, SUPERUSER_ID, {}) + group = env.ref("spp_dci.group_dci_admin", raise_if_not_found=False) + system = env.ref("base.group_system", raise_if_not_found=False) + if not group or not system or system not in group.implied_ids: + return + + affected = len(group.user_ids) + group.write({"implied_ids": [Command.unlink(system.id)]}) + _logger.warning( + "Removed the base.group_system implication from the DCI Administrator " + "group; %s user(s) held the group and lose the transitively granted " + "system administration rights. Audit changes made by these users while " + "escalated, and grant base.group_system explicitly where it is " + "genuinely intended.", + affected, + ) diff --git a/spp_dci/readme/HISTORY.md b/spp_dci/readme/HISTORY.md index 4aaf9afef..87dd868c1 100644 --- a/spp_dci/readme/HISTORY.md +++ b/spp_dci/readme/HISTORY.md @@ -1,3 +1,12 @@ +### 19.0.2.0.2 + +- fix(security): stop the DCI Administrator group from granting full system + administration. `implied_ids` grants the implied groups to members, so the + previous `base.group_system` link escalated any holder of the DCI + PII-visibility role to a Settings/System administrator. OpenSPP admins now + imply the group instead (preserving admin visibility of gated PII fields), + and a migration strips the unsafe link from existing databases. + ### 19.0.2.0.0 - Initial migration to OpenSPP2 diff --git a/spp_dci/security/dci_groups.xml b/spp_dci/security/dci_groups.xml index 7bfcf22f8..617604ac0 100644 --- a/spp_dci/security/dci_groups.xml +++ b/spp_dci/security/dci_groups.xml @@ -1,15 +1,29 @@ - - - - DCI Administrator - - Grants visibility to raw DCI payloads, full identifiers, disability data, and other sensitive fields exposed by the DCI cache and log models. Members must already be system administrators. + + + + + DCI Administrator + Grants visibility to raw DCI payloads, full identifiers, disability data, and other sensitive fields exposed by the DCI cache and log models. + + + + + + From fb3ae74d526a681050e5c176aacb4fe4a673a4c8 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Thu, 23 Jul 2026 11:43:15 +0800 Subject: [PATCH 3/6] style: apply ruff-format to DCI admin group test --- spp_dci/tests/test_dci_admin_group.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/spp_dci/tests/test_dci_admin_group.py b/spp_dci/tests/test_dci_admin_group.py index a6476a055..9a6588cea 100644 --- a/spp_dci/tests/test_dci_admin_group.py +++ b/spp_dci/tests/test_dci_admin_group.py @@ -68,12 +68,8 @@ def test_spp_admin_implies_dci_admin(self): def test_migration_removes_escalation(self): """The 19.0.2.0.2 migration strips base.group_system from the noupdate'd group record on databases installed before the fix.""" - migration_path = ( - Path(__file__).parent.parent / "migrations" / "19.0.2.0.2" / "post-migration.py" - ) - spec = importlib.util.spec_from_file_location( - "spp_dci_migration_19_0_2_0_2", migration_path - ) + migration_path = Path(__file__).parent.parent / "migrations" / "19.0.2.0.2" / "post-migration.py" + spec = importlib.util.spec_from_file_location("spp_dci_migration_19_0_2_0_2", migration_path) migration = importlib.util.module_from_spec(spec) spec.loader.exec_module(migration) From 2bc1db8ab7fda40155d8e8ba9effe95c39c6fe87 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Thu, 23 Jul 2026 11:58:43 +0800 Subject: [PATCH 4/6] docs(spp_dci): regenerate README from fragments (CI oca-gen output) --- spp_dci/README.rst | 11 +++++++++++ spp_dci/static/description/index.html | 12 ++++++++++++ 2 files changed, 23 insertions(+) diff --git a/spp_dci/README.rst b/spp_dci/README.rst index 7d8f6be8a..a326976dc 100644 --- a/spp_dci/README.rst +++ b/spp_dci/README.rst @@ -132,6 +132,17 @@ Dependencies Changelog ========= +19.0.2.0.2 +~~~~~~~~~~ + +- fix(security): stop the DCI Administrator group from granting full + system administration. ``implied_ids`` grants the implied groups to + members, so the previous ``base.group_system`` link escalated any + holder of the DCI PII-visibility role to a Settings/System + administrator. OpenSPP admins now imply the group instead (preserving + admin visibility of gated PII fields), and a migration strips the + unsafe link from existing databases. + 19.0.2.0.0 ~~~~~~~~~~ diff --git a/spp_dci/static/description/index.html b/spp_dci/static/description/index.html index 2f60a3b00..cffa2ed01 100644 --- a/spp_dci/static/description/index.html +++ b/spp_dci/static/description/index.html @@ -504,6 +504,18 @@

Changelog

+

19.0.2.0.2

+
    +
  • fix(security): stop the DCI Administrator group from granting full +system administration. implied_ids grants the implied groups to +members, so the previous base.group_system link escalated any +holder of the DCI PII-visibility role to a Settings/System +administrator. OpenSPP admins now imply the group instead (preserving +admin visibility of gated PII fields), and a migration strips the +unsafe link from existing databases.
    • +
+
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • From b61c3b134d9db18350c6b6ad3a1c382ca3991d0c Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Thu, 23 Jul 2026 12:04:45 +0800 Subject: [PATCH 5/6] security(dci): refresh stale group comment in migration; pin its warning Review follow-ups: the noupdate record keeps the old comment (which documented the inverted implied_ids mental model) on upgraded databases, so the migration now refreshes it; the migration test captures the operator-facing warning with assertLogs so test output stays clean and the message is pinned. --- spp_dci/migrations/19.0.2.0.2/post-migration.py | 14 +++++++++++++- spp_dci/tests/test_dci_admin_group.py | 5 ++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/spp_dci/migrations/19.0.2.0.2/post-migration.py b/spp_dci/migrations/19.0.2.0.2/post-migration.py index 89a16fd85..32735bf73 100644 --- a/spp_dci/migrations/19.0.2.0.2/post-migration.py +++ b/spp_dci/migrations/19.0.2.0.2/post-migration.py @@ -29,7 +29,19 @@ def migrate(cr, version): return affected = len(group.user_ids) - group.write({"implied_ids": [Command.unlink(system.id)]}) + group.write( + { + "implied_ids": [Command.unlink(system.id)], + # The record is noupdate, so also refresh the comment that + # documented the inverted mental model ("Members must already + # be system administrators"). + "comment": ( + "Grants visibility to raw DCI payloads, full identifiers, " + "disability data, and other sensitive fields exposed by the " + "DCI cache and log models." + ), + } + ) _logger.warning( "Removed the base.group_system implication from the DCI Administrator " "group; %s user(s) held the group and lose the transitively granted " diff --git a/spp_dci/tests/test_dci_admin_group.py b/spp_dci/tests/test_dci_admin_group.py index 9a6588cea..a5f027f59 100644 --- a/spp_dci/tests/test_dci_admin_group.py +++ b/spp_dci/tests/test_dci_admin_group.py @@ -79,8 +79,11 @@ def test_migration_removes_escalation(self): group.write({"implied_ids": [Command.link(system.id)]}) self.assertIn(system, group.implied_ids) - migration.migrate(self.env.cr, "19.0.2.0.1") + with self.assertLogs("spp_dci_migration_19_0_2_0_2", level="WARNING") as capture: + migration.migrate(self.env.cr, "19.0.2.0.1") + self.assertIn("Removed the base.group_system implication", capture.output[0]) group.invalidate_recordset() self.assertNotIn(system, group.implied_ids) self.assertNotIn(system, group.all_implied_ids) + self.assertNotIn("must already be system administrators", group.comment) From 33cf13616742f86a6ac47b7a7ad1d501db60e3a1 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Thu, 23 Jul 2026 16:27:58 +0800 Subject: [PATCH 6/6] security(dci): refresh comment idempotently; pin migration idempotence Staff-review polish (no behaviour change to the escalation fix): - the migration now refreshes the stale group comment even on databases where the base.group_system link was already removed manually, and only logs/counts when it actually unlinks - the migration test simulates the released stale comment and asserts a second run is a silent no-op --- .../migrations/19.0.2.0.2/post-migration.py | 55 +++++++++++-------- spp_dci/tests/test_dci_admin_group.py | 15 ++++- 2 files changed, 45 insertions(+), 25 deletions(-) diff --git a/spp_dci/migrations/19.0.2.0.2/post-migration.py b/spp_dci/migrations/19.0.2.0.2/post-migration.py index 32735bf73..ca6e2634a 100644 --- a/spp_dci/migrations/19.0.2.0.2/post-migration.py +++ b/spp_dci/migrations/19.0.2.0.2/post-migration.py @@ -24,29 +24,38 @@ def migrate(cr, version): env = api.Environment(cr, SUPERUSER_ID, {}) group = env.ref("spp_dci.group_dci_admin", raise_if_not_found=False) - system = env.ref("base.group_system", raise_if_not_found=False) - if not group or not system or system not in group.implied_ids: + if not group: return - affected = len(group.user_ids) - group.write( - { - "implied_ids": [Command.unlink(system.id)], - # The record is noupdate, so also refresh the comment that - # documented the inverted mental model ("Members must already - # be system administrators"). - "comment": ( - "Grants visibility to raw DCI payloads, full identifiers, " - "disability data, and other sensitive fields exposed by the " - "DCI cache and log models." - ), - } - ) - _logger.warning( - "Removed the base.group_system implication from the DCI Administrator " - "group; %s user(s) held the group and lose the transitively granted " - "system administration rights. Audit changes made by these users while " - "escalated, and grant base.group_system explicitly where it is " - "genuinely intended.", - affected, + system = env.ref("base.group_system", raise_if_not_found=False) + escalated = bool(system) and system in group.implied_ids + + vals = {} + # The record is noupdate, so refresh the comment that documented the + # inverted mental model ("Members must already be system administrators") + # even on databases where the link was already removed manually. + correct_comment = ( + "Grants visibility to raw DCI payloads, full identifiers, " + "disability data, and other sensitive fields exposed by the " + "DCI cache and log models." ) + if group.comment != correct_comment: + vals["comment"] = correct_comment + if escalated: + affected = len(group.user_ids) + vals["implied_ids"] = [Command.unlink(system.id)] + + if not vals: + return + + group.write(vals) + + if escalated: + _logger.warning( + "Removed the base.group_system implication from the DCI Administrator " + "group; %s user(s) held the group and lose the transitively granted " + "system administration rights. Audit changes made by these users while " + "escalated, and grant base.group_system explicitly where it is " + "genuinely intended.", + affected, + ) diff --git a/spp_dci/tests/test_dci_admin_group.py b/spp_dci/tests/test_dci_admin_group.py index a5f027f59..eb77b7987 100644 --- a/spp_dci/tests/test_dci_admin_group.py +++ b/spp_dci/tests/test_dci_admin_group.py @@ -75,8 +75,14 @@ def test_migration_removes_escalation(self): group = self.env.ref("spp_dci.group_dci_admin") system = self.env.ref("base.group_system") - # Recreate the released (vulnerable) state. - group.write({"implied_ids": [Command.link(system.id)]}) + # Recreate the released (vulnerable) state: the escalating link and + # the stale comment that the noupdate record would have preserved. + group.write( + { + "implied_ids": [Command.link(system.id)], + "comment": "Sensitive. Members must already be system administrators.", + } + ) self.assertIn(system, group.implied_ids) with self.assertLogs("spp_dci_migration_19_0_2_0_2", level="WARNING") as capture: @@ -87,3 +93,8 @@ def test_migration_removes_escalation(self): self.assertNotIn(system, group.implied_ids) self.assertNotIn(system, group.all_implied_ids) self.assertNotIn("must already be system administrators", group.comment) + + # Idempotent: a second run finds the link absent and the comment + # already refreshed, so it makes no changes and stays silent. + with self.assertNoLogs("spp_dci_migration_19_0_2_0_2", level="WARNING"): + migration.migrate(self.env.cr, "19.0.2.0.1")