diff --git a/.gitignore b/.gitignore index 18a76602..f5fbb39f 100644 --- a/.gitignore +++ b/.gitignore @@ -494,4 +494,5 @@ $RECYCLE.BIN/ .claude/settings.local.json # Integration test run settings (contain secrets) +test/IntegrationTests/.runsettings/ core/test/IntegrationTests/.runsettings/ diff --git a/test/IntegrationTests/CompatTeamsInfoTests.cs b/test/IntegrationTests/CompatTeamsInfoTests.cs index d6023790..1d5004d3 100644 --- a/test/IntegrationTests/CompatTeamsInfoTests.cs +++ b/test/IntegrationTests/CompatTeamsInfoTests.cs @@ -132,7 +132,7 @@ public async Task GetMembersAsync_ReturnsTeamsChannelAccounts() List members = [.. result]; Assert.NotEmpty(members); - foreach (Microsoft.Bot.Schema.Teams.TeamsChannelAccount m in members) + foreach (Microsoft.Bot.Schema.Teams.TeamsChannelAccount m in members.Take(5)) { _output.WriteLine($"GetMembers: {m.Id} — {m.Name}"); } @@ -152,7 +152,7 @@ public async Task GetPagedMembersAsync_ReturnsPaged() Assert.NotNull(result.Members); Assert.NotEmpty(result.Members); - foreach (Microsoft.Bot.Schema.Teams.TeamsChannelAccount m in result.Members) + foreach (Microsoft.Bot.Schema.Teams.TeamsChannelAccount m in result.Members.Take(5)) { _output.WriteLine($"PagedMember: {m.Id} — {m.Name}"); } @@ -203,7 +203,7 @@ public async Task GetTeamMembersAsync_ReturnsMembers() List members = [.. result]; Assert.NotEmpty(members); - foreach (Microsoft.Bot.Schema.Teams.TeamsChannelAccount m in members) + foreach (Microsoft.Bot.Schema.Teams.TeamsChannelAccount m in members.Take(5)) { _output.WriteLine($"TeamMember: {m.Id} — {m.Name}"); } @@ -223,7 +223,7 @@ public async Task GetPagedTeamMembersAsync_ReturnsPaged() Assert.NotNull(result.Members); Assert.NotEmpty(result.Members); - foreach (Microsoft.Bot.Schema.Teams.TeamsChannelAccount m in result.Members) + foreach (Microsoft.Bot.Schema.Teams.TeamsChannelAccount m in result.Members.Take(5)) { _output.WriteLine($"PagedTeamMember: {m.Id} — {m.Name}"); } diff --git a/test/IntegrationTests/ConversationClientTests.cs b/test/IntegrationTests/ConversationClientTests.cs index 4a355798..8e26de26 100644 --- a/test/IntegrationTests/ConversationClientTests.cs +++ b/test/IntegrationTests/ConversationClientTests.cs @@ -87,7 +87,7 @@ public async Task GetConversationMembers() Assert.NotNull(members); Assert.NotEmpty(members); - foreach (ChannelAccount m in members) + foreach (ChannelAccount m in members.Take(5)) { _output.WriteLine($"Member: {m.Id} — {m.Name}"); } diff --git a/test/IntegrationTests/FilesIntegrationTests.cs b/test/IntegrationTests/FilesIntegrationTests.cs new file mode 100644 index 00000000..e700baf8 --- /dev/null +++ b/test/IntegrationTests/FilesIntegrationTests.cs @@ -0,0 +1,127 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Teams.Apps.Files; +using Microsoft.Teams.Apps.Schema; +using Xunit.Abstractions; + +namespace IntegrationTests; + +/// +/// Integration tests for inbound file handling as shipped today (PR A), covering the two things unit tests +/// structurally cannot reach: that is actually wired into the DI container the app +/// builds, and that its byte-fetch contract holds against a real Microsoft endpoint over a real network. +/// What these do and do not prove. High fidelity for the contract the downloader depends on: +/// an https URL fetched with a plain unauthenticated GET, streamed with ResponseHeadersRead, returning bytes +/// or a clean 401. Low fidelity for provenance: these URLs are not Teams tempauth URLs, because no +/// automated route mints one. **These must never be described as end-to-end file receive.** Manual e2e remains the +/// only evidence for that. +/// Graph is used purely as a conveniently stable first-party HTTP endpoint. No token is sent and no Graph +/// application permission is required, so these run wherever the rest of the suite runs. +/// +public class FilesIntegrationTests : IClassFixture +{ + private readonly IntegrationTestFixture _f; + private readonly ITestOutputHelper _output; + + public FilesIntegrationTests(IntegrationTestFixture fixture, ITestOutputHelper output) + { + _f = fixture; + _f.OutputHelper = output; + _output = output; + } + + // Public, unauthenticated, returns real bytes with a real content-type. Stands in for the "URL that serves the + // file" half of the contract. + private static readonly Uri PublicBytesUrl = new("https://graph.microsoft.com/v1.0/$metadata"); + + // Requires auth, so an unauthenticated GET is a genuine 401 from a live Microsoft service. Stands in for an + // expired `tempauth` URL. + private static readonly Uri UnauthorizedUrl = new("https://graph.microsoft.com/v1.0/me"); + + private FileDownloader Downloader => _f.ServiceProvider.GetRequiredService(); + + // No Timeout: this one performs no I/O, and xUnit only supports Timeout on async tests. + [Fact] + [Trait("Category", "Files")] + public void FileDownloader_ResolvesFromTheContainerTheAppBuilds() + { + // Catches removal of `AddHttpClient()`. TeamsBotApplication falls back to + // FileDownloader.CreateDefault() silently and with no log, so a broken registration degrades to a + // process-wide static HttpClient with no signal and every unit test still passing. + FileDownloader downloader = _f.ServiceProvider.GetRequiredService(); + + Assert.NotNull(downloader); + + // Typed clients are transient, so two resolutions are distinct instances. Asserting that rather than + // singleton-ness documents the registration's actual lifetime instead of guessing at it. + Assert.NotSame(downloader, _f.ServiceProvider.GetRequiredService()); + _output.WriteLine("FileDownloader resolved from DI container"); + } + + [Fact(Timeout = 15000)] + [Trait("Category", "Files")] + public async Task OpenFileStream_FetchesRealBytesOverTheRealNetwork() + { + await using OpenedFileStream stream = await Downloader.OpenFileStreamAsync( + ConversationType.Personal, PublicBytesUrl, contentType: null, priorFetchSucceeded: false, CancellationToken.None); + + // Reading only a prefix is deliberate: it proves HttpCompletionOption.ResponseHeadersRead really streams + // rather than buffering the whole body, which a fake handler cannot demonstrate. + byte[] head = new byte[64]; + int read = await stream.ReadAsync(head, CancellationToken.None); + + Assert.True(read > 0); + Assert.Equal(PublicBytesUrl, stream.SourceUrl); + + // Resolved from the live response, not from the fallback. Real header casing and parameter handling. + Assert.StartsWith("application/xml", stream.ContentType, StringComparison.OrdinalIgnoreCase); + _output.WriteLine($"Fetched {read} bytes, contentType={stream.ContentType}"); + } + + [Fact(Timeout = 15000)] + [Trait("Category", "Files")] + public async Task OpenFileStream_MapsARealUnauthorizedResponseOntoFileUrlExpired() + { + // The failure mode this exists for: if the platform ever answered an expired URL with a redirect to a login + // page, or 200 plus an HTML login body instead of 401, every mocked test would still pass and the downloader + // would hand callers HTML as if it were the file. Only a live call can tell the difference. + FileUrlExpiredException ex = await Assert.ThrowsAsync( + () => Downloader.OpenFileStreamAsync( + ConversationType.Personal, UnauthorizedUrl, contentType: null, priorFetchSucceeded: false, CancellationToken.None)); + + Assert.Equal(FileUrlExpiredReason.FirstFetch, ex.Reason); + _output.WriteLine($"Live 401 mapped to FileUrlExpiredException: {ex.Reason}"); + } + + [Fact(Timeout = 15000)] + [Trait("Category", "Files")] + public async Task OpenFileStream_ReportsRereadWhenAPriorFetchSucceeded() + { + // Same live 401, opposite reason. Pins that the caller-supplied re-fetch state, not the response, chooses + // between FirstFetch and Reread. + FileUrlExpiredException ex = await Assert.ThrowsAsync( + () => Downloader.OpenFileStreamAsync( + ConversationType.Personal, UnauthorizedUrl, contentType: null, priorFetchSucceeded: true, CancellationToken.None)); + + Assert.Equal(FileUrlExpiredReason.Reread, ex.Reason); + } + + [Fact(Timeout = 15000)] + [Trait("Category", "Files")] + public async Task OpenFileStream_RejectsUnsupportedScopesAndNonHttpsUrls_BeforeAnyNetworkCall() + { + // Non-personal scopes have no receive path yet; this pins the shipped behaviour so PR J has to change it + // deliberately rather than by accident. + FileScopeNotSupportedException scopeEx = await Assert.ThrowsAsync( + () => Downloader.OpenFileStreamAsync( + ConversationType.GroupChat, PublicBytesUrl, contentType: null, priorFetchSucceeded: false, CancellationToken.None)); + Assert.Equal(ConversationType.GroupChat, scopeEx.Scope); + + InvalidOperationException httpEx = await Assert.ThrowsAsync( + () => Downloader.OpenFileStreamAsync( + ConversationType.Personal, new Uri("http://graph.microsoft.com/v1.0/$metadata"), contentType: null, priorFetchSucceeded: false, CancellationToken.None)); + Assert.Contains("must use https", httpEx.Message, StringComparison.Ordinal); + } +} diff --git a/test/IntegrationTests/README.md b/test/IntegrationTests/README.md index 965938ae..4bd8c6a8 100644 --- a/test/IntegrationTests/README.md +++ b/test/IntegrationTests/README.md @@ -25,6 +25,15 @@ Tests are configured via `.runsettings` files that set environment variables. Fo Place your `.runsettings` files in the `.runsettings/` directory (gitignored). +> Verify that before you write a secret into it. The ignore rule pointed at a +> pre-migration path until [#659](https://github.com/microsoft/teams.net/pull/659), so on +> older branches this directory is **not** ignored despite what this line says: +> +> ```bash +> git check-ignore -v test/IntegrationTests/.runsettings/botid-prod.runsettings +> # no output means NOT ignored — fix .gitignore first +> ``` + ### Required environment variables ```xml diff --git a/test/Microsoft.Teams.Apps.UnitTests/Files/FileDownloaderTransportTests.cs b/test/Microsoft.Teams.Apps.UnitTests/Files/FileDownloaderTransportTests.cs new file mode 100644 index 00000000..cef9fed6 --- /dev/null +++ b/test/Microsoft.Teams.Apps.UnitTests/Files/FileDownloaderTransportTests.cs @@ -0,0 +1,140 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Net; +using System.Net.Http.Headers; +using System.Text; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Http; +using Microsoft.Teams.Apps.Files; +using Microsoft.Teams.Apps.Schema; + +namespace Microsoft.Teams.Apps.UnitTests.Files; + +/// +/// Transport invariants for the file download request, as shipped today. A download URL embeds its own +/// tempauth credential and points at a third-party storage host, so the request must not carry bot +/// credentials: attaching one can get the request rejected and sends the credential to a host with no business +/// seeing it. +/// Where the invariant actually lives. does not strip anything. It issues a +/// plain GET and inherits whatever its carries, because merges +/// DefaultRequestHeaders into every request. The guarantee therefore rests entirely on the registration: +/// AddHttpClient<FileDownloader>() is deliberately bare, and the AddBotHttpClient call above it, +/// which is what attaches bot authentication, is deliberately not applied to it. +/// So these tests assert the invariant at the level that holds it, the DI container, rather than at a level +/// that would require changing the shipped downloader. teams.ts asserts the equivalent in +/// download.http-client.spec.ts; C# had no equivalent at any level. +/// This matters more once PR J lands, since it adds a Graph path on the same downloader that does +/// authenticate. Once one path attaches a credential and the other must not, "nobody has configured one" stops being +/// a safe place to leave the guarantee. +/// +public class FileDownloaderTransportTests +{ + private const string DownloadUrl = "https://download.example/notes.txt?tempauth=abc"; + + /// Records the outbound request and answers with a canned body, so nothing reaches the network. + private sealed class RecordingHandler : HttpMessageHandler + { + public List Requests { get; } = []; + + public HttpRequestMessage Last => Assert.Single(Requests); + + protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + Requests.Add(request); + return Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new ByteArrayContent(Encoding.UTF8.GetBytes("bytes")), + }); + } + } + + private static IncomingFile PersonalFile(FileDownloader downloader) + => new("notes.txt", ConversationType.Personal, FileSource.BotActivity, downloader) + { + DownloadUrl = new Uri(DownloadUrl), + }; + + /// + /// Builds the container the hosting extensions build, then overrides only the primary handler of the typed + /// client they already registered. Deliberately does not call AddHttpClient<FileDownloader>() + /// itself: doing so would register the downloader independently, so these tests would keep passing even if the + /// hosting extensions stopped registering it at all. Reaching into the existing + /// instead means resolution fails outright if the production registration + /// goes away, which is the regression these tests exist to catch. + /// Every other piece of the shipped registration, including any handler or default header it configures, + /// is left intact, so what the recorder sees is what a real download would send. + /// + private static ServiceProvider BuildAppContainer(RecordingHandler handler, Action? extraConfig = null) + { + ServiceCollection services = new(); + services.AddSingleton(new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["AzureAd:ClientId"] = "files-transport-client-id", + ["AzureAd:TenantId"] = "files-transport-tenant-id", + }) + .Build()); + services.AddLogging(); + services.AddTeamsBotApplication(); + + // The name AddHttpClient() registers its options under. + services.Configure(nameof(FileDownloader), options => + { + options.HttpMessageHandlerBuilderActions.Add(b => b.PrimaryHandler = handler); + extraConfig?.Invoke(options); + }); + + return services.BuildServiceProvider(); + } + + [Fact] + public async Task DiRegisteredDownloader_SendsNoAuthorizationHeader() + { + RecordingHandler handler = new(); + using ServiceProvider provider = BuildAppContainer(handler); + + await PersonalFile(provider.GetRequiredService()).DownloadAsync(); + + // Goes red if anyone attaches bot auth to this registration, adds a credential default header, or applies a + // global auth handler across AddHttpClient registrations. That is the regression worth catching: the code + // comment at the registration site asks for this, and nothing else enforces it. + Assert.False(handler.Last.Headers.Contains("Authorization")); + Assert.Null(handler.Last.Headers.Authorization); + } + + [Fact] + public async Task DiRegisteredDownloader_SendsAPlainGetToTheDownloadUrl() + { + RecordingHandler handler = new(); + using ServiceProvider provider = BuildAppContainer(handler); + + await PersonalFile(provider.GetRequiredService()).DownloadAsync(); + + Assert.Equal(HttpMethod.Get, handler.Last.Method); + Assert.Equal(new Uri(DownloadUrl), handler.Last.RequestUri); + Assert.Null(handler.Last.Content); + } + + /// + /// Characterizes the shipped behaviour that makes the test above the one that matters: the downloader has no + /// defence of its own. This is documentation of a constraint, not an endorsement of it. If C# ever grows an + /// explicit strip, as teams.ts has, this test is the one that should change. + /// + [Fact] + public async Task Download_ForwardsClientDefaults_SoTheTypedClientMustStayCredentialFree() + { + RecordingHandler handler = new(); + HttpClient credentialed = new(handler); + credentialed.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", "token"); + credentialed.DefaultRequestHeaders.UserAgent.ParseAdd("teams.net-test/1.0"); + + await PersonalFile(new FileDownloader(credentialed)).DownloadAsync(); + + // HttpClient merges DefaultRequestHeaders into every request and offers no per-request opt-out, so a + // credential on the client reaches the storage host. Hence the guarantee has to be held at registration. + Assert.True(handler.Last.Headers.Contains("Authorization")); + Assert.Equal(["teams.net-test/1.0"], handler.Last.Headers.GetValues("User-Agent")); + } +} diff --git a/test/Microsoft.Teams.Apps.UnitTests/Files/FilesAccessorWireShapeTests.cs b/test/Microsoft.Teams.Apps.UnitTests/Files/FilesAccessorWireShapeTests.cs new file mode 100644 index 00000000..3e529635 --- /dev/null +++ b/test/Microsoft.Teams.Apps.UnitTests/Files/FilesAccessorWireShapeTests.cs @@ -0,0 +1,140 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Text.Json; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Teams.Apps.Files; +using Microsoft.Teams.Apps.Schema; +using Microsoft.Teams.Core.Schema; + +namespace Microsoft.Teams.Apps.UnitTests.Files; + +/// +/// Mapping tests against the shape Teams actually puts on the wire for a personal-scope file upload, as opposed to +/// the shapes invents. The distinguishing features of the real payload, none of +/// which the hand-written tests exercise, are that it carries two attachments (the file plus an empty +/// text/html sibling), that content has exactly three keys with no etag despite +/// modelling one, and that the activity has no text property at all +/// when a file is attached with no message. +/// The fixture is constructed by allowlist rather than redacted from a capture: it contains only the seven +/// fields the mapper reads, with synthetic values throughout. A real capture carries identity in at least three +/// places that a denylist misses (the download URL's query token, the user's UPN inside both URL paths, and +/// the activity's from/recipient/conversation/channelData blocks), so an allowlist is +/// what fails closed. Do not add fields here that the mapper does not read. +/// +public class FilesAccessorWireShapeTests +{ + private static readonly NullLogger Log = NullLogger.Instance; + + // These tests only exercise attachment mapping, never a download, so the client is never used. + private static readonly FileDownloader Downloader = new(new HttpClient()); + + private const string ExpectedName = "quarterly report.pdf"; + private const string ExpectedUniqueId = "00000000-0000-4000-8000-00000000f11e"; + + // Percent-encoded spaces, exactly as Teams sends them in the browsable OneDrive path. + private const string ExpectedWebUrl = + "https://example.sharepoint.com/personal/synthetic_user_example_com/Documents/Microsoft%20Teams%20Chat%20Files/quarterly%20report.pdf"; + + private const string ExpectedDownloadUrl = + "https://example.sharepoint.com/personal/synthetic_user_example_com/_layouts/15/download.aspx?UniqueId=00000000-0000-4000-8000-00000000f11e&Translate=false&tempauth=synthetic-not-a-real-token&ApiVersion=2.1"; + + /// + /// The real wire shape with synthetic values. Deliberately minimal: `type` plus the seven allowlisted fields. + /// Every omission is load-bearing, so read the assertions before adding anything. + /// + private const string CapturedShapeJson = $$""" + { + "type": "message", + "conversation": { + "conversationType": "personal" + }, + "attachments": [ + { + "contentType": "application/vnd.microsoft.teams.file.download.info", + "contentUrl": "{{ExpectedWebUrl}}", + "name": "{{ExpectedName}}", + "content": { + "downloadUrl": "{{ExpectedDownloadUrl}}", + "uniqueId": "{{ExpectedUniqueId}}", + "fileType": "pdf" + } + }, + { + "contentType": "text/html", + "content": "" + } + ] + } + """; + + // Hydrates the fixture the way a real receive does: raw wire JSON through the inbound deserializer, then + // FromActivity. Nothing here hand-builds a TeamsAttachment, so a regression in either step fails these tests. + private static MessageActivity CapturedActivity() + => MessageActivity.FromActivity(CoreActivity.FromJsonString(CapturedShapeJson)); + + [Fact] + public async Task RealWireShape_YieldsExactlyOneFile_FromATwoAttachmentActivity() + { + MessageActivity activity = CapturedActivity(); + + // Positive control: assert the sibling really arrived, so "one file" below means the mapper skipped it + // rather than the fixture never having carried it. + Assert.Equal(2, activity.Attachments!.Count); + + IList files = await new FilesAccessor(activity, Log, Downloader).ListAsync(); + + IncomingFile file = Assert.Single(files); + Assert.Equal(ExpectedName, file.Name); + Assert.Equal("pdf", file.Extension); + Assert.Equal(ExpectedUniqueId, file.UniqueId); + Assert.Equal(new Uri(ExpectedWebUrl), file.WebUrl); + Assert.Equal(new Uri(ExpectedDownloadUrl), file.DownloadUrl); + Assert.Equal(ConversationType.Personal, file.Scope); + Assert.Equal(FileSource.BotActivity, file.Source); + + // `TeamsAttachment.ContentUrl` is a `Uri`, so the wire value is normalized before anything reads it. Teams + // percent-encodes the spaces in the OneDrive path; pin that they survive the round-trip, since a URL that + // silently decoded them would no longer address the item. + Assert.Contains("Microsoft%20Teams%20Chat%20Files", file.WebUrl!.AbsoluteUri, StringComparison.Ordinal); + Assert.DoesNotContain(' ', file.WebUrl.AbsoluteUri); + } + + [Fact] + public async Task RealWireShape_DoesNotLeakTheEmptyHtmlSibling() + { + MessageActivity activity = CapturedActivity(); + TeamsAttachment htmlSibling = activity.Attachments![1]; + + // The sibling is the thing the content-type guard has to reject: an attachment whose content is an empty + // string rather than an object, so it would not survive coercion into FileDownloadInfo either. + Assert.Equal(new AttachmentContentType("text/html"), htmlSibling.ContentType); + + IncomingFile file = Assert.Single(await new FilesAccessor(activity, Log, Downloader).ListAsync()); + + // Raw identifies which attachment produced the file, so this pins the mapping to index 0 and proves the + // sibling was skipped rather than mapped into some second, malformed handle. + Assert.Same(activity.Attachments[0], file.Raw); + Assert.NotSame(htmlSibling, file.Raw); + } + + [Fact] + public async Task RealWireShape_MapsWithNoEtagAndNoTextProperty() + { + using JsonDocument fixture = JsonDocument.Parse(CapturedShapeJson); + JsonElement content = fixture.RootElement.GetProperty("attachments")[0].GetProperty("content"); + + // Guards the fixture itself: Teams sends exactly downloadUrl, uniqueId and fileType on this path, so the + // mapper is only ever proven against the absent-etag shape if the fixture keeps it absent. + Assert.Equal(3, content.EnumerateObject().Count()); + Assert.False(content.TryGetProperty("etag", out _)); + Assert.False(fixture.RootElement.TryGetProperty("text", out _)); + + MessageActivity activity = CapturedActivity(); + Assert.Null(activity.Text); + + // The payload still maps, which is what would break if Etag were ever made required or if a missing `text` + // started tripping activity hydration. + Assert.Single(await new FilesAccessor(activity, Log, Downloader).ListAsync()); + } +}