From f51fa964d76edb408677fc5a5378faa11c12c4a8 Mon Sep 17 00:00:00 2001 From: Xiang Yan Date: Wed, 26 Oct 2022 13:44:13 -0700 Subject: [PATCH] Fix #26857: separate stdout from stderr in AzureCliCredential (#26953) (#27076) * Fix #26857: separate stdout from stderr in AzureCliCredential * Fix unit tests for AzureCliCredential * Fix #26857 for aio.AzureCliCredential * azure_cli.py: Fix types in _run_command * Fix test_cli_credential_async.py * Update sdk/identity/azure-identity/CHANGELOG.md Co-authored-by: Paul Van Eck Co-authored-by: Tingmao Wang Co-authored-by: Paul Van Eck Co-authored-by: Xiang Yan Co-authored-by: maowtm Co-authored-by: Tingmao Wang Co-authored-by: Paul Van Eck --- sdk/identity/azure-identity/CHANGELOG.md | 1 + .../azure/identity/_credentials/azure_cli.py | 10 ++++---- .../identity/aio/_credentials/azure_cli.py | 13 +++++----- .../tests/test_cli_credential.py | 24 +++++++++---------- .../tests/test_cli_credential_async.py | 18 +++++++------- 5 files changed, 34 insertions(+), 32 deletions(-) diff --git a/sdk/identity/azure-identity/CHANGELOG.md b/sdk/identity/azure-identity/CHANGELOG.md index c68c560bc41b..63c3ec36480d 100644 --- a/sdk/identity/azure-identity/CHANGELOG.md +++ b/sdk/identity/azure-identity/CHANGELOG.md @@ -4,6 +4,7 @@ ### Bugs Fixed +- `AzureCliCredential` now works even when `az` prints warnings to stderr. ([#26857](https://github.com/Azure/azure-sdk-for-python/issues/26857)) - Fixed issue where user-supplied `TokenCachePersistenceOptions` weren't propagated when using `SharedTokenCacheCredential` ([#26982](https://github.com/Azure/azure-sdk-for-python/issues/26982)) ### Breaking Changes diff --git a/sdk/identity/azure-identity/azure/identity/_credentials/azure_cli.py b/sdk/identity/azure-identity/azure/identity/_credentials/azure_cli.py index 39ce272a829e..24d7fd95744f 100644 --- a/sdk/identity/azure-identity/azure/identity/_credentials/azure_cli.py +++ b/sdk/identity/azure-identity/azure/identity/_credentials/azure_cli.py @@ -143,7 +143,7 @@ def _run_command(command): working_directory = get_safe_working_dir() kwargs = { - "stderr": subprocess.STDOUT, + "stderr": subprocess.PIPE, "cwd": working_directory, "universal_newlines": True, "env": dict(os.environ, AZURE_CORE_NO_COLOR="true"), @@ -154,14 +154,14 @@ def _run_command(command): return subprocess.check_output(args, **kwargs) except subprocess.CalledProcessError as ex: # non-zero return from shell - if ex.returncode == 127 or ex.output.startswith("'az' is not recognized"): + if ex.returncode == 127 or ex.stderr.startswith("'az' is not recognized"): raise CredentialUnavailableError(message=CLI_NOT_FOUND) - if "az login" in ex.output or "az account set" in ex.output: + if "az login" in ex.stderr or "az account set" in ex.stderr: raise CredentialUnavailableError(message=NOT_LOGGED_IN) # return code is from the CLI -> propagate its output - if ex.output: - message = sanitize_output(ex.output) + if ex.stderr: + message = sanitize_output(ex.stderr) else: message = "Failed to invoke Azure CLI" raise ClientAuthenticationError(message=message) diff --git a/sdk/identity/azure-identity/azure/identity/aio/_credentials/azure_cli.py b/sdk/identity/azure-identity/azure/identity/aio/_credentials/azure_cli.py index 0d089e708c74..fd9e8f60296c 100644 --- a/sdk/identity/azure-identity/azure/identity/aio/_credentials/azure_cli.py +++ b/sdk/identity/azure-identity/azure/identity/aio/_credentials/azure_cli.py @@ -100,12 +100,13 @@ async def _run_command(command: str) -> str: proc = await asyncio.create_subprocess_exec( *args, stdout=asyncio.subprocess.PIPE, - stderr=asyncio.subprocess.STDOUT, + stderr=asyncio.subprocess.PIPE, cwd=working_directory, env=dict(os.environ, AZURE_CORE_NO_COLOR="true") ) - stdout, _ = await asyncio.wait_for(proc.communicate(), 10) - output = stdout.decode() + stdout_b, stderr_b = await asyncio.wait_for(proc.communicate(), 10) + output = stdout_b.decode() + stderr = stderr_b.decode() except OSError as ex: # failed to execute 'cmd' or '/bin/sh'; CLI may or may not be installed error = CredentialUnavailableError(message="Failed to execute '{}'".format(args[0])) @@ -117,11 +118,11 @@ async def _run_command(command: str) -> str: if proc.returncode == 0: return output - if proc.returncode == 127 or output.startswith("'az' is not recognized"): + if proc.returncode == 127 or stderr.startswith("'az' is not recognized"): raise CredentialUnavailableError(CLI_NOT_FOUND) - if "az login" in output or "az account set" in output: + if "az login" in stderr or "az account set" in stderr: raise CredentialUnavailableError(message=NOT_LOGGED_IN) - message = sanitize_output(output) if output else "Failed to invoke Azure CLI" + message = sanitize_output(stderr) if stderr else "Failed to invoke Azure CLI" raise ClientAuthenticationError(message=message) diff --git a/sdk/identity/azure-identity/tests/test_cli_credential.py b/sdk/identity/azure-identity/tests/test_cli_credential.py index f3f735ed5be9..6a604de99a14 100644 --- a/sdk/identity/azure-identity/tests/test_cli_credential.py +++ b/sdk/identity/azure-identity/tests/test_cli_credential.py @@ -31,8 +31,8 @@ ) -def raise_called_process_error(return_code, output, cmd="..."): - error = subprocess.CalledProcessError(return_code, cmd=cmd, output=output) +def raise_called_process_error(return_code, output="", cmd="...", stderr=""): + error = subprocess.CalledProcessError(return_code, cmd=cmd, output=output, stderr=stderr) return mock.Mock(side_effect=error) @@ -76,8 +76,8 @@ def test_get_token(): def test_cli_not_installed_linux(): """The credential should raise CredentialUnavailableError when the CLI isn't installed""" - output = "/bin/sh: 1: az: not found" - with mock.patch(CHECK_OUTPUT, raise_called_process_error(127, output)): + stderr = "/bin/sh: 1: az: not found" + with mock.patch(CHECK_OUTPUT, raise_called_process_error(127, stderr=stderr)): with pytest.raises(CredentialUnavailableError, match=CLI_NOT_FOUND): AzureCliCredential().get_token("scope") @@ -85,8 +85,8 @@ def test_cli_not_installed_linux(): def test_cli_not_installed_windows(): """The credential should raise CredentialUnavailableError when the CLI isn't installed""" - output = "'az' is not recognized as an internal or external command, operable program or batch file." - with mock.patch(CHECK_OUTPUT, raise_called_process_error(1, output)): + stderr = "'az' is not recognized as an internal or external command, operable program or batch file." + with mock.patch(CHECK_OUTPUT, raise_called_process_error(1, stderr=stderr)): with pytest.raises(CredentialUnavailableError, match=CLI_NOT_FOUND): AzureCliCredential().get_token("scope") @@ -102,8 +102,8 @@ def test_cannot_execute_shell(): def test_not_logged_in(): """When the CLI isn't logged in, the credential should raise CredentialUnavailableError""" - output = "ERROR: Please run 'az login' to setup account." - with mock.patch(CHECK_OUTPUT, raise_called_process_error(1, output)): + stderr = "ERROR: Please run 'az login' to setup account." + with mock.patch(CHECK_OUTPUT, raise_called_process_error(1, stderr=stderr)): with pytest.raises(CredentialUnavailableError, match=NOT_LOGGED_IN): AzureCliCredential().get_token("scope") @@ -111,9 +111,9 @@ def test_not_logged_in(): def test_unexpected_error(): """When the CLI returns an unexpected error, the credential should raise an error containing the CLI's output""" - output = "something went wrong" - with mock.patch(CHECK_OUTPUT, raise_called_process_error(42, output)): - with pytest.raises(ClientAuthenticationError, match=output): + stderr = "something went wrong" + with mock.patch(CHECK_OUTPUT, raise_called_process_error(42, stderr=stderr)): + with pytest.raises(ClientAuthenticationError, match=stderr): AzureCliCredential().get_token("scope") @@ -181,7 +181,7 @@ def fake_check_output(command_line, **_): token = AzureCliCredential(tenant_id=second_tenant).get_token("scope") assert token.token == second_token - + def test_multitenant_authentication(): default_tenant = "first-tenant" first_token = "***" diff --git a/sdk/identity/azure-identity/tests/test_cli_credential_async.py b/sdk/identity/azure-identity/tests/test_cli_credential_async.py index 2276a1bff3e5..5863c4af2111 100644 --- a/sdk/identity/azure-identity/tests/test_cli_credential_async.py +++ b/sdk/identity/azure-identity/tests/test_cli_credential_async.py @@ -101,8 +101,8 @@ async def test_get_token(): async def test_cli_not_installed_linux(): """The credential should raise CredentialUnavailableError when the CLI isn't installed""" - output = "/bin/sh: 1: az: not found" - with mock.patch(SUBPROCESS_EXEC, mock_exec(output, return_code=127)): + stderr = "/bin/sh: 1: az: not found" + with mock.patch(SUBPROCESS_EXEC, mock_exec("", stderr, return_code=127)): with pytest.raises(CredentialUnavailableError, match=CLI_NOT_FOUND): credential = AzureCliCredential() await credential.get_token("scope") @@ -111,8 +111,8 @@ async def test_cli_not_installed_linux(): async def test_cli_not_installed_windows(): """The credential should raise CredentialUnavailableError when the CLI isn't installed""" - output = "'az' is not recognized as an internal or external command, operable program or batch file." - with mock.patch(SUBPROCESS_EXEC, mock_exec(output, return_code=1)): + stderr = "'az' is not recognized as an internal or external command, operable program or batch file." + with mock.patch(SUBPROCESS_EXEC, mock_exec("", stderr, return_code=1)): with pytest.raises(CredentialUnavailableError, match=CLI_NOT_FOUND): credential = AzureCliCredential() await credential.get_token("scope") @@ -130,8 +130,8 @@ async def test_cannot_execute_shell(): async def test_not_logged_in(): """When the CLI isn't logged in, the credential should raise CredentialUnavailableError""" - output = "ERROR: Please run 'az login' to setup account." - with mock.patch(SUBPROCESS_EXEC, mock_exec(output, return_code=1)): + stderr = "ERROR: Please run 'az login' to setup account." + with mock.patch(SUBPROCESS_EXEC, mock_exec("", stderr, return_code=1)): with pytest.raises(CredentialUnavailableError, match=NOT_LOGGED_IN): credential = AzureCliCredential() await credential.get_token("scope") @@ -140,9 +140,9 @@ async def test_not_logged_in(): async def test_unexpected_error(): """When the CLI returns an unexpected error, the credential should raise an error containing the CLI's output""" - output = "something went wrong" - with mock.patch(SUBPROCESS_EXEC, mock_exec(output, return_code=42)): - with pytest.raises(ClientAuthenticationError, match=output): + stderr = "something went wrong" + with mock.patch(SUBPROCESS_EXEC, mock_exec("", stderr, return_code=42)): + with pytest.raises(ClientAuthenticationError, match=stderr): credential = AzureCliCredential() await credential.get_token("scope")