Skip to content

feat: Add Azure CLI auth source - #265

Open
shirasassoon wants to merge 59 commits into
microsoft:mainfrom
shirasassoon:feature/azure-cli-auth-poc
Open

feat: Add Azure CLI auth source#265
shirasassoon wants to merge 59 commits into
microsoft:mainfrom
shirasassoon:feature/azure-cli-auth-poc

Conversation

@shirasassoon

@shirasassoon shirasassoon commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Summary

Add azure_cli as a new identity type that delegates token acquisition to Azure CLI via azure-identity's AzureCliCredential. This allows tools calling fab to reuse an existing az login session instead of requiring a separate interactive fab auth login.

Changes

Core auth (src/fabric_cli/core/fab_auth.py)

  • Add azure_cli to AUTH_KEYS identity type allowlist
  • Add _acquire_token_from_azure_cli() using AzureCliCredential
  • Per-command tenant drift check — compares stored tenant against current Azure CLI session via az account show
  • In-memory token cache with 60-second refresh buffer to reduce subprocess overhead
  • Tenant cache (10s TTL) to avoid repeated az account show calls during multi-scope operations
  • set_azure_cli() clears token cache on every login to prevent cross-tenant stale tokens
  • shutil.which("az") for Windows compatibility (az.cmd resolution)

Command handler (src/fabric_cli/commands/auth/fab_auth.py)

  • Add --azure-cli flag to fab auth login
  • Add "Azure CLI authentication" option to interactive login menu
  • Show auth_source in fab auth status output

Error handling (src/fabric_cli/errors/auth.py)

  • Centralized error messages for all Azure CLI auth scenarios
  • SDK exception allowlist: ClientAuthenticationError, HttpResponseError, ServiceRequestError, ServiceResponseError surfaced verbatim (pre-sanitized by azure-identity SDK)
  • All other exceptions receive a generic safe message — no token content is ever leaked

Documentation

  • docs/commands/auth/index.md--azure-cli flag in command reference with separate usage blocks per auth method
  • docs/examples/auth_examples.md — Azure CLI auth examples with tenant behavior note

Dependencies

  • Add azure-identity>=1.15.0

Tests

43 automated tests across 4 files:

File Count Coverage
test_fab_auth_azure_cli.py 34 Core auth: token acquisition, tenant drift, caching, scope handling, cache invalidation, tenant discovery, auth method isolation
test_fab_auth_parser.py 4 Parser: --azure-cli flag mapping
test_fab_msal_bridge_azure_cli.py 2 Bridge: MsalTokenCredential dispatch with azure_cli identity
test_fab_auth_command_azure_cli.py 3 Command: login/status/logout end-to-end

Key test scenarios:

  • Auth method isolation — bidirectional: user/SPN don't invoke AzureCliCredential, azure_cli doesn't invoke MSAL
  • Tenant drift detection and mismatch error
  • Token cache expiry and refresh buffer
  • Cross-tenant stale token cache clearing on login
  • Windows az.cmd resolution via shutil.which
  • Azure CLI not installed / not logged in

Security

  • Error messages sanitized via SDK exception allowlist (not denylist)
  • Auth methods are mutually exclusive — no credential path overlap
  • Per-command tenant drift check prevents wrong-tenant operations
  • Token cache cleared on every login to prevent cross-tenant reuse
  • shutil.which("az") resolves absolute path before subprocess (matches azure-identity SDK pattern)
  • shell=False for all subprocess calls — no injection risk

Open questions for team

  1. Per-command drift check vs. performance: Current behavior adds ~1s per CLI command for tenant verification. Alternative is to rely on SDK tenant pinning (no overhead) but loses the clear "tenant mismatch" error UX.
  2. az logout config persistence: When Azure CLI session is interrupted and restored with the same tenant, fab silently resumes. Should fab require explicit re-authentication instead?

Add azure_cli as a new identity type that delegates token acquisition
to Azure CLI via azure-identity's AzureCliCredential. This allows
tools calling fab to reuse an existing az login session instead of
requiring a separate interactive fab auth login.

Changes:
- Add 'azure_cli' to AUTH_KEYS identity type allow-list
- Add _acquire_token_from_azure_cli() using AzureCliCredential
- Add --azure-cli flag to fab auth login
- Add 'Azure CLI' option to interactive login menu
- Show auth_source in fab auth status output
- Add azure-identity>=1.15.0 dependency
- Add 12 unit tests covering dispatch, scopes, errors, sanitization

Security: error messages are sanitized to never leak token content.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings July 15, 2026 10:46
@shirasassoon
shirasassoon requested a review from a team as a code owner July 15, 2026 10:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a new authentication source (azure_cli) to Fabric CLI, delegating token acquisition to Azure CLI (via azure-identity’s AzureCliCredential) so fab can reuse an existing az login session. It also wires the new auth source into fab auth login (flag + interactive option) and exposes the selected auth source in fab auth status.

Changes:

  • Add azure_cli as an allowed identity type and implement Azure CLI token acquisition in FabAuth.
  • Add --azure-cli flag plus an “Azure CLI” interactive login option; include auth_source in auth status output.
  • Add azure-identity>=1.15.0 dependency and a new unit test module for Azure CLI auth.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tests/test_core/test_fab_auth_azure_cli.py Adds unit tests for the new Azure CLI auth flow, scopes, and error sanitization.
src/fabric_cli/parsers/fab_auth_parser.py Adds --azure-cli to fab auth login and updates examples.
src/fabric_cli/core/fab_constant.py Extends identity type allow-list to include azure_cli.
src/fabric_cli/core/fab_auth.py Implements set_azure_cli() and _acquire_token_from_azure_cli() and dispatches in acquire_token().
src/fabric_cli/commands/auth/fab_auth.py Wires Azure CLI auth into login flows and adds auth_source to status output.
pyproject.toml Adds the azure-identity runtime dependency required for AzureCliCredential.

Comment thread src/fabric_cli/core/fab_auth.py Outdated
Comment thread src/fabric_cli/core/fab_auth.py
Comment thread src/fabric_cli/commands/auth/fab_auth.py Outdated
Comment thread tests/test_core/test_fab_auth_azure_cli.py Outdated
Comment thread tests/test_core/test_fab_auth_azure_cli.py Outdated
Copilot AI review requested due to automatic review settings August 11, 2026 08:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (6)

tests/test_core/test_fab_auth_azure_cli.py:36

  • These fixtures are unused in this test module, and the singleton-reset logic inside them is brittle (it relies on non-existent __wrapped__ and decorator internals). After resetting the FabAuth singleton in the autouse fixture, these can be removed to keep the tests easier to maintain.
@pytest.fixture
def auth_instance(temp_dir_fixture):
    """Get a fresh FabAuth instance."""
    # Clear singleton for test isolation
    FabAuth.__wrapped__ = None  # type: ignore
    from fabric_cli.core import fab_auth as fab_auth_module

    if FabAuth in fab_auth_module.singleton.__wrapped__:  # type: ignore
        del fab_auth_module.singleton.__wrapped__[FabAuth]  # type: ignore
    return FabAuth()

tests/test_core/test_fab_auth_azure_cli.py:56

  • This fixture is unused, and it attempts to monkeypatch FabAuth.__init__.__globals__, which is not a safe or reliable way to reset the singleton (and may raise if executed). With the singleton reset handled in the autouse fixture, this block can be deleted.
@pytest.fixture
def fresh_auth(temp_dir_fixture, monkeypatch):
    """Get a fresh FabAuth instance with singleton cleared."""
    # Reset singleton instances dict
    import fabric_cli.core.fab_auth as auth_module

    # Access the closure variable of the singleton decorator
    singleton_instances = auth_module.singleton.__code__.co_consts  # noqa
    # Simpler approach: just patch the module-level reference
    monkeypatch.setattr(
        "fabric_cli.core.fab_auth.FabAuth.__init__.__globals__",
        {},
        raising=False,
    )
    # Re-instantiate
    auth = FabAuth.__new__(FabAuth)
    auth.__init__()
    return auth

src/fabric_cli/core/fab_auth.py:444

  • This line exceeds the repo’s Black line-length (88) and will be reformatted by CI (tox.toml:69-70). Please wrap the conditional instantiation so the formatted output is stable and easier to read.
        tenant_id = self.get_tenant_id()
        try:
            credential = AzureCliCredential(tenant_id=tenant_id) if tenant_id else AzureCliCredential()
            # AzureCliCredential.get_token expects scopes as positional args
            azure_token = credential.get_token(scope[0])

