Improve bfabric-cli auth command group

bfabric/docs/design/oauth_integration.md

@@ -45,8 +45,8 @@ All commands registered under `bfabric-cli auth` via cyclopts.
 
 | Command | File | Description |
 |---------|------|-------------|
-| `auth login <base_url>` | `cli/login/pkce.py` | Browser-based OAuth login. Caches tokens + writes config. |
-| `auth device-code <base_url>` | `cli/login/device_code.py` | Headless OAuth login. |
+| `auth login <base_url>` | `cli/login/oauth_login.py` | Browser-based OAuth login. Caches tokens + writes config. |
+| `auth device-code <base_url>` | `cli/login/oauth_login.py` | Headless OAuth login. |
 | `auth pat <base_url>` | `cli/login/pat.py` | Personal Access Token login. |
 | `auth register <client_name> <redirect_uri>` | `cli/login/register.py` | RFC 7591 dynamic client registration. Outputs JSON. |
 | `auth status` | `cli/login/status.py` | Show current auth status for an environment. |

bfabric/src/bfabric/config/config_writer.py

@@ -1,15 +1,14 @@
 """Write environment entries to the bfabricpy YAML config file.
 
-Note: ``yaml.dump`` does not preserve YAML comments — any comments in the
-existing config file will be lost when the file is rewritten.
+Note: rewriting the file drops any YAML comments in it (``yaml.dump`` doesn't preserve them).
 """
 
 from __future__ import annotations
 
 import copy
 import os
 from pathlib import Path
-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING, cast
 
 import yaml
 
@@ -20,12 +19,11 @@
 
 
 def _write_config_file(config_path: Path, data: Mapping[str, object]) -> None:
-    """Serialize *data* to *config_path* as YAML with ``0o600`` permissions.
-
-    Centralizes the secret-safe write shared by every config mutation: creates parent
-    directories, dumps the mapping, and forces ``0o600`` even on a pre-existing file (whose
-    permissions ``os.open`` would otherwise leave untouched) so a secret never lands in a
+    """Serialize *data* to *config_path* as YAML, always ``0o600`` so a secret never lands in a
     group/world-readable file.
+
+    The explicit ``fchmod`` forces the mode even on a pre-existing file, whose permissions
+    ``os.open`` would otherwise leave untouched.
     """
     config_path = Path(config_path).expanduser()
     config_path.parent.mkdir(parents=True, exist_ok=True)
@@ -39,16 +37,12 @@ def _write_config_file(config_path: Path, data: Mapping[str, object]) -> None:
 
 
 def _validate_round_trip(env_name: str, env_data: Mapping[str, object]) -> None:
-    """Reject an environment that the reader could not load back.
+    """Reject an environment the reader could not load back, so a write either persists a parseable
+    config or fails without touching the file.
 
-    Mirrors the reader's invariants so a write either persists a parseable config or fails
-    cleanly without touching the file. Checks two things:
-
-    * The name is not reserved -- the reader treats ``GENERAL`` as the general section and
-      forbids ``default``, so neither can be an environment.
-    * The fields form a valid :class:`EnvironmentConfig` (e.g. ``base_url`` present, auth
-      combination consistent). Only the single environment is validated, not the merged file,
-      so a pre-existing legacy environment can't block writing a new, valid one.
+    Rejects the reserved names (``GENERAL`` is the general section, ``default`` is forbidden) and
+    anything that isn't a valid :class:`EnvironmentConfig`. Only this one environment is validated,
+    not the merged file, so a pre-existing legacy environment can't block writing a new valid one.
     """
     if env_name in ("GENERAL", "default"):
         raise ValueError(f"Environment name {env_name!r} is reserved and cannot be used.")
@@ -62,26 +56,19 @@ def write_environment_to_config(
     *,
     set_default: bool,
 ) -> None:
