Skip to content

Persistent caching for interactive credentials on Linux and macOS#11319

Merged
chlowell merged 8 commits intoAzure:masterfrom
chlowell:upgrade-msal-extensions
May 12, 2020
Merged

Persistent caching for interactive credentials on Linux and macOS#11319
chlowell merged 8 commits intoAzure:masterfrom
chlowell:upgrade-msal-extensions

Conversation

@chlowell
Copy link
Member

@chlowell chlowell commented May 7, 2020

This adopts msal-extensions 0.2.2 to enable persistent token caching on Linux and macOS for DeviceCodeCredential and InteractiveBrowserCredential. It also adds a new keyword argument for those credentials, allow_unencrypted_cache, which determines whether to fall back to a plaintext cache on Linux when libsecret or PyGObject are unavailable (defaults False).

Strings and paths for the new platforms are from Azure/azure-sdk-for-java#9188 and the default behavior of MSAL's Java extension library.

@chlowell chlowell added Client This issue points to a problem in the data-plane of the library. Azure.Identity labels May 7, 2020
@chlowell chlowell requested a review from xiangyan99 May 7, 2020 23:06
@chlowell chlowell requested a review from schaabs as a code owner May 7, 2020 23:06
@chlowell chlowell self-assigned this May 7, 2020
if not self._cache:
if kwargs.pop("enable_persistent_cache", False):
self._cache = _load_persistent_cache()
allow_unencrypted = kwargs.pop("allow_unencrypted_cache", False)
Copy link
Member

Choose a reason for hiding this comment

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

Is this a public kwarg?

I don't see it in docstring.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Check the diffs for browser.py and device_code.py.

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean users are not supposed to use MsalCredential directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

@chlowell chlowell requested a review from xiangyan99 May 11, 2020 18:57
)
persistence = msal_extensions.FilePersistence(file_path)
else:
raise NotImplementedError("A persistent cache is not available on this platform.")
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we should give more accurate error message if it is used on windows by "LOCALAPPDATA" is not set?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you have in mind for a more accurate message? Something like "Environment variable LOCALAPPDATA isn't set"?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. If I see "A persistent cache is not available on this platform" on windows, I will feel very confused. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with you there. I'm content with simply saying the cache is unavailable in this case but I think saying it's unavailable on this platform is misleading. I changed the message to say the cache is unavailable in this environment instead.

xiangyan99
xiangyan99 previously approved these changes May 12, 2020
@chlowell chlowell merged commit ef77ed4 into Azure:master May 12, 2020
@chlowell chlowell deleted the upgrade-msal-extensions branch May 12, 2020 22:49
iscai-msft added a commit that referenced this pull request May 13, 2020
…into move_get_client_for

* 'master' of https://github.com/Azure/azure-sdk-for-python:
  [Cosmos] Remove unused files (#11388)
  Sync eng/common directory with azure-sdk-tools repository (#11417)
  [form recognizer] consistency renames for FormTrainingClient (#11390)
  Release for azure mgmt eventhub (#11403)
  Network 2020 04 01 (#11405)
  link in to reference docs for sub-clients (#11396)
  Persistent caching for interactive credentials on Linux and macOS (#11319)
  [formrecognizer] add AAD auth support (#11275)
  Search docs/readme updates (#11391)
  Sync eng/common directory with azure-sdk-tools repository (#11387)
  Update tests for msal 1.3.0 (#11307)
iscai-msft added a commit that referenced this pull request May 18, 2020
…into feature/text_analytics_v3.0

* 'master' of https://github.com/Azure/azure-sdk-for-python: (128 commits)
  add more content to index crud samples (#11443)
  Add a snippet to the Samples readme mirroring the core readme, guiding users to the currently mainline version of the lib if the end up on the wrong page. (#11420)
  20200515 run resource live test (#11454)
  Release azure mgmt eventgrid (#11431)
  Smoke Tests use new workflow for package install (#11438)
  ServiceFabric 7.1 (#11451)
  update (#11424)
  Typing for appconfiguration (#11427)
  [form recognizer] Move `get_client` method from FormRecognizer -> FormTraining (#11423)
  release customvision (#11428)
  Enforce https in search (#11337)
  [Cosmos] Remove unused files (#11388)
  Sync eng/common directory with azure-sdk-tools repository (#11417)
  [form recognizer] consistency renames for FormTrainingClient (#11390)
  Release for azure mgmt eventhub (#11403)
  Network 2020 04 01 (#11405)
  link in to reference docs for sub-clients (#11396)
  Persistent caching for interactive credentials on Linux and macOS (#11319)
  [formrecognizer] add AAD auth support (#11275)
  Search docs/readme updates (#11391)
  ...
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.

2 participants