Skip to content
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

add interactive_browser_client_id #20591

Merged
merged 11 commits into from
Sep 8, 2021
2 changes: 2 additions & 0 deletions sdk/identity/azure-identity/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
- `OnBehalfOfCredential` supports the on-behalf-of authentication flow for
accessing resources on behalf of users
([#19308](https://github.com/Azure/azure-sdk-for-python/issues/19308))
- `DefaultAzureCredential` allows specifying the client ID of interactive browser via keyword argument `interactive_browser_client_id`
([#20487](https://github.com/Azure/azure-sdk-for-python/issues/20487))

### Breaking Changes

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
# ------------------------------------
import logging
import os
import six
Copy link
Member

Choose a reason for hiding this comment

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

This is unused now


from .._constants import EnvironmentVariables
from .._internal import get_default_authority, normalize_authority
Expand Down Expand Up @@ -70,6 +71,8 @@ class DefaultAzureCredential(ChainedTokenCredential):
AZURE_TENANT_ID, if any. If unspecified, users will authenticate in their home tenants.
:keyword str managed_identity_client_id: The client ID of a user-assigned managed identity. Defaults to the value
of the environment variable AZURE_CLIENT_ID, if any. If not specified, a system-assigned identity will be used.
:keyword str interactive_browser_client_id: The client ID to be used in interactive browser credential. If not
specified, users will authenticate to an Azure development application.
:keyword str shared_cache_username: Preferred username for :class:`~azure.identity.SharedTokenCacheCredential`.
Defaults to the value of environment variable AZURE_USERNAME, if any.
:keyword str shared_cache_tenant_id: Preferred tenant for :class:`~azure.identity.SharedTokenCacheCredential`.
Expand Down Expand Up @@ -102,6 +105,7 @@ def __init__(self, **kwargs):
managed_identity_client_id = kwargs.pop(
"managed_identity_client_id", os.environ.get(EnvironmentVariables.AZURE_CLIENT_ID)
)
interactive_browser_client_id = kwargs.pop("interactive_browser_client_id", None)

shared_cache_username = kwargs.pop("shared_cache_username", os.environ.get(EnvironmentVariables.AZURE_USERNAME))
shared_cache_tenant_id = kwargs.pop(
Expand Down Expand Up @@ -137,7 +141,15 @@ def __init__(self, **kwargs):
if not exclude_powershell_credential:
credentials.append(AzurePowerShellCredential(**kwargs))
if not exclude_interactive_browser_credential:
credentials.append(InteractiveBrowserCredential(tenant_id=interactive_browser_tenant_id, **kwargs))
if interactive_browser_client_id:
if not isinstance(interactive_browser_client_id, six.string_types):
raise ValueError('"interactive_browser_client_id" must be a string')
credentials.append(InteractiveBrowserCredential(
tenant_id=interactive_browser_tenant_id,
client_id=interactive_browser_client_id,
Copy link
Member

Choose a reason for hiding this comment

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

InteractiveBrowserCredential expects this argument to be set only with a valid client ID (i.e. a string). So, we should pass a value here only when interactive_browser_client_id has a string value.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see we do similar validation for other client ids. Should we keep consistent with others?

Copy link
Member

Choose a reason for hiding this comment

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

InteractiveBrowserCredential(client_id=None, ...) creates a credential that doesn't work. ManagedIdentityCredential(client_id=None) creates a credential that could work, depending on the environment. Maybe both credentials should be consistent in how they handle client_id=None but they aren't today. (For what it's worth, I think InteractiveBrowserCredential is more correct here.)

**kwargs))
xiangyan99 marked this conversation as resolved.
Show resolved Hide resolved
else:
credentials.append(InteractiveBrowserCredential(tenant_id=interactive_browser_tenant_id, **kwargs))

super(DefaultAzureCredential, self).__init__(*credentials)

Expand Down
15 changes: 15 additions & 0 deletions sdk/identity/azure-identity/tests/test_default.py
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,21 @@ def validate_tenant_id(credential):
validate_tenant_id(mock_credential)


def test_interactive_browser_client_id():
"""the credential should allow configuring a client ID for InteractiveBrowserCredential by kwarg"""

client_id = "client-id"

def validate_client_id(credential):
assert len(credential.call_args_list) == 1, "InteractiveBrowserCredential should be instantiated once"
_, kwargs = credential.call_args
assert kwargs["client_id"] == client_id

with patch(DefaultAzureCredential.__module__ + ".InteractiveBrowserCredential") as mock_credential:
DefaultAzureCredential(exclude_interactive_browser_credential=False, interactive_browser_client_id=client_id)
validate_client_id(mock_credential)


@pytest.mark.parametrize("expected_value", (True, False))
def test_allow_multitenant_authentication(expected_value):
"""the credential should pass "allow_multitenant_authentication" to the inner credentials which support it"""
Expand Down