-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Refactor AzureCliCredential claims challenge logic and add tenant support in error messages #42195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
a0105bb
Initial plan
Copilot 1541bfd
Implement claims challenge error for AzureCliCredential get_token and…
Copilot 37b031e
Refine claims challenge handling to ignore whitespace-only claims and…
Copilot eb3eb89
Fix MyPy errors in AzureCliCredential claims challenge handling
Copilot 1ced477
Fix pylint line-too-long and trailing-whitespace errors in AzureCliCr…
Copilot 3f45fb2
Apply black formatting to AzureCliCredential files
Copilot ae796fc
black
xiangyan99 e778f33
Update error message format and docstrings for claims challenge handling
Copilot a9bd2a7
black
xiangyan99 2af955e
Include scopes in az login command for claims challenge error messages
Copilot 30832b6
Refactor claims challenge logic to _get_token_base and add tenant sup…
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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,116 @@ 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} --scope scope" | ||||||
|
|
||||||
| 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_claims_challenge_without_scopes(get_token_method): | ||||||
| """The credential should raise CredentialUnavailableError with appropriate message even when no scopes 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(claims=claims) # No scopes provided | ||||||
| else: # get_token_info | ||||||
| with pytest.raises(CredentialUnavailableError, match=re.escape(expected_message)): | ||||||
| AzureCliCredential().get_token_info(options={"claims": claims}) # No scopes provided | ||||||
|
|
||||||
|
|
||||||
| @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Positive FeedbackNegative Feedback |
||||||
| ): | ||||||
|
|
||||||
| 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" | ||||||
|
|
||||||
|
|
||||||
| @pytest.mark.parametrize("get_token_method", GET_TOKEN_METHODS) | ||||||
| def test_claims_challenge_with_tenant(get_token_method): | ||||||
| """The credential should include tenant in the error message when claims and tenant are provided""" | ||||||
|
|
||||||
| claims = "test-claims-challenge" | ||||||
| tenant_id = "test-tenant-id" | ||||||
|
|
||||||
| if get_token_method == "get_token": | ||||||
| # Test with get_token - tenant passed via get_token parameter (should appear in options) | ||||||
| expected_message = f"Failed to get token. Run az login --claims-challenge {claims} --tenant {tenant_id} --scope scope" | ||||||
| with pytest.raises(CredentialUnavailableError, match=re.escape(expected_message)): | ||||||
| AzureCliCredential().get_token("scope", claims=claims, tenant_id=tenant_id) | ||||||
| else: # get_token_info | ||||||
| # Test with get_token_info - tenant passed via options | ||||||
| expected_message = f"Failed to get token. Run az login --claims-challenge {claims} --tenant {tenant_id} --scope scope" | ||||||
| with pytest.raises(CredentialUnavailableError, match=re.escape(expected_message)): | ||||||
| AzureCliCredential().get_token_info("scope", options={"claims": claims, "tenant_id": tenant_id}) | ||||||
|
|
||||||
|
|
||||||
| @pytest.mark.parametrize("get_token_method", GET_TOKEN_METHODS) | ||||||
| def test_claims_challenge_with_tenant_without_scopes(get_token_method): | ||||||
| """The credential should include tenant in error message when claims and tenant are provided but no scopes""" | ||||||
|
|
||||||
| claims = "test-claims-challenge" | ||||||
| tenant_id = "test-tenant-id" | ||||||
| expected_message = f"Failed to get token. Run az login --claims-challenge {claims} --tenant {tenant_id}" | ||||||
|
|
||||||
| if get_token_method == "get_token": | ||||||
| with pytest.raises(CredentialUnavailableError, match=re.escape(expected_message)): | ||||||
| AzureCliCredential().get_token(claims=claims, tenant_id=tenant_id) # No scopes provided | ||||||
| else: # get_token_info | ||||||
| with pytest.raises(CredentialUnavailableError, match=re.escape(expected_message)): | ||||||
| AzureCliCredential().get_token_info(options={"claims": claims, "tenant_id": tenant_id}) # No scopes provided | ||||||
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,126 @@ 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} --scope scope" | ||||||
|
|
||||||
| 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_claims_challenge_without_scopes(get_token_method): | ||||||
| """The credential should raise CredentialUnavailableError with appropriate message even when no scopes 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(claims=claims) # No scopes provided | ||||||
| else: # get_token_info | ||||||
| with pytest.raises(CredentialUnavailableError, match=re.escape(expected_message)): | ||||||
| await AzureCliCredential().get_token_info(options={"claims": claims}) # No scopes provided | ||||||
|
|
||||||
|
|
||||||
| @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Positive FeedbackNegative Feedback |
||||||
| "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" | ||||||
|
|
||||||
|
|
||||||
| @pytest.mark.parametrize("get_token_method", GET_TOKEN_METHODS) | ||||||
| @pytest.mark.asyncio | ||||||
| async def test_claims_challenge_with_tenant(get_token_method): | ||||||
| """The credential should include tenant in the error message when claims and tenant are provided""" | ||||||
|
|
||||||
| claims = "test-claims-challenge" | ||||||
| tenant_id = "test-tenant-id" | ||||||
|
|
||||||
| if get_token_method == "get_token": | ||||||
| # Test with get_token - tenant passed via get_token parameter (should appear in options) | ||||||
| expected_message = f"Failed to get token. Run az login --claims-challenge {claims} --tenant {tenant_id} --scope scope" | ||||||
| with pytest.raises(CredentialUnavailableError, match=re.escape(expected_message)): | ||||||
| await AzureCliCredential().get_token("scope", claims=claims, tenant_id=tenant_id) | ||||||
| else: # get_token_info | ||||||
| # Test with get_token_info - tenant passed via options | ||||||
| expected_message = f"Failed to get token. Run az login --claims-challenge {claims} --tenant {tenant_id} --scope scope" | ||||||
| with pytest.raises(CredentialUnavailableError, match=re.escape(expected_message)): | ||||||
| await AzureCliCredential().get_token_info("scope", options={"claims": claims, "tenant_id": tenant_id}) | ||||||
|
|
||||||
|
|
||||||
| @pytest.mark.parametrize("get_token_method", GET_TOKEN_METHODS) | ||||||
| @pytest.mark.asyncio | ||||||
| async def test_claims_challenge_with_tenant_without_scopes(get_token_method): | ||||||
| """The credential should include tenant in error message when claims and tenant are provided but no scopes""" | ||||||
|
|
||||||
| claims = "test-claims-challenge" | ||||||
| tenant_id = "test-tenant-id" | ||||||
| expected_message = f"Failed to get token. Run az login --claims-challenge {claims} --tenant {tenant_id}" | ||||||
|
|
||||||
| if get_token_method == "get_token": | ||||||
| with pytest.raises(CredentialUnavailableError, match=re.escape(expected_message)): | ||||||
| await AzureCliCredential().get_token(claims=claims, tenant_id=tenant_id) # No scopes provided | ||||||
| else: # get_token_info | ||||||
| with pytest.raises(CredentialUnavailableError, match=re.escape(expected_message)): | ||||||
| await AzureCliCredential().get_token_info(options={"claims": claims, "tenant_id": tenant_id}) # No scopes provided | ||||||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
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.
@copilot Extract the base error message into a constant that can be used in both azure_cli.py files, and make the error message more descriptive. Something like: "Claims challenge received, but not supported. To authenticate with the required claims, please run the following command: ". Then you can append the login_cmd to the message.