Skip to content

Conversation

@chlowell
Copy link
Member

@chlowell chlowell commented May 26, 2020

This enables users who want to authenticate silently only to initialize the credential with an AuthenticationRecord from a prior authentication. Closes #11448.

@chlowell chlowell added Client This issue points to a problem in the data-plane of the library. Azure.Identity labels May 26, 2020
@chlowell chlowell requested a review from xiangyan99 May 26, 2020 22:00
@chlowell chlowell requested a review from schaabs as a code owner May 26, 2020 22:00
Copy link
Member

Choose a reason for hiding this comment

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

Why not from ..__auth_record?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a public class, and I don't want to depend on its internal location.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see we have normalize_authority(authority) in ctor of AuthenticationRecord. So maybe we need it here

Copy link
Member Author

Choose a reason for hiding this comment

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

Accounts in MSAL's cache don't have "normalized" authorities, so neither does AuthenticationRecord. This credential's client will normalize the authority it's given.

Copy link
Member

Choose a reason for hiding this comment

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

I expect w/ or w/o auth_record, we should have same behavior here as

environment = urlparse(self._authority).netloc
self._environment_aliases = KNOWN_ALIASES.get(environment) or frozenset((environment,))
?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good eye. We don't need to parse the netloc out of the record's authority because the record's authority is already a netloc. As for why I'm ignoring potential aliases: the purpose of the record is to enable an application to use tokens it cached in prior authentications. An AuthenticationRecord and the account it represents will have identical values for authority because they're both results of the same authentication. An account having an alias of the record's authority was cached during a different authentication, and therefore doesn't match the record.

Copy link
Member

Choose a reason for hiding this comment

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

Do we need try catch here? if it is an invalid auth_record, what's our expected behavior?

Copy link
Member

Choose a reason for hiding this comment

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

I mean if it is invalid, we want to raise ValueError or CredentialUnavailableError or ignore it?

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean if self._auth_record.home_account_id raises AttributeError? In that case self._auth_record isn't an AuthenticationRecord. Handling that exception seems tantamount to an isinstance check.

Copy link
Contributor

@schaabs schaabs Jun 4, 2020

Choose a reason for hiding this comment

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

If we're using the Azure CLI client id I believe we should be defaulting to the organizations endpoint rather than common.

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 kept common to maintain the existing behavior. That was probably too conservative given I don't know a scenario the change would break. So, I've switched to organizations.

@chlowell chlowell force-pushed the shared-cache-auth-record branch from b86790f to d6458ff Compare June 4, 2020 16:13
@chlowell chlowell requested review from schaabs and xiangyan99 June 4, 2020 19:31
@chlowell chlowell merged commit 994c77d into Azure:master Jun 5, 2020
@chlowell chlowell deleted the shared-cache-auth-record branch June 5, 2020 18:17
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.

SharedTokenCacheCredential accepts an AuthenticationRecord

3 participants