feat: Add Azure CLI auth source - #265
Conversation
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>
There was a problem hiding this comment.
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_clias an allowed identity type and implement Azure CLI token acquisition inFabAuth. - Add
--azure-cliflag plus an “Azure CLI” interactive login option; includeauth_sourcein auth status output. - Add
azure-identity>=1.15.0dependency 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. |
There was a problem hiding this comment.
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-cliis 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
@singletoninfabric_cli.core.fab_auth). These tests patchconfig_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>
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
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
FabricCLIErrormessages as hardcoded strings. Elsewhere in this module, auth failures useErrorMessages.Auth.*()helpers for consistent wording and easier localization/maintenance. Consider moving these new messages intoErrorMessages.Authand 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, butFabAuthis a singleton (via the@singletondecorator). 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 thesingletondecorator 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-clicurrently 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>
There was a problem hiding this comment.
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
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>
There was a problem hiding this comment.
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-decoratedFabAuth (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."""
There was a problem hiding this comment.
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_authfixture contains fragile/incorrect singleton-reset logic (e.g., readingsingleton.__code__.co_constsand patchingFabAuth.__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
FabricCLIErrorwith a hardcoded message. In this codebase, auth errors are consistently sourced fromErrorMessages.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_instancefixture tries to reset theFabAuthsingleton via__wrapped__, butFabAuthis a custom@singletonwrapper (a closure) and neitherFabAuth.__wrapped__norfabric_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>
There was a problem hiding this comment.
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()persistsaccount["principal_name"](often an email/UPN) into the auth file underFAB_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
- 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>
There was a problem hiding this comment.
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_timeare not used byFabAuth(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 onforce_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_tenantfields that don’t exist inFabAuth, so the setup is ineffective/misleading. Use_cached_az_account/_cached_az_account_timefor 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, butFabAuthcaches 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, butFabAuthdoesn’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_accountand_cached_az_account_timeinstead.
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
FabAuthuses_cached_az_account/_cached_az_account_timefor the Azure CLI account cache;_cached_az_tenantand_cached_az_tenant_timearen’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_accountand_cached_az_account_timeinstead.
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 fromaz account show’suser.name(typically a UPN/email), and this value is persisted intoauth.jsonunderFAB_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_IDis used to store Azure CLIuser.name(a principal name/UPN), not a stable principal/object ID. Renaming this constant/key to something likeFAB_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, butpyproject.tomlpins it toazure-identity>=1.25.0. If 1.25.0 is intentional (e.g., required forAzureCliCredentialbehavior 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",
…sassoon/fabric-cli into feature/azure-cli-auth-poc
There was a problem hiding this comment.
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
argparseis 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, butFabAuthactually 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_IDis used to store the Azure CLIuserName(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()persistsaccount["principal_name"](typically a user UPN/email) intoauth.jsonfor 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()callsAzureCliCredential.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>
There was a problem hiding this comment.
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. Whenlen(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)) % 4and 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 translatesCredentialUnavailableErrorinto aFabricCLIError. Other Azure Identity failures (e.g., invalid tenant, auth failures, transient errors) will bubble up as raw exceptions duringfab 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 JWTtid/oidfromAzureCliCredential.get_token(...)and does not implementaz account showchecks 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 isazure-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 (perfab_auth_parser.py) is-u/--username. This mismatch will confuse users copying the docs; update the docs to use--usernameconsistently.
- `-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
argparseis imported but never used in this test module. Removing the unused import will keep the test file minimal and avoid implyingargparseis 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>
There was a problem hiding this comment.
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 showplus in-memory token/tenant caches (60s refresh buffer, 10s tenant TTL) andshutil.which("az")handling. None of that behavior appears to be implemented in fab_auth.py (no subprocess/azinvocation, 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])
| # 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>
There was a problem hiding this comment.
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
argparseis 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_claimspads base64 with"=" * (4 - len(payload) % 4), which adds 4 '=' when the length is already a multiple of 4 and can break decoding. Also,urlsafe_b64decodecan raisebinascii.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_clionly wrapsCredentialUnavailableError. Other Azure CLI / azure-identity failures during the probeget_token(e.g., auth failures, response errors) will bubble up as raw exceptions, bypassing the safe/structuredFabricCLIErrorpath 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 withazure_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 usesAzureCliCredential.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, butpyproject.tomlpins it asazure-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>
There was a problem hiding this comment.
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 viaaz 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--tenantis ignored for Azure CLI auth (and users must useaz 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`.
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>
There was a problem hiding this comment.
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). Whenlen(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)) % 4so 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 theraise). 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 theraise). 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 showplus 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 byAzureCliCredential.get_token(...), and there is noaz account showsubprocess 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>
There was a problem hiding this comment.
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",
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>
There was a problem hiding this comment.
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 argparseis 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, andshutil.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/azprobing). 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>
There was a problem hiding this comment.
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 orazsubprocess 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
…sassoon/fabric-cli into feature/azure-cli-auth-poc
There was a problem hiding this comment.
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 beforeset_azure_cli()validates it, leaving persisted auth state inconsistent if validation fails. Preferset_access_mode("azure_cli")without tenant here as well, and rely onset_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 beforeset_azure_cli()validates it. Ifset_azure_cli()then raises a mismatch error, the auth state can be left with an incorrect tenant/identity_type even though login failed. Consider callingset_access_mode("azure_cli")(no tenant) first (to trigger logout), then lettingset_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 catchesCredentialUnavailableError. Other Azure Identity failures (e.g.,ClientAuthenticationError,HttpResponseError) will currently bubble up as raw exceptions (notFabricCLIError), 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 safeFabricCLIErrormessage (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
--tenantused 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, butpyproject.tomladdsazure-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 eachacquire_tokenand 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 repeatedazsubprocess 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])
There was a problem hiding this comment.
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 showplus 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 includeaz account showor 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 pinsazure-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>
There was a problem hiding this comment.
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 beforeset_azure_cli()validates it. A mismatched--tenant(or a staleargs.tenantvalue) can leave the persisted auth state in a partially-updated/failed-login configuration. Preferset_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 wrapsCredentialUnavailableErrorfrom the probeAzureCliCredential().get_token(...). Other Azure SDK errors (e.g., authentication/HTTP failures) will currently bubble up as raw exceptions duringfab 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-raisingFabricCLIErrorviaErrorMessages.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 beforeset_azure_cli()validates it against the current Azure CLI session. If--tenantmismatches the activeaz logintenant,set_azure_cli()raises, but the auth state has already been updated (tenant/id-type) and prior auth logged out. Consider callingset_access_mode("azure_cli")withouttenant, and letset_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 fromAzureCliCredential.get_token(...), and there is no token/tenant caching logic (noaz 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])
Summary
Add
azure_clias a new identity type that delegates token acquisition to Azure CLI viaazure-identity'sAzureCliCredential. This allows tools callingfabto reuse an existingaz loginsession instead of requiring a separate interactivefab auth login.Changes
Core auth (
src/fabric_cli/core/fab_auth.py)azure_clitoAUTH_KEYSidentity type allowlist_acquire_token_from_azure_cli()usingAzureCliCredentialaz account showaz account showcalls during multi-scope operationsset_azure_cli()clears token cache on every login to prevent cross-tenant stale tokensshutil.which("az")for Windows compatibility (az.cmdresolution)Command handler (
src/fabric_cli/commands/auth/fab_auth.py)--azure-cliflag tofab auth loginauth_sourceinfab auth statusoutputError handling (
src/fabric_cli/errors/auth.py)ClientAuthenticationError,HttpResponseError,ServiceRequestError,ServiceResponseErrorsurfaced verbatim (pre-sanitized by azure-identity SDK)Documentation
docs/commands/auth/index.md—--azure-cliflag in command reference with separate usage blocks per auth methoddocs/examples/auth_examples.md— Azure CLI auth examples with tenant behavior noteDependencies
azure-identity>=1.15.0Tests
43 automated tests across 4 files:
test_fab_auth_azure_cli.pytest_fab_auth_parser.py--azure-cliflag mappingtest_fab_msal_bridge_azure_cli.pyMsalTokenCredentialdispatch with azure_cli identitytest_fab_auth_command_azure_cli.pyKey test scenarios:
AzureCliCredential, azure_cli doesn't invoke MSALaz.cmdresolution viashutil.whichSecurity
shutil.which("az")resolves absolute path before subprocess (matches azure-identity SDK pattern)shell=Falsefor all subprocess calls — no injection riskOpen questions for team
az logoutconfig persistence: When Azure CLI session is interrupted and restored with the same tenant, fab silently resumes. Should fab require explicit re-authentication instead?