Remove application authentication APIs from stable 1.4.0 release by chlowell · Pull Request #12788 · Azure/azure-sdk-for-python

chlowell · 2020-07-29T23:45:01Z

This removes the application authentication APIs added in 1.4.0 betas. They will return in 1.5.0b1. Closes #12742.

…ic API

…cache

xiangyan99 · 2020-07-29T23:49:37Z

sdk/identity/azure-identity/azure/identity/_credentials/user_password.py

        # 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)


We don't want to honor "_authentication_record". why not just remove this line so we ignore "authentication_record"?

I like leaving the reading from kwargs. It lowers the code churn and makes it less error prone to reintroduce support for these kwargs

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.

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

I am confused.

If user passes in "authentication_record", do we want it break or not?

#1 will break it while #2 will not.

Seems to me you like neither of them. :)

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.

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.

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

… on Linux"

xiangyan99 · 2020-07-30T23:53:16Z

sdk/identity/azure-identity/tests/test_browser_credential.py

    credential = InteractiveBrowserCredential(
-        disable_automatic_authentication=True, transport=transport, _cache=empty_cache
+        _disable_automatic_authentication=True, transport=transport, _cache=empty_cache
    )


Seems to me they are invalid scenarios. Do we want to keep them?

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.

My concern is user uses this as reference or our samples.

But again, I will let you and @schaabs make the final call.

xiangyan99 · 2020-07-30T23:54:46Z

sdk/identity/azure-identity/azure/identity/_internal/shared_token_cache.py

        # 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]


Personally, I would like to use

# self._auth_record = kwargs.pop("authentication_record", None) self._auth_record = None

But I will let you to decide it.

xiangyan99 · 2020-08-04T16:14:06Z

sdk/identity/azure-identity/CHANGELOG.md

+  - Removed `allow_unencrypted_cache` keyword argument from
+    `SharedTokenCacheCredential`
+  - Removed classes `AuthenticationRecord` and `AuthenticationRequiredError`
+  - Removed `identity_config` keyword argument from `ManagedIdentityCredential`


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

chlowell added 8 commits July 29, 2020 13:02

exclude samples from sdist

d0a9216

authenticate -> _authenticate

ca1a935

remove AuthenticationRecord and AuthenticationRequiredError from publ…

505971c

…ic API

remove doc references

23c6102

_allow_unencrypted_cache, _authentication_record, _enable_persistent_…

5969ccd

…cache

SharedTokenCacheCredential allows unencrypted cache by default

ea94fbc

remove SharedTokenCacheCredential from DefaultAzureCredential on Linux

9697e03

update changelog

0899e7d

chlowell added Client This issue points to a problem in the data-plane of the library. Azure.Identity labels Jul 29, 2020

chlowell requested a review from xiangyan99 July 29, 2020 23:45

chlowell requested a review from schaabs as a code owner July 29, 2020 23:45

_disable_automatic_authentication

a00805e

xiangyan99 reviewed Jul 29, 2020

View reviewed changes

second draft of changelog entry

c430f04

schaabs reviewed Jul 30, 2020

View reviewed changes

sdk/identity/azure-identity/azure/identity/_internal/shared_token_cache.py Show resolved Hide resolved

chlowell added 2 commits July 30, 2020 08:05

Revert "remove SharedTokenCacheCredential from DefaultAzureCredential…

965bbcb

… on Linux"

remove samples

bddbb4e

chlowell requested a review from xiangyan99 July 30, 2020 23:45

xiangyan99 reviewed Jul 30, 2020

View reviewed changes

schaabs previously approved these changes Jul 31, 2020

View reviewed changes

identity_config -> _identity_config

a34dc78

chlowell dismissed schaabs’s stale review via a34dc78 August 3, 2020 21:55

chlowell requested review from schaabs and xiangyan99 August 4, 2020 15:08

xiangyan99 reviewed Aug 4, 2020

View reviewed changes

mention the silent acceptance of removed kwargs

5df1b40

chlowell requested a review from xiangyan99 August 4, 2020 17:22

xiangyan99 approved these changes Aug 4, 2020

View reviewed changes

chlowell merged commit d8dda15 into Azure:master Aug 5, 2020

chlowell deleted the remove-auth-api branch August 5, 2020 00:58

chlowell added a commit to chlowell/azure-sdk-for-python that referenced this pull request Aug 12, 2020

Revert "Remove application authentication APIs from stable 1.4.0 rele…

9e6018c

…ase (Azure#12788)" This reverts commit d8dda15

chlowell mentioned this pull request Aug 12, 2020

Restore user authentication API #13070

Merged

Conversation

chlowell commented Jul 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants