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 @@ -70,6 +70,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, a system-assigned identity will be used.
xiangyan99 marked this conversation as resolved.
Show resolved Hide resolved
: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 +104,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")
xiangyan99 marked this conversation as resolved.
Show resolved Hide resolved

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 +140,13 @@ 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:
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, "tenant_id": "72f988bf-86f1-41af-91ab-2d7cd011db47"}
xiangyan99 marked this conversation as resolved.
Show resolved Hide resolved

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