Skip to content

Make explicit that auth file is not supported anymore #17395

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.

Sign up for GitHub

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

Merged
lmazuel merged 1 commit into from
Mar 18, 2021
Merged

Make explicit that auth file is not supported anymore #17395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions sdk/core/azure-common/azure/common/client_factory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,14 @@ def get_client_from_cli_profile(client_class, **kwargs):
return _instantiate_client(client_class, **parameters)


def _is_autorest_v3(client_class):
"""Is this client a autorestv3/track2 one?.
Could be refined later if necessary.
"""
args = get_arg_spec(client_class.__init__).args
return "credential" in args


def get_client_from_json_dict(client_class, config_dict, **kwargs):
"""Return a SDK client initialized with a JSON auth dict.

Expand Down Expand Up @@ -152,6 +160,12 @@ def get_client_from_json_dict(client_class, config_dict, **kwargs):
:param dict config_dict: A config dict.
:return: An instantiated client
"""
if _is_autorest_v3(client_class):
raise ValueError(
"Auth file or JSON dict are deprecated auth approach and are not supported anymore. "
Copy link
Member

@mccoyp mccoyp Mar 17, 2021

Choose a reason for hiding this comment

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

This implies that these approaches are generally deprecated -- should the deprecation be specific to track 2 clients? e.g. by saying something like "azure-common factory methods are deprecated for client_class.__name__ and are not supported anymore."

Copy link
Member Author

@lmazuel lmazuel Mar 17, 2021

Choose a reason for hiding this comment

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

It's under a "track2" if statement, is that not enough you think?

Copy link
Member

@mccoyp mccoyp Mar 17, 2021

Choose a reason for hiding this comment

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

I think it should cover the cases where this should be raised, but I just wonder if users would be led to think that these factory methods wouldn't work for non-deprecated track 1 clients as well. Does that make sense?

Copy link
Member

@mccoyp mccoyp Mar 17, 2021

Choose a reason for hiding this comment

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

This is assuming that most users wouldn't look at the source code to notice that the error is specific to this condition

Copy link
Member Author

@lmazuel lmazuel Mar 18, 2021

Choose a reason for hiding this comment

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

Well, I think we don't love auth file, and therefore being sligtly wide in the message is ok to me. But this is a good point to think.

"Please read https://aka.ms/azsdk/python/azidmigration for details"
)

import adal
from msrestazure.azure_active_directory import AdalAuthentication

Expand Down
13 changes: 13 additions & 0 deletions sdk/core/azure-common/tests/test_client_factory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import os
import tempfile
import unittest
import pytest
try:
from unittest import mock
except ImportError:
Expand Down Expand Up @@ -112,6 +113,7 @@ def __init__(self, credentials):
get_azure_cli_credentials.assert_called_with(resource="https://vault.azure.net", with_tenant=True)
assert client.credentials == 'credentials'


@mock.patch('azure.common.client_factory.get_cli_active_cloud')
@mock.patch('azure.common.client_factory.get_azure_cli_credentials')
def test_get_client_from_cli_profile_core(self, get_azure_cli_credentials, get_cli_active_cloud):
Expand Down Expand Up @@ -225,6 +227,13 @@ def __init__(self, credentials):

self.credentials = credentials

class KeyVaultClientTrack2(object):
def __init__(self, credential):
if credential is None:
raise ValueError("Parameter 'credentials' must not be None.")

self.credential = credential

for encoding in ['utf-8', 'utf-8-sig', 'ascii']:

temp_auth_file = tempfile.NamedTemporaryFile(delete=False)
Expand Down Expand Up @@ -279,6 +288,10 @@ def __init__(self, credentials):
'password'
)

with pytest.raises(ValueError) as excinfo:
get_client_from_auth_file(KeyVaultClientTrack2, temp_auth_file.name)
assert "https://aka.ms/azsdk/python/azidmigration" in str(excinfo.value)

os.unlink(temp_auth_file.name)


Expand Down