Crash if public api_method does not have roles (#15347)

This commit is in response to regressions that have crept into
the product since 24.10 affecting restricted admins roles. To
simplify development, middleware will refuse to start if a
public api_method is present with the following characteristics:

1. requires authorization
2. requires authentication
3. does not have roles defined

This commit fixes the API definitions of current methods that
do not have proper roles definitions.

src/middlewared/middlewared/api/base/decorator.py

@@ -9,6 +9,13 @@
 __all__ = ["api_method"]
 
 
+CONFIG_CRUD_METHODS = frozenset([
+    'do_create', 'do_update', 'do_delete',
+    'create', 'update', 'delete',
+    'query', 'get_instance', 'config'
+])
+
+
 def api_method(
     accepts: type[BaseModel],
     returns: type[BaseModel],
@@ -97,6 +104,13 @@ def wrapped(*args):
             wrapped._no_auth_required = True
         elif not authorization_required:
             wrapped._no_authz_required = True
+        elif not private:
+            # All public methods should have a roles definition. This is a rough check to help developers not write
+            # methods that are only accesssible to full_admin. We don't bother checking CONFIG and CRUD methods
+            # and choices because they may have implicit roles through the role_prefix configuration.
+
+            if func.__name__ not in CONFIG_CRUD_METHODS and not func.__name__.endswith('choices'):
+                raise ValueError(f'{func.__name__}: Role definition is required for public API endpoints')
 
         wrapped.audit = audit
         wrapped.audit_callback = audit_callback

src/middlewared/middlewared/plugins/account_/2fa.py

@@ -21,7 +21,7 @@ async def provisioning_uri_internal(self, username, user_twofactor_config):
             'iXsystems'
         )
 
-    @api_method(UserProvisioningUriArgs, UserProvisioningUriResult)
+    @api_method(UserProvisioningUriArgs, UserProvisioningUriResult, roles=['ACCOUNT_WRITE'])
     async def provisioning_uri(self, username):
         """
         Returns the provisioning URI for the OTP for `username`. This can then be encoded in a QR code and used
@@ -36,7 +36,7 @@ async def provisioning_uri(self, username):
 
         return await self.provisioning_uri_internal(username, user_twofactor_config)
 
-    @api_method(UserTwofactorConfigArgs, UserTwofactorConfigResult)
+    @api_method(UserTwofactorConfigArgs, UserTwofactorConfigResult, roles=['ACCOUNT_READ'])
     async def twofactor_config(self, username):
         """
         Returns two-factor authentication configuration settings for specified `username`.
@@ -57,7 +57,7 @@ async def twofactor_config(self, username):
             'otp_digits': user_twofactor_config['otp_digits'],
         }
 
-    @api_method(UserVerifyTwofactorTokenArgs, UserVerifyTwofactorTokenResult)
+    @api_method(UserVerifyTwofactorTokenArgs, UserVerifyTwofactorTokenResult, roles=['FULL_ADMIN'])
     def verify_twofactor_token(self, username, token):
         """
         Returns boolean true if provided `token` is successfully authenticated for `username`.
@@ -92,7 +92,9 @@ async def translate_username(self, username):
         return await self.middleware.call('user.query', [['username', '=', user['pw_name']]], {'get': True})
 
     @api_method(UserUnset2faSecretArgs, UserUnset2faSecretResult,
-                audit='Unset two-factor authentication secret:', audit_extended=lambda username: username)
+                audit='Unset two-factor authentication secret:',
+                audit_extended=lambda username: username,
+                roles=['ACCOUNT_WRITE'])
     async def unset_2fa_secret(self, username):
         """
         Unset two-factor authentication secret for `username`.
@@ -120,12 +122,12 @@ async def unset_2fa_secret(self, username):
             }
         )
 
-    @no_authz_required
     @api_method(
         UserRenew2faSecretArgs,
         UserRenew2faSecretResult,
         audit='Renew two-factor authentication secret:',
-        audit_extended=lambda username, options: username
+        audit_extended=lambda username, options: username,
+        authorization_required=False
     )
     @pass_app()
     async def renew_2fa_secret(self, app, username, twofactor_options):

src/middlewared/middlewared/plugins/account_/builtin_administrator.py

@@ -6,7 +6,7 @@
 
 class GroupService(Service):
 
-    @api_method(GroupHasPasswordEnabledUserArgs, GroupHasPasswordEnabledUserResult)
+    @api_method(GroupHasPasswordEnabledUserArgs, GroupHasPasswordEnabledUserResult, roles=['ACCOUNT_READ'])
     async def has_password_enabled_user(self, gids, exclude_user_ids):
         """
         Checks whether at least one local user with a password is a member of any of the `group_ids`.

src/middlewared/middlewared/plugins/alert.py

@@ -400,7 +400,7 @@ def __alert_by_uuid(self, uuid):
         except IndexError:
             return None
 
-    @api_method(AlertDismissArgs, AlertDismissResult)
+    @api_method(AlertDismissArgs, AlertDismissResult, roles=['ALERT_LIST_WRITE'])
     async def dismiss(self, uuid):
         """
         Dismiss `id` alert.
@@ -436,7 +436,7 @@ def _delete_on_dismiss(self, alert):
         if removed:
             self._send_alert_deleted_event(alert)
 
-    @api_method(AlertRestoreArgs, AlertRestoreResult)
+    @api_method(AlertRestoreArgs, AlertRestoreResult, roles=['ALERT_LIST_WRITE'])
     async def restore(self, uuid):
         """
         Restore `id` alert which had been dismissed.
@@ -999,6 +999,7 @@ class Config:
         datastore_order_by = ["name"]
         cli_namespace = "system.alert.service"
         entry = AlertServiceEntry
+        role_prefix = 'ALERT'
 
     @private
     async def _extend(self, service):
@@ -1098,7 +1099,7 @@ async def do_delete(self, id_):
         """
         return await self.middleware.call("datastore.delete", self._config.datastore, id_)
 
-    @api_method(AlertServiceTestArgs, AlertServiceTestResult)
+    @api_method(AlertServiceTestArgs, AlertServiceTestResult, roles=['ALERT_WRITE'])
     async def test(self, data):
         """
         Send a test alert using `type` of Alert Service.
@@ -1170,6 +1171,7 @@ class Config:
         datastore = "system.alertclasses"
         cli_namespace = "system.alert.class"
         entry = AlertClassesEntry
+        role_prefix = 'ALERT'
 
     @api_method(AlertClassesUpdateArgs, AlertClassesUpdateResult)
     async def do_update(self, data):

src/middlewared/middlewared/plugins/boot_/environments.py

@@ -92,7 +92,7 @@ def promote_current_datasets(self):
                     except Exception:
                         self.logger.exception("Unexpected error promoting %r", ds)
 
-    @filterable_api_method(item=BootEnvironmentEntry)
+    @filterable_api_method(item=BootEnvironmentEntry, roles=['BOOT_ENV_READ'])
     def query(self, filters, options):
         results = list()
         try:
@@ -129,7 +129,7 @@ def query(self, filters, options):
 
         return filter_list(results, filters, options)
 
-    @api_method(BootEnvironmentActivateArgs, BootEnvironmentActivateResult)
+    @api_method(BootEnvironmentActivateArgs, BootEnvironmentActivateResult, roles=["BOOT_ENV_WRITE"])
     def activate(self, data):
         info = self.validate_be("boot.environment.activate", data["id"])
         if info["activated"]:
@@ -144,14 +144,14 @@ def activate(self, data):
             run_zectl_cmd(["activate", data["id"]])
             return self.query([["id", "=", data["id"]]], {"get": True})
 
-    @api_method(BootEnvironmentCloneArgs, BootEnvironmentCloneResult)
+    @api_method(BootEnvironmentCloneArgs, BootEnvironmentCloneResult, roles=['BOOT_ENV_WRITE'])
     def clone(self, data):
         be = self.validate_be("boot.environment.clone", data["id"])
         self.validate_be("boot.environment.clone", data["target"], should_exist=False)
         run_zectl_cmd(["create", "-r", "-e", be["dataset"], data["target"]])
         return self.query([["id", "=", data["target"]]], {"get": True})
 
-    @api_method(BootEnvironmentDestroyArgs, BootEnvironmentDestroyResult)
+    @api_method(BootEnvironmentDestroyArgs, BootEnvironmentDestroyResult, roles=['BOOT_ENV_WRITE'])
     def destroy(self, data):
         if self.validate_be("boot.environment.destroy", data["id"])["active"]:
             raise ValidationError(
@@ -160,7 +160,7 @@ def destroy(self, data):
             )
         run_zectl_cmd(["destroy", data["id"]])
 
-    @api_method(BootEnvironmentKeepArgs, BootEnvironmentKeepResult)
+    @api_method(BootEnvironmentKeepArgs, BootEnvironmentKeepResult, roles=['BOOT_ENV_WRITE'])
     def keep(self, data):
         self.middleware.call_sync(
             "zfs.dataset.update",

src/middlewared/middlewared/plugins/cloud_backup/restore.py

@@ -11,7 +11,7 @@ class Config:
         cli_namespace = "task.cloud_backup"
         namespace = "cloud_backup"
 
-    @api_method(CloudBackupRestoreArgs, CloudBackupRestoreResult)
+    @api_method(CloudBackupRestoreArgs, CloudBackupRestoreResult, roles=["FILESYSTEM_DATA_WRITE"])
     @job(logs=True)
     async def restore(self, job, id_, snapshot_id, subfolder, destination_path, options):
         """

