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/__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..ca6e2634a --- /dev/null +++ b/spp_dci/migrations/19.0.2.0.2/post-migration.py @@ -0,0 +1,61 @@ +# 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) + if not group: + return + + 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/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. + + + + + + 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
    • diff --git a/spp_dci/tests/test_dci_admin_group.py b/spp_dci/tests/test_dci_admin_group.py index b02024371..eb77b7987 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,76 @@ 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: 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: + 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) + + # 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")