NAS-133901 / 25.10 / Do not allow roles to be set on private endpoints

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

@@ -97,23 +97,28 @@ def wrapped(*args):
 
                 return result
 
-        if roles:
+        if private:
+            if roles or not authentication_required or not authorization_required:
+                raise ValueError('Cannot set roles, no authorization, or no authentication on private methods.')
+
+        elif roles:
             if not authorization_required or not authentication_required:
                 raise ValueError('Authentication and authorization must be enabled in order to use roles.')
-        elif not authentication_required and not authorization_required:
-            # Although this is technically valid the concern is that dev has fat-fingered something
-            raise ValueError('Either authentication or authorization may be disabled, but not both simultaneously.')
-        elif not authentication_required:
-            wrapped._no_auth_required = True
+
         elif not authorization_required:
+            if not authentication_required:
+                # Although this is technically valid the concern is that dev has fat-fingered something
+                raise ValueError('Either authentication or authorization may be disabled, but not both simultaneously.')
             wrapped._no_authz_required = True
-        elif not private:
+
+        elif not authentication_required:
+            wrapped._no_auth_required = True
+
+        elif func.__name__ not in CONFIG_CRUD_METHODS and not func.__name__.endswith('choices'):
             # 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')
+            raise ValueError(f'{func.__name__}: Role definition is required for public API endpoints')
 
         wrapped.audit = audit
         wrapped.audit_callback = audit_callback

src/middlewared/middlewared/api/v25_10_0/smb.py

@@ -21,6 +21,7 @@
     'SmbServiceUnixCharsetChoicesArgs', 'SmbServiceUnixCharsetChoicesResult',
     'SmbServiceBindIPChoicesArgs', 'SmbServiceBindIPChoicesResult',
     'SmbSharePresetsArgs', 'SmbSharePresetsResult',
+    'SmbSharePrecheckArgs', 'SmbSharePrecheckResult',
 ]
 
 EMPTY_STRING = ''
@@ -189,3 +190,12 @@ class SmbSharePresetsArgs(BaseModel):
 
 class SmbSharePresetsResult(BaseModel):
     result: dict[str, dict]
+
+
+@single_argument_args('smb_share_precheck')
+class SmbSharePrecheckArgs(BaseModel):
+    name: NonEmptyString | None = None
+
+
+class SmbSharePrecheckResult(BaseModel):
+    result: None

src/middlewared/middlewared/plugins/filesystem_/acl.py

@@ -965,7 +965,6 @@ def check_acl_for_entry(entry):
     @api_method(
         FilesystemAddToAclArgs,
         FilesystemAddToAclResult,
-        roles=['FILESYSTEM_ATTRS_WRITE'],
         audit='Filesystem add to ACL',
         audit_extended=lambda data: data['path'],
         private=True

src/middlewared/middlewared/plugins/ntp.py

@@ -173,7 +173,7 @@ def test_ntp_server(addr):
         except Exception:
             return False
 
-    @filterable_api_method(item=NTPPeerEntry, roles=['READONLY_ADMIN'], private=True)
+    @filterable_api_method(item=NTPPeerEntry, private=True)
     def peers(self, filters, options):
         peers = []
 

src/middlewared/middlewared/plugins/rdma/rdma.py

@@ -54,7 +54,7 @@ def get_pci_vpd(self, pci_addr):
                 result['part'] = sline[len(PART_NUMBER_PREFIX):]
         return result
 
-    @api_method(RdmaLinkConfigArgs, RdmaLinkConfigResult, private=True, roles=['NETWORK_INTERFACE_READ'])
+    @api_method(RdmaLinkConfigArgs, RdmaLinkConfigResult, private=True)
     async def get_link_choices(self, all):
         """Return a list containing dictionaries with keys 'rdma' and 'netdev'.
 

src/middlewared/middlewared/plugins/smb.py

@@ -4,23 +4,18 @@
 import middlewared.sqlalchemy as sa
 import os
 from pathlib import Path
-from typing import Literal
 import uuid
 import unicodedata
 
 from middlewared.api import api_method
-from middlewared.api.base import (
-    BaseModel,
-    NonEmptyString,
-    single_argument_args,
-)
 from middlewared.api.current import (
     GetSmbAclArgs, GetSmbAclResult,
     SetSmbAclArgs, SetSmbAclResult,
     SmbServiceEntry, SmbServiceUpdateArgs, SmbServiceUpdateResult,
     SmbServiceUnixCharsetChoicesArgs, SmbServiceUnixCharsetChoicesResult,
     SmbServiceBindIPChoicesArgs, SmbServiceBindIPChoicesResult,
     SmbSharePresetsArgs, SmbSharePresetsResult,
+    SmbSharePrecheckArgs, SmbSharePrecheckResult,
 )
 from middlewared.common.attachment import LockableFSAttachmentDelegate
 from middlewared.common.listen import SystemServiceListenMultipleDelegate
@@ -91,15 +86,6 @@ class SMBModel(sa.Model):
     cifs_srv_encryption = sa.Column(sa.String(120), nullable=True)
 
 
-@single_argument_args('smb_share_precheck')
-class SmbSharePrecheckArgs(BaseModel):
-    name: NonEmptyString | None = None
-
-
-class SmbSharePrecheckResult(BaseModel):
-    result: Literal[None]
-
-
 class SMBService(ConfigService):
 
     class Config:
@@ -1338,7 +1324,7 @@ async def validate(self, data, schema_name, verrors, old=None):
                     'This feature may be enabled in the general SMB server configuration.'
                 )
 
-    @api_method(SmbSharePrecheckArgs, SmbSharePrecheckResult, roles=['READONLY_ADMIN'], private=True)
+    @api_method(SmbSharePrecheckArgs, SmbSharePrecheckResult, roles=['READONLY_ADMIN'])
     async def share_precheck(self, data):
         verrors = ValidationErrors()
         ad_enabled = (await self.middleware.call('activedirectory.config'))['enable']

src/middlewared/middlewared/pytest/unit/api/handler/decorator.py

@@ -0,0 +1,46 @@
+import pytest
+
+from middlewared.api import api_method
+from middlewared.api.base import BaseModel
+
+
+class MethodArgs(BaseModel):...
+class MethodResult(BaseModel):
+    result: None
+
+
+@pytest.mark.parametrize("kwargs, error", [
+    (
+        {"private": True, "roles": ["READONLY_ADMIN"]},
+        "Cannot set roles, no authorization, or no authentication on private methods."
+    ),
+    (
+        {"roles": ["READONLY_ADMIN"], "authorization_required": False},
+        "Authentication and authorization must be enabled in order to use roles."
+    ),
+    (
+        {"authorization_required": False, "authentication_required": False},
+        "Either authentication or authorization may be disabled, but not both simultaneously."
+    ),
+    (
+        {},
+        "method: Role definition is required for public API endpoints"
+    )
+])
+def test_bad_api_method_args(kwargs, error):
+    with pytest.raises(ValueError, match=error):
+        @api_method(MethodArgs, MethodResult, **kwargs)
+        def method():...
+
+
+@pytest.mark.parametrize("kwargs, attr_name, attr_value", [
+    ({"private": True}, "_private", True),
+    ({"roles": ["READONLY_ADMIN"]}, "roles", ["READONLY_ADMIN"]),
+    ({"authorization_required": False}, "_no_authz_required", True),
+    ({"authentication_required": False}, "_no_auth_required", True),
+])
+def test_api_method_args(kwargs, attr_name, attr_value):
+    @api_method(MethodArgs, MethodResult, **kwargs)
+    def method():...
+
+    assert getattr(method, attr_name) == attr_value