Skip to content

Handle ImportError when constructing DefaultAzureCredential#8294

Merged
chlowell merged 4 commits intoAzure:masterfrom
chlowell:default-38
Nov 13, 2019
Merged

Handle ImportError when constructing DefaultAzureCredential#8294
chlowell merged 4 commits intoAzure:masterfrom
chlowell:default-38

Conversation

@chlowell
Copy link
Copy Markdown
Member

As described in #8292, SharedTokenCacheCredential raises an ImportError in Python 3.8 running on Windows. DefaultAzureCredential is therefore unusable on 3.8/win because it constructs SharedTokenCacheCredential. With this change, DefaultAzureCredential expects the exception and will not attempt to use the shared token cache on 3.8/win.

With this change:

  • DefaultAzureCredential works on 3.8/win minus the shared token cache, whose unavailability it logs
  • SharedTokenCacheCredential still allows the exception to propagate

@chlowell chlowell added Client This issue points to a problem in the data-plane of the library. Azure.Identity labels Oct 30, 2019
@chlowell chlowell requested a review from lmazuel October 30, 2019 16:55
@chlowell chlowell requested a review from schaabs as a code owner October 30, 2019 16:55
@chlowell chlowell self-assigned this Oct 30, 2019
@adxsdk6
Copy link
Copy Markdown

adxsdk6 commented Oct 30, 2019

Can one of the admins verify this patch?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would make this guy bulletproof, and catch whatever could happen, and not just ImportError. Thoughts?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I was specific here because any other exception is a new bug I want to know about, and I don't want to rely on e.g. a test case expecting ImportError from SharedTokenCacheCredential() to alert me to such bugs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I understand your point, from a dev perspective it makes sense, it's just on customer's machine, we won't be able to recover from yet another issue from "msal_extensions", where a generic except would allow to.
I was more thinking "if anything goes wrong while trying to create the cache, skip it", that would still allow a customer to authenticate even in panic unexpected error from the cache.

My remark being just a suggestion, approved it :)

@chlowell chlowell merged commit 3e878ff into Azure:master Nov 13, 2019
@chlowell chlowell deleted the default-38 branch November 13, 2019 23:32
xiangyan99 added a commit that referenced this pull request Nov 15, 2019
* InteractiveBrowserCredential prompts for account selection (#8470)

* remove *.yaml under tests folder from manifest.in (#8625)

* [AutoPR] cosmos-db/resource-manager (#8560)

* regenerated

* fix script

* another place

* fix

* fix

* one more change

* one more

* undo multiapi script

* regeneated

* undo multiapi script

* history, version + additional fix

* regenerated

* fixed test

* fixed generation mistake

* added more breaking changes to history

* regenerated again

* additional fixes

* udpated test names

* test recordings

* Update README.md (#8628)

Add standard section on security issues below the "file an issue via GitHub Issues' bullet.

* Handle exceptions when constructing DefaultAzureCredential (#8294)

* Python 3.8 test pipeline is not cancelled when build is cancelled (#8544)

* Skip python 3.8 testing when CI is cancelled

* Cosmos docstring review (#8607)

* cosmos docstring edits

* user-defined

* trailing whitespace

* xfail flaky emulator tests

* review feedback

* Default credentials are configurable by kwargs (#8514)

* Nightly build is failing due version conflict between storage datalak… (#8635)

* Nightly build is failing due version conflict between storage datalake and storage blob

* InkRecognizer to use GA azure-core (#8341)

* InkRecognizer to use Ga azure-core

* Disable auto-pr update

* Azconfig remove tests from whl (#8663)

* update tests

* ignoring the azure folder directly (#8673)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Azure.Identity Client This issue points to a problem in the data-plane of the library.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants