Add unit test for role_manager FULL_ADMIN allowlist (#15427)

This commit adds a private API endpoint to dump current role
manager contents and adds a unit test to verify that the methods
explicitly granted to the FULL_ADMIN role have not changed. This
provides coverage for a regression that granted virt write methods
in STIG mode to users with FULL_ADMIN privilege.

src/middlewared/middlewared/plugins/account_/privilege_roles.py

@@ -1,7 +1,9 @@
+from copy import deepcopy
+
 from middlewared.api.current import PrivilegeRoleEntry
 
 from middlewared.role import ROLES
-from middlewared.service import Service, filterable_api_method, filter_list
+from middlewared.service import Service, filterable_api_method, filter_list, private
 
 
 class PrivilegeService(Service):
@@ -38,3 +40,27 @@ async def roles(self, filters, options):
         ]
 
         return filter_list(roles, filters, options)
+
+    @private
+    async def dump_role_manager(self):
+        """
+        Private method for CI in order to dump current information in role manager
+        This is consumed in tests/unit/test_role_manager.py
+
+        And possibly more tests
+        """
+
+        # deepcopy is okay here because this should basically never be called and
+        # we'd rather err on side of paranoia
+        method_resources = deepcopy(self.middleware.role_manager.methods.resources)
+        method_allowlists = deepcopy(self.middleware.role_manager.methods.allowlists_for_roles)
+
+        event_resources = deepcopy(self.middleware.role_manager.events.resources)
+        event_allowlists = deepcopy(self.middleware.role_manager.events.allowlists_for_roles)
+
+        return {
+            'method_resources': method_resources,
+            'method_allowlists': method_allowlists,
+            'event_resources': event_resources,
+            'event_allowlists': event_allowlists
+        }

src/middlewared/middlewared/plugins/cron.py

@@ -188,7 +188,7 @@ async def do_delete(self, id_):
 
         return response
 
-    @api_method(CronJobRunArgs, CronJobRunResult, roles=['FULL_ADMIN'])
+    @api_method(CronJobRunArgs, CronJobRunResult, roles=['SYSTEM_CRON_WRITE'])
     @job(lock=lambda args: f'cron_job_run_{args[0]}', logs=True, lock_queue_size=1)
     def run(self, job, id_, skip_disabled):
         """

src/middlewared/middlewared/plugins/init_shutdown_script.py

@@ -39,7 +39,7 @@ class Config:
         entry = InitShutdownScriptEntry
         role_prefix = 'SYSTEM_CRON'
 
-    @api_method(InitShutdownScriptCreateArgs, InitShutdownScriptCreateResult, roles=['FULL_ADMIN'])
+    @api_method(InitShutdownScriptCreateArgs, InitShutdownScriptCreateResult)
     async def do_create(self, data):
         """
         Create an initshutdown script task.
@@ -68,7 +68,7 @@ async def do_create(self, data):
         )
         return await self.get_instance(data['id'])
 
-    @api_method(InitShutdownScriptUpdateArgs, InitShutdownScriptUpdateResult, roles=['FULL_ADMIN'])
+    @api_method(InitShutdownScriptUpdateArgs, InitShutdownScriptUpdateResult)
     async def do_update(self, id_, data):
         """
         Update initshutdown script task of `id`.

tests/unit/test_role_manager.py

@@ -2,6 +2,7 @@
 
 from middlewared.role import RoleManager, ROLES
 from middlewared.utils import security
+from truenas_api_client import Client
 
 
 FAKE_METHODS = [
@@ -18,6 +19,19 @@
 WRITE_METHODS = frozenset(['fakemethod3', 'fakemethod6'])
 READONLY_ADMIN_METHODS = ALL_METHODS - WRITE_METHODS
 FULL_ADMIN_STIG = ALL_METHODS - FAKE_METHODS_NOSTIG
+EXPECTED_FA_RESOURCES = frozenset({
+    'user.verify_twofactor_token',
+    'failover.reboot.other_node',
+    'truenas.accept_eula',
+    'filesystem.put',
+    'truenas.set_production',
+    'system.shutdown',
+    'config.reset',
+    'system.reboot',
+    'config.upload',
+    'filesystem.get',
+    'config.save',
+})
 
 
 @pytest.fixture(scope='module')
@@ -93,3 +107,15 @@ def test__role_manager_reject_already_registered(tmp_role_manager):
 
     with pytest.raises(ValueError, match='is already registered in this role manager'):
         tmp_role_manager.register_method(method_name='canary.update', roles=['VM_WRITE'])
+
+
+def test__check_readonly_role():
+    """
+    We _really_ shouldn't be directly assigning resources to FULL_ADMIN. The reason for this
+    is that it provides no granularity for restricting what FA can do when STIG is enabled.
+    Mostly we don't want methods like "virt.global.update" being populated here.
+    """
+    with Client() as c:
+        method_allowlists = c.call('privilege.dump_role_manager')['method_allowlists']
+        fa_resources = set([entry['resource'] for entry in method_allowlists['FULL_ADMIN']])
+        assert fa_resources == EXPECTED_FA_RESOURCES