src/fabric_cli/core/fab_auth.py:461

  • This sanitized error message is long enough to violate Black’s 88-char line length and will be reformatted by CI (tox.toml:69-70). Splitting it across adjacent string literals keeps formatting stable.
            error_msg = str(e)
            if "accessToken" in error_msg or "token" in error_msg.lower():
                error_msg = "Azure CLI token acquisition failed. Run 'az account get-access-token' manually to diagnose."
            raise FabricCLIError(

src/fabric_cli/commands/auth/fab_auth.py:37

  • When --azure-cli is provided, other credential flags (e.g., -u/-p, --certificate, --federated-token, --identity) are silently ignored due to branch precedence. This can lead to confusing CLI behavior; please validate and fail fast on incompatible combinations.
    if getattr(args, "azure_cli", False):
        FabAuth().set_access_mode("azure_cli", args.tenant)
        FabAuth().set_azure_cli(args.tenant)
        FabAuth().get_access_token(scope=fab_constant.SCOPE_FABRIC_DEFAULT)
        FabAuth().get_access_token(scope=fab_constant.SCOPE_ONELAKE_DEFAULT)

tests/test_core/test_fab_auth_azure_cli.py:24

  • FabAuth is a singleton (see @singleton in fabric_cli.core.fab_auth). These tests patch config_location() per-test, but without clearing the singleton, FabAuth() will reuse the first instance (and its first auth/cache paths), causing state leakage across tests and making the tmp_path isolation ineffective.

This issue also appears in the following locations of the same file:

  • line 27
  • line 39
@pytest.fixture(autouse=True)
def temp_dir_fixture(monkeypatch, tmp_path):
    """Create a temporary directory and configure FabAuth to use it."""
    monkeypatch.setattr(
        "fabric_cli.core.fab_state_config.config_location", lambda: str(tmp_path)
    )

- Expand sanitization patterns (eyJ, Bearer, refresh_token, Authorization)
- Auto-capture tenant from az account show at login
- Tenant drift detection on every token acquisition
- In-memory token caching by audience with 60s expiry buffer
- Display tenant and auth mode at login and in auth status
- Add 11 new tests (23 total) for drift, caching, sanitization

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 11, 2026 08:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/fabric_cli/core/fab_auth.py:502

  • _acquire_token_from_azure_cli() calls credential.get_token(scope[0]) even though the method signature allows an empty scope list. If scope is empty, this will raise IndexError instead of a FabricCLIError, and it also ignores additional scopes if ever provided.
            credential = AzureCliCredential(tenant_id=stored_tenant) if stored_tenant else AzureCliCredential()
            # AzureCliCredential.get_token expects scopes as positional args
            azure_token = credential.get_token(scope[0])
            token_result = {
                "access_token": azure_token.token,

src/fabric_cli/commands/auth/fab_auth.py:35

  • --azure-cli is not validated as mutually exclusive with managed identity/service principal flags. Because the code checks azure_cli first, a user can accidentally pass conflicting flags and silently get Azure CLI auth instead of an error.
    if getattr(args, "azure_cli", False):
        FabAuth().set_access_mode("azure_cli", args.tenant)
        FabAuth().set_azure_cli(args.tenant)
        FabAuth().get_access_token(scope=fab_constant.SCOPE_FABRIC_DEFAULT)
        FabAuth().get_access_token(scope=fab_constant.SCOPE_ONELAKE_DEFAULT)

tests/test_core/test_fab_auth_azure_cli.py:18

  • FabAuth is a singleton (decorator returns a cached instance), but this autouse fixture only patches config_location/env vars and doesn’t clear the singleton cache. If any other test module instantiates FabAuth before this fixture runs, these tests will share state and potentially write auth/cache files outside tmp_path, causing order-dependent failures.
@pytest.fixture(autouse=True)
def temp_dir_fixture(monkeypatch, tmp_path):
    """Create a temporary directory and configure FabAuth to use it."""
    monkeypatch.setattr(
        "fabric_cli.core.fab_state_config.config_location", lambda: str(tmp_path)

src/fabric_cli/core/fab_auth.py:522

  • The generic exception handler includes the raw exception message in the CLI error unless it matches a small allow-list of substrings. That does not guarantee token material won’t leak (e.g., access tokens that don’t contain the current patterns), which contradicts the PR’s “never leak token content” claim.
        except Exception as e:
            # Sanitize: never include token content in error messages
            error_msg = str(e)
            if any(p.lower() in error_msg.lower() for p in self._SENSITIVE_PATTERNS):
                error_msg = "Azure CLI token acquisition failed. Run 'az account get-access-token' manually to diagnose."

Defer OneLake and Azure management token acquisition to first use,
matching the lazy approach. Tokens are cached in-memory after first call.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 11, 2026 09:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (5)

src/fabric_cli/core/fab_auth.py:477

  • Azure CLI auth introduces new FabricCLIError messages as hardcoded strings. Elsewhere in this module, auth failures use ErrorMessages.Auth.*() helpers for consistent wording and easier localization/maintenance. Consider moving these new messages into ErrorMessages.Auth and reusing them here.
        try:
            from azure.identity import AzureCliCredential, CredentialUnavailableError
        except ImportError:
            raise FabricCLIError(
                "Azure CLI auth requires the 'azure-identity' package. "
                "Install it with: pip install azure-identity",
                status_code=con.ERROR_AUTHENTICATION_FAILED,
            )

tests/test_core/test_fab_auth_azure_cli.py:36

  • These tests call FabAuth() directly, but FabAuth is a singleton (via the @singleton decorator). Without reliably clearing the singleton cache per test, state (auth_file paths, env-loaded tokens, tenant id) can leak between tests and make behavior depend on execution order. The current fixture attempts (__wrapped__, singleton.__wrapped__) don't match how the singleton decorator is implemented, so they won't actually reset anything.
@pytest.fixture
def auth_instance(temp_dir_fixture):
    """Get a fresh FabAuth instance."""
    # Clear singleton for test isolation
    FabAuth.__wrapped__ = None  # type: ignore
    from fabric_cli.core import fab_auth as fab_auth_module

    if FabAuth in fab_auth_module.singleton.__wrapped__:  # type: ignore
        del fab_auth_module.singleton.__wrapped__[FabAuth]  # type: ignore
    return FabAuth()

src/fabric_cli/commands/auth/fab_auth.py:39

  • fab auth login --azure-cli currently only acquires the Fabric token, while other login flows (interactive, SPN, managed identity) also acquire OneLake and Azure management tokens. This makes Azure CLI login behave differently and can leave later commands without the required secondary tokens.
    if getattr(args, "azure_cli", False):
        FabAuth().set_access_mode("azure_cli", args.tenant)
        FabAuth().set_azure_cli(args.tenant)
        FabAuth().get_access_token(scope=fab_constant.SCOPE_FABRIC_DEFAULT)
        Context().context = FabAuth().get_tenant()

src/fabric_cli/core/fab_auth.py:500

  • The Azure CLI token acquisition block includes lines that exceed the repo's Black line-length (88), which will cause formatting churn and makes the code harder to read (e.g., the inline conditional credential construction). Please wrap these statements in Black-friendly form.
        try:
            credential = AzureCliCredential(tenant_id=stored_tenant) if stored_tenant else AzureCliCredential()
            # AzureCliCredential.get_token expects scopes as positional args
            azure_token = credential.get_token(scope[0])
            token_result = {

src/fabric_cli/core/fab_auth.py:522

  • This sanitization fallback message is on a single very long line (over Black's 88-char limit). Wrapping it will keep formatting stable and improve readability.
            error_msg = str(e)
            if any(p.lower() in error_msg.lower() for p in self._SENSITIVE_PATTERNS):
                error_msg = "Azure CLI token acquisition failed. Run 'az account get-access-token' manually to diagnose."
            raise FabricCLIError(
                f"Azure CLI authentication failed: {error_msg}",

All auth modes validate Fabric, OneLake, and Azure scopes at login.
In-memory caching ensures no redundant subprocess calls at runtime.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 11, 2026 11:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

src/fabric_cli/core/fab_auth.py:501

  • _acquire_token_from_azure_cli() calls scope[0] unconditionally (including in credential.get_token(scope[0])). If a caller passes an empty scope list, this will raise IndexError instead of a structured FabricCLIError. Also, AzureCliCredential.get_token expects scopes as positional args; using only scope[0] silently drops additional scopes if they’re ever introduced.
        try:
            credential = AzureCliCredential(tenant_id=stored_tenant) if stored_tenant else AzureCliCredential()
            # AzureCliCredential.get_token expects scopes as positional args
            azure_token = credential.get_token(scope[0])
            token_result = {

tests/test_core/test_fab_auth_azure_cli.py:36

  • The auth_instance fixture is unused and its singleton-reset logic is incorrect for FabAuth (FabAuth is a function returned by the singleton decorator, and singleton() doesn’t expose a wrapped instances map). Keeping this dead code is misleading and risks future test failures if someone starts using it.
@pytest.fixture
def auth_instance(temp_dir_fixture):
    """Get a fresh FabAuth instance."""
    # Clear singleton for test isolation
    FabAuth.__wrapped__ = None  # type: ignore
    from fabric_cli.core import fab_auth as fab_auth_module

    if FabAuth in fab_auth_module.singleton.__wrapped__:  # type: ignore
        del fab_auth_module.singleton.__wrapped__[FabAuth]  # type: ignore
    return FabAuth()

tests/test_core/test_fab_auth_azure_cli.py:56

  • The fresh_auth fixture is unused and attempts to mutate FabAuth.init.globals / singleton internals, which is brittle and not a valid way to reset the singleton. This should be removed to keep the test module deterministic and maintainable.
@pytest.fixture
def fresh_auth(temp_dir_fixture, monkeypatch):
    """Get a fresh FabAuth instance with singleton cleared."""
    # Reset singleton instances dict
    import fabric_cli.core.fab_auth as auth_module

    # Access the closure variable of the singleton decorator
    singleton_instances = auth_module.singleton.__code__.co_consts  # noqa
    # Simpler approach: just patch the module-level reference
    monkeypatch.setattr(
        "fabric_cli.core.fab_auth.FabAuth.__init__.__globals__",
        {},
        raising=False,
    )
    # Re-instantiate
    auth = FabAuth.__new__(FabAuth)
    auth.__init__()
    return auth

Comment thread src/fabric_cli/core/fab_auth.py Outdated
Ensures both --azure-cli flag and interactive menu selection
show the same confirmation message with tenant ID.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 11, 2026 12:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Suppressed comments (3)

src/fabric_cli/core/fab_auth.py:477

  • New FabricCLIError messages here are hardcoded strings. Elsewhere in this module, auth errors consistently use ErrorMessages.Auth.* helpers (e.g., invalid_identity_type(), token_acquisition_failed(), access_token_error()), which centralizes wording and keeps UX consistent. Consider adding Azure CLI-specific ErrorMessages.Auth helpers and using them here (and for the other Azure CLI error branches) instead of inline strings.
            raise FabricCLIError(
                "Azure CLI auth requires the 'azure-identity' package. "
                "Install it with: pip install azure-identity",
                status_code=con.ERROR_AUTHENTICATION_FAILED,
            )

src/fabric_cli/core/fab_auth.py:441

  • In set_azure_cli(), identity_type is set before calling set_tenant(). If set_tenant() detects a tenant change it calls logout(), which clears _auth_info and can wipe out the just-set identity_type. This can leave the auth config without identity_type when switching tenants via set_azure_cli(tenant_id=...). Reorder so tenant changes (and any logout) happen first, then set identity_type last.
        self._set_auth_properties(
            {
                con.IDENTITY_TYPE: "azure_cli",
            }
        )

tests/test_core/test_fab_auth_azure_cli.py:57

  • The auth_instance and fresh_auth fixtures are defined but never used in this test module, and they contain brittle/incorrect attempts to reset the @singleton-decorated FabAuth (e.g., mutating FabAuth.wrapped and FabAuth.init.globals). Keeping these unused fixtures risks future accidental use and makes the tests harder to understand; remove them or refactor to a single, actually-used fixture.
@pytest.fixture
def auth_instance(temp_dir_fixture):
    """Get a fresh FabAuth instance."""
    # Clear singleton for test isolation
    FabAuth.__wrapped__ = None  # type: ignore
    from fabric_cli.core import fab_auth as fab_auth_module

    if FabAuth in fab_auth_module.singleton.__wrapped__:  # type: ignore
        del fab_auth_module.singleton.__wrapped__[FabAuth]  # type: ignore
    return FabAuth()


@pytest.fixture
def fresh_auth(temp_dir_fixture, monkeypatch):
    """Get a fresh FabAuth instance with singleton cleared."""

Comment thread src/fabric_cli/core/fab_auth.py Outdated
Comment thread src/fabric_cli/core/fab_auth.py Outdated
Copilot AI review requested due to automatic review settings August 11, 2026 12:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (5)

tests/test_core/test_fab_auth_azure_cli.py:57

  • The fresh_auth fixture contains fragile/incorrect singleton-reset logic (e.g., reading singleton.__code__.co_consts and patching FabAuth.__init__.__globals__ to {}), and it is unused in this test module. Leaving this in place makes the tests harder to understand and could break badly if someone starts using it later.

Consider removing it and relying on a single, correct autouse singleton-reset fixture instead.

@pytest.fixture
def fresh_auth(temp_dir_fixture, monkeypatch):
    """Get a fresh FabAuth instance with singleton cleared."""
    # Reset singleton instances dict
    import fabric_cli.core.fab_auth as auth_module

    # Access the closure variable of the singleton decorator
    singleton_instances = auth_module.singleton.__code__.co_consts  # noqa
    # Simpler approach: just patch the module-level reference
    monkeypatch.setattr(
        "fabric_cli.core.fab_auth.FabAuth.__init__.__globals__",
        {},
        raising=False,
    )
    # Re-instantiate
    auth = FabAuth.__new__(FabAuth)
    auth.__init__()
    return auth

src/fabric_cli/core/fab_auth.py:477

  • This new ImportError path raises FabricCLIError with a hardcoded message. In this codebase, auth errors are consistently sourced from ErrorMessages.Auth.* (see e.g. fab_auth.py:590-636) so messages stay centralized and reusable.

Please add an AuthErrors.azure_cli_missing_dependency() (or similar) and use it here for consistency.

        try:
            from azure.identity import AzureCliCredential, CredentialUnavailableError
        except ImportError:
            raise FabricCLIError(
                "Azure CLI auth requires the 'azure-identity' package. "
                "Install it with: pip install azure-identity",
                status_code=con.ERROR_AUTHENTICATION_FAILED,
            )

src/fabric_cli/core/fab_auth.py:513

  • The Azure CLI unavailable case uses a hardcoded user-facing message. Elsewhere in this module, user-facing auth errors come from ErrorMessages.Auth.*.

Consider adding a dedicated AuthErrors.azure_cli_unavailable() (and possibly a separate one for "not logged in") and using it here so error messaging stays consistent and maintainable.

        except CredentialUnavailableError:
            raise FabricCLIError(
                "Azure CLI is not installed or not logged in. "
                "Run 'az login' to authenticate, then retry.",
                status_code=con.ERROR_AUTHENTICATION_FAILED,
            )

src/fabric_cli/core/fab_auth.py:500

  • This credential = ... if ... else ... line exceeds Black’s default line length and will likely fail formatting checks in CI.

Wrap it onto multiple lines so black src/ tests/ stays clean.

        try:
            credential = AzureCliCredential(tenant_id=stored_tenant) if stored_tenant else AzureCliCredential()
            # AzureCliCredential.get_token expects scopes as positional args
            azure_token = credential.get_token(scope[0])
            token_result = {

tests/test_core/test_fab_auth_azure_cli.py:37

  • The auth_instance fixture tries to reset the FabAuth singleton via __wrapped__, but FabAuth is a custom @singleton wrapper (a closure) and neither FabAuth.__wrapped__ nor fabric_cli.core.fab_auth.singleton.__wrapped__ exist. If this fixture is ever used it will raise AttributeError, and the current tests also risk leaking singleton state between test modules.

Use a robust singleton reset that clears the wrapped closure’s instances dict, ideally as an autouse fixture so all tests in this module get isolation.

This issue also appears on line 39 of the same file.

@pytest.fixture
def auth_instance(temp_dir_fixture):
    """Get a fresh FabAuth instance."""
    # Clear singleton for test isolation
    FabAuth.__wrapped__ = None  # type: ignore
    from fabric_cli.core import fab_auth as fab_auth_module

    if FabAuth in fab_auth_module.singleton.__wrapped__:  # type: ignore
        del fab_auth_module.singleton.__wrapped__[FabAuth]  # type: ignore
    return FabAuth()

…ss calls

During login, _get_azure_cli_tenant() was called 4 times (auto-capture + 3 drift checks). Now caches for 30s, reducing to 1 subprocess call.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (7)

tests/test_core/test_fab_auth_azure_cli.py:689

  • These tests reset _cached_az_tenant / _cached_az_tenant_time, but the production code caches Azure CLI account info in _cached_az_account / _cached_az_account_time. Resetting the wrong attributes makes the test intent unclear and doesn’t actually clear the cache.
        auth._cached_az_tenant = None
        auth._cached_az_tenant_time = 0.0

tests/test_core/test_fab_auth_azure_cli.py:697

  • These tests reset _cached_az_tenant / _cached_az_tenant_time, but the production code caches Azure CLI account info in _cached_az_account / _cached_az_account_time. Resetting the wrong attributes makes the test intent unclear and doesn’t actually clear the cache.
        auth._cached_az_tenant = None
        auth._cached_az_tenant_time = 0.0

tests/test_core/test_fab_auth_azure_cli.py:705

  • These tests reset _cached_az_tenant / _cached_az_tenant_time, but the production code caches Azure CLI account info in _cached_az_account / _cached_az_account_time. Resetting the wrong attributes makes the test intent unclear and doesn’t actually clear the cache.
        auth._cached_az_tenant = None
        auth._cached_az_tenant_time = 0.0

src/fabric_cli/core/fab_auth.py:455

  • set_azure_cli() persists account["principal_name"] (often an email/UPN) into the auth file under FAB_AZURE_CLI_PRINCIPAL_ID. This contradicts the nearby comment implying no PII impact, and it stores more sensitive data than needed for drift detection. Consider storing a one-way hash/fingerprint instead.
        auth_props: dict = {con.IDENTITY_TYPE: "azure_cli"}
        # Store principal for drift detection (no PII exposed in errors)
        if account and account.get("principal_name"):
            auth_props[con.FAB_AZURE_CLI_PRINCIPAL_ID] = account["principal_name"]
        self._set_auth_properties(auth_props)

src/fabric_cli/core/fab_auth.py:530

  • Principal drift detection compares the stored Azure CLI principal value to the current principal. If the stored value is updated to be a fingerprint/hash (to avoid persisting PII), this block should compare fingerprints rather than raw principal strings so drift checks keep working.
        stored_principal = self._auth_info.get(con.FAB_AZURE_CLI_PRINCIPAL_ID)
        if stored_principal:
            current_principal = self._get_azure_cli_principal()
            if current_principal and current_principal != stored_principal:
                raise FabricCLIError(
                    ErrorMessages.Auth.azure_cli_principal_mismatch(),
                    status_code=con.ERROR_AUTHENTICATION_FAILED,
                )

tests/test_core/test_fab_msal_bridge_azure_cli.py:29

  • This fixture clears _cached_az_tenant / _cached_az_tenant_time, but the implementation uses _cached_az_account / _cached_az_account_time. Clearing the wrong attributes doesn’t reset the real cache and can cause confusing state leakage between tests.
    auth = FabAuth()
    auth._azure_cli_token_cache.clear()
    auth._cached_az_tenant = None
    auth._cached_az_tenant_time = 0.0
    auth._auth_info = {}

tests/test_core/test_fab_auth_azure_cli.py:680

  • These tests reset _cached_az_tenant / _cached_az_tenant_time, but the production code caches Azure CLI account info in _cached_az_account / _cached_az_account_time. Resetting the wrong attributes makes the test intent unclear and doesn’t actually clear the cache.

This issue also appears in the following locations of the same file:

  • line 688
  • line 696
  • line 704
        auth._cached_az_tenant = None
        auth._cached_az_tenant_time = 0.0

shirasassoon and others added 2 commits August 19, 2026 15:57
- Remove _azure_cli_token_cache dict, _get_cached_azure_cli_token(),
  _cache_azure_cli_token(), and _AZURE_CLI_TOKEN_REFRESH_BUFFER_SECONDS
- Add self._azure_cli_credential as singleton instance variable
  (consistent with self.app for MSAL), created lazily in
  _acquire_token_from_azure_cli(), cleared in set_azure_cli() and logout()
- Ensures same credential instance across all 3 scopes, preventing
  cross-scope identity mismatch
- Update tests: replace 4 cache tests with 3 singleton credential tests

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (8)

tests/test_core/test_fab_auth_azure_cli.py:690

  • _cached_az_tenant/_cached_az_tenant_time are not used by FabAuth (the implementation caches _cached_az_account). These lines create unused attributes and don’t actually influence _get_azure_cli_tenant() behavior. Reset _cached_az_account/_cached_az_account_time (or just rely on force_refresh=True) to keep the test aligned with the real cache.
        mock_run.return_value = MagicMock(returncode=0, stdout="   \n")
        auth = FabAuth()
        auth._cached_az_tenant = None
        auth._cached_az_tenant_time = 0.0
        assert auth._get_azure_cli_tenant(force_refresh=True) is None

tests/test_core/test_fab_auth_azure_cli.py:698

  • Same issue as above: these tests reset _cached_az_tenant fields that don’t exist in FabAuth, so the setup is ineffective/misleading. Use _cached_az_account/_cached_az_account_time for cache control, or remove the assignments if they aren’t needed.
        """Should return None on subprocess timeout."""
        auth = FabAuth()
        auth._cached_az_tenant = None
        auth._cached_az_tenant_time = 0.0
        assert auth._get_azure_cli_tenant(force_refresh=True) is None

tests/test_core/test_fab_auth_azure_cli.py:706

  • These cache-reset lines refer to _cached_az_tenant/_cached_az_tenant_time, but FabAuth caches Azure CLI account info in _cached_az_account/_cached_az_account_time. Keeping the wrong attribute names here makes the test intent unclear and may hide regressions if the implementation changes. Switch to resetting _cached_az_account + _cached_az_account_time (or delete if unnecessary).
    def test_az_not_installed_returns_none(self, monkeypatch, temp_dir_fixture):
        """Should return None when az CLI is not installed."""
        monkeypatch.setattr("shutil.which", lambda cmd: None)
        auth = FabAuth()
        auth._cached_az_tenant = None
        auth._cached_az_tenant_time = 0.0
        assert auth._get_azure_cli_tenant(force_refresh=True) is None

tests/test_core/test_fab_msal_bridge_azure_cli.py:28

  • These assignments reset _cached_az_tenant/_cached_az_tenant_time, but FabAuth doesn’t define or use those fields (it uses _cached_az_account/_cached_az_account_time). As written, the fixture creates unused attributes and doesn’t actually reset the Azure CLI account cache between tests. Update the fixture to clear _cached_az_account and _cached_az_account_time instead.
    auth = FabAuth()
    auth._azure_cli_token_cache.clear()
    auth._cached_az_tenant = None
    auth._cached_az_tenant_time = 0.0
    auth._auth_info = {}

tests/test_core/test_fab_auth_azure_cli.py:680

  • FabAuth uses _cached_az_account/_cached_az_account_time for the Azure CLI account cache; _cached_az_tenant and _cached_az_tenant_time aren’t part of the implementation. Resetting the wrong attributes here makes the test setup misleading and doesn’t affect the real cache state. Reset _cached_az_account and _cached_az_account_time instead.

This issue also appears in the following locations of the same file:

  • line 686
  • line 694
  • line 700
        auth = FabAuth()
        auth._cached_az_tenant = None
        auth._cached_az_tenant_time = 0.0
        assert auth._get_azure_cli_tenant(force_refresh=True) is None

src/fabric_cli/core/fab_auth.py:455

  • account["principal_name"] comes from az account show’s user.name (typically a UPN/email), and this value is persisted into auth.json under FAB_AZURE_CLI_PRINCIPAL_ID. This both stores PII on disk and the key name suggests an immutable ID when it’s actually a display/name string. Consider (a) not persisting the principal at all, (b) persisting a hashed value, or (c) renaming the key to reflect it’s a principal name and documenting that it may contain an email/UPN.
        auth_props: dict = {con.IDENTITY_TYPE: "azure_cli"}
        # Store principal for drift detection (no PII exposed in errors)
        if account and account.get("principal_name"):
            auth_props[con.FAB_AZURE_CLI_PRINCIPAL_ID] = account["principal_name"]
        self._set_auth_properties(auth_props)

src/fabric_cli/core/fab_constant.py:60

  • FAB_AZURE_CLI_PRINCIPAL_ID is used to store Azure CLI user.name (a principal name/UPN), not a stable principal/object ID. Renaming this constant/key to something like FAB_AZURE_CLI_PRINCIPAL_NAME (and updating its usage) would avoid confusing “ID” vs “name” semantics.
FAB_REFRESH_TOKEN = "fab_refresh_token"
FAB_AZURE_CLI_PRINCIPAL_ID = "fab_azure_cli_principal_id"
IDENTITY_TYPE = "identity_type"

pyproject.toml:24

  • PR description says the new dependency is azure-identity>=1.15.0, but pyproject.toml pins it to azure-identity>=1.25.0. If 1.25.0 is intentional (e.g., required for AzureCliCredential behavior or exception types), consider updating the PR description; otherwise, lower the minimum to match the stated requirement.
dependencies = [
    "msal[broker]>=1.34,<2 ; platform_system != 'Linux'",
    "msal>=1.34,<2",
    "msal_extensions",
    "azure-core>=1.29.0",
    "azure-identity>=1.25.0",
    "questionary",

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (6)

tests/test_parsers/test_fab_auth_parser.py:6

  • argparse is imported but never used in this test module; this will fail linting in repos that enforce unused-import checks. Remove the import or use it explicitly.
import argparse

tests/test_core/test_fab_msal_bridge_azure_cli.py:29

  • This fixture resets auth._cached_az_tenant / _cached_az_tenant_time, but FabAuth actually caches Azure CLI state in _cached_az_account / _cached_az_account_time. As written, the real cache may leak between tests and make bridge tests order-dependent.
    auth = FabAuth()
    auth._azure_cli_credential = None
    auth._cached_az_tenant = None
    auth._cached_az_tenant_time = 0.0
    auth._auth_info = {}

tests/test_core/test_fab_auth_azure_cli.py:654

  • These tests clear auth._cached_az_tenant / _cached_az_tenant_time, but _get_azure_cli_tenant() reads from _cached_az_account / _cached_az_account_time. Clearing the wrong attributes is a no-op and makes the test intent misleading; clear the actual cache fields instead (or rely on the fixture).
        auth = FabAuth()
        auth._cached_az_tenant = None
        auth._cached_az_tenant_time = 0.0
        assert auth._get_azure_cli_tenant(force_refresh=True) is None

src/fabric_cli/core/fab_constant.py:59

  • FAB_AZURE_CLI_PRINCIPAL_ID is used to store the Azure CLI userName (a principal name / UPN), not an ID. Consider renaming this key (and any persisted auth.json field) to reflect what it contains, e.g. ...PRINCIPAL_NAME (or store a stable ID instead).
FAB_AZURE_CLI_PRINCIPAL_ID = "fab_azure_cli_principal_id"

src/fabric_cli/core/fab_auth.py:453

  • set_azure_cli() persists account["principal_name"] (typically a user UPN/email) into auth.json for drift detection. This introduces new PII-at-rest; consider storing a non-reversible hash/fingerprint instead, or avoiding persistence and limiting drift detection to tenant-only.
        auth_props: dict = {con.IDENTITY_TYPE: "azure_cli"}
        # Store principal for drift detection (no PII exposed in errors)
        if account and account.get("principal_name"):
            auth_props[con.FAB_AZURE_CLI_PRINCIPAL_ID] = account["principal_name"]
        self._set_auth_properties(auth_props)

src/fabric_cli/core/fab_auth.py:540

  • The PR description mentions an in-memory Azure CLI token cache with a 60s refresh buffer, but _acquire_token_from_azure_cli() calls AzureCliCredential.get_token(...) on every request and only caches the account info/credential object. Either implement the described token caching (per-scope) or update the PR description to match the actual behavior/perf characteristics.
            # Create singleton credential if not yet initialized
            if self._azure_cli_credential is None:
                self._azure_cli_credential = (
                    AzureCliCredential(tenant_id=stored_tenant)
                    if stored_tenant
                    else AzureCliCredential()
                )
            # AzureCliCredential.get_token expects scopes as positional args
            azure_token = self._azure_cli_credential.get_token(scope[0])
            token_result = {

- Remove all subprocess calls to 'az account show' — AzureCliCredential
  is now the only interface to Azure CLI
- Remove _get_azure_cli_account(), _get_azure_cli_tenant(),
  _get_azure_cli_principal(), cached account state, and TTL constant
- Remove subprocess, shutil, time imports from fab_auth.py
- Add _decode_jwt_claims() to extract tid/oid from token payload
- set_azure_cli() now acquires a probe token and reads identity from
  JWT claims instead of a separate subprocess call
- _acquire_token_from_azure_cli() verifies tid:oid from every returned
  token (post-acquisition drift detection)
- Add 'except FabricCLIError: raise' to prevent drift errors from being
  swallowed by the generic exception handler
- Update tests: JWT-based mocking, add decode/lifecycle tests, remove
  subprocess-based discovery failure tests

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (6)

src/fabric_cli/core/fab_auth.py:480

  • In _decode_jwt_claims, the base64url padding logic always appends (4 - len(payload) % 4) '=' characters. When len(payload) % 4 == 0, this appends 4 '=' even though no padding is needed, producing non-standard base64 padding and potentially causing decode failures (claims become {}), which would break tenant/principal discovery and drift checks. Compute padding as (-len(payload)) % 4 and append only that many '='.
            # Add padding for base64url decoding
            payload = parts[1]
            payload += "=" * (4 - len(payload) % 4)
            decoded = base64.urlsafe_b64decode(payload)
            return json.loads(decoded)

src/fabric_cli/core/fab_auth.py:449

  • set_azure_cli() only translates CredentialUnavailableError into a FabricCLIError. Other Azure Identity failures (e.g., invalid tenant, auth failures, transient errors) will bubble up as raw exceptions during fab auth login --azure-cli, bypassing the centralized/sanitized Azure CLI error messages added in this PR. Consider catching the same allowlisted SDK exceptions (and a generic fallback) here as well, similar to _acquire_token_from_azure_cli().

This issue also appears on line 476 of the same file.

        try:
            probe_credential = (
                AzureCliCredential(tenant_id=tenant_id)
                if tenant_id
                else AzureCliCredential()
            )
            probe_token = probe_credential.get_token(con.SCOPE_FABRIC_DEFAULT[0])
            claims = self._decode_jwt_claims(probe_token.token)
        except CredentialUnavailableError:
            raise FabricCLIError(
                ErrorMessages.Auth.azure_cli_not_available(),
                status_code=con.ERROR_AUTHENTICATION_FAILED,
            )

src/fabric_cli/core/fab_auth.py:490

  • PR description mentions a per-command tenant drift check via az account show, plus in-memory token caching / tenant-cache TTLs to reduce subprocess overhead. The current implementation performs drift detection by decoding JWT tid/oid from AzureCliCredential.get_token(...) and does not implement az account show checks or caching/TTLs. Please align the PR description (and any docs that reference those details) with the implemented behavior, or implement the described checks/caches.
        """Acquire a token using Azure CLI's AzureCliCredential.

        After acquiring the token, decodes JWT claims and verifies
        that tid and oid match the stored values from login to detect
        identity drift (e.g., user ran 'az login' as a different user).
        """

pyproject.toml:24

  • The PR description states azure-identity>=1.15.0, but the dependency added here is azure-identity>=1.25.0. Please update the PR description (or adjust the dependency constraint) so they match, since this affects downstream dependency resolution and support expectations.
dependencies = [
    "msal[broker]>=1.34,<2 ; platform_system != 'Linux'",
    "msal>=1.34,<2",
    "msal_extensions",
    "azure-core>=1.29.0",
    "azure-identity>=1.25.0",
    "questionary",

docs/commands/auth/index.md:52

  • Docs list the login parameter as -u, --user, but the actual CLI flag (per fab_auth_parser.py) is -u/--username. This mismatch will confuse users copying the docs; update the docs to use --username consistently.
- `-u, --user`: Client ID for service principal. Optional.
- `-p, --password`: Client secret for service principal. Optional.

tests/test_parsers/test_fab_auth_parser.py:6

  • argparse is imported but never used in this test module. Removing the unused import will keep the test file minimal and avoid implying argparse is part of the test surface.
import argparse

- Add FAB_AZURE_CLI_ISSUER constant and store iss claim at login
- Check iss before tid and oid on every token acquisition
- Drift key is now iss + tid + oid (environment + tenant + identity)
- Add azure_cli_environment_mismatch error message
- Add TestAzureCliEnvironmentDrift tests (public vs sovereign)
- Update JWT test helpers to include iss claim

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (5)

src/fabric_cli/core/fab_auth.py:449

  • set_azure_cli only handles CredentialUnavailableError during the probe token acquisition. If AzureCliCredential.get_token fails with other azure-identity/azure-core exceptions (e.g., ClientAuthenticationError/HttpResponseError), they will currently bubble out as raw exceptions instead of being surfaced as a FabricCLIError with a consistent, safe message/status code (unlike _acquire_token_from_azure_cli).
        try:
            probe_credential = (
                AzureCliCredential(tenant_id=tenant_id)
                if tenant_id
                else AzureCliCredential()
            )
            probe_token = probe_credential.get_token(con.SCOPE_FABRIC_DEFAULT[0])
            claims = self._decode_jwt_claims(probe_token.token)
        except CredentialUnavailableError:
            raise FabricCLIError(
                ErrorMessages.Auth.azure_cli_not_available(),
                status_code=con.ERROR_AUTHENTICATION_FAILED,
            )

docs/commands/auth/index.md:56

  • The parameter list documents -u, --user, but the parser defines -u/--username (src/fabric_cli/parsers/fab_auth_parser.py:51-57). This mismatch will confuse users and makes the command reference inaccurate.
**Parameters:**

- `-u, --user`: Client ID for service principal. Optional.
- `-p, --password`: Client secret for service principal. Optional.
- `--federated-token`: Federated token for workload identity. Optional.
- `--certificate`: Path to certificate file. Optional.
- `--azure-cli`: Use an existing Azure CLI login session as the token provider. Requires Azure CLI to be installed and logged in (`az login`). Optional.
- `--tenant`: Tenant ID. Optional. When used with `--azure-cli`, pins Fabric CLI to the specified tenant.

tests/test_core/test_fab_auth_azure_cli.py:23

  • This new test file doesn't appear to be Black-formatted (e.g., long lines like the payload = base64.urlsafe_b64encode(...).rstrip(...).decode() assignment exceed the repo's Black line-length of 88 from tox.toml:[tool.black]. This may fail formatting checks in CI.
def _make_jwt(tid: str = "test-tenant", oid: str = "test-oid",
              iss: str = "https://sts.windows.net/test-tenant/", **extra_claims) -> str:
    """Create a fake JWT with specified claims (no signature validation needed)."""
    header = base64.urlsafe_b64encode(b'{"alg":"none"}').rstrip(b"=").decode()
    claims = {"tid": tid, "oid": oid, "iss": iss, **extra_claims}
    payload = base64.urlsafe_b64encode(_json.dumps(claims).encode()).rstrip(b"=").decode()
    return f"{header}.{payload}.fakesig"

tests/test_core/test_fab_msal_bridge_azure_cli.py:23

  • This new test file includes lines that exceed the repo's Black line-length (88) (e.g., the long payload = base64.urlsafe_b64encode(...).rstrip(...).decode() line). Please run Black to keep formatting consistent with tox.toml:[tool.black].
def _make_jwt(tid: str = "test-tenant", oid: str = "test-oid") -> str:
    """Create a fake JWT with specified claims."""
    header = base64.urlsafe_b64encode(b'{"alg":"none"}').rstrip(b"=").decode()
    claims = {"tid": tid, "oid": oid, "iss": f"https://sts.windows.net/{tid}/"}
    payload = base64.urlsafe_b64encode(_json.dumps(claims).encode()).rstrip(b"=").decode()
    return f"{header}.{payload}.fakesig"

src/fabric_cli/core/fab_auth.py:505

  • The PR description mentions a per-command tenant drift check via az account show plus in-memory token/tenant caches (60s refresh buffer, 10s tenant TTL) and shutil.which("az") handling. None of that behavior appears to be implemented in fab_auth.py (no subprocess/az invocation, no cache/TTL logic). Either update the PR description/docs to match the current implementation (JWT-claim drift checks only), or add the described drift/caching behavior if it’s a requirement.
    def _acquire_token_from_azure_cli(self, scope: list[str]) -> dict:
        """Acquire a token using Azure CLI's AzureCliCredential.

        After acquiring the token, decodes JWT claims and verifies
        that iss, tid, and oid match the stored values from login to detect
        identity or environment drift.
        """
        stored_tenant = self.get_tenant_id()

        try:
            # Create singleton credential if not yet initialized
            if self._azure_cli_credential is None:
                self._azure_cli_credential = (
                    AzureCliCredential(tenant_id=stored_tenant)
                    if stored_tenant
                    else AzureCliCredential()
                )
            # AzureCliCredential.get_token expects scopes as positional args
            azure_token = self._azure_cli_credential.get_token(scope[0])

Comment thread src/fabric_cli/core/fab_auth.py Outdated
Comment on lines +478 to +482
# Add padding for base64url decoding
payload = parts[1]
payload += "=" * (4 - len(payload) % 4)
decoded = base64.urlsafe_b64decode(payload)
return json.loads(decoded)
Reject tokens that lack iss, tid, or oid instead of silently skipping
drift checks. Applied at both login (set_azure_cli) and token
acquisition (_acquire_token_from_azure_cli).

Also fixed redundant nested if in environment drift check and
incorrect indentation in principal drift check.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (6)

tests/test_parsers/test_fab_auth_parser.py:6

  • argparse is imported but never used in this test module, which can trigger linting/type-check noise. Remove the unused import.
import argparse

src/fabric_cli/core/fab_auth.py:493

  • _decode_jwt_claims pads base64 with "=" * (4 - len(payload) % 4), which adds 4 '=' when the length is already a multiple of 4 and can break decoding. Also, urlsafe_b64decode can raise binascii.Error, which isn’t caught here, so malformed tokens may raise unexpectedly instead of returning {}.
            # Add padding for base64url decoding
            payload = parts[1]
            payload += "=" * (4 - len(payload) % 4)
            decoded = base64.urlsafe_b64decode(payload)
            return json.loads(decoded)
        except (ValueError, json.JSONDecodeError, UnicodeDecodeError):
            return {}

src/fabric_cli/core/fab_auth.py:449

  • set_azure_cli only wraps CredentialUnavailableError. Other Azure CLI / azure-identity failures during the probe get_token (e.g., auth failures, response errors) will bubble up as raw exceptions, bypassing the safe/structured FabricCLIError path used in _acquire_token_from_azure_cli. Consider applying the same allowlist + generic-safe fallback here.
            probe_token = probe_credential.get_token(con.SCOPE_FABRIC_DEFAULT[0])
            claims = self._decode_jwt_claims(probe_token.token)
        except CredentialUnavailableError:
            raise FabricCLIError(
                ErrorMessages.Auth.azure_cli_not_available(),

src/fabric_cli/core/fab_auth.py:580

  • The generic exception path sets error_msg = ErrorMessages.Auth.azure_cli_token_acquisition_failed() and then wraps it with azure_cli_auth_failed(...), resulting in a redundant message like "Azure CLI authentication failed: Azure CLI token acquisition failed...". Consider raising the generic message directly (or adjusting the strings) to avoid duplication.
            else:
                error_msg = ErrorMessages.Auth.azure_cli_token_acquisition_failed()
            raise FabricCLIError(
                ErrorMessages.Auth.azure_cli_auth_failed(error_msg),
                status_code=con.ERROR_AUTHENTICATION_FAILED,

src/fabric_cli/core/fab_auth.py:500

  • PR description mentions Azure CLI tenant drift check via az account show, shutil.which("az") resolution, and in-memory token/tenant caching, but the implementation here only uses AzureCliCredential.get_token(...) and JWT-claim drift checks, with no subprocess-based checks or caching logic present. Please either implement the described behavior or update the PR description to match the actual behavior.
        """Acquire a token using Azure CLI's AzureCliCredential.

        After acquiring the token, decodes JWT claims and verifies
        that iss, tid, and oid match the stored values from login to detect
        identity or environment drift.

pyproject.toml:23

  • PR description says the dependency added is azure-identity>=1.15.0, but pyproject.toml pins it as azure-identity>=1.25.0. Please align the PR description (or adjust the constraint) so consumers know the actual minimum required version.
    "azure-identity>=1.25.0",

Remove tenant_id parameter from set_azure_cli() — Fabric CLI always
inherits the tenant from Azure CLI's active session via JWT tid claim.
Users must switch tenants via 'az login --tenant' rather than
overriding at the Fabric CLI level.

Also removes tenant pinning from AzureCliCredential at acquisition
time, ensuring drift detection fires if the user switches Azure CLI
context between calls.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (7)

tests/test_commands/test_auth.py:989

  • This test expects set_azure_cli("my-tenant") to be called, but init() currently calls set_azure_cli() with no arguments. Either adjust the test to match the implementation (no args), or implement a tenant-aware Azure CLI login flow where the tenant argument is validated/used.
        mock_fab_auth_instance = mock_fab_auth.get("instance")
        mock_fab_auth_instance.set_access_mode.assert_called_with(
            "azure_cli", "my-tenant"
        )
        mock_set_azure_cli.assert_called_once_with("my-tenant")
        assert result is True

tests/test_commands/test_auth.py:1012

  • This test expects set_azure_cli(None) when Azure CLI is selected interactively, but init() calls set_azure_cli() with no arguments. The assertion should be updated to reflect the actual no-arg call (or the implementation changed to always pass an explicit tenant argument).
        mock_fab_auth_instance = mock_fab_auth.get("instance")
        mock_fab_auth_instance.set_access_mode.assert_called_with(
            "azure_cli", None
        )
        mock_set_azure_cli.assert_called_once_with(None)
        assert_get_access_token(mock_fab_auth_instance)

src/fabric_cli/core/fab_auth.py:488

  • _decode_jwt_claims pads with "=" * (4 - len(payload) % 4), which adds 4 '=' when the payload length is already a multiple of 4. Also, base64.urlsafe_b64decode can raise binascii.Error for malformed input (e.g., the "a.!!!.c" test case), which isn't caught here, so the helper may raise instead of returning {}. Adjust padding to only add the required 0–3 chars (e.g., missing = (-len(payload)) % 4) and broaden the exception handling so malformed tokens fail closed to {}.
            # Add padding for base64url decoding
            payload = parts[1]
            payload += "=" * (4 - len(payload) % 4)
            decoded = base64.urlsafe_b64decode(payload)
            return json.loads(decoded)
        except (ValueError, json.JSONDecodeError, UnicodeDecodeError):
            return {}

src/fabric_cli/commands/auth/fab_auth.py:33

  • In the Azure CLI login path, args.tenant is passed to set_access_mode("azure_cli", args.tenant), but set_azure_cli() then overwrites the tenant from the Azure CLI token claims (and its docstring says tenant override is not supported). This means fab auth login --azure-cli --tenant <tid> does not actually pin to the provided tenant. Either (a) remove/ignore --tenant for Azure CLI in the command/parser/docs, or (b) treat --tenant as a required match and fail fast if the Azure CLI token's tid differs from args.tenant.
    if getattr(args, "azure_cli", False):
        FabAuth().set_access_mode("azure_cli", args.tenant)
        FabAuth().set_azure_cli()

docs/commands/auth/index.md:56

  • The docs say --tenant "pins Fabric CLI to the specified tenant" when used with --azure-cli, but the implementation explicitly inherits tenant from the Azure CLI token (FabAuth.set_azure_cli() states tenant override isn't supported and overwrites the tenant from claims). Please update the documentation to match actual behavior, or implement true tenant pinning/validation for Azure CLI auth.
- `--azure-cli`: Use an existing Azure CLI login session as the token provider. Requires Azure CLI to be installed and logged in (`az login`). Optional.
- `--tenant`: Tenant ID. Optional. When used with `--azure-cli`, pins Fabric CLI to the specified tenant.

src/fabric_cli/parsers/fab_auth_parser.py:36

  • The login examples imply fab auth login --azure-cli --tenant <tenant_id> is supported as a way to select a specific tenant. Currently Azure CLI login inherits tenant from the Azure CLI session token claims (no override), so this example is misleading unless the tenant argument is validated/enforced. Consider removing the tenant-specific example for Azure CLI or clarifying that tenant must be switched via az login --tenant (and/or enforced by the code).
        "# command_line mode using Azure CLI auth",
        "$ fab auth login --azure-cli\n",
        "# command_line mode using Azure CLI auth with specific tenant",
        "$ fab auth login --azure-cli --tenant <tenant_id>\n",

docs/examples/auth_examples.md:55

  • This section shows fab auth login --azure-cli --tenant <tenant_id>, but the current implementation inherits tenant from the Azure CLI session token and doesn't use the provided tenant as an override. Please clarify in the example/note whether --tenant is ignored for Azure CLI auth (and users must use az login --tenant), or update the implementation to validate/pin to the provided tenant.
#### Log in using Azure CLI with a specific tenant

fab auth login --azure-cli --tenant <tenant_id>


!!! note "Tenant behavior"
    - If `--tenant` is not specified, Fabric CLI captures and records the tenant from the current Azure CLI session at login time.
    - Throughout the `fab` session, the Azure CLI's active tenant is checked against the recorded tenant. If you switch tenants in Azure CLI (e.g., `az login --tenant <other>`), Fabric CLI will raise a tenant mismatch error and require you to re-authenticate, e.g., `fab auth login --azure-cli`.

Comment thread tests/test_commands/test_auth.py
In azure-cli auth mode, --tenant is now validated against the Azure CLI
session's actual tenant (from the JWT tid claim). If they match, login
proceeds. If they differ, an error guides the user to run
'az login --tenant <desired>' instead. This preserves strict context
inheritance while giving clear feedback when the user's intent doesn't
match reality.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/fabric_cli/core/fab_auth.py:496

  • _decode_jwt_claims adds padding unconditionally via payload += "=" * (4 - len(payload) % 4). When len(payload) % 4 == 0, this appends 4 '=' characters, which can make otherwise-valid base64url payloads fail to decode (incorrect padding). Use a modulo-based calculation like (-len(payload)) % 4 so 0 padding is added when already aligned.
            # Add padding for base64url decoding
            payload = parts[1]
            payload += "=" * (4 - len(payload) % 4)
            decoded = base64.urlsafe_b64decode(payload)
            return json.loads(decoded)

src/fabric_cli/core/fab_auth.py:553

  • The raise FabricCLIError(...) block in the principal drift check is mis-indented (arguments indented deeper than the raise). Please reformat this block (e.g., run Black) to keep formatting consistent and avoid CI formatting failures.
            stored_principal = self._auth_info.get(con.FAB_AZURE_CLI_PRINCIPAL_ID)
            if stored_principal and claims["oid"] != stored_principal:
                raise FabricCLIError(
                        ErrorMessages.Auth.azure_cli_principal_mismatch(),
                        status_code=con.ERROR_AUTHENTICATION_FAILED,
                    )

src/fabric_cli/core/fab_auth.py:536

  • The raise FabricCLIError(...) block in the environment drift check is mis-indented (the arguments are indented 2 levels deeper than the raise). This is likely to be reformatted by Black and may fail style checks if formatting is enforced in CI; please reformat this block to standard indentation.

This issue also appears on line 548 of the same file.

            if stored_issuer and claims["iss"] != stored_issuer:
                raise FabricCLIError(
                        ErrorMessages.Auth.azure_cli_environment_mismatch(),
                        status_code=con.ERROR_AUTHENTICATION_FAILED,
                    )

src/fabric_cli/core/fab_auth.py:434

  • The PR description calls out a per-command tenant drift check via az account show plus in-memory token/tenant caches (60s refresh buffer, 10s TTL). In the current implementation, tenant/environment/principal drift is inferred solely from JWT claims returned by AzureCliCredential.get_token(...), and there is no az account show subprocess usage or cache logic in this module. Please either implement the described behavior or update the PR description to match what was actually shipped.
    def set_azure_cli(self, tenant_id=None):
        """Configure Azure CLI as the authentication source.

        Acquires a probe token from Azure CLI to discover and store
        the tenant ID and principal OID from the actual JWT claims.
        Fabric CLI strictly inherits the Azure CLI auth context.
        If tenant_id is provided, it is validated against the Azure CLI
        context — a mismatch raises an error directing the user to
        switch tenants via 'az login --tenant'.
        """

The iss claim includes the tenant ID (e.g., https://sts.windows.net/{tid}/),
so switching tenants falsely triggered the environment mismatch error
instead of the tenant mismatch error. Now stores and compares only the
issuer hostname (e.g., sts.windows.net vs sts.chinacloudapi.cn).

Also reordered drift checks: tenant first (most common), then
environment, then principal.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

tests/test_core/test_fab_auth_azure_cli.py:23

  • New test file isn’t Black-formatted (e.g., multi-line function definitions/arguments and long lines exceed the repo’s Black conventions). Please run Black on this file so it matches the rest of the test suite and avoids noisy auto-reformatting later.
def _make_jwt(tid: str = "test-tenant", oid: str = "test-oid",
              iss: str = "https://sts.windows.net/test-tenant/", **extra_claims) -> str:
    """Create a fake JWT with specified claims (no signature validation needed)."""
    header = base64.urlsafe_b64encode(b'{"alg":"none"}').rstrip(b"=").decode()
    claims = {"tid": tid, "oid": oid, "iss": iss, **extra_claims}
    payload = base64.urlsafe_b64encode(_json.dumps(claims).encode()).rstrip(b"=").decode()
    return f"{header}.{payload}.fakesig"

src/fabric_cli/core/fab_auth.py:434

  • PR description mentions (a) tenant drift detection via az account show, (b) an in-memory token cache with a 60s refresh buffer, (c) a 10s tenant cache TTL, and (d) shutil.which("az") handling. The current implementation in set_azure_cli/_acquire_token_from_azure_cli appears to rely solely on decoding JWT claims from tokens returned by AzureCliCredential and does not include those subprocess-based checks/caches. Please either update the PR description to match the actual approach, or implement the described behavior so performance/UX expectations are accurate.
    def set_azure_cli(self, tenant_id=None):
        """Configure Azure CLI as the authentication source.

        Acquires a probe token from Azure CLI to discover and store
        the tenant ID and principal OID from the actual JWT claims.
        Fabric CLI strictly inherits the Azure CLI auth context.
        If tenant_id is provided, it is validated against the Azure CLI
        context — a mismatch raises an error directing the user to
        switch tenants via 'az login --tenant'.
        """

pyproject.toml:24

  • pyproject.toml pins azure-identity to >=1.25.0, but the PR description says >=1.15.0. Please align the PR description and the dependency constraint (and document why the higher minimum is needed, if intentional) to avoid confusion for reviewers and downstream packagers.
dependencies = [
    "msal[broker]>=1.34,<2 ; platform_system != 'Linux'",
    "msal>=1.34,<2",
    "msal_extensions",
    "azure-core>=1.29.0",
    "azure-identity>=1.25.0",
    "questionary",

Comment thread src/fabric_cli/core/fab_auth.py Outdated
AzureCliCredential logs 'Please run az login' to stderr on every
failed get_token call. Since status acquires 4 tokens, this produced
4 identical error messages. Now suppresses Azure SDK logging during
status and shows one clear Fabric CLI message instead.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (6)

src/fabric_cli/core/fab_auth.py:498

  • In _decode_jwt_claims(), base64 padding is calculated as "=" * (4 - len(payload) % 4). When len(payload) % 4 == 0 this appends 4 '=' characters, which can cause urlsafe_b64decode() to raise "Incorrect padding" for already-correctly-sized JWT payloads. Use a conditional remainder (or "=" * (-len(payload) % 4)) so no padding is added when not needed.
            parts = token.split(".")
            if len(parts) < 2:
                return {}
            # Add padding for base64url decoding
            payload = parts[1]
            payload += "=" * (4 - len(payload) % 4)
            decoded = base64.urlsafe_b64decode(payload)
            return json.loads(decoded)

src/fabric_cli/core/fab_auth.py:447

  • set_azure_cli() only catches CredentialUnavailableError around the AzureCliCredential probe. Other Azure Identity / subprocess errors will bubble up to the top-level unexpected-error handler, bypassing the PR’s intended safe/sanitized Azure CLI error handling and potentially surfacing sensitive details. Consider wrapping non-CredentialUnavailableError exceptions into FabricCLIError using the same allowlist/generic-message policy as _acquire_token_from_azure_cli().
        try:
            probe_credential = AzureCliCredential()
            probe_token = probe_credential.get_token(con.SCOPE_FABRIC_DEFAULT[0])
            claims = self._decode_jwt_claims(probe_token.token)
        except CredentialUnavailableError:
            raise FabricCLIError(
                ErrorMessages.Auth.azure_cli_not_available(),
                status_code=con.ERROR_AUTHENTICATION_FAILED,
            )

src/fabric_cli/core/fab_auth.py:560

  • Indentation in the principal drift error path is inconsistent (the ErrorMessages.Auth.azure_cli_principal_mismatch() line is over-indented), which is likely to fail formatting/lint checks and makes the block harder to read. Align the raise FabricCLIError(...) call with the surrounding raise statements.
            if stored_principal and claims["oid"] != stored_principal:
                raise FabricCLIError(
                        ErrorMessages.Auth.azure_cli_principal_mismatch(),
                        status_code=con.ERROR_AUTHENTICATION_FAILED,
                    )

docs/commands/auth/index.md:56

  • The auth login parameter docs list -u, --user, but the CLI parser defines -u/--username. This makes the docs misleading for users trying to copy/paste commands. Update the flag name in the docs to match the actual parser (--username).
**Parameters:**

- `-u, --user`: Client ID for service principal. Optional.
- `-p, --password`: Client secret for service principal. Optional.
- `--federated-token`: Federated token for workload identity. Optional.
- `--certificate`: Path to certificate file. Optional.
- `--azure-cli`: Use an existing Azure CLI login session as the token provider. Requires Azure CLI to be installed and logged in (`az login`). Optional.
- `--tenant`: Tenant ID. Optional. When used with `--azure-cli`, pins Fabric CLI to the specified tenant.

tests/test_parsers/test_fab_auth_parser.py:6

  • Unused import: import argparse is not referenced anywhere in this test module. Removing it avoids lint failures and keeps the test focused.
import argparse

src/fabric_cli/core/fab_auth.py:508

  • The PR description states Azure CLI auth includes per-command tenant drift checks via az account show, an in-memory token cache with a 60-second refresh buffer, a tenant cache TTL, and shutil.which("az") resolution. The current implementation in _acquire_token_from_azure_cli() does none of these (drift is checked via decoded JWT claims on each get_token call, with no explicit caching/az probing). Either implement the described behavior or update the PR description to match what’s actually shipped.
        """Acquire a token using Azure CLI's AzureCliCredential.

        After acquiring the token, decodes JWT claims and verifies
        that iss, tid, and oid match the stored values from login to detect
        identity or environment drift.
        """

The caller uses set_azure_cli(tenant_id=args.tenant) (keyword arg),
but the test assertions used assert_called_once_with(None) (positional).
Also fix base64 padding bug and add binascii.Error catch in JWT decoder.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (5)

src/fabric_cli/core/fab_auth.py:561

  • The FabricCLIError raise block in the principal drift check is mis-indented. It’s still syntactically valid, but it reduces readability and will likely be reformatted by black in a later run; adjust indentation to match the surrounding raise blocks.
            stored_principal = self._auth_info.get(con.FAB_AZURE_CLI_PRINCIPAL_ID)
            if stored_principal and claims["oid"] != stored_principal:
                raise FabricCLIError(
                        ErrorMessages.Auth.azure_cli_principal_mismatch(),
                        status_code=con.ERROR_AUTHENTICATION_FAILED,
                    )

src/fabric_cli/core/fab_auth.py:449

  • In set_azure_cli(), only CredentialUnavailableError is caught. Other AzureCliCredential.get_token() failures (e.g., ClientAuthenticationError/HttpResponseError) will bubble up as raw SDK exceptions and bypass FabricCLIError formatting/sanitization. Consider catching the same allowlisted Azure SDK exceptions as acquire_token_from_azure_cli() and re-raising FabricCLIError with the centralized ErrorMessages.Auth.azure_cli* messages.
        # Acquire a probe token to discover identity from JWT claims
        try:
            probe_credential = AzureCliCredential()
            probe_token = probe_credential.get_token(con.SCOPE_FABRIC_DEFAULT[0])
            claims = self._decode_jwt_claims(probe_token.token)
        except CredentialUnavailableError:
            raise FabricCLIError(
                ErrorMessages.Auth.azure_cli_not_available(),
                status_code=con.ERROR_AUTHENTICATION_FAILED,
            )

src/fabric_cli/core/fab_auth.py:57

  • The PR description mentions an in-memory token cache (with refresh buffer), a tenant cache (TTL), and tenant drift verification via az account show / shutil.which("az"), but the implementation only adds a singleton AzureCliCredential and JWT-claim drift checks (no caching or az subprocess checks were added). Either implement the described caching/drift-check mechanism or update the PR description to match the actual behavior/perf characteristics.
        # Singleton AzureCliCredential instance (like self.app for MSAL)
        self._azure_cli_credential: Optional[AzureCliCredential] = None

docs/commands/auth/index.md:56

  • Docs say that when used with --azure-cli, --tenant "pins Fabric CLI to the specified tenant". In the implementation, Fabric CLI always inherits the Azure CLI tenant; providing --tenant only validates it matches the current Azure CLI session and errors if it doesn’t. Please update the wording to reflect the validation/inheritance behavior (and that tenant switching requires az login --tenant ...).
- `--azure-cli`: Use an existing Azure CLI login session as the token provider. Requires Azure CLI to be installed and logged in (`az login`). Optional.
- `--tenant`: Tenant ID. Optional. When used with `--azure-cli`, pins Fabric CLI to the specified tenant.

tests/test_parsers/test_fab_auth_parser.py:7

  • Unused import: argparse is imported but never referenced in this test module. Remove it to keep tests clean and avoid failing any future linting.
import argparse

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (6)

src/fabric_cli/commands/auth/fab_auth.py:83

  • Same issue in the interactive Azure CLI branch: set_access_mode("azure_cli", args.tenant) can write the requested tenant before set_azure_cli() validates it, leaving persisted auth state inconsistent if validation fails. Prefer set_access_mode("azure_cli") without tenant here as well, and rely on set_azure_cli(tenant_id=...) to set/validate the tenant.
            elif selected_auth.startswith("Azure CLI"):
                FabAuth().set_access_mode("azure_cli", args.tenant)
                FabAuth().set_azure_cli(tenant_id=args.tenant)
                _acquire_default_access_tokens(FabAuth())
                Context().context = FabAuth().get_tenant()

src/fabric_cli/commands/auth/fab_auth.py:34

  • When --azure-cli --tenant <tenant> is used and the Azure CLI is actually logged into a different tenant, set_access_mode("azure_cli", args.tenant) persists the requested tenant before set_azure_cli() validates it. If set_azure_cli() then raises a mismatch error, the auth state can be left with an incorrect tenant/identity_type even though login failed. Consider calling set_access_mode("azure_cli") (no tenant) first (to trigger logout), then letting set_azure_cli(tenant_id=...) set the tenant only after validation succeeds.

This issue also appears on line 79 of the same file.

    if getattr(args, "azure_cli", False):
        FabAuth().set_access_mode("azure_cli", args.tenant)
        FabAuth().set_azure_cli(tenant_id=args.tenant)
        _acquire_default_access_tokens(FabAuth())

src/fabric_cli/core/fab_auth.py:448

  • set_azure_cli() only catches CredentialUnavailableError. Other Azure Identity failures (e.g., ClientAuthenticationError, HttpResponseError) will currently bubble up as raw exceptions (not FabricCLIError), which can break CLI error formatting and potentially expose unsanitized messages. Align the login probe with _acquire_token_from_azure_cli() by catching unexpected exceptions and raising a safe FabricCLIError message (and/or the same allowlisted SDK exceptions) instead.
        # Acquire a probe token to discover identity from JWT claims
        try:
            probe_credential = AzureCliCredential()
            probe_token = probe_credential.get_token(con.SCOPE_FABRIC_DEFAULT[0])
            claims = self._decode_jwt_claims(probe_token.token)
        except CredentialUnavailableError:
            raise FabricCLIError(
                ErrorMessages.Auth.azure_cli_not_available(),
                status_code=con.ERROR_AUTHENTICATION_FAILED,
            )

docs/commands/auth/index.md:56

  • Docs say --tenant used with --azure-cli “pins Fabric CLI to the specified tenant”, but the implementation doesn’t pin/switch tenants; it validates that the current Azure CLI tenant matches the requested tenant and errors otherwise. Consider rewording this to reflect the validation behavior (e.g., “validates the Azure CLI session tenant matches the provided tenant”).
- `--azure-cli`: Use an existing Azure CLI login session as the token provider. Requires Azure CLI to be installed and logged in (`az login`). Optional.
- `--tenant`: Tenant ID. Optional. When used with `--azure-cli`, pins Fabric CLI to the specified tenant.

pyproject.toml:24

  • PR description says the dependency change is azure-identity>=1.15.0, but pyproject.toml adds azure-identity>=1.25.0. If 1.25.0 is the actual minimum required, update the PR description (and any docs/release notes) to match; otherwise consider lowering the constraint to the intended minimum to avoid unnecessary bumps.
dependencies = [
    "msal[broker]>=1.34,<2 ; platform_system != 'Linux'",
    "msal>=1.34,<2",
    "msal_extensions",
    "azure-core>=1.29.0",
    "azure-identity>=1.25.0",
    "questionary",

src/fabric_cli/core/fab_auth.py:514

  • The PR description calls out an in-memory Azure CLI token cache (60s refresh buffer) and tenant cache (10s TTL), but the current implementation always calls AzureCliCredential.get_token(...) on each acquire_token and performs drift checks via JWT decoding only. If caching was intentionally dropped, the PR description should be updated; if it’s still a requirement, implement it here to avoid repeated az subprocess calls under multi-scope operations.
    def _acquire_token_from_azure_cli(self, scope: list[str]) -> dict:
        """Acquire a token using Azure CLI's AzureCliCredential.

        After acquiring the token, decodes JWT claims and verifies
        that iss, tid, and oid match the stored values from login to detect
        identity or environment drift.
        """
        stored_tenant = self.get_tenant_id()

        try:
            # Create singleton credential — no tenant pinning, inherit Azure CLI context
            if self._azure_cli_credential is None:
                self._azure_cli_credential = AzureCliCredential()
            # AzureCliCredential.get_token expects scopes as positional args
            azure_token = self._azure_cli_credential.get_token(scope[0])

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (5)

src/fabric_cli/commands/auth/fab_auth.py:83

  • The interactive Azure CLI selection path has the same ordering issue: set_access_mode("azure_cli", args.tenant) can persist an incorrect tenant before set_azure_cli validates it, leaving stale state if validation fails. Prefer set_access_mode("azure_cli") without tenant, then set_azure_cli(tenant_id=...).
            elif selected_auth.startswith("Azure CLI"):
                FabAuth().set_access_mode("azure_cli", args.tenant)
                FabAuth().set_azure_cli(tenant_id=args.tenant)
                _acquire_default_access_tokens(FabAuth())

src/fabric_cli/core/fab_auth.py:448

  • set_azure_cli() only converts CredentialUnavailableError into a FabricCLIError. Other common AzureCliCredential failures (e.g., ClientAuthenticationError/HttpResponseError when Azure CLI is logged in but token acquisition fails) will currently propagate as raw exceptions, bypassing CLI error formatting and the PR's "safe messaging" guarantees. Handle these exceptions here similarly to _acquire_token_from_azure_cli (allowlist known SDK exceptions, otherwise use the generic safe message).
        # Acquire a probe token to discover identity from JWT claims
        try:
            probe_credential = AzureCliCredential()
            probe_token = probe_credential.get_token(con.SCOPE_FABRIC_DEFAULT[0])
            claims = self._decode_jwt_claims(probe_token.token)
        except CredentialUnavailableError:
            raise FabricCLIError(
                ErrorMessages.Auth.azure_cli_not_available(),
                status_code=con.ERROR_AUTHENTICATION_FAILED,
            )

src/fabric_cli/commands/auth/fab_auth.py:35

  • In the --azure-cli flow, calling set_access_mode("azure_cli", args.tenant) persists the tenant ID before set_azure_cli() validates it against the current Azure CLI context. If the user passes a mismatched --tenant, the command will error but leave a wrong tenant/identity_type saved in auth.json, which can break subsequent commands. Consider calling set_access_mode("azure_cli") without tenant (or logging out explicitly) and letting set_azure_cli(tenant_id=...) perform validation + tenant persistence only on success.

This issue also appears on line 80 of the same file.

    if getattr(args, "azure_cli", False):
        FabAuth().set_access_mode("azure_cli", args.tenant)
        FabAuth().set_azure_cli(tenant_id=args.tenant)
        _acquire_default_access_tokens(FabAuth())

src/fabric_cli/core/fab_auth.py:514

  • PR description mentions a per-command tenant drift check via az account show plus in-memory token/tenant caches (TTL + refresh buffer) to reduce Azure CLI subprocess overhead. The implementation here appears to perform drift detection by decoding JWT claims on every AzureCliCredential.get_token call and does not include az account show or any Azure-CLI-specific caching. Please either implement the described behavior or update the PR description to match the current approach/performance characteristics.
    def _acquire_token_from_azure_cli(self, scope: list[str]) -> dict:
        """Acquire a token using Azure CLI's AzureCliCredential.

        After acquiring the token, decodes JWT claims and verifies
        that iss, tid, and oid match the stored values from login to detect
        identity or environment drift.
        """
        stored_tenant = self.get_tenant_id()

        try:
            # Create singleton credential — no tenant pinning, inherit Azure CLI context
            if self._azure_cli_credential is None:
                self._azure_cli_credential = AzureCliCredential()
            # AzureCliCredential.get_token expects scopes as positional args
            azure_token = self._azure_cli_credential.get_token(scope[0])

pyproject.toml:24

  • The PR description says the new dependency is azure-identity>=1.15.0, but pyproject.toml pins azure-identity>=1.25.0. If 1.25.0 is required (e.g., for AzureCliCredential behavior), update the PR description; otherwise consider lowering the minimum to the intended version to reduce constraints for consumers.
    "msal>=1.34,<2",
    "msal_extensions",
    "azure-core>=1.29.0",
    "azure-identity>=1.25.0",
    "questionary",

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/fabric_cli/commands/auth/fab_auth.py:83

  • Same issue in the interactive Azure CLI branch: set_access_mode("azure_cli", args.tenant) writes the tenant before set_azure_cli() validates it. A mismatched --tenant (or a stale args.tenant value) can leave the persisted auth state in a partially-updated/failed-login configuration. Prefer set_access_mode("azure_cli") without tenant and only persist the tenant after successful Azure CLI probe/validation.
            elif selected_auth.startswith("Azure CLI"):
                FabAuth().set_access_mode("azure_cli", args.tenant)
                FabAuth().set_azure_cli(tenant_id=args.tenant)
                _acquire_default_access_tokens(FabAuth())

src/fabric_cli/core/fab_auth.py:448

  • set_azure_cli() only wraps CredentialUnavailableError from the probe AzureCliCredential().get_token(...). Other Azure SDK errors (e.g., authentication/HTTP failures) will currently bubble up as raw exceptions during fab auth login --azure-cli, bypassing the centralized/safe error messaging added for _acquire_token_from_azure_cli(). Consider catching the same allowlisted Azure SDK exceptions here and re-raising FabricCLIError via ErrorMessages.Auth.azure_cli_auth_failed(...) (or a dedicated login/probe error) so login failures are consistently handled.
        # Acquire a probe token to discover identity from JWT claims
        try:
            probe_credential = AzureCliCredential()
            probe_token = probe_credential.get_token(con.SCOPE_FABRIC_DEFAULT[0])
            claims = self._decode_jwt_claims(probe_token.token)
        except CredentialUnavailableError:
            raise FabricCLIError(
                ErrorMessages.Auth.azure_cli_not_available(),
                status_code=con.ERROR_AUTHENTICATION_FAILED,
            )

src/fabric_cli/commands/auth/fab_auth.py:35

  • In the Azure CLI login path, set_access_mode("azure_cli", args.tenant) persists the requested tenant before set_azure_cli() validates it against the current Azure CLI session. If --tenant mismatches the active az login tenant, set_azure_cli() raises, but the auth state has already been updated (tenant/id-type) and prior auth logged out. Consider calling set_access_mode("azure_cli") without tenant, and let set_azure_cli(tenant_id=...) perform validation + tenant persistence only on success (or reorder so validation happens before persisting tenant).

This issue also appears on line 80 of the same file.

    if getattr(args, "azure_cli", False):
        FabAuth().set_access_mode("azure_cli", args.tenant)
        FabAuth().set_azure_cli(tenant_id=args.tenant)
        _acquire_default_access_tokens(FabAuth())

src/fabric_cli/core/fab_auth.py:514

  • PR description mentions a per-command tenant drift check via az account show, plus in-memory token caching (60s refresh buffer) and a tenant cache (10s TTL). In the current implementation, drift detection is done by decoding JWT claims from AzureCliCredential.get_token(...), and there is no token/tenant caching logic (no az account show, no TTL caches) beyond reusing a singleton credential. Please either update the PR description to match the implemented approach or add the described caching/drift-check mechanism.
    def _acquire_token_from_azure_cli(self, scope: list[str]) -> dict:
        """Acquire a token using Azure CLI's AzureCliCredential.

        After acquiring the token, decodes JWT claims and verifies
        that iss, tid, and oid match the stored values from login to detect
        identity or environment drift.
        """
        stored_tenant = self.get_tenant_id()

        try:
            # Create singleton credential — no tenant pinning, inherit Azure CLI context
            if self._azure_cli_credential is None:
                self._azure_cli_credential = AzureCliCredential()
            # AzureCliCredential.get_token expects scopes as positional args
            azure_token = self._azure_cli_credential.get_token(scope[0])

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] Support Azure Cli credential

2 participants