content: Open uploaded files using authed temporary URLs

gnprice · gnprice · commit f9a3a6a063c5 · 2025-10-09T22:23:17.000-07:00
Fixes zulip#1732.
diff --git a/assets/l10n/app_en.arb b/assets/l10n/app_en.arb
@@ -329,6 +329,10 @@
   "@errorCouldNotFetchMessageSource": {
     "description": "Error message when the source of a message could not be fetched."
   },
+  "errorCouldNotAccessUploadedFileTitle": "Could not access uploaded file",
+  "@errorCouldNotAccessUploadedFileTitle": {
+    "description": "Error title on failure in opening a file someone previously uploaded to Zulip"
+  },
   "errorCopyingFailed": "Copying failed",
   "@errorCopyingFailed": {
     "description": "Error message when copying the text of a message to the user's system clipboard failed."
diff --git a/lib/generated/l10n/zulip_localizations.dart b/lib/generated/l10n/zulip_localizations.dart
@@ -593,6 +593,12 @@ abstract class ZulipLocalizations {
   /// **'Could not fetch message source.'**
   String get errorCouldNotFetchMessageSource;
 
+  /// Error title on failure in opening a file someone previously uploaded to Zulip
+  ///
+  /// In en, this message translates to:
+  /// **'Could not access uploaded file'**
+  String get errorCouldNotAccessUploadedFileTitle;
+
   /// Error message when copying the text of a message to the user's system clipboard failed.
   ///
   /// In en, this message translates to:
diff --git a/lib/generated/l10n/zulip_localizations_ar.dart b/lib/generated/l10n/zulip_localizations_ar.dart
@@ -277,6 +277,10 @@ class ZulipLocalizationsAr extends ZulipLocalizations {
   String get errorCouldNotFetchMessageSource =>
       'Could not fetch message source.';
 
+  @override
+  String get errorCouldNotAccessUploadedFileTitle =>
+      'Could not access uploaded file';
+
   @override
   String get errorCopyingFailed => 'Copying failed';
 
diff --git a/lib/generated/l10n/zulip_localizations_de.dart b/lib/generated/l10n/zulip_localizations_de.dart
@@ -285,6 +285,10 @@ class ZulipLocalizationsDe extends ZulipLocalizations {
   String get errorCouldNotFetchMessageSource =>
       'Konnte Nachrichtenquelle nicht abrufen.';
 
+  @override
+  String get errorCouldNotAccessUploadedFileTitle =>
+      'Could not access uploaded file';
+
   @override
   String get errorCopyingFailed => 'Kopieren fehlgeschlagen';
 
diff --git a/lib/generated/l10n/zulip_localizations_en.dart b/lib/generated/l10n/zulip_localizations_en.dart
@@ -277,6 +277,10 @@ class ZulipLocalizationsEn extends ZulipLocalizations {
   String get errorCouldNotFetchMessageSource =>
       'Could not fetch message source.';
 
+  @override
+  String get errorCouldNotAccessUploadedFileTitle =>
+      'Could not access uploaded file';
+
   @override
   String get errorCopyingFailed => 'Copying failed';
 
diff --git a/lib/generated/l10n/zulip_localizations_fr.dart b/lib/generated/l10n/zulip_localizations_fr.dart
@@ -284,6 +284,10 @@ class ZulipLocalizationsFr extends ZulipLocalizations {
   String get errorCouldNotFetchMessageSource =>
       'Impossible d\'atteindre le message source.';
 
+  @override
+  String get errorCouldNotAccessUploadedFileTitle =>
+      'Could not access uploaded file';
+
   @override
   String get errorCopyingFailed => 'Échec de la copie';
 
diff --git a/lib/generated/l10n/zulip_localizations_it.dart b/lib/generated/l10n/zulip_localizations_it.dart
@@ -283,6 +283,10 @@ class ZulipLocalizationsIt extends ZulipLocalizations {
   String get errorCouldNotFetchMessageSource =>
       'Impossibile recuperare l\'origine del messaggio.';
 
+  @override
+  String get errorCouldNotAccessUploadedFileTitle =>
+      'Could not access uploaded file';
+
   @override
   String get errorCopyingFailed => 'Copia non riuscita';
 
diff --git a/lib/generated/l10n/zulip_localizations_ja.dart b/lib/generated/l10n/zulip_localizations_ja.dart
@@ -272,6 +272,10 @@ class ZulipLocalizationsJa extends ZulipLocalizations {
   @override
   String get errorCouldNotFetchMessageSource => 'メッセージのソースを取得できませんでした。';
 
+  @override
+  String get errorCouldNotAccessUploadedFileTitle =>
+      'Could not access uploaded file';
+
   @override
   String get errorCopyingFailed => 'コピーに失敗しました';
 
diff --git a/lib/generated/l10n/zulip_localizations_nb.dart b/lib/generated/l10n/zulip_localizations_nb.dart
@@ -277,6 +277,10 @@ class ZulipLocalizationsNb extends ZulipLocalizations {
   String get errorCouldNotFetchMessageSource =>
       'Could not fetch message source.';
 
+  @override
+  String get errorCouldNotAccessUploadedFileTitle =>
+      'Could not access uploaded file';
+
   @override
   String get errorCopyingFailed => 'Copying failed';
 
diff --git a/lib/generated/l10n/zulip_localizations_pl.dart b/lib/generated/l10n/zulip_localizations_pl.dart
@@ -288,6 +288,10 @@ class ZulipLocalizationsPl extends ZulipLocalizations {
   String get errorCouldNotFetchMessageSource =>
       'Nie można uzyskać źródłowej wiadomości.';
 
+  @override
+  String get errorCouldNotAccessUploadedFileTitle =>
+      'Could not access uploaded file';
+
   @override
   String get errorCopyingFailed => 'Nie udało się skopiować';
 
diff --git a/lib/generated/l10n/zulip_localizations_ru.dart b/lib/generated/l10n/zulip_localizations_ru.dart
@@ -289,6 +289,10 @@ class ZulipLocalizationsRu extends ZulipLocalizations {
   String get errorCouldNotFetchMessageSource =>
       'Не удалось извлечь источник сообщения.';
 
+  @override
+  String get errorCouldNotAccessUploadedFileTitle =>
+      'Could not access uploaded file';
+
   @override
   String get errorCopyingFailed => 'Сбой копирования';
 
diff --git a/lib/generated/l10n/zulip_localizations_sk.dart b/lib/generated/l10n/zulip_localizations_sk.dart
@@ -278,6 +278,10 @@ class ZulipLocalizationsSk extends ZulipLocalizations {
   String get errorCouldNotFetchMessageSource =>
       'Nepodarilo sa nahrať zdroj správy';
 
+  @override
+  String get errorCouldNotAccessUploadedFileTitle =>
+      'Could not access uploaded file';
+
   @override
   String get errorCopyingFailed => 'Kopírovanie zlyhalo';
 
diff --git a/lib/generated/l10n/zulip_localizations_sl.dart b/lib/generated/l10n/zulip_localizations_sl.dart
@@ -286,6 +286,10 @@ class ZulipLocalizationsSl extends ZulipLocalizations {
   String get errorCouldNotFetchMessageSource =>
       'Ni bilo mogoče pridobiti vira sporočila.';
 
+  @override
+  String get errorCouldNotAccessUploadedFileTitle =>
+      'Could not access uploaded file';
+
   @override
   String get errorCopyingFailed => 'Kopiranje ni uspelo';
 
diff --git a/lib/generated/l10n/zulip_localizations_uk.dart b/lib/generated/l10n/zulip_localizations_uk.dart
@@ -287,6 +287,10 @@ class ZulipLocalizationsUk extends ZulipLocalizations {
   String get errorCouldNotFetchMessageSource =>
       'Не вдалося отримати джерело повідомлення.';
 
+  @override
+  String get errorCouldNotAccessUploadedFileTitle =>
+      'Could not access uploaded file';
+
   @override
   String get errorCopyingFailed => 'Помилка копіювання';
 
diff --git a/lib/generated/l10n/zulip_localizations_zh.dart b/lib/generated/l10n/zulip_localizations_zh.dart
@@ -277,6 +277,10 @@ class ZulipLocalizationsZh extends ZulipLocalizations {
   String get errorCouldNotFetchMessageSource =>
       'Could not fetch message source.';
 
+  @override
+  String get errorCouldNotAccessUploadedFileTitle =>
+      'Could not access uploaded file';
+
   @override
   String get errorCopyingFailed => 'Copying failed';
 
diff --git a/lib/model/internal_link.dart b/lib/model/internal_link.dart
@@ -149,9 +149,20 @@ class UserUploadLink extends InternalLink {
 
   /// The remaining path components after the realm ID.
   ///
-  /// This excludes the slash that separates the realm ID from the
+  /// This value excludes the slash that separates the realm ID from the
   /// next component, but includes the rest of the URL path after that slash.
   ///
+  /// This value should only be used as part of a URL, not elsewhere
+  /// in the Zulip API.  Other uses are likely to cause a new version
+  /// of the following issue:
+  ///   https://github.com/zulip/zulip-flutter/issues/1709
+  /// Concretely, this string might differ from the corresponding substring of
+  /// the original URL string (e.g., one found in the HTML of a Zulip message):
+  /// if there were non-ASCII characters in the original string,
+  /// then [Uri.parse] will have converted them to percent-encoded form.
+  /// This is fine as part of a URL, because then the HTTP client would
+  /// otherwise have had to percent-encode those characters anyway.
+  ///
   /// This corresponds to `filename` in the arguments to [getFileTemporaryUrl];
   /// but it's typically several path components,
   /// not just one as that name would suggest.
diff --git a/lib/widgets/actions.dart b/lib/widgets/actions.dart
@@ -7,9 +7,11 @@ import '../api/exception.dart';
 import '../api/model/model.dart';
 import '../api/model/narrow.dart';
 import '../api/route/messages.dart';
+import '../api/route/messages.dart' as messages_api;
 import '../api/route/channels.dart' as channels_api;
 import '../generated/l10n/zulip_localizations.dart';
 import '../model/binding.dart';
+import '../model/internal_link.dart';
 import '../model/narrow.dart';
 import '../model/realm.dart';
 import 'dialog.dart';
@@ -241,6 +243,42 @@ abstract final class ZulipAction {
     return fetchedMessage?.content;
   }
 
+  /// Fetch and parse a URL from [messages_api.getFileTemporaryUrl];
+  /// on failure, show an error dialog and return null.
+  static Future<Uri?> getFileTemporaryUrl(BuildContext context, UserUploadLink link) async {
+    final zulipLocalizations = ZulipLocalizations.of(context);
+
+    String? resultUrl;
+    try {
+      final store = PerAccountStoreWidget.of(context);
+      resultUrl = (await messages_api.getFileTemporaryUrl(store.connection,
+        realmId: link.realmId, filename: link.path)).url;
+    } catch (e) {
+      if (!context.mounted) return null;
+      final errorMessage = switch (e) {
+        ZulipApiException() => e.message,
+        // TODO specific messages for common errors, like network errors
+        //   (support with reusable code)
+        _ => null,
+      };
+      showErrorDialog(context: context,
+        title: zulipLocalizations.errorCouldNotAccessUploadedFileTitle,
+        message: errorMessage);
+      return null;
+    }
+    if (!context.mounted) return null;
+
+    final tempUrl = PerAccountStoreWidget.of(context).tryResolveUrl(resultUrl);
+    if (tempUrl == null) {
+      showErrorDialog(context: context,
+        title: zulipLocalizations.errorCouldNotAccessUploadedFileTitle,
+        message: zulipLocalizations.errorInvalidResponse);
+      return null;
+    }
+
+    return tempUrl;
+  }
+
   static Future<void> subscribeToChannel(BuildContext context, {
     required int channelId,
   }) async {
diff --git a/lib/widgets/content.dart b/lib/widgets/content.dart
@@ -1436,7 +1436,12 @@ void _launchUrl(BuildContext context, String urlString) async {
           narrow: internalLink.narrow,
           initAnchorMessageId: internalLink.nearMessageId)));
 
-    case UserUploadLink(): // TODO(#1732): handle these specially
+    case UserUploadLink():
+      final tempUrl = await ZulipAction.getFileTemporaryUrl(context, internalLink);
+      if (!context.mounted) return null;
+      if (tempUrl == null) return;
+      await PlatformActions.launchUrl(context, tempUrl);
+
     case null:
       await PlatformActions.launchUrl(context, url);
   }
diff --git a/test/widgets/actions_test.dart b/test/widgets/actions_test.dart
@@ -11,6 +11,7 @@ import 'package:zulip/api/model/model.dart';
 import 'package:zulip/api/model/narrow.dart';
 import 'package:zulip/api/route/messages.dart';
 import 'package:zulip/model/binding.dart';
+import 'package:zulip/model/internal_link.dart';
 import 'package:zulip/model/localizations.dart';
 import 'package:zulip/model/narrow.dart';
 import 'package:zulip/model/settings.dart';
@@ -244,6 +245,27 @@ void main() {
         check(await didPass).isFalse();
       });
     });
+
+    group('getFileTemporaryUrl', () {
+      testWidgets('smoke', (tester) async {
+        await prepare(tester);
+        connection.prepare(json: GetFileTemporaryUrlResult(
+          url: '/temp/s3kr1t-auth-token/paper.pdf').toJson());
+        final link = parseInternalLink(
+          store.tryResolveUrl('/user_uploads/123/ab/paper.pdf')!, store);
+
+        final future = ZulipAction.getFileTemporaryUrl(context,
+          link as UserUploadLink);
+        await tester.pump(Duration.zero);
+        check(connection.lastRequest).isA<http.Request>()
+          ..method.equals('GET')
+          ..url.path.equals('/api/v1/user_uploads/123/ab/paper.pdf')
+          ..url.query.isEmpty()
+          ..body.isEmpty();
+        check(await future).equals(
+          store.tryResolveUrl('/temp/s3kr1t-auth-token/paper.pdf')!);
+      });
+    });
   });
 
   group('PlatformActions', () {
diff --git a/test/widgets/content_test.dart b/test/widgets/content_test.dart
@@ -9,6 +9,7 @@ import 'package:url_launcher/url_launcher.dart';
 import 'package:zulip/api/core.dart';
 import 'package:zulip/api/model/initial_snapshot.dart';
 import 'package:zulip/api/model/model.dart';
+import 'package:zulip/api/route/messages.dart';
 import 'package:zulip/model/content.dart';
 import 'package:zulip/model/narrow.dart';
 import 'package:zulip/model/settings.dart';
@@ -21,6 +22,7 @@ import 'package:zulip/widgets/page.dart';
 import 'package:zulip/widgets/store.dart';
 import 'package:zulip/widgets/text.dart';
 
+import '../api/fake_api.dart';
 import '../example_data.dart' as eg;
 import '../flutter_checks.dart';
 import '../model/binding.dart';
@@ -944,6 +946,9 @@ void main() {
   });
 
   group('LinkNode on internal links', () {
+    late PerAccountStore store;
+    late FakeApiConnection connection;
+
     Future<List<Route<dynamic>>> prepare(WidgetTester tester, String html) async {
       final pushedRoutes = <Route<dynamic>>[];
       final testNavObserver = TestNavigatorObserver()
@@ -959,12 +964,13 @@ void main() {
       assert(pushedRoutes.length == 1);
       pushedRoutes.removeLast();
 
-      final store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
+      store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
+      connection = store.connection as FakeApiConnection;
       await store.addStream(eg.stream(name: 'stream'));
       return pushedRoutes;
     }
 
-    testWidgets('valid internal links are navigated to within app', (tester) async {
+    testWidgets('narrow links are navigated to within app', (tester) async {
       final pushedRoutes = await prepare(tester,
         '<p><a href="/#narrow/stream/1-check">stream</a></p>');
 
@@ -976,6 +982,22 @@ void main() {
 
     // TODO(#1570): test links with /near/ go to the specific message
 
+    testWidgets('uploaded-file links are opened with temporary authed URL', (tester) async {
+      final pushedRoutes = await prepare(tester,
+        '<p><a href="/user_uploads/123/ab/paper.pdf">paper.pdf</a></p>');
+
+      final tempUrlString = '/temp/s3kr1t-auth-token/paper.pdf';
+      final expectedUrl = eg.realmUrl.resolve(tempUrlString);
+
+      connection.prepare(json: GetFileTemporaryUrlResult(
+        url: tempUrlString).toJson());
+      await tapText(tester, find.text('paper.pdf'));
+      await tester.pump(Duration.zero);
+      check(testBinding.takeLaunchUrlCalls())
+        .single.equals((url: expectedUrl, mode: LaunchMode.inAppBrowserView));
+      check(pushedRoutes).isEmpty();
+    });
+
     testWidgets('invalid internal links are opened in browser', (tester) async {
       // Link is invalid due to `topic` operator missing an operand.
       final pushedRoutes = await prepare(tester,
@@ -1190,10 +1212,14 @@ void main() {
     }
 
     testWidgets('tapping on audio link opens it in browser', (tester) async {
-      final url = eg.realmUrl.resolve('/user_uploads/2/f2/a_WnijOXIeRnI6OSxo9F6gZM/crab-rave.mp3');
       await prepare(tester, ContentExample.audioInline.html);
+      final store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
+      final connection = store.connection as FakeApiConnection;
 
+      final url = eg.realmUrl.resolve('/temp/token/crab-rave.mp3');
+      connection.prepare(json: GetFileTemporaryUrlResult(url: url.path).toJson());
       await tapText(tester, find.text('crab-rave.mp3'));
+      await tester.pump(Duration.zero);
 
       final expectedLaunchMode = defaultTargetPlatform == TargetPlatform.iOS ?
         LaunchMode.externalApplication : LaunchMode.inAppBrowserView;
