feat(apps): resolve Agentic User file bytes through Graph /shares - #594
feat(apps): resolve Agentic User file bytes through Graph /shares#594Corina (corinagum) wants to merge 17 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Critical and moderate review findings remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Microsoft Graph /shares retrieval for content-URL-only Agentic User attachments while preserving pre-authorized URL downloads.
Changes:
- Adds Graph URL construction, credential selection, cloud routing, and typed errors.
- Supports redirects and preserves expired-URL behavior.
- Adds tests and updates API/example documentation.
File summaries
| File | Summary |
|---|---|
packages/apps/tests/test_optional_graph_dependencies.py |
Tests Graph host derivation and optional dependencies. |
packages/apps/tests/test_graph_share.py |
Tests share URL encoding and endpoint construction. |
packages/apps/tests/test_files_errors.py |
Tests file retrieval error guidance. |
packages/apps/tests/test_files_download_graph.py |
Tests Graph downloads, redirects, credentials, and failures. |
packages/apps/tests/test_files_credential.py |
Tests credential selection. |
packages/apps/tests/test_files_accessor.py |
Tests cleanup, compatibility, and pre-authorized downloads. |
packages/apps/tests/test_files_accessor_agentic.py |
Tests Agentic attachment mapping and scope behavior. |
packages/apps/tests/test_app.py |
Tests Agentic User token acquisition. |
packages/apps/tests/test_app_process.py |
Tests credential propagation. |
packages/apps/tests/test_app_oauth.py |
Updates processor construction tests. |
packages/apps/src/microsoft_teams/apps/utils/graph.py |
Derives Graph host URLs. |
packages/apps/src/microsoft_teams/apps/utils/__init__.py |
Exports Graph URL helpers. |
packages/apps/src/microsoft_teams/apps/routing/activity_context.py |
Passes file credentials into contexts. |
packages/apps/src/microsoft_teams/apps/files/incoming_file.py |
Carries content URLs and credentials. |
packages/apps/src/microsoft_teams/apps/files/graph_share.py |
Builds Graph sharing endpoints. |
packages/apps/src/microsoft_teams/apps/files/files_accessor.py |
Maps content-URL-only attachments. |
packages/apps/src/microsoft_teams/apps/files/errors.py |
Defines retrieval errors and guidance. |
packages/apps/src/microsoft_teams/apps/files/download.py |
Implements Graph and pre-authorized downloads. |
packages/apps/src/microsoft_teams/apps/files/__init__.py |
Exports file error types. |
packages/apps/src/microsoft_teams/apps/files_credential.py |
Selects file credentials. |
packages/apps/src/microsoft_teams/apps/app.py |
Acquires Agentic User Graph tokens. |
packages/apps/src/microsoft_teams/apps/app_process.py |
Wires credentials into activity contexts. |
packages/api/src/microsoft_teams/api/models/file/file_download_info.py |
Clarifies file identifier semantics. |
examples/ai-file-analysis/src/file_card.py |
Clarifies file source labeling. |
examples/ai-file-analysis/README.md |
Documents retrieval routes and expiry behavior. |
Review details
Suppressed comments (8)
examples/ai-file-analysis/README.md:84
- The implementation only admits content-URL-only attachments in
personalscope;groupChatandchannelentries are skipped. This sentence says every missingdownload_urlfollows Graph, which overpromises support for the other scopes. State the personal-scope limitation here.
The route is determined by whether or not the pre-authorized URL exists. If it does, it is used. Otherwise, the Graph path is followed.
packages/apps/src/microsoft_teams/apps/app_process.py:172
- The agentic token callback receives only
activity.recipient.agentic_identity, so it ignores thetenant_idalready extracted from the conversation/channel data. If an inbound identity omitstenantId(the field is optional),TokenManager.get_agentic_user_tokenhas no default tenant and raises, or a configured single-tenant fallback can select the wrong directory. Pass the activity tenant as the fallback for agentic token acquisition, as the app-token callback does here.
get_agentic_graph_token=self.get_agentic_graph_token,
packages/apps/src/microsoft_teams/apps/files/download.py:210
- The Graph path has the same downgrade issue: a trusted HTTPS Graph endpoint can return an HTTP
Location, whichfollow_redirects=Truewill follow and use to transfer the file over cleartext. Apply the same HTTPS-only redirect policy here as on the pre-authorized path.
response = await http.send(request, stream=True, follow_redirects=True)
packages/apps/src/microsoft_teams/apps/files/download.py:114
- This second new
collections.abc.AsyncGenerator[OpenedFileStream]annotation has the same import-time typing error: the send type is missing. UseAsyncGenerator[OpenedFileStream, None]here as well.
) -> AsyncGenerator[OpenedFileStream]:
packages/apps/src/microsoft_teams/apps/files/download.py:185
- This third new
collections.abc.AsyncGenerator[OpenedFileStream]annotation likewise omits the required send type and can prevent the module from importing. UseAsyncGenerator[OpenedFileStream, None].
) -> AsyncGenerator[OpenedFileStream]:
packages/apps/src/microsoft_teams/apps/files/errors.py:19
- This public reason contract says a missing drive item is covered by
access_deniedbecause Graph returns 403, but the implementation maps only 401/403 and the added missing-drive scenario returnsRuntimeErrorfor 404. Either map 404 consistently or document the current behavior so callers are not given an inaccurate exception contract.
- `access_denied`: the identity used was refused by the storage service. Covers an unconsented scope, a file the
identity was never granted, and a drive item that does not exist, which are indistinguishable on the wire: Graph
answers all three with `403`, because telling an unauthorized caller whether a resource exists would disclose it.
packages/apps/src/microsoft_teams/apps/files/errors.py:124
- When token acquisition raises,
download.pypasses that exception string asdetailsforno_graph_credential, but this constructor labels every detail as(service said: ...). Graph was never called in that branch, so AAD/provider failures are misattributed to Graph and make the diagnostic misleading.
if details:
message = f"{message} (service said: {details})"
packages/apps/tests/test_files_accessor.py:350
- This ordinary in-package dependency is imported inside the test body, unlike the module-level imports used by this test and the source package. Move
IncomingFileinto the existing import block so import failures are detected during collection and imports remain consistent.
from microsoft_teams.apps.files import IncomingFile
- Files reviewed: 25/25 changed files
- Comments generated: 7
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Port of the TypeScript change. FileRetrievalError and its reason discriminant are replaced by FileCredentialError and FileAccessError, the activity tenant is threaded through to the Graph token request, redirects are followed manually so a non-https hop is refused, and the actor is no longer defaulted. The manual redirect walk is needed because the client follows redirects internally, which would otherwise carry the Authorization header across a downgrade before anything could inspect it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7d326e8a-aeb4-4481-bc0a-58d2a012584e
What this does
Agentic User file attachments arrive with a
contentUrland nodownloadUrl, so the pre-authorized ("tempauth") URL traditional bots rely on is absent. This resolves those bytes through Microsoft Graph,GET /shares/{u!encoded}/driveItem/content, using the Agentic User's own credential.The route is chosen from the shape of the attachment, not from the identity: a
contentUrlwith nodownloadUrl, at personal scope, resolves viagraphShare. Everything else keeps the existingpreauthUrlpath unchanged.What it deliberately does not do
An expired pre-authorized URL is terminal. It raises
FileUrlExpiredErrorand makes no Graph call.A developer can still call Graph with their own client, and keeps the locator because
contentUrlis public API. The constraint is on what the SDK does automatically.Auditing trap: the agentic token is itself delegated-shaped, carrying
scprather thanroles. Follow the identity, not the word "delegated".Why
.defaultand not a named scopeA by-name request hard-fails on exact-string consent mismatch: a blueprint consented to
Files.ReadWrite.AllreturnsAADSTS65001and no token at all for aFiles.Read.Allrequest. Graph's own/sharesdocs listFiles.ReadWrite.Allas least-privileged and list no read-only permission, so admins following Microsoft's guidance land exactly where a by-name request breaks.The cost is a late, ambiguous 403 when a file permission is missing. We pay that in the SDK instead: the pre-flight rejects a token carrying permissions but none file-capable, which restores an early, legible failure. A
Files.Read.All-then-.defaultladder was rejected, becauseAADSTS65001is also what "no file permission consented at all" produces, so the ladder makes the most common deployment error harder to diagnose, not easier.For security review: under
.defaultthe token carries the blueprint's full consented set, including unrelated scopes. The mitigation is the grant, not the request string: do not consent unrelated scopes on a file-handling blueprint.Testing
Unit and integration coverage is in the diff. All three scenarios were also run live against real Teams, tenant asserted twice per run (browser session state, and the SharePoint host in the bot's own log).
preauthUrl, byte-exact/shares/{u!}/driveItem/content200, byte-exact, driving the shipped helpersFileUrlExpiredError, zero requests tograph.microsoft.comin the entire process logThe third row was checked by grepping the whole bot process log for an absence, not by reading the rendered result, and the expiry is real rather than simulated: the same
UniqueIdreturns 200 early and a hard 401 later. And the pre-authorized URL lifetime is not a constant: three runs bounded the flip at (62.7, 72.4], (62.5, 68.4] and (63.6, 66.4] minutes. Poll rather than picking a wait time.This language
9 commits, 25 files, +1650 / -49.
Follow storage redirects on the pre-authorized download pathcloses a real divergence rather than introducing one.httpxdoes not follow redirects by default, unlike the TS and .NET transports, so a storage 302 surfaced as a bareRuntimeErrorinstead of bytes. Pinned by unit tests and transport parity; no storage 302 occurred during live testing, so it has no live confirmation.