Skip to content

refactor: Add user drift detection to azure cli auth mode - #285

Open
shirasassoon wants to merge 9 commits into
microsoft:mainfrom
shirasassoon:add-user-drift-detection
Open

refactor: Add user drift detection to azure cli auth mode#285
shirasassoon wants to merge 9 commits into
microsoft:mainfrom
shirasassoon:add-user-drift-detection

Conversation

@shirasassoon

Copy link
Copy Markdown
Contributor

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:

  • Added tracking of both tenant and principal IDs (FAB_TENANT_ID, FAB_PRINCIPAL_ID) in authentication state, and implemented _check_azure_cli_identity to detect and respond to identity drift by logging out, clearing caches, and resetting context. [1] [2] [3]
  • Updated _acquire_token_from_azure_cli to validate both tenant and principal IDs from the acquired token, and to call the new drift detection logic. [1] [2]
  • Improved error handling and user messaging for missing or changed identity claims, adding new error messages for these scenarios.

Authentication status and logout flow improvements:

  • Refactored the status command to properly handle cases where identity information is missing or has changed, ensuring accurate reporting of login state. [1] [2] [3]
  • Ensured that logout and cache clearing are triggered and tested when identity drift is detected.

Test coverage enhancements:

  • Added comprehensive tests for all Azure CLI identity drift scenarios (tenant, principal, or both changing), for missing claims, and for baseline establishment and updates.
  • Updated test fixtures and status command tests to reflect the new identity tracking and error handling. [1] [2] [3]

These changes make the authentication experience more reliable and secure, preventing accidental use of cached credentials after an Azure CLI identity change.

@shirasassoon
shirasassoon requested a review from a team as a code owner September 6, 2026 18:57
Copilot AI lite review requested due to automatic review settings September 6, 2026 18:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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_ID tracking and _check_azure_cli_identity(...) to detect tenant/principal drift and clear state on change.
  • Update auth status to 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.

Comment thread src/fabric_cli/commands/auth/fab_auth.py
Comment thread src/fabric_cli/core/fab_auth.py Outdated
Copilot AI review requested due to automatic review settings September 7, 2026 07:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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

Copilot AI review requested due to automatic review settings September 7, 2026 08:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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

Copilot AI review requested due to automatic review settings September 8, 2026 06:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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

Comment thread src/fabric_cli/commands/auth/fab_auth.py Outdated
Copilot AI review requested due to automatic review settings September 8, 2026 09:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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

Comment thread src/fabric_cli/core/fab_auth.py
Copilot AI review requested due to automatic review settings September 9, 2026 09:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants