Refactor AzureCliCredential claims challenge logic and add tenant support in error messages #42195
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -395,3 +395,66 @@ def fake_check_output(command_line, **_): | |||||
| kwargs = {"options": kwargs} | ||||||
| token = getattr(credential, get_token_method)("scope", **kwargs) | ||||||
| assert token.token == expected_token | ||||||
|
|
||||||
|
|
||||||
| @pytest.mark.parametrize("get_token_method", GET_TOKEN_METHODS) | ||||||
| def test_claims_challenge_raises_error(get_token_method): | ||||||
| """The credential should raise CredentialUnavailableError when claims challenge is provided""" | ||||||
|
|
||||||
| claims = "test-claims-challenge" | ||||||
| expected_message = f"Failed to get token. Run az login --claims-challenge {claims}" | ||||||
|
|
||||||
| if get_token_method == "get_token": | ||||||
| with pytest.raises(CredentialUnavailableError, match=re.escape(expected_message)): | ||||||
| AzureCliCredential().get_token("scope", claims=claims) | ||||||
| else: # get_token_info | ||||||
| with pytest.raises(CredentialUnavailableError, match=re.escape(expected_message)): | ||||||
| AzureCliCredential().get_token_info("scope", options={"claims": claims}) | ||||||
|
|
||||||
|
|
||||||
| @pytest.mark.parametrize("get_token_method", GET_TOKEN_METHODS) | ||||||
| def test_empty_claims_does_not_raise_error(get_token_method): | ||||||
| """The credential should not raise error when claims parameter is empty or None""" | ||||||
|
|
||||||
| # Mock the CLI to avoid actual invocation | ||||||
| with mock.patch("shutil.which", return_value="az"): | ||||||
| with mock.patch( | ||||||
| CHECK_OUTPUT, mock.Mock(return_value='{"accessToken": "token", "expiresOn": "2021-10-07 12:00:00.000000"}') | ||||||
|
There was a problem hiding this comment. The hardcoded datetime string '2021-10-07 12:00:00.000000' in the mock response should use a dynamic date or a more recent date to avoid potential issues with token expiration validation.
Suggested change
Copilot uses AI. Check for mistakes. |
||||||
| ): | ||||||
|
|
||||||
| if get_token_method == "get_token": | ||||||
| # Test with None (default) | ||||||
| token = AzureCliCredential().get_token("scope") | ||||||
| assert token.token == "token" | ||||||
|
|
||||||
| # Test with empty string | ||||||
| token = AzureCliCredential().get_token("scope", claims="") | ||||||
| assert token.token == "token" | ||||||
|
|
||||||
| # Test with None explicitly | ||||||
| token = AzureCliCredential().get_token("scope", claims=None) | ||||||
| assert token.token == "token" | ||||||
|
|
||||||
| # Test with whitespace-only string | ||||||
| token = AzureCliCredential().get_token("scope", claims=" ") | ||||||
| assert token.token == "token" | ||||||
| else: # get_token_info | ||||||
| # Test with None options | ||||||
| token = AzureCliCredential().get_token_info("scope") | ||||||
| assert token.token == "token" | ||||||
|
|
||||||
| # Test with empty options | ||||||
| token = AzureCliCredential().get_token_info("scope", options={}) | ||||||
| assert token.token == "token" | ||||||
|
|
||||||
| # Test with None claims in options | ||||||
| token = AzureCliCredential().get_token_info("scope", options={"claims": None}) | ||||||
| assert token.token == "token" | ||||||
|
|
||||||
| # Test with empty string claims in options | ||||||
| token = AzureCliCredential().get_token_info("scope", options={"claims": ""}) | ||||||
| assert token.token == "token" | ||||||
|
|
||||||
| # Test with whitespace-only claims in options | ||||||
| token = AzureCliCredential().get_token_info("scope", options={"claims": " "}) | ||||||
| assert token.token == "token" | ||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -389,3 +389,74 @@ async def fake_exec(*args, **_): | |||||
| kwargs = {"options": kwargs} | ||||||
| token = await getattr(credential, get_token_method)("scope", **kwargs) | ||||||
| assert token.token == expected_token | ||||||
|
|
||||||
|
|
||||||
| @pytest.mark.parametrize("get_token_method", GET_TOKEN_METHODS) | ||||||
| async def test_claims_challenge_raises_error(get_token_method): | ||||||
| """The credential should raise CredentialUnavailableError when claims challenge is provided""" | ||||||
|
|
||||||
| claims = "test-claims-challenge" | ||||||
| expected_message = f"Failed to get token. Run az login --claims-challenge {claims}" | ||||||
|
|
||||||
| if get_token_method == "get_token": | ||||||
| with pytest.raises(CredentialUnavailableError, match=re.escape(expected_message)): | ||||||
| await AzureCliCredential().get_token("scope", claims=claims) | ||||||
| else: # get_token_info | ||||||
| with pytest.raises(CredentialUnavailableError, match=re.escape(expected_message)): | ||||||
| await AzureCliCredential().get_token_info("scope", options={"claims": claims}) | ||||||
|
|
||||||
|
|
||||||
| @pytest.mark.parametrize("get_token_method", GET_TOKEN_METHODS) | ||||||
| async def test_empty_claims_does_not_raise_error(get_token_method): | ||||||
| """The credential should not raise error when claims parameter is empty or None""" | ||||||
|
|
||||||
| successful_output = json.dumps( | ||||||
| { | ||||||
| "expiresOn": datetime.now().strftime("%Y-%m-%d %H:%M:%S.%f"), | ||||||
|
There was a problem hiding this comment. Using datetime.now() without importing datetime module from the datetime package. Should be 'from datetime import datetime' or use a fixed future date for consistency.
Suggested change
Copilot uses AI. Check for mistakes. |
||||||
| "accessToken": "access-token", | ||||||
| "subscription": "subscription", | ||||||
| "tenant": "tenant", | ||||||
| "tokenType": "Bearer", | ||||||
| } | ||||||
| ) | ||||||
|
|
||||||
| # Mock the CLI to avoid actual invocation | ||||||
| with mock.patch("shutil.which", return_value="az"): | ||||||
| with mock.patch(SUBPROCESS_EXEC, mock_exec(successful_output)): | ||||||
|
|
||||||
| if get_token_method == "get_token": | ||||||
| # Test with None (default) | ||||||
| token = await AzureCliCredential().get_token("scope") | ||||||
| assert token.token == "access-token" | ||||||
|
|
||||||
| # Test with empty string | ||||||
| token = await AzureCliCredential().get_token("scope", claims="") | ||||||
| assert token.token == "access-token" | ||||||
|
|
||||||
| # Test with None explicitly | ||||||
| token = await AzureCliCredential().get_token("scope", claims=None) | ||||||
| assert token.token == "access-token" | ||||||
|
|
||||||
| # Test with whitespace-only string | ||||||
| token = await AzureCliCredential().get_token("scope", claims=" ") | ||||||
| assert token.token == "access-token" | ||||||
| else: # get_token_info | ||||||
| # Test with None options | ||||||
| token = await AzureCliCredential().get_token_info("scope") | ||||||
| assert token.token == "access-token" | ||||||
|
|
||||||
| # Test with empty options | ||||||
| token = await AzureCliCredential().get_token_info("scope", options={}) | ||||||
| assert token.token == "access-token" | ||||||
|
|
||||||
| # Test with None claims in options | ||||||
| token = await AzureCliCredential().get_token_info("scope", options={"claims": None}) | ||||||
| assert token.token == "access-token" | ||||||
|
|
||||||
| # Test with empty string claims in options | ||||||
| token = await AzureCliCredential().get_token_info("scope", options={"claims": ""}) | ||||||
| assert token.token == "access-token" | ||||||
|
|
||||||
| # Test with whitespace-only claims in options | ||||||
| token = await AzureCliCredential().get_token_info("scope", options={"claims": " "}) | ||||||
| assert token.token == "access-token" | ||||||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't fully form the required login command, for example
--scopescould also be provided that should be included in the final login command.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the error message to include scopes in the suggested
az logincommand. Now when scopes are provided, the error message will suggest:az login --claims-challenge {claims} --scope {first_scope}. When no scopes are provided, it will suggest:az login --claims-challenge {claims}. This provides users with a more complete and actionable command. Commit 2af955e.