From 4a843b0c4820930e6700b006dbcd7dc61b4d7c45 Mon Sep 17 00:00:00 2001 From: Adrianno Esnarriaga Date: Thu, 21 May 2026 16:46:21 -0300 Subject: [PATCH 1/2] perf(cli): avoid duplicate load_session in auth status (#146) --- packages/cli/src/pipefy_cli/auth.py | 86 +++++++++++++------- packages/cli/src/pipefy_cli/commands/auth.py | 62 +++++++++++--- packages/cli/tests/test_auth.py | 18 ++++ packages/cli/tests/test_auth_status.py | 71 +++++++++++++--- 4 files changed, 185 insertions(+), 52 deletions(-) diff --git a/packages/cli/src/pipefy_cli/auth.py b/packages/cli/src/pipefy_cli/auth.py index 107e4ea3..a265188e 100644 --- a/packages/cli/src/pipefy_cli/auth.py +++ b/packages/cli/src/pipefy_cli/auth.py @@ -16,6 +16,7 @@ from __future__ import annotations +import time from dataclasses import dataclass from typing import Literal, NoReturn @@ -32,7 +33,14 @@ describe_missing_oauth_vars, ensure_public_graphql_configured, ) -from pipefy_cli.oauth import RefreshError, ensure_fresh_session, load_session +from pipefy_cli.oauth import ( + RefreshError, + StoredSession, + ensure_fresh_session, + load_session, +) + +_SESSION_FRESHNESS_LEEWAY_S = 60 AuthSource = Literal[ "flag-token", @@ -108,9 +116,19 @@ def _cache_key( ) +def _session_is_fresh( + session: StoredSession, *, leeway_s: int = _SESSION_FRESHNESS_LEEWAY_S +) -> bool: + """Return whether the access token is outside the refresh leeway window.""" + expires_in = session.expires_in or 0 + return time.time() < session.obtained_at + expires_in - leeway_s + + def detect_all_sources( pipefy_settings: PipefySettings, auth: AuthContext, + *, + cached_stored_session: bool | None = None, ) -> list[AuthSource]: """Return every configured credential source, highest-precedence first. @@ -125,15 +143,18 @@ def detect_all_sources( ) if not describe_missing_oauth_vars(pipefy_settings): sources.append("service-account") - if ( - auth.oidc_client is not None - and load_session( - issuer=auth.oidc_client.issuer_url, - client_id=auth.oidc_client.client_id, + if auth.oidc_client is not None: + has_stored = ( + cached_stored_session + if cached_stored_session is not None + else load_session( + issuer=auth.oidc_client.issuer_url, + client_id=auth.oidc_client.client_id, + ) + is not None ) - is not None - ): - sources.append("stored-session") + if has_stored: + sources.append("stored-session") return sources @@ -169,6 +190,8 @@ def _raise_internal_error(detail: str) -> NoReturn: def get_authenticated_client( pipefy_settings: PipefySettings, auth: AuthContext, + *, + prefetched_session: StoredSession | None = None, ) -> PipefyClient: """Return a facade client using the highest-precedence available auth source. @@ -179,6 +202,9 @@ def get_authenticated_client( tier 4 (stored user session keyed by issuer + client id). Tier 3 (service-account client credentials) is resolved from ``pipefy_settings`` alone. + prefetched_session: When set (e.g. by ``pipefy auth status`` after a + single keychain read + refresh), skips a second + ``ensure_fresh_session`` round-trip for the stored-session branch. Returns: Shared in-process instance when settings match a prior call; otherwise @@ -196,7 +222,10 @@ def get_authenticated_client( typer.echo(str(exc), err=True) raise typer.Exit(2) from exc - source = detect_auth_source(pipefy_settings, auth) + if prefetched_session is not None: + source: AuthSource = "stored-session" + else: + source = detect_auth_source(pipefy_settings, auth) if source == "none": typer.echo(_missing_auth_message(pipefy_settings), err=True) @@ -216,24 +245,25 @@ def get_authenticated_client( _raise_internal_error( "auth source 'stored-session' detected but no OIDC client is configured." ) - try: - session = ensure_fresh_session( - issuer=auth.oidc_client.issuer_url, - client_id=auth.oidc_client.client_id, - ) - except RefreshError as exc: - typer.echo( - f"Stored Pipefy session could not be refreshed: {exc}. " - "Run `pipefy auth login` to sign in again.", - err=True, - ) - raise typer.Exit(2) from exc - # ``detect_auth_source`` already confirmed a session is present; if it - # vanished between the two reads, fall through to the missing-auth path. - if session is None: - typer.echo(_missing_auth_message(pipefy_settings), err=True) - raise typer.Exit(2) - effective_bearer = session.access_token + if prefetched_session is not None: + effective_bearer = prefetched_session.access_token + else: + try: + session = ensure_fresh_session( + issuer=auth.oidc_client.issuer_url, + client_id=auth.oidc_client.client_id, + ) + except RefreshError as exc: + typer.echo( + f"Stored Pipefy session could not be refreshed: {exc}. " + "Run `pipefy auth login` to sign in again.", + err=True, + ) + raise typer.Exit(2) from exc + if session is None: + typer.echo(_missing_auth_message(pipefy_settings), err=True) + raise typer.Exit(2) + effective_bearer = session.access_token key = _cache_key(pipefy_settings, effective_bearer) if _cached_client is not None and _cached_signature == key: diff --git a/packages/cli/src/pipefy_cli/commands/auth.py b/packages/cli/src/pipefy_cli/commands/auth.py index 13ccda43..b8393dd6 100644 --- a/packages/cli/src/pipefy_cli/commands/auth.py +++ b/packages/cli/src/pipefy_cli/commands/auth.py @@ -22,6 +22,7 @@ AuthContext, AuthSource, OidcClient, + _session_is_fresh, detect_all_sources, get_authenticated_client, ) @@ -30,6 +31,7 @@ DiscoveryPolicy, LoginError, RefreshError, + StoredSession, ensure_fresh_session, keychain_backend_name, load_session, @@ -319,14 +321,25 @@ class _StatusExit(Exception): stderr: str | None = None -def _populate_stored_session(report: AuthStatusReport, oidc: OidcClient) -> None: - """Populate stored-session fields on ``report``; raise ``_StatusExit`` on failure.""" +def _populate_stored_session( + report: AuthStatusReport, + oidc: OidcClient, + *, + loaded_session: StoredSession | None = None, +) -> StoredSession: + """Populate stored-session fields on ``report``; raise ``_StatusExit`` on failure. + + Returns the (possibly refreshed) session for reuse by :func:`_fetch_identity`. + """ report.issuer = oidc.issuer_url report.keychain_backend = keychain_backend_name() try: - fresh_session = ensure_fresh_session( - issuer=oidc.issuer_url, client_id=oidc.client_id - ) + if loaded_session is not None and _session_is_fresh(loaded_session): + fresh_session = loaded_session + else: + fresh_session = ensure_fresh_session( + issuer=oidc.issuer_url, client_id=oidc.client_id + ) except RefreshError as exc: # Branch on the RFC 6749 ``error`` field, not on ``str(exc)``. The # message text is for humans; ``error_code`` is the machine contract. @@ -334,7 +347,11 @@ def _populate_stored_session(report: AuthStatusReport, oidc: OidcClient) -> None "refresh-expired" if exc.error_code == "invalid_grant" else "needs-login" ) # Best-effort expiry from the pre-refresh blob so users see *why*. - stale = load_session(issuer=oidc.issuer_url, client_id=oidc.client_id) + stale = ( + loaded_session + if loaded_session is not None + else load_session(issuer=oidc.issuer_url, client_id=oidc.client_id) + ) if stale is not None: report.access_expires_at = _iso_expiry(stale.obtained_at, stale.expires_in) report.refresh_expires_at = _iso_expiry( @@ -363,13 +380,20 @@ def _populate_stored_session(report: AuthStatusReport, oidc: OidcClient) -> None report.refresh_expires_at = _iso_expiry( fresh_session.obtained_at, fresh_session.refresh_expires_in ) + return fresh_session def _fetch_identity( - report: AuthStatusReport, settings: PipefySettings, auth: AuthContext + report: AuthStatusReport, + settings: PipefySettings, + auth: AuthContext, + *, + prefetched_session: StoredSession | None = None, ) -> None: """Populate ``report.identity``; raise ``_StatusExit`` on transport / 401.""" - client = get_authenticated_client(settings, auth) + client = get_authenticated_client( + settings, auth, prefetched_session=prefetched_session + ) try: report.identity = asyncio.run(client.get_me()) except TransportServerError as exc: @@ -399,7 +423,19 @@ def auth_status( ) -> None: """Print which auth source is active, the authenticated identity, and session expiry.""" settings, auth = settings_and_auth_from_ctx(ctx) - detected = detect_all_sources(settings, auth) + loaded_session: StoredSession | None = None + if auth.oidc_client is not None: + loaded_session = load_session( + issuer=auth.oidc_client.issuer_url, + client_id=auth.oidc_client.client_id, + ) + detected = detect_all_sources( + settings, + auth, + cached_stored_session=loaded_session is not None + if auth.oidc_client is not None + else None, + ) source: AuthSource = detected[0] if detected else "none" report = AuthStatusReport(auth_source=source, detected_sources=detected) # Surface masking env vars whenever a stored session exists — that's the @@ -421,8 +457,12 @@ def auth_status( "but no OIDC client is configured. Please file an issue." ), ) - _populate_stored_session(report, auth.oidc_client) - _fetch_identity(report, settings, auth) + prefetched = _populate_stored_session( + report, auth.oidc_client, loaded_session=loaded_session + ) + _fetch_identity(report, settings, auth, prefetched_session=prefetched) + else: + _fetch_identity(report, settings, auth) except _StatusExit as exit_: _render(exit_.report, json_out=json_out) if exit_.stderr and not json_out: diff --git a/packages/cli/tests/test_auth.py b/packages/cli/tests/test_auth.py index 30055878..8a01cc5d 100644 --- a/packages/cli/tests/test_auth.py +++ b/packages/cli/tests/test_auth.py @@ -183,6 +183,24 @@ def test_oauth_client_creds_wins_over_stored_session(clean_pipefy_env): mock_ensure.assert_not_called() +def test_prefetched_session_skips_ensure_fresh_session(clean_pipefy_env): + """``prefetched_session`` bypasses a second keychain read on the stored-session path.""" + settings = _public_only_settings() + session = _fresh_stored_session() + with ( + patch("pipefy_cli.auth.PipefyClient") as mock_pc, + patch("pipefy_cli.auth.ensure_fresh_session") as mock_ensure, + ): + mock_pc.return_value = MagicMock() + get_authenticated_client( + settings, + _auth(issuer_url=_ISSUER, client_id="pipefy-cli"), + prefetched_session=session, + ) + mock_ensure.assert_not_called() + mock_pc.assert_called_once_with(settings, bearer_token=session.access_token) + + def test_stored_session_used_when_no_other_source(clean_pipefy_env): """Priority 4 activates when bearer absent AND OAuth triple incomplete.""" settings = _public_only_settings() diff --git a/packages/cli/tests/test_auth_status.py b/packages/cli/tests/test_auth_status.py index 8795495d..7aa54469 100644 --- a/packages/cli/tests/test_auth_status.py +++ b/packages/cli/tests/test_auth_status.py @@ -77,6 +77,42 @@ def _invoke_status(runner: Any, args: list[str] | None = None) -> Any: return runner.invoke(app, ["auth", "status", *(args or [])]) +_LOAD_SESSION_PATCH_TARGETS = ( + "pipefy_cli.oauth.storage.load_session", + "pipefy_cli.oauth.refresh.load_session", + "pipefy_cli.commands.auth.load_session", + "pipefy_cli.auth.load_session", +) + + +def test_auth_status_stored_session_calls_load_session_once( + clean_pipefy_env, + saved_cwd, + monkeypatch, + runner, + fake_keyring, +): + """``auth status`` must not re-read the keychain after the initial preload.""" + _set_auth_env(monkeypatch) + _seed_session(monkeypatch) + load_calls = 0 + real_load = storage.load_session + + def counting_load(**kwargs: object) -> StoredSession | None: + nonlocal load_calls + load_calls += 1 + return real_load(**kwargs) + + for target in _LOAD_SESSION_PATCH_TARGETS: + monkeypatch.setattr(target, counting_load) + client = _mock_client_with_me() + with patch("pipefy_cli.auth.PipefyClient", return_value=client): + result = _invoke_status(runner, ["--json"]) + + assert result.exit_code == 0, (result.stdout or "") + (result.stderr or "") + assert load_calls == 1 + + # --------------------------------------------------------------------------- # # Scenario 1: stored-session, fresh access token # # --------------------------------------------------------------------------- # @@ -161,11 +197,14 @@ def test_status_refresh_expired_exits_2( ): _set_auth_env(monkeypatch) _seed_session(monkeypatch) - with patch( - "pipefy_cli.commands.auth.ensure_fresh_session", - side_effect=RefreshError( - "Refresh failed (HTTP 400): invalid_grant", error_code="invalid_grant" + with ( + patch( + "pipefy_cli.commands.auth.ensure_fresh_session", + side_effect=RefreshError( + "Refresh failed (HTTP 400): invalid_grant", error_code="invalid_grant" + ), ), + patch("pipefy_cli.commands.auth._session_is_fresh", return_value=False), ): result = _invoke_status(runner, ["--json"]) @@ -182,11 +221,14 @@ def test_status_refresh_expired_text_includes_relogin_hint( ): _set_auth_env(monkeypatch) _seed_session(monkeypatch) - with patch( - "pipefy_cli.commands.auth.ensure_fresh_session", - side_effect=RefreshError( - "Refresh failed (HTTP 400): invalid_grant", error_code="invalid_grant" + with ( + patch( + "pipefy_cli.commands.auth.ensure_fresh_session", + side_effect=RefreshError( + "Refresh failed (HTTP 400): invalid_grant", error_code="invalid_grant" + ), ), + patch("pipefy_cli.commands.auth._session_is_fresh", return_value=False), ): result = _invoke_status(runner) @@ -205,12 +247,15 @@ def test_status_other_refresh_error_categorized_as_needs_login( """ _set_auth_env(monkeypatch) _seed_session(monkeypatch) - with patch( - "pipefy_cli.commands.auth.ensure_fresh_session", - side_effect=RefreshError( - "Refresh failed (HTTP 400): unauthorized_client (mentions invalid_grant in prose)", - error_code="unauthorized_client", + with ( + patch( + "pipefy_cli.commands.auth.ensure_fresh_session", + side_effect=RefreshError( + "Refresh failed (HTTP 400): unauthorized_client (mentions invalid_grant in prose)", + error_code="unauthorized_client", + ), ), + patch("pipefy_cli.commands.auth._session_is_fresh", return_value=False), ): result = _invoke_status(runner, ["--json"]) From 7224cba380fc32b842a263b42704e6b29a23bf09 Mon Sep 17 00:00:00 2001 From: Adrianno Esnarriaga Date: Thu, 21 May 2026 16:53:59 -0300 Subject: [PATCH 2/2] fix(cli): honor auth precedence with prefetched_session (#146) Always run detect_auth_source before using prefetched_session so bearer and service-account tiers are not bypassed; pass cached_stored_session into detection to preserve the single load_session contract for auth status. --- packages/cli/src/pipefy_cli/auth.py | 15 ++++++++++----- packages/cli/tests/test_auth.py | 22 ++++++++++++++++++++++ 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/pipefy_cli/auth.py b/packages/cli/src/pipefy_cli/auth.py index a265188e..f30c9dd2 100644 --- a/packages/cli/src/pipefy_cli/auth.py +++ b/packages/cli/src/pipefy_cli/auth.py @@ -161,9 +161,13 @@ def detect_all_sources( def detect_auth_source( pipefy_settings: PipefySettings, auth: AuthContext, + *, + cached_stored_session: bool | None = None, ) -> AuthSource: """Return the precedence winner among configured sources, or ``"none"``.""" - sources = detect_all_sources(pipefy_settings, auth) + sources = detect_all_sources( + pipefy_settings, auth, cached_stored_session=cached_stored_session + ) return sources[0] if sources else "none" @@ -222,10 +226,11 @@ def get_authenticated_client( typer.echo(str(exc), err=True) raise typer.Exit(2) from exc - if prefetched_session is not None: - source: AuthSource = "stored-session" - else: - source = detect_auth_source(pipefy_settings, auth) + source = detect_auth_source( + pipefy_settings, + auth, + cached_stored_session=True if prefetched_session is not None else None, + ) if source == "none": typer.echo(_missing_auth_message(pipefy_settings), err=True) diff --git a/packages/cli/tests/test_auth.py b/packages/cli/tests/test_auth.py index 8a01cc5d..7b4a7e19 100644 --- a/packages/cli/tests/test_auth.py +++ b/packages/cli/tests/test_auth.py @@ -201,6 +201,28 @@ def test_prefetched_session_skips_ensure_fresh_session(clean_pipefy_env): mock_pc.assert_called_once_with(settings, bearer_token=session.access_token) +def test_prefetched_session_does_not_override_bearer_precedence(clean_pipefy_env): + """``prefetched_session`` must not bypass tiers 1/2 even when supplied.""" + settings = _public_only_settings() + session = _fresh_stored_session() + with ( + patch("pipefy_cli.auth.PipefyClient") as mock_pc, + patch("pipefy_cli.auth.ensure_fresh_session") as mock_ensure, + ): + mock_pc.return_value = MagicMock() + get_authenticated_client( + settings, + _auth( + bearer_token="explicit-bearer", + issuer_url=_ISSUER, + client_id="pipefy-cli", + ), + prefetched_session=session, + ) + mock_pc.assert_called_once_with(settings, bearer_token="explicit-bearer") + mock_ensure.assert_not_called() + + def test_stored_session_used_when_no_other_source(clean_pipefy_env): """Priority 4 activates when bearer absent AND OAuth triple incomplete.""" settings = _public_only_settings()