Improve bfabric-cli auth command group#558
Merged
Merged
Conversation
Collapse `auth default set [ENV]` into the bare `auth default`: with no argument it opens an arrow-key picker (type to filter, Enter to select), each row showing the environment's host and auth method. A positional `ENV` still sets the default non-interactively, and with no TTY the command lists the environments instead of prompting. Add a small reusable `cli.interactive` helper around questionary (`resolve_choice` / `select_choice` / `select_or_input`) covering the value-on-CLI / menu / type-a-new-value pattern for future commands.
…or auth login The `auth pkce` / `auth device-code` / `auth pat` commands no longer hardcode `config_env="PRODUCTION"`. When `--config-env` is omitted they prompt to pick an existing environment or type a new name (prefilled with the current default, Tab to autocomplete); non-interactively they target the current default env, else PRODUCTION. `auth pkce` / `auth device-code` gain named `--scope` presets — `read-only`, `read-write`, `read-write-upload` (adds `tus`) — that expand to the real scope string; a raw scope string still passes through. Omitting `--scope` opens a picker of the presets plus a Custom option that prompts for raw scopes. Shared resolution and the presets live in cli/login/_common.py so the three commands don't duplicate them; cli.interactive gains a text_input helper and a Tab-autocomplete hint on select_or_input.
…tus, logout removal, set-default prompt) - auth list: list configured environments (host + auth method), marking the default - auth status: for OAuth, report cached-token freshness and the granted scope, annotated with the matching named preset - auth logout: rework into "remove environment" — interactive picker (preselecting the current default), deletes the config entry and any cached OAuth tokens, guarded by a confirmation prompt (--no-confirm to skip; required to remove non-interactively) - login (pkce/device-code/pat): unless --set-default/--no-set-default is given, ask (preselected yes) whether the new environment should become the config default - consolidate the shared auth-group rendering/resolution into cli/login/_common.py; add cli.interactive.confirm and config_writer.remove_environment_from_config (also clears a dangling default so the file stays loadable)
Extract the repeated OAuth token/session boilerplate and the thrice-copied BFABRICPY_CONFIG_ENV cleanup into a login-dir conftest fixture set, and drop the redundant repeated ask()-is-Any comments from the interactive wrappers. No behavior change; login + interactive suites (83 tests) green.
…lpers - auth logout: when removing the current default while other environments remain, warn in the confirmation prompt and point to 'auth default' after, instead of silently leaving the config with no default. - auth logout: remove the config entry before clearing the token cache, so a failed write can't strand an environment without its token. - print_environments: guard the width computation against an empty mapping. Review fixes from the pre-PR readiness pass.
leoschwarz
marked this pull request as ready for review
July 15, 2026 09:55
leoschwarz
commented
Jul 15, 2026
leoschwarz
changed the title
The core bfabric OAuth entry points no longer bake in a "CLI" client ID or a default scope. connect_oauth/connect_pkce/connect_device_code and the _oauth helpers now require client_id and scope explicitly. The CLI-facing defaults move to bfabric_scripts (cli/login/_constants.py). connect() with auth_method: oauth now raises if the config lacks client_id instead of falling back to "CLI".
…it-client-id-scope # Conflicts: # bfabric/docs/changelog.md # bfabric/src/bfabric/_oauth/credential_provider.py
Model login scope presets in cli/login/_constants.py as ScopePreset named tuples (name, scope, description), oriented by use case: read-only (api:read), read-write (api:write, which implies api:read), and upload (api:write tus). `auth login` / `auth device-code` now default to the read-write preset instead of the broad OIDC-inclusive scope. Client/webapp registration keeps DEFAULT_OAUTH_SCOPE (webapps need the OIDC claims). The presets + by-name index + default live in _constants.py for the interactive scope picker to consume as a follow-up.
The CLI login default is now the read-write preset (api:write), not the broad OIDC scope, so the troubleshooting doc no longer claims groups is in the default (employees must request it explicitly for file access). Rename the CLI registration-scope constant DEFAULT_OAUTH_SCOPE -> DEFAULT_REGISTRATION_SCOPE to say what it is, and fix a stale comment in transfer/tokens.py that referenced the removed core constant.
…ng scope
The re-auth hint (_PKCE_SCOPE_HINT) and the upload examples requested the full
OIDC scope set, which the operation never needs. Reduce them to "api:write
{scope}" (api:write implies api:read) — for upload that is exactly the `upload`
preset (api:write tus). Also fix the changelog preset name (upload, not
read-write-upload).
…moval Reverts the CLI scope presets (7f15ffd), the DEFAULT_OAUTH_SCOPE rename + troubleshooting-doc edits (58d4de1), and the upload/hint scope changes (48cd276). #562 stays scoped to removing baked-in OAuth defaults from the core library; the CLI keeps its original DEFAULT_OAUTH_SCOPE / DEFAULT_CLIENT_ID and behavior unchanged. Scope presets belong in the interactive-picker follow-up.
…pt is cancelled Ctrl-C at the interactive "Set '<env>' as the default environment?" prompt made questionary's .ask() print "Cancelled by user" and return None; confirm() collapsed that None into False, so login silently proceeded as if --no-set-default were given instead of aborting. confirm() now surfaces cancellation as None (consistent with the other interactive wrappers), resolve_set_default propagates it, and the pkce / device-code / pat login commands abort with "Login aborted." before touching the OAuth flow or the config. Adds regression tests for all three commands. 🤖 Prepared with assistance from Claude Opus 4.8 via Claude Code.
leoschwarz
marked this pull request as draft
July 15, 2026 14:43
Integrates the core-defaults removal (#562) and the auth login rename (#561) into the interactive auth command group. Conflict/breakage resolution: - Relocate DEFAULT_CLIENT_ID import from the deleted bfabric._oauth._constants to bfabric_scripts.cli.login._constants (pkce, device_code, logout, status). - Apply the #561 rename: cmd_login_pkce -> cmd_auth_login (auth login); test_cmd_login_pkce.py -> test_cmd_auth_login.py; changelog auth pkce -> auth login. - Adopt #562's use-case scope presets (read-only / read-write / upload) in _constants.py (ScopePreset) and consume them from _common.py, replacing the OIDC-inclusive presets #558 had built on the (now CLI-owned) DEFAULT_OAUTH_SCOPE. - Repoint bfabric_scripts login tests at _constants; update preset expansion assertions (upload = "api:write tus"; login default = DEFAULT_LOGIN_SCOPE).
…licitly The CLI login presets (read-only / read-write / upload) are minimal API scopes and no longer include `groups` or the OIDC scopes. Update the OAuth troubleshooting doc, the transfer/tokens.py PKCE hint comment, and the bfabric_scripts changelog so employees know to request `groups` explicitly for file access.
…tration scope Move the CLI OAuth scope policy into #562 (was reverted in b4175b5 as an interactive-picker follow-up; now homed here so #562 owns scope policy and #558 owns only the interactive picker). - cli/login/_constants.py: add use-case-oriented scope presets (read-only=api:read, read-write=api:write, upload=api:write tus) with a by-name index and read-write default; rename the broad registration-scope constant DEFAULT_OAUTH_SCOPE -> DEFAULT_REGISTRATION_SCOPE to say what it is now that login differs. - auth login / auth device-code default to the minimal read-write scope (api:write) instead of the broad OIDC set; registration keeps DEFAULT_REGISTRATION_SCOPE. - Simplify upload/re-auth scope hints (_PKCE_SCOPE_HINT, upload examples/docs) to 'api:write {scope}' since api:write implies api:read.
leoschwarz
changed the base branch from
main
to
refactor/oauth-explicit-client-id-scope
July 16, 2026 07:59
…uth-default-interactive-picker # Conflicts: # bfabric/src/bfabric/transfer/tokens.py # bfabric_scripts/docs/changelog.md # bfabric_scripts/src/bfabric_scripts/cli/login/_constants.py # bfabric_scripts/src/bfabric_scripts/cli/login/device_code.py # bfabric_scripts/src/bfabric_scripts/cli/login/pkce.py
Drop the ScopePreset NamedTuple and SCOPE_PRESETS_BY_NAME index in favour of a
plain {name: scope} dict — shorter, and it removes the three same-typed positional
args that made preset construction easy to get wrong. Human-readable descriptions
are UI copy and move to the interactive picker (bfabric_scripts _common).
In register_client / register_webapp the required 'scope' sat after the defaulted 'service_user'. Legal for keyword-only args, but list the required one first for consistency; docstrings reordered to match. No call-site impact (keyword-only).
Revert the name->scope dict switch (8c089b1). The NamedTuple keeps each preset's human-readable description co-located with its name and scope — which the interactive picker (bfabric_scripts _common) consumes — instead of splitting descriptions into a parallel map. Restores ScopePreset, SCOPE_PRESETS, and SCOPE_PRESETS_BY_NAME.
…uth-default-interactive-picker
leoschwarz
marked this pull request as ready for review
July 16, 2026 11:49
No behaviour change; removes indirection that wasn't earning its keep. - Delete cli.interactive.resolve_choice: it was used fully by only one caller (auth default) and half-bypassed by resolve_config_env, which pre-checks the explicit value and the TTY itself and then called it with value=None (two of its four branches dead on that path). Both callers now use select_choice / select_or_input directly. Drops its 4 unit tests; behaviour stays covered by the auth-default and login-common suites. - Inline three single-use _common helpers into their only callers: _existing_environments -> resolve_config_env, auth_method_label -> environment_summary (also dropping a stale "mirrors auth status" note, since status keeps its own copy), environment_line -> print_environments. Net -38 production lines / -64 with tests. Tests + basedpyright green.
…LOGIN_SCOPE) Make --scope required on `auth login` / `auth device-code` and delete the DEFAULT_LOGIN_SCOPE constant, so the CLI no longer bakes in a default scope (matching the core library). Removes the [default: ...] hint from --help. SCOPE_PRESETS stays as the documented preset catalog.
…icker Reconcile the "require explicit --scope / drop DEFAULT_LOGIN_SCOPE" cleanup with this branch's interactive scope picker: - Keep --scope OPTIONAL on `auth login` / `auth device-code` so the picker runs when it is omitted in a terminal (this branch's feature), rather than making it a required argument. - Honor the dropped baked-in default: resolve_scope no longer falls back to DEFAULT_LOGIN_SCOPE non-interactively (returns None -> login aborts, so a headless run must pass --scope); the picker preselects the least-privilege preset and the Custom prompt starts empty. - Restore SCOPE_PRESETS_BY_NAME in _constants.py (still needed by the picker); DEFAULT_LOGIN_SCOPE / DEFAULT_SCOPE_PRESET stay removed. - Update the changelog + the resolve_scope test for the no-default behavior.
Same reconciliation as the previous merge: main's #562 ("require explicit client_id and scope", no CLI default) vs this branch's interactive scope picker. Keep the picker. - pkce / device_code: keep --scope OPTIONAL (picker runs when omitted); discard main's required-scope signature. - _constants: keep SCOPE_PRESETS_BY_NAME (needed by the picker); our file is main's plus that one binding. - logout / status: keep our feature imports; main's DEFAULT_CLIENT_ID relocation is already present in our version. - changelog: keep the picker + no-default entry; drop main's "require --scope" line.
The prompt kit always passes use_jk_keys=False (arrow keys are the sole navigation), but the select_choice test still asserted True. It lives under cli/ rather than cli/login/, so the login test runs never caught it.
cmd_auth_login (browser/PKCE) and cmd_login_device_code were ~90% identical: same resolve-or-abort preamble and same token-persist + config-write tail. Merge them into cli/login/oauth_login.py with two local helpers (_resolve_params, _persist) and shared cyclopts help constants. Cuts ~45 production lines and one module without loading _common (behavior and --help output unchanged).
list / default / status / logout all load the config file and act on a named environment, and duplicated the config-load block (x4) and the interactive env picker (x2). Merge the four into cli/login/manage.py with local _load_config and _select_environment helpers, and move the display helpers (environment_summary, print_environments, describe_scope, describe_token_cache) there from _common, since these commands are their only consumers. _common.py is now single-purpose (login-parameter resolution); the login package drops from 11 modules to 8 and ~48 lines. Behavior and --help output unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes environment and scope selection in
bfabric-cli authinteractive and rounds out the command group for managing configured environments. Builds on #562 (core OAuth API now requires explicitclient_id/scope); the CLI-level defaults and scope presets live inbfabric_scripts/cli/login/_constants.py.Adds
questionary, behind a reusablecli.interactivehelper (resolve_choice/select_choice/select_or_input/text_input/confirm) with non-interactive fallbacks, plusconfig_writer.remove_environment_from_config.🤖 Prepared with assistance from Claude Opus 4.8 via Claude Code.