Conversation
️✔️AzureCLI-FullTest
|
|
Hi @jiasli, |
️✔️AzureCLI-BreakingChangeTest
|
|
Login experience v2 |
jiasli
modified the milestones:
June 2024 (2024-07-02),
May 2024 (2024-05-21)❗ Breaking change release ❗
| except NoTTYException: | ||
| # This is a good example showing interactive and non-TTY are not contradictory | ||
| logger.warning("No TTY to select the default subscription.") | ||
| return active_one |
There was a problem hiding this comment.
If no TTY is available, should we:
- print the subscription table
- echo the selection
- simply skip the whole subscription selection process
There was a problem hiding this comment.
We probably want to skip the selection process in the case there is no TTY available.
There was a problem hiding this comment.
I agree, but should we still show the interactive interface, or switch to the previous behavior of returning a JSON?
There was a problem hiding this comment.
If stdin is not a TTY, the user may be running echo 1 | az login. This is unlikely as the user can't know beforehand which subscription will be the one they want to select, and the number can change.
If stdout is not a TTY, the user may be running az login | grep. Not showing the JSON output will be a breaking change in this case.
Therefore, we decide to show interactive subscription selection only when both stdin and stdout are TTY.
| logger = get_logger(__name__) | ||
|
|
||
|
|
||
| class SubscriptionSelector: |
There was a problem hiding this comment.
Some refactors are made in commit 5884ecb:
- Part of
Profile._interactively_select_subscriptionis split into_format_subscription_tableso that it can be unit-tested. Profile._interactively_select_subscriptionand_format_subscription_tableare extracted into aSubscriptionSelectorclass.SubscriptionSelectoris moved toprofilemodule to reduce the change on core.
| from ._subscription_selector import SubscriptionSelector | ||
| from azure.cli.core._profile import _SUBSCRIPTION_ID | ||
|
|
||
| selected = SubscriptionSelector(subscriptions)() |
There was a problem hiding this comment.
subscriptions here only contain subscriptions accessible by this user, provided to az login. az login's original behavior is also to return subscriptions accessible by this user. Should we also show subscriptions from other users as candidates in interactive selection?
There was a problem hiding this comment.
If the user does not have access to the subscription, why should be show it?
There was a problem hiding this comment.
Even though the current user doesn't have access, another user used in a previous az login has. Running az account set will automatically switch to the other user.
| # create=True is required as running this test alone won't set format_styled_text.theme. patch.object | ||
| # will fail because of the missing theme attribute. For example, this command fails: | ||
| # azdev test test_format_subscription_table | ||
| with mock.patch.object(format_styled_text, 'theme', 'dark', create=True): |
There was a problem hiding this comment.
This mock can also be achieved by deleting the theme attribute, just like running test_format_subscription_table alone. Pytest's monkeypatch fixture has a monkeypatch.delattr(obj, name, raising=True) method for this purpose, but I can't find a way to do this with unittest.mock.
There was a problem hiding this comment.
We can write some custom logic to achieve this:
is_attr_set = False
original_attr_value = None
if hasattr(format_styled_text, 'theme'):
is_attr_set = True
original_attr_value = format_styled_text.theme
delattr(format_styled_text, 'theme')
...
if is_attr_set:
format_styled_text.theme = original_attr_valueor even wrap it as a context manager:
class delattr_patch:
def __init__(self, object, attr):
self.object = object
self.attr = attr
self.is_attr_set = False
self.original_attr_value = None
def __enter__(self):
if hasattr(self.object, self.attr):
self.is_attr_set = True
self.original_attr_value = getattr(self.object, self.attr)
delattr(self.object, self.attr)
def __exit__(self, exc_type, exc_value, traceback):
if self.is_attr_set:
setattr(self.object, self.attr, self.original_attr_value)Then we can use it as
with delattr_patch(format_styled_text, 'theme'):
...
| subscription_name = subscription_name[:subscription_name_length_limit - 3] + '...' | ||
|
|
||
| row = { | ||
| 'No': f'[{index_str}]' + (' ' + self.DEFAULT_ROW_MARKER if is_default else ''), |
There was a problem hiding this comment.
Why No is not highlighted?
There was a problem hiding this comment.
No is also highlighted in the latest code.
| password = ServicePrincipalAuth.build_credential(password, client_assertion, use_cert_sn_issuer) | ||
|
|
||
| select_subscription = interactive and sys.stdin.isatty() and sys.stdout.isatty() and \ | ||
| cmd.cli_ctx.config.getboolean('core', 'login_experience_v2', fallback=True) |
There was a problem hiding this comment.
This is long logic, how about creating a function?
If service_principal is not None, the select_subscription is also enabled? This conflicts with PR description.
There was a problem hiding this comment.
This is long logic, how about creating a function?
True, but not that complex as it is still only one line.
If
service_principalis not None, theselect_subscriptionis also enabled? This conflicts with PR description.
service_principal must be used together with username:
azure-cli/src/azure-cli/azure/cli/command_modules/profile/custom.py
Lines 127 to 128 in 3b20c9a
interactive is True only when username is not provided:
azure-cli/src/azure-cli/azure/cli/command_modules/profile/custom.py
Lines 141 to 148 in 3b20c9a
In other words, when username is provided, interactive must be False.
There was a problem hiding this comment.
How about az login --identity?
There was a problem hiding this comment.
It is guarded by
azure-cli/src/azure-cli/azure/cli/command_modules/profile/custom.py
Lines 131 to 136 in 4f15180
| login_experience_v2 = cmd.cli_ctx.config.getboolean('core', 'login_experience_v2', fallback=True) | ||
| # Send login_experience_v2 config to telemetry | ||
| from azure.cli.core.telemetry import set_login_experience_v2 | ||
| set_login_experience_v2(login_experience_v2) | ||
|
|
||
| select_subscription = interactive and sys.stdin.isatty() and sys.stdout.isatty() and login_experience_v2 |
There was a problem hiding this comment.
Should we also send select_subscription to telemetry? login_experience_v2 stands for the config value, while select_subscription stands for whether interactive selection is actually invoked. @dcaro
There was a problem hiding this comment.
We'll consider adding telemetry for select_subscription in the future.
| # authentication-related | ||
| self.enable_broker_on_windows = None | ||
| self.msal_telemetry = None | ||
| self.login_experience_v2 = None |
There was a problem hiding this comment.
Grouping these properties together for better readability.
| # authentication-related | ||
| set_custom_properties(result, 'EnableBrokerOnWindows', str(self.enable_broker_on_windows)) | ||
| set_custom_properties(result, 'MsalTelemetry', self.msal_telemetry) | ||
| set_custom_properties(result, 'LoginExperienceV2', str(self.login_experience_v2)) |
There was a problem hiding this comment.
Should we force str values? self.enable_broker_on_windows is also a bool and we convert it to str (#24304). Can we use bool as it is? @evelyn-ys
There was a problem hiding this comment.
bool may not appear correctly in the final telemetry. Using str is more robust.
|
After #26854, there is no easy way to verify the telemetry record sent. I have to add a breakpoint at and check |
jiasli
requested review from
calvinhzy,
evelyn-ys,
jsntcy,
kairu-ms and
zhoxing-ms
as code owners
|
Related Azure PowerShell PRs: |
5 participants
Related command
az loginDescription
Interactive commands now prompt the user to interactively select the default subscription and tenant:
az login: Auth code flow/WAMaz login --use-device-code: Device code flowAutomation commands are not affected:
az login --username --password: Username password flowaz login --service-principal: Service principal authenticationaz login --identity: managed identity authenticationTesting Guide
az loginaz login --use-device-codeTurn off:
Additional Information
Require #28660
History Notes
[Profile]
az login: Introduce login experience v2. For more details, see https://go.microsoft.com/fwlink/?linkid=2271236[Core] Add
tenantDefaultDomainandtenantDisplayNameproperties to login contexts (shown byaz account list)