diff --git a/docs/dev/rest-api/endpoints.md b/docs/dev/rest-api/endpoints.md index ec7ab1cd..3d8f2dad 100644 --- a/docs/dev/rest-api/endpoints.md +++ b/docs/dev/rest-api/endpoints.md @@ -205,7 +205,8 @@ Get the list of titles behind a title ID, for example episodes or a movie, witho | `cdm_type` | string | no | Preferred CDM type. | | `credentials`, `cookies`, `cache` | - | no | Your own login material, on a `--remote-only` server only. See [Client cache on a remote-only server](#client-cache-on-a-remote-only-server). | -You can also pass service-specific CLI options as additional keys. +You can also pass a service's own CLI options, in a nested `service_params` object or as +additional keys. When the body has `service_params`, the server reads service options only from it. #### Logins that need the user @@ -490,9 +491,10 @@ Make a download job. It requires `service` and `title_id`. Every other field is **Validation.** Invalid values return `400 INVALID_PARAMETERS`. `vcodec`, `acodec` and `sub_format` take the same spellings as the CLI options of the same name (`unshackle dl --help` prints each list). The API and the CLI read one list, so they cannot disagree. `range` must be one of SDR/HDR10/HDR10P/DV/HLG/HYBRID, and `HDR10+` is also valid. The bitrate, download worker, and download counts must be positive integers. You may set at most one of the `*_only` flags. You cannot combine `no_subs` with `subs_only`, or `no_audio` with `audio_only`. **Service options.** A service's own CLI options travel in the same body. Send them in a nested -`service_params` object, or as flat top-level keys. `service_params` wins over a flat key of the -same name. An option you leave out takes the default declared on the service's `cli` command, so -the job matches the equivalent `dl` run. `profile` at the top level always means the credential +`service_params` object, or as flat top-level keys. When the body has `service_params`, the server +reads service options only from it, and every flat key is a `dl` value. An option you leave out +takes the default declared on the service's `cli` command, so the job matches the equivalent `dl` +run. `profile` at the top level always means the credential profile, never a service's own `--profile` option. === "Request" @@ -969,7 +971,7 @@ Make a remote session for a service and title. Authentication runs asynchronousl `dl_params` is an object with the `dl` track selection: `lang`, `v_lang`, `a_lang` and `s_lang` (arrays of language strings), `acodec` (an array of audio codec names or `dl -a` aliases such as `ddp`), and `forced_subs` and `exact_lang` (booleans). The service reads these values from `ctx.parent.params`. The server uses the `dl` default for each absent or malformed value, and drops an unknown codec name. -Service CLI options also travel in a nested `service_params` object, which wins over a flat key with the same name. `profile` at the top level always means the credentials profile, never a service's own `--profile` option. +Service CLI options also travel in a nested `service_params` object. When the body has `service_params`, the server reads service options only from it, and a flat key with the same name stays a `dl` value. `profile` at the top level always means the credentials profile, never a service's own `--profile` option. The `proxy` value must be a full proxy URI, unless the operator gives your API key `server_proxy` in the `serve.users` config. Without `server_proxy`, the server does not resolve country codes with its own proxy providers. It rejects the request with `INVALID_PROXY` when no `proxy` is set and the reported `client_region` differs from the server's region. A request that reports no `client_region` is not blocked. Pass `proxy` with your own proxy, or `no_proxy` to accept the server's own connection. With `server_proxy`, the server resolves a country code and picks a proxy for your `client_region` itself. A Control D resolver, `controld://@dns.controld.com`, counts as a full proxy URI: the server runs a forwarder for it and needs no Control D configuration of its own. diff --git a/docs/dev/rest-api/remote-sessions.md b/docs/dev/rest-api/remote-sessions.md index c6b8765d..9750e005 100644 --- a/docs/dev/rest-api/remote-sessions.md +++ b/docs/dev/rest-api/remote-sessions.md @@ -282,9 +282,9 @@ There are two ways DRM keys get resolved, chosen by the client's `server_cdm` fl `server_cdm: false` when it creates the remote session, and the server builds the service on your device even when the API key has the server CDM. - A service option that the service lists in `SERVER_DEVICE_OPTIONS` keeps the server - CDM. The server reads the option from `service_params`, then from the top level of - the request. The create response gives the result in `server_cdm`, and the client - shows a warning when the server did not apply its choice. + CDM. The server reads the option from `service_params`, or from the top level of + a request that has no `service_params`. The create response gives the result in + `server_cdm`, and the client shows a warning when the server did not apply its choice. - The service on the server sees your device, not the server's: its DRM system, security level, system ID and device type. A CDM call that the service makes (for example a session-key exchange at login) goes to your client as a `cdm_call`, diff --git a/tests/remote/unit/test_api_service_dl_overrides.py b/tests/remote/unit/test_api_service_dl_overrides.py index 69060be7..8899eb3a 100644 --- a/tests/remote/unit/test_api_service_dl_overrides.py +++ b/tests/remote/unit/test_api_service_dl_overrides.py @@ -103,3 +103,19 @@ def test_client_sent_keys_uses_the_stamp(): def test_client_sent_keys_without_stamp_counts_all_keys(): assert dm.client_sent_keys({"workers": 1, "quality": None}) == {"workers", "quality"} + + +def test_service_defaults_and_options_travel_nested(env, monkeypatch): + import click + + @click.command() + @click.option("--profile", type=int, default=1) + @click.option("--device", default="chrome") + def cli(**kwargs): + pass + + monkeypatch.setattr(handlers.Services, "load", lambda service: SimpleNamespace(cli=cli)) + params = env(body={"profile": "work", "service_params": {"profile": 2}}) + assert params["profile"] == "work" + assert params["service_params"] == {"profile": 2, "device": "chrome"} + assert env(body={"device": "tv"})["service_params"] == {"profile": 1, "device": "tv"} diff --git a/tests/remote/unit/test_remote_relay.py b/tests/remote/unit/test_remote_relay.py index bc3ff13e..3717e151 100644 --- a/tests/remote/unit/test_remote_relay.py +++ b/tests/remote/unit/test_remote_relay.py @@ -136,6 +136,12 @@ def test_make_remote_service_passes_only_cli_options(remote_config, monkeypatch) assert built["args"][5]["EXAMPLE"] == {"x": 1} +def test_a_service_option_named_like_a_dl_value_passes_only_nested(): + info = {"cli_params": [{"kind": "option", "name": "profile"}]} + assert remote_service.remote_service_params(info, {"service_params": {"profile": "QC"}}) == {"profile": "QC"} + assert remote_service.remote_service_params(info, {"profile": "QC"}) == {} + + def test_remote_error_carries_its_reason_and_exits_a_cli_run(): err = remote_service.RemoteError("Server error [SESSION_NOT_FOUND]: gone", "SESSION_NOT_FOUND") assert isinstance(err, SystemExit) and err.code == 1 and err.kind == "SESSION_NOT_FOUND" diff --git a/tests/remote/unit/test_remote_service_helpers.py b/tests/remote/unit/test_remote_service_helpers.py index 96740917..a18d9a22 100644 --- a/tests/remote/unit/test_remote_service_helpers.py +++ b/tests/remote/unit/test_remote_service_helpers.py @@ -645,9 +645,15 @@ def test_dl_selection_travels_nested_and_a_service_option_cannot_override_it() - "forced_subs": True, "exact_lang": True, } - assert data["lang"] == 3 + assert "lang" not in data and "a_lang" not in data assert data["service_params"] == {"lang": 3, "a_lang": "x"} +def test_a_service_profile_option_travels_nested_and_keeps_the_credential_profile() -> None: + data = _sent_create_data({"profile": "work"}, {"profile": "QC"}) + assert data["profile"] == "work" + assert data["service_params"] == {"profile": "QC"} + + def test_no_dl_params_when_the_ctx_has_no_selection() -> None: assert "dl_params" not in _sent_create_data({}, {}) diff --git a/tests/remote/unit/test_server_cdm_max_height.py b/tests/remote/unit/test_server_cdm_max_height.py index 18128166..ea0aef21 100644 --- a/tests/remote/unit/test_server_cdm_max_height.py +++ b/tests/remote/unit/test_server_cdm_max_height.py @@ -2,6 +2,7 @@ import json import logging +from datetime import datetime from types import SimpleNamespace from uuid import UUID @@ -97,8 +98,8 @@ def test_a_server_device_option_keeps_the_server_cdm_against_the_clients_choice( nested = {"cdm_type": "widevine", "server_cdm": False, "service_params": {"server_side": True}} assert handlers.choose_session_cdm(request("flat"), "EXAMPLE1", nested) == (True, 1080) - # service_params wins over the flat key, as it does when the service is built - nested["server_side"], nested["service_params"] = True, {"server_side": False} + # with service_params present a flat key is a dl value, as it is when the service is built + nested["server_side"], nested["service_params"] = True, {} assert handlers.choose_session_cdm(request("flat"), "EXAMPLE1", nested) == (False, None) @@ -318,8 +319,8 @@ def test_a_capped_download_job_cannot_ask_for_hybrid(hdr_range): handlers.enforce_download_gates({"service": "EXAMPLE1", "quality": [1080], "range": ["HDR10"]}, request("flat")) -def test_the_download_gate_reads_the_service_click_defaults(monkeypatch): - """A service's own click default overrides the serve default, and the gate sees what the job runs with.""" +def test_a_service_click_default_stays_out_of_the_download_gate(monkeypatch): + """A service's own option is not the dl option of the same name, so its default never trips the gate.""" import asyncio from unshackle.core.api import download_manager as dm @@ -329,15 +330,21 @@ def test_the_download_gate_reads_the_service_click_defaults(monkeypatch): monkeypatch.setattr( handlers.Services, "load", lambda service: SimpleNamespace(cli=SimpleNamespace(params=[option])) ) + jobs = [] - def no_job(): - raise AssertionError("a refused job must not reach the download manager") + class Manager: + async def start_workers(self): + pass - monkeypatch.setattr(dm, "get_download_manager", no_job) + def create_job(self, service, title_id, owner_key=None, **params): + jobs.append(params) + return SimpleNamespace(job_id="j", status=SimpleNamespace(value="queued"), created_time=datetime.now()) + + monkeypatch.setattr(dm, "get_download_manager", Manager) body = {"service": "EXAMPLE1", "title_id": "t1", "quality": [1080]} - with pytest.raises(APIError) as ei: - asyncio.run(handlers.download_handler(body, request("flat"))) - assert ei.value.error_code is APIErrorCode.SERVER_CDM_CAPPED + asyncio.run(handlers.download_handler(body, request("flat"))) + assert jobs[0]["service_params"] == {"best_available": True} + assert not jobs[0].get("best_available") @pytest.fixture diff --git a/tests/remote/unit/test_session_log.py b/tests/remote/unit/test_session_log.py index ef6e2f6d..d175494d 100644 --- a/tests/remote/unit/test_session_log.py +++ b/tests/remote/unit/test_session_log.py @@ -121,6 +121,31 @@ def test_transport_keys_block_profile_and_service_params() -> None: assert "service_params" in LIST_HANDLER_TRANSPORT_KEYS +def test_service_params_are_the_only_service_options_when_present() -> None: + import click + + from unshackle.core.api.errors import APIError + from unshackle.core.api.handlers import SESSION_TRANSPORT_KEYS, instantiate_service + + @click.command() + @click.argument("title") + @click.option("--profile", default=None) + @click.option("--device", default=None) + @click.option("--count", type=int, default=1) + def cli(title: str, **kwargs: object) -> dict: + return kwargs + + def build(data: dict) -> dict: + return instantiate_service(click.Context(click.Command("dl")), module, "t", data, SESSION_TRANSPORT_KEYS) + + module = type("Svc", (), {"cli": cli}) + nested = {"profile": "work", "device": "tv", "service_params": {"profile": "QC", "count": "2"}} + assert build(nested) == {"profile": "QC", "device": None, "count": 2} + assert build({"profile": "work", "device": "tv"}) == {"profile": None, "device": "tv", "count": 1} + with pytest.raises(APIError): + build({"service_params": {"count": "x"}}) + + def test_drain_server_logs_survives_client_systemexit() -> None: """RemoteClient raises SystemExit on transport errors; the drain must swallow it.""" from unshackle.core.remote_service import RemoteService diff --git a/unshackle/core/api/download_manager.py b/unshackle/core/api/download_manager.py index e468e53b..32fff10c 100644 --- a/unshackle/core/api/download_manager.py +++ b/unshackle/core/api/download_manager.py @@ -21,6 +21,7 @@ from unshackle.core.api.events import bus, publish_service_event from unshackle.core.api.sanitize import sanitize_log from unshackle.core.utils.click_types import AUDIO_CODEC_LIST, QUALITY_LIST, VIDEO_CODEC_LIST from unshackle.core.utils.redact import REDACTED, URL_USERINFO_RE, redact_all, redact_path, redact_text +from unshackle.core.utils.service_params import service_options log = logging.getLogger("download_manager") @@ -574,14 +575,7 @@ def perform_download( if "title" in service_init_params: service_kwargs["title"] = title_id - for key, value in params.items(): - if key in service_init_params and key not in ["service", "title_id", "profile", "service_params"]: - service_kwargs[key] = value - service_params = params.get("service_params") - if isinstance(service_params, dict): - for key, value in service_params.items(): - if key in service_init_params and key != "title": - service_kwargs[key] = value + service_kwargs.update(service_options(params, service_init_params.keys() - {"title"})) for param_name, param_info in service_init_params.items(): if param_name not in service_kwargs and param_name not in ["self", "ctx"]: diff --git a/unshackle/core/api/handlers.py b/unshackle/core/api/handlers.py index 80ba1d56..bbbaac52 100644 --- a/unshackle/core/api/handlers.py +++ b/unshackle/core/api/handlers.py @@ -15,7 +15,7 @@ from datetime import date as date_ from functools import partial from http.cookiejar import CookieJar from pathlib import Path -from typing import Any, Callable, Dict, Iterable, List, Optional, Set, cast +from typing import AbstractSet, Any, Callable, Dict, Iterable, List, Optional, Set, cast import click from aiohttp import web @@ -45,6 +45,7 @@ from unshackle.core.utils.click_types import AUDIO_CODEC_LIST, QUALITY_LIST, SUB from unshackle.core.utils.collections import ci_get from unshackle.core.utils.hdr10plus import probed_tracks from unshackle.core.utils.redact import REDACTED, URL_USERINFO_RE, redact_all, redact_secrets, redact_text +from unshackle.core.utils.service_params import service_options log = logging.getLogger("api") @@ -246,30 +247,26 @@ def instantiate_service( service_module: Any, title: str, data: Optional[Dict[str, Any]] = None, - transport_keys: Optional[set] = None, + transport_keys: AbstractSet[str] = frozenset(), ) -> Any: - """Instantiate a service by invoking its click cli through Click. + """Instantiate a service by invoking its click cli with the ``service_options`` in ``data``. - Click fills option defaults through ``param.get_default()`` and runs type coercion, - so we no longer have to inspect ``__init__`` or stitch defaults by hand. This function - pulls extra kwargs from the ``data`` entries whose names match a cli option name - and are not in the transport-key blocklist. + Click fills the option defaults, but ``ctx.invoke`` passes the given kwargs uncast, so each value + goes through its option's type here: JSON or a remote client's str-typed stub can send "1" for an int. """ cli_params = getattr(getattr(service_module, "cli", None), "params", []) or [] - cli_param_names = {p.name for p in cli_params if hasattr(p, "name") and p.name} - transport_keys = transport_keys or set() + params_by_name = {p.name: p for p in cli_params if getattr(p, "name", None) and p.name != "title"} extras: Dict[str, Any] = {} if data: - for k, v in data.items(): - if k in cli_param_names and k not in transport_keys and k != "title": - extras[k] = v - service_params = data.get("service_params") - if isinstance(service_params, dict): - for k, v in service_params.items(): - if k in cli_param_names and k != "title": - extras[k] = v - elif service_params is None: - for k in cli_param_names & transport_keys & set(data): + for k, v in service_options(data, params_by_name, transport_keys).items(): + try: + extras[k] = params_by_name[k].type_cast_value(parent_ctx, v) + except click.BadParameter as e: + raise APIError( + APIErrorCode.INVALID_PARAMETERS, f"Invalid service option '{sanitize_log(k)}': {e.message}" + ) from e + if data.get("service_params") is None: + for k in params_by_name.keys() & transport_keys & data.keys(): if k not in ("service", "title_id", "proxy", "no_proxy") and data.get(k) is not None: log.warning( f"Ignoring flat '{sanitize_log(k)}' as a service option, send it under 'service_params'" @@ -1044,9 +1041,7 @@ def choose_session_cdm( return False, None if data.get("server_cdm") is False and data.get("cdm_type"): server_device_options = getattr(Services.load(service), "SERVER_DEVICE_OPTIONS", ()) - service_params = data.get("service_params") - options = {**data, **service_params} if isinstance(service_params, dict) else data - if not any(options.get(option) for option in server_device_options): + if not any(service_options(data, server_device_options).values()): return False, None cap = server_cdm_max_height(request, service) wanted = requested_height(data.get("quality")) @@ -2456,16 +2451,19 @@ async def download_handler(data: Dict[str, Any], request: Optional[web.Request] try: service_module = None if server else Services.load(normalized_service) - service_specific_defaults = {} + service_params: Optional[Dict[str, Any]] = None - # Skip None defaults here: this dict overlays into job params; injecting - # None for keys like `profile` would clobber serve-config overrides. - # Missing required __init__ params are handled in download_manager.perform_download. + # Service defaults and options travel nested: flat, a service default would overwrite the dl value of + # the same name. Missing required __init__ params are handled in download_manager.perform_download. # A remote service applies its own defaults on the remote server. - if hasattr(service_module, "cli") and hasattr(service_module.cli, "params"): - for param in service_module.cli.params: - if hasattr(param, "name") and param.default is not None and not isinstance(param.default, enum.Enum): - service_specific_defaults[param.name] = param.default + cli_params = getattr(getattr(service_module, "cli", None), "params", None) + if cli_params is not None: + defaults = { + param.name: param.default + for param in cli_params + if param.name and param.default is not None and not isinstance(param.default, enum.Enum) + } + service_params = {**defaults, **service_options(data, {param.name for param in cli_params})} filtered_params = {k: v for k, v in data.items() if k not in ["service", "title_id"]} # Overlay any dl-relevant keys from `serve:` config (e.g. downloads, workers) so the API @@ -2478,10 +2476,11 @@ async def download_handler(data: Dict[str, Any], request: Optional[web.Request] params_with_defaults = { **DEFAULT_DOWNLOAD_PARAMS, **serve_overrides, - **service_specific_defaults, **service_dl, **filtered_params, } + if service_params is not None: + params_with_defaults["service_params"] = service_params await asyncio.to_thread(enforce_download_gates, {**params_with_defaults, "service": service_tag}, request) manager = get_download_manager() diff --git a/unshackle/core/api/routes.py b/unshackle/core/api/routes.py index 8ea6a634..e1b6b362 100644 --- a/unshackle/core/api/routes.py +++ b/unshackle/core/api/routes.py @@ -1958,8 +1958,9 @@ async def session_create(request: web.Request) -> web.Response: description: | False when the user picked the local CDM. With `cdm_type`, the server then builds the service on the client's device, also for an API key with the server CDM. - A request that sets one of the service's `SERVER_DEVICE_OPTIONS`, in - `service_params` or at the top level, keeps the server CDM + A request that sets one of the service's `SERVER_DEVICE_OPTIONS` keeps the server + CDM. The server reads it from `service_params`, or from the top level when + the request has no `service_params` cdm_type: type: string enum: [widevine, playready] diff --git a/unshackle/core/remote_service.py b/unshackle/core/remote_service.py index f7c5dd62..46189c19 100644 --- a/unshackle/core/remote_service.py +++ b/unshackle/core/remote_service.py @@ -55,6 +55,7 @@ from unshackle.core.tunnel import TunnelClient from unshackle.core.utils import run_proxy from unshackle.core.utils.collections import ci_get from unshackle.core.utils.redact import redact_path, redact_text, safe_display_url +from unshackle.core.utils.service_params import service_options from unshackle.core.vault import Vault log = logging.getLogger("remote_service") @@ -873,15 +874,9 @@ def find_remote_service(services: list[dict], name: str) -> Optional[dict]: def remote_service_params(info: dict, data: Dict[str, Any]) -> Dict[str, Any]: - """The service options to send: ``data["service_params"]``, else the ``data`` keys naming a cli option. - - Only the remote service's cli option names pass, either way: RemoteService merges them into the - session-create body, where a key such as ``service`` or ``proxy`` would override the checked value. - """ + """The remote service's cli options in ``data``, read by ``service_options``.""" names = {p.get("name") for p in info.get("cli_params") or [] if p.get("kind") == "option"} - names -= {"title", "service", "title_id", "profile", "proxy", "no_proxy"} - source = data.get("service_params") if isinstance(data.get("service_params"), dict) else data - return {k: v for k, v in source.items() if k in names and v is not None and v is not False} + return {k: v for k, v in service_options(data, names).items() if v is not None and v is not False} def make_remote_service( @@ -1385,9 +1380,8 @@ class RemoteService: if dl_params: create_data["dl_params"] = dl_params - if self._service_params: - create_data.update(self._service_params) - create_data["service_params"] = self._service_params + # sent even empty: without it the server reads service options from the flat dl keys + create_data["service_params"] = self._service_params # sent with the server CDM too: a capped server hands the tracks above its cap to this device cdm = self.local_cdm diff --git a/unshackle/core/utils/service_params.py b/unshackle/core/utils/service_params.py new file mode 100644 index 00000000..7aed319e --- /dev/null +++ b/unshackle/core/utils/service_params.py @@ -0,0 +1,18 @@ +from typing import Any, Collection, Dict, Iterable + +FLAT_DL_KEYS = frozenset({"title", "service", "title_id", "profile", "proxy", "no_proxy"}) + + +def service_options( + data: Dict[str, Any], names: Collection[str], flat_excluded: Iterable[str] = FLAT_DL_KEYS +) -> Dict[str, Any]: + """A request's service options named in ``names``: from its ``service_params`` object, else its flat keys. + + With ``service_params`` present every flat key is a dl value, so it never fills a service option of the same name. + Without it, the flat keys in ``flat_excluded`` stay dl values. + """ + nested = data.get("service_params") + if isinstance(nested, dict): + return {k: v for k, v in nested.items() if k in names} + excluded = set(flat_excluded) + return {k: v for k, v in data.items() if k in names and k not in excluded}