-    """Write (or update) an environment section in the bfabricpy YAML config.
-
-    * Creates the file if it does not exist.
-    * Merges *env_data* into the target environment, preserving other
-      environments.
-    * Sets ``GENERAL.default_config`` to *env_name* when *set_default* is
-      ``True``.
-    * File permissions are set to ``0o600``.
+    """Write (or update) an environment section in the bfabricpy YAML config, creating the file
+    (mode ``0o600``) if needed and preserving other environments. Sets ``GENERAL.default_config``
+    to *env_name* when *set_default*.
 
     :param config_path: Path to the YAML config file (will be expanded).
     :param env_name: Name of the environment to create / update.
-    :param env_data: Dictionary of fields for the environment.
-    :param set_default: If ``True``, set this environment as the default. Required: callers must
-        decide explicitly whether the new environment becomes the default.
-    :raises pydantic.ValidationError: If *env_data* would not parse back through the reader
-        (e.g. a missing ``base_url`` or an invalid auth combination). Validated before any
-        filesystem change, so a rejected write leaves an existing config untouched.
+    :param env_data: Fields for the environment.
+    :param set_default: Whether this environment becomes the default. Required: callers must decide
+        explicitly.
+    :raises pydantic.ValidationError: If *env_data* would not parse back through the reader (e.g. a
+        missing ``base_url`` or an invalid auth combination). Checked before any filesystem change,
+        so a rejected write leaves an existing config untouched.
     """
-    # Guarantee the round-trip before any filesystem change, so a rejected write leaves an
-    # existing config untouched.
     _validate_round_trip(env_name, env_data)
 
     config_path = Path(config_path).expanduser()
@@ -108,15 +95,13 @@ def write_environment_to_config(
 def set_default_config(config_path: Path, env_name: str) -> None:
     """Set ``GENERAL.default_config`` to an already-defined environment.
 
-    Unlike :func:`write_environment_to_config`, this only flips the default -- it never
-    creates or modifies an environment. Other environments and the general section are
-    preserved.
+    Only flips the default; never creates or modifies an environment.
 
     :param config_path: Path to the YAML config file (will be expanded).
     :param env_name: Name of an existing environment to mark as default.
     :raises FileNotFoundError: If the config file does not exist.
