Skip to content
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

Do not require AZURE_USERNAME for shared cache #8095

Merged
merged 4 commits into from
Oct 24, 2019

Conversation

ellismg
Copy link
Member

@ellismg ellismg commented Oct 21, 2019

Previously, a username was required when using the
SharedTokenCacheCredential, in order to handle the case where multiple
identities were found in the cache. Since it is common to have only a
single account in your user cache (e.g. you have signed in with only a
single identity), we should allow reading from the cache even when an
explicit AZURE_USERNAME is not specified, if there is exactly one
account in the cache.

When username is unset, if we can not find a token in the cache or we
find multiple tokens, a ClientAuthenticationError error is raised,
with the text "No cached token found". This is similar to how other
cache related failures are handled by the API (they raise this error
with similar text but it includes a hint about what username was used.)

As part of this work, DefaultAzureCredential now unconditionally uses
the shared cache on supported platforms.

This behavior matches how we handle this case in both the .NET and Java
SDKs.

Fixes #7944

@ellismg ellismg requested a review from chlowell October 21, 2019 22:07
@ellismg ellismg requested a review from schaabs as a code owner October 21, 2019 22:07
@adxsdk6
Copy link

adxsdk6 commented Oct 21, 2019

Can one of the admins verify this patch?

@ellismg ellismg force-pushed the ellismg/fix-7944 branch 2 times, most recently from 7c8081e to f34ff97 Compare October 23, 2019 16:47
@mayurid mayurid assigned ellismg and BernieEllis and unassigned BernieEllis Oct 23, 2019
@mayurid mayurid added Azure.Identity blocking-release Blocks release Client This issue points to a problem in the data-plane of the library. labels Oct 23, 2019
@ellismg
Copy link
Member Author

ellismg commented Oct 24, 2019

Okay @chlowell and @schaabs, based on our in person discussions, I have updated the code. Once you have signed off, I'll contact shiproom.

Previously, a username was required when using the
SharedTokenCacheCredential, in order to handle the case where multiple
identities were found in the cache. Since it is common to have only a
single account in your user cache (e.g. you have signed in with only a
single identity), we should allow reading from the cache even when an
explicit AZURE_USERNAME is not specified, if there is exactly one
account in the cache.

When username is unset, if we can not find a token in the cache or we
find multiple tokens, a `ClientAuthenticationError` error is raised,
with the text "No cached token found".  This is similar to how other
cache related failures are handled by the API (they raise this error
with similar text but it includes a hint about what username was used.)

As part of this work, `DefaultAzureCredential` now unconditionally uses
the shared cache on supported platforms.

This behavior matches how we handle this case in both the .NET and Java
SDKs.

Fixes Azure#7944
Copy link
Member

@chlowell chlowell left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for making it happen 🎂

@joshfree joshfree requested a review from jianghaolu October 24, 2019 17:49
@ellismg ellismg merged commit de802a5 into Azure:master Oct 24, 2019
@ellismg ellismg deleted the ellismg/fix-7944 branch October 24, 2019 18:16
fengzhou-msft pushed a commit that referenced this pull request Nov 5, 2019
Previously, a username was required when using the
SharedTokenCacheCredential, in order to handle the case where multiple
identities were found in the cache. Since it is common to have only a
single account in your user cache (e.g. you have signed in with only a
single identity), we should allow reading from the cache even when an
explicit AZURE_USERNAME is not specified, if there is exactly one
account in the cache.

When username is unset, if we can not find a token in the cache or we
find multiple tokens, a `ClientAuthenticationError` error is raised,
with the text "No cached token found".  This is similar to how other
cache related failures are handled by the API (they raise this error
with similar text but it includes a hint about what username was used.)

As part of this work, `DefaultAzureCredential` now unconditionally uses
the shared cache on supported platforms.

This behavior matches how we handle this case in both the .NET and Java
SDKs.

Fixes #7944
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Identity blocking-release Blocks release 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.

Should we requrie AZURE_USERNAME being set to use the shared cache from DefaultAzureCredential?
8 participants