Remove application authentication APIs from stable 1.4.0 release #12788
Conversation
chlowell
added
Client
| @@ -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) | |||
There was a problem hiding this comment.
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.
I like leaving the reading from kwargs. It lowers the code churn and makes it less error prone to reintroduce support for these kwargs
There was a problem hiding this comment.
- We can comment this line if pylint is not angry.
- We can keep this line, it does not hurt.
- The only possible side effect to rename it is if user uses "_authentication_record", they will get broken when we re-enable the feature. - not too bad, but not good.
There was a problem hiding this comment.
- Commenting the line breaks tests. We can skip them but then we may not catch regressions that complicate reverting these changes.
- If we keep this and similar lines, code written against 1.4.0b7 will continue to work in 1.4.0 despite using unsupported features.
- "_authentication_record" is underscored and undocumented: clearly not part of the public API
There was a problem hiding this comment.
I'm sorry to have made confusion. I was responding to each of your points, not laying out a plan.
If user passes in "authentication_record", do we want it break or not?
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
So I think just commenting this line will be good enough, right?
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.
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.
We don't support unsupported features.
sdk/identity/azure-identity/azure/identity/_internal/shared_token_cache.py
Outdated
Show resolved
Hide resolved
| @@ -100,7 +101,7 @@ def test_disable_automatic_authentication(): | |||
| empty_cache = TokenCache() # empty cache makes silent auth impossible | |||
| transport = Mock(send=Mock(side_effect=Exception("no request should be sent"))) | |||
| credential = InteractiveBrowserCredential( | |||
| disable_automatic_authentication=True, transport=transport, _cache=empty_cache | |||
| _disable_automatic_authentication=True, transport=transport, _cache=empty_cache | |||
| ) | |||
There was a problem hiding this comment.
Seems to me they are invalid scenarios. Do we want to keep them?
There was a problem hiding this comment.
The tests? I think we do. We're disconnecting the features from the public API, not removing them. We want them to continue functioning because we'll make them public again in the next beta.
There was a problem hiding this comment.
My concern is user uses this as reference or our samples.
But again, I will let you and @schaabs make the final call.
| @@ -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] | |||
There was a problem hiding this comment.
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.
| - Removed `allow_unencrypted_cache` keyword argument from | ||
| `SharedTokenCacheCredential` | ||
| - Removed classes `AuthenticationRecord` and `AuthenticationRequiredError` | ||
| - Removed `identity_config` keyword argument from `ManagedIdentityCredential` |
There was a problem hiding this comment.
Do we want to describe more on the effect?
e.g. those kwargs will be ignored or will cause exception?
…ase (Azure#12788)" This reverts commit d8dda15
This removes the application authentication APIs added in 1.4.0 betas. They will return in 1.5.0b1. Closes #12742.