-
Notifications
You must be signed in to change notification settings - Fork 1
User authentication API #4
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
Conversation
0ac48c7 to
f004cfa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and account.get("realm") == self._profile.tenant_id [](start = 11, length = 52)
this check is incorrect based on my test, the value of account.get('realm') is 'organizations'. I think we do not need this check since home_account_id include the tenant id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I specify tenant_id like
credential, profile = InteractiveBrowserCredential.authenticate(
# silent_auth_only=True
scope = 'https://management.azure.com/.default',
tenant_id = tenant
)
The value of realm is the tenant_id. But in this scenario, the cached account still not match the profile. The home_account_id in the profile is correct, but the home_account_id in the MSAL cache is my organization tenants's home_account_id
In reply to: 396249087 [](ancestors = 396249087)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're correct we needn't check the realm, thank you for pointing that out.
sdk/identity/azure-identity/azure/identity/_credentials/user.py
Outdated
Show resolved
Hide resolved
sdk/identity/azure-identity/azure/identity/_credentials/user.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return AccessToken(token["access_token"], now + int(token["expires_in"])) [](start = 20, length = 73)
can you add token_type in AccessToken which is needed for client to use, by default token_type is "Bearer"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the documentation I've read (for example) Azure AD provides only bearer tokens in the flows we use. What other types do you expect, and how does the client use them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we add this type in the authorization head of http request. AAD may add new type in the future so that I would prefer not hard-code in client code. Currently CLI get the value from ADAL token information.
In reply to: 398762506 [](ancestors = 398762506)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to extend AccessToken without a definite use case. If AAD adds a new token type in the future, using it in azure-identity will require design and implementation work I don't want to try to preempt today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
) [](start = 8, length = 1)
sorry that I was wrong, the home_account_id is the home_tenent_id, we still need and account.get("realm") == self._profile.tenant_id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practical terms I think home_account_id alone is sufficient because it's the only detail of the account MSAL uses to find a token: finding an access token, finding a refresh token. The environment and realm (tenant) are set when the credential's msal.PublicClientApplication instance is created. So it looks like this filter could be much simpler: take the first account with a matching home_account_id.
I want to be sure my understanding is correct though. Does the above make sense to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, since you will loop the accounts (may have multiple tenant accounts per user account) to try to acquire access token, I think this behavior is correct.
In reply to: 398946277 [](ancestors = 398946277)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can let client_id default to None? Thus we can automatically use AZURE_CLI_CLIENT_ID. Otherwise,
credential, profile = InteractiveBrowserCredential.authenticate(
silent_auth_only=True,
scope='https://management.azure.com/.default'
)gives
TypeError: authenticate() missing 1 required positional argument: 'client_id'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API is intended for applications beyond the CLI, which we don't want unintentionally authenticating users to the CLI. It's true InteractiveBrowserCredential.get_token does that today, but that was designed for a different scenario and in a future release its implicit default client id will change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Thanks for the clarification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I know the difference between SharedTokenCache and DeviceCodeCredential, InteractiveBrowserCredential, UsernamePasswordCredential with profile provided?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SharedTokenCacheCredential handles the cache directly, essentially reimplementing parts of msal to support a narrow scenario. The other credentials you list defer caching entirely to msal. I intend to remove all the changes to SharedTokenCacheCredential from this PR. It will continue to support its particular scenario and won't be part of the new user authentication API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am actually thinking if we can provide a helper for cross-tenant get_token or simple support credential.get_token(tenant), since the credential already has everything we need, including _profile, _username, environment, etc.
credential, msal_profile = InteractiveBrowserCredential.authenticate(
client_id=_CLIENT_ID,
silent_auth_only=True,
scope='https://management.azure.com/.default'
)
specific_tenant_credential = SharedTokenCacheCredential(username=credential._profile.username,
authority=credential._profile.environment,
tenant_id=tenant_id)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're correct the credential has enough information to authenticate in arbitrary tenants. However, credential and msal application instances are designed to authenticate in a single tenant. Revising that design for credentials requires careful thought and a (likely clumsy) workaround for msal.
I expect we'll keep the current design of a credential instance representing a single identity in a single tenant. For now, here's a way to create a credential instance per tenant:
credential, profile = InteractiveBrowserCredential.authenticate(client_id=_CLIENT_ID)
for other_tenant in tenants:
new_profile = AuthProfile(
environment=profile.environment,
home_account_id=profile.home_account_id,
tenant_id=other_tenant,
username=profile.username
)
new_credential = InteractiveBrowserCredential(
client_id=_CLIENT_ID,
profile=new_profile,
silent_auth_only=True
)We can at least make this less awkward in a future revision.
3742edc to
848fe69
Compare
| if not token: | ||
| if self._silent_auth_only: | ||
| raise AuthenticationRequiredError() | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add the MSAL error result in the error exception so that client side can get the fail reason when get the access token
|
Thanks for all the feedback. This PR is superseded by Azure#10612. Breaking changes between this and that are:
There may be more before the merge. |
usage
az login
az command ...