diff --git a/src/modelscope_hub/_openapi.py b/src/modelscope_hub/_openapi.py index 24ee5c8..55d6aec 100644 --- a/src/modelscope_hub/_openapi.py +++ b/src/modelscope_hub/_openapi.py @@ -1307,6 +1307,8 @@ def list_mcp_servers( filter : dict, optional Nested filter object. Supported keys: ``category``, ``is_hosted``. """ + if isinstance(page_number, bool) or not isinstance(page_number, int) or page_number < 1: + raise InvalidParameter("page_number must be an integer >= 1.") if page_number * page_size > 100: # The service enforces this itself, answering 403 QuotaLimitExceed with # exactly this rule. Checking here spares the round trip and reports it diff --git a/src/modelscope_hub/agent/_api.py b/src/modelscope_hub/agent/_api.py index 16effcc..531bf41 100644 --- a/src/modelscope_hub/agent/_api.py +++ b/src/modelscope_hub/agent/_api.py @@ -22,8 +22,8 @@ from .._openapi import OpenAPIClient from ..config import HubConfig -from ..constants import Visibility -from ..errors import AuthenticationError, NotExistError +from ..constants import TokenScope, Visibility +from ..errors import APIError, AuthenticationError, NotExistError, PermissionDeniedError logger = logging.getLogger("modelscope_hub.agent") @@ -230,12 +230,88 @@ def check_repo(self, path: str, name: str) -> bool: """True if the repo exists, False on 404.""" return self.repo_info(path, name) is not None + @staticmethod + def _decode_dolphin_list_response(response: object, *, list_url: str) -> object: + """Decode the legacy dolphin list envelope without hiding soft errors. + + Most endpoints signal access denial with HTTP 403, which OpenAPIClient + maps normally. ``/api/v1/dolphin/agents`` can instead answer HTTP 200 + with ``Code=OperationNotAllowed``. Its former decode path then treated + the payload as an empty result, making a scope error indistinguishable + from an owner with no repositories. + """ + try: + payload = response.json() # type: ignore[union-attr] + except (AttributeError, ValueError) as exc: + raise APIError( + "Agent repository list endpoint returned a non-JSON response.", + status_code=500, + url=list_url, + method="PUT", + ) from exc + + if not isinstance(payload, dict): + return payload + + code = payload.get("Code") if payload.get("Code") is not None else payload.get("code") + success = payload.get("Success") if "Success" in payload else payload.get("success") + message = next( + ( + value.strip() + for key in ("Message", "message", "Msg", "msg") + if isinstance(value := payload.get(key), str) and value.strip() + ), + "Agent repository list endpoint rejected the request.", + ) + code_text = str(code).strip() + message_lower = message.lower() + permission_denied = ( + code_text == "OperationNotAllowed" + or code_text == "403" + or "operationnotallowed" in message_lower + or "permission denied" in message_lower + or "not allowed" in message_lower + or "forbidden" in message_lower + ) + failed_envelope = success is False or (code is not None and code_text not in ("", "0", "200")) + if permission_denied: + error = PermissionDeniedError( + f"OperationNotAllowed: {message}", + status_code=403, + request_id=payload.get("RequestId") or payload.get("request_id"), + response_body=payload, + url=list_url, + method="PUT", + ) + error.suggestion = ( + "Agent repository listing requires a token with 'read' permission; " + "an api-inference-only token cannot access this endpoint." + ) + raise error + if failed_envelope: + raise APIError( + f"Agent repository list endpoint rejected the request: {message}", + status_code=400, + request_id=payload.get("RequestId") or payload.get("request_id"), + response_body=payload, + url=list_url, + method="PUT", + ) + + # Match OpenAPIClient's success-envelope unwrapping while preserving the + # outer envelope long enough to inspect soft errors above. + if "data" in payload: + return payload["data"] + if "Data" in payload: + return payload["Data"] + return payload + def list_agents(self, owner: str | None = None, page_number: int = 1, page_size: int = 10) -> dict: """List agent repositories (PUT /api/v1/dolphin/agents). - Queries the dolphin search endpoint. When *owner* is given it is sent as - a ``Path contains`` criterion (the group filter). Returns a dict with - 'items' (list of agent metadata dicts) and 'total_count' (int). + This is an account-scoped operation, not a public catalogue lookup. It + requires a token with ``read`` permission and must never downgrade a + denied request into an anonymous empty list. """ criterion: list[dict] = [] if owner: @@ -254,7 +330,15 @@ def list_agents(self, owner: str | None = None, page_number: int = 1, page_size: "Criterion": criterion, } list_url = f"{self.server}/api/v1/dolphin/agents" - data = self._openapi.request("PUT", url=list_url, json_body=body, require_token=False) + response = self._openapi._request( + "PUT", + url=list_url, + json_body=body, + require_token=True, + required_scope=TokenScope.READ, + unwrap=False, + ) + data = self._decode_dolphin_list_response(response, list_url=list_url) if isinstance(data, list): return {"items": data, "total_count": len(data)} if isinstance(data, dict): diff --git a/src/modelscope_hub/agent_idp.py b/src/modelscope_hub/agent_idp.py index e2ad5da..c9e479e 100644 --- a/src/modelscope_hub/agent_idp.py +++ b/src/modelscope_hub/agent_idp.py @@ -54,6 +54,30 @@ def _decode_base64url(value: object, field: str) -> bytes: return decoded +def _canonical_signing_component(value: str, field: str) -> str: + """Validate one component of the server-defined ASCII signing message. + + The Agent-IDP protocol signs the literal + ``agent_id|kid|audience|timestamp`` byte sequence. ``audience`` is the + target Hub application's ``client_id``, not a display name. Encoding a + Unicode display name as UTF-8 here would silently change the bytes the + service verifies, so reject it explicitly instead of leaking a Python + ``UnicodeEncodeError``. + """ + try: + value.encode("ascii") + except UnicodeEncodeError: + reason = ( + "the target Hub application's client_id" + if field == "audience" + else "the canonical Agent-IDP signature message" + ) + raise InvalidParameter(f"{field} must contain only ASCII characters because it is {reason}.") from None + if "|" in value: + raise InvalidParameter(f"{field} must not contain '|', the Agent-IDP signature message delimiter.") + return value + + def _normalise_private_jwk(value: AgentJWK | Mapping[str, Any]) -> AgentJWK: """Validate an Ed25519 private JWK and return a safe structured copy.""" if isinstance(value, AgentJWK): @@ -186,20 +210,28 @@ def sign_agent_token_request( audience: str, timestamp: int, ) -> TokenSignPayload: - """Build the signed body required by anonymous ``POST /agent_id/token``.""" + """Build the signed body required by anonymous ``POST /agent_id/token``. + + ``audience`` must be the ASCII Hub application ``client_id``. It is not a + human-readable application name: every component is signed as the literal + ASCII ``agent_id|kid|audience|timestamp`` protocol byte sequence. + """ if not isinstance(agent_id, str) or not agent_id: raise InvalidParameter("agent_id must be a non-empty string.") if not isinstance(audience, str) or not audience: raise InvalidParameter("audience must be a non-empty string.") if isinstance(timestamp, bool) or not isinstance(timestamp, int) or timestamp <= 0: raise InvalidParameter("timestamp must be a positive Unix timestamp in seconds.") + agent_id = _canonical_signing_component(agent_id, "agent_id") + audience = _canonical_signing_component(audience, "audience") key = _normalise_private_jwk(private_jwk) - message = f"{agent_id}|{key.kid}|{audience}|{timestamp}".encode("ascii") + kid = _canonical_signing_component(key.kid, "kid") + message = f"{agent_id}|{kid}|{audience}|{timestamp}".encode("ascii") private_bytes = _decode_base64url(key.d, "d") signature = Ed25519PrivateKey.from_private_bytes(private_bytes).sign(message) return { "agent_id": agent_id, - "kid": key.kid, + "kid": kid, "audience": audience, "timestamp": timestamp, "signature": _encode_base64url(signature), diff --git a/src/modelscope_hub/api.py b/src/modelscope_hub/api.py index 00ae696..0244ea5 100644 --- a/src/modelscope_hub/api.py +++ b/src/modelscope_hub/api.py @@ -1707,11 +1707,9 @@ def delete_files( """ rt = self._normalize_repo_type(repo_type) paths = self._normalize_delete_values(file_paths, "file_paths") - patterns = self._normalize_delete_values( - delete_patterns, "delete_patterns") + patterns = self._normalize_delete_values(delete_patterns, "delete_patterns") if not paths and not patterns: - raise InvalidParameter( - "Provide at least one file path or delete pattern.") + raise InvalidParameter("Provide at least one file path or delete pattern.") resolved_revision = revision or "master" if patterns: @@ -1726,9 +1724,8 @@ def delete_files( if file.path and file.type != "tree" ] paths.extend( - path for path in remote_paths - if any(fnmatch.fnmatchcase(path, pattern) - for pattern in patterns)) + path for path in remote_paths if any(fnmatch.fnmatchcase(path, pattern) for pattern in patterns) + ) paths = list(dict.fromkeys(paths)) if not paths: @@ -1758,8 +1755,7 @@ def _normalize_delete_values( normalized: list[str] = [] for value in raw_values: if not isinstance(value, str): - raise InvalidParameter( - f"{parameter_name} must contain only strings.") + raise InvalidParameter(f"{parameter_name} must contain only strings.") if value: normalized.append(value) return normalized @@ -2240,8 +2236,16 @@ def list_mcp_servers( filter=filter, extra={k: v for k, v in extra.items() if v is not None} or None, ) - items, total, page, size = self._extract_paged(payload) - return PagedResult(items=list(items), total_count=total, page_number=page, page_size=size) + items, total, _page, _size = self._extract_paged(payload) + # The MCP service currently omits page_number/page_size in its response. + # The request values are authoritative, exactly as in list_repos("mcp"), + # so callers can reliably tell which page they received. + return PagedResult( + items=list(items), + total_count=total, + page_number=page_number, + page_size=page_size, + ) def list_operational_mcp_servers(self) -> PagedResult[dict]: """List the MCP servers the caller currently has hosted. diff --git a/src/modelscope_hub/cli/agent.py b/src/modelscope_hub/cli/agent.py index e70748c..1f0621a 100644 --- a/src/modelscope_hub/cli/agent.py +++ b/src/modelscope_hub/cli/agent.py @@ -35,11 +35,27 @@ def _fail(message: str) -> int: return 1 +def _is_operation_not_allowed(e: APIError) -> bool: + """Recognise the server's permission code in either message or envelope.""" + if "OperationNotAllowed" in e.message: + return True + body = e.response_body + if not isinstance(body, dict): + return False + code = body.get("Code") if body.get("Code") is not None else body.get("code") + return str(code) == "OperationNotAllowed" + + def _api_error_message(e: APIError, action: str = "request") -> str: status = e.status_code or 0 if status == 401: return "authentication failed. Please login again." if status == 403: + if action == "list" and _is_operation_not_allowed(e): + return ( + "permission denied (403 OperationNotAllowed). Agent repository listing requires a token with " + "'read' permission; an api-inference-only token cannot access this endpoint." + ) return "permission denied. You do not have access to this resource." if status == 404: return "resource not found. Check the repository name and try again." diff --git a/src/modelscope_hub/cli/mcp.py b/src/modelscope_hub/cli/mcp.py index 70c501d..d719c88 100644 --- a/src/modelscope_hub/cli/mcp.py +++ b/src/modelscope_hub/cli/mcp.py @@ -64,12 +64,11 @@ def execute(self) -> None: ( item.get("id") or item.get("Id") or "-", item.get("name") or item.get("Name") or "-", - item.get("status") or item.get("Status") or "-", item.get("description") or item.get("Description") or "-", ) for item in result.items ] - info(render_table(rows, headers=["id", "name", "status", "description"])) + info(render_table(rows, headers=["id", "name", "description"])) info(f"\npage {result.page_number} / total {result.total_count}") @staticmethod diff --git a/tests/agent/test_agent_cli.py b/tests/agent/test_agent_cli.py index 03b0c4a..d9209ec 100644 --- a/tests/agent/test_agent_cli.py +++ b/tests/agent/test_agent_cli.py @@ -11,6 +11,8 @@ import argparse import base64 +import contextlib +import io import tempfile import unittest from pathlib import Path @@ -18,6 +20,8 @@ from modelscope_hub.agent import AgentApi, RemoteFileInfo, is_lfs_file from modelscope_hub.cli import agent as cli_agent +from modelscope_hub.constants import TokenScope +from modelscope_hub.errors import PermissionDeniedError class _StubClient: @@ -135,6 +139,77 @@ def test_list_requires_endpoint(self): rc = cli_agent._cmd_list(None, 1, 10, endpoint=None, token="t") self.assertEqual(rc, 1) + def test_list_raises_explicit_permission_error_for_soft_operation_not_allowed(self): + client = AgentApi(endpoint="https://modelscope.cn", token="api-inference-only") + response = mock.Mock() + response.json.return_value = { + "Success": False, + "Code": "OperationNotAllowed", + "Message": "token scope denied", + "RequestId": "request-1", + "Data": [], + } + with mock.patch.object(client._openapi, "_request", return_value=response) as request: + with self.assertRaises(PermissionDeniedError) as raised: + client.list_agents(owner="wangxingjun778", page_number=2, page_size=5) + error = raised.exception + self.assertEqual(error.error_code, "E3002") + self.assertEqual(error.status_code, 403) + self.assertIn("OperationNotAllowed", error.message) + self.assertEqual(error.request_id, "request-1") + request.assert_called_once_with( + "PUT", + url="https://modelscope.cn/api/v1/dolphin/agents", + json_body={ + "PageSize": 5, + "PageNumber": 2, + "Query": "", + "Sort": "Default", + "Criterion": [{"Category": "Path", "Predicate": "contains", "StringValues": ["wangxingjun778"]}], + }, + require_token=True, + required_scope=TokenScope.READ, + unwrap=False, + ) + + def test_list_converts_numeric_soft_403_to_permission_denied(self): + client = AgentApi(endpoint="https://modelscope.cn", token="api-inference-only") + response = mock.Mock() + response.json.return_value = { + "Success": False, + "Code": 403, + "Message": "permission denied", + "Data": [], + } + with mock.patch.object(client._openapi, "_request", return_value=response): + with self.assertRaises(PermissionDeniedError) as raised: + client.list_agents(owner="wangxingjun778") + self.assertEqual(raised.exception.error_code, "E3002") + self.assertEqual(raised.exception.status_code, 403) + + def test_list_cli_reports_operation_not_allowed_not_empty_result(self): + class DeniedClient: + def __init__(self, *args, **kwargs): + pass + + def list_agents(self, **kwargs): + # Real HTTP 403 may carry OperationNotAllowed only in body.code, + # not in the server's human-readable message. + raise PermissionDeniedError( + "token scope denied", + status_code=403, + response_body={"code": "OperationNotAllowed"}, + ) + + stderr = io.StringIO() + with mock.patch.object(cli_agent, "AgentApi", DeniedClient), contextlib.redirect_stderr(stderr): + rc = cli_agent._cmd_list("wangxingjun778", 1, 10, endpoint="https://modelscope.cn", token="scoped") + self.assertEqual(rc, 1) + message = stderr.getvalue() + self.assertIn("403 OperationNotAllowed", message) + self.assertIn("'read' permission", message) + self.assertNotIn("no agent repositories found", message) + class TestCmdDownload(unittest.TestCase): def setUp(self): diff --git a/tests/cli/conftest.py b/tests/cli/conftest.py index 49f03d5..3856ac5 100644 --- a/tests/cli/conftest.py +++ b/tests/cli/conftest.py @@ -91,7 +91,7 @@ def mock_api(): {"key": "API_KEY", "description": "test", "updated_at": "2000-01-01T00:00:00Z"}, ] api.list_mcp_servers.return_value = PagedResult( - items=[{"id": "mcp-1", "name": "weather", "status": "running", "description": "Weather MCP"}], + items=[{"id": "mcp-1", "name": "weather", "description": "Weather MCP"}], total_count=1, page_number=1, page_size=20, diff --git a/tests/cli/test_mcp.py b/tests/cli/test_mcp.py index 80db398..ccfa6b3 100644 --- a/tests/cli/test_mcp.py +++ b/tests/cli/test_mcp.py @@ -177,6 +177,12 @@ def test_list_with_results(self, parser, mock_api, capsys): out = capsys.readouterr().out assert "weather" in out + def test_list_omits_unsupported_status_column(self, parser, mock_api, capsys): + args = parser.parse_args(["mcp", "list"]) + with patch("modelscope_hub.cli.mcp.make_api", return_value=mock_api): + _McpList(args).execute() + assert "status" not in capsys.readouterr().out.lower() + def test_list_empty(self, parser, mock_api, capsys): mock_api.list_mcp_servers.return_value = PagedResult( items=[], diff --git a/tests/cli/test_openapi.py b/tests/cli/test_openapi.py index c66c384..424b885 100644 --- a/tests/cli/test_openapi.py +++ b/tests/cli/test_openapi.py @@ -155,6 +155,15 @@ def test_within_limit(self, client): with patch.object(client._session, "request", return_value=resp): client.list_mcp_servers(page_number=5, page_size=20) + @pytest.mark.parametrize("page_number", [0, -1]) + def test_rejects_non_positive_page_number_before_request(self, client, page_number): + with patch.object(client._session, "request") as mock_request: + with pytest.raises(InvalidParameter, match="page_number must be an integer >= 1") as excinfo: + client.list_mcp_servers(page_number=page_number) + assert excinfo.value.error_code == "E3021" + assert excinfo.value.retryable is False + mock_request.assert_not_called() + def test_exceeds_limit(self, client): with pytest.raises(InvalidParameter, match="<= 100"): client.list_mcp_servers(page_number=11, page_size=10) @@ -211,6 +220,26 @@ def test_list_repos_mcp_overrides_page_size(self): assert result.page_size == 10 assert result.total_count == 30 + def test_list_mcp_servers_backfills_requested_pagination_when_service_omits_metadata(self): + api = HubApi(config=HubConfig(token="t", endpoint="https://modelscope.cn")) + mcp_response = { + "mcp_server_list": [{"id": "server-6"}], + "total": 30, + } + with patch.object(api.openapi, "list_mcp_servers", return_value=mcp_response) as list_servers: + result = api.list_mcp_servers(page_number=2, page_size=5) + list_servers.assert_called_once_with( + search=None, + page_number=2, + page_size=5, + filter=None, + extra=None, + ) + assert result.items == [{"id": "server-6"}] + assert result.total_count == 30 + assert result.page_number == 2 + assert result.page_size == 5 + # ================================================================== # Item 5: stop_studio no body diff --git a/tests/test_agent_idp.py b/tests/test_agent_idp.py index 5a34b94..c3b1261 100644 --- a/tests/test_agent_idp.py +++ b/tests/test_agent_idp.py @@ -8,6 +8,7 @@ import requests from modelscope_hub._openapi import OpenAPIClient +from modelscope_hub.agent_idp import generate_agent_key_pair from modelscope_hub.api import HubApi from modelscope_hub.config import HubConfig from modelscope_hub.errors import InvalidParameter @@ -145,3 +146,17 @@ def test_facade_converts_issued_token(self): ) assert isinstance(token, AgentToken) assert token.access_token == "jwt" + + def test_private_key_facade_rejects_non_ascii_audience_before_http(self): + private_jwk, _ = generate_agent_key_pair("key-1") + api = HubApi(token="test-token") + api._openapi = MagicMock() + with pytest.raises(InvalidParameter, match="audience must contain only ASCII characters") as raised: + api.issue_agent_token_with_private_key( + private_jwk, + agent_id="agent_id:modelscope:agent_xxx", + audience="高德地图", + timestamp=1700000000, + ) + assert raised.value.error_code == "E3021" + api._openapi.issue_agent_token.assert_not_called() diff --git a/tests/test_agent_idp_keys.py b/tests/test_agent_idp_keys.py index ec5bd2f..259f7bb 100644 --- a/tests/test_agent_idp_keys.py +++ b/tests/test_agent_idp_keys.py @@ -73,3 +73,36 @@ def test_signing_rejects_invalid_timestamp(): private_jwk, _ = generate_agent_key_pair() with pytest.raises(InvalidParameter, match="timestamp"): sign_agent_token_request(private_jwk, agent_id="agent-1", audience="hub", timestamp=0) + + +@pytest.mark.parametrize( + ("field", "value"), + [ + ("audience", "高德地图"), + ("agent_id", "agent-高德"), + ("kid", "kid-高德"), + ], +) +def test_signing_rejects_non_ascii_canonical_component_without_leaking_encoder_error(field, value): + private_jwk, _ = generate_agent_key_pair(value if field == "kid" else "key-1") + kwargs = {"agent_id": "agent-1", "audience": "hub", "timestamp": 100} + if field != "kid": + kwargs[field] = value + with pytest.raises(InvalidParameter) as raised: + sign_agent_token_request(private_jwk, **kwargs) + error = raised.value + assert error.error_code == "E3021" + assert f"{field} must contain only ASCII characters" in str(error) + assert "UnicodeEncodeError" not in str(error) + assert error.__cause__ is None + + +@pytest.mark.parametrize("field", ["agent_id", "audience", "kid"]) +def test_signing_rejects_canonical_message_delimiter(field): + private_jwk, _ = generate_agent_key_pair("key|1" if field == "kid" else "key-1") + kwargs = {"agent_id": "agent-1", "audience": "hub", "timestamp": 100} + if field != "kid": + kwargs[field] = "value|break" + with pytest.raises(InvalidParameter, match="must not contain '\\|'") as raised: + sign_agent_token_request(private_jwk, **kwargs) + assert raised.value.error_code == "E3021"