diff --git a/src/middlewared/middlewared/api/base/decorator.py b/src/middlewared/middlewared/api/base/decorator.py index ae35260eeb4a5..fd3a3bfd878f3 100644 --- a/src/middlewared/middlewared/api/base/decorator.py +++ b/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 diff --git a/src/middlewared/middlewared/api/v25_10_0/smb.py b/src/middlewared/middlewared/api/v25_10_0/smb.py index 536e23e47cde3..635058e3f858a 100644 --- a/src/middlewared/middlewared/api/v25_10_0/smb.py +++ b/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 diff --git a/src/middlewared/middlewared/plugins/filesystem_/acl.py b/src/middlewared/middlewared/plugins/filesystem_/acl.py index 2d45e9e37a1ac..86b47b32e3fd0 100644 --- a/src/middlewared/middlewared/plugins/filesystem_/acl.py +++ b/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 diff --git a/src/middlewared/middlewared/plugins/ntp.py b/src/middlewared/middlewared/plugins/ntp.py index dd49f2e2f0775..e5ffbffdadd48 100644 --- a/src/middlewared/middlewared/plugins/ntp.py +++ b/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 = [] diff --git a/src/middlewared/middlewared/plugins/rdma/rdma.py b/src/middlewared/middlewared/plugins/rdma/rdma.py index c570d866e606a..9819f6809932b 100644 --- a/src/middlewared/middlewared/plugins/rdma/rdma.py +++ b/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'. diff --git a/src/middlewared/middlewared/plugins/smb.py b/src/middlewared/middlewared/plugins/smb.py index 7321df3f4004a..38cdf6c5cbb61 100644 --- a/src/middlewared/middlewared/plugins/smb.py +++ b/src/middlewared/middlewared/plugins/smb.py @@ -4,16 +4,10 @@ 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, @@ -21,6 +15,7 @@ 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'] diff --git a/src/middlewared/middlewared/pytest/unit/api/handler/decorator.py b/src/middlewared/middlewared/pytest/unit/api/handler/decorator.py new file mode 100644 index 0000000000000..7b3648cbca17e --- /dev/null +++ b/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