Conversation
chlowell
added
Client
mccoyp
left a comment
mccoyp
left a comment
There was a problem hiding this comment.
I see recordings for test_device_code and test_username_password from test_cae.py, but are there any other recordings that can be/need to be added?
| # decode the recorded token | ||
| body = json.loads(six.ensure_str(response["body"]["string"])) | ||
| header, encoded_payload, signed = body["id_token"].split(".") | ||
| decoded_payload = base64.b64decode(encoded_payload + "=" * (4 - len(encoded_payload) % 4)) |
There was a problem hiding this comment.
Out of curiosity, what are the "=" added onto the payload for?
There was a problem hiding this comment.
A base64 encoded string should be padded with "=" to make its length divisible by 4. CPython's base64 strictly requires padding. However, because a decoder can infer the padding, encoders commonly omit it.
| msal_app = Mock() | ||
| msal_app.get_accounts.return_value = [{"home_account_id": record.home_account_id}] | ||
| msal_app.acquire_token_silent_with_error.return_value = dict( | ||
| build_aad_response(access_token="**", id_token=build_id_token()) |
There was a problem hiding this comment.
Does the SharedTokenCacheCredential not have an id_token_claims portion of the return value (like this one) because it's the only credential of these that doesn't subclass InteractiveCredential?
There was a problem hiding this comment.
Yes. That's relevant because MSAL token acquisition methods return a dict containing both the raw ID token and its claims. InteractiveCredential uses these claims to build an AuthenticationRecord. SharedTokenCacheCredential does not, because it already has that information.
| real = urlparse(self.cae_settings["arm_url"]) | ||
| self.scrubber.register_name_pair(real.netloc, "management.azure.com") | ||
| self.scrubber.register_name_pair(self.cae_settings["tenant_id"], "tenant") | ||
| self.scrubber.register_name_pair(self.cae_settings["username"], "username") |
There was a problem hiding this comment.
| self.scrubber.register_name_pair(self.cae_settings["username"], "username") | |
| self.scrubber.register_name_pair(self.cae_settings["username"], "username") | |
| self.scrubber.register_name_pair(self.cae_settings["password"], "password") |
Would this be necessary as well, or are passwords already hidden in recordings?
There was a problem hiding this comment.
Passwords are in request bodies, which aren't recorded.
|
I didn't record |
| result = app.acquire_token_silent_with_error( | ||
| list(scopes), account=account, claims_challenge=kwargs.get("claims") | ||
| ) |
There was a problem hiding this comment.
The removal of **kwargs will make acquiring SSH certificate stop working as data is passed in kwargs.
Azure CLI is aiming to support acquiring SSH certificate for Service Principal as well (#16397).
There was a problem hiding this comment.
Summarizing offline discussion here, there are two orthogonal concerns: acquiring SSH certificates, and allowing applications to pass keyword arguments through get_token to MSAL. Neither is supported today. Acquiring SSH certificates may be supported in a future version. Passing MSAL arguments through get_token is possible in some cases but really isn't supportable because routine maintenance requires internal changes (e.g. #16449) that can break applications relying on this behavior. My goal with this change is to prevent applications from taking a dependency on such implementation details.
…into enum-meta * 'master' of https://github.com/Azure/azure-sdk-for-python: bump six dependencies in some libraries (#16496) call on_error if timeout in flush (#16485) Sync eng/common directory with azure-sdk-tools for PR 1365 (#16505) Fix min dependency tests - update azure core (#16504) Sync eng/common directory with azure-sdk-tools for PR 1364 (#16503) Ma arch feedback (#16502) Adding a new limitation to the README file. (#16475) [Blob][Datalake] STG76 Preview (#16349) append code coverage over each other (#16202) Arch preview feedback (#16441) Support CAE in azure-identity (#16323) [EventHubs] Support for Custom endpoint adddress and custom certificate (#16295) [Communication] - Phone Number Management - Added support for AAD auth (#16075) fix live tests (#16495)
…into analyze_redesign * 'master' of https://github.com/Azure/azure-sdk-for-python: (32 commits) Adopt new MSAL auth code flow API (Azure#16449) [formrecognizer] use ARM template for tests (Azure#16432) T2 kusto 2021 02 04 (Azure#16527) T2 applicationinsights 2021 02 04 (Azure#16525) Sync eng/common directory with azure-sdk-tools for PR 1366 (Azure#16506) [Python] python track2 new pipeline fix (Azure#16494) Added package properties SDKType and NewSDK (Azure#16476) bump six dependencies in some libraries (Azure#16496) call on_error if timeout in flush (Azure#16485) Sync eng/common directory with azure-sdk-tools for PR 1365 (Azure#16505) Fix min dependency tests - update azure core (Azure#16504) Sync eng/common directory with azure-sdk-tools for PR 1364 (Azure#16503) Ma arch feedback (Azure#16502) Adding a new limitation to the README file. (Azure#16475) [Blob][Datalake] STG76 Preview (Azure#16349) append code coverage over each other (Azure#16202) Arch preview feedback (Azure#16441) Support CAE in azure-identity (Azure#16323) [EventHubs] Support for Custom endpoint adddress and custom certificate (Azure#16295) [Communication] - Phone Number Management - Added support for AAD auth (Azure#16075) ...
3 participants
With this, user credentials configure MSAL with client capability "CP1", and accept an optional "claims" argument to get_token.