From 13b191adf93644cc2df6e92f2f0cebe5a0beb551 Mon Sep 17 00:00:00 2001 From: Carl Robben Date: Fri, 29 May 2026 17:16:46 +1200 Subject: [PATCH 1/2] fix: skip up-front auth for admin-install and health, surface 'already installed' cleanly - New get_unauthenticated_client() factory in client.py; only requires DATAMASQUE_URL (no username/password) - admin-install and health switch to it; the endpoints themselves are anonymous and shouldn't gate on a token - admin-install catches 401 from /api/users/admin-install/ and returns ErrorCode.CONFLICT with a clear message - Tests cover the new factory, both command repointings, the 401 conflict translation, and the no-401-swallow path - New integration test exercises both commands end-to-end against a real instance, skips when already configured Closes #12 --- src/datamasque_cli/client.py | 72 +++++++++++++++++- src/datamasque_cli/commands/system.py | 38 ++++++++-- tests/commands/test_system.py | 104 ++++++++++++++++++++++++++ tests/integration/test_system.py | 96 ++++++++++++++++++++++++ tests/test_client_auth.py | 23 +++++- 5 files changed, 321 insertions(+), 12 deletions(-) create mode 100644 tests/integration/test_system.py diff --git a/src/datamasque_cli/client.py b/src/datamasque_cli/client.py index 73e9c0a..99394cd 100644 --- a/src/datamasque_cli/client.py +++ b/src/datamasque_cli/client.py @@ -48,6 +48,25 @@ def profile_from_env() -> Profile | None: return None +def _profile_from_env_url_only() -> Profile | None: + """Build a URL-only profile from `DATAMASQUE_URL`, with empty username/password. + + Used by the unauthenticated client factory so callers can hit anonymous + endpoints (admin-install, health) without setting `DATAMASQUE_USERNAME` + and `DATAMASQUE_PASSWORD` -- those fields aren't read by anonymous calls + and demanding them is friction for the first-run setup workflow. + """ + url = os.environ.get(ENV_URL) + if not url: + return None + return Profile( + url=url.rstrip("/"), + username="", + password="", + verify_ssl=_verify_ssl_from_env(default=True), + ) + + def _resolve_profile(config: Config, profile_name: str | None) -> Profile: profile = config.get_profile(profile_name) if not profile.is_configured: @@ -60,6 +79,25 @@ def _resolve_profile(config: Config, profile_name: str | None) -> Profile: return profile +def _resolve_profile_for_unauthenticated(profile_name: str | None) -> Profile: + """Resolve a profile for an unauthenticated call -- only the URL is required. + + Order: explicit `--profile`, env vars (URL-only is sufficient here), + saved active profile. If none yield a URL, abort with a clear hint. + """ + if profile_name is not None: + profile = load_config().get_profile(profile_name) + else: + profile = _profile_from_env_url_only() or load_config().get_profile() + if not profile.url: + abort( + "No DataMasque URL configured.", + code=ErrorCode.AUTH_REQUIRED, + hint=f"Set {ENV_URL} or run: dm auth login", + ) + return profile + + def _resolve_profile_with_verify(profile_name: str | None) -> tuple[Profile, bool]: """Resolve the active `Profile` and apply env-var overrides for `verify_ssl`.""" env_profile = profile_from_env() if profile_name is None else None @@ -98,17 +136,43 @@ def get_client(profile_name: str | None = None) -> DataMasqueClient: `DATAMASQUE_VERIFY_SSL` always wins over the stored profile so you can flip TLS verification per-call without re-running `dm auth login`. """ - profile, verify_ssl = _resolve_profile_with_verify(profile_name) + client, profile, verify_ssl = _build_client(profile_name) + _authenticate_or_abort(client, profile.url, verify_ssl=verify_ssl) + return client + + +def get_unauthenticated_client(profile_name: str | None = None) -> DataMasqueClient: + """Build a `DataMasqueClient` without performing the up-front login handshake. + + Used by commands that hit endpoints which don't require — or can't yet + use — a token. `admin-install` is the canonical example: on a fresh + server there's no user to authenticate as, so `client.authenticate()` + would always fail before the command ran. + + Only `DATAMASQUE_URL` (or a profile with a URL) is required — username + and password aren't read by anonymous endpoints, so demanding them + would be unnecessary friction for first-run setup. + """ + profile = _resolve_profile_for_unauthenticated(profile_name) + verify_ssl = _verify_ssl_from_env(default=profile.verify_ssl) instance_config = DataMasqueInstanceConfig( base_url=profile.url, username=profile.username, password=profile.password, verify_ssl=verify_ssl, ) + return DataMasqueClient(instance_config) - client = DataMasqueClient(instance_config) - _authenticate_or_abort(client, profile.url, verify_ssl=verify_ssl) - return client + +def _build_client(profile_name: str | None) -> tuple[DataMasqueClient, Profile, bool]: + profile, verify_ssl = _resolve_profile_with_verify(profile_name) + instance_config = DataMasqueInstanceConfig( + base_url=profile.url, + username=profile.username, + password=profile.password, + verify_ssl=verify_ssl, + ) + return DataMasqueClient(instance_config), profile, verify_ssl # Substrings that suggest the underlying error was a TLS failure rather than diff --git a/src/datamasque_cli/commands/system.py b/src/datamasque_cli/commands/system.py index d0543dc..adbb63d 100644 --- a/src/datamasque_cli/commands/system.py +++ b/src/datamasque_cli/commands/system.py @@ -5,10 +5,11 @@ from pathlib import Path import typer +from datamasque.client.exceptions import DataMasqueApiError -from datamasque_cli.client import get_client +from datamasque_cli.client import get_client, get_unauthenticated_client from datamasque_cli.commands.rulesets import export_bundle, import_bundle -from datamasque_cli.output import print_json, print_success, print_warning, render_output, should_emit_json +from datamasque_cli.output import ErrorCode, abort, print_json, print_success, print_warning, render_output, should_emit_json app = typer.Typer(help="System administration commands.", no_args_is_help=True) @@ -18,8 +19,14 @@ def health( profile: str | None = typer.Option(None, "--profile", "-p", help="Profile to use"), is_json: bool = typer.Option(False, "--json", help="Output as JSON"), ) -> None: - """Check DataMasque instance health.""" - client = get_client(profile) + """Check DataMasque instance health. + + Uses an unauthenticated client because `/api/healthcheck/` does not require a + token and should answer even when no admin user has been created yet -- + the whole point of a health probe is to be the lowest-friction signal of + "is the server up?" + """ + client = get_unauthenticated_client(profile) client.healthcheck() if should_emit_json(is_json): @@ -101,9 +108,26 @@ def admin_install( password: str = typer.Option(..., prompt=True, hide_input=True, help="Admin password"), profile: str | None = typer.Option(None, "--profile", "-p", help="Profile to use"), ) -> None: - """Initial admin setup for a fresh DataMasque instance.""" - client = get_client(profile) - client.admin_install(email=email, username=username, password=password) + """Initial admin setup for a fresh DataMasque instance. + + Uses an unauthenticated client because the admin-install endpoint is itself + anonymous and on a fresh server there's no user account to authenticate as. + """ + client = get_unauthenticated_client(profile) + try: + client.admin_install(email=email, username=username, password=password) + except DataMasqueApiError as e: + # The server returns 401 on /api/users/admin-install/ once any user exists -- + # the endpoint is gated on "no user has been created yet" and DRF treats it + # as a normal auth-required endpoint thereafter. Translate that into a clear + # message rather than letting the underlying "401 Unauthorized" bubble up. + if e.response is not None and e.response.status_code == 401: + abort( + "Admin install is already complete on this DataMasque instance.", + code=ErrorCode.CONFLICT, + hint="Use `dm auth login` to sign in as an existing user.", + ) + raise print_success(f"Admin user '{username}' created.") diff --git a/tests/commands/test_system.py b/tests/commands/test_system.py index 2b855fd..0678bb1 100644 --- a/tests/commands/test_system.py +++ b/tests/commands/test_system.py @@ -4,6 +4,7 @@ from unittest.mock import MagicMock, patch import pytest +from datamasque.client.exceptions import DataMasqueApiError from datamasque.client.models.license import LicenseInfo, SwitchableLicenseMetadata from typer.testing import CliRunner @@ -80,3 +81,106 @@ def test_ai_engine_set_patches_settings_with_url(mock_get_client: MagicMock, run client.make_request.assert_called_once_with( "PATCH", "/api/settings/", data={"dm_ai_engine_url": "http://engine.example.com:9021"} ) + + +# Commands that hit anonymous endpoints must use `get_unauthenticated_client`, not `get_client`. +# Using `get_client` would call `authenticate()` first, which always fails on a fresh server +# (no admin user yet → 401 → SystemExit) and never reaches the actual endpoint. + + +@patch(f"{MODULE}.get_unauthenticated_client") +@patch(f"{MODULE}.get_client") +def test_admin_install_uses_unauthenticated_client( + mock_get_client: MagicMock, mock_get_unauth: MagicMock, runner: CliRunner +) -> None: + client = MagicMock() + mock_get_unauth.return_value = client + + result = runner.invoke( + app, + [ + "system", "admin-install", + "--email", "admin@example.com", + "--username", "admin", + "--password", "P@ssword12", + ], + ) + + assert result.exit_code == 0, result.output + mock_get_unauth.assert_called_once() + mock_get_client.assert_not_called() + client.admin_install.assert_called_once_with( + email="admin@example.com", username="admin", password="P@ssword12" + ) + + +@patch(f"{MODULE}.get_unauthenticated_client") +def test_admin_install_translates_401_into_conflict( + mock_get_unauth: MagicMock, runner: CliRunner +) -> None: + """A 401 from /api/users/admin-install/ means the instance is already set up. + + Translate the raw API error into a user-facing conflict message instead of + letting the misleading "Unable to login" traceback bubble up. + """ + client = MagicMock() + mock_get_unauth.return_value = client + response = MagicMock() + response.status_code = 401 + client.admin_install.side_effect = DataMasqueApiError("401 Unauthorized", response=response) + + result = runner.invoke( + app, + [ + "system", "admin-install", + "--email", "admin@example.com", + "--username", "admin", + "--password", "P@ssword12", + ], + ) + + assert result.exit_code == 8 # ErrorCode.CONFLICT + assert "already complete" in result.stderr + assert "dm auth login" in result.stderr + + +@patch(f"{MODULE}.get_unauthenticated_client") +def test_admin_install_does_not_swallow_non_401_errors( + mock_get_unauth: MagicMock, runner: CliRunner +) -> None: + """Only 401 is the "already installed" signal -- other errors must surface.""" + client = MagicMock() + mock_get_unauth.return_value = client + response = MagicMock() + response.status_code = 400 + client.admin_install.side_effect = DataMasqueApiError("400 Bad Request", response=response) + + result = runner.invoke( + app, + [ + "system", "admin-install", + "--email", "admin@example.com", + "--username", "admin", + "--password", "weak", + ], + ) + + assert result.exit_code != 0 + assert "already complete" not in result.stderr + + +@patch(f"{MODULE}.get_unauthenticated_client") +@patch(f"{MODULE}.get_client") +def test_health_uses_unauthenticated_client( + mock_get_client: MagicMock, mock_get_unauth: MagicMock, runner: CliRunner +) -> None: + client = MagicMock() + mock_get_unauth.return_value = client + + result = runner.invoke(app, ["system", "health", "--json"]) + + assert result.exit_code == 0, result.output + mock_get_unauth.assert_called_once() + mock_get_client.assert_not_called() + client.healthcheck.assert_called_once_with() + assert '"status": "healthy"' in result.stdout diff --git a/tests/integration/test_system.py b/tests/integration/test_system.py new file mode 100644 index 0000000..3a8ee42 --- /dev/null +++ b/tests/integration/test_system.py @@ -0,0 +1,96 @@ +"""Integration tests for `dm system` commands that hit anonymous endpoints. + +These end-to-end exercise the `get_unauthenticated_client` path against a real +DataMasque instance, the regression they guard against (silently re-introducing +`get_client` on either command) can only be caught against a real server: a unit +test can mock the factory, but only a live instance reveals that the auth call +fails on a fresh server. +""" + +from __future__ import annotations + +import json +import os +import uuid + +import pytest +from typer.testing import CliRunner + +from datamasque_cli.main import app + +pytestmark = pytest.mark.integration + + +def _is_installed(runner: CliRunner) -> bool: + """Check installation state via the dm CLI's health-style probe. + + `/api/app/check/` is what the admin frontend uses to decide whether to show + the install wizard, so it's the canonical signal for "this instance is + already set up". + + Implemented via the python client directly because there's no `dm` command + for `/api/app/check/` yet; that's the only thing here that bypasses the CLI. + """ + # Build a URL-only client so this works on a fresh instance too. + from datamasque_cli.client import get_unauthenticated_client + + client = get_unauthenticated_client() + response = client.make_request("GET", "/api/app/check/", requires_authorization=False) + return bool(response.json().get("installed")) + + +def test_health_works_without_authentication(runner: CliRunner, monkeypatch: pytest.MonkeyPatch) -> None: + """`dm system health` must succeed even when no credentials are configured. + + The whole point of a health probe is to be the lowest-friction "is the + server up?" signal -- it shouldn't depend on a valid login. + """ + monkeypatch.delenv("DATAMASQUE_USERNAME", raising=False) + monkeypatch.delenv("DATAMASQUE_PASSWORD", raising=False) + + result = runner.invoke(app, ["system", "health", "--json"]) + + assert result.exit_code == 0, result.output + assert json.loads(result.stdout)["status"] == "healthy" + + +def test_admin_install_creates_admin_user_on_fresh_instance( + runner: CliRunner, monkeypatch: pytest.MonkeyPatch +) -> None: + """End-to-end admin-install against a real instance. + + Skips when the instance is already configured -- the endpoint is gated on + "no user has been created yet" and reusing a server between runs is the + common case. To exercise this path, point the integration suite at a + freshly-restarted DataMasque (e.g. `docker compose down -v && up -d`). + """ + if _is_installed(runner): + pytest.skip( + "Instance is already installed. Reset it (e.g. `docker compose down -v && up -d`) " + "to exercise the admin-install path." + ) + + # `dm system admin-install` needs no credentials -- the password is passed + # via --password and the endpoint itself is anonymous. Clear the env vars + # the parent fixture set so this test proves the no-creds path works. + monkeypatch.delenv("DATAMASQUE_USERNAME", raising=False) + monkeypatch.delenv("DATAMASQUE_PASSWORD", raising=False) + + # Use the same credentials the parent fixture configures, so any + # subsequent authenticated tests in this session can log in. + username = os.environ["DM_TEST_USERNAME"] + password = os.environ["DM_TEST_PASSWORD"] + email = f"{uuid.uuid4().hex[:8]}@dm-integration.test" + + result = runner.invoke( + app, + [ + "system", "admin-install", + "--email", email, + "--username", username, + "--password", password, + ], + ) + + assert result.exit_code == 0, result.output + assert _is_installed(runner), "Instance should be marked installed after admin-install" diff --git a/tests/test_client_auth.py b/tests/test_client_auth.py index 74af231..a33dc88 100644 --- a/tests/test_client_auth.py +++ b/tests/test_client_auth.py @@ -5,7 +5,7 @@ import pytest from datamasque.client.exceptions import DataMasqueApiError, DataMasqueTransportError -from datamasque_cli.client import _format_transport_error, get_client +from datamasque_cli.client import _format_transport_error, get_client, get_unauthenticated_client from datamasque_cli.config import Config, Profile @@ -46,6 +46,27 @@ def test_get_client_aborts_on_auth_failure( assert exc_info.value.code == 7 # auth_failed +@patch("datamasque_cli.client.DataMasqueClient") +@patch("datamasque_cli.client.load_config") +def test_get_unauthenticated_client_does_not_call_authenticate( + mock_load: MagicMock, mock_client_cls: MagicMock, monkeypatch: pytest.MonkeyPatch +) -> None: + """Commands that hit anonymous endpoints (admin-install, health) must not try + to log in first -- on a fresh server there's no user to authenticate as.""" + monkeypatch.delenv("DATAMASQUE_URL", raising=False) + monkeypatch.delenv("DATAMASQUE_USERNAME", raising=False) + monkeypatch.delenv("DATAMASQUE_PASSWORD", raising=False) + + config = Config() + config.set_profile("default", Profile(url="https://dm", username="admin", password="secret")) + mock_load.return_value = config + + client = get_unauthenticated_client() + + assert client is mock_client_cls.return_value + mock_client_cls.return_value.authenticate.assert_not_called() + + @pytest.mark.parametrize( ("error_message", "verify_ssl", "expect_hint"), [ From 2fa9e1b63c7df4e13e633ff5525ac561056c3c1b Mon Sep 17 00:00:00 2001 From: Carl Robben Date: Fri, 29 May 2026 17:26:30 +1200 Subject: [PATCH 2/2] docs(contributing): document running dm locally and pairing a local datamasque-python checkout - Add 'Running dm locally' section: editable install, env-var vs profile auth - Add cross-repo editable-install workflow for a sibling datamasque-python checkout --- CONTRIBUTING.md | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e48bad2..83b3fa1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -41,6 +41,50 @@ uv sync Then either activate the venv (`source .venv/bin/activate`) or prefix commands with `uv run`. +## Running `dm` locally + +`uv sync` installs the CLI in editable mode, +so the `dm` entry point on the venv reflects your working tree — +no reinstall after each edit. + +```console +uv run dm --version # one-shot, no venv activation needed +source .venv/bin/activate && dm --version # or activate once per shell +``` + +Point it at a DataMasque instance. +For ad-hoc development, env vars are the lowest-friction path +(no `~/.config/datamasque-cli/config.toml` to clean up afterwards): + +```console +export DATAMASQUE_URL=http://127.0.0.1:8000 +export DATAMASQUE_USERNAME=admin +export DATAMASQUE_PASSWORD='P@ssword12' +export DATAMASQUE_VERIFY_SSL=false # for self-signed local builds +dm system health +dm connections list +``` + +For longer-lived work, save a profile with `dm auth login` +(stored at `~/.config/datamasque-cli/config.toml`, mode 600). + +### Pairing with a local `datamasque-python` checkout + +`datamasque-cli` depends on the `datamasque-python` package +for its actual API client. +If you're changing both repos at once +(for example, adding a new endpoint that needs a CLI surface), +install the sibling checkout in editable mode against the CLI's venv: + +```console +uv pip install -e ../datamasque-python +``` + +The dependency is satisfied by the local checkout +and edits to either repo are picked up immediately by `dm`. +A subsequent `uv sync` will re-pin to the registered version — +re-run the `uv pip install -e` if you want the local override back. + ## Running the tests ```console