From 4f9a1a49dcb80b324addacfdd4572e63833aa8fe Mon Sep 17 00:00:00 2001 From: igor-ctrl Date: Mon, 17 Aug 2026 09:35:42 -0500 Subject: [PATCH] fix: persist MSAL's token cache so interactive auth stops prompting hourly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BrowserAuth and DeviceCodeAuth both built msal.PublicClientApplication without a token_cache=, so MSAL kept its cache in memory only. Every CLI invocation is a fresh process, so get_accounts() always returned [] and the acquire_token_silent() call sitting in both flows was unreachable dead code. Only the ~1h access token in tokens.json was persisted, so users were pushed through a full browser or device-code round trip every time it expired. MSAL's own cache is now serialized to ~/.config/bcli/msal_cache.json via bcli.auth.MsalTokenCache, written through the existing _secure_io.write_secret_file path — atomic replace, 0600, 0700 parent — the same treatment tokens.json already gets. The refresh token survives between invocations, so renewal is silent and interactive sign-in drops to roughly once per refresh-token lifetime. A corrupt or version-skewed cache degrades to "sign in again" rather than raising. auth logout and clear_cache() on both providers clear this cache too: dropping only the access token would leave a usable refresh token on disk, so logout would not have logged the user out. auth status gained a line reporting whether silent renewal is available, since an expired access token no longer implies an interactive prompt. Client-credentials profiles are unaffected — a service principal mints tokens from its secret on demand and has no refresh token to cache. tests/conftest.py gains an autouse fixture redirecting the cache path to tmp_path, so no test can read or overwrite a developer's real credential cache. Claude-Session: https://claude.ai/code/session_01SfwmhstQdh7c9mB6w3mYUs --- CHANGELOG.md | 28 +++ docs/authentication.md | 24 ++- docs/configuration.md | 3 +- src/bcli/auth/__init__.py | 9 +- src/bcli/auth/_browser.py | 26 ++- src/bcli/auth/_device_code.py | 26 ++- src/bcli/auth/_msal_cache.py | 152 ++++++++++++++++ src/bcli/config/_defaults.py | 9 +- src/bcli_cli/commands/auth_cmd.py | 31 ++++ tests/conftest.py | 25 +++ tests/test_auth/test_browser_auth.py | 14 +- tests/test_auth/test_msal_cache.py | 194 ++++++++++++++++++++ tests/test_auth/test_silent_renewal.py | 235 +++++++++++++++++++++++++ 13 files changed, 762 insertions(+), 14 deletions(-) create mode 100644 src/bcli/auth/_msal_cache.py create mode 100644 tests/test_auth/test_msal_cache.py create mode 100644 tests/test_auth/test_silent_renewal.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a3272d..f2021bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,34 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Interactive auth no longer prompts roughly once an hour. `BrowserAuth` and + `DeviceCodeAuth` both built `msal.PublicClientApplication` without a + `token_cache=`, so MSAL's cache existed only in memory. In a fresh process — + which is every CLI invocation — `get_accounts()` returned `[]`, the + `acquire_token_silent()` call sitting in both flows was unreachable dead code, + and only the ~1h access token in `tokens.json` was persisted. The visible + effect was a full browser or device-code round trip every time that access + token expired, all day long. + + MSAL's own cache is now persisted to `~/.config/bcli/msal_cache.json` + (`bcli.auth.MsalTokenCache`), written through the existing + `_secure_io.write_secret_file` path — atomic replace, `0600`, `0700` parent — + the same treatment `tokens.json` already gets. The refresh token survives + between invocations, so silent renewal works and interactive sign-in drops to + roughly once per refresh-token lifetime. A corrupt or version-skewed cache + degrades to "sign in again" rather than raising. + + `bcli auth logout` and `clear_cache()` on both providers now clear this cache + too — dropping only the access token would have left a usable refresh token on + disk, so "logout" would not have logged the user out. `bcli auth status` gained + a line reporting whether silent renewal is available, since an expired access + token no longer implies an interactive prompt. + + Client-credentials profiles are unaffected: a service principal mints tokens + from its secret on demand and has no refresh token to cache. + ## [0.8.1] - 2026-08-11 ### Fixed diff --git a/docs/authentication.md b/docs/authentication.md index 7e6b38a..08dd045 100644 --- a/docs/authentication.md +++ b/docs/authentication.md @@ -85,14 +85,30 @@ and bcli caches the resulting delegated token. ## Token Cache -After authentication, bcli caches access tokens at -`~/.config/bcli/tokens.json`. Tokens are reused until shortly before expiry. +bcli keeps two caches, both `0600` in a `0700` directory: + +| File | Holds | Lifetime | +|------|-------|----------| +| `~/.config/bcli/tokens.json` | access tokens | ~1 hour (5-minute expiry buffer) | +| `~/.config/bcli/msal_cache.json` | MSAL's cache, incl. **refresh tokens** | until revoked or expired | + +The second one is what keeps interactive auth rare. When an access token +expires, the `browser` and `device_code` flows renew silently from the cached +refresh token instead of prompting — so you sign in about once per +refresh-token lifetime, not once an hour. ```bash -bcli auth status -bcli auth logout +bcli auth status # reports whether silent renewal is available +bcli auth logout # clears both caches — after this, the next command prompts ``` +`bcli auth logout` deliberately removes the MSAL cache too. Dropping only the +access token would leave the refresh token in place, so the next command would +renew silently and the "logout" would not have logged you out. + +Client-credentials profiles are unaffected: a service principal mints a fresh +token from its secret on demand and has no refresh token to persist. + ## Common Failures | Symptom | Likely cause | Fix | diff --git a/docs/configuration.md b/docs/configuration.md index 2da78d4..1baa77b 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -195,7 +195,8 @@ logging. | File | Purpose | |------|---------| | `~/.config/bcli/config.toml` | Main configuration | -| `~/.config/bcli/tokens.json` | Cached auth tokens | +| `~/.config/bcli/tokens.json` | Cached access tokens (~1h) | +| `~/.config/bcli/msal_cache.json` | MSAL cache incl. refresh tokens — enables silent renewal | | `~/.config/bcli/registries/*.json` | Imported custom API registries | | `~/.config/bcli/queries/*.yaml` | Saved queries | | `~/.config/bcli/audit/*.jsonl` | Per-profile audit log (when `[audit] enabled = true`) | diff --git a/src/bcli/auth/__init__.py b/src/bcli/auth/__init__.py index a56b400..7fe422f 100644 --- a/src/bcli/auth/__init__.py +++ b/src/bcli/auth/__init__.py @@ -3,6 +3,13 @@ from bcli.auth._base import AuthProvider from bcli.auth._browser import BrowserAuth from bcli.auth._credentials import ClientCredentialsAuth +from bcli.auth._msal_cache import MsalTokenCache from bcli.auth._token_cache import TokenCache -__all__ = ["AuthProvider", "BrowserAuth", "ClientCredentialsAuth", "TokenCache"] +__all__ = [ + "AuthProvider", + "BrowserAuth", + "ClientCredentialsAuth", + "MsalTokenCache", + "TokenCache", +] diff --git a/src/bcli/auth/_browser.py b/src/bcli/auth/_browser.py index b4a559b..2143cbf 100644 --- a/src/bcli/auth/_browser.py +++ b/src/bcli/auth/_browser.py @@ -19,6 +19,7 @@ import msal +from bcli.auth._msal_cache import MsalTokenCache from bcli.auth._token_cache import TokenCache from bcli.config._defaults import BC_SCOPE, ENTRA_AUTHORITY_BASE from bcli.errors import AuthError @@ -103,10 +104,12 @@ def __init__( token_cache: TokenCache | None = None, login_hint: str | None = None, incognito: bool = False, + msal_cache: MsalTokenCache | None = None, ) -> None: self._tenant_id = tenant_id self._client_id = client_id self._token_cache = token_cache or TokenCache() + self._msal_cache = msal_cache or MsalTokenCache() self._authority = f"{ENTRA_AUTHORITY_BASE}/{tenant_id}" self._login_hint = login_hint self._incognito = incognito @@ -118,13 +121,18 @@ async def get_access_token(self) -> str: if cached: return cached - # Build MSAL public client + # Build MSAL public client. Passing token_cache is what lets the silent + # path below survive process exit: MSAL's default cache is in-memory, + # so without this get_accounts() is always empty in a fresh process and + # the user gets a browser prompt every time the ~1h access token dies. app = msal.PublicClientApplication( client_id=self._client_id, authority=self._authority, + token_cache=self._msal_cache.cache, ) - # Try silent acquisition from MSAL in-memory cache + # Try silent acquisition — backed by the refresh token persisted by a + # previous invocation, not merely this process's memory. accounts = app.get_accounts() if accounts: result = app.acquire_token_silent( @@ -132,7 +140,10 @@ async def get_access_token(self) -> str: account=accounts[0], ) if result and "access_token" in result: + # A silent refresh usually rotates the refresh token; persist. + self._msal_cache.save() self._cache_token(result) + logger.info("Renewed BC API token silently (no browser)") return result["access_token"] # Start localhost server on an ephemeral port BEFORE generating the @@ -289,6 +300,7 @@ def log_message(self, format: str, *args: object) -> None: error_desc = result.get("error_description", result.get("error", "Unknown error")) raise AuthError(f"Token acquisition failed: {error_desc}", status_code=401) + self._msal_cache.save() self._cache_token(result) logger.info("Acquired BC API token via browser auth flow") return result["access_token"] @@ -300,5 +312,13 @@ def _cache_token(self, result: dict) -> None: self._token_cache.put(self._tenant_id, self._client_id, access_token, expires_in) def clear_cache(self) -> None: - """Clear cached tokens for this tenant/client.""" + """Clear cached tokens for this tenant/client. + + Clears the persisted MSAL cache too. Dropping only the access token + would leave the refresh token on disk, so a "logged out" user could + still renew silently. + """ self._token_cache.clear(self._tenant_id, self._client_id) + self._msal_cache.remove_accounts( + client_id=self._client_id, authority=self._authority + ) diff --git a/src/bcli/auth/_device_code.py b/src/bcli/auth/_device_code.py index 2f4a5d7..2e2e808 100644 --- a/src/bcli/auth/_device_code.py +++ b/src/bcli/auth/_device_code.py @@ -7,6 +7,7 @@ import msal +from bcli.auth._msal_cache import MsalTokenCache from bcli.auth._token_cache import TokenCache from bcli.config._defaults import BC_SCOPE, ENTRA_AUTHORITY_BASE from bcli.errors import AuthError @@ -26,10 +27,12 @@ def __init__( tenant_id: str, client_id: str, token_cache: TokenCache | None = None, + msal_cache: MsalTokenCache | None = None, ) -> None: self._tenant_id = tenant_id self._client_id = client_id self._token_cache = token_cache or TokenCache() + self._msal_cache = msal_cache or MsalTokenCache() self._authority = f"{ENTRA_AUTHORITY_BASE}/{tenant_id}" async def get_access_token(self) -> str: @@ -39,13 +42,18 @@ async def get_access_token(self) -> str: if cached: return cached - # Build MSAL public client (no client_secret needed) + # Build MSAL public client (no client_secret needed). token_cache is + # what makes the silent path below work at all: without it MSAL keeps + # its cache in memory, so a fresh process has no account and no refresh + # token, and every expiry costs the user another device-code prompt. app = msal.PublicClientApplication( client_id=self._client_id, authority=self._authority, + token_cache=self._msal_cache.cache, ) - # Try silent acquisition first (MSAL in-memory cache from prior flows) + # Try silent acquisition first — now backed by the refresh token + # persisted from a previous invocation, not just this process. accounts = app.get_accounts() if accounts: result = app.acquire_token_silent( @@ -53,7 +61,10 @@ async def get_access_token(self) -> str: account=accounts[0], ) if result and "access_token" in result: + # A silent refresh usually rotates the refresh token; persist. + self._msal_cache.save() self._cache_token(result) + logger.info("Renewed BC API token silently (no prompt)") return result["access_token"] # Initiate device code flow @@ -75,6 +86,7 @@ async def get_access_token(self) -> str: error_desc = result.get("error_description", result.get("error", "Unknown error")) raise AuthError(f"Device code auth failed: {error_desc}", status_code=401) + self._msal_cache.save() self._cache_token(result) logger.info("Acquired BC API token via device code flow") return result["access_token"] @@ -86,5 +98,13 @@ def _cache_token(self, result: dict) -> None: self._token_cache.put(self._tenant_id, self._client_id, access_token, expires_in) def clear_cache(self) -> None: - """Clear cached tokens for this tenant/client.""" + """Clear cached tokens for this tenant/client. + + Clears the persisted MSAL cache too. Dropping only the access token + would leave the refresh token on disk, so a "logged out" user could + still renew silently. + """ self._token_cache.clear(self._tenant_id, self._client_id) + self._msal_cache.remove_accounts( + client_id=self._client_id, authority=self._authority + ) diff --git a/src/bcli/auth/_msal_cache.py b/src/bcli/auth/_msal_cache.py new file mode 100644 index 0000000..a6b365b --- /dev/null +++ b/src/bcli/auth/_msal_cache.py @@ -0,0 +1,152 @@ +"""Persistent MSAL token cache, so refresh tokens survive between invocations. + +``BrowserAuth`` and ``DeviceCodeAuth`` both call ``acquire_token_silent()`` +before falling back to an interactive prompt. That call can only ever succeed +if MSAL has an account and a refresh token to work with — and MSAL keeps those +in a cache that is **in-memory by default**. Construct +``msal.PublicClientApplication`` without ``token_cache=`` and every new process +starts blind: ``get_accounts()`` returns ``[]``, the silent path is dead code, +and the user re-authenticates interactively as soon as the ~1h access token in +:class:`bcli.auth.TokenCache` expires. + +Persisting MSAL's own cache fixes that. The refresh token is reused silently and +the interactive prompt drops to roughly once per refresh-token lifetime. + +Security posture +---------------- +This file holds a **refresh token**, which is a longer-lived credential than +anything in ``tokens.json``. It is written through +:func:`bcli.auth._secure_io.write_secret_file` — atomic replace, ``0600``, in a +``0700`` parent — the same path the access-token cache already uses. That is a +deliberate consistency choice rather than a new mechanism. + +We do **not** reach for ``msal_extensions`` (OS-keychain-backed persistence). +It would encrypt at rest on macOS/Windows, but it is not currently a bcli +dependency, its persistence backends vary by platform, and on headless Linux it +degrades to a plain file anyway. Adding it is a defensible follow-up; it is not +required to fix the re-auth defect and would widen the dependency surface of a +tool that ships to a managed fleet. +""" + +from __future__ import annotations + +import logging +from pathlib import Path + +import msal + +from bcli.auth._secure_io import warn_if_insecure_perms, write_secret_file +from bcli.config._defaults import MSAL_CACHE_FILE + +logger = logging.getLogger(__name__) + + +class MsalTokenCache: + """Disk-backed wrapper around ``msal.SerializableTokenCache``. + + Pass :attr:`cache` to ``msal.PublicClientApplication(token_cache=...)``, + then call :meth:`save` after any token acquisition. ``save()`` is a no-op + when MSAL didn't change anything, so it is cheap to call unconditionally. + """ + + def __init__(self, cache_file: Path | None = None) -> None: + self._file = cache_file or MSAL_CACHE_FILE + self._cache: msal.SerializableTokenCache | None = None + + @property + def cache(self) -> msal.SerializableTokenCache: + """The live MSAL cache, deserialized from disk on first access.""" + if self._cache is None: + self._cache = self._load() + return self._cache + + def _load(self) -> msal.SerializableTokenCache: + cache = msal.SerializableTokenCache() + if not self._file.is_file(): + return cache + + warn_if_insecure_perms(self._file) + try: + cache.deserialize(self._file.read_text(encoding="utf-8")) + except Exception as exc: + # A truncated, hand-edited or version-skewed cache must degrade to + # "you need to sign in again", never to a traceback on every + # command. Deliberately broad: MSAL does not document a single + # exception type for deserialize failures. + logger.debug("Ignoring unreadable MSAL cache %s: %s", self._file, exc) + return cache + + def save(self) -> bool: + """Persist the cache if MSAL mutated it. Returns True if written.""" + cache = self.cache + if not cache.has_state_changed: + return False + + write_secret_file(self._file, cache.serialize()) + # MSAL clears this flag only inside its own persistence helpers, so we + # clear it here — otherwise every later save() rewrites the same bytes. + cache.has_state_changed = False + return True + + def clear(self) -> None: + """Delete the persisted cache and reset the in-memory copy.""" + self._cache = msal.SerializableTokenCache() + try: + self._file.unlink() + except FileNotFoundError: + pass + except OSError as exc: + logger.debug("Could not remove MSAL cache %s: %s", self._file, exc) + + def remove_accounts(self, *, client_id: str, authority: str) -> int: + """Sign out every cached account for this client, and persist. + + Used by ``bcli auth logout``. Dropping the access token alone would + leave the refresh token on disk, so a "logged out" user could keep + minting tokens silently — this closes that. + + Returns the number of accounts removed. On any failure we fall back to + deleting the whole cache file: over-logging-out is the safe direction. + """ + try: + app = msal.PublicClientApplication( + client_id=client_id, + authority=authority, + token_cache=self.cache, + # Keep logout usable offline. Instance discovery would other- + # wise reach login.microsoftonline.com just to forget a local + # credential. + instance_discovery=False, + ) + accounts = app.get_accounts() + for account in accounts: + app.remove_account(account) + + if accounts: + # remove_account() mutates the cache but does not reliably set + # has_state_changed, so write unconditionally here. + write_secret_file(self._file, self.cache.serialize()) + self.cache.has_state_changed = False + return len(accounts) + except Exception as exc: + logger.debug( + "Falling back to deleting %s after remove_accounts failed: %s", + self._file, + exc, + ) + self.clear() + return 0 + + def has_accounts(self, *, client_id: str, authority: str) -> bool: + """True if a silent refresh could plausibly succeed. Best-effort.""" + try: + app = msal.PublicClientApplication( + client_id=client_id, + authority=authority, + token_cache=self.cache, + instance_discovery=False, + ) + return bool(app.get_accounts()) + except Exception as exc: + logger.debug("Could not inspect MSAL cache: %s", exc) + return False diff --git a/src/bcli/config/_defaults.py b/src/bcli/config/_defaults.py index 73430cb..29dfcf5 100644 --- a/src/bcli/config/_defaults.py +++ b/src/bcli/config/_defaults.py @@ -22,9 +22,16 @@ # Config file CONFIG_FILE = CONFIG_DIR / "config.toml" -# Token cache +# Token cache — bcli's own store, holds only raw access tokens (~1h TTL). TOKEN_CACHE_FILE = CONFIG_DIR / "tokens.json" +# MSAL's own serialized cache. Separate file, and deliberately so: this one +# holds refresh tokens, which outlive access tokens by a long way and are what +# make silent (non-interactive) renewal possible across CLI invocations. Kept +# apart from TOKEN_CACHE_FILE so `bcli auth logout` can reason about the two +# independently, and so an operator can delete one without nuking the other. +MSAL_CACHE_FILE = CONFIG_DIR / "msal_cache.json" + # Stable per-laptop installation id used as a low-cardinality dimension # on telemetry events. Generated once on first emission, then reused. The # file is plaintext; it carries no PII (it's a random UUID), so the goal diff --git a/src/bcli_cli/commands/auth_cmd.py b/src/bcli_cli/commands/auth_cmd.py index 492bcb7..c284181 100644 --- a/src/bcli_cli/commands/auth_cmd.py +++ b/src/bcli_cli/commands/auth_cmd.py @@ -9,7 +9,9 @@ from rich.prompt import Prompt from bcli.auth._credentials import ClientCredentialsAuth +from bcli.auth._msal_cache import MsalTokenCache from bcli.auth._token_cache import TokenCache +from bcli.config._defaults import ENTRA_AUTHORITY_BASE from bcli_cli._state import state app = typer.Typer(no_args_is_help=True) @@ -126,6 +128,21 @@ def status() -> None: else: console.print("[yellow]No valid cached token.[/yellow] Run 'bcli auth login'.") + # An expired access token is not the same as "you must sign in again" — if a + # refresh token is persisted, the next command renews without a prompt. Say + # so, otherwise the line above reads as more alarming than it is. + if profile.auth_method in ("browser", "device_code"): + has_account = MsalTokenCache().has_accounts( + client_id=profile.client_id or "", + authority=f"{ENTRA_AUTHORITY_BASE}/{profile.tenant_id}", + ) + if has_account: + console.print(" Silent renewal: [green]available[/green] (refresh token cached)") + else: + console.print( + " Silent renewal: [dim]unavailable[/dim] — next command prompts interactively" + ) + # Show keychain status if ClientCredentialsAuth.has_keyring(): from bcli.auth._credentials import _try_keyring_get, KEYRING_SERVICE @@ -146,7 +163,21 @@ def logout() -> None: profile = state.profile cache = TokenCache() cache.clear(profile.tenant_id, profile.client_id) + + # Also forget the persisted MSAL cache. Clearing only the access token + # above would leave the refresh token on disk, so the next command would + # renew silently and "logout" would be a lie. + removed = MsalTokenCache().remove_accounts( + client_id=profile.client_id or "", + authority=f"{ENTRA_AUTHORITY_BASE}/{profile.tenant_id}", + ) + console.print(f"[green]✓[/green] Cleared tokens for profile '{state.active_profile_name}'") + if removed: + console.print( + f" [dim]Signed out {removed} cached account(s); " + f"next command will prompt.[/dim]" + ) @app.command("store-secret") diff --git a/tests/conftest.py b/tests/conftest.py index 5632edd..c898f5f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1 +1,26 @@ """Shared test fixtures.""" + +from __future__ import annotations + +import pytest + + +@pytest.fixture(autouse=True) +def _isolate_msal_cache(tmp_path, monkeypatch): + """Keep the persistent MSAL cache out of the developer's real config dir. + + ``MsalTokenCache`` defaults to ``~/.config/bcli/msal_cache.json``, which + holds a real refresh token on any machine where bcli has been used. A test + that constructs ``BrowserAuth`` / ``DeviceCodeAuth`` without an explicit + ``msal_cache`` would otherwise read — and on save, overwrite — it. + + Autouse so no future test has to remember. Pointing the module-level + default at ``tmp_path`` is enough; ``MsalTokenCache`` resolves it lazily + per instance. + """ + import bcli.auth._msal_cache as msal_cache_mod + + monkeypatch.setattr( + msal_cache_mod, "MSAL_CACHE_FILE", tmp_path / "msal_cache.json" + ) + yield diff --git a/tests/test_auth/test_browser_auth.py b/tests/test_auth/test_browser_auth.py index 2362ef2..9de6eca 100644 --- a/tests/test_auth/test_browser_auth.py +++ b/tests/test_auth/test_browser_auth.py @@ -52,9 +52,18 @@ class _FakeMSALApp: last_redirect_uri: str | None = None - def __init__(self, client_id: str | None = None, authority: str | None = None) -> None: + def __init__( + self, + client_id: str | None = None, + authority: str | None = None, + token_cache: object | None = None, + **kwargs: object, + ) -> None: self.client_id = client_id self.authority = authority + # BrowserAuth now hands MSAL a persistent cache so refresh tokens + # survive process exit. Capture it so a test can assert it was passed. + self.token_cache = token_cache def get_accounts(self) -> list: return [] @@ -93,6 +102,9 @@ def stub_msal(monkeypatch): def _make_auth() -> BrowserAuth: + # The MSAL cache path is redirected to tmp_path by the autouse + # ``_isolate_msal_cache`` fixture in tests/conftest.py, so this never + # touches the developer's real ~/.config/bcli/msal_cache.json. return BrowserAuth( tenant_id="tenant", client_id="client", diff --git a/tests/test_auth/test_msal_cache.py b/tests/test_auth/test_msal_cache.py new file mode 100644 index 0000000..758f6a8 --- /dev/null +++ b/tests/test_auth/test_msal_cache.py @@ -0,0 +1,194 @@ +"""Tests for the persistent MSAL token cache. + +Before this cache existed, ``BrowserAuth`` / ``DeviceCodeAuth`` built +``msal.PublicClientApplication`` with no ``token_cache=`` argument. MSAL then +kept its cache in memory only, so: + + * ``app.get_accounts()`` was always empty in a fresh process; + * ``acquire_token_silent()`` could therefore never return anything; + * only the raw access token was persisted (``TokenCache``, ~60-75 min TTL). + +Net effect: a full interactive re-auth roughly every hour. Persisting MSAL's +own cache keeps the refresh token, so silent renewal works across invocations. + +These tests use placeholder tenant/client identifiers only — never real ones. +""" + +from __future__ import annotations + +import json +import os +import stat + +import pytest + +from bcli.auth._msal_cache import MsalTokenCache + +TENANT = "11111111-1111-1111-1111-111111111111" +CLIENT = "22222222-2222-2222-2222-222222222222" +AUTHORITY = f"https://login.microsoftonline.com/{TENANT}" + + +# ── Round-tripping ──────────────────────────────────────────────────────── + + +def test_load_returns_empty_cache_when_file_absent(tmp_path): + store = MsalTokenCache(cache_file=tmp_path / "msal_cache.json") + # A brand-new cache has no state to serialize. + assert store.cache.serialize() in ("", "{}") + + +def test_save_is_a_noop_when_nothing_changed(tmp_path): + path = tmp_path / "msal_cache.json" + store = MsalTokenCache(cache_file=path) + + # Touching nothing means has_state_changed is False — don't write. + assert store.save() is False + assert not path.exists() + + +def test_save_writes_when_state_changed_and_reload_sees_it(tmp_path): + path = tmp_path / "msal_cache.json" + store = MsalTokenCache(cache_file=path) + + # Drive MSAL's own cache API rather than hand-rolling its JSON shape. + store.cache.add( + { + "client_id": CLIENT, + "scope": ["https://example.invalid/.default"], + "token_endpoint": f"{AUTHORITY}/oauth2/v2.0/token", + "response": { + "access_token": "placeholder-access-token", + "refresh_token": "placeholder-refresh-token", + "token_type": "Bearer", + "expires_in": 3600, + }, + } + ) + + assert store.save() is True + assert path.exists() + + # A second store pointed at the same file must see the refresh token. + reloaded = MsalTokenCache(cache_file=path) + blob = reloaded.cache.serialize() + assert "placeholder-refresh-token" in blob + + +def test_persisted_file_is_private(tmp_path): + """The cache holds a refresh token — it must not be world/group readable.""" + if os.name != "posix": + pytest.skip("POSIX permission semantics only") + + path = tmp_path / "msal_cache.json" + store = MsalTokenCache(cache_file=path) + store.cache.add( + { + "client_id": CLIENT, + "scope": ["https://example.invalid/.default"], + "token_endpoint": f"{AUTHORITY}/oauth2/v2.0/token", + "response": { + "access_token": "placeholder-access-token", + "refresh_token": "placeholder-refresh-token", + "token_type": "Bearer", + "expires_in": 3600, + }, + } + ) + store.save() + + mode = stat.S_IMODE(path.stat().st_mode) + assert mode & 0o077 == 0, f"cache file mode {oct(mode)} is group/other accessible" + + +def test_corrupt_cache_file_does_not_raise(tmp_path): + """A truncated or hand-edited cache must degrade to re-auth, not crash.""" + path = tmp_path / "msal_cache.json" + path.write_text("{not valid json", encoding="utf-8") + + store = MsalTokenCache(cache_file=path) + # Must not raise; an unusable cache is simply an empty one. + assert store.cache is not None + + +# ── Clearing ────────────────────────────────────────────────────────────── + + +def test_clear_removes_the_file(tmp_path): + path = tmp_path / "msal_cache.json" + path.write_text("{}", encoding="utf-8") + + store = MsalTokenCache(cache_file=path) + store.clear() + + assert not path.exists() + + +def test_clear_is_idempotent_when_file_missing(tmp_path): + store = MsalTokenCache(cache_file=tmp_path / "nope.json") + store.clear() # must not raise + + +def test_remove_accounts_drops_persisted_refresh_token(tmp_path): + """logout must actually invalidate locally, not just drop the access token.""" + path = tmp_path / "msal_cache.json" + store = MsalTokenCache(cache_file=path) + store.cache.add( + { + "client_id": CLIENT, + "scope": ["https://example.invalid/.default"], + "token_endpoint": f"{AUTHORITY}/oauth2/v2.0/token", + "response": { + "access_token": "placeholder-access-token", + "refresh_token": "placeholder-refresh-token", + "token_type": "Bearer", + "expires_in": 3600, + "id_token_claims": { + # MSAL requires "sub" (OIDC guarantees it) to derive the + # home account id when client_info is absent. + "sub": "44444444-4444-4444-4444-444444444444", + "preferred_username": "placeholder@example.invalid", + "oid": "33333333-3333-3333-3333-333333333333", + "tid": TENANT, + }, + }, + } + ) + store.save() + + store.remove_accounts(client_id=CLIENT, authority=AUTHORITY) + + reloaded = MsalTokenCache(cache_file=path) + assert "placeholder-refresh-token" not in reloaded.cache.serialize() + + +def test_default_path_lives_under_the_config_dir(): + from bcli.config._defaults import CONFIG_DIR, MSAL_CACHE_FILE + + assert MSAL_CACHE_FILE.parent == CONFIG_DIR + # Must not collide with the access-token cache. + from bcli.config._defaults import TOKEN_CACHE_FILE + + assert MSAL_CACHE_FILE != TOKEN_CACHE_FILE + + +def test_save_emits_valid_json(tmp_path): + """Other tooling (and our own reload) must be able to parse the artefact.""" + path = tmp_path / "msal_cache.json" + store = MsalTokenCache(cache_file=path) + store.cache.add( + { + "client_id": CLIENT, + "scope": ["https://example.invalid/.default"], + "token_endpoint": f"{AUTHORITY}/oauth2/v2.0/token", + "response": { + "access_token": "placeholder-access-token", + "refresh_token": "placeholder-refresh-token", + "token_type": "Bearer", + "expires_in": 3600, + }, + } + ) + store.save() + + json.loads(path.read_text(encoding="utf-8")) # must not raise diff --git a/tests/test_auth/test_silent_renewal.py b/tests/test_auth/test_silent_renewal.py new file mode 100644 index 0000000..d13bf82 --- /dev/null +++ b/tests/test_auth/test_silent_renewal.py @@ -0,0 +1,235 @@ +"""Regression tests for hourly re-authentication. + +The defect: ``BrowserAuth`` / ``DeviceCodeAuth`` built +``msal.PublicClientApplication`` with no ``token_cache=``. MSAL's cache was +therefore in-memory only, so in a fresh process ``get_accounts()`` returned +``[]``, ``acquire_token_silent()`` was unreachable, and the user was pushed +through a full interactive flow every time the ~1h access token in +``TokenCache`` expired. + +These tests assert the fix at the level the user actually feels it: a *second, +independently constructed* auth provider — standing in for the next CLI +invocation — renews from the persisted refresh token and never prompts. + +Placeholder identifiers only; never real tenant or client values. +""" + +from __future__ import annotations + +import asyncio + +import pytest + +import bcli.auth._browser as browser_mod +import bcli.auth._device_code as device_mod +from bcli.auth._browser import BrowserAuth +from bcli.auth._device_code import DeviceCodeAuth +from bcli.auth._msal_cache import MsalTokenCache + +TENANT = "11111111-1111-1111-1111-111111111111" +CLIENT = "22222222-2222-2222-2222-222222222222" + +_TOKEN_RESPONSE = { + "client_id": CLIENT, + "scope": ["https://example.invalid/.default"], + "token_endpoint": ( + f"https://login.microsoftonline.com/{TENANT}/oauth2/v2.0/token" + ), + "response": { + "access_token": "placeholder-interactive-token", + "refresh_token": "placeholder-refresh-token", + "token_type": "Bearer", + "expires_in": 3600, + "id_token_claims": { + "sub": "44444444-4444-4444-4444-444444444444", + "preferred_username": "placeholder@example.invalid", + "tid": TENANT, + }, + }, +} + + +class _NullAccessTokenCache: + """bcli's own access-token cache, always cold, so the MSAL path is used.""" + + def __init__(self) -> None: + self.puts: list[str] = [] + + def get(self, tenant_id: str, client_id: str) -> None: + return None + + def put(self, tenant_id: str, client_id: str, access_token: str, expires_in: int) -> None: + self.puts.append(access_token) + + def clear(self, *args: object, **kwargs: object) -> None: + pass + + +class _FakeMSALApp: + """Minimal stand-in that honours the token_cache it is handed. + + Mirrors the two MSAL behaviours under test: an account exists only if one + was persisted, and the interactive flow writes into the cache. + """ + + interactive_calls = 0 + + def __init__( + self, + client_id: str | None = None, + authority: str | None = None, + token_cache: object | None = None, + **kwargs: object, + ) -> None: + self.token_cache = token_cache + + def get_accounts(self) -> list[dict]: + if self.token_cache is None: + return [] + if "placeholder-refresh-token" in self.token_cache.serialize(): + return [{"username": "placeholder@example.invalid"}] + return [] + + def acquire_token_silent(self, scopes, account): + return {"access_token": "placeholder-silent-token", "expires_in": 3600} + + # ── interactive paths ──────────────────────────────────────────────── + def initiate_device_flow(self, scopes): + return {"user_code": "PLACEHOLDER", "message": "placeholder message"} + + def acquire_token_by_device_flow(self, flow): + return self._interactive() + + def initiate_auth_code_flow(self, scopes, redirect_uri, **kwargs): + return {"auth_uri": "http://example.invalid/auth", "state": "S"} + + def acquire_token_by_auth_code_flow(self, flow, response): + return self._interactive() + + def _interactive(self) -> dict: + type(self).interactive_calls += 1 + # Mimic MSAL populating its own cache during acquisition. + if self.token_cache is not None: + self.token_cache.add(_TOKEN_RESPONSE) + return {"access_token": "placeholder-interactive-token", "expires_in": 3600} + + +@pytest.fixture(autouse=True) +def _reset_counter(): + _FakeMSALApp.interactive_calls = 0 + yield + + +# ── Device code ─────────────────────────────────────────────────────────── + + +def test_device_code_second_invocation_renews_without_prompting(tmp_path, monkeypatch): + monkeypatch.setattr(device_mod.msal, "PublicClientApplication", _FakeMSALApp) + cache_file = tmp_path / "msal_cache.json" + + # First invocation: nothing cached, so the interactive flow runs and the + # refresh token lands on disk. + first = DeviceCodeAuth( + tenant_id=TENANT, + client_id=CLIENT, + token_cache=_NullAccessTokenCache(), + msal_cache=MsalTokenCache(cache_file=cache_file), + ) + token = asyncio.run(first.get_access_token()) + + assert token == "placeholder-interactive-token" + assert _FakeMSALApp.interactive_calls == 1 + assert cache_file.exists(), "refresh token was not persisted" + + # Second invocation — a brand-new provider, as a separate CLI run would + # build. This is the case that used to force another prompt. + second = DeviceCodeAuth( + tenant_id=TENANT, + client_id=CLIENT, + token_cache=_NullAccessTokenCache(), + msal_cache=MsalTokenCache(cache_file=cache_file), + ) + token2 = asyncio.run(second.get_access_token()) + + assert token2 == "placeholder-silent-token" + assert _FakeMSALApp.interactive_calls == 1, "second run prompted the user again" + + +# ── Browser ─────────────────────────────────────────────────────────────── + + +def test_browser_second_invocation_renews_without_opening_a_browser(tmp_path, monkeypatch): + monkeypatch.setattr(browser_mod.msal, "PublicClientApplication", _FakeMSALApp) + opened: list[str] = [] + monkeypatch.setattr( + browser_mod, "_open_browser", lambda url, **kw: opened.append(url) + ) + cache_file = tmp_path / "msal_cache.json" + + # Seed the cache as a prior interactive login would have. + seed = MsalTokenCache(cache_file=cache_file) + seed.cache.add(_TOKEN_RESPONSE) + seed.save() + + auth = BrowserAuth( + tenant_id=TENANT, + client_id=CLIENT, + token_cache=_NullAccessTokenCache(), + msal_cache=MsalTokenCache(cache_file=cache_file), + ) + token = asyncio.run(auth.get_access_token()) + + assert token == "placeholder-silent-token" + assert opened == [], "browser was opened despite a usable refresh token" + assert _FakeMSALApp.interactive_calls == 0 + + +def test_browser_passes_a_persistent_cache_to_msal(tmp_path, monkeypatch): + """The whole fix hinges on this argument being present.""" + captured: dict[str, object] = {} + + class _Capturing(_FakeMSALApp): + def __init__(self, *args, **kwargs): + captured["token_cache"] = kwargs.get("token_cache") + super().__init__(*args, **kwargs) + + monkeypatch.setattr(browser_mod.msal, "PublicClientApplication", _Capturing) + monkeypatch.setattr(browser_mod, "_open_browser", lambda *a, **kw: None) + + store = MsalTokenCache(cache_file=tmp_path / "msal_cache.json") + store.cache.add(_TOKEN_RESPONSE) + + auth = BrowserAuth( + tenant_id=TENANT, + client_id=CLIENT, + token_cache=_NullAccessTokenCache(), + msal_cache=store, + ) + asyncio.run(auth.get_access_token()) + + assert captured["token_cache"] is store.cache + + +# ── Logout must actually log out ────────────────────────────────────────── + + +def test_clear_cache_removes_the_refresh_token(tmp_path, monkeypatch): + """Clearing only the access token would leave silent renewal working.""" + monkeypatch.setattr(device_mod.msal, "PublicClientApplication", _FakeMSALApp) + cache_file = tmp_path / "msal_cache.json" + + store = MsalTokenCache(cache_file=cache_file) + store.cache.add(_TOKEN_RESPONSE) + store.save() + assert "placeholder-refresh-token" in cache_file.read_text(encoding="utf-8") + + auth = DeviceCodeAuth( + tenant_id=TENANT, + client_id=CLIENT, + token_cache=_NullAccessTokenCache(), + msal_cache=MsalTokenCache(cache_file=cache_file), + ) + auth.clear_cache() + + remaining = MsalTokenCache(cache_file=cache_file).cache.serialize() + assert "placeholder-refresh-token" not in remaining