Uh oh!
There was an error while loading. Please reload this page.
Simplifying ClaimsIdentity anonymous state - #540
Simplifying ClaimsIdentity anonymous state#540Rodrigo Brandão (rodrigobr-msft) wants to merge 12 commits into
ClaimsIdentity anonymous state#540Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors identity/claims handling in hosting-core to simplify anonymous ClaimsIdentity usage, and modernizes the activity models with updated type annotations and safer defaults. It aims to streamline authentication flows (especially anonymous) and improve model robustness.
Changes:
- Refactors
ClaimsIdentityconstruction/usage across adapters and JWT validation, introducing anallow_anonymoushelper. - Updates activity models to modern
| Nonetyping and usesdefault_factory=listto avoid mutable defaults. - Adjusts proactive conversation claims reconstruction to safely fall back to anonymous identities when claims are empty.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/channel_service_adapter.py | Switches anonymous-auth decision logic to ClaimsIdentity.allow_anonymous and threads it into client creation. |
| libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/authorization/jwt/jwt_token_validator.py | Updates validated/anonymous identity construction to the new ClaimsIdentity API. |
| libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/authorization/claims_identity.py | Refactors ClaimsIdentity initialization and adds allow_anonymous. |
| libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/app/proactive/conversation.py | Improves proactive identity reconstruction when claims are missing/empty. |
| libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/_http_adapter_base.py | Defaults missing request identity to ClaimsIdentity() for anonymous scenarios. |
| libraries/microsoft-agents-activity/microsoft_agents/activity/agents_model.py | Updates pick_properties to safely handle None inputs. |
| libraries/microsoft-agents-activity/microsoft_agents/activity/activity.py | Modernizes field typings/defaults and refactors create_reply / trace helpers. |
Suppressed comments (2)
libraries/microsoft-agents-activity/microsoft_agents/activity/activity.py:1017
get_conversation_referencecurrently declares an uninitializedactivity_idand returns early, making the real ConversationReference construction below unreachable and returning an invalid reference.
activity_id: str | None
return ConversationReference(activity_id=activity_id)
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/app/proactive/conversation.py:103
identity_from_claimsuses the deprecatedis_authenticatedparameter, which now logs a warning on every call. Sinceis_authenticatedis being deprecated, prefer settingauthentication_typeinstead.
if not claims:
return ClaimsIdentity()
return ClaimsIdentity(claims=dict(claims), is_authenticated=True)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (3)
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/app/proactive/conversation.py:47
Conversation.__init__has a strayself.identityexpression whenclaimsis aClaimsIdentity. This will raiseAttributeErrorduring construction and prevents proactive conversations from being created from aClaimsIdentity.
if isinstance(claims, ClaimsIdentity):
self.claims: dict[str, str] = Conversation.claims_from_identity(claims)
self.identity
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/authorization/claims_identity.py:41
ClaimsIdentity.__init__leavesis_authenticatedasNonewhen the caller omits it (e.g., JWT validation now constructsClaimsIdentity(decoded_token, security_token=token)). Downstream code and tests treat validated identities as authenticated (is_authenticated is True), so this change can silently flip behavior. Consider derivingis_authenticatedwhen it isn’t explicitly provided.
self.claims = claims or {}
if is_authenticated is not None:
logger.warning(
"The 'is_authenticated' parameter is deprecated and will be removed in future versions. Please use 'authentication_type' instead."
)
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/app/proactive/conversation.py:103
Conversation.identity_from_claimsstill uses the deprecatedis_authenticatedparameter, which will emit a warning on every call after the refactor. Since the identity already has non-empty claims, it can rely onClaimsIdentity’s default authentication inference instead.
if not claims:
return ClaimsIdentity()
return ClaimsIdentity(claims=dict(claims), is_authenticated=True)
…microsoft/Agents-for-python into users/robrandao/claims-identity
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (4)
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/authorization/claims_identity.py:60
allow_anonymouscurrently ignores the deprecatedis_authenticatedconstructor parameter (stored in_is_authenticated). This breaks the intended compatibility behavior and causes cases likeClaimsIdentity(is_authenticated=True)with empty claims to still allow anonymous access (and will fail the added tests).
"""Returns True if the identity allows anonymous access, otherwise False."""
return not self.authentication_type and not self.claims
@property
def is_authenticated(self) -> bool:
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/authorization/claims_identity.py:130
get_token_audienceis annotated as returningstr | None, but the current implementation always returns astr(either the Agents SDK scope or anapp://...audience). The optional return type needlessly forces callers to handleNoneand may introduce type-checking noise.
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/app/proactive/conversation.py:103identity_from_claimsstill passes the deprecatedis_authenticated=Trueparameter. This will emit a deprecation warning on every proactive identity reconstruction and contradicts the stated goal of removing reliance on deprecated auth flags; the identity will be considered authenticated based on non-empty claims anyway.
if not claims:
return ClaimsIdentity()
return ClaimsIdentity(claims=dict(claims), is_authenticated=True)
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/authorization/claims_identity.py:36
self.claims = claims or {}treats an explicitly provided empty dict the same asNoneand replaces it with a new dict. This is inconsistent with the behavior for non-empty dicts (where the passed object is preserved) and can surprise callers that pass{}intentionally.
This issue also appears in the following locations of the same file:
- line 56
- line 130
self.claims = claims or {}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/authorization/claims_identity.py:44
claims or {}replaces a caller-provided empty dict with a new dict, which can unexpectedly drop reference identity and makes it impossible to intentionally pass an empty claims mapping. Prefer an explicitNonecheck soClaimsIdentity(claims={})preserves the provided object while still avoiding shared defaults; also set_is_authenticatedbefore later properties rely on it.
self.claims = claims or {}
if is_authenticated is not None:
logger.warning(
"The 'is_authenticated' parameter is deprecated and will be removed in future versions."
)
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/app/proactive/conversation.py:103
Conversation.identity_from_claimsstill passes the deprecatedis_authenticatedparameter, which will emit warnings for a normal (non-empty) claims restore path. Since non-empty claims already imply an authenticated identity in the new model, drop the deprecated parameter to avoid noisy logs and keep the API surface consistent.
if not claims:
return ClaimsIdentity()
return ClaimsIdentity(claims=dict(claims), is_authenticated=True)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Suppressed comments (4)
tests/hosting_core/authorization/test_claims_identity.py:97
ClaimsIdentity.is_authenticatedemits aDeprecationWarningon access, so this assertion needs to capture the warning (or avoid the deprecated property) to keep tests passing under warnings-as-errors.
assert identity.is_authenticated is expected
tests/hosting_core/authorization/test_claims_identity.py:39
- This test currently accesses the deprecated
is_authenticatedproperty without capturing the emittedDeprecationWarning. With warnings-as-errors enabled, this will fail the test run.
assert identity.security_token == "token"
assert identity.is_authenticated is True
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/authorization/claims_identity.py:44
self.claims = claims or {}will replace a caller-provided empty dict with a new dict (breaking object identity / mutation expectations). Also_is_authenticatedis assigned but never used. Prefer an explicitNonecheck and drop the unused attribute to avoid confusion and subtle bugs.
self.claims = claims or {}
if is_authenticated is not None:
warnings.warn(
"The 'is_authenticated' parameter is deprecated and will be removed in future versions.",
DeprecationWarning,
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/authorization/claims_identity.py:31
- The
authentication_typedocstring currently saysNonemeans "not authenticated", but this class now derives authentication from presence of claims and usesallow_anonymousfor anonymous handling. Updating the wording will prevent confusion for SDK consumers (especially since validated identities may haveauthentication_type=None).
:param authentication_type: A string representing the type of authentication used.
None values indicate that the identity is not authenticated.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (4)
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/app/proactive/conversation.py:102
identity_from_claimsstill passes the deprecatedis_authenticatedparameter. Since authentication is now derived from the presence of claims, this argument is unnecessary and will emit a deprecation warning.
if not claims:
return ClaimsIdentity()
return ClaimsIdentity(claims=dict(claims), is_authenticated=True)
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/authorization/claims_identity.py:34
claims or {}will replace a caller-provided empty dict (or dict subclass) with a new dict, which is an observable behavior change. Use an explicitis Nonecheck so passing{}preserves the provided object while still avoiding shared defaults.
self.claims = claims or {}
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/authorization/claims_identity.py:57
allow_anonymouscurrently returns False whenauthentication_typeis set to "Anonymous" (even if there are no claims). This changes behavior from the previous anonymous detection logic and can break callers/tests that still useauthentication_type="Anonymous"to represent anonymous identities (e.g.tests/hosting_core/authorization/test_authorize_request.py:44). Consider treating "Anonymous" as anonymous during the deprecation window.
def allow_anonymous(self) -> bool:
"""Returns True if the identity allows anonymous access, otherwise False."""
return not self.authentication_type and not self.claims
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/authorization/claims_identity.py:131
get_token_audienceis annotated as returningstr | None, but the current implementation always returns astr(eitherapp://...orAuthenticationConstants.AGENTS_SDK_SCOPE). Consider tightening the return type to avoid forcing callers into unnecessaryNonehandling.
def get_token_audience(self) -> str | None:
…into users/robrandao/claims-identity
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (3)
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/authorization/claims_identity.py:54
get_claim_value()is annotated to returnstr | None, but claims values may be non-strings (JWT numeric timestamps, arrays, etc.). Returningobject | Nonebetter reflects reality and avoids misleading callers.
def get_claim_value(self, claim_type: str) -> str | None:
"""Gets the value of a specific claim type from the claims dictionary.
:param claim_type: The type of claim to retrieve.
:return: The value of the claim if found, otherwise None.
"""
return self.claims.get(claim_type)
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/authorization/claims_identity.py:46
self._is_authenticatedis assigned but never read anywhere, which makes the deprecation path harder to reason about and suggests stale state is being kept around unnecessarily.
self.authentication_type = authentication_type
self.security_token = security_token
self._is_authenticated = is_authenticated
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/authorization/claims_identity.py:17
ClaimsIdentity.claimsis typed asdict[str, str], butJwtTokenValidator.validate_token()passes the full decoded JWT payload (which can include non-string values like ints/lists, e.g.exp,nbf, oraudarrays). This makes the public type hints inaccurate and can cause type-checking friction for SDK consumers.
This issue also appears on line 48 of the same file.
claims: dict[str, str]
authentication_type: str | None
security_token: str | None # deprecated, will be removed in future versions
This pull request refactors the
ClaimsIdentityclass and its usage throughout the codebase to deprecate theis_authenticatedproperty in favor of a newallow_anonymousproperty, simplifying the handling of anonymous and authenticated identities. It updates the logic for identity creation, token validation, and adapter methods to consistently use the new approach, and revises tests and warnings accordingly.ClaimsIdentity Refactor and Deprecation:
ClaimsIdentityto deprecate theis_authenticatedproperty and introduceallow_anonymous, with corresponding warnings for deprecated usage. Updated constructor and methods to support this change, and improved docstrings and type hints. (microsoft_agents/hosting/core/authorization/claims_identity.py)ClaimsIdentityto remove theis_authenticatedparameter and rely on the new logic for anonymous and authenticated identities. (jwt_token_validator.py,channel_service_adapter.py,_http_adapter_base.py,conversation.py) [1][2][3][4][5][6][7][8][9][10]Test and Assertion Updates:
allow_anonymousinstead ofauthenticated, and to expectauthentication_typeto beNonefor anonymous requests. (test_aiohttp_jwt_validation.py,test_fastapi_jwt_validation.py) [1][2][3][4][5][6][7][8]test_conversation.py)Deprecation Warning Management:
pytest.inito suppress the newly introduced deprecation warnings foris_authenticated.Minor API and Type Improvements:
ClaimsIdentityfor better clarity and Python 3.10+ compatibility. [1][2]Test Adapter Updates:
is_authenticatedparameter when creatingClaimsIdentityobjects. (test_aiohttp_cloud_adapter.py,test_fastapi_cloud_adapter.py) [1][2]