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.

Sign up for GitHub

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

Jump to bottom

[Core] Replace allow_broker with enable_broker_on_windows #27726

Merged
merged 1 commit into from
Feb 26, 2024
Merged

[Core] Replace allow_broker with enable_broker_on_windows #27726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/azure-cli-core/azure/cli/core/_profile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -856,13 +856,14 @@ def _create_identity_instance(cli_ctx, *args, **kwargs):
# EXPERIMENTAL: Use core.use_msal_http_cache=False to turn off MSAL HTTP cache.
use_msal_http_cache = cli_ctx.config.getboolean('core', 'use_msal_http_cache', fallback=True)

# PREVIEW: On Windows, use core.allow_broker=true to use broker (WAM) for authentication.
allow_broker = cli_ctx.config.getboolean('core', 'allow_broker', fallback=False)
# PREVIEW: On Windows, use core.enable_broker_on_windows=true to use broker (WAM) for authentication.
enable_broker_on_windows = cli_ctx.config.getboolean('core', 'enable_broker_on_windows', fallback=False)
from .telemetry import set_broker_info
set_broker_info(allow_broker=allow_broker)
set_broker_info(enable_broker_on_windows)

# PREVIEW: In Azure Stack environment, use core.instance_discovery=false to disable MSAL's instance discovery.
instance_discovery = cli_ctx.config.getboolean('core', 'instance_discovery', True)

return Identity(*args, encrypt=encrypt, use_msal_http_cache=use_msal_http_cache, allow_broker=allow_broker,
return Identity(*args, encrypt=encrypt, use_msal_http_cache=use_msal_http_cache,
enable_broker_on_windows=enable_broker_on_windows,
instance_discovery=instance_discovery, **kwargs)
8 changes: 4 additions & 4 deletions src/azure-cli-core/azure/cli/core/auth/identity.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class Identity: # pylint: disable=too-many-instance-attributes
_service_principal_store_instance = None

def __init__(self, authority, tenant_id=None, client_id=None, encrypt=False, use_msal_http_cache=True,
allow_broker=None, instance_discovery=None):
enable_broker_on_windows=None, instance_discovery=None):
"""
:param authority: Authentication authority endpoint. For example,
- AAD: https://login.microsoftonline.com
Expand All @@ -69,7 +69,7 @@ def __init__(self, authority, tenant_id=None, client_id=None, encrypt=False, use
self.client_id = client_id or AZURE_CLI_CLIENT_ID
self._encrypt = encrypt
self._use_msal_http_cache = use_msal_http_cache
self._allow_broker = allow_broker
self._enable_broker_on_windows = enable_broker_on_windows
self._instance_discovery = instance_discovery

# Build the authority in MSAL style
Expand Down Expand Up @@ -107,8 +107,8 @@ def _msal_app_kwargs(self):
@property
def _msal_public_app_kwargs(self):
"""kwargs for creating PublicClientApplication."""
# allow_broker can only be used on PublicClientApplication.
return {**self._msal_app_kwargs, "allow_broker": self._allow_broker}
# enable_broker_on_windows can only be used on PublicClientApplication.
return {**self._msal_app_kwargs, "enable_broker_on_windows": self._enable_broker_on_windows}

@property
def _msal_app(self):
Expand Down
2 changes: 1 addition & 1 deletion src/azure-cli-core/azure/cli/core/auth/landing_pages/success.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ <h3>Announcements</h3>
<p>[Windows only] Azure CLI is collecting feedback on using the <a href="https://learn.microsoft.com/windows/uwp/security/web-account-manager">Web Account Manager</a> (WAM) broker for the login experience.</p>
<p>You may opt-in to use WAM by running the following commands:</p>
<code>
az config set core.allow_broker=true<br>
az config set core.enable_broker_on_windows=true<br>
az account clear<br>
az login
</code>
Expand Down
10 changes: 5 additions & 5 deletions src/azure-cli-core/azure/cli/core/telemetry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ def __init__(self, correlation_id=None, application=None):
self.suppress_new_event = False
self.poll_start_time = None
self.poll_end_time = None
self.allow_broker = None
self.enable_broker_on_windows = None
self.msal_telemetry = None
self.secrets_detected = None
self.user_agent = None
Expand Down Expand Up @@ -221,7 +221,7 @@ def _get_azure_cli_properties(self):
set_custom_properties(result, 'ShowSurveyMessage', str(self.show_survey_message))
set_custom_properties(result, 'RegionInput', self.region_input)
set_custom_properties(result, 'RegionIdentified', self.region_identified)
set_custom_properties(result, 'AllowBroker', str(self.allow_broker))
set_custom_properties(result, 'EnableBrokerOnWindows', str(self.enable_broker_on_windows))
Copy link
Member Author

Choose a reason for hiding this comment

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

Initially I forgot to rename self.allow_broker to self.enable_broker_on_windows. This results in the below error in debug log:

cli.azure.cli.core.decorators: Suppress exception:
Traceback (most recent call last):
  File "d:\cli\azure-cli\src\azure-cli\azure\cli\__main__.py", line 62, in <module>
    raise ex
  File "d:\cli\azure-cli\src\azure-cli\azure\cli\__main__.py", line 55, in <module>
    sys.exit(exit_code)
SystemExit: 0

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "d:\cli\azure-cli\src\azure-cli-core\azure\cli\core\decorators.py", line 79, in _wrapped_func
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "d:\cli\azure-cli\src\azure-cli-core\azure\cli\core\telemetry.py", line 110, in generate_payload
    cli = self._get_azure_cli_properties()
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "d:\cli\azure-cli\src\azure-cli-core\azure\cli\core\telemetry.py", line 224, in _get_azure_cli_properties
    set_custom_properties(result, 'AllowBroker', str(self.allow_broker))
                                                     ^^^^^^^^^^^^^^^^^
AttributeError: 'TelemetrySession' object has no attribute 'allow_broker'

telemetry.main: Split cli events and extra events failure: the JSON object must be str, bytes or bytearray, not NoneType

However, the CI doesn't find this. @evelyn-ys

Copy link
Member

Choose a reason for hiding this comment

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

good catch. We should add telemetry check in CI too

set_custom_properties(result, 'MsalTelemetry', self.msal_telemetry)
set_custom_properties(result, 'SecretsWarning', _get_secrets_warning_config())
set_custom_properties(result, 'SecretsDetected', self.secrets_detected)
Expand Down Expand Up @@ -462,9 +462,9 @@ def set_region_identified(region_input, region_identified):


@decorators.suppress_all_exceptions()
def set_broker_info(allow_broker):
# whether customer has configured `allow_broker` to enable WAM(Web Account Manager) login for authentication
_session.allow_broker = allow_broker
def set_broker_info(enable_broker_on_windows):
# Log the value of `enable_broker_on_windows`
_session.enable_broker_on_windows = enable_broker_on_windows


@decorators.suppress_all_exceptions()
Expand Down