src/middlewared/middlewared/plugins/cloud_backup/snapshot.py

@@ -17,7 +17,7 @@ class Config:
         cli_namespace = "task.cloud_backup"
         namespace = "cloud_backup"
 
-    @api_method(CloudBackupListSnapshotsArgs, CloudBackupListSnapshotsResult)
+    @api_method(CloudBackupListSnapshotsArgs, CloudBackupListSnapshotsResult, roles=['CLOUD_BACKUP_READ'])
     def list_snapshots(self, id_):
         """
         List existing snapshots for the cloud backup job `id`.
@@ -44,7 +44,8 @@ def list_snapshots(self, id_):
 
         return snapshots
 
-    @api_method(CloudBackupListSnapshotDirectoryArgs, CloudBackupListSnapshotDirectoryResult)
+    @api_method(CloudBackupListSnapshotDirectoryArgs, CloudBackupListSnapshotDirectoryResult,
+                roles=['CLOUD_BACKUP_READ'])
     def list_snapshot_directory(self, id_, snapshot_id, path):
         """
         List files in the directory `path` of the `snapshot_id` created by the cloud backup job `id`.
@@ -80,7 +81,8 @@ def list_snapshot_directory(self, id_, snapshot_id, path):
 
         return contents
 
-    @api_method(CloudBackupDeleteSnapshotArgs, CloudBackupDeleteSnapshotResult)
+    @api_method(CloudBackupDeleteSnapshotArgs, CloudBackupDeleteSnapshotResult,
+                roles=['CLOUD_BACKUP_WRITE'])
     @job(lock=lambda args: "cloud_backup:{}".format(args[-1]), lock_queue_size=1)
     def delete_snapshot(self, job, id_, snapshot_id):
         """

src/middlewared/middlewared/plugins/cloud_backup/sync.py

@@ -121,7 +121,7 @@ class Config:
         namespace = "cloud_backup"
 
     @item_method
-    @api_method(CloudBackupSyncArgs, CloudBackupSyncResult)
+    @api_method(CloudBackupSyncArgs, CloudBackupSyncResult, roles=['CLOUD_BACKUP_WRITE'])
     @job(lock=lambda args: "cloud_backup:{}".format(args[-1]), lock_queue_size=1, logs=True, abortable=True)
     async def sync(self, job, id_, options):
         """
