refactor: Add user drift detection to azure cli auth mode - #285
refactor: Add user drift detection to azure cli auth mode#285shirasassoon wants to merge 9 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
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.