feat(oauth): route sign-in callbacks on turn state - #573
Merged
Lily Du (lilyydu) merged 10 commits intoAug 27, 2026
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes OAuth sign-in callback handling connection-aware in the Teams Python SDK by routing signin/tokenExchange, signin/verifyState, and signin/failure to the correct registered OAuthFlow (rather than always using the app’s default_connection_name). It introduces a small, internal pending-sign-in hint mechanism in per-turn user state to attribute callbacks that do not carry a connection name.
Changes:
- Add awaited global event emission (
EventEmitter.emit_async) and update OAuth handlers to await globalsign_in/sign_in_failureevents before invoking per-flow handlers. - Record and consume per-user pending OAuth “hints” in turn state to route connection-less callbacks (
verifyState,failure) to the right flow. - Add state mid-turn flush + rollback hooks so pending hints are persisted before sending the OAuth card.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/common/src/microsoft_teams/common/events/event_emitter.py | Adds emit_async() and fixes latent awaitable/once-handler edge cases in emit(). |
| packages/common/tests/test_event_emitter.py | Adds tests ensuring emit_async() awaits async handlers and awaitables returned by sync callables. |
| packages/apps/src/microsoft_teams/apps/oauth_state.py | New internal module for pending OAuth hint storage/validation/expiration. |
| packages/apps/src/microsoft_teams/apps/routing/activity_context.py | Records pending hints during ctx.sign_in(), flushes state pre-send, and rolls back on send failure. |
| packages/apps/src/microsoft_teams/apps/oauth_flow.py | Adds internal helpers to read/clear/retire pending hints and map them to registered flows. |
| packages/apps/src/microsoft_teams/apps/app_oauth.py | Routes callbacks by connection, awaits global events, and invokes per-flow handlers. |
| packages/apps/src/microsoft_teams/apps/state/container.py | Adds internal _save() hook for mid-turn persistence. |
| packages/apps/src/microsoft_teams/apps/state/loader.py | Injects saver into containers for _save() to use. |
| packages/apps/src/microsoft_teams/apps/state/turn_state.py | Adds _mark_dirty() to force persistence in rollback scenarios. |
| packages/apps/src/microsoft_teams/apps/app.py | Documents state-enabled behavior for multi-connection OAuth routing. |
| packages/apps/tests/test_activity_context.py | Adds tests for pre-send hint persistence and rollback behavior. |
| packages/apps/tests/test_app_oauth.py | Adds extensive routing/ordering/attribution tests for multi-connection OAuth callbacks. |
| packages/apps/tests/test_oauth_flow.py | Confirms OAuthFlow.sign_in() remains supported when ctx.state is None. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Lily Du (lilyydu)
pushed a commit
that referenced
this pull request
Aug 25, 2026
Resolves the two Copilot review comments on #573. `sign_in_verify_state` recorded telemetry for a non-HTTP exception but never emitted the global `error` event, so a crash during token verification was invisible to app-level error handlers. Token exchange already emitted it; verify-state now matches. Also finishes the newest-first hint ordering started in 04a308c: `_write_pending_oauth_sign_ins` is the single write chokepoint, so normalizing there keeps the stored document newest-first no matter which caller wrote it, and the reader now iterates in stored order instead of reversing it. Documents the reserved key's shape and replaces a stale comment that contradicted the sort below it. Reformats 04a308c to satisfy `ruff format --check`, which it was failing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Callback handlers assumed a single connection, so a registered non-default OAuthFlow's on_signin/on_signin_failure handlers never fired. Route signin/tokenExchange on the connection name in its payload (no state needed). For signin/verifyState and signin/failure, which carry no connection name, attribute via versioned pending hints recorded in per-turn user state when a sign-in card is sent. verifyState probes hinted flows, then remaining registered flows, then the legacy default. Failures are attributed to the newest hint that offered silent SSO, falling back to notifying all registered flows. After an SSO failure the hint is retired (sso_offered=False) rather than deleted, so it cannot re-attribute a second failure but a follow-up verifyState from the card's sign-in button still routes correctly. Populate connection_name on SignInEvent/SignInFailureEvent, preserving the registry's original casing. Global sign-in events are now awaited so async global handlers complete before per-flow handlers. State is optional: with state disabled, tokenExchange still routes correctly and the other callbacks use the registered-flow fallback. Legacy single-connection apps are unchanged. Error precedence and Token Service status handling are preserved: only 404 continues probing, 400/412 return 412, and other statuses propagate unchanged. Flush hints to storage before sending the card (with best-effort rollback) so a token-exchange callback arriving mid-turn can still be attributed. Add EventEmitter.emit_async(); this also fixes awaitables from non-async-def callables never being awaited, and async once handlers firing twice. Token-exchange deduplication is intentionally out of scope. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Resolves the two Copilot review comments on #573. `sign_in_verify_state` recorded telemetry for a non-HTTP exception but never emitted the global `error` event, so a crash during token verification was invisible to app-level error handlers. Token exchange already emitted it; verify-state now matches. Also finishes the newest-first hint ordering started in 04a308c: `_write_pending_oauth_sign_ins` is the single write chokepoint, so normalizing there keeps the stored document newest-first no matter which caller wrote it, and the reader now iterates in stored order instead of reversing it. Documents the reserved key's shape and replaces a stale comment that contradicted the sort below it. Reformats 04a308c to satisfy `ruff format --check`, which it was failing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
TurnStateLoader.save reads conversation_id/user_id back off the container to build its storage keys, so rebinding either one mid-turn would persist the turn's state under a different conversation or user while the scopes still hold the original identity's data. Expose both as read-only properties over private backing fields, which makes reassignment an error at runtime and under Pyright. Only the identity is bound: conversation/user and the injected deleter/saver stay ordinary mutable attributes, and the TurnState scopes remain mutable, so seal(), delete() and _save() are unchanged. @DataClass(frozen=True) was not used because it would also freeze the scope slots and the injected hooks. Declaring the fields privately under a dataclass was rejected too, since it would rename the public keyword arguments; an InitVar plus a same-named property is worse still, as the property object silently becomes the field default and turns the required conversation_id into an optional one. The class is therefore hand-written, with __eq__ and __repr__ reproducing what the dataclass generated: the same four compared fields, hooks excluded from both, and instances left unhashable. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Pending sign-in state used a Python-only document at `__oauth:pending`
holding a versioned list of hints. The C# SDK instead writes two keys per
connection, each holding a bare ISO 8601 timestamp:
__oauth:pending:{connection}
__oauth:pending:sso:{connection}
Adopt that layout and value format so all three SDKs describe this state
identically. Reserved keys become a public contract once shipped, so this
lands before the first release that exposes them.
The in-memory API is unchanged: `PendingOAuthSignIn` still carries an
epoch `created_at` and a `sso_offered` bool, and all five helpers keep
their signatures, so `oauth_flow`, `activity_context` and `app_oauth`
need no edits. Conversion happens only at the storage boundary.
Timestamps are read with `datetime.fromisoformat`, which since Python
3.11 (this package's floor) parses every shape .NET emits for a
`DateTimeOffset`, including 7-digit fractional seconds, a `Z` suffix and
non-UTC offsets. A value with no offset is read as UTC; forcing the
result to be aware also makes `timestamp()` pure arithmetic, so it can
neither raise nor return a non-finite value.
Two places where we do better than the layout we are adopting:
- `sso:` is a legal start to a connection name, so `__oauth:pending:sso:x`
is only treated as an SSO marker when `x`'s own marker exists. C# reads
its keys through a per-flow connection field and never disambiguates.
- Duplicate keys differing only in case resolve to the newer attempt and
the loser is removed, instead of being picked arbitrarily.
Per-key storage also shrinks the blast radius: a malformed key now
discards only itself, where the old reader dropped every hint. Orphaned
SSO markers are swept so they cannot outlive the sign-in they describe.
No migration is required. Pending hints expire after five minutes, so
old-format state simply reads as absent and takes the existing fallback
probe path.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The `nonfinite` and `oversized` corrupt-attribution params were named for the isfinite and overflow guards removed when timestamps became ISO 8601 strings. Both now take the same non-string rejection path as `malformed`, so they described code that no longer exists. Keep one, named for what it actually exercises; `stale` and `future` still reach the age and skew branches through well-formed values. Add coverage for state written by an earlier revision of this branch: the single `__oauth:pending` document has no trailing separator, so the prefix scan never sees it. It reads as absent, sign-in starts over, and healthy markers alongside it are untouched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Nine behaviour fixes across the OAuth surface, all found by comparing the
Python lifecycle against the TypeScript one.
Connection names are now normalised in one place (`oauth_connection.py`)
instead of each call site calling `.lower()`. Names are trimmed, blanks are
rejected at registration, and lookup is case- and whitespace-insensitive
while flows and events keep the casing the app registered. A stray space in
portal config no longer registers a silent second flow.
Error precedence:
* `ctx.sign_in()` only falls back to a sign-in card on a 404. A 400, 412,
transport fault or bug used to be swallowed and shown to the user as
"please sign in", hiding real outages.
* `ctx.sign_out()` no longer swallows Token Service errors. A failed
sign-out leaves the token in place, so callers must see it.
* Non-HTTP failures in token-exchange and verify-state propagate instead
of being flattened into a 412. `ActivityProcessor` already emits the
ErrorEvent and re-raises, so they are reported exactly once. `next()`
still runs from the finally block.
`signin/verifyState` carries no connection name, so 400, 404 and 412 are all
candidate misses rather than terminal failures: probing continues and 404 is
returned only once every candidate has missed. Unexpected statuses still stop
immediately with their status preserved.
`SignInOptions.override_sign_in_activity` is honoured. It has existed since
#85 and was read by nothing, so it was dead from birth. Group conversations
still target the requesting user unless the override picks its own recipient
— an override replaces the card, not the privacy rule.
Pending sign-in attribution gains a bounded process-local fallback for apps
that have not enabled state, keyed by conversation, user and connection with
a five-minute TTL and a 1000-entry cap evicted oldest-first. State always
wins when available and nothing auto-enables it; another instance finds
nothing and falls back to the existing probing, as before.
`ctx.get_token_status()` is registry-aware. The bulk call can lag a token
that was just written or omit a connection entirely, so registered flows that
come back missing or false are re-checked with a direct lookup. Unregistered
connections pass through untouched and non-404 failures propagate.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…tion Two review fixes on the per-flow OAuth surface. Flow handlers were dispatched with a bare `await handler(event)`, which meant a synchronous handler raised `TypeError` on an invalid await, and a handler that raised took down every handler registered after it and turned a successful sign-in into a failed invoke response. Dispatch now calls the handler, awaits the result only when it is actually awaitable, and logs a failure before continuing to the next one. Handlers may now be sync, async, or a sync callable returning an awaitable, and the type aliases say so. Order is unchanged: each handler still completes before the next begins. `signin/failure` carries no connection name, so the failed connection is recovered from the pending sign-in recorded at sign-in time. When flows are registered and nothing resolves — usually a callback that reached a different process, with no state to bridge them — the connection is genuinely unknown, but it was reported as the default. That is a guess, and it puts an uninvolved connection on a failure event and in telemetry. The global `SignInFailureEvent.connection_name` is now `None` on that path and `record_oauth_operation` omits the connection attribute rather than naming one. Legacy apps with no registered flows are unaffected: the default really is the only connection, so it is still reported. Fan-out is unchanged — every registered flow's failure handlers still run, each with its own canonical name — and the docstrings now spell out why that happens and that enabling state avoids it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Registering a flow needs somewhere durable to record which connection a sign-in started on, because `signin/verifyState` and `signin/failure` arrive without one. Python required opting into state by hand and otherwise fell back to the bounded process-local cache, so the common setup — register a flow, say nothing about state — silently got attribution that cannot survive a callback landing on another instance. C# sets IsStateEnabled when a flow is added and TypeScript auto-enables when state is omitted; this matches them. `add_oauth_flow` now resolves the default loader when the `state` option was never set. Nothing else changes: an explicit value is always honoured, so `state=False` still opts out and keeps the process-local fallback, and `state=True` or a `StateOptions` keeps the exact loader and storage it built. Registering a second flow reuses the loader rather than rebuilding it. The distinction this relies on already existed and needed no new sentinel: `options.state` is `Optional[Union[bool, StateOptions]]`, `from_typeddict` drops `None` values, and `False` survives — so `None` means "unset" and `False` means "off". `options.state` is left recording what the caller passed rather than being rewritten to True, since the loader is the only thing read downstream. The loader is also assigned onto the ActivityProcessor, which holds its own reference and reads it per turn; updating only the app would leave turns without state. State is switched on only after the registry accepts the flow, so a blank or duplicate connection name cannot enable it as a side effect. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Lily Du (lilyydu)
force-pushed
the
lilyydu-implement-python-oauth-state-routing
branch
from
August 27, 2026 23:21
fc75071 to
bbaf708
Compare
Corina (corinagum)
approved these changes
Aug 27, 2026
Lily Du (lilyydu)
deleted the
lilyydu-implement-python-oauth-state-routing
branch
August 27, 2026 23:57
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack: #553 (state foundation) → #554 (public state API / dispatch lifecycle) →
#561 (OAuthFlow + registry) → this PR (4/5) → PR 5 (token-exchange dedup)
This PR makes callback routing connection-aware.
Problem
The three sign-in callbacks carry very different amounts of information:
signin/tokenExchangeactivity.value.connection_namesignin/verifyStatesignin/failureToken exchange can be routed directly. The other two cannot, so the app has to
remember which connection it last offered a sign-in on.
Approach
Explicit routing where possible.
signin/tokenExchangeroutes purely on theconnection name in the payload — no state required, and it works with state
disabled.
Pending markers for connection-less callbacks. When a sign-in card is sent,
ctx.sign_in()records the attempt in per-turn user state under privatereserved keys — up to two per connection, each holding an ISO 8601 UTC timestamp:
Probing with fallback.
signin/verifyStatetries attributed flows first, thenthe remaining registered flows, then the legacy default connection, stopping at
the first connection that returns a token. Attribution makes the normal path a
single Token Service call; the fan-out is the fallback for missing or stale
markers. It is intentionally uncapped — dropping candidates would turn a slow
sign-in into a silently failed one.
Failure attribution.
signin/failureis attributed to the newest attempt thatactually offered silent SSO. With no attribution it falls back to notifying every
registered flow.
SSO markers are retired, not deleted. After a silent-SSO failure Teams still
renders the sign-in button on the same card, so the sign-in is still pending even
though its SSO attempt is spent. Only the
sso:key is dropped: the attempt canno longer re-attribute a second failure, but a follow-up
verifyStatestillroutes straight to the right connection, and it expires on its original schedule.
Behavior
SignInEvent.connection_nameandSignInFailureEvent.connection_nameare nowpopulated, preserving the registry's original casing (lookup is
case-insensitive;
"github"resolves the flow registered as"GitHub").sign_in/sign_in_failureevents fire before per-flow handlers, andare now awaited, so an async global handler is guaranteed to complete first.
falls through to
default_connection_nameexactly as before.flow and unregistered connections coexist.
continues probing, 400/412 return 412, and any other status propagates
unchanged and still emits
error. No service failure is masked as alogged-out/unknown callback.
future-dated and unknown-connection markers, is discarded and routing falls
back to the default connection.
State is optional
State is only required for the two callbacks that genuinely cannot self-identify.
With state disabled,
tokenExchangestill routes correctly andverifyState/failureuse the registered-flow fallback. No generic TTL knobs are introduced —storage expiry remains provider-configured; the 5-minute freshness window is
OAuth-specific and internal.
Race fix: markers are flushed before the card is sent
Turn state is normally persisted at end-of-turn, but a token-exchange callback can
arrive before the turn that sent the card has finished.
ctx.sign_in()nowflushes the marker to storage before sending the card, and rolls it back (best
effort, without masking the caller's error) if the send fails.
Container identity is bound at load
TurnStateContainer.conversation_idand.user_iddescribe which state wasloaded. Reassigning them after construction cannot move the data and would make
the container save under a key that no longer matches its contents, so they are
now read-only: private backing fields behind getter-only properties, rejected at
both runtime and type-check time.
The contained
TurnStateobjects stay fully mutable andseal(),delete()and_save()are unchanged. The constructor signature is identical (conversation_id=/
user_id=keywords), so no call site moved.TurnStateContaineris no longer a@dataclass—__eq__and__repr__are hand-written to reproduce the generatedones exactly, including excluding the injected hooks.
Supporting changes
EventEmitter.emit_async()— awaits all handlers. Purely additive; existingemit()stays fire-and-forget andEventEmitterProtocolis unchanged, soexternal implementers are unaffected.
emit()refactor fixes two latent bugs: awaitables returned by callablesthat aren't
async def(e.g.functools.partial,__call__) were created andnever awaited, and an async
oncehandler could fire twice.TurnStateContainer._save()/TurnState._mark_dirty()— internal hooks forthe mid-turn flush and its rollback.
Not in this PR
Token-exchange deduplication (in-flight guard + completed marker) is PR 5.
Duplicate
tokenExchangecallbacks still run the exchange twice here, and thereare tests pinning that so the change in PR 5 is visible.