@@ -163,7 +163,7 @@ async def _sync(self, cloud_backup, options, job):
             raise
 
     @item_method
-    @api_method(CloudBackupAbortArgs, CloudBackupAbortResult)
+    @api_method(CloudBackupAbortArgs, CloudBackupAbortResult, roles=['CLOUD_BACKUP_WRITE'])
     async def abort(self, id_):
         """
         Aborts cloud backup task.

src/middlewared/middlewared/plugins/config.py

@@ -63,7 +63,7 @@ def save_tar_file(self, options, job):
             with open(ntf.name, 'rb') as f:
                 shutil.copyfileobj(f, job.pipes.output.w)
 
-    @api_method(ConfigSaveArgs, ConfigSaveResult)
+    @api_method(ConfigSaveArgs, ConfigSaveResult, roles=['FULL_ADMIN'])
     @job(pipes=["output"])
     async def save(self, job, options):
         """
@@ -83,7 +83,7 @@ async def save(self, job, options):
         method = self.save_db_only if not any(options.values()) else self.save_tar_file
         await self.middleware.run_in_thread(method, options, job)
 
-    @api_method(ConfigUploadArgs, ConfigUploadResult)
+    @api_method(ConfigUploadArgs, ConfigUploadResult, roles=['FULL_ADMIN'])
     @job(pipes=["input"])
     @pass_app(rest=True)
     def upload(self, app, job):
@@ -190,7 +190,7 @@ def upload_impl(self, job, file_or_tar, is_tar_file=False):
         self._handle_failover(job, 'uploaded', send_to_remote, UPLOADED_DB_PATH, True,
                               CONFIGURATION_UPLOAD_REBOOT_REASON)
 
-    @api_method(ConfigResetArgs, ConfigResetResult)
+    @api_method(ConfigResetArgs, ConfigResetResult, roles=['FULL_ADMIN'])
     @job(lock='config_reset', logs=True)
     @pass_app(rest=True)
     def reset(self, app, job, options):

src/middlewared/middlewared/plugins/cron.py

@@ -187,7 +187,7 @@ async def do_delete(self, id_):
 
         return response
 
-    @api_method(CronJobRunArgs, CronJobRunResult)
+    @api_method(CronJobRunArgs, CronJobRunResult, roles=['FULL_ADMIN'])
     @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/docker/update.py

@@ -136,7 +136,7 @@ async def status(self):
         """
         return await self.middleware.call('docker.state.get_status_dict')
 
-    @api_method(DockerNvidiaPresentArgs, DockerNvidiaPresentResult)
+    @api_method(DockerNvidiaPresentArgs, DockerNvidiaPresentResult, roles=['DOCKER_READ'])
     def nvidia_present(self):
         adv_config = self.middleware.call_sync("system.advanced.config")
 

src/middlewared/middlewared/plugins/enclosure_/enclosure_label.py

@@ -25,7 +25,7 @@ async def get_all(self):
             for label in await self.middleware.call("datastore.query", "enclosure.label")
         }
 
-    @api_method(EnclosureLabelSetArgs, EnclosureLabelUpdateResult)
+    @api_method(EnclosureLabelSetArgs, EnclosureLabelUpdateResult, roles=["ENCLOSURE_WRITE"])
     async def set(self, id_, label):
         await self.middleware.call(
             "datastore.delete", "enclosure.label", [["encid", "=", id_]]

src/middlewared/middlewared/plugins/filesystem.py

@@ -421,7 +421,7 @@ def file_receive(self, path, content, options):
 
         return True
 
-    @api_method(FilesystemGetFileArgs, FilesystemGetFileResult, audit='Filesystem get')
+    @api_method(FilesystemGetFileArgs, FilesystemGetFileResult, audit='Filesystem get', roles=['FULL_ADMIN'])
     @job(pipes=["output"])
     def get(self, job, path):
         """
