-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[Core] Conditional Access: Show --scope for az login message when failed to refresh the access token
#17738
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
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 _get_token never works because it doesn't comply with KeyVaultAuthentication's authorization_callback signature.
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.
As pointed by #15854, calling the protected method _token_retriever is simply wrong.
Switch to calling azure.cli.command_modules.keyvault._client_factory.keyvault_data_plane_factory instead which calls the correct get_raw_token method provided by #15805 (comment).
Appconfig already adopted this approach in #15826.
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.
To support being called as keyvault_data_plane_factory(cli_ctx).
|
Show --scope for az login message when failed to refresh the access token |
yonzhan
left a comment
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.
Nice
# Conflicts: # .github/CODEOWNERS
| def setUp(self): | ||
| patch_main_exception_handler(self) |
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.
azure.cli.core.util.handle_exception is patched for ScenarioTest. We have to patch azure.cli.core.util.handle_exception for LiveScenarioTest as well. Otherwise, AuthenticationError can't be checked in tests.
--scope for az login message--scope for az login message when failed to refresh the access token
|
Some more information I provided in an internal ticket: Why token refreshing failsConsider the following scenario:
Without If ARM doesn’t have Conditional Access policies enforced, the refresh token won’t be compliant with MS Graph’s Conditional Access policy. However, when the user (possibly through Terraform) requests an access token for MS Graph (step-up refreshing) with the token refreshing request will fail. That’s exactly why the error message is introduced in this PR to instruct the user to re-authenticate interactively. Best practice for using Azure CLI non-interactivelyThe best practice for using Azure CLI (or through Terraform) without any interaction after Then the step-down refreshing (MS Graph -> ARM) is silent. 2 possible scenariosHowever, there are 2 scenarios we have considered but haven’t solved yet (luckily, haven’t encountered either):
Providing 2 scopes at the same time? Unfortunately, this doesn't work because only one static resource can be provided: Previously we had some discussion on this topic with MSAL team @rayluo. This can fundamentally affect how MSAL’s token refreshing mechanism works, as this would require MSAL to bind refresh tokens to different scopes and tenants as well. |
Resolve #15220
Require #17778 which resolves #17551
Context
When ARM doesn't require MFA, but data-plane service does, CLI fails and can't recover.
Change
When CLI fails because ARM's Refresh Token doesn't have permission to get data-plane service's Access Token (step-up), CLI gives an instruction to re-login with
--scope:Additional context
This is a non-automated solution to #17093, because we don't want CLI to launch a browser automatically during script execution.