-    :raises ValueError: If *env_name* is not among the configured environments; the file is
-        left untouched in that case.
+    :raises ValueError: If *env_name* is not among the configured environments; the file is left
+        untouched.
     """
     config_path = Path(config_path).expanduser()
     if not config_path.is_file():
@@ -126,13 +111,10 @@ def set_default_config(config_path: Path, env_name: str) -> None:
     existing: dict[str, object]
     existing = loaded if isinstance(loaded, dict) else {}  # pyright: ignore[reportUnknownVariableType]
 
-    # Enumerate the configured environments through the reader so the check matches how the
-    # file will actually load back. ConfigFile's "before" validators mutate their input in
-    # place, so always validate a deep copy and keep ``existing`` pristine for the write.
-    # This also doubles as the round-trip guard: since env_name is confirmed to be one of
-    # config_file_obj.environments, and environments are otherwise untouched below, setting
-    # GENERAL.default_config to env_name cannot fail ConfigFile's own default-config-must-exist
-    # validator -- so no second validation pass is needed after the mutation.
+    # Validate through the reader (on a deep copy — ConfigFile's "before" validators mutate their
+    # input in place) so the membership check matches how the file loads back, keeping ``existing``
+    # pristine for the write. This doubles as the round-trip guard: env_name is a known environment
+    # and the environments are untouched below, so setting the default can't fail the reader after.
     config_file_obj = ConfigFile.model_validate(copy.deepcopy(existing))
     if env_name not in config_file_obj.environments:
         available = ", ".join(sorted(config_file_obj.environments)) or "(none)"
@@ -144,3 +126,41 @@ def set_default_config(config_path: Path, env_name: str) -> None:
     general["default_config"] = env_name
 
     _write_config_file(config_path, existing)
+
+
+def remove_environment_from_config(config_path: Path, env_name: str) -> None:
+    """Delete an environment section from the bfabricpy YAML config.
+
+    Also clears ``GENERAL.default_config`` if it pointed at *env_name* — otherwise the reader would
+    refuse to load a file whose default names a missing environment.
+
+    :param config_path: Path to the YAML config file (will be expanded).
+    :param env_name: Name of an existing environment to remove.
+    :raises FileNotFoundError: If the config file does not exist.
+    :raises ValueError: If *env_name* is not among the configured environments; the file is left
+        untouched.
+    """
+    config_path = Path(config_path).expanduser()
+    if not config_path.is_file():
+        raise FileNotFoundError(f"Config file not found: {config_path}")
+
+    loaded: object = yaml.safe_load(config_path.read_text())  # pyright: ignore[reportAny]
+    existing: dict[str, object]
+    existing = loaded if isinstance(loaded, dict) else {}  # pyright: ignore[reportUnknownVariableType]
+
+    # Enumerate through the reader so the membership check matches how the file loads back.
+    # ConfigFile's "before" validators mutate their input in place, so validate a deep copy and
+    # keep ``existing`` pristine for the write.
+    config_file_obj = ConfigFile.model_validate(copy.deepcopy(existing))
+    if env_name not in config_file_obj.environments:
+        available = ", ".join(sorted(config_file_obj.environments)) or "(none)"
+        raise ValueError(f"Environment {env_name!r} is not defined. Available environments: {available}")
+
+    _ = existing.pop(env_name, None)
+    general = existing.get("GENERAL")
+    if isinstance(general, dict):
+        general_map = cast("dict[str, object]", general)
+        if general_map.get("default_config") == env_name:
+            _ = general_map.pop("default_config", None)
+
+    _write_config_file(config_path, existing)

bfabric_scripts/docs/changelog.md

@@ -10,21 +10,13 @@ Versioning currently follows `X.Y.Z` semantic versioning, independent of the `bf
 
 ## \[Unreleased\]
 
-- `auth login` / `auth device-code` now **require** an explicit `--scope` (no default). Named presets document the useful sets — `read-only` (`api:read`), `read-write` (`api:write`), `upload` (`api:write tus`) — or pass any scope string. Client/webapp registration (`auth register` / `auth register-webapp`) keeps the OIDC-inclusive default, which webapps need for user-identity claims.
-
 ## \[1.16.0rc2\] - 2026-07-15
 
-- `bfabric-cli auth` command group for OAuth authentication and client management:
-    - Login: `auth login` (browser), `auth device-code` (headless), `auth pat` (Personal Access Token).
-    - `auth register` / `auth register-webapp` — dynamic client registration, optionally with a linked B-Fabric app.
-    - `auth default` / `auth default set [CONFIG_ENV]` — show and set the default environment (interactive picker when no value is given).
-    - `auth status` and `auth logout`.
-    - `auth pat` stores the token under a `pat` key with `auth_method: pat` (not `login: __oauth__` / `password:`), keeping the shared config parseable by older (≤1.19.0) clients; `auth status` reports these as `pat`.
-- `bfabric-cli workunit upload FILES...` — upload files and directories to a workunit over tus (resumable, large-file capable), creating a new workunit or targeting `--workunit-id`. One resource per file, skips duplicates (unless `--force`), expands directories, live progress (`--no-progress`); `--track-job` records an `UPLOAD` job. Requires an OAuth client with the `tus` scope.
-- `bfabric-cli api create` / `api update` accept `--format json|yaml|tsv|table_rich` (default `json`), matching `api read`.
-- `bfabric-cli dataset update` — update an existing dataset with a change preview before confirming (`csv`/`tsv`/`xlsx`/`parquet`, same validation flags as `dataset upload`).
-- `api create` / `api update` now emit valid JSON (was Python `repr` via `rich.pretty.pprint`, breaking `jq`) and serialise `datetime` / `Decimal` to strings instead of raising `TypeError` ([#503](https://github.com/fgcz/bfabricPy/issues/503)).
-- Internal: `dataset upload` and `bfabric_save_csv2dataset.py` now use `bfabric.operations.dataset.create_dataset`; API error handling is centralized in `@use_client` (no more `@logger.catch`); `--config-env` naming unified. Declares `lxml` as an explicit dependency (was transitive via the now-optional `zeep`) and requires `bfabric[transfer]` >= 1.20 (for `upload_files` / tus).
+- `bfabric-cli auth` — OAuth authentication & client management. Login: `login` (browser), `device-code` (headless), `pat`; client registration: `register` / `register-webapp`; environment management: `default`, `list`, `status`, `logout`. Scope presets (`read-only` / `read-write` / `upload`) or a raw scope, via an interactive picker when `--scope` is omitted in a terminal; no baked-in default scope, so a headless run must pass `--scope` (registration keeps the OIDC-inclusive default webapps need). When `--config-env` is omitted it prompts for the environment (else targets the current default / `PRODUCTION`); unless `--set-default` / `--no-set-default` is given it asks (default yes) whether to make the env the default, and cancelling that prompt aborts the login. `status` reports an OAuth env's cached-token freshness and granted scope (annotated with the matching preset); `logout` removes an env's config entry and cached tokens (confirmation required). PATs are stored under a `pat` key (`auth_method: pat`), keeping the config parseable by ≤1.19.0 clients.
+- `bfabric-cli workunit upload FILES...` — upload files/directories to a workunit over tus (resumable, large-file capable): new or `--workunit-id`, one resource per file, skips duplicates (`--force`), live progress (`--no-progress`), optional `--track-job`. Requires an OAuth client with the `tus` scope.
+- `bfabric-cli api create` / `api update` — accept `--format json|yaml|tsv|table_rich` (default `json`); now emit valid JSON and serialise `datetime` / `Decimal` (was Python `repr`, breaking `jq`) ([#503](https://github.com/fgcz/bfabricPy/issues/503)).
+- `bfabric-cli dataset update` — update an existing dataset with a change preview before confirming (`csv`/`tsv`/`xlsx`/`parquet`).
+- Internal: `dataset upload` / `bfabric_save_csv2dataset.py` use `bfabric.operations.dataset.create_dataset`; API error handling centralized in `@use_client` (dropped `@logger.catch`); `--config-env` naming unified; `lxml` now an explicit dep (was transitive via optional `zeep`); requires `bfabric[transfer]>=1.20`; adds `questionary`-based `cli.interactive` helpers and `config_writer.remove_environment_from_config`.
 
 ## \[1.15.0\] - 2026-04-20
 

bfabric_scripts/pyproject.toml

@@ -12,6 +12,7 @@ dependencies = [
     "bfabric[transfer]>=1.20.0rc2,<2",
     "cyclopts>=4.0,<5.0",
     "Flask>=3.0.3,<4.0",
+    "questionary>=2.0,<3",
     "rich>=13.0,<15",
     "lxml>=5.0,<7.0",
     "xmltodict>=1.0.0,<2.0.0",
@@ -35,7 +36,7 @@ excel = [
 ]
 
 [project.scripts]
-"bfabric-cli" = "bfabric_scripts.cli.__main__:app"
+"bfabric-cli" = "bfabric_scripts.cli.__main__:main"
 
 # TODO scripts for feeder
 "bfabric_list_not_existing_storage_directories.py" = "bfabric_scripts.bfabric_list_not_existing_storage_directories:app"

bfabric_scripts/src/bfabric_scripts/cli/__main__.py

@@ -2,6 +2,7 @@
 
 import cyclopts
 
+from bfabric.utils.cli_integration import setup_script_logging
 from bfabric_scripts.cli.cli_api import cmd_api
 from bfabric_scripts.cli.cli_dataset import cmd_dataset
 from bfabric_scripts.cli.cli_executable import cmd_executable
@@ -23,5 +24,17 @@
 # TODO delete after transitory release
 _ = app.command(_app_external_job, name="external-job")
 
-if __name__ == "__main__":
+
+def main() -> None:
+    """CLI entry point: configure logging once, then dispatch.
+
+    Commands using ``@use_client`` set logging up themselves, but those that don't (e.g. ``auth``)
+    would otherwise run under loguru's default DEBUG handler. Setting up here covers every command;
+    the per-command call is idempotent.
+    """
+    setup_script_logging()
     app()
+
+
+if __name__ == "__main__":
+    main()

bfabric_scripts/src/bfabric_scripts/cli/cli_auth.py

@@ -1,13 +1,10 @@
 import cyclopts
 
-from bfabric_scripts.cli.login.default_config import cmd_auth_default
-from bfabric_scripts.cli.login.device_code import cmd_login_device_code
-from bfabric_scripts.cli.login.logout import cmd_login_logout
+from bfabric_scripts.cli.login.manage import cmd_auth_default, cmd_auth_list, cmd_login_logout, cmd_login_status
+from bfabric_scripts.cli.login.oauth_login import cmd_auth_login, cmd_login_device_code
 from bfabric_scripts.cli.login.pat import cmd_login_pat
-from bfabric_scripts.cli.login.pkce import cmd_auth_login
 from bfabric_scripts.cli.login.register import cmd_login_register
 from bfabric_scripts.cli.login.register_webapp import cmd_login_register_webapp
-from bfabric_scripts.cli.login.status import cmd_login_status
 
 cmd_auth = cyclopts.App(help="Authentication commands for B-Fabric.")
 _ = cmd_auth.command(cmd_login_pat, name="pat")
@@ -18,3 +15,4 @@
 _ = cmd_auth.command(cmd_login_status, name="status")
 _ = cmd_auth.command(cmd_login_logout, name="logout")
 _ = cmd_auth.command(cmd_auth_default, name="default")
+_ = cmd_auth.command(cmd_auth_list, name="list")

bfabric_scripts/src/bfabric_scripts/cli/interactive.py

@@ -0,0 +1,61 @@
+"""Interactive prompt helpers built on ``questionary``.
+
+Prompts require a TTY; callers guard with :func:`is_interactive` and handle the ``None``
+returned on cancel (or empty input, where noted).
+"""
+
+from __future__ import annotations
+
+import sys
+from collections.abc import Callable, Sequence
+from typing import cast
+
+import questionary
+
+
+def is_interactive() -> bool:
+    """Whether both stdin and stdout are attached to a terminal."""
+    return sys.stdin.isatty() and sys.stdout.isatty()
+
+
+def select_choice(
+    message: str,
+    choices: Sequence[str],
+    *,
+    default: str | None = None,
+    describe: Callable[[str], str] | None = None,
+    search: bool = False,
+) -> str | None:
+    """Arrow-key menu over *choices*; returns the picked value or ``None`` on cancel.
+
+    *default* must be one of *choices*. *describe* maps a value to its display label. *search*
+    lets the user type to filter.
+    """
+    items: list[str | questionary.Choice]
+    if describe is not None:
+        items = [questionary.Choice(title=describe(choice), value=choice) for choice in choices]
+    else:
+        items = list(choices)
+    return cast(
+        "str | None",
+        questionary.select(message, choices=items, default=default, use_search_filter=search, use_jk_keys=False).ask(),
+    )
+
+
+def select_or_input(message: str, choices: Sequence[str], *, default: str | None = None) -> str | None:
+    """Autocomplete over *choices* that also accepts a value not in the list; ``None`` on cancel/empty."""
+    items = list(choices)
+    prompt = f"{message} (Tab to autocomplete)" if items else message
+    answer = cast("str | None", questionary.autocomplete(prompt, choices=items, default=default or "").ask())
+    return answer or None
+
+
+def text_input(message: str, *, default: str = "") -> str | None:
+    """Free-text prompt prefilled with *default*; ``None`` on cancel/empty."""
+    answer = cast("str | None", questionary.text(message, default=default).ask())
+    return answer or None
+
+
+def confirm(message: str, *, default: bool = False) -> bool | None:
+    """Yes/no prompt; ``True``/``False``, or ``None`` on cancel (distinct from a declined "no")."""
+    return cast("bool | None", questionary.confirm(message, default=default).ask())