-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Remove application authentication APIs from stable 1.4.0 release #12788
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
Changes from all commits
d0a9216
ca1a935
505971c
23c6102
5969ccd
ea94fbc
9697e03
0899e7d
a00805e
c430f04
965bbcb
bddbb4e
a34dc78
5df1b40
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,3 @@ | ||
| recursive-include samples *.py | ||
| recursive-include tests *.py | ||
| include *.md | ||
| include azure/__init__.py |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,10 +33,6 @@ class UsernamePasswordCredential(InteractiveCredential): | |
| defines authorities for other clouds. | ||
| :keyword str tenant_id: tenant ID or a domain associated with a tenant. If not provided, defaults to the | ||
| 'organizations' tenant, which supports only Azure Active Directory work or school accounts. | ||
| :keyword bool enable_persistent_cache: if True, the credential will store tokens in a persistent cache shared by | ||
| other user credentials. Defaults to False. | ||
| :keyword bool allow_unencrypted_cache: if True, the credential will fall back to a plaintext cache on platforms | ||
| where encryption is unavailable. Default to False. Has no effect when `enable_persistent_cache` is False. | ||
| """ | ||
|
|
||
| def __init__(self, client_id, username, password, **kwargs): | ||
|
|
@@ -46,7 +42,7 @@ def __init__(self, client_id, username, password, **kwargs): | |
| # first time it's asked for a token. However, we want to ensure this first authentication is not silent, to | ||
| # validate the given password. This class therefore doesn't document the authentication_record argument, and we | ||
| # discard it here. | ||
| kwargs.pop("authentication_record", None) | ||
| kwargs.pop("_authentication_record", None) | ||
|
Member
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. We don't want to honor "_authentication_record". why not just remove this line so we ignore "authentication_record"? 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. I like leaving the reading from kwargs. It lowers the code churn and makes it less error prone to reintroduce support for these kwargs
Member
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.
Member
Author
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.
Member
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.
Member
Author
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. I'm sorry to have made confusion. I was responding to each of your points, not laying out a plan.
We want to release 1.4.0 without the application authentication and cache configuration APIs present in 1.4.0b7. Not breaking code using these APIs is tantamount to including them in the stable release. So, we want it to break.
Member
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. Thanks for the clarification. So I think just commenting this line will be good enough, right? The only difference for adding "kwargs.pop("_authentication_record", None)" is if user passes "_authentication_record" (which is not a supported scenario), it will work in this release but will be broken when we bring the feature back. I don't think it is our intension to make this happen.
Member
Author
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.
Good enough for breaking the code we want to break but at the cost of breaking our tests. Skipping those would introduce a risk of unintentionally breaking features we want in our next beta. Also, the specific case you commented on here is a simple one because the argument is discarded. In other cases commenting a single line isn't enough because the argument is used.
We don't support unsupported features. |
||
| super(UsernamePasswordCredential, self).__init__(client_id=client_id, **kwargs) | ||
| self._username = username | ||
| self._password = password | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,7 @@ | |
| # pylint:disable=unused-import,ungrouped-imports | ||
| from typing import Any, Iterable, List, Mapping, Optional | ||
| from .._internal import AadClientBase | ||
| from azure.identity import AuthenticationRecord | ||
| from azure.identity._auth_record import AuthenticationRecord | ||
|
|
||
| CacheItem = Mapping[str, str] | ||
|
|
||
|
|
@@ -90,7 +90,7 @@ class SharedTokenCacheBase(ABC): | |
| def __init__(self, username=None, **kwargs): # pylint:disable=unused-argument | ||
| # type: (Optional[str], **Any) -> None | ||
|
|
||
| self._auth_record = kwargs.pop("authentication_record", None) # type: Optional[AuthenticationRecord] | ||
| self._auth_record = kwargs.pop("_authentication_record", None) # type: Optional[AuthenticationRecord] | ||
|
Member
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. Personally, I would like to use # self._auth_record = kwargs.pop("authentication_record", None)
self._auth_record = NoneBut I will let you to decide it. |
||
| if self._auth_record: | ||
| # authenticate in the tenant that produced the record unless 'tenant_id' specifies another | ||
| authenticating_tenant = kwargs.pop("tenant_id", None) or self._auth_record.tenant_id | ||
|
|
@@ -118,7 +118,7 @@ def _initialize(self): | |
| return | ||
|
|
||
| if not self._cache and self.supported(): | ||
| allow_unencrypted = self._client_kwargs.get("allow_unencrypted_cache", False) | ||
| allow_unencrypted = self._client_kwargs.get("_allow_unencrypted_cache", True) | ||
| try: | ||
| self._cache = load_user_cache(allow_unencrypted) | ||
| except Exception: # pylint:disable=broad-except | ||
|
|
||
This file was deleted.
This file was deleted.
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.
Do we want to describe more on the effect?
e.g. those kwargs will be ignored or will cause exception?