From e6114791b42c3ffb149a255c41d435fbba4fb88b Mon Sep 17 00:00:00 2001 From: Ray Luo Date: Sun, 28 Aug 2022 23:56:51 -0700 Subject: [PATCH 01/10] Broker (WAM) integration Disabled SSH Cert when using broker --- msal/application.py | 283 +++++++++++++++++++++++++++++++++-- msal/authority.py | 5 +- msal/broker.py | 239 +++++++++++++++++++++++++++++ msal/token_cache.py | 20 ++- sample/interactive_sample.py | 6 + setup.py | 10 +- tests/msaltest.py | 15 +- tests/test_broker.py | 63 ++++++++ tests/test_e2e.py | 36 ++++- 9 files changed, 642 insertions(+), 35 deletions(-) create mode 100644 msal/broker.py create mode 100644 tests/test_broker.py diff --git a/msal/application.py b/msal/application.py index 62fc6cb..a16e184 100644 --- a/msal/application.py +++ b/msal/application.py @@ -17,7 +17,7 @@ from .mex import send_request as mex_send_request from .wstrust_request import send_request as wst_send_request from .wstrust_response import * -from .token_cache import TokenCache +from .token_cache import TokenCache, _get_username import msal.telemetry from .region import _detect_region from .throttled_http_client import ThrottledHttpClient @@ -25,7 +25,7 @@ # The __init__.py will import this. Not the other way around. -__version__ = "1.18.0" # When releasing, also check and bump our dependencies's versions if needed +__version__ = "1.20.0b1" # When releasing, also check and bump our dependencies's versions if needed logger = logging.getLogger(__name__) _AUTHORITY_TYPE_CLOUDSHELL = "CLOUDSHELL" @@ -67,8 +67,12 @@ def _str2bytes(raw): def _clean_up(result): if isinstance(result, dict): - result.pop("refresh_in", None) # MSAL handled refresh_in, customers need not - return result + return { + k: result[k] for k in result + if k != "refresh_in" # MSAL handled refresh_in, customers need not + and not k.startswith('_') # Skim internal properties + } + return result # It could be None def _preferred_browser(): @@ -174,6 +178,7 @@ def __init__( # when we would eventually want to add this feature to PCA in future. exclude_scopes=None, http_cache=None, + allow_broker=None, ): """Create an instance of application. @@ -409,6 +414,34 @@ def __init__( Personally Identifiable Information (PII). Encryption is unnecessary. New in version 1.16.0. + + :param boolean allow_broker: + Brokers provide Single-Sign-On, device identification, + and application identification verification. + If this parameter is set to True, + MSAL will use the broker and return either a token or an error, + when your scenario is supported by a broker, + otherwise it will automatically fall back to non-broker behavior. + This also means you could set this flag as True universally, + as long as your app meets the following prerequisite: + + * Installed optional dependency, e.g. ``pip install msal[broker]>=1.20,<2``. + (Note that broker is currently only available on Windows 10+) + + * Register a new redirect_uri for your desktop app as: + ``ms-appx-web://Microsoft.AAD.BrokerPlugin/your_client_id`` + + * Tested your app in following scenarios: + + * Windows 10+ + + * PublicClientApplication's following methods:: + acquire_token_interactive(), acquire_token_by_username_password(), + acquire_token_silent() (or acquire_token_silent_with_error()). + + * AAD and MSA accounts (i.e. Non-ADFS, non-B2C) + + New in version 1.20.0. """ self.client_id = client_id self.client_credential = client_credential @@ -467,6 +500,15 @@ def __init__( self.http_client, validate_authority=False) else: raise + is_confidential_app = bool( + isinstance(self, ConfidentialClientApplication) or self.client_credential) + if is_confidential_app and allow_broker: + raise ValueError("allow_broker=True is only supported in PublicClientApplication") + self._enable_broker = bool( + allow_broker and not is_confidential_app + and sys.platform == "win32" + and not self.authority.is_adfs and not self.authority._is_b2c) + logger.debug("Broker enabled? %s", self._enable_broker) self.token_cache = token_cache or TokenCache() self._region_configured = azure_region @@ -1028,6 +1070,15 @@ def _get_authority_aliases(self, instance): def remove_account(self, account): """Sign me out and forget me from token cache""" self._forget_me(account) + if self._enable_broker: + try: + from .broker import _signout_silently + except RuntimeError: # TODO: TBD + logger.debug("Broker is unavailable on this platform. Fallback to non-broker.") + else: + error = _signout_silently(self.client_id, account["local_account_id"]) + if error: + logger.debug("_signout_silently() returns error: %s", error) def _sign_out(self, home_account): # Remove all relevant RTs and ATs from token cache @@ -1255,9 +1306,28 @@ def _acquire_token_silent_from_cache_and_possibly_refresh_it( refresh_reason = msal.telemetry.FORCE_REFRESH # TODO: It could also mean claims_challenge assert refresh_reason, "It should have been established at this point" try: + data = kwargs.get("data", {}) if account and account.get("authority_type") == _AUTHORITY_TYPE_CLOUDSHELL: - return self._acquire_token_by_cloud_shell( - scopes, data=kwargs.get("data")) + return self._acquire_token_by_cloud_shell(scopes, data=data) + + if self._enable_broker and account is not None and data.get("token_type") != "ssh-cert": + try: + from .broker import _acquire_token_silently + except RuntimeError: # TODO: TBD + logger.debug("Broker is unavailable on this platform. Fallback to non-broker.") + else: + response = _acquire_token_silently( + "https://{}/{}".format(self.authority.instance, self.authority.tenant), + self.client_id, + account["local_account_id"], + scopes, + claims=_merge_claims_challenge_and_capabilities( + self._client_capabilities, claims_challenge), + correlation_id=correlation_id, + **data) + if response: # The broker provided a decisive outcome, so we use it + return self._process_broker_response(response, scopes, data) + result = _clean_up(self._acquire_token_silent_by_finding_rt_belongs_to_me_or_my_family( authority, self._decorate_scope(scopes), account, refresh_reason=refresh_reason, claims_challenge=claims_challenge, @@ -1271,6 +1341,18 @@ def _acquire_token_silent_from_cache_and_possibly_refresh_it( raise # We choose to bubble up the exception return access_token_from_cache + def _process_broker_response(self, response, scopes, data): + if "error" not in response: + self.token_cache.add(dict( + client_id=self.client_id, + scope=response["scope"].split() if "scope" in response else scopes, + token_endpoint=self.authority.token_endpoint, + response=response.copy(), + data=data, + _account_id=response["_account_id"], + )) + return _clean_up(response) + def _acquire_token_silent_by_finding_rt_belongs_to_me_or_my_family( self, authority, scopes, account, **kwargs): query = { @@ -1447,14 +1529,33 @@ def acquire_token_by_username_password( - A successful response would contain "access_token" key, - an error response would contain "error" and usually "error_description". """ + claims = _merge_claims_challenge_and_capabilities( + self._client_capabilities, claims_challenge) + if self._enable_broker: + try: + from .broker import _signin_silently + except RuntimeError: # TODO: TBD + logger.debug("Broker is unavailable on this platform. Fallback to non-broker.") + else: + response = _signin_silently( + "https://{}/{}".format(self.authority.instance, self.authority.tenant), + self.client_id, + scopes, # Decorated scopes won't work due to offline_access + MSALRuntime_Username=username, + MSALRuntime_Password=password, + validateAuthority="no" + if self.authority._validate_authority is False + or self.authority.is_adfs or self.authority._is_b2c + else None, + claims=claims, + ) + return self._process_broker_response(response, scopes, kwargs.get("data", {})) + scopes = self._decorate_scope(scopes) telemetry_context = self._build_telemetry_context( self.ACQUIRE_TOKEN_BY_USERNAME_PASSWORD_ID) headers = telemetry_context.generate_headers() - data = dict( - kwargs.pop("data", {}), - claims=_merge_claims_challenge_and_capabilities( - self._client_capabilities, claims_challenge)) + data = dict(kwargs.pop("data", {}), claims=claims) if not self.authority.is_adfs: user_realm_result = self.authority.user_realm_discovery( username, correlation_id=headers[msal.telemetry.CLIENT_REQUEST_ID]) @@ -1520,6 +1621,7 @@ def _acquire_token_by_username_password_federated( class PublicClientApplication(ClientApplication): # browser app or mobile app DEVICE_FLOW_CORRELATION_ID = "_correlation_id" + CONSOLE_WINDOW_HANDLE = object() def __init__(self, client_id, client_credential=None, **kwargs): if client_credential is not None: @@ -1538,11 +1640,16 @@ def acquire_token_interactive( port=None, extra_scopes_to_consent=None, max_age=None, + parent_window_handle=None, + on_before_launching_ui=None, **kwargs): """Acquire token interactively i.e. via a local browser. Prerequisite: In Azure Portal, configure the Redirect URI of your "Mobile and Desktop application" as ``http://localhost``. + If you opts in to use broker during ``PublicClientApplication`` creation, + your app also need this Redirect URI: + ``ms-appx-web://Microsoft.AAD.BrokerPlugin/YOUR_CLIENT_ID`` :param list scopes: It is a list of case-sensitive strings. @@ -1594,17 +1701,79 @@ def acquire_token_interactive( New in version 1.15. + :param int parent_window_handle: + OPTIONAL. If your app is a GUI app running on modern Windows system, + and your app opts in to use broker, + you are recommended to also provide its window handle, + so that the sign in UI window will properly pop up on top of your window. + + New in version 1.20.0. + + :param function on_before_launching_ui: + A callback with the form of + ``lambda ui="xyz", **kwargs: print("A {} will be launched".format(ui))``, + where ``ui`` will be either "browser" or "broker". + You can use it to inform your end user to expect a pop-up window. + + New in version 1.20.0. + :return: - A dict containing no "error" key, and typically contains an "access_token" key. - A dict containing an "error" key, when token refresh failed. """ - self._validate_ssh_cert_input_data(kwargs.get("data", {})) + data = kwargs.pop("data", {}) + self._validate_ssh_cert_input_data(data) + if not on_before_launching_ui: + on_before_launching_ui = lambda **kwargs: None if _is_running_in_cloud_shell() and prompt == "none": - return self._acquire_token_by_cloud_shell( - scopes, data=kwargs.pop("data", {})) + # Note: _acquire_token_by_cloud_shell() is always silent, + # so we would not fire on_before_launching_ui() + return self._acquire_token_by_cloud_shell(scopes, data=data) claims = _merge_claims_challenge_and_capabilities( self._client_capabilities, claims_challenge) + if self._enable_broker and data.get("token_type") != "ssh-cert": + if parent_window_handle is None: + raise ValueError( + "parent_window_handle is required when you opted into using broker. " + "You need to provide the window handle of your GUI application, " + "or use msal.PublicClientApplication.CONSOLE_WINDOW_HANDLE " + "when and only when your application is a console app.") + if extra_scopes_to_consent: + logger.warning( + "Ignoring parameter extra_scopes_to_consent, " + "which is not supported by broker") + enable_msa_passthrough = kwargs.pop( + "enable_msa_passthrough", # Keep it as a hidden param, for now. + # OPTIONAL. MSA-Passthrough is a legacy configuration, + # needed by a small amount of Microsoft first-party apps, + # which would login MSA accounts via ".../organizations" authority. + # If you app belongs to this category, AND you are enabling broker, + # you would want to enable this flag. Default value is equivalent to False. + self.client_id in [ + # Experimental: Automatically enable MSA-PT mode for known MSA-PT apps + # More background of MSA-PT is available from this internal docs: + # https://microsoft.sharepoint.com/:w:/t/Identity-DevEx/EatIUauX3c9Ctw1l7AQ6iM8B5CeBZxc58eoQCE0IuZ0VFw?e=tgc3jP&CID=39c853be-76ea-79d7-ee73-f1b2706ede05 + "04b07795-8ddb-461a-bbee-02f9e1bf7b46", # Azure CLI + "04f0c124-f2bc-4f59-8241-bf6df9866bbd", # Visual Studio + ] and data.get("token_type") != "ssh-cert" # Work around a known issue as of PyMsalRuntime 0.8 + ) + try: + return self._acquire_token_interactive_via_broker( + scopes, + parent_window_handle, + enable_msa_passthrough, + claims, + data, + on_before_launching_ui, + prompt=prompt, + login_hint=login_hint, + max_age=max_age, + ) + except RuntimeError: # TODO: TBD + logger.debug("Broker is unavailable on this platform. Fallback to non-broker.") + + on_before_launching_ui(ui="browser") telemetry_context = self._build_telemetry_context( self.ACQUIRE_TOKEN_INTERACTIVE) response = _clean_up(self.client.obtain_token_by_browser( @@ -1621,13 +1790,99 @@ def acquire_token_interactive( "claims": claims, "domain_hint": domain_hint, }, - data=dict(kwargs.pop("data", {}), claims=claims), + data=dict(data, claims=claims), headers=telemetry_context.generate_headers(), browser_name=_preferred_browser(), **kwargs)) telemetry_context.update_telemetry(response) return response + def _acquire_token_interactive_via_broker( + self, + scopes, # type: list[str] + parent_window_handle, # type: int + enable_msa_passthrough, # type: boolean + claims, # type: str + data, # type: dict + on_before_launching_ui, # type: callable + prompt=None, + login_hint=None, # type: Optional[str] + max_age=None, + **kwargs): + from .broker import _signin_interactively, _signin_silently, _acquire_token_silently + if "welcome_template" in kwargs: + logger.debug(kwargs["welcome_template"]) # Experimental + authority = "https://{}/{}".format( + self.authority.instance, self.authority.tenant) + validate_authority = ( + "no" if self.authority._validate_authority is False + or self.authority.is_adfs or self.authority._is_b2c + else None) + # Calls different broker methods to mimic the OIDC behaviors + if login_hint and prompt != "select_account": # OIDC prompts when the user did not sign in + accounts = self.get_accounts(username=login_hint) + if len(accounts) == 1: # Unambiguously proceed with this account + response = _acquire_token_silently( # When it works, it bypasses prompt + authority, + self.client_id, + accounts[0]["local_account_id"], + scopes, + claims=claims, + **data) + if response and "error" not in response: + return self._process_broker_response(response, scopes, data) + # login_hint undecisive or not exists + if prompt == "none" or not prompt: # Must/Can attempt _signin_silently() + response = _signin_silently( # Unlike OIDC, it doesn't honor login_hint + authority, self.client_id, scopes, + validateAuthority=validate_authority, + claims=claims, + max_age=max_age, + enable_msa_pt=enable_msa_passthrough, + **data) + is_wrong_account = bool( + # _signin_silently() only gets tokens for default account, + # but this seems to have been fixed in PyMsalRuntime 0.11.2 + "access_token" in response and login_hint + and response.get("id_token_claims", {}) != login_hint) + wrong_account_error_message = ( + 'prompt="none" will not work for login_hint="non-default-user"') + if is_wrong_account: + logger.debug(wrong_account_error_message) + if prompt == "none": + return self._process_broker_response( # It is either token or error + response, scopes, data + ) if not is_wrong_account else { + "error": "broker_error", + "error_description": wrong_account_error_message, + } + else: + assert bool(prompt) is False + from pymsalruntime import Response_Status + recoverable_errors = frozenset([ + Response_Status.Status_AccountUnusable, + Response_Status.Status_InteractionRequired, + ]) + if is_wrong_account or "error" in response and response.get( + "_broker_status") in recoverable_errors: + pass # It will fall back to the _signin_interactively() + else: + return self._process_broker_response(response, scopes, data) + # Falls back to _signin_interactively() + on_before_launching_ui(ui="broker") + response = _signin_interactively( + authority, self.client_id, scopes, + None if parent_window_handle is self.CONSOLE_WINDOW_HANDLE + else parent_window_handle, + validateAuthority=validate_authority, + login_hint=login_hint, + prompt=prompt, + claims=claims, + max_age=max_age, + enable_msa_pt=enable_msa_passthrough, + **data) + return self._process_broker_response(response, scopes, data) + def initiate_device_flow(self, scopes=None, **kwargs): """Initiate a Device Flow instance, which will be used in :func:`~acquire_token_by_device_flow`. diff --git a/msal/authority.py b/msal/authority.py index 8178820..93aafee 100644 --- a/msal/authority.py +++ b/msal/authority.py @@ -72,9 +72,10 @@ def __init__(self, authority_url, http_client, validate_authority=True): authority_url = str(authority_url) authority, self.instance, tenant = canonicalize(authority_url) parts = authority.path.split('/') - is_b2c = any(self.instance.endswith("." + d) for d in WELL_KNOWN_B2C_HOSTS) or ( + self._is_b2c = any(self.instance.endswith("." + d) for d in WELL_KNOWN_B2C_HOSTS) or ( len(parts) == 3 and parts[2].lower().startswith("b2c_")) - if (tenant != "adfs" and (not is_b2c) and validate_authority + self._validate_authority = True if validate_authority is None else bool(validate_authority) + if (tenant != "adfs" and (not self._is_b2c) and self._validate_authority and self.instance not in WELL_KNOWN_AUTHORITY_HOSTS): payload = instance_discovery( "https://{}{}/oauth2/v2.0/authorize".format( diff --git a/msal/broker.py b/msal/broker.py new file mode 100644 index 0000000..e9bbe28 --- /dev/null +++ b/msal/broker.py @@ -0,0 +1,239 @@ +"""This module is an adaptor to the underlying broker. +It relies on PyMsalRuntime which is the package providing broker's functionality. +""" +from threading import Event +import json +import logging +import time +import uuid + + +logger = logging.getLogger(__name__) +try: + import pymsalruntime # ImportError would be raised on unsupported platforms such as Windows 8 + # Its API description is available in site-packages/pymsalruntime/PyMsalRuntime.pyi + pymsalruntime.register_logging_callback(lambda message, level: { # New in pymsalruntime 0.7 + pymsalruntime.LogLevel.TRACE: logger.debug, # Python has no TRACE level + pymsalruntime.LogLevel.DEBUG: logger.debug, + # Let broker's excess info, warning and error logs map into default DEBUG, for now + #pymsalruntime.LogLevel.INFO: logger.info, + #pymsalruntime.LogLevel.WARNING: logger.warning, + #pymsalruntime.LogLevel.ERROR: logger.error, + pymsalruntime.LogLevel.FATAL: logger.critical, + }.get(level, logger.debug)(message)) +except (ImportError, AttributeError): # AttributeError happens when a prior pymsalruntime uninstallation somehow leaved an empty folder behind + # PyMsalRuntime currently supports these Windows versions, listed in this MSFT internal link + # https://github.com/AzureAD/microsoft-authentication-library-for-cpp/pull/2406/files + raise ImportError( # TODO: Remove or adjust this line right before merging this PR + 'You need to install dependency by: pip install "msal[broker]>=1.20.0b1,<2"') +# Other exceptions (possibly RuntimeError) would be raised if its initialization fails + + +class RedirectUriError(ValueError): + pass + + +class TokenTypeError(ValueError): + pass + + +class _CallbackData: + def __init__(self): + self.signal = Event() + self.result = None + + def complete(self, result): + self.signal.set() + self.result = result + + +def _convert_error(error, client_id): + context = error.get_context() # Available since pymsalruntime 0.0.4 + if ( + "AADSTS50011" in context # In WAM, this could happen on both interactive and silent flows + or "AADSTS7000218" in context # This "request body must contain ... client_secret" is just a symptom of current app has no WAM redirect_uri + ): + raise RedirectUriError( # This would be seen by either the app developer or end user + "MsalRuntime won't work unless this one more redirect_uri is registered to current app: " + "ms-appx-web://Microsoft.AAD.BrokerPlugin/{}".format(client_id)) + # OTOH, AAD would emit other errors when other error handling branch was hit first, + # so, the AADSTS50011/RedirectUriError is not guaranteed to happen. + return { + "error": "broker_error", # Note: Broker implies your device needs to be compliant. + # You may use "dsregcmd /status" to check your device state + # https://docs.microsoft.com/en-us/azure/active-directory/devices/troubleshoot-device-dsregcmd + "error_description": "{}. Status: {}, Error code: {}, Tag: {}".format( + context, + error.get_status(), error.get_error_code(), error.get_tag()), + "_broker_status": error.get_status(), + "_broker_error_code": error.get_error_code(), + "_broker_tag": error.get_tag(), + } + + +def _read_account_by_id(account_id, correlation_id): + """Return an instance of MSALRuntimeError or MSALRuntimeAccount, or None""" + callback_data = _CallbackData() + pymsalruntime.read_account_by_id( + account_id, + correlation_id, + lambda result, callback_data=callback_data: callback_data.complete(result) + ) + callback_data.signal.wait() + return (callback_data.result.get_error() or callback_data.result.get_account() + or None) # None happens when the account was not created by broker + + +def _convert_result(result, client_id, expected_token_type=None): # Mimic an on-the-wire response from AAD + error = result.get_error() + if error: + return _convert_error(error, client_id) + id_token_claims = json.loads(result.get_id_token()) if result.get_id_token() else {} + account = result.get_account() + assert account, "Account is expected to be always available" + ## Note: As of pymsalruntime 0.1.0, only wam_account_ids property is available + #account.get_account_property("wam_account_ids") + return_value = {k: v for k, v in { + "access_token": result.get_access_token(), + "expires_in": result.get_access_token_expiry_time() - int(time.time()), # Convert epoch to count-down + "id_token": result.get_raw_id_token(), # New in pymsalruntime 0.8.1 + "id_token_claims": id_token_claims, + "client_info": account.get_client_info(), + "_account_id": account.get_account_id(), + "token_type": expected_token_type or "Bearer", # Workaround its absence from broker + }.items() if v} + likely_a_cert = return_value["access_token"].startswith("AAAA") # Empirical observation + if return_value["token_type"].lower() == "ssh-cert" and not likely_a_cert: + raise TokenTypeError("Broker could not get an SSH Cert: {}...".format( + return_value["access_token"][:8])) + granted_scopes = result.get_granted_scopes() # New in pymsalruntime 0.3.x + if granted_scopes: + return_value["scope"] = " ".join(granted_scopes) # Mimic the on-the-wire data format + return return_value + + +def _get_new_correlation_id(): + return str(uuid.uuid4()) + + +def _enable_msa_pt(params): + params.set_additional_parameter("msal_request_type", "consumer_passthrough") # PyMsalRuntime 0.8+ + + +def _signin_silently( + authority, client_id, scopes, correlation_id=None, claims=None, + enable_msa_pt=False, + **kwargs): + params = pymsalruntime.MSALRuntimeAuthParameters(client_id, authority) + params.set_requested_scopes(scopes) + if claims: + params.set_decoded_claims(claims) + callback_data = _CallbackData() + for k, v in kwargs.items(): # This can be used to support domain_hint, max_age, etc. + if v is not None: + params.set_additional_parameter(k, str(v)) + if enable_msa_pt: + _enable_msa_pt(params) + pymsalruntime.signin_silently( + params, + correlation_id or _get_new_correlation_id(), + lambda result, callback_data=callback_data: callback_data.complete(result)) + callback_data.signal.wait() + return _convert_result( + callback_data.result, client_id, expected_token_type=kwargs.get("token_type")) + + +def _signin_interactively( + authority, client_id, scopes, + parent_window_handle, # None means auto-detect for console apps + prompt=None, + login_hint=None, + claims=None, + correlation_id=None, + enable_msa_pt=False, + **kwargs): + params = pymsalruntime.MSALRuntimeAuthParameters(client_id, authority) + params.set_requested_scopes(scopes) + params.set_redirect_uri("placeholder") # pymsalruntime 0.1 requires non-empty str, + # the actual redirect_uri will be overridden by a value hardcoded by the broker + if prompt: + if prompt == "select_account": + if login_hint: + # FWIW, AAD's browser interactive flow would honor select_account + # and ignore login_hint in such a case. + # But pymsalruntime 0.3.x would pop up a meaningless account picker + # and then force the account_hint user to re-input password. Not what we want. + # https://identitydivision.visualstudio.com/Engineering/_workitems/edit/1744492 + login_hint = None # Mimicing the AAD behavior + logger.warning("Using both select_account and login_hint is ambiguous. Ignoring login_hint.") + else: + logger.warning("prompt=%s is not supported by this module", prompt) + if parent_window_handle is None: + # This fixes account picker hanging in IDE debug mode on some machines + params.set_additional_parameter("msal_gui_thread", "true") # Since pymsalruntime 0.8.1 + if enable_msa_pt: + _enable_msa_pt(params) + for k, v in kwargs.items(): # This can be used to support domain_hint, max_age, etc. + if v is not None: + params.set_additional_parameter(k, str(v)) + if claims: + params.set_decoded_claims(claims) + callback_data = _CallbackData() + pymsalruntime.signin_interactively( + parent_window_handle or pymsalruntime.get_console_window() or pymsalruntime.get_desktop_window(), # Since pymsalruntime 0.2+ + params, + correlation_id or _get_new_correlation_id(), + login_hint, # None value will be accepted since pymsalruntime 0.3+ + lambda result, callback_data=callback_data: callback_data.complete(result)) + callback_data.signal.wait() + return _convert_result( + callback_data.result, client_id, expected_token_type=kwargs.get("token_type")) + + +def _acquire_token_silently( + authority, client_id, account_id, scopes, claims=None, correlation_id=None, + **kwargs): + # For MSA PT scenario where you use the /organizations, yes, + # acquireTokenSilently is expected to fail. - Sam Wilson + correlation_id = correlation_id or _get_new_correlation_id() + account = _read_account_by_id(account_id, correlation_id) + if isinstance(account, pymsalruntime.MSALRuntimeError): + return _convert_error(account, client_id) + if account is None: + return + params = pymsalruntime.MSALRuntimeAuthParameters(client_id, authority) + params.set_requested_scopes(scopes) + if claims: + params.set_decoded_claims(claims) + for k, v in kwargs.items(): # This can be used to support domain_hint, max_age, etc. + if v is not None: + params.set_additional_parameter(k, str(v)) + callback_data = _CallbackData() + pymsalruntime.acquire_token_silently( + params, + correlation_id, + account, + lambda result, callback_data=callback_data: callback_data.complete(result)) + callback_data.signal.wait() + return _convert_result( + callback_data.result, client_id, expected_token_type=kwargs.get("token_type")) + + +def _signout_silently(client_id, account_id, correlation_id=None): + correlation_id = correlation_id or _get_new_correlation_id() + account = _read_account_by_id(account_id, correlation_id) + if isinstance(account, pymsalruntime.MSALRuntimeError): + return _convert_error(account, client_id) + if account is None: + return + callback_data = _CallbackData() + pymsalruntime.signout_silently( # New in PyMsalRuntime 0.7 + client_id, + correlation_id, + account, + lambda result, callback_data=callback_data: callback_data.complete(result)) + callback_data.signal.wait() + error = callback_data.result.get_error() + if error: + return _convert_error(error, client_id) + diff --git a/msal/token_cache.py b/msal/token_cache.py index f7d9f95..dc26e84 100644 --- a/msal/token_cache.py +++ b/msal/token_cache.py @@ -12,6 +12,10 @@ def is_subdict_of(small, big): return dict(big, **small) == big +def _get_username(id_token_claims): + return id_token_claims.get( + "preferred_username", # AAD + id_token_claims.get("upn")) # ADFS 2019 class TokenCache(object): """This is considered as a base class containing minimal cache behavior. @@ -149,10 +153,9 @@ def __add(self, event, now=None): access_token = response.get("access_token") refresh_token = response.get("refresh_token") id_token = response.get("id_token") - id_token_claims = ( - decode_id_token(id_token, client_id=event["client_id"]) - if id_token - else response.get("id_token_claims", {})) # Broker would provide id_token_claims + id_token_claims = response.get("id_token_claims") or ( # Prefer the claims from broker + # Only use decode_id_token() when necessary, it contains time-sensitive validation + decode_id_token(id_token, client_id=event["client_id"]) if id_token else {}) client_info, home_account_id = self.__parse_account(response, id_token_claims) target = ' '.join(event.get("scope") or []) # Per schema, we don't sort it @@ -190,10 +193,11 @@ def __add(self, event, now=None): "home_account_id": home_account_id, "environment": environment, "realm": realm, - "local_account_id": id_token_claims.get( - "oid", id_token_claims.get("sub")), - "username": id_token_claims.get("preferred_username") # AAD - or id_token_claims.get("upn") # ADFS 2019 + "local_account_id": event.get( + "_account_id", # Came from mid-tier code path. + # Emperically, it is the oid in AAD or cid in MSA. + id_token_claims.get("oid", id_token_claims.get("sub"))), + "username": _get_username(id_token_claims) or data.get("username") # Falls back to ROPC username or event.get("username") # Falls back to Federated ROPC username or "", # The schema does not like null diff --git a/sample/interactive_sample.py b/sample/interactive_sample.py index 530892e..98acd29 100644 --- a/sample/interactive_sample.py +++ b/sample/interactive_sample.py @@ -1,4 +1,7 @@ """ +Prerequisite is documented here: +https://msal-python.readthedocs.io/en/latest/#msal.PublicClientApplication.acquire_token_interactive + The configuration file would look like this: { @@ -30,6 +33,8 @@ # Create a preferably long-lived app instance which maintains a token cache. app = msal.PublicClientApplication( config["client_id"], authority=config["authority"], + #allow_broker=True, # If opted in, you will be guided to meet the prerequisites, when applicable + # See also: https://docs.microsoft.com/en-us/azure/active-directory/develop/scenario-desktop-acquire-token-wam#wam-value-proposition # token_cache=... # Default cache is in memory only. # You can learn how to use SerializableTokenCache from # https://msal-python.readthedocs.io/en/latest/#msal.SerializableTokenCache @@ -55,6 +60,7 @@ print("A local browser window will be open for you to sign in. CTRL+C to cancel.") result = app.acquire_token_interactive( # Only works if your app is registered with redirect_uri as http://localhost config["scope"], + #parent_window_handle=..., # If broker is enabled, you will be guided to provide a window handle login_hint=config.get("username"), # Optional. # If you know the username ahead of time, this parameter can pre-fill # the username (or email address) field of the sign-in page for the user, diff --git a/setup.py b/setup.py index 814627f..1384634 100644 --- a/setup.py +++ b/setup.py @@ -85,6 +85,14 @@ # https://cryptography.io/en/latest/api-stability/#deprecation "mock;python_version<'3.3'", - ] + ], + extras_require={ # It does not seem to work if being defined inside setup.cfg + "broker": [ + # The broker is defined as optional dependency, + # so that downstream apps can opt in. The opt-in is needed, partially because + # most existing MSAL Python apps do not have the redirect_uri needed by broker. + "pymsalruntime>=0.11.2,<0.12;python_version>='3.6' and platform_system=='Windows'", + ], + }, ) diff --git a/tests/msaltest.py b/tests/msaltest.py index c1ef1e7..5d33f2c 100644 --- a/tests/msaltest.py +++ b/tests/msaltest.py @@ -74,12 +74,14 @@ def _acquire_token_interactive(app, scopes, data=None): # login_hint is unnecessary when prompt=select_account, # but we still let tester input login_hint, just for testing purpose. [None] + [a["username"] for a in app.get_accounts()], - header="login_hint? (If you have multiple signed-in sessions in browser, and you specify a login_hint to match one of them, you will bypass the account picker.)", + header="login_hint? (If you have multiple signed-in sessions in browser/broker, and you specify a login_hint to match one of them, you will bypass the account picker.)", accept_nonempty_string=True, ) login_hint = raw_login_hint["username"] if isinstance(raw_login_hint, dict) else raw_login_hint result = app.acquire_token_interactive( - scopes, prompt=prompt, login_hint=login_hint, data=data or {}) + scopes, + parent_window_handle=app.CONSOLE_WINDOW_HANDLE, # This test app is a console app + prompt=prompt, login_hint=login_hint, data=data or {}) if login_hint and "id_token_claims" in result: signed_in_user = result.get("id_token_claims", {}).get("preferred_username") if signed_in_user != login_hint: @@ -127,9 +129,13 @@ def remove_account(app): app.remove_account(account) print('Account "{}" and/or its token(s) are signed out from MSAL Python'.format(account["username"])) -def exit(_): +def exit(app): """Exit""" - bug_link = "https://github.com/AzureAD/microsoft-authentication-library-for-python/issues/new/choose" + bug_link = ( + "https://identitydivision.visualstudio.com/Engineering/_queries/query/79b3a352-a775-406f-87cd-a487c382a8ed/" + if app._enable_broker else + "https://github.com/AzureAD/microsoft-authentication-library-for-python/issues/new/choose" + ) print("Bye. If you found a bug, please report it here: {}".format(bug_link)) sys.exit() @@ -155,6 +161,7 @@ def main(): header="Input authority (Note that MSA-PT apps would NOT use the /common authority)", accept_nonempty_string=True, ), + allow_broker=_input_boolean("Allow broker? (Azure CLI currently only supports @microsoft.com accounts when enabling broker)"), ) if _input_boolean("Enable MSAL Python's DEBUG log?"): logging.basicConfig(level=logging.DEBUG) diff --git a/tests/test_broker.py b/tests/test_broker.py new file mode 100644 index 0000000..bb7d928 --- /dev/null +++ b/tests/test_broker.py @@ -0,0 +1,63 @@ +from tests import unittest +import logging +import sys + +if not sys.platform.startswith("win"): + raise unittest.SkipTest("Currently, our broker supports Windows") +from msal.broker import ( # Import them after the platform check + _signin_silently, _signin_interactively, _acquire_token_silently, RedirectUriError, + _signout_silently, _read_account_by_id, + ) + + +logging.basicConfig(level=logging.DEBUG) + +class BrokerTestCase(unittest.TestCase): + """These are the unit tests for the thin broker.py layer. + + It currently hardcodes some test apps which might be changed/gone in the future. + The existing test_e2e.py is sophisticated to pull test configuration securely from lab. + """ + _client_id = "04f0c124-f2bc-4f59-8241-bf6df9866bbd" # Visual Studio + _authority = "https://login.microsoftonline.com/common" + _scopes = ["https://graph.microsoft.com/.default"] + + def test_signin_interactive_then_acquire_token_silent_then_signout(self): + result = _signin_interactively(self._authority, self._client_id, self._scopes, None) + self.assertIsNotNone(result.get("access_token"), result) + + account_id = result["_account_id"] + self.assertIsNotNone(_read_account_by_id(account_id, "correlation_id")) + result = _acquire_token_silently( + self._authority, self._client_id, account_id, self._scopes) + self.assertIsNotNone(result.get("access_token"), result) + + signout_error = _signout_silently(self._client_id, account_id) + self.assertIsNone(signout_error, msg=signout_error) + account = _read_account_by_id(account_id, "correlation_id") + self.assertIsNotNone(account, msg="pymsalruntime still has this account") + result = _acquire_token_silently( + self._authority, self._client_id, account_id, self._scopes) + self.assertIn("Status_AccountUnusable", result.get("error_description", "")) + + def test_unconfigured_app_should_raise_exception(self): + app_without_needed_redirect_uri = "289a413d-284b-4303-9c79-94380abe5d22" + with self.assertRaises(RedirectUriError): + _signin_interactively( + self._authority, app_without_needed_redirect_uri, self._scopes, None) + # Note: _acquire_token_silently() would raise same exception, + # we skip its test here due to the lack of a valid account_id + + def test_signin_interactively_and_select_account(self): + print("An account picker UI will pop up. See whether the auth result matches your account") + result = _signin_interactively( + self._authority, self._client_id, self._scopes, None, prompt="select_account") + self.assertIsNotNone(result.get("access_token"), result) + if "access_token" in result: + result["access_token"] = "********" + import pprint; pprint.pprint(result) + + def test_signin_silently(self): + result = _signin_silently(self._authority, self._client_id, self._scopes) + self.assertIsNotNone(result.get("access_token"), result) + diff --git a/tests/test_e2e.py b/tests/test_e2e.py index 9a971f4..57ef820 100644 --- a/tests/test_e2e.py +++ b/tests/test_e2e.py @@ -134,6 +134,31 @@ def assertCacheWorksForApp(self, result_from_wire, scope): result_from_wire['access_token'], result_from_cache['access_token'], "We should get a cached AT") + @classmethod + def _build_app(cls, + client_id, + client_credential=None, + authority="https://login.microsoftonline.com/common", + scopes=["https://graph.microsoft.com/.default"], # Microsoft Graph + http_client=None, + azure_region=None, + **kwargs): + try: + import pymsalruntime + broker_available = True + except ImportError: + broker_available = False + return (msal.ConfidentialClientApplication + if client_credential else msal.PublicClientApplication)( + client_id, + client_credential=client_credential, + authority=authority, + azure_region=azure_region, + http_client=http_client or MinimalHttpClient(), + allow_broker=broker_available # This way, we reuse same test cases, by run them with and without broker + and not client_credential, + ) + def _test_username_password(self, authority=None, client_id=None, username=None, password=None, scope=None, client_secret=None, # Since MSAL 1.11, confidential client has ROPC too @@ -141,9 +166,9 @@ def _test_username_password(self, http_client=None, **ignored): assert authority and client_id and username and password and scope - self.app = msal.ClientApplication( + self.app = self._build_app( client_id, authority=authority, - http_client=http_client or MinimalHttpClient(), + http_client=http_client, azure_region=azure_region, # Regional endpoint does not support ROPC. # Here we just use it to test a regional app won't break ROPC. client_credential=client_secret) @@ -158,8 +183,7 @@ def _test_username_password(self, def _test_device_flow( self, client_id=None, authority=None, scope=None, **ignored): assert client_id and authority and scope - self.app = msal.PublicClientApplication( - client_id, authority=authority, http_client=MinimalHttpClient()) + self.app = self._build_app(client_id, authority=authority) flow = self.app.initiate_device_flow(scopes=scope) assert "user_code" in flow, "DF does not seem to be provisioned: %s".format( json.dumps(flow, indent=4)) @@ -188,13 +212,13 @@ def _test_acquire_token_interactive( prompt=None, **ignored): assert client_id and authority and scope - self.app = msal.PublicClientApplication( - client_id, authority=authority, http_client=MinimalHttpClient()) + self.app = self._build_app(client_id, authority=authority) result = self.app.acquire_token_interactive( scope, prompt=prompt, timeout=120, port=port, + parent_window_handle=self.app.CONSOLE_WINDOW_HANDLE, # This test app is a console app welcome_template= # This is an undocumented feature for testing """

