Adding MsalTokenCredential implementation of azure.core.credentials.AsyncTokenProvider - #565
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an MSAL-backed Azure Core AsyncTokenCredential (MsalTokenCredential) to the microsoft-agents-authentication-msal package, updates MsalAuth to produce Azure AccessToken objects with proper expiry handling, and introduces unit/integration coverage to validate Azure SDK compatibility.
Changes:
- Introduces
MsalTokenCredentialimplementing Azure CoreAsyncTokenCredentialand exports it from the package public API. - Refactors
MsalAuthtoken acquisition to computeexpires_onfromexpires_inand return AzureAccessTokeninternally. - Adds unit + integration tests and improves integration-test env var gating utilities.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/authentication_msal/test_msal_token_credential.py | Adds unit tests for scope/resource handling, error cases, and MSAL forwarding. |
| tests/authentication_msal/test_msal_auth.py | Adds unit test validating expires_in → expires_on conversion. |
| tests/_common/testing_objects/mocks/mock_msal_auth.py | Updates MSAL auth mock payload defaults for expiration-aware tokens. |
| libraries/microsoft-agents-authentication-msal/readme.md | Documents the new MsalTokenCredential API and usage example. |
| libraries/microsoft-agents-authentication-msal/microsoft_agents/authentication/msal/msal_token_credential.py | Adds the new Azure Core credential implementation. |
| libraries/microsoft-agents-authentication-msal/microsoft_agents/authentication/msal/msal_auth.py | Adds AccessToken support and expiry computation via expires_in. |
| libraries/microsoft-agents-authentication-msal/microsoft_agents/authentication/msal/init.py | Exposes MsalTokenCredential from the package root. |
| dev/integration/tests/utils/pytest.py | Refines env var skip marker helper for integration tests. |
| dev/integration/tests/auth/test_msal_token_credential.py | Adds an integration test validating real token acquisition returns an AccessToken. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
libraries/microsoft-agents-authentication-msal/microsoft_agents/authentication/msal/msal_auth.py:145
_get_access_token()currently requiresexpires_inand derivesexpires_onastime.time() + expires_in. MSAL token payloads may supplyexpires_ondirectly (epoch seconds), and using it when present avoids failures whenexpires_inis missing and reduces reliance on local clock math.
expires_in = auth_result_payload.get("expires_in")
if expires_in is None:
raise ValueError("Token response does not include an expiration.")
return AccessToken(res, int(time.time()) + int(expires_in))
dev/integration/tests/utils/pytest.py:24
- When
load_root_env_file=True, the merge order makes.envvalues override real environment variables ({**os.environ, **dotenv_values('.env')}). This is surprising in CI/local shells where exported env vars should take precedence over.envdefaults.
if load_root_env_file:
# Load environment variables from the root .env file if specified
environ = {**os.environ, **dotenv_values(".env")}
environment = os.environ if environ is None else environ
libraries/microsoft-agents-authentication-msal/readme.md:251
- The docs state the first scope "must be an absolute resource URI", but
MsalTokenCredential.get_token()currently accepts any string and only strips a trailing/.default. Either enforce the constraint in code or soften the wording here to match actual behavior.
At least one scope is required. The first scope must be an absolute resource
URI (typically ending in `/.default`). For client-credential flows, all requested
scopes are passed to MSAL; managed identity uses the derived resource.
…microsoft/Agents-for-python into users/robrandao/msal-token-cred
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
libraries/microsoft-agents-authentication-msal/readme.md:246
- The README example uses
awaitat top-level, which isn’t valid in a normal Python script. Adjusting the snippet to define an async entry point (and run it viaasyncio.run) makes it directly runnable and clearer.
Explore working examples in the [Python samples repository](https://github.com/microsoft/Agents/tree/main/samples/python):
libraries/microsoft-agents-authentication-msal/microsoft_agents/authentication/msal/msal_token_credential.py:55
- MsalTokenCredential eagerly constructs MsalAuth in init, so calling get_token() with no scopes still creates the auth provider. This both does unnecessary work and makes the new unit test
test_get_token_requires_at_least_one_scopefail (it expects MsalAuth is never constructed when scopes are missing). Lazy-initialize MsalAuth after validating scopes.
self._config = config
self._provider = MsalAuth(config)
async def get_token(self, *scopes: str, **kwargs) -> AccessToken:
"""Acquire an access token for the specified scopes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
libraries/microsoft-agents-authentication-msal/microsoft_agents/authentication/msal/msal_auth.py:144
- The ValueError raised when the token response lacks expiration information is not very actionable, and it also misses the opportunity to indicate what the response contained without leaking the access token. Consider including the response keys (or other non-sensitive metadata) so callers can diagnose MSAL/AAD responses more easily.
expires_in = auth_result_payload.get("expires_in")
if expires_in is None:
raise ValueError("Token response does not include an expiration.")
changelog.md:9
- The v1.6.0 release notes only mention the resource extraction bug fix, but this PR also introduces a new public API (
MsalTokenCredential) and changes token handling to returnAccessTokenwithexpires_on. Please capture the new API/behavior in the changelog so consumers can discover it.
## Bug Fixes
- **MSAL Resource Extraction**: Preserved `api://` and other non-default scopes while removing only a trailing `/.default` when deriving the authentication resource.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
libraries/microsoft-agents-authentication-msal/microsoft_agents/authentication/msal/msal_token_credential.py:4
- If
get_token()is called concurrently on the same credential instance, the lazy_providerinitialization can race. Introducing an async lock for provider initialization requires importingasyncio.
import logging
libraries/microsoft-agents-authentication-msal/microsoft_agents/authentication/msal/msal_token_credential.py:36
MsalTokenCredentiallazily initializes_providerwithout synchronization. Adding anasyncio.Lockon the instance avoids races when multiple tasks callget_token()concurrently on the same credential.
self._config = config
self._provider: MsalAuth | None = None
libraries/microsoft-agents-authentication-msal/microsoft_agents/authentication/msal/msal_token_credential.py:55
- Lazy initialization of
_provideris not concurrency-safe: two concurrentget_token()calls can create multipleMsalAuthinstances and lose token-cache reuse. Guard initialization with an async lock and preferis Nonechecks over truthiness checks.
if not self._provider:
self._provider = MsalAuth(self._config)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
libraries/microsoft-agents-authentication-msal/microsoft_agents/authentication/msal/msal_auth.py:145
- MSAL token responses may include an absolute
expires_onfield (epoch seconds) instead of (or in addition to)expires_in. Requiringexpires_inunconditionally can raise even when the response includes a usable expiration, and it can block token acquisition for some MSAL flows.
expires_in = auth_result_payload.get("expires_in")
if expires_in is None:
raise ValueError("Token response does not include an expiration.")
return AccessToken(res, int(time.time()) + int(expires_in))
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
dev/integration/tests/utils/pytest.py:23
- When
load_root_env_file=True, the merge order gives.envvalues precedence over real environment variables (os.environ). This is inconsistent with_ENVIRONMENT = {**dotenv_values('.env'), **os.environ}used elsewhere and can cause CI/secret-injected env vars to be overridden by a checked-in.envfile. Swap the merge order soos.environwins.
environ = {**os.environ, **dotenv_values(".env")}
tests/_common/testing_objects/mocks/mock_msal_auth.py:18
acquire_token_for_client_returnuses a mutable dict as a default argument, which can be shared across instances and lead to cross-test contamination if it’s ever mutated. UseNoneas the default and create a new dict inside the constructor.
acquire_token_for_client_return={
"access_token": "token",
"expires_in": 3600,
},
This pull request introduces a new
MsalTokenCredentialclass that provides an asynchronous Azure Core token credential backed by MSAL, along with related updates to authentication logic, documentation, and tests. The changes enhance compatibility with Azure SDKs, improve access token handling, and add comprehensive integration and unit tests.New Azure Core Token Credential and Authentication Improvements:
MsalTokenCredentialimplementingAsyncTokenCredential, enabling use of agent authentication with Azure SDKs and other libraries that accept Azure Core credentials. This includes resource extraction from scopes and error handling for missing scopes.MsalAuthto returnAccessTokenobjects with proper expiration (expires_on), and refactored token acquisition logic to support the new credential class. [1] [2] [3] [4]MsalTokenCredentialin the package’s public API and documented its usage in thereadme.md, including example code and usage notes. [1] [2]Testing and Integration:
MsalTokenCredential, including scope/resource handling, error propagation, and Azure SDK compatibility. [1] [2]These changes make it significantly easier to use agent authentication with Azure SDKs and ensure robust, well-tested token acquisition logic.