From 377dd24560e7f9b0f22dede38399a73b062820c7 Mon Sep 17 00:00:00 2001 From: Shira Sassoon Date: Sun, 6 Sep 2026 21:50:02 +0300 Subject: [PATCH 1/5] add user drift detection to azure cli auth mode + refine logout behavior --- src/fabric_cli/commands/auth/fab_auth.py | 17 +- src/fabric_cli/core/fab_auth.py | 70 +++++- src/fabric_cli/core/fab_constant.py | 1 + src/fabric_cli/errors/auth.py | 11 + tests/conftest.py | 6 +- tests/test_commands/test_auth.py | 75 +++++- tests/test_core/test_fab_auth_azure_cli.py | 272 +++++++++++++++++++-- 7 files changed, 401 insertions(+), 51 deletions(-) diff --git a/src/fabric_cli/commands/auth/fab_auth.py b/src/fabric_cli/commands/auth/fab_auth.py index ca0a2be8..1b6042cf 100644 --- a/src/fabric_cli/commands/auth/fab_auth.py +++ b/src/fabric_cli/commands/auth/fab_auth.py @@ -220,8 +220,6 @@ def logout(args: Namespace) -> None: def status(args: Namespace) -> None: auth = FabAuth() - identity_type = auth.get_identity_type() - tenant_id = auth.get_tenant_id() def __get_token_info(scope): try: @@ -240,11 +238,6 @@ def __get_token_info(scope): token_info = __get_token_info(fab_constant.SCOPE_FABRIC_DEFAULT) - upn = token_info.get("upn") or "N/A" - oid = token_info.get("oid") or "N/A" - tid = token_info.get("tid", tenant_id) or "N/A" - appid = token_info.get("appid") or "N/A" - def __mask_token(scope): try: token = auth.get_access_token(scope, interactive_renew=False) @@ -268,6 +261,16 @@ def __mask_token(scope): storage_secret = __mask_token(fab_constant.SCOPE_ONELAKE_DEFAULT) azure_secret = __mask_token(fab_constant.SCOPE_AZURE_DEFAULT) + identity_type = auth.get_identity_type() + tenant_id = auth.get_tenant_id() + if identity_type is None: + token_info = {} + + upn = token_info.get("upn") or "N/A" + oid = token_info.get("oid") or "N/A" + tid = token_info.get("tid", tenant_id) or "N/A" + appid = token_info.get("appid") or "N/A" + # Check login status is_logged_in = fabric_secret != "N/A" login_status = ( diff --git a/src/fabric_cli/core/fab_auth.py b/src/fabric_cli/core/fab_auth.py index bf8ee275..28f43103 100644 --- a/src/fabric_cli/core/fab_auth.py +++ b/src/fabric_cli/core/fab_auth.py @@ -421,10 +421,10 @@ def set_managed_identity(self, client_id=None): ) def _acquire_token_from_azure_cli(self, scope: list[str]) -> dict: - """Acquire a token using the current Azure CLI authentication context. + """Acquire a token and validate its identity against the stored baseline. - Synchronizes Fabric CLI's tenant, resource caches, and command context - with the tenant claim in the acquired token. + Records the tenant and principal on first use. If either identity later + changes, logs out and clears cached Fabric CLI state. """ from azure.core.exceptions import ( ClientAuthenticationError, @@ -440,11 +440,9 @@ def _acquire_token_from_azure_cli(self, scope: list[str]) -> dict: # AzureCliCredential.get_token expects scopes as positional args azure_token = self._azure_cli_credential.get_token(scope[0]) - # Keep tenant-scoped context and caches aligned with Azure CLI + # Extract claims from the acquired token to determine the current Azure CLI identity claims = self._decode_jwt_token(azure_token.token) - tid = claims.get("tid") - if tid and tid != self.get_tenant_id(): - self._synchronize_azure_cli_tenant(tid) + self._check_azure_cli_identity(claims) token_result = { "access_token": azure_token.token, @@ -478,14 +476,62 @@ def _acquire_token_from_azure_cli(self, scope: list[str]) -> dict: status_code=con.ERROR_AUTHENTICATION_FAILED, ) - def _synchronize_azure_cli_tenant(self, tenant_id: str) -> None: - """Synchronize state when the active Azure CLI tenant changes.""" + def _check_azure_cli_identity(self, claims: dict) -> None: + """Records Azure CLI tenant and principal IDs and rejects identity drift.""" from fabric_cli.core.fab_context import Context from fabric_cli.utils import fab_mem_store - self._set_auth_properties({con.FAB_TENANT_ID: tenant_id}) - fab_mem_store.clear_caches() - Context().context = self.get_tenant() + # Get the tenant and principal IDs from the claims + tenant_id = claims.get("tid") + principal_id = claims.get("oid") + + if tenant_id is None or principal_id is None: + raise FabricCLIError( + ErrorMessages.Auth.azure_cli_identity_claims_missing(), + status_code=con.ERROR_AUTHENTICATION_FAILED, + ) + + # Get the current tenant and principal IDs from the auth context + current_tenant_id = self.get_tenant_id() + current_principal_id = self._get_auth_property(con.FAB_PRINCIPAL_ID) + + # Determine if there is a identity drift (tenant or principal) + tenant_drifted = ( + current_tenant_id is not None and tenant_id != current_tenant_id + ) + principal_drifted = ( + current_principal_id is not None and principal_id != current_principal_id + ) + # If any drift is detected, set the changed identity description + if tenant_drifted or principal_drifted: + if tenant_drifted and principal_drifted: + changed_identity = "Tenant ID and Principal ID" + elif tenant_drifted: + changed_identity = "Tenant ID" + else: + changed_identity = "Principal ID" + fab_logger.log_warning(f"Change detected in Azure CLI {changed_identity}") + + # Logout, clear caches, and reset context before raising an error + self.logout() + fab_mem_store.clear_caches() + Context().reset_context() + raise FabricCLIError( + ErrorMessages.Auth.azure_cli_identity_changed(), + status_code=con.ERROR_AUTHENTICATION_FAILED, + ) + + # Record the initial Azure CLI identity as the baseline for future drift checks + auth_properties: dict[str, str] = {} + + if current_tenant_id is None: + auth_properties[con.FAB_TENANT_ID] = tenant_id + if current_principal_id is None: + auth_properties[con.FAB_PRINCIPAL_ID] = principal_id + if auth_properties: + self._set_auth_properties(auth_properties) + if current_tenant_id is None: + Context().context = self.get_tenant() def print_auth_info(self): utils_ui.print_grey(json.dumps(self._get_auth_info(), indent=2)) diff --git a/src/fabric_cli/core/fab_constant.py b/src/fabric_cli/core/fab_constant.py index d883bb05..95a02f1a 100644 --- a/src/fabric_cli/core/fab_constant.py +++ b/src/fabric_cli/core/fab_constant.py @@ -54,6 +54,7 @@ FAB_SPN_CERT_PASSWORD = "fab_spn_cert_password" FAB_SPN_FEDERATED_TOKEN = "fab_spn_federated_token" FAB_TENANT_ID = "fab_tenant_id" +FAB_PRINCIPAL_ID = "fab_principal_id" FAB_REFRESH_TOKEN = "fab_refresh_token" IDENTITY_TYPE = "identity_type" diff --git a/src/fabric_cli/errors/auth.py b/src/fabric_cli/errors/auth.py index 52640f6a..e32701c2 100644 --- a/src/fabric_cli/errors/auth.py +++ b/src/fabric_cli/errors/auth.py @@ -138,6 +138,17 @@ def azure_cli_token_acquisition_failed() -> str: "Run 'az login' to authenticate, then retry" ) + @staticmethod + def azure_cli_identity_claims_missing() -> str: + return "Azure CLI token is missing required tenant or principal identity claims" + + @staticmethod + 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" + ) + @staticmethod def incompatible_authentication_arguments(arguments: list[str]) -> str: return f"Authentication arguments cannot be combined: {', '.join(arguments)}" diff --git a/tests/conftest.py b/tests/conftest.py index 176b8035..1b96f0fe 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -117,7 +117,11 @@ def azure_cli_auth_fixture(monkeypatch, tmp_path): auth = FabAuth() monkeypatch.setattr(auth, "auth_file", str(tmp_path / "auth.json")) monkeypatch.setattr(auth, "cache_file", str(tmp_path / "cache.bin")) - monkeypatch.setattr(auth, "_decode_jwt_token", lambda _: {"tid": "test-tenant"}) + monkeypatch.setattr( + auth, + "_decode_jwt_token", + lambda _: {"tid": "test-tenant", "oid": "test-principal"}, + ) auth._azure_cli_credential = None auth._auth_info = {} auth.app = None diff --git a/tests/test_commands/test_auth.py b/tests/test_commands/test_auth.py index f4208cc3..f70313ed 100644 --- a/tests/test_commands/test_auth.py +++ b/tests/test_commands/test_auth.py @@ -881,9 +881,13 @@ def test_auth_logout( args = argparse.Namespace() # Act - fab_auth.logout(args) + with patch.object( + fab_auth.utils_mem_store, "clear_caches" + ) as mock_clear_caches: + fab_auth.logout(args) # Assert + mock_clear_caches.assert_called_once_with() mock_fab_context_instance = mock_fab_context.get("instance") mock_fab_context_instance.reset_context.assert_called_once() @@ -919,14 +923,18 @@ def test_auth_status(self, mock_fab_auth, capsys): auth_subcommand="status", output_format="text", ) - with patch( - "fabric_cli.commands.auth.fab_auth._get_token_info_from_bearer_token", - return_value={ - "appid": "mocked_appid", - "upn": "mocked_upn", - "oid": "mocked_oid", - "tid": "mocked_tenant_id", - }, + auth = mock_fab_auth["instance"] + with ( + patch.object(auth, "get_identity_type", return_value="user"), + patch( + "fabric_cli.commands.auth.fab_auth._get_token_info_from_bearer_token", + return_value={ + "appid": "mocked_appid", + "upn": "mocked_upn", + "oid": "mocked_oid", + "tid": "mocked_tenant_id", + }, + ), ): # Act fab_auth.status(args) @@ -989,6 +997,55 @@ def test_auth_status_azure_cli_session_unavailable(self, mock_fab_auth, capsys): assert "Azure CLI Session: Unavailable" in captured.out assert "Logged In: False" in captured.out + def test_auth_status_identity_drift_during_token_masking( + self, mock_fab_auth, capsys + ): + """Status should report logged-out state after Azure CLI identity drift.""" + args = argparse.Namespace( + command="auth", + auth_subcommand="status", + output_format="text", + ) + auth = mock_fab_auth["instance"] + auth._auth_info = { + fab_constant.IDENTITY_TYPE: "azure_cli", + fab_constant.FAB_TENANT_ID: "previous-tenant", + } + auth.get_tenant_id.side_effect = lambda: auth._auth_info.get( + fab_constant.FAB_TENANT_ID + ) + + def get_access_token(*args, **kwargs): + if auth.get_access_token.call_count == 1: + return "mocked_access_token" + auth._auth_info = {} + raise FabricCLIError( + ErrorMessages.Auth.azure_cli_identity_changed(), + fab_constant.ERROR_AUTHENTICATION_FAILED, + ) + + auth.get_access_token.side_effect = get_access_token + + with patch( + "fabric_cli.commands.auth.fab_auth._get_token_info_from_bearer_token", + return_value={"tid": "previous-tenant"}, + ): + fab_auth.status(args) + + captured = capsys.readouterr() + assert "Not logged in to app.fabric.microsoft.com" in captured.err + assert "Authentication Mode: Azure CLI" not in captured.out + assert "Azure CLI Session:" not in captured.out + assert "Account: N/A" in captured.out + assert "Principal ID: N/A" in captured.out + assert "Tenant ID: N/A" in captured.out + assert "App ID: N/A" in captured.out + assert "Token Fabric PowerBI: N/A" in captured.out + assert "Token Storage: N/A" in captured.out + assert "Token Azure: N/A" in captured.out + assert "Logged In: False" in captured.out + assert "previous-tenant" not in captured.out + def test_init_when_user_cancels_the_prompt( self, mock_fab_auth, mock_fab_context, mock_fab_logger_log_warning, capsys ): diff --git a/tests/test_core/test_fab_auth_azure_cli.py b/tests/test_core/test_fab_auth_azure_cli.py index 6585dc06..575a1053 100644 --- a/tests/test_core/test_fab_auth_azure_cli.py +++ b/tests/test_core/test_fab_auth_azure_cli.py @@ -38,50 +38,263 @@ def test_set_access_mode_accepts_azure_cli_success(self, azure_cli_auth_fixture) auth.set_access_mode("azure_cli") assert auth.get_identity_type() == "azure_cli" + @pytest.mark.parametrize( + "claims", + [ + {}, + {"tid": "tenant-A"}, + {"oid": "principal-A"}, + ], + ) + def test_check_azure_cli_identity_missing_claims_failure( + self, claims, azure_cli_auth_fixture + ): + """Missing tenant or principal claims should fail authentication.""" + auth = FabAuth() + auth._set_auth_properties( + { + con.IDENTITY_TYPE: "azure_cli", + con.FAB_TENANT_ID: "tenant-A", + con.FAB_PRINCIPAL_ID: "principal-A", + } + ) + + with pytest.raises(FabricCLIError) as exc_info: + auth._check_azure_cli_identity(claims) + + assert ErrorMessages.Auth.azure_cli_identity_claims_missing() in str( + exc_info.value + ) + assert exc_info.value.status_code == con.ERROR_AUTHENTICATION_FAILED + assert auth.get_identity_type() == "azure_cli" + assert auth.get_tenant_id() == "tenant-A" + assert auth._get_auth_property(con.FAB_PRINCIPAL_ID) == "principal-A" + + def test_unchanged_identity_does_not_reset_state_success( + self, azure_cli_auth_fixture + ): + """A matching identity should not rewrite or clear authentication state.""" + auth = FabAuth() + auth._set_auth_properties( + { + con.IDENTITY_TYPE: "azure_cli", + con.FAB_TENANT_ID: "tenant-A", + con.FAB_PRINCIPAL_ID: "principal-A", + } + ) + + with ( + patch.object(auth, "_set_auth_properties") as mock_set_properties, + patch.object(auth, "logout") as mock_logout, + patch.object(fab_mem_store, "clear_caches") as mock_clear_caches, + patch.object(Context(), "reset_context") as mock_reset_context, + ): + auth._check_azure_cli_identity({"tid": "tenant-A", "oid": "principal-A"}) + + mock_set_properties.assert_not_called() + mock_logout.assert_not_called() + mock_clear_caches.assert_not_called() + mock_reset_context.assert_not_called() + assert auth.get_identity_type() == "azure_cli" + assert auth.get_tenant_id() == "tenant-A" + assert auth._get_auth_property(con.FAB_PRINCIPAL_ID) == "principal-A" + + @pytest.mark.parametrize( + ("stored_properties", "expected_tenant", "expected_principal"), + [ + ( + {con.FAB_TENANT_ID: "tenant-A"}, + "tenant-A", + "principal-A", + ), + ( + {con.FAB_PRINCIPAL_ID: "principal-A"}, + "tenant-A", + "principal-A", + ), + ], + ) + def test_partial_identity_baseline_is_completed_success( + self, + stored_properties, + expected_tenant, + expected_principal, + azure_cli_auth_fixture, + ): + """A partial stored baseline should be completed from matching claims.""" + auth = FabAuth() + auth._set_auth_properties(stored_properties) + + auth._check_azure_cli_identity({"tid": "tenant-A", "oid": "principal-A"}) + + assert auth.get_tenant_id() == expected_tenant + assert auth._get_auth_property(con.FAB_PRINCIPAL_ID) == expected_principal + @patch("fabric_cli.core.fab_auth.AzureCliCredential") - def test_first_token_acquisition_stores_tenant_success( + def test_first_token_acquisition_stores_identity_success( self, mock_credential_class, azure_cli_auth_fixture ): - """First token acquisition should discover and store tenant from JWT.""" + """First token acquisition should store tenant and principal from JWT.""" _mock_credential(mock_credential_class) auth = FabAuth() auth.set_access_mode("azure_cli") auth._azure_cli_credential = None assert auth.get_tenant_id() is None - with patch.object( - auth, "_decode_jwt_token", return_value={"tid": "discovered-tenant"} + with ( + patch.object( + auth, + "_decode_jwt_token", + return_value={"tid": "discovered-tenant", "oid": "principal-A"}, + ), + patch.object(auth, "logout", wraps=auth.logout) as mock_logout, ): auth._acquire_token_from_azure_cli(con.SCOPE_FABRIC_DEFAULT) assert auth.get_tenant_id() == "discovered-tenant" + assert auth._get_auth_property(con.FAB_PRINCIPAL_ID) == "principal-A" + mock_logout.assert_not_called() @patch("fabric_cli.core.fab_auth.AzureCliCredential") def test_tenant_updated_on_subsequent_calls_success( self, mock_credential_class, azure_cli_auth_fixture ): - """A changed Azure CLI tenant should reset context and cached resources.""" + """A changed Azure CLI tenant should log out and reset cached state.""" mock_credential, _ = _mock_credential(mock_credential_class) auth = FabAuth() auth.set_access_mode("azure_cli") auth._azure_cli_credential = None - with patch.object( - auth, - "_decode_jwt_token", - side_effect=[{"tid": "original-tenant"}, {"tid": "new-tenant"}], + context = Context() + with ( + patch.object( + auth, + "_decode_jwt_token", + side_effect=[ + {"tid": "original-tenant", "oid": "principal-A"}, + {"tid": "new-tenant", "oid": "principal-A"}, + ], + ), + patch.object(auth, "logout", wraps=auth.logout) as mock_logout, + patch.object( + fab_mem_store, "clear_caches", wraps=fab_mem_store.clear_caches + ) as mock_clear_caches, + patch.object( + context, "reset_context", wraps=context.reset_context + ) as mock_reset_context, + patch("fabric_cli.core.fab_auth.fab_logger.log_warning") as mock_warning, ): auth._acquire_token_from_azure_cli(con.SCOPE_FABRIC_DEFAULT) fab_mem_store._get_workspaces_from_cache.cache.update({"key": "value"}) fab_mem_store._get_workspace_folders_from_cache.cache.update( {"key": "value"} ) + with pytest.raises(FabricCLIError) as exc_info: + auth._acquire_token_from_azure_cli(con.SCOPE_FABRIC_DEFAULT) + assert "Run `fab auth login --azure-cli`" in exc_info.value.message + assert auth.get_tenant_id() is None + assert auth.get_identity_type() is None + assert auth._azure_cli_credential is None + assert fab_mem_store._get_workspaces_from_cache.cache.currsize == 0 + assert fab_mem_store._get_workspace_folders_from_cache.cache.currsize == 0 + mock_credential_class.assert_called_once() + assert mock_credential.get_token.call_count == 2 + mock_logout.assert_called_once_with() + mock_clear_caches.assert_called_once_with() + mock_reset_context.assert_called_once_with() + mock_warning.assert_called_once_with("Change detected in Azure CLI Tenant ID") + + @patch("fabric_cli.core.fab_auth.AzureCliCredential") + def test_principal_updated_on_subsequent_calls_success( + self, mock_credential_class, azure_cli_auth_fixture + ): + """A changed Azure CLI principal should log out and reset cached state.""" + mock_credential, _ = _mock_credential(mock_credential_class) + auth = FabAuth() + auth.set_access_mode("azure_cli") + auth._azure_cli_credential = None + context = Context() + with ( + patch.object( + auth, + "_decode_jwt_token", + side_effect=[ + {"tid": "tenant-A", "oid": "principal-A"}, + {"tid": "tenant-A", "oid": "principal-B"}, + ], + ), + patch.object(auth, "logout", wraps=auth.logout) as mock_logout, + patch.object( + fab_mem_store, "clear_caches", wraps=fab_mem_store.clear_caches + ) as mock_clear_caches, + patch.object( + context, "reset_context", wraps=context.reset_context + ) as mock_reset_context, + patch("fabric_cli.core.fab_auth.fab_logger.log_warning") as mock_warning, + ): auth._acquire_token_from_azure_cli(con.SCOPE_FABRIC_DEFAULT) - assert auth.get_tenant_id() == "new-tenant" - assert auth.get_identity_type() == "azure_cli" - assert Context().get_tenant_id() == "new-tenant" + fab_mem_store._get_workspaces_from_cache.cache.update({"key": "value"}) + fab_mem_store._get_workspace_folders_from_cache.cache.update( + {"key": "value"} + ) + with pytest.raises(FabricCLIError) as exc_info: + auth._acquire_token_from_azure_cli(con.SCOPE_FABRIC_DEFAULT) + assert "Run `fab auth login --azure-cli`" in exc_info.value.message + assert auth.get_tenant_id() is None + assert auth._get_auth_property(con.FAB_PRINCIPAL_ID) is None + assert auth.get_identity_type() is None + assert auth._azure_cli_credential is None assert fab_mem_store._get_workspaces_from_cache.cache.currsize == 0 assert fab_mem_store._get_workspace_folders_from_cache.cache.currsize == 0 mock_credential_class.assert_called_once() assert mock_credential.get_token.call_count == 2 + mock_logout.assert_called_once_with() + mock_clear_caches.assert_called_once_with() + mock_reset_context.assert_called_once_with() + mock_warning.assert_called_once_with( + "Change detected in Azure CLI Principal ID" + ) + + @patch("fabric_cli.core.fab_auth.AzureCliCredential") + def test_tenant_and_principal_updated_on_subsequent_calls_success( + self, mock_credential_class, azure_cli_auth_fixture + ): + """Changed Azure CLI tenant and principal should report both changes.""" + _mock_credential(mock_credential_class) + auth = FabAuth() + auth.set_access_mode("azure_cli") + auth._azure_cli_credential = None + context = Context() + with ( + patch.object( + auth, + "_decode_jwt_token", + side_effect=[ + {"tid": "tenant-A", "oid": "principal-A"}, + {"tid": "tenant-B", "oid": "principal-B"}, + ], + ), + patch.object(auth, "logout", wraps=auth.logout) as mock_logout, + patch.object( + fab_mem_store, "clear_caches", wraps=fab_mem_store.clear_caches + ) as mock_clear_caches, + patch.object( + context, "reset_context", wraps=context.reset_context + ) as mock_reset_context, + patch("fabric_cli.core.fab_auth.fab_logger.log_warning") as mock_warning, + ): + auth._acquire_token_from_azure_cli(con.SCOPE_FABRIC_DEFAULT) + with pytest.raises(FabricCLIError): + auth._acquire_token_from_azure_cli(con.SCOPE_FABRIC_DEFAULT) + + assert auth.get_tenant_id() is None + assert auth._get_auth_property(con.FAB_PRINCIPAL_ID) is None + assert auth.get_identity_type() is None + assert auth._azure_cli_credential is None + mock_logout.assert_called_once_with() + mock_clear_caches.assert_called_once_with() + mock_reset_context.assert_called_once_with() + mock_warning.assert_called_once_with( + "Change detected in Azure CLI Tenant ID and Principal ID" + ) class TestAzureCliTokenAcquisition: @@ -302,21 +515,30 @@ class TestAzureCliLoginLogoutLifecycle: """Test login/logout lifecycle and credential management.""" @patch("fabric_cli.core.fab_auth.AzureCliCredential") - def test_logout_clears_credential_success( + def test_logout_then_login_records_new_identity_success( self, mock_credential_class, azure_cli_auth_fixture ): - """logout() should clear the credential instance.""" + """Explicit logout should allow a different identity to become the baseline.""" _mock_credential(mock_credential_class) - auth = FabAuth() auth.set_access_mode("azure_cli") - # Acquire token to set credential - auth._acquire_token_from_azure_cli(con.SCOPE_FABRIC_DEFAULT) - assert auth._azure_cli_credential is not None + with patch.object( + auth, + "_decode_jwt_token", + side_effect=[ + {"tid": "tenant-A", "oid": "principal-A"}, + {"tid": "tenant-B", "oid": "principal-B"}, + ], + ): + auth._acquire_token_from_azure_cli(con.SCOPE_FABRIC_DEFAULT) + auth.logout() + auth.set_access_mode("azure_cli") + auth._acquire_token_from_azure_cli(con.SCOPE_FABRIC_DEFAULT) - auth.logout() - assert auth._azure_cli_credential is None + assert auth.get_identity_type() == "azure_cli" + assert auth.get_tenant_id() == "tenant-B" + assert auth._get_auth_property(con.FAB_PRINCIPAL_ID) == "principal-B" @patch("fabric_cli.core.fab_auth.AzureCliCredential") def test_first_acquisition_discovers_tenant_success( @@ -329,7 +551,9 @@ def test_first_acquisition_discovers_tenant_success( auth.set_access_mode("azure_cli") auth._azure_cli_credential = None with patch.object( - auth, "_decode_jwt_token", return_value={"tid": "discovered-tenant"} + auth, + "_decode_jwt_token", + return_value={"tid": "discovered-tenant", "oid": "principal-A"}, ): auth._acquire_token_from_azure_cli(con.SCOPE_FABRIC_DEFAULT) assert auth.get_tenant_id() == "discovered-tenant" @@ -342,7 +566,11 @@ def test_re_login_resets_state_success( _mock_credential(mock_credential_class) auth = FabAuth() auth.set_access_mode("azure_cli") - with patch.object(auth, "_decode_jwt_token", return_value={"tid": "tenant-A"}): + with patch.object( + auth, + "_decode_jwt_token", + return_value={"tid": "tenant-A", "oid": "principal-A"}, + ): auth._acquire_token_from_azure_cli(con.SCOPE_FABRIC_DEFAULT) assert auth.get_tenant_id() == "tenant-A" From f9a4a1a1c87518a29afaee9c3e70830606781652 Mon Sep 17 00:00:00 2001 From: Shira Sassoon Date: Mon, 7 Sep 2026 10:23:06 +0300 Subject: [PATCH 2/5] feedback --- src/fabric_cli/commands/auth/fab_auth.py | 5 +++ src/fabric_cli/core/fab_auth.py | 2 +- tests/test_commands/test_auth.py | 43 +++++++++++++++++++++++- 3 files changed, 48 insertions(+), 2 deletions(-) diff --git a/src/fabric_cli/commands/auth/fab_auth.py b/src/fabric_cli/commands/auth/fab_auth.py index 1b6042cf..ff74ee38 100644 --- a/src/fabric_cli/commands/auth/fab_auth.py +++ b/src/fabric_cli/commands/auth/fab_auth.py @@ -220,6 +220,7 @@ def logout(args: Namespace) -> None: def status(args: Namespace) -> None: auth = FabAuth() + initial_identity_type = auth.get_identity_type() def __get_token_info(scope): try: @@ -265,6 +266,10 @@ def __mask_token(scope): tenant_id = auth.get_tenant_id() if identity_type is None: token_info = {} + if initial_identity_type == "azure_cli": + fabric_secret = "N/A" + storage_secret = "N/A" + azure_secret = "N/A" upn = token_info.get("upn") or "N/A" oid = token_info.get("oid") or "N/A" diff --git a/src/fabric_cli/core/fab_auth.py b/src/fabric_cli/core/fab_auth.py index 28f43103..c33cea89 100644 --- a/src/fabric_cli/core/fab_auth.py +++ b/src/fabric_cli/core/fab_auth.py @@ -495,7 +495,7 @@ def _check_azure_cli_identity(self, claims: dict) -> None: current_tenant_id = self.get_tenant_id() current_principal_id = self._get_auth_property(con.FAB_PRINCIPAL_ID) - # Determine if there is a identity drift (tenant or principal) + # Determine if there is an identity drift (tenant or principal) tenant_drifted = ( current_tenant_id is not None and tenant_id != current_tenant_id ) diff --git a/tests/test_commands/test_auth.py b/tests/test_commands/test_auth.py index f70313ed..58cecdd6 100644 --- a/tests/test_commands/test_auth.py +++ b/tests/test_commands/test_auth.py @@ -1016,7 +1016,7 @@ def test_auth_status_identity_drift_during_token_masking( ) def get_access_token(*args, **kwargs): - if auth.get_access_token.call_count == 1: + if auth.get_access_token.call_count <= 2: return "mocked_access_token" auth._auth_info = {} raise FabricCLIError( @@ -1046,6 +1046,47 @@ def get_access_token(*args, **kwargs): assert "Logged In: False" in captured.out assert "previous-tenant" not in captured.out + def test_auth_status_without_identity_preserves_available_tokens( + self, mock_fab_auth, capsys + ): + args = argparse.Namespace( + command="auth", + auth_subcommand="status", + output_format="text", + ) + auth = mock_fab_auth["instance"] + + with ( + patch.object(auth, "get_identity_type", return_value=None), + patch( + "fabric_cli.commands.auth.fab_auth._get_token_info_from_bearer_token", + return_value={}, + ), + ): + fab_auth.status(args) + + captured = capsys.readouterr() + assert "Logged in to app.fabric.microsoft.com" in captured.err + assert "Logged In: True" in captured.out + assert ( + "Token Fabric PowerBI: mock************************************" + in captured.out + ) + assert "Token Storage: mock************************************" in captured.out + assert "Token Azure: mock************************************" in captured.out + assert [ + token_call.args[0] for token_call in auth.get_access_token.call_args_list + ] == [ + fab_constant.SCOPE_FABRIC_DEFAULT, + fab_constant.SCOPE_FABRIC_DEFAULT, + fab_constant.SCOPE_ONELAKE_DEFAULT, + fab_constant.SCOPE_AZURE_DEFAULT, + ] + assert all( + token_call.kwargs == {"interactive_renew": False} + for token_call in auth.get_access_token.call_args_list + ) + def test_init_when_user_cancels_the_prompt( self, mock_fab_auth, mock_fab_context, mock_fab_logger_log_warning, capsys ): From 79baef2d28a7b1b32bb17b4f3ea7e324586a2b4b Mon Sep 17 00:00:00 2001 From: Shira Sassoon Date: Mon, 7 Sep 2026 11:17:40 +0300 Subject: [PATCH 3/5] Add drift detection to REPL mode --- src/fabric_cli/core/fab_auth.py | 5 +++ src/fabric_cli/core/fab_decorators.py | 13 ++++-- tests/test_core/test_fab_auth_azure_cli.py | 29 +++++++++++++ tests/test_core/test_fab_skill_attribution.py | 43 +++++++++++++++++++ 4 files changed, 87 insertions(+), 3 deletions(-) diff --git a/src/fabric_cli/core/fab_auth.py b/src/fabric_cli/core/fab_auth.py index c33cea89..3bdd585e 100644 --- a/src/fabric_cli/core/fab_auth.py +++ b/src/fabric_cli/core/fab_auth.py @@ -328,6 +328,11 @@ def get_tenant_id(self): def get_identity_type(self): return self._get_auth_property(con.IDENTITY_TYPE) + def validate_azure_cli_identity(self) -> None: + """Validate the current Azure CLI identity when that mode is active.""" + if self.get_identity_type() == "azure_cli": + self.get_access_token(con.SCOPE_FABRIC_DEFAULT, interactive_renew=False) + def set_access_mode(self, mode, tenant_id=None): if mode not in con.AUTH_KEYS[con.IDENTITY_TYPE]: raise FabricCLIError( diff --git a/src/fabric_cli/core/fab_decorators.py b/src/fabric_cli/core/fab_decorators.py index 90acee6a..226b7e4b 100644 --- a/src/fabric_cli/core/fab_decorators.py +++ b/src/fabric_cli/core/fab_decorators.py @@ -8,6 +8,7 @@ ERROR_UNAUTHORIZED, EXIT_CODE_AUTHORIZATION_REQUIRED, EXIT_CODE_ERROR, + FAB_MODE_INTERACTIVE, ) from fabric_cli.core.fab_exceptions import FabricCLIError from fabric_cli.utils import fab_ui @@ -21,7 +22,7 @@ def getinstance(*args, **kwargs): if class_ not in instances: instances[class_] = class_(*args, **kwargs) return instances[class_] - + return getinstance @@ -64,8 +65,14 @@ def decorator(func): def wrapper(*args, **kwargs): # Import Context locally to avoid circular import from fabric_cli.core.fab_context import Context - Context().command = args[0].command_path - Context().fabric_skill = getattr(args[0], "skill", None) + + context = Context() + context.command = args[0].command_path + context.fabric_skill = getattr(args[0], "skill", None) + if context.get_runtime_mode() == FAB_MODE_INTERACTIVE: + from fabric_cli.core.fab_auth import FabAuth + + FabAuth().validate_azure_cli_identity() return func(*args, **kwargs) return wrapper diff --git a/tests/test_core/test_fab_auth_azure_cli.py b/tests/test_core/test_fab_auth_azure_cli.py index 575a1053..0750be38 100644 --- a/tests/test_core/test_fab_auth_azure_cli.py +++ b/tests/test_core/test_fab_auth_azure_cli.py @@ -38,6 +38,35 @@ def test_set_access_mode_accepts_azure_cli_success(self, azure_cli_auth_fixture) auth.set_access_mode("azure_cli") assert auth.get_identity_type() == "azure_cli" + def test_validate_azure_cli_identity_checks_active_mode_success( + self, azure_cli_auth_fixture + ): + auth = FabAuth() + auth._auth_info = {con.IDENTITY_TYPE: "azure_cli"} + + with patch.object(auth, "get_access_token") as get_access_token: + auth.validate_azure_cli_identity() + + get_access_token.assert_called_once_with( + con.SCOPE_FABRIC_DEFAULT, interactive_renew=False + ) + + @pytest.mark.parametrize( + "identity_type", [None, "user", "service_principal", "managed_identity"] + ) + def test_validate_azure_cli_identity_skips_other_auth_modes_success( + self, identity_type, azure_cli_auth_fixture + ): + auth = FabAuth() + auth._auth_info = ( + {con.IDENTITY_TYPE: identity_type} if identity_type is not None else {} + ) + + with patch.object(auth, "get_access_token") as get_access_token: + auth.validate_azure_cli_identity() + + get_access_token.assert_not_called() + @pytest.mark.parametrize( "claims", [ diff --git a/tests/test_core/test_fab_skill_attribution.py b/tests/test_core/test_fab_skill_attribution.py index 3ce02da8..b3ea7f65 100644 --- a/tests/test_core/test_fab_skill_attribution.py +++ b/tests/test_core/test_fab_skill_attribution.py @@ -13,7 +13,9 @@ from fabric_cli.core.fab_auth import FabAuth from fabric_cli.core.fab_context import Context from fabric_cli.core.fab_decorators import set_command_context +from fabric_cli.core.fab_exceptions import FabricCLIError from fabric_cli.core.fab_parser_setup import create_parser_and_subparsers +from fabric_cli.errors import ErrorMessages from fabric_cli.parsers import fab_global_params pytestmark = pytest.mark.usefixtures("reset_context") @@ -74,6 +76,47 @@ def command(args: Namespace) -> None: command(Namespace(command_path="export", skill=None)) +def test_command_context_validates_identity_before_interactive_command_success(): + Context().set_runtime_mode(fab_constant.FAB_MODE_INTERACTIVE) + command_executed = False + + @set_command_context() + def command(args: Namespace) -> None: + nonlocal command_executed + command_executed = True + + with patch.object(FabAuth(), "validate_azure_cli_identity") as validate_identity: + command(Namespace(command_path="ls", skill=None)) + + validate_identity.assert_called_once_with() + assert command_executed is True + + +def test_command_context_prevents_interactive_command_on_identity_drift_failure(): + Context().set_runtime_mode(fab_constant.FAB_MODE_INTERACTIVE) + command_executed = False + + @set_command_context() + def command(args: Namespace) -> None: + nonlocal command_executed + command_executed = True + + with ( + patch.object( + FabAuth(), + "validate_azure_cli_identity", + side_effect=FabricCLIError( + ErrorMessages.Auth.azure_cli_identity_changed(), + fab_constant.ERROR_AUTHENTICATION_FAILED, + ), + ), + pytest.raises(FabricCLIError), + ): + command(Namespace(command_path="ls", skill=None)) + + assert command_executed is False + + @pytest.mark.parametrize( "value", [ From c99100bd6ca40cad39b75998e84ce16f84015f71 Mon Sep 17 00:00:00 2001 From: Shira Sassoon Date: Tue, 8 Sep 2026 12:58:20 +0300 Subject: [PATCH 4/5] feedback --- src/fabric_cli/commands/auth/fab_auth.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/fabric_cli/commands/auth/fab_auth.py b/src/fabric_cli/commands/auth/fab_auth.py index ff74ee38..d71aebd0 100644 --- a/src/fabric_cli/commands/auth/fab_auth.py +++ b/src/fabric_cli/commands/auth/fab_auth.py @@ -264,12 +264,10 @@ def __mask_token(scope): identity_type = auth.get_identity_type() tenant_id = auth.get_tenant_id() - if identity_type is None: + + if identity_type is None and initial_identity_type == "azure_cli": token_info = {} - if initial_identity_type == "azure_cli": - fabric_secret = "N/A" - storage_secret = "N/A" - azure_secret = "N/A" + fabric_secret, storage_secret, azure_secret = ("N/A",) * 3 upn = token_info.get("upn") or "N/A" oid = token_info.get("oid") or "N/A" From b2929c982254a7df38e18df305fdcdd07b7e6add Mon Sep 17 00:00:00 2001 From: Shira Sassoon Date: Wed, 9 Sep 2026 11:59:55 +0300 Subject: [PATCH 5/5] fix re-login to azure cli mode error --- src/fabric_cli/core/fab_auth.py | 10 ++ tests/test_core/test_fab_auth_azure_cli.py | 132 ++++++++++++++++++++- 2 files changed, 137 insertions(+), 5 deletions(-) diff --git a/src/fabric_cli/core/fab_auth.py b/src/fabric_cli/core/fab_auth.py index 3bdd585e..3dbc9d12 100644 --- a/src/fabric_cli/core/fab_auth.py +++ b/src/fabric_cli/core/fab_auth.py @@ -343,6 +343,10 @@ def set_access_mode(self, mode, tenant_id=None): ) if mode != self.get_identity_type(): self.logout() + elif mode == "azure_cli": + # Reset the baseline so an explicit re-login establishes the + # current Azure CLI identity instead of reporting identity drift + self._reset_azure_cli_identity_baseline() if tenant_id and self.get_tenant_id() != tenant_id: self.set_tenant(tenant_id) self._set_auth_property(con.IDENTITY_TYPE, mode) @@ -538,6 +542,12 @@ def _check_azure_cli_identity(self, claims: dict) -> None: if current_tenant_id is None: Context().context = self.get_tenant() + def _reset_azure_cli_identity_baseline(self): + 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() + def print_auth_info(self): utils_ui.print_grey(json.dumps(self._get_auth_info(), indent=2)) diff --git a/tests/test_core/test_fab_auth_azure_cli.py b/tests/test_core/test_fab_auth_azure_cli.py index 0750be38..8ab0f1b6 100644 --- a/tests/test_core/test_fab_auth_azure_cli.py +++ b/tests/test_core/test_fab_auth_azure_cli.py @@ -184,7 +184,7 @@ def test_first_token_acquisition_stores_identity_success( mock_logout.assert_not_called() @patch("fabric_cli.core.fab_auth.AzureCliCredential") - def test_tenant_updated_on_subsequent_calls_success( + def test_tenant_drift_logs_out_failure( self, mock_credential_class, azure_cli_auth_fixture ): """A changed Azure CLI tenant should log out and reset cached state.""" @@ -232,7 +232,7 @@ def test_tenant_updated_on_subsequent_calls_success( mock_warning.assert_called_once_with("Change detected in Azure CLI Tenant ID") @patch("fabric_cli.core.fab_auth.AzureCliCredential") - def test_principal_updated_on_subsequent_calls_success( + def test_principal_drift_logs_out_failure( self, mock_credential_class, azure_cli_auth_fixture ): """A changed Azure CLI principal should log out and reset cached state.""" @@ -283,7 +283,7 @@ def test_principal_updated_on_subsequent_calls_success( ) @patch("fabric_cli.core.fab_auth.AzureCliCredential") - def test_tenant_and_principal_updated_on_subsequent_calls_success( + def test_tenant_and_principal_drift_logs_out_failure( self, mock_credential_class, azure_cli_auth_fixture ): """Changed Azure CLI tenant and principal should report both changes.""" @@ -591,7 +591,8 @@ def test_first_acquisition_discovers_tenant_success( def test_re_login_resets_state_success( self, mock_credential_class, azure_cli_auth_fixture ): - """Re-login (set_access_mode again) should reset state.""" + """Re-login in azure_cli mode clears the identity baseline so a new + identity is adopted without a drift error.""" _mock_credential(mock_credential_class) auth = FabAuth() auth.set_access_mode("azure_cli") @@ -603,9 +604,67 @@ def test_re_login_resets_state_success( auth._acquire_token_from_azure_cli(con.SCOPE_FABRIC_DEFAULT) assert auth.get_tenant_id() == "tenant-A" - # Re-login — set_access_mode("azure_cli") when already azure_cli does NOT logout + # Re-login (set_access_mode again while already azure_cli) must clear baseline auth.set_access_mode("azure_cli") + assert auth.get_tenant_id() is None + assert auth._get_auth_property(con.FAB_PRINCIPAL_ID) is None + + # A different identity now becomes the new baseline, no drift error + with patch.object( + auth, + "_decode_jwt_token", + return_value={"tid": "tenant-B", "oid": "principal-B"}, + ): + auth._acquire_token_from_azure_cli(con.SCOPE_FABRIC_DEFAULT) + assert auth.get_tenant_id() == "tenant-B" + assert auth._get_auth_property(con.FAB_PRINCIPAL_ID) == "principal-B" + + @patch("fabric_cli.utils.fab_version_check.check_and_notify_update") + @patch("fabric_cli.core.fab_auth.AzureCliCredential") + def test_relogin_after_identity_switch_success( + self, mock_credential_class, _mock_version_check, azure_cli_auth_fixture + ): + """`fab auth login --azure-cli` after switching the az identity must + succeed on the first attempt and adopt the new identity as baseline.""" + import argparse + + from fabric_cli.commands.auth import fab_auth as auth_cmd + + _mock_credential(mock_credential_class) + auth = FabAuth() + + def _login_args(): + return argparse.Namespace( + azure_cli=True, + identity=False, + username=None, + password=None, + tenant=None, + certificate=None, + federated_token=None, + ) + + # First login establishes identity A as the baseline + with patch.object( + auth, + "_decode_jwt_token", + return_value={"tid": "tenant-A", "oid": "principal-A"}, + ): + assert auth_cmd.init(_login_args()) is True assert auth.get_tenant_id() == "tenant-A" + assert auth._get_auth_property(con.FAB_PRINCIPAL_ID) == "principal-A" + + # After `az login` to identity B, re-login must NOT raise a drift error + with patch.object( + auth, + "_decode_jwt_token", + return_value={"tid": "tenant-B", "oid": "principal-B"}, + ): + assert auth_cmd.init(_login_args()) is True + + assert auth.get_identity_type() == "azure_cli" + assert auth.get_tenant_id() == "tenant-B" + assert auth._get_auth_property(con.FAB_PRINCIPAL_ID) == "principal-B" class TestNonAzureCliIsolation: @@ -668,3 +727,66 @@ def test_azure_cli_does_not_invoke_msal_app_success( mock_app.acquire_token_silent.assert_not_called() mock_app.acquire_token_interactive.assert_not_called() mock_app.acquire_token_for_client.assert_not_called() + + +class TestAuthModeTransitions: + """Guard set_access_mode transitions affected by the azure_cli baseline reset.""" + + @pytest.mark.parametrize("mode", ["user", "service_principal", "managed_identity"]) + def test_same_mode_relogin_preserves_state_non_azure_cli_success( + self, mode, azure_cli_auth_fixture + ): + """Re-login in a non-azure_cli mode must not reset stored state.""" + auth = FabAuth() + auth.set_access_mode(mode) + auth._set_auth_property(con.FAB_TENANT_ID, "seed-tenant") + auth._set_auth_property(con.FAB_PRINCIPAL_ID, "seed-principal") + + auth.set_access_mode(mode) + + assert auth.get_identity_type() == mode + assert auth.get_tenant_id() == "seed-tenant" + assert auth._get_auth_property(con.FAB_PRINCIPAL_ID) == "seed-principal" + + @patch("fabric_cli.core.fab_auth.AzureCliCredential") + def test_switch_from_azure_cli_to_user_clears_principal_baseline_success( + self, mock_credential_class, azure_cli_auth_fixture + ): + """Leaving azure_cli for another mode must clear the identity baseline.""" + _mock_credential(mock_credential_class) + auth = FabAuth() + auth.set_access_mode("azure_cli") + with patch.object( + auth, + "_decode_jwt_token", + return_value={"tid": "tenant-A", "oid": "principal-A"}, + ): + auth._acquire_token_from_azure_cli(con.SCOPE_FABRIC_DEFAULT) + assert auth._get_auth_property(con.FAB_PRINCIPAL_ID) == "principal-A" + + auth.set_access_mode("user") + + assert auth.get_identity_type() == "user" + assert auth.get_tenant_id() is None + assert auth._get_auth_property(con.FAB_PRINCIPAL_ID) is None + + @patch("fabric_cli.core.fab_auth.AzureCliCredential") + def test_switch_from_user_to_azure_cli_starts_clean_success( + self, mock_credential_class, azure_cli_auth_fixture + ): + """Entering azure_cli from another mode must not carry a stale principal.""" + _mock_credential(mock_credential_class) + auth = FabAuth() + auth.set_access_mode("user") + auth._set_auth_property(con.FAB_PRINCIPAL_ID, "stale-principal") + + auth.set_access_mode("azure_cli") + with patch.object( + auth, + "_decode_jwt_token", + return_value={"tid": "tenant-B", "oid": "principal-B"}, + ): + auth._acquire_token_from_azure_cli(con.SCOPE_FABRIC_DEFAULT) + + assert auth.get_tenant_id() == "tenant-B" + assert auth._get_auth_property(con.FAB_PRINCIPAL_ID) == "principal-B"