Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new authentication mode intended to support “FederatedCredentials” across the hosting-core configuration surface and the MSAL-based token acquisition implementation.
Changes:
- Added
AuthTypes.federated_credentialsto the auth type enum. - Extended
AgentAuthConfigurationwith aFEDERATED_CLIENT_IDfield. - Added a new
federated_credentialsbranch inMsalAuth._create_client_applicationand did minor formatting cleanup inAgentApplicationerror raising.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/authorization/auth_types.py |
Adds the new federated_credentials enum value. |
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/authorization/agent_auth_configuration.py |
Adds configuration storage for FEDERATED_CLIENT_ID. |
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/app/agent_application.py |
Refactors ApplicationError raising formatting (no functional change). |
libraries/microsoft-agents-authentication-msal/microsoft_agents/authentication/msal/msal_auth.py |
Introduces token acquisition logic for the new auth type. |
Comments suppressed due to low confidence (1)
libraries/microsoft-agents-authentication-msal/microsoft_agents/authentication/msal/msal_auth.py:264
federated_credentialsbranch is using a ManagedIdentityClient to fetch an access token and then storing that access token intoclient_credentialforConfidentialClientApplication. MSAL interprets a stringclient_credentialas a client secret (or expects a cert/assertion structure), so passing an access token here will cause auth failures and the cached value will also expire without refresh. Consider implementing federated credentials as a proper client assertion (MSAL supports client assertions viaclient_credential), or if the intent is managed identity, return aManagedIdentityClientlike the existing managed identity auth types instead of wrapping it in a confidential client.
elif self._msal_configuration.AUTH_TYPE == AuthTypes.federated_credentials:
assert self._msal_configuration.FEDERATED_CLIENT_ID
mi_client = ManagedIdentityClient(
SystemAssignedManagedIdentity(), # TODO
http_client=Session(),
)
mi_token = mi_client.acquire_token_for_client(resource=self._msal_configuration.FEDERATED_CLIENT_ID)
if "access_token" not in mi_token:
logger.error(
f"Failed to acquire token for federated credentials: {mi_token}"
)
raise ValueError(
authentication_errors.FailedToAcquireToken.format(str(mi_token))
)
self._client_credential_cache = mi_token["access_token"]
else:
logger.error(
f"Unsupported authentication type: {self._msal_configuration.AUTH_TYPE}"
)
raise NotImplementedError(
str(authentication_errors.AuthenticationTypeNotSupported)
)
return ConfidentialClientApplication(
client_id=self._msal_configuration.CLIENT_ID,
authority=authority,
client_credential=self._client_credential_cache,
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert self._msal_configuration.FEDERATED_CLIENT_ID | ||
| mi_client = ManagedIdentityClient( | ||
| SystemAssignedManagedIdentity(), # TODO |
There was a problem hiding this comment.
Using assert self._msal_configuration.FEDERATED_CLIENT_ID for runtime configuration validation is unsafe because asserts can be stripped with Python optimization flags, leading to harder-to-debug failures later. Please replace this with an explicit exception (e.g., ValueError) that clearly explains which configuration field is missing.
| assert self._msal_configuration.FEDERATED_CLIENT_ID | |
| mi_client = ManagedIdentityClient( | |
| SystemAssignedManagedIdentity(), # TODO | |
| if not self._msal_configuration.FEDERATED_CLIENT_ID: | |
| raise ValueError( | |
| "Missing MSAL configuration field 'FEDERATED_CLIENT_ID' " | |
| "required for 'federated_credentials' authentication type." | |
| ) | |
| mi_client = ManagedIdentityClient( | |
| SystemAssignedManagedIdentity(), # TODO |
| elif self._msal_configuration.AUTH_TYPE == AuthTypes.federated_credentials: | ||
| assert self._msal_configuration.FEDERATED_CLIENT_ID | ||
| mi_client = ManagedIdentityClient( | ||
| SystemAssignedManagedIdentity(), # TODO | ||
| http_client=Session(), | ||
| ) | ||
| mi_token = mi_client.acquire_token_for_client(resource=self._msal_configuration.FEDERATED_CLIENT_ID) | ||
| if "access_token" not in mi_token: | ||
| logger.error( | ||
| f"Failed to acquire token for federated credentials: {mi_token}" | ||
| ) | ||
| raise ValueError( | ||
| authentication_errors.FailedToAcquireToken.format(str(mi_token)) | ||
| ) | ||
| self._client_credential_cache = mi_token["access_token"] |
There was a problem hiding this comment.
This new auth type introduces new behavior in _create_client_application, but there are existing pytest suites for MsalAuth and tenant resolution. Please add tests that cover the AuthTypes.federated_credentials branch (including failure paths when token acquisition fails), so regressions are caught early.
| def __init__( | ||
| self, | ||
| auth_type: AuthTypes = None, | ||
| client_id: str = None, | ||
| tenant_id: Optional[str] = None, | ||
| client_secret: Optional[str] = None, | ||
| cert_pem_file: Optional[str] = None, | ||
| cert_key_file: Optional[str] = None, | ||
| connection_name: Optional[str] = None, | ||
| authority: Optional[str] = None, | ||
| scopes: Optional[list[str]] = None, | ||
| anonymous_allowed: bool = False, | ||
| **kwargs: Optional[dict[str, str]], | ||
| ): | ||
|
|
||
| self.AUTH_TYPE = auth_type or kwargs.get("AUTHTYPE", AuthTypes.client_secret) | ||
| self.CLIENT_ID = client_id or kwargs.get("CLIENTID", None) | ||
| self.AUTHORITY = authority or kwargs.get("AUTHORITY", None) | ||
| self.TENANT_ID = tenant_id or kwargs.get("TENANTID", None) | ||
| self.CLIENT_SECRET = client_secret or kwargs.get("CLIENTSECRET", None) | ||
| self.CERT_PEM_FILE = cert_pem_file or kwargs.get("CERTPEMFILE", None) | ||
| self.CERT_KEY_FILE = cert_key_file or kwargs.get("CERTKEYFILE", None) | ||
| self.CONNECTION_NAME = connection_name or kwargs.get("CONNECTIONNAME", None) | ||
| self.SCOPES = scopes or kwargs.get("SCOPES", None) | ||
| self.ALT_BLUEPRINT_ID = kwargs.get("ALT_BLUEPRINT_NAME", None) | ||
| self.FEDERATED_CLIENT_ID = kwargs.get("FEDERATEDCLIENTID", None) | ||
| self.ANONYMOUS_ALLOWED = anonymous_allowed or kwargs.get( |
There was a problem hiding this comment.
AgentAuthConfiguration adds FEDERATED_CLIENT_ID, but the constructor doesn’t accept a corresponding explicit parameter (it’s only set via kwargs). For API consistency with the other fields (e.g., client_id, tenant_id, authority, etc.) and to match the PR description, add a federated_client_id: Optional[str] = None parameter and set self.FEDERATED_CLIENT_ID = federated_client_id or kwargs.get("FEDERATEDCLIENTID").
This pull request introduces support for a new "FederatedCredentials" authentication type. The main changes include adding the new authentication type, updating configuration options, and implementing the logic to acquire tokens for federated credentials.
Authentication Enhancements:
federated_credentialsto theAuthTypesenumeration, enabling support for federated credentials authentication flows.AgentAuthConfigurationto include a newFEDERATED_CLIENT_IDproperty, and updated the constructor to accept this parameter. [1] [2]msal_auth.pyto handle the newfederated_credentialstype, acquiring an access token using a managed identity client and caching it for subsequent use.