refactor(apps): add transport seam at HttpServerAdapter layer - #590
Open
Lily Du (lilyydu) wants to merge 3 commits into
Open
refactor(apps): add transport seam at HttpServerAdapter layer#590Lily Du (lilyydu) wants to merge 3 commits into
Lily Du (lilyydu) wants to merge 3 commits into
Conversation
Port of microsoft/teams.ts#720. Adds a transport-agnostic inbound seam so non-HTTP transports (e.g. a future Socket Mode adapter) can plug into HttpServer. - HttpRequest.token: transports that authenticate at the connection level (e.g. Socket Mode) can deliver a pre-resolved token; HttpServer trusts it and skips per-request JWT validation. HTTP adapters never set it, so HTTP behavior is unchanged. - HttpServerInitializeDeps + optional adapter.initialize(deps) hook: HttpServer.initialize() forwards credentials/cloud to the adapter via getattr(), so it isn't a required Protocol member (avoids breaking existing/external adapters that don't implement it. - Kept HttpServer.initialize() sync (TS's is async) since the actual Socket Mode adapter.initialize() does no awaiting. Pure refactor: no behavior change for existing HTTP adapters. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> EOF )
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new pre-authenticated token bypass currently lacks basic runtime validation/expiry handling and the adapter initialize() contract documentation does not match actual synchronous invocation behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a transport-agnostic inbound seam at the HttpServerAdapter boundary so future non-HTTP transports (e.g., Socket Mode) can integrate with HttpServer without changing existing HTTP adapter behavior.
Changes:
- Introduces
HttpServerInitializeDepsand forwards app-level deps (credentials,cloud) to an optional adapterinitialize(...)hook. - Extends
HttpRequestwith an optional pre-authenticatedtokenthat bypasses per-request JWT validation. - Adds/updates unit tests covering adapter dep forwarding and the pre-authenticated token bypass.
File summaries
| File | Description |
|---|---|
| packages/apps/src/microsoft_teams/apps/http/http_server.py | Forwards init deps to adapter hook; adds request["token"] pre-auth seam in handle_request(). |
| packages/apps/src/microsoft_teams/apps/http/adapter.py | Adds HttpServerInitializeDeps; extends HttpRequest with optional token; updates adapter protocol docs. |
| packages/apps/src/microsoft_teams/apps/http/init.py | Re-exports HttpServerInitializeDeps. |
| packages/apps/src/microsoft_teams/apps/init.py | Re-exports HttpServerInitializeDeps at the microsoft_teams.apps package level. |
| packages/apps/tests/test_http_server.py | Adds tests for dep forwarding and pre-auth token bypass. |
Review details
Suppressed comments (1)
packages/apps/tests/test_http_server.py:296
- The pre-authenticated token stub in this test is missing several
TokenProtocolattributes (e.g.,app_display_name,tenant_id,expiration). IfHttpServeradds aTokenProtocolruntime check (recommended), this test will fail and the stub won't reflect the actual contract adapters must provide.
pre_authenticated_token = SimpleNamespace(
app_id="socket-app",
from_="azure",
from_id="",
service_url="https://smba.trafficmanager.net/teams",
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+140
to
+146
| pre_authenticated_token = request.get("token") | ||
| if pre_authenticated_token is not None: | ||
| # A transport that authenticates at the connection level (e.g. Socket | ||
| # Mode) already resolved the caller's identity; trust it and skip the | ||
| # per-request JWT validation below. | ||
| token: TokenProtocol = pre_authenticated_token | ||
| elif self._dangerously_allow_unauthenticated_requests: |
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>
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.
Port of microsoft/teams.ts#720.
Adds a transport-agnostic inbound seam so non-HTTP transports (e.g. a future Socket Mode adapter) can plug into HttpServer.
HttpServerInitializeDeps+ optionaladapter.initialize(deps)hook:HttpServer.initialize()forwards credentials/cloud to the adapter via getattr(), so it isn't a required Protocol member (avoids breaking existing/external adapters that don't implement it.HttpServer.initialize()sync (TS's is async) since the actual Socket Modeadapter.initialize()does no awaiting.Pure refactor: no behavior change for existing HTTP adapters.