{id}

  1. Get a username from the upn shown at here
    2. From 6e9110907f0ba214b08f4c02e735168102cb4539 Mon Sep 17 00:00:00 2001 From: Ray Luo Date: Mon, 29 Aug 2022 12:31:06 -0700 Subject: [PATCH 02/10] Refine document --- msal/application.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/msal/application.py b/msal/application.py index a16e184..ef2cfd9 100644 --- a/msal/application.py +++ b/msal/application.py @@ -418,11 +418,12 @@ def __init__( :param boolean allow_broker: Brokers provide Single-Sign-On, device identification, and application identification verification. + This flag defaults to None, which means MSAL will not utilize broker. If this parameter is set to True, - MSAL will use the broker and return either a token or an error, - when your scenario is supported by a broker, - otherwise it will automatically fall back to non-broker behavior. - This also means you could set this flag as True universally, + MSAL will use the broker whenever possible, + and automatically fall back to non-broker behavior. + That also means your app does not need to enable broker conditionally, + you can always set allow_broker to True, as long as your app meets the following prerequisite: * Installed optional dependency, e.g. ``pip install msal[broker]>=1.20,<2``. From 9c34873901f46ddefcb842045eb91bf2d84d84f4 Mon Sep 17 00:00:00 2001 From: Ray Luo Date: Mon, 12 Sep 2022 14:36:57 -0700 Subject: [PATCH 03/10] Gracefully handle RuntimeError upfront --- msal/application.py | 112 ++++++++++++++++++++------------------------ msal/broker.py | 5 +- 2 files changed, 54 insertions(+), 63 deletions(-) diff --git a/msal/application.py b/msal/application.py index ef2cfd9..66370f5 100644 --- a/msal/application.py +++ b/msal/application.py @@ -505,10 +505,17 @@ def __init__( isinstance(self, ConfidentialClientApplication) or self.client_credential) if is_confidential_app and allow_broker: raise ValueError("allow_broker=True is only supported in PublicClientApplication") - self._enable_broker = bool( - allow_broker and not is_confidential_app - and sys.platform == "win32" - and not self.authority.is_adfs and not self.authority._is_b2c) + self._enable_broker = False + if (allow_broker and not is_confidential_app + and sys.platform == "win32" + and not self.authority.is_adfs and not self.authority._is_b2c): + try: + from . import broker # Trigger Broker's initialization + self._enable_broker = True + except RuntimeError: + logger.exception( + "Broker is unavailable on this platform. " + "We will fallback to non-broker.") logger.debug("Broker enabled? %s", self._enable_broker) self.token_cache = token_cache or TokenCache() @@ -1072,14 +1079,10 @@ def remove_account(self, account): """Sign me out and forget me from token cache""" self._forget_me(account) if self._enable_broker: - try: - from .broker import _signout_silently - except RuntimeError: # TODO: TBD - logger.debug("Broker is unavailable on this platform. Fallback to non-broker.") - else: - error = _signout_silently(self.client_id, account["local_account_id"]) - if error: - logger.debug("_signout_silently() returns error: %s", error) + from .broker import _signout_silently + error = _signout_silently(self.client_id, account["local_account_id"]) + if error: + logger.debug("_signout_silently() returns error: %s", error) def _sign_out(self, home_account): # Remove all relevant RTs and ATs from token cache @@ -1312,22 +1315,18 @@ def _acquire_token_silent_from_cache_and_possibly_refresh_it( return self._acquire_token_by_cloud_shell(scopes, data=data) if self._enable_broker and account is not None and data.get("token_type") != "ssh-cert": - try: - from .broker import _acquire_token_silently - except RuntimeError: # TODO: TBD - logger.debug("Broker is unavailable on this platform. Fallback to non-broker.") - else: - response = _acquire_token_silently( - "https://{}/{}".format(self.authority.instance, self.authority.tenant), - self.client_id, - account["local_account_id"], - scopes, - claims=_merge_claims_challenge_and_capabilities( - self._client_capabilities, claims_challenge), - correlation_id=correlation_id, - **data) - if response: # The broker provided a decisive outcome, so we use it - return self._process_broker_response(response, scopes, data) + from .broker import _acquire_token_silently + response = _acquire_token_silently( + "https://{}/{}".format(self.authority.instance, self.authority.tenant), + self.client_id, + account["local_account_id"], + scopes, + claims=_merge_claims_challenge_and_capabilities( + self._client_capabilities, claims_challenge), + correlation_id=correlation_id, + **data) + if response: # The broker provided a decisive outcome, so we use it + return self._process_broker_response(response, scopes, data) result = _clean_up(self._acquire_token_silent_by_finding_rt_belongs_to_me_or_my_family( authority, self._decorate_scope(scopes), account, @@ -1533,24 +1532,20 @@ def acquire_token_by_username_password( claims = _merge_claims_challenge_and_capabilities( self._client_capabilities, claims_challenge) if self._enable_broker: - try: - from .broker import _signin_silently - except RuntimeError: # TODO: TBD - logger.debug("Broker is unavailable on this platform. Fallback to non-broker.") - else: - response = _signin_silently( - "https://{}/{}".format(self.authority.instance, self.authority.tenant), - self.client_id, - scopes, # Decorated scopes won't work due to offline_access - MSALRuntime_Username=username, - MSALRuntime_Password=password, - validateAuthority="no" - if self.authority._validate_authority is False - or self.authority.is_adfs or self.authority._is_b2c - else None, - claims=claims, - ) - return self._process_broker_response(response, scopes, kwargs.get("data", {})) + from .broker import _signin_silently + response = _signin_silently( + "https://{}/{}".format(self.authority.instance, self.authority.tenant), + self.client_id, + scopes, # Decorated scopes won't work due to offline_access + MSALRuntime_Username=username, + MSALRuntime_Password=password, + validateAuthority="no" + if self.authority._validate_authority is False + or self.authority.is_adfs or self.authority._is_b2c + else None, + claims=claims, + ) + return self._process_broker_response(response, scopes, kwargs.get("data", {})) scopes = self._decorate_scope(scopes) telemetry_context = self._build_telemetry_context( @@ -1759,20 +1754,17 @@ def acquire_token_interactive( "04f0c124-f2bc-4f59-8241-bf6df9866bbd", # Visual Studio ] and data.get("token_type") != "ssh-cert" # Work around a known issue as of PyMsalRuntime 0.8 ) - try: - return self._acquire_token_interactive_via_broker( - scopes, - parent_window_handle, - enable_msa_passthrough, - claims, - data, - on_before_launching_ui, - prompt=prompt, - login_hint=login_hint, - max_age=max_age, - ) - except RuntimeError: # TODO: TBD - logger.debug("Broker is unavailable on this platform. Fallback to non-broker.") + return self._acquire_token_interactive_via_broker( + scopes, + parent_window_handle, + enable_msa_passthrough, + claims, + data, + on_before_launching_ui, + prompt=prompt, + login_hint=login_hint, + max_age=max_age, + ) on_before_launching_ui(ui="browser") telemetry_context = self._build_telemetry_context( diff --git a/msal/broker.py b/msal/broker.py index e9bbe28..1443505 100644 --- a/msal/broker.py +++ b/msal/broker.py @@ -10,8 +10,7 @@ logger = logging.getLogger(__name__) try: - import pymsalruntime # ImportError would be raised on unsupported platforms such as Windows 8 - # Its API description is available in site-packages/pymsalruntime/PyMsalRuntime.pyi + import pymsalruntime # Its API description is available in site-packages/pymsalruntime/PyMsalRuntime.pyi pymsalruntime.register_logging_callback(lambda message, level: { # New in pymsalruntime 0.7 pymsalruntime.LogLevel.TRACE: logger.debug, # Python has no TRACE level pymsalruntime.LogLevel.DEBUG: logger.debug, @@ -26,7 +25,7 @@ # https://github.com/AzureAD/microsoft-authentication-library-for-cpp/pull/2406/files raise ImportError( # TODO: Remove or adjust this line right before merging this PR 'You need to install dependency by: pip install "msal[broker]>=1.20.0b1,<2"') -# Other exceptions (possibly RuntimeError) would be raised if its initialization fails +# It could throw RuntimeError when running on ancient versions of Windows class RedirectUriError(ValueError): From 06590ab3f78cd1eec575ee44fed69fe24774a350 Mon Sep 17 00:00:00 2001 From: Ray Luo Date: Tue, 20 Sep 2022 22:34:00 -0700 Subject: [PATCH 04/10] Test acquire_token_silent() for confidential client --- tests/test_e2e.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/test_e2e.py b/tests/test_e2e.py index 9a971f4..51da742 100644 --- a/tests/test_e2e.py +++ b/tests/test_e2e.py @@ -133,6 +133,11 @@ def assertCacheWorksForApp(self, result_from_wire, scope): self.assertEqual( result_from_wire['access_token'], result_from_cache['access_token'], "We should get a cached AT") + self.app.acquire_token_silent( + # Result will typically be None, because client credential grant returns no RT. + # But we care more on this call should succeed without exception. + scope, account=None, + force_refresh=True) # Mimic the AT already expires def _test_username_password(self, authority=None, client_id=None, username=None, password=None, scope=None, @@ -618,11 +623,12 @@ def _test_acquire_token_by_client_secret( self, client_id=None, client_secret=None, authority=None, scope=None, **ignored): assert client_id and client_secret and authority and scope - app = msal.ConfidentialClientApplication( + self.app = msal.ConfidentialClientApplication( client_id, client_credential=client_secret, authority=authority, http_client=MinimalHttpClient()) - result = app.acquire_token_for_client(scope) + result = self.app.acquire_token_for_client(scope) self.assertIsNotNone(result.get("access_token"), "Got %s instead" % result) + self.assertCacheWorksForApp(result, scope) class WorldWideTestCase(LabBasedTestCase): From f41d5469cbec95e1781396f883e5abeb94ec7acd Mon Sep 17 00:00:00 2001 From: Ray Luo Date: Tue, 27 Sep 2022 22:25:15 -0700 Subject: [PATCH 05/10] Adopt pymsalruntime 0.13 --- msal/broker.py | 3 +-- setup.py | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/msal/broker.py b/msal/broker.py index 1443505..c9d0e38 100644 --- a/msal/broker.py +++ b/msal/broker.py @@ -90,8 +90,7 @@ def _convert_result(result, client_id, expected_token_type=None): # Mimic an on id_token_claims = json.loads(result.get_id_token()) if result.get_id_token() else {} account = result.get_account() assert account, "Account is expected to be always available" - ## Note: As of pymsalruntime 0.1.0, only wam_account_ids property is available - #account.get_account_property("wam_account_ids") + # Note: There are more account attribute getters available in pymsalruntime 0.13+ return_value = {k: v for k, v in { "access_token": result.get_access_token(), "expires_in": result.get_access_token_expiry_time() - int(time.time()), # Convert epoch to count-down diff --git a/setup.py b/setup.py index 31f585b..effc825 100644 --- a/setup.py +++ b/setup.py @@ -91,7 +91,7 @@ # The broker is defined as optional dependency, # so that downstream apps can opt in. The opt-in is needed, partially because # most existing MSAL Python apps do not have the redirect_uri needed by broker. - "pymsalruntime>=0.11.2,<0.12;python_version>='3.6' and platform_system=='Windows'", + "pymsalruntime>=0.11.2,<0.14;python_version>='3.6' and platform_system=='Windows'", ], }, ) From 28b45a394cec06ebea4dcd4ff165f2938af8cd96 Mon Sep 17 00:00:00 2001 From: Ray Luo Date: Tue, 27 Sep 2022 22:33:58 -0700 Subject: [PATCH 06/10] Remove automatic msa-pt for Azure CLI and Visual Studio --- msal/application.py | 26 +++++++++++--------------- tests/msaltest.py | 10 ++++++++-- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/msal/application.py b/msal/application.py index 5c8b7ba..7399235 100644 --- a/msal/application.py +++ b/msal/application.py @@ -1766,6 +1766,17 @@ def acquire_token_interactive( - A dict containing an "error" key, when token refresh failed. """ data = kwargs.pop("data", {}) + enable_msa_passthrough = kwargs.pop( # MUST remove it from kwargs + "enable_msa_passthrough", # Keep it as a hidden param, for now. + # OPTIONAL. MSA-Passthrough is a legacy configuration, + # needed by a small amount of Microsoft first-party apps, + # which would login MSA accounts via ".../organizations" authority. + # If you app belongs to this category, AND you are enabling broker, + # you would want to enable this flag. Default value is False. + # More background of MSA-PT is available from this internal docs: + # https://microsoft.sharepoint.com/:w:/t/Identity-DevEx/EatIUauX3c9Ctw1l7AQ6iM8B5CeBZxc58eoQCE0IuZ0VFw?e=tgc3jP&CID=39c853be-76ea-79d7-ee73-f1b2706ede05 + False + ) and data.get("token_type") != "ssh-cert" # Work around a known issue as of PyMsalRuntime 0.8 self._validate_ssh_cert_input_data(data) if not on_before_launching_ui: on_before_launching_ui = lambda **kwargs: None @@ -1786,21 +1797,6 @@ def acquire_token_interactive( logger.warning( "Ignoring parameter extra_scopes_to_consent, " "which is not supported by broker") - enable_msa_passthrough = kwargs.pop( - "enable_msa_passthrough", # Keep it as a hidden param, for now. - # OPTIONAL. MSA-Passthrough is a legacy configuration, - # needed by a small amount of Microsoft first-party apps, - # which would login MSA accounts via ".../organizations" authority. - # If you app belongs to this category, AND you are enabling broker, - # you would want to enable this flag. Default value is equivalent to False. - self.client_id in [ - # Experimental: Automatically enable MSA-PT mode for known MSA-PT apps - # More background of MSA-PT is available from this internal docs: - # https://microsoft.sharepoint.com/:w:/t/Identity-DevEx/EatIUauX3c9Ctw1l7AQ6iM8B5CeBZxc58eoQCE0IuZ0VFw?e=tgc3jP&CID=39c853be-76ea-79d7-ee73-f1b2706ede05 - "04b07795-8ddb-461a-bbee-02f9e1bf7b46", # Azure CLI - "04f0c124-f2bc-4f59-8241-bf6df9866bbd", # Visual Studio - ] and data.get("token_type") != "ssh-cert" # Work around a known issue as of PyMsalRuntime 0.8 - ) return self._acquire_token_interactive_via_broker( scopes, parent_window_handle, diff --git a/tests/msaltest.py b/tests/msaltest.py index 5d33f2c..cc4e560 100644 --- a/tests/msaltest.py +++ b/tests/msaltest.py @@ -1,6 +1,9 @@ import getpass, logging, pprint, sys, msal +AZURE_CLI = "04b07795-8ddb-461a-bbee-02f9e1bf7b46" +VISUAL_STUDIO = "04f0c124-f2bc-4f59-8241-bf6df9866bbd" + def _input_boolean(message): return input( "{} (N/n/F/f or empty means False, otherwise it is True): ".format(message) @@ -81,6 +84,9 @@ def _acquire_token_interactive(app, scopes, data=None): result = app.acquire_token_interactive( scopes, parent_window_handle=app.CONSOLE_WINDOW_HANDLE, # This test app is a console app + enable_msa_passthrough=app.client_id in [ # Apps are expected to set this right + AZURE_CLI, VISUAL_STUDIO, + ], # Here this test app mimics the setting for some known MSA-PT apps prompt=prompt, login_hint=login_hint, data=data or {}) if login_hint and "id_token_claims" in result: signed_in_user = result.get("id_token_claims", {}).get("preferred_username") @@ -142,8 +148,8 @@ def exit(app): def main(): print("Welcome to the Msal Python Console Test App, committed at 2022-5-2\n") chosen_app = _select_options([ - {"client_id": "04b07795-8ddb-461a-bbee-02f9e1bf7b46", "name": "Azure CLI (Correctly configured for MSA-PT)"}, - {"client_id": "04f0c124-f2bc-4f59-8241-bf6df9866bbd", "name": "Visual Studio (Correctly configured for MSA-PT)"}, + {"client_id": AZURE_CLI, "name": "Azure CLI (Correctly configured for MSA-PT)"}, + {"client_id": VISUAL_STUDIO, "name": "Visual Studio (Correctly configured for MSA-PT)"}, {"client_id": "95de633a-083e-42f5-b444-a4295d8e9314", "name": "Whiteboard Services (Non MSA-PT app. Accepts AAD & MSA accounts.)"}, ], option_renderer=lambda a: a["name"], From 3d6e977f7c2e7df2a3d1ad92fe1d960c2094730d Mon Sep 17 00:00:00 2001 From: Ray Luo Date: Thu, 29 Sep 2022 19:11:05 -0700 Subject: [PATCH 07/10] Refactor test infrastructure to expose a known bug Apply the refactor to similar code path --- msal/application.py | 5 +- msal/broker.py | 2 +- tests/test_e2e.py | 113 +++++++++++++++++++++++++++++++------------- 3 files changed, 85 insertions(+), 35 deletions(-) diff --git a/msal/application.py b/msal/application.py index 7399235..b132428 100644 --- a/msal/application.py +++ b/msal/application.py @@ -1857,6 +1857,7 @@ def _acquire_token_interactive_via_broker( if login_hint and prompt != "select_account": # OIDC prompts when the user did not sign in accounts = self.get_accounts(username=login_hint) if len(accounts) == 1: # Unambiguously proceed with this account + logger.debug("Calling broker._acquire_token_silently()") response = _acquire_token_silently( # When it works, it bypasses prompt authority, self.client_id, @@ -1868,6 +1869,7 @@ def _acquire_token_interactive_via_broker( return self._process_broker_response(response, scopes, data) # login_hint undecisive or not exists if prompt == "none" or not prompt: # Must/Can attempt _signin_silently() + logger.debug("Calling broker._signin_silently()") response = _signin_silently( # Unlike OIDC, it doesn't honor login_hint authority, self.client_id, scopes, validateAuthority=validate_authority, @@ -1903,7 +1905,8 @@ def _acquire_token_interactive_via_broker( pass # It will fall back to the _signin_interactively() else: return self._process_broker_response(response, scopes, data) - # Falls back to _signin_interactively() + + logger.debug("Falls back to broker._signin_interactively()") on_before_launching_ui(ui="broker") response = _signin_interactively( authority, self.client_id, scopes, diff --git a/msal/broker.py b/msal/broker.py index c9d0e38..f1de262 100644 --- a/msal/broker.py +++ b/msal/broker.py @@ -144,7 +144,7 @@ def _signin_silently( def _signin_interactively( authority, client_id, scopes, parent_window_handle, # None means auto-detect for console apps - prompt=None, + prompt=None, # Note: This function does not really use this parameter login_hint=None, claims=None, correlation_id=None, diff --git a/tests/test_e2e.py b/tests/test_e2e.py index 607af6e..cd3ee46 100644 --- a/tests/test_e2e.py +++ b/tests/test_e2e.py @@ -54,6 +54,25 @@ def _get_app_and_auth_code( assert ac is not None return (app, ac, redirect_uri) +def _render(url, description=None): + # Render a url in html if description is available, otherwise return url as-is + return "{description}".format( + url=url, description=description) if description else url + + +def _get_hint(html_mode=None, username=None, lab_name=None, username_uri=None): + return "Sign in with {user} whose password is available from {lab}".format( + user=("{}".format(username) if html_mode else username) + if username + else "the upn from {}".format(_render( + username_uri, description="here" if html_mode else None)), + lab=_render( + "https://aka.ms/GetLabUserSecret?Secret=" + (lab_name or "msidlabXYZ"), + description="this password api" if html_mode else None, + ), + ) + + @unittest.skipIf(os.getenv("TRAVIS_TAG"), "Skip e2e tests during tagged release") class E2eTestCase(unittest.TestCase): @@ -212,25 +231,31 @@ def _test_device_flow( def _test_acquire_token_interactive( self, client_id=None, authority=None, scope=None, port=None, - username_uri="", # But you would want to provide one + username=None, lab_name=None, + username_uri="", # Unnecessary if you provided username and lab_name data=None, # Needed by ssh-cert feature prompt=None, + enable_msa_passthrough=None, **ignored): assert client_id and authority and scope self.app = self._build_app(client_id, authority=authority) + logger.info(_get_hint( # Useful when testing broker which shows no welcome_template + username=username, lab_name=lab_name, username_uri=username_uri)) result = self.app.acquire_token_interactive( scope, + login_hint=username, prompt=prompt, timeout=120, port=port, parent_window_handle=self.app.CONSOLE_WINDOW_HANDLE, # This test app is a console app + enable_msa_passthrough=enable_msa_passthrough, # Needed when testing MSA-PT app welcome_template= # This is an undocumented feature for testing """

    {id}

      -
    1. Get a username from the upn shown at here
      2. -
    2. Get its password from https://aka.ms/GetLabUserSecret?Secret=msidlabXYZ - (replace the lab name with the labName from the link above).
      3. +
    3. {hint}
    4. Sign In or Abort
      5. -
    """.format(id=self.id(), username_uri=username_uri), +
""".format(id=self.id(), hint=_get_hint( + html_mode=True, + username=username, lab_name=lab_name, username_uri=username_uri)), data=data or {}, ) self.assertIn( @@ -239,6 +264,11 @@ def _test_acquire_token_interactive( # Note: No interpolation here, cause error won't always present error=result.get("error"), error_description=result.get("error_description"))) + if username and result.get("id_token_claims", {}).get("preferred_username"): + self.assertEqual( + username, result["id_token_claims"]["preferred_username"], + "You are expected to sign in as account {}, but tokens returned is for {}".format( + username, result["id_token_claims"]["preferred_username"])) self.assertCacheWorksForUser(result, scope, username=None, data=data or {}) return result # For further testing @@ -260,7 +290,7 @@ def test_ssh_cert_for_service_principal(self): self.assertEqual("ssh-cert", result["token_type"]) @unittest.skipIf(os.getenv("TRAVIS"), "Browser automation is not yet implemented") - def test_ssh_cert_for_user(self): + def test_ssh_cert_for_user_should_work_with_any_account(self): result = self._test_acquire_token_interactive( client_id="04b07795-8ddb-461a-bbee-02f9e1bf7b46", # Azure CLI is one # of the only 2 clients that are PreAuthz to use ssh cert feature @@ -555,7 +585,8 @@ def _test_acquire_token_by_auth_code( def _test_acquire_token_by_auth_code_flow( self, client_id=None, authority=None, port=None, scope=None, - username_uri="", # But you would want to provide one + username=None, lab_name=None, + username_uri="", # Optional if you provided username and lab_name **ignored): assert client_id and authority and scope self.app = msal.ClientApplication( @@ -568,11 +599,11 @@ def _test_acquire_token_by_auth_code_flow( auth_response = receiver.get_auth_response( auth_uri=flow["auth_uri"], state=flow["state"], timeout=60, welcome_template="""

{id}

    -
  1. Get a username from the upn shown at here
    2. -
  2. Get its password from https://aka.ms/GetLabUserSecret?Secret=msidlabXYZ - (replace the lab name with the labName from the link above).
    3. +
  3. {hint}
  4. Sign In or Abort
    5. -
""".format(id=self.id(), username_uri=username_uri), + """.format(id=self.id(), hint=_get_hint( + html_mode=True, + username=username, lab_name=lab_name, username_uri=username_uri)), ) if auth_response is None: self.skipTest("Timed out. Did not have test settings in hand? Prepare and retry.") @@ -592,6 +623,11 @@ def _test_acquire_token_by_auth_code_flow( # Note: No interpolation here, cause error won't always present error=result.get("error"), error_description=result.get("error_description"))) + if username and result.get("id_token_claims", {}).get("preferred_username"): + self.assertEqual( + username, result["id_token_claims"]["preferred_username"], + "You are expected to sign in as account {}, but tokens returned is for {}".format( + username, result["id_token_claims"]["preferred_username"])) self.assertCacheWorksForUser(result, scope, username=None) def _test_acquire_token_obo(self, config_pca, config_cca, @@ -689,10 +725,23 @@ def test_adfs2019_fed_user(self): @unittest.skipIf(os.getenv("TRAVIS"), "Browser automation is not yet implemented") def test_cloud_acquire_token_interactive(self): - config = self.get_lab_user(usertype="cloud") - self._test_acquire_token_interactive( - username_uri="https://msidlab.com/api/user?usertype=cloud", - **config) + self._test_acquire_token_interactive(**self.get_lab_user(usertype="cloud")) + + @unittest.skipIf(os.getenv("TRAVIS"), "Browser automation is not yet implemented") + def test_msa_pt_app_signin_via_organizations_authority_without_login_hint(self): + """There is/was an upstream bug. See test case full docstring for the details. + + When a MSAL-PT flow that account control is launched, user has 2+ AAD accounts in WAM, + selects an AAD account that is NOT the default AAD account from the OS, + it will incorrectly get tokens for default AAD account. + """ + self._test_acquire_token_interactive(**dict( + self.get_lab_user(usertype="cloud"), # This is generally not the current laptop's default AAD account + authority="https://login.microsoftonline.com/organizations", + client_id="04b07795-8ddb-461a-bbee-02f9e1bf7b46", # Azure CLI is an MSA-PT app + enable_msa_passthrough=True, + prompt="select_account", # In MSAL Python, this resets login_hint + )) def test_ropc_adfs2019_onprem(self): # Configuration is derived from https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/4.7.0/tests/Microsoft.Identity.Test.Common/TestConstants.cs#L250-L259 @@ -719,22 +768,22 @@ def test_adfs2019_onprem_acquire_token_by_auth_code(self): @unittest.skipIf(os.getenv("TRAVIS"), "Browser automation is not yet implemented") def test_adfs2019_onprem_acquire_token_by_auth_code_flow(self): config = self.get_lab_user(usertype="onprem", federationProvider="ADFSv2019") - config["authority"] = "https://fs.%s.com/adfs" % config["lab_name"] - config["scope"] = self.adfs2019_scopes - config["port"] = 8080 - self._test_acquire_token_by_auth_code_flow( - username_uri="https://msidlab.com/api/user?usertype=onprem&federationprovider=ADFSv2019", - **config) + self._test_acquire_token_by_auth_code_flow(**dict( + config, + authority="https://fs.%s.com/adfs" % config["lab_name"], + scope=self.adfs2019_scopes, + port=8080, + )) @unittest.skipIf(os.getenv("TRAVIS"), "Browser automation is not yet implemented") def test_adfs2019_onprem_acquire_token_interactive(self): config = self.get_lab_user(usertype="onprem", federationProvider="ADFSv2019") - config["authority"] = "https://fs.%s.com/adfs" % config["lab_name"] - config["scope"] = self.adfs2019_scopes - config["port"] = 8080 - self._test_acquire_token_interactive( - username_uri="https://msidlab.com/api/user?usertype=onprem&federationprovider=ADFSv2019", - **config) + self._test_acquire_token_interactive(**dict( + config, + authority="https://fs.%s.com/adfs" % config["lab_name"], + scope=self.adfs2019_scopes, + port=8080, + )) @unittest.skipUnless( os.getenv("LAB_OBO_CLIENT_SECRET"), @@ -816,14 +865,12 @@ def test_b2c_acquire_token_by_auth_code(self): @unittest.skipIf(os.getenv("TRAVIS"), "Browser automation is not yet implemented") def test_b2c_acquire_token_by_auth_code_flow(self): - config = self.get_lab_app_object(azureenvironment="azureb2ccloud") - self._test_acquire_token_by_auth_code_flow( + self._test_acquire_token_by_auth_code_flow(**dict( + self.get_lab_user(usertype="b2c", b2cprovider="local"), authority=self._build_b2c_authority("B2C_1_SignInPolicy"), - client_id=config["appId"], port=3843, # Lab defines 4 of them: [3843, 4584, 4843, 60000] - scope=config["scopes"], - username_uri="https://msidlab.com/api/user?usertype=b2c&b2cprovider=local", - ) + scope=self.get_lab_app_object(azureenvironment="azureb2ccloud")["scopes"], + )) def test_b2c_acquire_token_by_ropc(self): config = self.get_lab_app_object(azureenvironment="azureb2ccloud") From 66a90824267efe5ddd2a2413dd6050aaee3607cc Mon Sep 17 00:00:00 2001 From: Ray Luo Date: Thu, 6 Oct 2022 21:20:32 -0700 Subject: [PATCH 08/10] Add more docs --- msal/application.py | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/msal/application.py b/msal/application.py index b132428..8c803f7 100644 --- a/msal/application.py +++ b/msal/application.py @@ -444,9 +444,22 @@ def __init__( New in version 1.19.0. :param boolean allow_broker: - Brokers provide Single-Sign-On, device identification, - and application identification verification. - This flag defaults to None, which means MSAL will not utilize broker. + A broker is a component installed on your device. + Broker implicitly gives your device an identity. By using a broker, + your device becomes a factor that can satisfy MFA (Multi-factor authentication). + This factor would become mandatory + if a tenant's admin enables a corresponding Conditional Access (CA) policy. + The broker's presence allows Microsoft identity platform + to have higher confidence that the tokens are being issued to your device, + and that is more secure. + + An additional benefit of broker is, + it runs as a long-lived process with your device's OS, + and maintains its own cache, + so that your broker-enabled apps (even a CLI) + could automatically SSO from a previously established signed-in session. + + This parameter defaults to None, which means MSAL will not utilize a broker. If this parameter is set to True, MSAL will use the broker whenever possible, and automatically fall back to non-broker behavior. From 789342c4bc57f80314a83662af614f6f02a207d0 Mon Sep 17 00:00:00 2001 From: Ray Luo Date: Thu, 6 Oct 2022 21:32:36 -0700 Subject: [PATCH 09/10] MSAL Python 1.20.0 Bumping version number --- msal/application.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/msal/application.py b/msal/application.py index 8c803f7..916f717 100644 --- a/msal/application.py +++ b/msal/application.py @@ -25,7 +25,7 @@ # The __init__.py will import this. Not the other way around. -__version__ = "1.20.0b1" # When releasing, also check and bump our dependencies's versions if needed +__version__ = "1.20.0" # When releasing, also check and bump our dependencies's versions if needed logger = logging.getLogger(__name__) _AUTHORITY_TYPE_CLOUDSHELL = "CLOUDSHELL" From a044c324cbb59945547f8c7745e5f985b5320a55 Mon Sep 17 00:00:00 2001 From: Ray Luo Date: Thu, 6 Oct 2022 21:39:47 -0700 Subject: [PATCH 10/10] Update installation instruction --- msal/broker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/msal/broker.py b/msal/broker.py index f1de262..8b997c6 100644 --- a/msal/broker.py +++ b/msal/broker.py @@ -24,7 +24,7 @@ # PyMsalRuntime currently supports these Windows versions, listed in this MSFT internal link # https://github.com/AzureAD/microsoft-authentication-library-for-cpp/pull/2406/files raise ImportError( # TODO: Remove or adjust this line right before merging this PR - 'You need to install dependency by: pip install "msal[broker]>=1.20.0b1,<2"') + 'You need to install dependency by: pip install "msal[broker]>=1.20,<2"') # It could throw RuntimeError when running on ancient versions of Windows