security(dci): stop DCI Administrator group from granting system admin

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
 ~~~~~~~~~~
 

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",

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,
+        )

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

spp_dci/security/dci_groups.xml

@@ -1,15 +1,29 @@
 <?xml version="1.0" encoding="utf-8" ?>
-<odoo noupdate="1">
-    <!-- Security group used by every spp_dci_* module to gate access to
-         raw DCI payloads, cached PII, and other sensitive callback data.
-         Place it on individual view fields and notebook pages with
-         groups="spp_dci.group_dci_admin" so operators can still navigate
-         the DCI menus without seeing the underlying personal data. -->
-    <record id="group_dci_admin" model="res.groups">
-        <field name="name">DCI Administrator</field>
-        <field name="implied_ids" eval="[Command.link(ref('base.group_system'))]" />
-        <field
-            name="comment"
-        >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.</field>
+<odoo>
+    <data noupdate="1">
+        <!-- Security group used by every spp_dci_* module to gate access to
+             raw DCI payloads, cached PII, and other sensitive callback data.
+             Place it on individual view fields and notebook pages with
+             groups="spp_dci.group_dci_admin" so operators can still navigate
+             the DCI menus without seeing the underlying personal data.
+
+             This group must never list base.group_system in implied_ids:
+             implied_ids GRANTS the implied groups to members, so that link
+             would silently promote anyone holding this PII-visibility role
+             to a full Settings/System administrator. -->
+        <record id="group_dci_admin" model="res.groups">
+            <field name="name">DCI Administrator</field>
+            <field
+                name="comment"
+            >Grants visibility to raw DCI payloads, full identifiers, disability data, and other sensitive fields exposed by the DCI cache and log models.</field>
+        </record>
+    </data>
+
+    <!-- OpenSPP admins (and system admins, who imply group_spp_admin) get
+         DCI payload visibility automatically, following the extension
+         pattern documented in spp_security/security/groups_admin.xml.
+         Kept outside the noupdate block so upgrades apply it. -->
+    <record id="spp_security.group_spp_admin" model="res.groups">
+        <field name="implied_ids" eval="[Command.link(ref('group_dci_admin'))]" />
     </record>
 </odoo>

spp_dci/static/description/index.html

@@ -504,6 +504,18 @@ <h2><a class="toc-backref" href="#toc-entry-1">Changelog</a></h2>
 </div>
 </div>
 <div class="section" id="section-1">
+<h1>19.0.2.0.2</h1>
+<ul class="simple">
+<li>fix(security): stop the DCI Administrator group from granting full
+system administration. <tt class="docutils literal">implied_ids</tt> grants the implied groups to
+members, so the previous <tt class="docutils literal">base.group_system</tt> 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.</li>
+</ul>
+</div>
+<div class="section" id="section-2">
 <h1>19.0.2.0.0</h1>
 <ul class="simple">
 <li>Initial migration to OpenSPP2</li>

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,15 +14,87 @@ 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):
         group = self.env.ref("spp_dci.group_dci_admin", raise_if_not_found=False)
         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")