@@ -434,7 +434,7 @@ def get(self, job, path):
         with open(path, 'rb') as f:
             shutil.copyfileobj(f, job.pipes.output.w)
 
-    @api_method(FilesystemPutFileArgs, FilesystemPutFileResult, audit='Filesystem put')
+    @api_method(FilesystemPutFileArgs, FilesystemPutFileResult, audit='Filesystem put', roles=['FULL_ADMIN'])
     @job(pipes=["input"])
     def put(self, job, path, options):
         """

src/middlewared/middlewared/plugins/init_shutdown_script.py

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

src/middlewared/middlewared/plugins/keychain.py

@@ -428,7 +428,7 @@ async def do_delete(self, audit_callback, id_, options):
             id_,
         )
 
-    @api_method(KeychainCredentialUsedByArgs, KeychainCredentialUsedByResult)
+    @api_method(KeychainCredentialUsedByArgs, KeychainCredentialUsedByResult, roles=['KEYCHAIN_CREDENTIAL_READ'])
     async def used_by(self, id_):
         """
         Returns list of objects that use this credential.

src/middlewared/middlewared/plugins/pool_/scrub.py

@@ -182,7 +182,7 @@ async def do_delete(self, id_):
         await self.middleware.call('service.restart', 'cron')
         return response
 
-    @api_method(PoolScrubScrubArgs, PoolScrubScrubResult)
+    @api_method(PoolScrubScrubArgs, PoolScrubScrubResult, roles=['POOL_WRITE'])
     @job(
         description=lambda name, action="START": (
             f"Scrub of pool {name!r}" if action == "START"
@@ -219,7 +219,7 @@ async def scrub(self, job, name, action):
 
                 await asyncio.sleep(1)
 
-    @api_method(PoolScrubRunArgs, PoolScrubRunResult)
+    @api_method(PoolScrubRunArgs, PoolScrubRunResult, roles=['POOL_WRITE'])
     async def run(self, name, threshold):
         """
         Initiate a scrub of a pool `name` if last scrub was performed more than `threshold` days before.

src/middlewared/middlewared/plugins/snapshot.py

@@ -46,6 +46,7 @@ class Config:
         namespace = 'pool.snapshottask'
         cli_namespace = 'task.snapshot'
         entry = PoolSnapshotTaskEntry
+        role_prefix='SNAPSHOT_TASK'
 
     @private
     async def extend_context(self, rows, extra):
@@ -296,7 +297,7 @@ async def do_delete(self, audit_callback, id_, options):
 
         return response
 
-    @api_method(PoolSnapshotTaskMaxCountArgs, PoolSnapshotTaskMaxCountResult)
+    @api_method(PoolSnapshotTaskMaxCountArgs, PoolSnapshotTaskMaxCountResult, roles=['SNAPSHOT_TASK_READ'])
     def max_count(self):
         """
         Returns a maximum amount of snapshots (per-dataset) the system can sustain.
@@ -306,7 +307,7 @@ def max_count(self):
         # with too many, then File Explorer will show no snapshots available.
         return 512
 
-    @api_method(PoolSnapshotTaskMaxTotalCountArgs, PoolSnapshotTaskMaxTotalCountResult)
+    @api_method(PoolSnapshotTaskMaxTotalCountArgs, PoolSnapshotTaskMaxTotalCountResult, roles=['SNAPSHOT_TASK_READ'])
     def max_total_count(self):
         """
         Returns a maximum amount of snapshots (total) the system can sustain.
@@ -317,7 +318,7 @@ def max_total_count(self):
         return 10000
 
     @item_method
-    @api_method(PoolSnapshotTaskRunArgs, PoolSnapshotTaskRunResult)
+    @api_method(PoolSnapshotTaskRunArgs, PoolSnapshotTaskRunResult, roles=['SNAPSHOT_TASK_WRITE'])
     async def run(self, id_):
         """
         Execute a Periodic Snapshot Task of `id`.