refactor: Add user drift detection to azure cli auth mode - #285
shirasassoon wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
It changes security-sensitive authentication behavior and there is a correctness issue in auth status output consistency when identity drift occurs mid-execution.
Pull request overview
This PR hardens the Azure CLI authentication mode by persisting both tenant and principal identity baselines and detecting “identity drift” mid-session, triggering a forced logout + cache/context reset to prevent use of stale credentials. It also updates the fab auth status flow and expands unit/command test coverage for these drift scenarios.
Changes:
- Add
FAB_PRINCIPAL_IDtracking and_check_azure_cli_identity(...)to detect tenant/principal drift and clear state on change. - Update
auth statusto better handle identity loss/drift during token inspection/masking. - Extend tests/fixtures to validate baseline establishment, drift cases (tenant/principal/both), and status output behavior.
File summaries
| File | Description |
|---|---|
src/fabric_cli/core/fab_constant.py |
Adds FAB_PRINCIPAL_ID constant for persisted identity baseline tracking. |
src/fabric_cli/core/fab_auth.py |
Implements Azure CLI identity drift detection and state reset behavior. |
src/fabric_cli/errors/auth.py |
Adds new user-facing error messages for missing identity claims and identity changes. |
src/fabric_cli/commands/auth/fab_auth.py |
Refactors status to account for identity drift during token retrieval/masking. |
tests/conftest.py |
Updates Azure CLI auth fixture JWT claims to include both tid and oid. |
tests/test_core/test_fab_auth_azure_cli.py |
Adds comprehensive drift/baseline tests for Azure CLI auth mode. |
tests/test_commands/test_auth.py |
Extends logout and status command tests for cache-clearing and drift behavior. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ssoon/fabric-cli into add-user-drift-detection
There was a problem hiding this comment.
🔵 Needs a closer look
It modifies authentication (a security-sensitive area in this repo) and alters logout/status behavior, so it warrants final human review despite strong test coverage.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It modifies authentication behavior in security-sensitive areas (Azure CLI token acquisition/logout/status flows) and should receive final team review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
auth status currently clears decoded token info whenever identity_type is None, which can suppress valid status details in non-drift scenarios and should be scoped to the Azure CLI drift case.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
…ssoon/fabric-cli into add-user-drift-detection
There was a problem hiding this comment.
🔵 Needs a closer look
It changes security-sensitive authentication flow and includes a correctness issue around cache invalidation that should be addressed and re-validated by a human reviewer.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It changes security-sensitive authentication behavior (a restricted area per AGENTS.md), so it should receive final team review despite strong test coverage.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/fabric_cli/errors/auth.py:150
- The error message uses backticks around the suggested command (``Run
fab auth login --azure-cli```), which is inconsistent with nearby Azure CLI guidance using single quotes (e.g.,Run 'az login' ...`). Backticks are also shell syntax (command substitution) in many shells, so users copying/pasting the full message can get unexpected behavior. Prefer single quotes (or no markup) for CLI instructions.
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
| raise FabricCLIError( | ||
| ErrorMessages.Auth.azure_cli_identity_changed(), | ||
| status_code=con.ERROR_AUTHENTICATION_FAILED, | ||
| ) |
There was a problem hiding this comment.
why we raise an error?
There was a problem hiding this comment.
We raise error to prevent the command from completing when identity drift it detected. Without it, even when identity drift is detected, if you do fab ls it will return the items in the workspace of the new identity.
| self._auth_info.pop(con.FAB_TENANT_ID, None) | ||
| self._auth_info.pop(con.FAB_PRINCIPAL_ID, None) | ||
| self._azure_cli_credential = None | ||
| self._save_auth() |
There was a problem hiding this comment.
do we want to remove all auth_info? consider using self.logout() method. note that if we choose to use self.logout(), it also removes the msal's cache.bin file and resets the config. perhaps we can extract the common code (reset auth file, save auth, set azure_cli_cred to None) to another method and use both, or add a new argument to logout method that tells it to skip remove cache and config..
There was a problem hiding this comment.
This function is used in the following context: when the user explicitly run fab auth login --azure-cli again while already authenticated previously with --azure-cli. It clears the old tenant/principal baseline and cached AzureCliCredential, allowing _acquire_default_access_tokens() to establish current az login identity as the new baseline instead of throwing an identity drift error.
When switching from another authentication mode to Azure CLI, the first branch calls the full logout() instead so this function isn't called.
| if context.get_runtime_mode() == FAB_MODE_INTERACTIVE: | ||
| from fabric_cli.core.fab_auth import FabAuth | ||
|
|
||
| FabAuth().validate_azure_cli_identity() |
There was a problem hiding this comment.
can you explain this change?
There was a problem hiding this comment.
Interactive mode keeps authentication, cache, and hierarchy context alive across commands. The user may change their Azure CLI account or tenant in another terminal during that session. This check detects the change before the next command runs, preventing use of stale identity-specific context.
There was a problem hiding this comment.
🔵 Needs a closer look
Several unresolved moderate findings affect authentication state, context handling, environment-token status, and interactive error handling.
Review details
Suppressed comments (5)
src/fabric_cli/core/fab_auth.py:545
- This new
FabAuthhelper is missing a return type annotation, while the repository guidance requires type hints for all functions (AGENTS.md:72) and the adjacent new helpers are annotated. Add-> Nonefor consistency with the stated typing standard.
def _clear_azure_cli_identity_baseline(self):
src/fabric_cli/core/fab_auth.py:543
- When only the new principal baseline is missing,
auth_propertiesis still non-empty, so this unconditional assignment resets an existing workspace/folder context to the tenant even though the tenant has not changed. In command-line mode with context persistence enabled, it also overwrites the saved context path on the first command after upgrading an older Azure CLI auth file. Update the context only when the tenant baseline is newly established (or the tenant actually changed).
if auth_properties:
self._set_auth_properties(auth_properties)
Context().context = self.get_tenant()
src/fabric_cli/core/fab_auth.py:527
Context.reset_context()does not reload the tenant fromFabAuth; it assignsself.context.tenant(src/fabric_cli/core/fab_context.py:85-87). Sinceself.logout()has cleared auth state but leaves the current context object intact, this call retains the old tenant after identity drift instead of resetting to the logged-out tenant. Subsequent path resolution can therefore continue using the previous tenant until the next login; assignContext().context = self.get_tenant()after the cleanup, or update the reset semantics.
Context().reset_context()
src/fabric_cli/core/fab_decorators.py:75
- This preflight can raise
FabricCLIErrorbefore the wrapped command body runs. Thedescribecommand is only decorated withset_command_context, while the interactive loop treats an uncaught exception as fatal and exits, so identity drift duringdescribeterminates the REPL instead of using the normal authentication error path. Wrap that command withhandle_exceptions, or move the error handling into this decorator.
if context.get_runtime_mode() == FAB_MODE_INTERACTIVE:
from fabric_cli.core.fab_auth import FabAuth
FabAuth().validate_azure_cli_identity()
src/fabric_cli/errors/auth.py:146
- These new user-facing authentication errors and identity-drift behavior are not accompanied by a new
.changes/unreleasedentry. AGENTS.md:58-64 requires every PR to add a changie entry, so please add the appropriate release-note file for this change.
def azure_cli_identity_changed() -> str:
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Critical and moderate issues, plus the required changie entry, remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
src/fabric_cli/core/fab_decorators.py:75
- This validation runs before the decorated function body, so its
FabricCLIErroris only handled when the caller also has@handle_exceptions.describeis registered with only@set_command_context(), and its internaltrystarts after this wrapper, so an Azure CLI drift while running interactivedescribeescapes toInteractiveCLI.start_interactiveand terminates the REPL instead of reporting the auth error and continuing. Apply the validation through a path all interactive commands handle, or add the standard error wrapper to this unwrapped command.
FabAuth().validate_azure_cli_identity()
src/fabric_cli/core/fab_decorators.py:75
- This validation runs for every command using
@set_command_context, including the localconfighandlers (config get,config set,config ls, and especiallyconfig clear-cache). When Azure CLI mode is selected butazis unavailable or its identity has drifted,validate_azure_cli_identity()raises before the local handler runs, preventing users from changing configuration or clearing caches to recover. Exempt local config commands (or move the validation to only commands that require a remote Fabric token).
if context.get_runtime_mode() == FAB_MODE_INTERACTIVE:
from fabric_cli.core.fab_auth import FabAuth
FabAuth().validate_azure_cli_identity()
src/fabric_cli/errors/auth.py:149
- The repository contribution guide requires every PR to add a changie entry under
.changes/unreleased(AGENTS.md:58-64), but the presented changes contain no entry for this user-visible Azure CLI identity-drift behavior. Please add the appropriate release-note entry before merging.
def azure_cli_identity_changed() -> str:
return (
"Fabric CLI logged out due to change in Azure CLI identity. "
"Run `fab auth login --azure-cli` to re-authenticate with the current Azure CLI identity"
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
| # Logout, clear caches, and reset context before raising an error | ||
| self.logout() | ||
| fab_mem_store.clear_caches() | ||
| Context().reset_context() |
This pull request introduces robust detection and handling of Azure CLI identity changes in the authentication flow, ensuring that the Fabric CLI logs out and clears cached state if the Azure CLI tenant or principal changes mid-session. It also extends test coverage for these scenarios and improves error messaging for identity drift cases.
Azure CLI identity drift detection and handling:
FAB_TENANT_ID,FAB_PRINCIPAL_ID) in authentication state, and implemented_check_azure_cli_identityto detect and respond to identity drift by logging out, clearing caches, and resetting context. [1] [2] [3]_acquire_token_from_azure_clito validate both tenant and principal IDs from the acquired token, and to call the new drift detection logic. [1] [2]Authentication status and logout flow improvements:
statuscommand to properly handle cases where identity information is missing or has changed, ensuring accurate reporting of login state. [1] [2] [3]Test coverage enhancements:
These changes make the authentication experience more reliable and secure, preventing accidental use of cached credentials after an Azure CLI identity change.