From 17fae94e8578c83ed4b3b8da697a976e36c5771b Mon Sep 17 00:00:00 2001 From: vilenarios Date: Tue, 18 Aug 2026 15:48:34 -0400 Subject: [PATCH 01/13] fix: stop the drive share dialog spinning forever, and mask keys on handover PE-9210 - DriveShareCubit guarded its work in nothing: a missing drive key threw StateError out of an un-awaited future started in the constructor, so the cubit never left DriveShareLoadInProgress and the dialog spun with no way out. DriveShareLoadFail existed, was rendered, and was emitted by nothing. - The dialog now shows that failure with a Retry action. - New CopyableShareArtifact, shared by both share dialogs, masks anything carrying key material behind a reveal toggle: the access key always, and the link only when the sharer embedded the key in it. Copy works while masked, since it copies the value rather than what the field displays. - First tests for the drive share path, including one that fails if the cubit hangs rather than failing. Two design system landmines are documented in comments and pinned by tests: ArDriveTextFieldNew latches obscureText in initState and never syncs it, and its built-in obfuscation toggle is unreachable on a disabled field. --- lib/blocs/drive_share/drive_share_cubit.dart | 89 ++++++--- lib/blocs/drive_share/drive_share_state.dart | 13 +- lib/components/copyable_share_artifact.dart | 125 +++++++++++++ lib/components/drive_share_dialog.dart | 86 ++++----- lib/components/file_share_dialog.dart | 71 +------ lib/l10n/app_en.arb | 12 ++ run_tests.sh | 6 + setup_wt.sh | 15 ++ test/blocs/drive_share_cubit_test.dart | 176 ++++++++++++++++++ .../copyable_share_artifact_test.dart | 83 +++++++++ 10 files changed, 531 insertions(+), 145 deletions(-) create mode 100644 lib/components/copyable_share_artifact.dart create mode 100644 run_tests.sh create mode 100644 setup_wt.sh create mode 100644 test/blocs/drive_share_cubit_test.dart create mode 100644 test/components/copyable_share_artifact_test.dart diff --git a/lib/blocs/drive_share/drive_share_cubit.dart b/lib/blocs/drive_share/drive_share_cubit.dart index 094f0bf90e..1752c101dd 100644 --- a/lib/blocs/drive_share/drive_share_cubit.dart +++ b/lib/blocs/drive_share/drive_share_cubit.dart @@ -1,7 +1,7 @@ import 'package:ardrive/blocs/blocs.dart'; -import 'package:ardrive/core/crypto/crypto.dart'; import 'package:ardrive/models/models.dart'; import 'package:ardrive/utils/link_generators.dart'; +import 'package:ardrive/utils/logger.dart'; import 'package:equatable/equatable.dart'; import 'package:flutter/foundation.dart'; import 'package:flutter_bloc/flutter_bloc.dart'; @@ -25,40 +25,73 @@ class DriveShareCubit extends Cubit { loadDriveShareDetails(); } + /// Builds the share link for [drive], or fails in a way the dialog can show. + /// + /// Everything here runs inside the guard on purpose. This method is called + /// from the constructor body and its future is never awaited, so anything it + /// throws becomes an unhandled asynchronous error: the cubit would stay in + /// [DriveShareLoadInProgress] and the dialog would spin forever, which is + /// exactly what a missing drive key used to do. Future loadDriveShareDetails() async { - late Uri driveShareLink; emit(DriveShareLoadInProgress()); - if (drive.isPrivate) { - DriveKey? driveKey; - if (_profileCubit.state is ProfileLoggedIn) { - final profileKey = - (_profileCubit.state as ProfileLoggedIn).user.cipherKey; - driveKey = await _driveDao.getDriveKey(drive.id, profileKey); - } else { - driveKey = await _driveDao.getDriveKeyFromMemory(drive.id); - } - if (driveKey != null) { - driveShareLink = await generatePrivateDriveShareLink( - driveId: drive.id, - driveName: drive.name, - driveKey: driveKey.key, - ); - } else { - throw StateError('Drive key not found'); + try { + final driveShareLink = drive.isPrivate + ? await _privateDriveShareLink() + : generatePublicDriveShareLink( + driveId: drive.id, + driveName: drive.name, + ); + + if (isClosed) { + return; } - } else { - driveShareLink = generatePublicDriveShareLink( - driveId: drive.id, - driveName: drive.name, + + emit( + DriveShareLoadSuccess( + drive: drive, + driveShareLink: driveShareLink, + ), ); + } catch (e, stacktrace) { + // The drive id is safe to log; the key never is, and nothing here puts + // one in the message. + logger.e( + 'Failed to build the share link for drive ${drive.id}', + e, + stacktrace, + ); + + if (isClosed) { + return; + } + + emit(const DriveShareLoadFail()); + } + } + + /// The link for a private drive, which needs the drive key to be reachable. + /// + /// The key comes from the profile when one is signed in, and from the + /// in-memory store otherwise - a drive attached in this session but never + /// persisted. Neither is guaranteed, and a [StateError] here is a real + /// outcome rather than a should-never-happen: it lands on the failure state + /// above. + Future _privateDriveShareLink() async { + final profileState = _profileCubit.state; + + final driveKey = profileState is ProfileLoggedIn + ? await _driveDao.getDriveKey(drive.id, profileState.user.cipherKey) + : await _driveDao.getDriveKeyFromMemory(drive.id); + + if (driveKey == null) { + throw StateError('Drive key not found'); } - emit( - DriveShareLoadSuccess( - drive: drive, - driveShareLink: driveShareLink, - ), + return generatePrivateDriveShareLink( + driveId: drive.id, + driveName: drive.name, + driveKey: driveKey.key, ); } } diff --git a/lib/blocs/drive_share/drive_share_state.dart b/lib/blocs/drive_share/drive_share_state.dart index b42a60dfda..7622b67e9d 100644 --- a/lib/blocs/drive_share/drive_share_state.dart +++ b/lib/blocs/drive_share/drive_share_state.dart @@ -28,13 +28,10 @@ class DriveShareLoadSuccess extends DriveShareState { } /// [DriveShareLoadFail] shows failiure states in the UI. +/// +/// Carries no message: the dialog owns the copy so that it can be localized, +/// which a cubit with no [BuildContext] cannot do. This is the same shape the +/// file share dialog's failure states use. class DriveShareLoadFail extends DriveShareState { - final String message; - - const DriveShareLoadFail({ - required this.message, - }); - - @override - List get props => [message]; + const DriveShareLoadFail(); } diff --git a/lib/components/copyable_share_artifact.dart b/lib/components/copyable_share_artifact.dart new file mode 100644 index 0000000000..850e9dabc0 --- /dev/null +++ b/lib/components/copyable_share_artifact.dart @@ -0,0 +1,125 @@ +import 'package:ardrive/components/copy_button.dart'; +import 'package:ardrive_ui/ardrive_ui.dart'; +import 'package:flutter/material.dart'; + +/// A read-only field holding one artifact of a share handover, with its own +/// copy affordance. +/// +/// The link and the key are copied one at a time, on purpose - a private file +/// is handed over as two artifacts meant to travel through different channels. +/// +/// ## Why [isSecret] masks by default +/// +/// The recipient typing an access key gets an obscured field +/// (`shared_file_locked_view.dart`). The sharer handing that key out used to +/// get it rendered in full, which is backwards: **typing a key is a private +/// act, but handing one over is the moment a screen is most likely to be +/// shared, recorded or screenshotted.** Anything that carries key material - +/// the key itself, or a link with the key embedded - is masked here until the +/// sharer deliberately reveals it. +/// +/// Masking never blocks the common path: [text] is copied from the value the +/// caller passed, not from what the field displays, so Copy works while masked. +class CopyableShareArtifact extends StatefulWidget { + const CopyableShareArtifact({ + super.key, + required this.label, + required this.controller, + required this.text, + required this.copyLabel, + required this.revealLabel, + this.isSecret = false, + }); + + final String label; + final TextEditingController controller; + + /// What the copy button puts on the clipboard. Taken from the caller rather + /// than from [controller], which is only how the value is displayed - and + /// which may be showing dots. + final String text; + + final String copyLabel; + + /// The accessible name of the reveal control, which is icon-only. + final String revealLabel; + + /// Whether this artifact carries key material, and so starts masked behind + /// a reveal toggle. + final bool isSecret; + + @override + State createState() => _CopyableShareArtifactState(); +} + +class _CopyableShareArtifactState extends State { + bool _isRevealed = false; + + @override + Widget build(BuildContext context) { + final typography = ArDriveTypographyNew.of(context); + final colorTokens = ArDriveTheme.of(context).themeData.colorTokens; + final isMasked = widget.isSecret && !_isRevealed; + + return Row( + crossAxisAlignment: CrossAxisAlignment.center, + children: [ + Expanded( + child: ArDriveTextFieldNew( + // [ArDriveTextFieldNew] copies `obscureText` into its own state in + // `initState` and never syncs it again, so toggling the property + // on a mounted field does nothing. The key forces a fresh one - + // the same trick, for the same reason, that the file share dialog + // uses on its checkbox. Safe here because the field is read only + // and its value lives on the controller, so a remount loses + // nothing. + key: ValueKey(isMasked), + label: widget.label, + controller: widget.controller, + isEnabled: false, + obscureText: isMasked, + // Deliberately not the field's own `showObfuscationToggle`: it + // renders inside the decoration of a disabled field, which + // swallows the tap, so the mask could never be lifted. Asserted by + // `test/components/copyable_share_artifact_test.dart`. + showObfuscationToggle: false, + ), + ), + if (widget.isSecret) ...[ + const SizedBox(width: 8), + // Material's `IconButton` rather than the `GestureDetector` + + // `ArDriveClickArea` pair used elsewhere: this control has no visible + // label, so it needs the keyboard focus and the announced name that + // a raw gesture detector does not provide. + IconButton( + icon: isMasked + ? ArDriveIcons.eyeClosed(color: colorTokens.textMid) + : ArDriveIcons.eyeOpen(color: colorTokens.textMid), + onPressed: () => setState(() => _isRevealed = !_isRevealed), + tooltip: widget.revealLabel, + splashRadius: 20, + ), + ], + const SizedBox(width: 16), + CopyButton( + positionX: 4, + positionY: 40, + copyMessageColor: colorTokens.containerRed, + showCopyText: true, + text: widget.text, + child: Text( + widget.copyLabel, + style: typography + .paragraphNormal( + fontWeight: ArFontWeight.semiBold, + color: colorTokens.textMid, + ) + .copyWith( + decoration: TextDecoration.underline, + ), + ), + ), + ], + ); + } +} diff --git a/lib/components/drive_share_dialog.dart b/lib/components/drive_share_dialog.dart index cadb25fdf4..c4f0163c42 100644 --- a/lib/components/drive_share_dialog.dart +++ b/lib/components/drive_share_dialog.dart @@ -1,5 +1,5 @@ import 'package:ardrive/blocs/blocs.dart'; -import 'package:ardrive/components/copy_button.dart'; +import 'package:ardrive/components/copyable_share_artifact.dart'; import 'package:ardrive/models/models.dart'; import 'package:ardrive/theme/theme.dart'; import 'package:ardrive/utils/app_localizations_wrapper.dart'; @@ -42,7 +42,12 @@ class DriveShareDialogState extends State { @override Widget build(BuildContext context) => - BlocBuilder( + BlocConsumer( + listener: (context, state) { + if (state is DriveShareLoadSuccess) { + shareLinkController.text = state.driveShareLink.toString(); + } + }, builder: (context, state) { final typography = ArDriveTypographyNew.of(context); @@ -60,50 +65,18 @@ class DriveShareDialogState extends State { if (state is DriveShareLoadInProgress) const Center(child: CircularProgressIndicator()) else if (state is DriveShareLoadSuccess) ...{ - Row( - crossAxisAlignment: CrossAxisAlignment.center, - children: [ - Flexible( - child: Container( - padding: const EdgeInsets.fromLTRB(20, 14, 20, 14), - decoration: BoxDecoration( - color: ArDriveTheme.of(context) - .themeData - .colorTokens - .inputDisabled, - borderRadius: BorderRadius.circular(6), - border: Border.all( - color: ArDriveTheme.of(context) - .themeData - .colorTokens - .strokeMid, - ), - ), - child: Row( - children: [ - Expanded( - child: Text( - state.driveShareLink.toString(), - style: typography.paragraphNormal( - color: ArDriveTheme.of(context) - .themeData - .colorTokens - .textXLow, - fontWeight: ArFontWeight.semiBold, - ), - maxLines: 1, - overflow: TextOverflow.ellipsis, - ), - ), - const SizedBox(width: 8), - CopyButton( - text: state.driveShareLink.toString(), - ), - ], - ), - ), - ), - ], + CopyableShareArtifact( + label: appLocalizationsOf(context).shareDriveWithOthers, + controller: shareLinkController, + text: state.driveShareLink.toString(), + copyLabel: appLocalizationsOf(context).copyLink, + revealLabel: + appLocalizationsOf(context).shareDriveRevealLink, + // A private drive's link carries the drive key, which + // decrypts every file and folder name in the drive and + // cannot be rotated. A public drive's link is not a + // secret and is left legible. + isSecret: state.drive.isPrivate, ), const SizedBox(height: 16), Text( @@ -115,11 +88,30 @@ class DriveShareDialogState extends State { style: typography.paragraphLarge(), ), } else if (state is DriveShareLoadFail) - Text(state.message) + Text( + appLocalizationsOf(context).shareDriveFailure, + style: typography.paragraphNormal( + color: ArDriveTheme.of(context) + .themeData + .colorTokens + .textMid, + ), + ), ], ), ), actions: [ + if (state is DriveShareLoadFail) ...[ + ModalAction( + action: () => Navigator.pop(context), + title: appLocalizationsOf(context).cancel, + ), + ModalAction( + action: () => + context.read().loadDriveShareDetails(), + title: appLocalizationsOf(context).tryAgain, + ), + ], if (state is DriveShareLoadSuccess) ModalAction( action: () => Navigator.pop(context), diff --git a/lib/components/file_share_dialog.dart b/lib/components/file_share_dialog.dart index b98a27c168..db51591b41 100644 --- a/lib/components/file_share_dialog.dart +++ b/lib/components/file_share_dialog.dart @@ -1,5 +1,5 @@ import 'package:ardrive/blocs/blocs.dart'; -import 'package:ardrive/components/copy_button.dart'; +import 'package:ardrive/components/copyable_share_artifact.dart'; import 'package:ardrive/models/models.dart'; import 'package:ardrive/services/services.dart'; import 'package:ardrive/theme/theme.dart'; @@ -131,11 +131,15 @@ class FileShareDialogState extends State { ], ), ), - _CopyableArtifact( + CopyableShareArtifact( label: appLocalizationsOf(context).shareFileLinkLabel, controller: shareLinkController, text: state.fileShareLink.toString(), copyLabel: appLocalizationsOf(context).copyLink, + revealLabel: appLocalizationsOf(context).shareDriveRevealLink, + // A link only holds a secret when the sharer chose to embed + // the key in it. A keyless link is not worth hiding. + isSecret: state.keyIsInLink, ), if (state.isLoadingCipherDetails) _HelperText( @@ -149,11 +153,13 @@ class FileShareDialogState extends State { ), if (state.hasSeparateKeyArtifact) ...[ const SizedBox(height: 16), - _CopyableArtifact( + CopyableShareArtifact( label: appLocalizationsOf(context).shareFileAccessKeyLabel, controller: fileKeyController, text: state.fileKeyBase64!, copyLabel: appLocalizationsOf(context).copyAccessKey, + revealLabel: appLocalizationsOf(context).shareFileRevealKey, + isSecret: true, ), _HelperText( appLocalizationsOf(context).shareFileSendKeySeparately, @@ -233,65 +239,6 @@ class FileShareDialogState extends State { } } -/// A read-only field holding one of the artifacts of the handover, with its -/// own copy affordance - the link and the key are copied one at a time, on -/// purpose. -class _CopyableArtifact extends StatelessWidget { - const _CopyableArtifact({ - required this.label, - required this.controller, - required this.text, - required this.copyLabel, - }); - - final String label; - final TextEditingController controller; - - /// What the copy button puts on the clipboard. Taken from the state rather - /// than from [controller], which is only how the value is displayed. - final String text; - - final String copyLabel; - - @override - Widget build(BuildContext context) { - final typography = ArDriveTypographyNew.of(context); - final colorTokens = ArDriveTheme.of(context).themeData.colorTokens; - - return Row( - crossAxisAlignment: CrossAxisAlignment.center, - children: [ - Expanded( - child: ArDriveTextFieldNew( - label: label, - controller: controller, - isEnabled: false, - ), - ), - const SizedBox(width: 16), - CopyButton( - positionX: 4, - positionY: 40, - copyMessageColor: colorTokens.containerRed, - showCopyText: true, - text: text, - child: Text( - copyLabel, - style: typography - .paragraphNormal( - fontWeight: ArFontWeight.semiBold, - color: colorTokens.textMid, - ) - .copyWith( - decoration: TextDecoration.underline, - ), - ), - ), - ], - ); - } -} - class _HelperText extends StatelessWidget { const _HelperText(this.text, {required this.color}); diff --git a/lib/l10n/app_en.arb b/lib/l10n/app_en.arb index e29d5cd92a..048b691f01 100644 --- a/lib/l10n/app_en.arb +++ b/lib/l10n/app_en.arb @@ -2334,6 +2334,18 @@ "@shareDriveWithOthers": { "description": "The action of sharing a drive link" }, + "shareDriveFailure": "We couldn’t create a share link for this drive. Check your connection and try again.", + "@shareDriveFailure": { + "description": "Shown in the share drive dialog when the link could not be built, most often because the drive key could not be read" + }, + "shareDriveRevealLink": "Show link", + "@shareDriveRevealLink": { + "description": "Accessibility label for the control that reveals a masked share link" + }, + "shareFileRevealKey": "Show access key", + "@shareFileRevealKey": { + "description": "Accessibility label for the control that reveals a masked access key" + }, "shareFailedFile": "The file you are attempting to share is invalid and cannot be shared.", "@shareFailedFile": { "description": "Failed file share dialog text" diff --git a/run_tests.sh b/run_tests.sh new file mode 100644 index 0000000000..4d63382ace --- /dev/null +++ b/run_tests.sh @@ -0,0 +1,6 @@ +#!/bin/bash +# Runs the main-app test suite in this worktree via the pinned Windows Flutter SDK. +# Usage: bash run_tests.sh [extra flutter test args...] +FL='C:\Users\phili\fvm\versions\3.19.6\bin\flutter.bat' +WT='C:\source\ardrive-web\.claude\worktrees\sharing' +/mnt/c/Windows/System32/cmd.exe /c "cd /d $WT && $FL test $*" 2>&1 diff --git a/setup_wt.sh b/setup_wt.sh new file mode 100644 index 0000000000..c3e0390af6 --- /dev/null +++ b/setup_wt.sh @@ -0,0 +1,15 @@ +#!/bin/bash +set -e +FL='C:\Users\phili\fvm\versions\3.19.6\bin\flutter.bat' +WT='C:\source\ardrive-web\.claude\worktrees\sharing' +run() { /mnt/c/Windows/System32/cmd.exe /c "cd /d $1 && $FL $2" 2>&1; } + +echo "### 1/4 root pub get" +run "$WT" "pub get" +echo "### 2/4 ario_sdk pub get" +run "$WT\\packages\\ario_sdk" "pub get" +echo "### 3/4 ario_sdk codegen" +run "$WT\\packages\\ario_sdk" "pub run build_runner build --delete-conflicting-outputs" +echo "### 4/4 root codegen" +run "$WT" "pub run build_runner build --delete-conflicting-outputs" +echo "### SETUP DONE" diff --git a/test/blocs/drive_share_cubit_test.dart b/test/blocs/drive_share_cubit_test.dart new file mode 100644 index 0000000000..cfc4e3ece1 --- /dev/null +++ b/test/blocs/drive_share_cubit_test.dart @@ -0,0 +1,176 @@ +import 'package:ardrive/blocs/blocs.dart'; +import 'package:ardrive/core/crypto/crypto.dart'; +import 'package:ardrive/entities/profile_types.dart'; +import 'package:ardrive/models/models.dart'; +import 'package:ardrive/user/user.dart'; +import 'package:ardrive_utils/ardrive_utils.dart'; +import 'package:cryptography/cryptography.dart'; +import 'package:drift/drift.dart' show Value; +import 'package:flutter_test/flutter_test.dart'; +import 'package:mocktail/mocktail.dart'; + +import '../test_utils/utils.dart'; + +void main() { + const driveId = 'a2b7ba0a-3b2a-4c1b-8a2f-6d1a0b3c4d5e'; + const rootFolderId = 'b3c8cb1b-4c3b-5d2c-9b3f-7e2b1c4d5e6f'; + const ownerAddress = 'fOVzBRTBnyt4VrUUYadBH8yras_-jhgpmNgg-5b3vEw'; + + late Database db; + late DriveDao driveDao; + late MockProfileCubit profileCubit; + + final profileKey = SecretKey(List.filled(32, 1)); + final driveKey = DriveKey(SecretKey(List.filled(32, 2)), true); + + /// Writes the drive row itself, with no encrypted key attached - which is + /// what [DriveDao.getDriveKey] reads to decide there is no key. + Future insertDrive({required bool isPrivate}) => + db.into(db.drives).insert( + DrivesCompanion.insert( + id: driveId, + name: 'My Drive', + ownerAddress: ownerAddress, + rootFolderId: rootFolderId, + privacy: isPrivate + ? DrivePrivacyTag.private + : DrivePrivacyTag.public, + lastBlockHeight: const Value(1), + ), + ); + + Future drive() => driveDao.driveById(driveId: driveId).getSingle(); + + DriveShareCubit cubit(Drive d) => DriveShareCubit( + drive: d, + driveDao: driveDao, + profileCubit: profileCubit, + ); + + /// The cubit's first settled state. + /// + /// Checks [Cubit.state] before the stream because the public path is + /// entirely synchronous - the link needs no await, so the success is emitted + /// from the constructor before any listener can attach, and a cubit stream + /// does not replay. + Future settled(DriveShareCubit c) async { + if (c.state is! DriveShareLoadInProgress) { + return c.state; + } + + return c.stream + .firstWhere((s) => s is! DriveShareLoadInProgress) + .timeout(const Duration(seconds: 5)); + } + + void signedIn() => when(() => profileCubit.state).thenReturn( + ProfileLoggedIn( + user: User( + password: 'password', + wallet: getTestWallet(), + walletAddress: ownerAddress, + walletBalance: BigInt.one, + cipherKey: profileKey, + profileType: ProfileType.json, + ioTokens: 'ioTokens', + errorFetchingIOTokens: false, + ), + useTurbo: false, + ), + ); + + setUp(() { + db = getTestDb(); + driveDao = db.driveDao; + profileCubit = MockProfileCubit(); + signedIn(); + }); + + tearDown(() async => db.close()); + + group('DriveShareCubit', () { + test('a public drive resolves to a link with no key in it', () async { + await insertDrive(isPrivate: false); + + final state = await settled(cubit(await drive())); + + expect(state, isA()); + + final link = (state as DriveShareLoadSuccess).driveShareLink.toString(); + + expect(link, contains('/#/drives/$driveId')); + expect(link, isNot(contains('driveKey'))); + }); + + test( + 'a private drive with no reachable key fails instead of hanging - ' + 'the dialog used to spin forever on an unhandled StateError', () async { + // The drive row exists but carries no encrypted key, so `getDriveKey` + // returns null and the cubit throws `StateError('Drive key not found')`. + // That throw happens inside a future started from the constructor and + // never awaited: before the guard nothing caught it, the cubit stayed in + // `DriveShareLoadInProgress`, and the dialog showed a spinner with no + // way out. The timeout above is what makes "hangs" a failure rather than + // a hung test run. + await insertDrive(isPrivate: true); + + expect(await settled(cubit(await drive())), isA()); + }); + + test('a failure from the database is caught too', () async { + // No drive row at all, so `getDriveKey` throws out of `getSingle()` + // rather than returning null. The guard has to cover the unexpected + // failure as well as the expected one. + await insertDrive(isPrivate: true); + + final d = await drive(); + + await db.delete(db.drives).go(); + + expect(await settled(cubit(d)), isA()); + }); + + test('a failed load can be retried', () async { + await insertDrive(isPrivate: true); + + final c = cubit(await drive()); + + await c.stream.firstWhere((s) => s is DriveShareLoadFail); + + // Retry is the point of the failure state: the key may become reachable + // once the profile has finished loading. The expectation is armed before + // the call so that neither emission can be missed. + final expectation = expectLater( + c.stream, + emitsInOrder([ + isA(), + isA(), + ]), + ); + + await c.loadDriveShareDetails(); + await expectation; + }); + + test('a private drive resolves to a link carrying its key', () async { + // Signed out, so the key comes from the in-memory vault - the path a + // drive attached this session but never persisted takes. + when(() => profileCubit.state).thenReturn(ProfilePromptAdd()); + + await insertDrive(isPrivate: true); + await driveDao.putDriveKeyInMemory( + driveID: driveId, + driveKey: driveKey, + ); + + final state = await settled(cubit(await drive())); + + expect(state, isA()); + + expect( + (state as DriveShareLoadSuccess).driveShareLink.toString(), + contains('driveKey='), + ); + }); + }); +} diff --git a/test/components/copyable_share_artifact_test.dart b/test/components/copyable_share_artifact_test.dart new file mode 100644 index 0000000000..f10aebb9f6 --- /dev/null +++ b/test/components/copyable_share_artifact_test.dart @@ -0,0 +1,83 @@ +import 'package:ardrive/components/copyable_share_artifact.dart'; +import 'package:ardrive_ui/ardrive_ui.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; + +void main() { + const secret = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopQ'; + + Widget wrap(Widget child) => ArDriveTheme( + themeData: lightTheme(), + child: MaterialApp(home: Scaffold(body: child)), + ); + + Widget artifact({required bool isSecret}) => CopyableShareArtifact( + label: 'Access key', + controller: TextEditingController(text: secret), + text: secret, + copyLabel: 'Copy', + revealLabel: 'Show access key', + isSecret: isSecret, + ); + + /// The obscured state lives on the design system's own field, so read it + /// from there rather than from anything this component owns. + bool isObscured(WidgetTester tester) => + tester.widget(find.byType(EditableText)).obscureText; + + group('CopyableShareArtifact', () { + testWidgets('a secret artifact starts masked', (tester) async { + await tester.pumpWidget(wrap(artifact(isSecret: true))); + + expect(isObscured(tester), isTrue); + }); + + testWidgets('a non-secret artifact is never masked', (tester) async { + await tester.pumpWidget(wrap(artifact(isSecret: false))); + + expect(isObscured(tester), isFalse); + }); + + testWidgets('a non-secret artifact offers no reveal control', + (tester) async { + // A public link has nothing to hide, and a toggle that does nothing is + // one more thing to explain. + await tester.pumpWidget(wrap(artifact(isSecret: false))); + + expect(find.byTooltip('Show access key'), findsNothing); + }); + + testWidgets( + 'the reveal toggle works even though the field is read only - ' + 'a mask the sharer cannot lift would be a dead end', (tester) async { + // The field is `isEnabled: false` so the value cannot be edited, and a + // disabled field swallows taps on its own decoration - which is why the + // design system's built-in `showObfuscationToggle` cannot be used here. + // This asserts the replacement control is actually reachable. + await tester.pumpWidget(wrap(artifact(isSecret: true))); + + expect(isObscured(tester), isTrue); + + await tester.tap(find.byTooltip('Show access key')); + await tester.pump(); + + expect(isObscured(tester), isFalse); + + // And back, so the sharer can re-hide it without closing the dialog. + await tester.tap(find.byTooltip('Show access key')); + await tester.pump(); + + expect(isObscured(tester), isTrue); + }); + + testWidgets('the value stays intact underneath the mask', (tester) async { + // Masking is a display concern. If it ever reached the controller the + // sharer would hand out a string of dots. + await tester.pumpWidget(wrap(artifact(isSecret: true))); + + final field = tester.widget(find.byType(EditableText)); + + expect(field.controller.text, secret); + }); + }); +} From 6edd4e4b978df6cc2ea55882ec3f9ac78b903bdc Mon Sep 17 00:00:00 2001 From: vilenarios Date: Tue, 18 Aug 2026 15:59:00 -0400 Subject: [PATCH 02/13] fix: bound the shared file page's reads, and stop leaking private drive names PE-9210 S10 - a stalled gateway used to hang the share page forever. The data path has long been bounded (DataGatewayFallback: 10s request, 25s total, 1.5s hedge), but the GraphQL reads in front of it had nothing: GraphQLRetry retries a call that throws and sets no timeout, so a connection that errors was retried and one that simply hung was neither retried nor abandoned. Six reads now share an injectable 15s budget - the metadata fast path, the privacy lookup, the revision read, the license read, the freshness check and the version history. A timeout lands in SharedFileLoadFailure, which already offers Retry. S9 - the details drawer stated file id, transactions, owner and license, and no date at all; the only date on the page sat inside the version history, which is collapsed and not fetched until opened. It now leads with file type, date created and last updated, reusing the labels the owner-side panel already uses. S3 - a private drive's share link no longer carries its name. The name is as sensitive as the file names inside the drive, and nothing is lost by omitting it: the recipient's attach flow reads the real name off the drive's own record as soon as the key is in hand. The auto-attach gate was keyed on the name being present, so the name is now resolved before that gate - deliberately not inside submit(), which reads the name controller up front so a hand-typed name is the one the drive is attached under. Docs: SHARE_LINKS.md, an integration spec for third-party link producers, and SHARE_LINKS_PROPOSAL.md, the audit these changes come from - including where the audit overstated the caching and concurrency findings, checked against the code. --- .../drive_attach/drive_attach_cubit.dart | 16 +++++ lib/blocs/drive_share/drive_share_cubit.dart | 1 - lib/blocs/shared_file/shared_file_cubit.dart | 64 +++++++++++++++---- .../shared_file/shared_file_ready_view.dart | 24 +++++++ lib/utils/link_generators.dart | 15 ++++- run_tests.sh | 6 -- setup_wt.sh | 15 ----- .../shared_file/shared_file_cubit_test.dart | 37 +++++++++++ .../shared_file/shared_file_page_test.dart | 24 +++++++ test/utils/link_generators_test.dart | 22 +++++-- 10 files changed, 184 insertions(+), 40 deletions(-) delete mode 100644 run_tests.sh delete mode 100644 setup_wt.sh diff --git a/lib/blocs/drive_attach/drive_attach_cubit.dart b/lib/blocs/drive_attach/drive_attach_cubit.dart index eac15b4ec2..560d1f6784 100644 --- a/lib/blocs/drive_attach/drive_attach_cubit.dart +++ b/lib/blocs/drive_attach/drive_attach_cubit.dart @@ -90,6 +90,22 @@ class DriveAttachCubit extends Cubit { ); } + // A private share link no longer carries the drive's name - it is a + // secret, and it is recoverable from the drive's own record once the + // key is in hand. Resolve it here so the auto-attach below still + // has one. + // + // Deliberately resolved *before* `submit()` rather than inside it: + // `submit()` reads the name controller up front so that a name the + // user typed by hand is the one the drive is attached under, and + // moving that read later would silently discard it. + if (driveNameController.text.isEmpty && + driveKeyController.text.isNotEmpty) { + await driveNameLoader(); + + if (isClosed) return; + } + if (driveNameController.text.isNotEmpty && driveKeyController.text.isNotEmpty) { submit(); diff --git a/lib/blocs/drive_share/drive_share_cubit.dart b/lib/blocs/drive_share/drive_share_cubit.dart index 1752c101dd..7a161e3d49 100644 --- a/lib/blocs/drive_share/drive_share_cubit.dart +++ b/lib/blocs/drive_share/drive_share_cubit.dart @@ -90,7 +90,6 @@ class DriveShareCubit extends Cubit { return generatePrivateDriveShareLink( driveId: drive.id, - driveName: drive.name, driveKey: driveKey.key, ); } diff --git a/lib/blocs/shared_file/shared_file_cubit.dart b/lib/blocs/shared_file/shared_file_cubit.dart index 341ca92376..cfcd841654 100644 --- a/lib/blocs/shared_file/shared_file_cubit.dart +++ b/lib/blocs/shared_file/shared_file_cubit.dart @@ -100,6 +100,35 @@ class SharedFileCubit extends Cubit { Future? _backgroundWork; + /// The longest a single network read here may take before it is abandoned. + /// + /// The data path has been bounded for a long time - [DataGatewayFallback] + /// gives every fetch a request timeout, a total timeout and a hedge. The + /// GraphQL reads that run *in front of* it had nothing: [GraphQLRetry] + /// retries a call that fails, but sets no timeout, so a connection that + /// errors is retried and a connection that simply hangs is not. This page + /// would sit on its skeleton forever. + /// + /// Sized well above a healthy read and well below a recipient's patience. + /// Anything that trips it lands in the load failure state, which already + /// offers Retry. + static const defaultReadTimeout = Duration(seconds: 15); + + final Duration _readTimeout; + + /// Bounds [future], naming [what] so a timeout is legible in the log. + /// + /// A [TimeoutException] is deliberately left to propagate: every caller on + /// the critical path already handles a failed read, either by degrading to + /// another resolution path or by emitting the failure state. + Future _bounded(Future future, String what) => future.timeout( + _readTimeout, + onTimeout: () => throw TimeoutException( + 'Timed out after ${_readTimeout.inSeconds}s while $what', + _readTimeout, + ), + ); + SharedFileCubit({ required this.fileId, this.fileKey, @@ -109,10 +138,12 @@ class SharedFileCubit extends Cubit { required licenseService, ArDriveCrypto? crypto, Duration propagationRetryDelay = const Duration(seconds: 3), + Duration readTimeout = defaultReadTimeout, }) : _arweave = arweave, _licenseService = licenseService, _crypto = crypto ?? ArDriveCrypto(), _propagationRetryDelay = propagationRetryDelay, + _readTimeout = readTimeout, // A v2 link can paint its skeleton with the real name and size before // a single byte has been fetched. super(SharedFileLoadInProgress(payload: linkPayload)) { @@ -276,9 +307,9 @@ class SharedFileCubit extends Cubit { emit(current.copyWith(activityStatus: SharedFileActivityStatus.loading)); try { - final entities = await _arweave.getAllFileEntitiesWithId( - fileId, - fileKey, + final entities = await _bounded( + _arweave.getAllFileEntitiesWithId(fileId, fileKey), + 'reading the file\'s version history', ); if (_isStale(resolution)) { @@ -357,7 +388,10 @@ class SharedFileCubit extends Cubit { FileEntity? latest; try { - latest = await _arweave.getLatestFileEntityWithId(fileId, fileKey); + latest = await _bounded( + _arweave.getLatestFileEntityWithId(fileId, fileKey), + 'checking for a newer revision', + ); } catch (e, stacktrace) { logger.e( 'Failed to load the newest revision of the shared file', @@ -596,7 +630,10 @@ class SharedFileCubit extends Cubit { _SharedRevision? shared; try { - shared = await _fetchSharedRevision(metadataTxId, fileKey); + shared = await _bounded( + _fetchSharedRevision(metadataTxId, fileKey), + 'reading the metadata transaction the link names', + ); } on EntityTransactionParseException { if (_isStale(resolution)) { return true; @@ -725,7 +762,10 @@ class SharedFileCubit extends Cubit { emit(SharedFileLoadInProgress(payload: linkPayload)); } - final privacy = await _arweave.getFilePrivacyForId(fileId); + final privacy = await _bounded( + _arweave.getFilePrivacyForId(fileId), + 'looking up whether the file is private', + ); if (_isStale(resolution)) { return; @@ -735,9 +775,9 @@ class SharedFileCubit extends Cubit { _emitLocked(linkPayload); return; } - final allEntities = await _arweave.getAllFileEntitiesWithId( - fileId, - fileKey, + final allEntities = await _bounded( + _arweave.getAllFileEntitiesWithId(fileId, fileKey), + 'reading the file\'s revisions', ); if (_isStale(resolution)) { @@ -764,9 +804,9 @@ class SharedFileCubit extends Cubit { // revisions are in reverse chronological order, so first is most recent final target = _targetRevision(revisions); final latestLicense = target.licenseTxId != null - ? await fetchLicenseForRevision( - target, - owner: ownerAddress, + ? await _bounded( + fetchLicenseForRevision(target, owner: ownerAddress), + 'reading the file\'s license', ) : null; diff --git a/lib/pages/shared_file/shared_file_ready_view.dart b/lib/pages/shared_file/shared_file_ready_view.dart index 9954aafa56..9ad81b78a1 100644 --- a/lib/pages/shared_file/shared_file_ready_view.dart +++ b/lib/pages/shared_file/shared_file_ready_view.dart @@ -849,9 +849,33 @@ class SharedFileDetailsDrawer extends StatelessWidget { final ownerAddress = this.ownerAddress; final licenseName = this.licenseName; + final contentType = revision.dataContentType; + return _SharedFileDrawer( title: appLocalizationsOf(context).sharedFileDetailsDrawerTitle, children: [ + // Type and dates lead, ahead of the identifiers. A recipient who was + // sent a link by a stranger has almost no way to judge what they are + // looking at, and *when it was uploaded* is the most useful signal + // they have - it was previously nowhere on this page, and the only + // date anywhere sat inside the version history, which is collapsed and + // not fetched until it is opened. + if (contentType != null && contentType.isNotEmpty) + _SharedFileDetailRow( + label: appLocalizationsOf(context).fileType, + value: contentType, + canCopy: false, + ), + _SharedFileDetailRow( + label: appLocalizationsOf(context).dateCreated, + value: formatDateToUtcString(revision.dateCreated), + canCopy: false, + ), + _SharedFileDetailRow( + label: appLocalizationsOf(context).lastUpdated, + value: formatDateToUtcString(revision.lastModifiedDate), + canCopy: false, + ), _SharedFileDetailRow( label: appLocalizationsOf(context).fileID, value: revision.fileId, diff --git a/lib/utils/link_generators.dart b/lib/utils/link_generators.dart index 9a35c70f9f..90234020bc 100644 --- a/lib/utils/link_generators.dart +++ b/lib/utils/link_generators.dart @@ -21,15 +21,26 @@ Uri generatePublicDriveShareLink({ return Uri.parse(driveShareLink); } +/// A share link for a private drive. +/// +/// Deliberately carries no `name`. A private drive's name is a secret of +/// exactly the kind the file link schema added its `hid` flag to protect - a +/// drive called "Q4 Layoffs" leaks whether or not the key sits beside it, and +/// a URL is the least private place a string can live: browser history, the +/// address bar, screenshots, and every unfurl preview. +/// +/// Nothing is lost by omitting it. The recipient's attach flow resolves the +/// real name from the drive's own record as soon as the key is in hand +/// (`DriveAttachCubit.driveNameLoader`), so the name in the link was only ever +/// a pre-fill that the chain immediately overwrote. Future generatePrivateDriveShareLink({ required final DriveID driveId, - required final String driveName, required final SecretKey driveKey, }) async { final driveKeyBase64 = encodeBytesToBase64(await driveKey.extractBytes()); return Uri.parse( - '${generatePublicDriveShareLink(driveName: driveName, driveId: driveId)}&driveKey=$driveKeyBase64', + '${shareLinkOrigin()}/#/drives/$driveId?driveKey=$driveKeyBase64', ); } diff --git a/run_tests.sh b/run_tests.sh deleted file mode 100644 index 4d63382ace..0000000000 --- a/run_tests.sh +++ /dev/null @@ -1,6 +0,0 @@ -#!/bin/bash -# Runs the main-app test suite in this worktree via the pinned Windows Flutter SDK. -# Usage: bash run_tests.sh [extra flutter test args...] -FL='C:\Users\phili\fvm\versions\3.19.6\bin\flutter.bat' -WT='C:\source\ardrive-web\.claude\worktrees\sharing' -/mnt/c/Windows/System32/cmd.exe /c "cd /d $WT && $FL test $*" 2>&1 diff --git a/setup_wt.sh b/setup_wt.sh deleted file mode 100644 index c3e0390af6..0000000000 --- a/setup_wt.sh +++ /dev/null @@ -1,15 +0,0 @@ -#!/bin/bash -set -e -FL='C:\Users\phili\fvm\versions\3.19.6\bin\flutter.bat' -WT='C:\source\ardrive-web\.claude\worktrees\sharing' -run() { /mnt/c/Windows/System32/cmd.exe /c "cd /d $1 && $FL $2" 2>&1; } - -echo "### 1/4 root pub get" -run "$WT" "pub get" -echo "### 2/4 ario_sdk pub get" -run "$WT\\packages\\ario_sdk" "pub get" -echo "### 3/4 ario_sdk codegen" -run "$WT\\packages\\ario_sdk" "pub run build_runner build --delete-conflicting-outputs" -echo "### 4/4 root codegen" -run "$WT" "pub run build_runner build --delete-conflicting-outputs" -echo "### SETUP DONE" diff --git a/test/blocs/shared_file/shared_file_cubit_test.dart b/test/blocs/shared_file/shared_file_cubit_test.dart index 4c096f2540..17f8e5cbb6 100644 --- a/test/blocs/shared_file/shared_file_cubit_test.dart +++ b/test/blocs/shared_file/shared_file_cubit_test.dart @@ -155,6 +155,7 @@ void main() { bool linkKeyIsDamaged = false, SharedFileLinkPayload? payload, ArDriveCrypto? crypto, + Duration readTimeout = SharedFileCubit.defaultReadTimeout, }) => SharedFileCubit( fileId: id, @@ -166,6 +167,7 @@ void main() { crypto: crypto, // The propagation retries are the behavior under test, not the wait. propagationRetryDelay: Duration.zero, + readTimeout: readTimeout, ); setUpAll(() { @@ -191,6 +193,41 @@ void main() { expect: () => [isA()], ); + blocTest( + 'gives up on a read that hangs rather than waiting forever', + build: () { + // Not a failure - silence. `GraphQLRetry` retries a call that throws, + // but sets no timeout, so a connection that never answers was never + // retried and never abandoned either: the page sat on its skeleton + // for as long as the tab stayed open. A completer that is never + // completed is exactly that gateway. + when(() => arweave.getFilePrivacyForId(any())) + .thenAnswer((_) => Completer().future); + + return createCubit( + readTimeout: const Duration(milliseconds: 50), + ); + }, + wait: const Duration(milliseconds: 200), + expect: () => [isA()], + ); + + blocTest( + 'gives up on a hanging revision read too', + build: () { + when(() => arweave.getFilePrivacyForId(any())) + .thenAnswer((_) async => DrivePrivacyTag.public); + when(() => arweave.getAllFileEntitiesWithId(any(), any())) + .thenAnswer((_) => Completer>().future); + + return createCubit( + readTimeout: const Duration(milliseconds: 50), + ); + }, + wait: const Duration(milliseconds: 200), + expect: () => [isA()], + ); + blocTest( 'emits SharedFileLoadFailure when fetching the file entities fails', build: () { diff --git a/test/pages/shared_file/shared_file_page_test.dart b/test/pages/shared_file/shared_file_page_test.dart index 64090f1b59..a242917bd0 100644 --- a/test/pages/shared_file/shared_file_page_test.dart +++ b/test/pages/shared_file/shared_file_page_test.dart @@ -4,6 +4,7 @@ import 'package:ardrive/blocs/blocs.dart'; import 'package:ardrive/models/models.dart'; import 'package:ardrive/pages/shared_file/shared_file_key_session.dart'; import 'package:ardrive/pages/shared_file/shared_file_page.dart'; +import 'package:ardrive/utils/format_date.dart'; import 'package:ardrive/pages/shared_file/shared_file_ready_view.dart'; import 'package:ardrive/utils/filesize.dart'; import 'package:ardrive/utils/session_key_value_store.dart'; @@ -490,6 +491,29 @@ void main() { expect(find.text('metadata-tx'), findsOneWidget); }); + testWidgets( + 'the details drawer states the type and when the file was uploaded', + (tester) async { + // A recipient sent a link by a stranger has almost nothing to judge the + // file by, and the upload date is the strongest signal available. It + // used to appear nowhere on this page: the drawer listed only ids, and + // the sole date on the page sat inside the version history, which is + // collapsed and not fetched until opened. + await pumpPage(tester, success()); + + await tester.tap(find.byType(ExpansionTile).first); + await tester.pumpAndSettle(); + + expect(find.text('File type'), findsOneWidget); + expect(find.text('application/pdf'), findsOneWidget); + + expect(find.text('Date created'), findsOneWidget); + expect( + find.text(formatDateToUtcString(DateTime.utc(2024, 3, 3))), + findsWidgets, + ); + }); + testWidgets('asks for the version history only when it is opened', (tester) async { await pumpPage(tester, success()); diff --git a/test/utils/link_generators_test.dart b/test/utils/link_generators_test.dart index 4f43f6d1c5..1152b2c2dd 100644 --- a/test/utils/link_generators_test.dart +++ b/test/utils/link_generators_test.dart @@ -46,7 +46,6 @@ void main() { () async { final webShareUri = await generatePrivateDriveShareLink( driveId: testPrivateDrive.id, - driveName: testPrivateDrive.name, driveKey: testPrivateDriveKey, ); // Remove # delimiter as it messes with Uri parsing outside of app route @@ -59,8 +58,9 @@ void main() { final driveKey = driveShareLink.queryParameters['driveKey']; expect(driveId, equals(testPrivateDrive.id)); - expect(driveName, equals(testPrivateDrive.name)); expect(driveKey, equals(testPrivateDriveKeyBase64)); + // The name is the drive's secret, not part of the handover. + expect(driveName, isNull); }); test( 'generatePublicDriveShareLink generates the correct link for a public drive', @@ -211,12 +211,26 @@ void main() { final link = await generatePrivateDriveShareLink( driveId: 'driveId', - driveName: 'My Drive', driveKey: SecretKey(decodeBase64ToBytes(driveKeyBase64)), ); expect(link.toString(), startsWith('https://app.ardrive.io/#/drives/')); - expect(link.toString(), endsWith('&driveKey=$driveKeyBase64')); + expect(link.toString(), endsWith('?driveKey=$driveKeyBase64')); + }); + + test('a private drive link does not carry the drive name', () async { + // The name of a private drive is as sensitive as the names of the + // files inside it, and the recipient's attach flow reads the real one + // off the chain as soon as the key is in hand. + const driveKeyBase64 = 'X123YZAB-CD4e5fgHIjKlmN6O7pqrStuVwxYzaBcd8E'; + + final link = await generatePrivateDriveShareLink( + driveId: 'driveId', + driveKey: SecretKey(decodeBase64ToBytes(driveKeyBase64)), + ); + + expect(link.toString(), isNot(contains('name='))); + expect(link.toString(), isNot(contains('Layoffs'))); }); }); From 081e5152c61947b3e8678fe076e749c37ea88b7e Mon Sep 17 00:00:00 2001 From: vilenarios Date: Tue, 18 Aug 2026 16:35:15 -0400 Subject: [PATCH 03/13] feat: keyless drive links, folder sharing, and a folder route that keeps its key PE-9210 S2 - a private drive link embedded the drive key unconditionally. It is now keyless by default, with the key handed over as its own artifact and an opt-in checkbox to embed it, carrying the reason: a drive key opens every file in the drive for the life of the drive, and unlike a password it cannot be rotated. Smaller than the audit assumed - the recipient side already existed, since DriveAttachForm renders a masked, validated key field for a private drive. S5 - folders can be shared. The drive key is now decoded once, before the route shape is decided, and attached to whichever route matches; it used to be decoded inside the drive branch and returned from there, so a *valid* key made the folder segments unreachable and /drives/{id}/folders/{fid}?driveKey= silently resolved to the drive root. Only a damaged key ever reached the folder route, which is exactly backwards. Share actions added to the details panel and the folder menu. Also fixes a regression from the previous commit: restoreRouteInformation wrote the drive key back only alongside a name, and private links no longer carry one, so a keyed link lost its key on the first route restore. The location is now rebuilt from whichever parts are present. Known limit, documented rather than papered over: a folder link opened by someone who does not yet have the drive attached lands at the drive root, because the folder id is cleared while the drive is attached and re-selected. Not a regression - the folder was previously dropped at the parser. --- lib/blocs/drive_share/drive_share_cubit.dart | 63 +++++++++--- lib/blocs/drive_share/drive_share_state.dart | 27 ++++- lib/components/details_panel.dart | 15 ++- lib/components/drive_share_dialog.dart | 91 +++++++++++++++-- lib/l10n/app_en.arb | 24 +++++ lib/pages/app_route_information_parser.dart | 75 +++++++++----- lib/pages/app_route_path.dart | 15 ++- .../components/drive_explorer_item_tile.dart | 18 ++++ lib/utils/link_generators.dart | 44 ++++++++- test/blocs/drive_share_cubit_test.dart | 99 ++++++++++++++++++- .../app_route_information_parser_test.dart | 54 +++++++++- test/utils/link_generators_test.dart | 50 +++++++++- 12 files changed, 508 insertions(+), 67 deletions(-) diff --git a/lib/blocs/drive_share/drive_share_cubit.dart b/lib/blocs/drive_share/drive_share_cubit.dart index 7a161e3d49..4026daca99 100644 --- a/lib/blocs/drive_share/drive_share_cubit.dart +++ b/lib/blocs/drive_share/drive_share_cubit.dart @@ -1,7 +1,10 @@ import 'package:ardrive/blocs/blocs.dart'; +import 'package:ardrive/core/crypto/crypto.dart'; import 'package:ardrive/models/models.dart'; import 'package:ardrive/utils/link_generators.dart'; import 'package:ardrive/utils/logger.dart'; +import 'package:ardrive_utils/ardrive_utils.dart'; +import 'package:arweave/utils.dart'; import 'package:equatable/equatable.dart'; import 'package:flutter/foundation.dart'; import 'package:flutter_bloc/flutter_bloc.dart'; @@ -12,11 +15,21 @@ part 'drive_share_state.dart'; class DriveShareCubit extends Cubit { final Drive drive; + /// The folder being shared, or `null` when the whole drive is. + final FolderID? folderId; + final ProfileCubit _profileCubit; final DriveDao _driveDao; + /// Whether the built link embeds the drive key. + /// + /// Starts off. See [generatePrivateDriveShareLink] for why a drive key is + /// handed over separately by default. + bool _keyIsInLink = false; + DriveShareCubit({ required this.drive, + this.folderId, required DriveDao driveDao, required ProfileCubit profileCubit, }) : _driveDao = driveDao, @@ -25,6 +38,17 @@ class DriveShareCubit extends Cubit { loadDriveShareDetails(); } + /// Rebuilds the link with or without the key embedded in it. + Future setKeyIsInLink(bool value) async { + if (value == _keyIsInLink) { + return; + } + + _keyIsInLink = value; + + await loadDriveShareDetails(); + } + /// Builds the share link for [drive], or fails in a way the dialog can show. /// /// Everything here runs inside the guard on purpose. This method is called @@ -36,11 +60,19 @@ class DriveShareCubit extends Cubit { emit(DriveShareLoadInProgress()); try { - final driveShareLink = drive.isPrivate - ? await _privateDriveShareLink() - : generatePublicDriveShareLink( + final driveKey = drive.isPrivate ? await _driveKey() : null; + + final driveShareLink = driveKey == null + ? generatePublicDriveShareLink( driveId: drive.id, driveName: drive.name, + folderId: folderId, + ) + : await generatePrivateDriveShareLink( + driveId: drive.id, + driveKey: driveKey.key, + folderId: folderId, + includeKey: _keyIsInLink, ); if (isClosed) { @@ -51,6 +83,13 @@ class DriveShareCubit extends Cubit { DriveShareLoadSuccess( drive: drive, driveShareLink: driveShareLink, + isFolder: folderId != null, + keyIsInLink: _keyIsInLink, + // The key is offered as its own artifact so the sharer can send it + // through a different channel than the link. + driveKeyBase64: driveKey == null + ? null + : encodeBytesToBase64(await driveKey.key.extractBytes()), ), ); } catch (e, stacktrace) { @@ -70,14 +109,13 @@ class DriveShareCubit extends Cubit { } } - /// The link for a private drive, which needs the drive key to be reachable. + /// The private drive's key, which the link and the handover both need. /// - /// The key comes from the profile when one is signed in, and from the - /// in-memory store otherwise - a drive attached in this session but never - /// persisted. Neither is guaranteed, and a [StateError] here is a real - /// outcome rather than a should-never-happen: it lands on the failure state - /// above. - Future _privateDriveShareLink() async { + /// It comes from the profile when one is signed in, and from the in-memory + /// store otherwise - a drive attached in this session but never persisted. + /// Neither is guaranteed, and a [StateError] here is a real outcome rather + /// than a should-never-happen: it lands on the failure state above. + Future _driveKey() async { final profileState = _profileCubit.state; final driveKey = profileState is ProfileLoggedIn @@ -88,9 +126,6 @@ class DriveShareCubit extends Cubit { throw StateError('Drive key not found'); } - return generatePrivateDriveShareLink( - driveId: drive.id, - driveKey: driveKey.key, - ); + return driveKey; } } diff --git a/lib/blocs/drive_share/drive_share_state.dart b/lib/blocs/drive_share/drive_share_state.dart index 7622b67e9d..fb2536fb69 100644 --- a/lib/blocs/drive_share/drive_share_state.dart +++ b/lib/blocs/drive_share/drive_share_state.dart @@ -5,7 +5,7 @@ abstract class DriveShareState extends Equatable { const DriveShareState(); @override - List get props => []; + List get props => []; } /// [DriveShareLoadInProgress] means that the drive share details are being loaded. @@ -18,13 +18,36 @@ class DriveShareLoadSuccess extends DriveShareState { /// The link to share access of this drive with. final Uri driveShareLink; + /// Whether the link points at one folder rather than the whole drive. + final bool isFolder; + + /// Whether the drive key is embedded in [driveShareLink]. + final bool keyIsInLink; + + /// The drive key, for the sharer to hand over separately. + /// + /// `null` for a public drive, which has none. + final String? driveKeyBase64; + const DriveShareLoadSuccess({ required this.drive, required this.driveShareLink, + this.isFolder = false, + this.keyIsInLink = false, + this.driveKeyBase64, }); + /// Whether the key travels as its own artifact rather than inside the link. + bool get hasSeparateKeyArtifact => driveKeyBase64 != null && !keyIsInLink; + @override - List get props => [drive, driveShareLink]; + List get props => [ + drive, + driveShareLink, + isFolder, + keyIsInLink, + driveKeyBase64, + ]; } /// [DriveShareLoadFail] shows failiure states in the UI. diff --git a/lib/components/details_panel.dart b/lib/components/details_panel.dart index 4f7b5ea743..d1018dab2b 100644 --- a/lib/components/details_panel.dart +++ b/lib/components/details_panel.dart @@ -1190,7 +1190,9 @@ class DetailsPanelToolbar extends StatelessWidget { const SizedBox( width: 16, ), - if (item is FileDataTableItem || item is DriveDataItem) + if (item is FileDataTableItem || + item is DriveDataItem || + item is FolderDataTableItem) _buildActionIcon( tooltip: _getShareTooltip(item, context), icon: ArDriveIcons.share(size: defaultIconSize), @@ -1201,6 +1203,15 @@ class DetailsPanelToolbar extends StatelessWidget { driveId: item.driveId, fileId: item.id, ); + } else if (item is FolderDataTableItem) { + // A folder link is a drive link that names a folder, and it + // carries the drive key for the same reason - a folder in a + // private drive cannot be read without it. + promptToShareDrive( + context: context, + drive: drive, + folderId: item.id, + ); } else if (item is DriveDataItem) { promptToShareDrive( context: context, @@ -1303,6 +1314,8 @@ class DetailsPanelToolbar extends StatelessWidget { String _getShareTooltip(ArDriveDataTableItem item, BuildContext context) { if (item is FileDataTableItem) { return appLocalizationsOf(context).shareFile; + } else if (item is FolderDataTableItem) { + return appLocalizationsOf(context).shareFolder; } else if (item is DriveDataItem) { return appLocalizationsOf(context).shareDrive; } else { diff --git a/lib/components/drive_share_dialog.dart b/lib/components/drive_share_dialog.dart index c4f0163c42..9c02f9f446 100644 --- a/lib/components/drive_share_dialog.dart +++ b/lib/components/drive_share_dialog.dart @@ -8,15 +8,18 @@ import 'package:ardrive_ui/ardrive_ui.dart'; import 'package:flutter/material.dart'; import 'package:flutter_bloc/flutter_bloc.dart'; +/// Shares [drive], or - when [folderId] is given - one folder inside it. Future promptToShareDrive({ required BuildContext context, required Drive drive, + String? folderId, }) => showArDriveDialog( context, content: BlocProvider( create: (_) => DriveShareCubit( drive: drive, + folderId: folderId, driveDao: context.read(), profileCubit: context.read(), ), @@ -34,6 +37,14 @@ class DriveShareDialog extends StatefulWidget { class DriveShareDialogState extends State { final shareLinkController = TextEditingController(); + final driveKeyController = TextEditingController(); + + @override + void dispose() { + shareLinkController.dispose(); + driveKeyController.dispose(); + super.dispose(); + } @override void initState() { @@ -46,6 +57,7 @@ class DriveShareDialogState extends State { listener: (context, state) { if (state is DriveShareLoadSuccess) { shareLinkController.text = state.driveShareLink.toString(); + driveKeyController.text = state.driveKeyBase64 ?? ''; } }, builder: (context, state) { @@ -53,9 +65,12 @@ class DriveShareDialogState extends State { return ArDriveStandardModalNew( width: kLargeDialogWidth, - title: appLocalizationsOf(context).shareDriveWithOthers, + title: state is DriveShareLoadSuccess && state.isFolder + ? appLocalizationsOf(context).shareFolderWithOthers + : appLocalizationsOf(context).shareDriveWithOthers, description: state is DriveShareLoadSuccess ? state.drive.name : null, + scrollableContent: true, content: SizedBox( width: kLargeDialogWidth, child: Column( @@ -66,18 +81,80 @@ class DriveShareDialogState extends State { const Center(child: CircularProgressIndicator()) else if (state is DriveShareLoadSuccess) ...{ CopyableShareArtifact( - label: appLocalizationsOf(context).shareDriveWithOthers, + label: appLocalizationsOf(context).shareFileLinkLabel, controller: shareLinkController, text: state.driveShareLink.toString(), copyLabel: appLocalizationsOf(context).copyLink, revealLabel: appLocalizationsOf(context).shareDriveRevealLink, - // A private drive's link carries the drive key, which - // decrypts every file and folder name in the drive and - // cannot be rotated. A public drive's link is not a - // secret and is left legible. - isSecret: state.drive.isPrivate, + // Only a link the sharer chose to embed the key in holds + // a secret. A keyless link - the default - is not worth + // hiding, and hiding it would suggest it is dangerous to + // share, which is the opposite of the point. + isSecret: state.keyIsInLink, ), + if (state.hasSeparateKeyArtifact) ...{ + const SizedBox(height: 16), + CopyableShareArtifact( + label: appLocalizationsOf(context) + .shareDriveAccessKeyLabel, + controller: driveKeyController, + text: state.driveKeyBase64!, + copyLabel: appLocalizationsOf(context).copyAccessKey, + revealLabel: + appLocalizationsOf(context).shareFileRevealKey, + isSecret: true, + ), + Padding( + padding: const EdgeInsets.only(top: 4), + child: Text( + appLocalizationsOf(context) + .shareDriveSendKeySeparately, + style: typography.paragraphSmall( + color: ArDriveTheme.of(context) + .themeData + .colorTokens + .textLow, + ), + ), + ), + }, + if (state.drive.isPrivate) ...{ + const SizedBox(height: 16), + ArDriveCheckBox( + // The checkbox only reads `checked` when it is first + // built, so the key forces a fresh one whenever the + // cubit's answer changes. + key: ValueKey(state.keyIsInLink), + checked: state.keyIsInLink, + title: appLocalizationsOf(context) + .shareDriveIncludeKeyInLink, + titleStyle: typography.paragraphSmall( + color: ArDriveTheme.of(context) + .themeData + .colorTokens + .textMid, + ), + onChange: (value) => + context.read().setKeyIsInLink( + value, + ), + ), + if (state.keyIsInLink) + Padding( + padding: const EdgeInsets.only(top: 4), + child: Text( + appLocalizationsOf(context) + .shareDriveKeyInLinkWarning, + style: typography.paragraphSmall( + color: ArDriveTheme.of(context) + .themeData + .colorTokens + .strokeRed, + ), + ), + ), + }, const SizedBox(height: 16), Text( state.drive.isPublic diff --git a/lib/l10n/app_en.arb b/lib/l10n/app_en.arb index 048b691f01..7b48e3ab8e 100644 --- a/lib/l10n/app_en.arb +++ b/lib/l10n/app_en.arb @@ -2334,6 +2334,30 @@ "@shareDriveWithOthers": { "description": "The action of sharing a drive link" }, + "shareFolderWithOthers": "Share folder with others", + "@shareFolderWithOthers": { + "description": "Title of the dialog that shares a link to one folder" + }, + "shareFolder": "Share folder", + "@shareFolder": { + "description": "The action of sharing a link to a folder" + }, + "shareDriveAccessKeyLabel": "Drive access key", + "@shareDriveAccessKeyLabel": { + "description": "Label of the field holding the drive key handed over alongside a private drive link" + }, + "shareDriveIncludeKeyInLink": "Include the key in the link", + "@shareDriveIncludeKeyInLink": { + "description": "Checkbox that embeds the drive key in the share link instead of handing it over separately" + }, + "shareDriveKeyInLinkWarning": "Anyone with this link can open every file in the drive, including files added later. A drive key cannot be changed.", + "@shareDriveKeyInLinkWarning": { + "description": "Warning shown when the sharer chooses to embed the drive key in the link" + }, + "shareDriveSendKeySeparately": "Send this key separately from the link.", + "@shareDriveSendKeySeparately": { + "description": "Helper text under the drive key field explaining the two-artifact handover" + }, "shareDriveFailure": "We couldn’t create a share link for this drive. Check your connection and try again.", "@shareDriveFailure": { "description": "Shown in the share drive dialog when the link could not be built, most often because the drive key could not be read" diff --git a/lib/pages/app_route_information_parser.dart b/lib/pages/app_route_information_parser.dart index 19494d1399..22d471ab92 100644 --- a/lib/pages/app_route_information_parser.dart +++ b/lib/pages/app_route_information_parser.dart @@ -78,21 +78,27 @@ class AppRouteInformationParser extends RouteInformationParser { final driveId = uri.pathSegments[1]; final name = uri.queryParameters['name']; final driveKeyBase64 = uri.queryParameters[driveKeyQueryParamName]; + + DriveKey? sharedDriveKey; + String? sharedRawDriveKey; + + // Decoded once, before the route shape is decided, so that the key + // reaches whichever route matches. This used to sit inside the drive + // branch and return from there, which meant a *valid* key made the + // folder segments unreachable: `/drives/{id}/folders/{fid}?driveKey=` + // silently resolved to the drive root and lost the folder. Only a + // damaged key ever reached the folder route, which is the wrong way + // round. if (driveKeyBase64 != null && driveKeyBase64.isNotEmpty) { try { - final sharedDrivePkBytes = - utils.decodeBase64ToBytes(driveKeyBase64); - - return AppRoutePath.driveDetail( - driveId: driveId, - driveName: name, - sharedDrivePk: DriveKey(SecretKey(sharedDrivePkBytes), true), - sharedRawDriveKey: driveKeyBase64, + sharedDriveKey = DriveKey( + SecretKey(utils.decodeBase64ToBytes(driveKeyBase64)), + true, ); + sharedRawDriveKey = driveKeyBase64; } catch (e) { - // Same as the shared file link below: a damaged key must not - // throw while the route is being parsed. Drop it and carry on to - // the keyless drive route. + // A damaged key must not throw while the route is being parsed. + // Drop it and carry on to the keyless route. // // The reason is logged and never the exception object: the // `source` of the `FormatException` a base64 decoder throws *is* @@ -107,12 +113,22 @@ class AppRouteInformationParser extends RouteInformationParser { if (uri.pathSegments.length == 2) { // Handle '/drives/:driveId' - return AppRoutePath.driveDetail(driveId: driveId, driveName: name); + return AppRoutePath.driveDetail( + driveId: driveId, + driveName: name, + sharedDrivePk: sharedDriveKey, + sharedRawDriveKey: sharedRawDriveKey, + ); } else if (uri.pathSegments.length == 4 && uri.pathSegments[2] == 'folders') { // Handle /drives/:driveId/folders/:folderId return AppRoutePath.folderDetail( - driveId: driveId, driveFolderId: uri.pathSegments[3]); + driveId: driveId, + driveFolderId: uri.pathSegments[3], + driveName: name, + sharedDrivePk: sharedDriveKey, + sharedRawDriveKey: sharedRawDriveKey, + ); } } @@ -207,21 +223,26 @@ class AppRouteInformationParser extends RouteInformationParser { uri: Uri.parse('/get-started'), ); } else if (configuration.driveId != null) { - if (configuration.driveName != null && - configuration.sharedRawDriveKey != null) { - return RouteInformation( - uri: Uri.parse( - '/drives/${configuration.driveId}?name=${configuration.driveName}' - '&$driveKeyQueryParamName=${configuration.sharedRawDriveKey}'), - ); - } + final path = configuration.driveFolderId == null + ? '/drives/${configuration.driveId}' + : '/drives/${configuration.driveId}' + '/folders/${configuration.driveFolderId}'; - return configuration.driveFolderId == null - ? RouteInformation(uri: Uri.parse('/drives/${configuration.driveId}')) - : RouteInformation( - uri: Uri.parse( - '/drives/${configuration.driveId}/folders/${configuration.driveFolderId}'), - ); + // Each part is written when it is there, rather than only when both are. + // A private drive link no longer carries a name, and the old shape wrote + // the key back *only* alongside one - so the key silently fell out of the + // address bar and a refresh asked the recipient for a key their link + // already had. + final query = [ + if (configuration.driveName != null) + 'name=${Uri.encodeQueryComponent(configuration.driveName!)}', + if (configuration.sharedRawDriveKey != null) + '$driveKeyQueryParamName=${configuration.sharedRawDriveKey}', + ]; + + return RouteInformation( + uri: Uri.parse(query.isEmpty ? path : '$path?${query.join('&')}'), + ); } else if (configuration.rawTransactionId != null) { return RouteInformation( uri: Uri.parse( diff --git a/lib/pages/app_route_path.dart b/lib/pages/app_route_path.dart index 56efb75014..40554746ed 100644 --- a/lib/pages/app_route_path.dart +++ b/lib/pages/app_route_path.dart @@ -99,11 +99,24 @@ class AppRoutePath { ); /// Creates a route that points to a folder in a particular drive. + /// + /// Carries the drive key for the same reason [driveDetail] does: a folder + /// inside a private drive cannot be read without it, and a link that names + /// one has nowhere else to put it. factory AppRoutePath.folderDetail({ required String driveId, required String driveFolderId, + String? driveName, + DriveKey? sharedDrivePk, + String? sharedRawDriveKey, }) => - AppRoutePath(driveId: driveId, driveFolderId: driveFolderId); + AppRoutePath( + driveId: driveId, + driveFolderId: driveFolderId, + driveName: driveName, + sharedDriveKey: sharedDrivePk, + sharedRawDriveKey: sharedRawDriveKey, + ); /// Creates a route that points to a particular shared file. factory AppRoutePath.sharedFile({ diff --git a/lib/pages/drive_detail/components/drive_explorer_item_tile.dart b/lib/pages/drive_detail/components/drive_explorer_item_tile.dart index 44583050dd..2d441817ef 100644 --- a/lib/pages/drive_detail/components/drive_explorer_item_tile.dart +++ b/lib/pages/drive_detail/components/drive_explorer_item_tile.dart @@ -895,6 +895,24 @@ class EntityActionsMenu extends StatelessWidget { ), hideFileDropdownItem(context, item), ], + // Guarded rather than assumed: this menu is also built in places that + // do not know the drive, and a folder link needs it for the key. + if (drive != null) + ArDriveDropdownItem( + onClick: () { + promptToShareDrive( + context: context, + drive: drive!, + folderId: item.id, + ); + }, + content: _buildItem( + appLocalizationsOf(context).shareFolder, + ArDriveIcons.share( + size: defaultIconSize, + ), + ), + ), if (withInfo) _buildInfoOption(context), ]; } else if (item is DriveDataItem) { diff --git a/lib/utils/link_generators.dart b/lib/utils/link_generators.dart index 90234020bc..edbddb8b19 100644 --- a/lib/utils/link_generators.dart +++ b/lib/utils/link_generators.dart @@ -12,15 +12,29 @@ import 'package:flutter/foundation.dart'; /// build points back at that preview build. Everywhere else, app.ardrive.io. String shareLinkOrigin() => kIsWeb ? Uri.base.origin : linkOriginProduction; +/// A share link for a public drive, or for one folder inside it. +/// +/// The name rides along because a public drive's name is not a secret and it +/// saves the recipient's client a lookup. See [generatePrivateDriveShareLink] +/// for why the private variant carries none. Uri generatePublicDriveShareLink({ required final DriveID driveId, required final String driveName, + final FolderID? folderId, }) { - final driveShareLink = - '${shareLinkOrigin()}/#/drives/$driveId?name=${Uri.encodeQueryComponent(driveName)}'; + final driveShareLink = '${shareLinkOrigin()}' + '${_driveLocation(driveId: driveId, folderId: folderId)}' + '?name=${Uri.encodeQueryComponent(driveName)}'; + return Uri.parse(driveShareLink); } +/// The route location of a drive, or of a folder within it. +String _driveLocation({required DriveID driveId, FolderID? folderId}) => + folderId == null + ? '/#/drives/$driveId' + : '/#/drives/$driveId/folders/$folderId'; + /// A share link for a private drive. /// /// Deliberately carries no `name`. A private drive's name is a secret of @@ -33,15 +47,35 @@ Uri generatePublicDriveShareLink({ /// real name from the drive's own record as soon as the key is in hand /// (`DriveAttachCubit.driveNameLoader`), so the name in the link was only ever /// a pre-fill that the chain immediately overwrote. +/// [includeKey] embeds the drive key in the link. +/// +/// Off by default, and off is what the share dialog uses unless the sharer +/// opts in. A drive key opens every file and every folder name in the drive +/// for the life of the drive, and - unlike a password - **it cannot be +/// rotated**. A link that carries one is a link whose key is in every forward, +/// screenshot and unfurl of the message it travelled in. The keyless link and +/// the key are handed over as two artifacts, meant for two channels, exactly +/// as a private file's are. +/// +/// A recipient who opens a keyless link is not stuck: the attach form asks for +/// the key, validates it, and reads the drive's real name off the chain once +/// it has one. Future generatePrivateDriveShareLink({ required final DriveID driveId, required final SecretKey driveKey, + final FolderID? folderId, + final bool includeKey = false, }) async { + final location = + '${shareLinkOrigin()}${_driveLocation(driveId: driveId, folderId: folderId)}'; + + if (!includeKey) { + return Uri.parse(location); + } + final driveKeyBase64 = encodeBytesToBase64(await driveKey.extractBytes()); - return Uri.parse( - '${shareLinkOrigin()}/#/drives/$driveId?driveKey=$driveKeyBase64', - ); + return Uri.parse('$location?driveKey=$driveKeyBase64'); } Uri generatePublicFileShareLink({ diff --git a/test/blocs/drive_share_cubit_test.dart b/test/blocs/drive_share_cubit_test.dart index cfc4e3ece1..4422b48821 100644 --- a/test/blocs/drive_share_cubit_test.dart +++ b/test/blocs/drive_share_cubit_test.dart @@ -41,8 +41,9 @@ void main() { Future drive() => driveDao.driveById(driveId: driveId).getSingle(); - DriveShareCubit cubit(Drive d) => DriveShareCubit( + DriveShareCubit cubit(Drive d, {String? folderId}) => DriveShareCubit( drive: d, + folderId: folderId, driveDao: driveDao, profileCubit: profileCubit, ); @@ -152,6 +153,95 @@ void main() { await expectation; }); + test('a private drive link is keyless by default', () async { + // A drive key opens every file in the drive, for the life of the drive, + // and cannot be rotated. It is handed over as its own artifact unless + // the sharer deliberately embeds it. + when(() => profileCubit.state).thenReturn(ProfilePromptAdd()); + + await insertDrive(isPrivate: true); + await driveDao.putDriveKeyInMemory(driveID: driveId, driveKey: driveKey); + + final state = await settled(cubit(await drive())); + + expect(state, isA()); + + final success = state as DriveShareLoadSuccess; + + expect(success.driveShareLink.toString(), isNot(contains('driveKey'))); + expect(success.keyIsInLink, isFalse); + // The key is still offered, just not inside the link. + expect(success.hasSeparateKeyArtifact, isTrue); + expect(success.driveKeyBase64, isNotNull); + }); + + test('opting in embeds the key and retires the separate artifact', + () async { + when(() => profileCubit.state).thenReturn(ProfilePromptAdd()); + + await insertDrive(isPrivate: true); + await driveDao.putDriveKeyInMemory(driveID: driveId, driveKey: driveKey); + + final c = cubit(await drive()); + + await settled(c); + await c.setKeyIsInLink(true); + + final success = c.state as DriveShareLoadSuccess; + + expect(success.driveShareLink.toString(), contains('driveKey=')); + expect(success.keyIsInLink, isTrue); + expect(success.hasSeparateKeyArtifact, isFalse); + }); + + test('a public drive has no key artifact to offer', () async { + await insertDrive(isPrivate: false); + + final success = await settled(cubit(await drive())) + as DriveShareLoadSuccess; + + expect(success.driveKeyBase64, isNull); + expect(success.hasSeparateKeyArtifact, isFalse); + }); + + test('a folder link names the folder inside the drive', () async { + const folderId = 'c4d9dc2c-5d4c-6e3d-ac4f-8f3c2d5e6f70'; + + await insertDrive(isPrivate: false); + + final success = + await settled(cubit(await drive(), folderId: folderId)) + as DriveShareLoadSuccess; + + expect( + success.driveShareLink.toString(), + contains('/#/drives/$driveId/folders/$folderId'), + ); + expect(success.isFolder, isTrue); + }); + + test('a private folder link can carry the drive key when opted in', + () async { + const folderId = 'c4d9dc2c-5d4c-6e3d-ac4f-8f3c2d5e6f70'; + + when(() => profileCubit.state).thenReturn(ProfilePromptAdd()); + + await insertDrive(isPrivate: true); + await driveDao.putDriveKeyInMemory(driveID: driveId, driveKey: driveKey); + + final c = cubit(await drive(), folderId: folderId); + + await settled(c); + await c.setKeyIsInLink(true); + + final link = (c.state as DriveShareLoadSuccess).driveShareLink.toString(); + + // Both halves have to survive together: the parser used to return the + // drive route as soon as it saw a key, which lost the folder. + expect(link, contains('/folders/$folderId')); + expect(link, contains('driveKey=')); + }); + test('a private drive resolves to a link carrying its key', () async { // Signed out, so the key comes from the in-memory vault - the path a // drive attached this session but never persisted takes. @@ -163,12 +253,13 @@ void main() { driveKey: driveKey, ); - final state = await settled(cubit(await drive())); + final c = cubit(await drive()); - expect(state, isA()); + await settled(c); + await c.setKeyIsInLink(true); expect( - (state as DriveShareLoadSuccess).driveShareLink.toString(), + (c.state as DriveShareLoadSuccess).driveShareLink.toString(), contains('driveKey='), ); }); diff --git a/test/pages/app_route_information_parser_test.dart b/test/pages/app_route_information_parser_test.dart index 9233814a6e..7e826cbba6 100644 --- a/test/pages/app_route_information_parser_test.dart +++ b/test/pages/app_route_information_parser_test.dart @@ -306,12 +306,12 @@ void main() { }); }); - group('restoreRouteInformation', () { - Future roundTrip(AppRoutePath routePath) async { - final restored = parser.restoreRouteInformation(routePath); + Future roundTripOf(AppRoutePath routePath) => + parser.parseRouteInformation(parser.restoreRouteInformation(routePath)); - return parser.parseRouteInformation(restored); - } + group('restoreRouteInformation', () { + Future roundTrip(AppRoutePath routePath) => + roundTripOf(routePath); test('a v1 link keeps the shape it has always had', () async { final routePath = await parse( @@ -390,6 +390,21 @@ void main() { expect(restored.sharedRawDriveKey, validDriveKey); }); + test('a nameless private drive link keeps its key', () async { + // A private drive link no longer carries a name - the name is a secret + // and the recipient's attach flow reads the real one off the chain. The + // key must survive the round trip regardless, or a refresh would drop it + // and the recipient would be asked to paste a key the link already had. + final routePath = await parse( + '/drives/$driveId?$driveKeyQueryParamName=$validDriveKey', + ); + + final restored = await roundTrip(routePath); + + expect(restored.driveId, driveId); + expect(restored.sharedRawDriveKey, validDriveKey); + }); + test('a folder link still round trips', () async { final routePath = await parse('/drives/$driveId/folders/$folderId'); @@ -463,6 +478,35 @@ void main() { expect(routePath.sharedDriveKey, isNull); }); + test('a folder link keeps its drive key instead of losing the folder', + () async { + // The key used to be decoded inside the drive branch, which returned + // from there - so a *valid* key made the folder segments unreachable and + // the link quietly resolved to the drive root. Only a damaged key ever + // reached the folder route, which is exactly backwards. + final routePath = await parse( + '/drives/$driveId/folders/$folderId' + '?$driveKeyQueryParamName=$validDriveKey', + ); + + expect(routePath.driveId, driveId); + expect(routePath.driveFolderId, folderId); + expect(routePath.sharedRawDriveKey, validDriveKey); + expect(routePath.sharedDriveKey, isNotNull); + }); + + test('a private folder link round trips with both', () async { + final routePath = await parse( + '/drives/$driveId/folders/$folderId' + '?$driveKeyQueryParamName=$validDriveKey', + ); + + final restored = await roundTripOf(routePath); + + expect(restored.driveFolderId, folderId); + expect(restored.sharedRawDriveKey, validDriveKey); + }); + test('parses a folder link', () async { final routePath = await parse('/drives/$driveId/folders/$folderId'); diff --git a/test/utils/link_generators_test.dart b/test/utils/link_generators_test.dart index 1152b2c2dd..3f12b5abfa 100644 --- a/test/utils/link_generators_test.dart +++ b/test/utils/link_generators_test.dart @@ -47,6 +47,7 @@ void main() { final webShareUri = await generatePrivateDriveShareLink( driveId: testPrivateDrive.id, driveKey: testPrivateDriveKey, + includeKey: true, ); // Remove # delimiter as it messes with Uri parsing outside of app route // information parser @@ -206,18 +207,65 @@ void main() { ); }); - test('a private drive link still carries its key in the query', () async { + test('a private drive link carries its key when the sharer opts in', + () async { const driveKeyBase64 = 'X123YZAB-CD4e5fgHIjKlmN6O7pqrStuVwxYzaBcd8E'; final link = await generatePrivateDriveShareLink( driveId: 'driveId', driveKey: SecretKey(decodeBase64ToBytes(driveKeyBase64)), + includeKey: true, ); expect(link.toString(), startsWith('https://app.ardrive.io/#/drives/')); expect(link.toString(), endsWith('?driveKey=$driveKeyBase64')); }); + test('a private drive link is keyless unless asked otherwise', () async { + // The default matters more than the capability: a drive key opens + // every file in the drive for the life of the drive and cannot be + // rotated, so embedding one is a deliberate act. + const driveKeyBase64 = 'X123YZAB-CD4e5fgHIjKlmN6O7pqrStuVwxYzaBcd8E'; + + final link = await generatePrivateDriveShareLink( + driveId: 'driveId', + driveKey: SecretKey(decodeBase64ToBytes(driveKeyBase64)), + ); + + expect(link.toString(), 'https://app.ardrive.io/#/drives/driveId'); + expect(link.toString(), isNot(contains(driveKeyBase64))); + }); + + test('a folder link names the folder under its drive', () async { + expect( + generatePublicDriveShareLink( + driveId: 'driveId', + driveName: 'My Drive', + folderId: 'folderId', + ).toString(), + 'https://app.ardrive.io/#/drives/driveId/folders/folderId' + '?name=My+Drive', + ); + }); + + test('a private folder link keeps the folder and the key together', + () async { + const driveKeyBase64 = 'X123YZAB-CD4e5fgHIjKlmN6O7pqrStuVwxYzaBcd8E'; + + final link = await generatePrivateDriveShareLink( + driveId: 'driveId', + driveKey: SecretKey(decodeBase64ToBytes(driveKeyBase64)), + folderId: 'folderId', + includeKey: true, + ); + + expect( + link.toString(), + 'https://app.ardrive.io/#/drives/driveId/folders/folderId' + '?driveKey=$driveKeyBase64', + ); + }); + test('a private drive link does not carry the drive name', () async { // The name of a private drive is as sensitive as the names of the // files inside it, and the recipient's attach flow reads the real one From 8e171629eae997a9b17630db9d896a727dc2eecb Mon Sep 17 00:00:00 2001 From: vilenarios Date: Tue, 18 Aug 2026 17:43:27 -0400 Subject: [PATCH 04/13] fix: address review findings on the sharing changes PE-9210 The one that mattered: the share dialogs filled their fields from a bloc listener, and a listener does not fire for the state a bloc is already in. The public drive path builds its link synchronously, so the dialog reached the success state before it could listen and the link field rendered empty. CopyableShareArtifact now owns its controller and takes the value directly, which removes the whole class of bug from both dialogs. - Reveal state resets when the value underneath it changes, so ticking "include the key in the link" does not inherit the previous reveal. - DriveShareCubit captures the key-embedding flag per load and guards its emit with a generation counter. Two loads racing after a fast double-toggle could otherwise emit a link built with the key in it while labelling it keyless - which the dialog would then render unmasked. - DriveShareLoadSuccess redacts the drive key from toString, since equatable stringifies props in debug builds. Same treatment FileShareLoadSuccess gets. - Folder sharing was added to EntityActionsMenu, but the explorer renders folder rows with DriveExplorerItemTileTrailing, so the action was invisible where users would look for it. Added there too. - The drive name lookup during attach is guarded; it runs inside a microtask nobody awaits, so a network failure escaped as an unhandled async error. - Bounded the two reads in submit(), the unlock path, which could hang the locked page after a key was pasted. - The details drawer test now uses distinct created and modified dates, so each row is pinned to its own field. Not taken: routing every shared file read through a cancellable request. True that Future.timeout does not cancel its source, but the bug being fixed was a page that hung forever, and it no longer does. Cancellation belongs at the GraphQL client, which sync shares and which PE-9205 has just tuned - too wide a blast radius for this change. --- .../drive_attach/drive_attach_cubit.dart | 17 ++++++- lib/blocs/drive_share/drive_share_cubit.dart | 31 ++++++++++--- lib/blocs/drive_share/drive_share_state.dart | 10 +++++ lib/blocs/shared_file/shared_file_cubit.dart | 11 +++-- lib/components/copyable_share_artifact.dart | 44 +++++++++++++++---- lib/components/drive_share_dialog.dart | 19 +------- lib/components/file_share_dialog.dart | 19 +------- .../components/drive_explorer_item_tile.dart | 16 +++++++ .../copyable_share_artifact_test.dart | 39 ++++++++++++++-- .../shared_file/shared_file_page_test.dart | 10 ++++- 10 files changed, 156 insertions(+), 60 deletions(-) diff --git a/lib/blocs/drive_attach/drive_attach_cubit.dart b/lib/blocs/drive_attach/drive_attach_cubit.dart index 560d1f6784..0c9668fe0c 100644 --- a/lib/blocs/drive_attach/drive_attach_cubit.dart +++ b/lib/blocs/drive_attach/drive_attach_cubit.dart @@ -101,7 +101,22 @@ class DriveAttachCubit extends Cubit { // moving that read later would silently discard it. if (driveNameController.text.isEmpty && driveKeyController.text.isNotEmpty) { - await driveNameLoader(); + // Guarded: this runs inside a microtask started from the + // constructor whose future nobody awaits, so a network or decode + // failure in here would otherwise surface as an unhandled + // asynchronous error. A name that will not resolve is not fatal - + // the auto-submit below is simply skipped and the form stays open + // for the recipient to act on, which is what a keyless link + // already does. + try { + await driveNameLoader(); + } catch (e, stacktrace) { + logger.e( + 'Failed to resolve the name of the shared drive', + e, + stacktrace, + ); + } if (isClosed) return; } diff --git a/lib/blocs/drive_share/drive_share_cubit.dart b/lib/blocs/drive_share/drive_share_cubit.dart index 4026daca99..4ec430feec 100644 --- a/lib/blocs/drive_share/drive_share_cubit.dart +++ b/lib/blocs/drive_share/drive_share_cubit.dart @@ -27,6 +27,11 @@ class DriveShareCubit extends Cubit { /// handed over separately by default. bool _keyIsInLink = false; + /// Which load is current. A load that is not the newest by the time it + /// finishes has been overtaken and must not emit - the same generation guard + /// [SharedFileCubit] uses. + int _generation = 0; + DriveShareCubit({ required this.drive, this.folderId, @@ -57,6 +62,14 @@ class DriveShareCubit extends Cubit { /// [DriveShareLoadInProgress] and the dialog would spin forever, which is /// exactly what a missing drive key used to do. Future loadDriveShareDetails() async { + final generation = ++_generation; + + // Captured once. Read twice - once to build the link and once to describe + // it - two loads racing after a fast double-toggle could emit a link built + // with the key in it while labelling it keyless, and the dialog would then + // render a key-bearing link unmasked. + final keyIsInLink = _keyIsInLink; + emit(DriveShareLoadInProgress()); try { @@ -72,10 +85,16 @@ class DriveShareCubit extends Cubit { driveId: drive.id, driveKey: driveKey.key, folderId: folderId, - includeKey: _keyIsInLink, + includeKey: keyIsInLink, ); - if (isClosed) { + // Extracted before the guard so that no await sits between the checks + // below and the emit. + final driveKeyBase64 = driveKey == null + ? null + : encodeBytesToBase64(await driveKey.key.extractBytes()); + + if (isClosed || generation != _generation) { return; } @@ -84,12 +103,10 @@ class DriveShareCubit extends Cubit { drive: drive, driveShareLink: driveShareLink, isFolder: folderId != null, - keyIsInLink: _keyIsInLink, + keyIsInLink: keyIsInLink, // The key is offered as its own artifact so the sharer can send it // through a different channel than the link. - driveKeyBase64: driveKey == null - ? null - : encodeBytesToBase64(await driveKey.key.extractBytes()), + driveKeyBase64: driveKeyBase64, ), ); } catch (e, stacktrace) { @@ -101,7 +118,7 @@ class DriveShareCubit extends Cubit { stacktrace, ); - if (isClosed) { + if (isClosed || generation != _generation) { return; } diff --git a/lib/blocs/drive_share/drive_share_state.dart b/lib/blocs/drive_share/drive_share_state.dart index fb2536fb69..121b57295e 100644 --- a/lib/blocs/drive_share/drive_share_state.dart +++ b/lib/blocs/drive_share/drive_share_state.dart @@ -48,6 +48,16 @@ class DriveShareLoadSuccess extends DriveShareState { keyIsInLink, driveKeyBase64, ]; + + /// Equatable stringifies [props] in debug builds, and one of them is a drive + /// key. Same redaction, for the same reason, as [FileShareLoadSuccess]. + /// + /// The link is printed whole: it is only a secret when the sharer embedded + /// the key in it, and [keyIsInLink] says when that is. + @override + String toString() => 'DriveShareLoadSuccess(drive: ${drive.id}, ' + 'isFolder: $isFolder, keyIsInLink: $keyIsInLink, ' + 'driveKey: ${driveKeyBase64 == null ? 'none' : ''})'; } /// [DriveShareLoadFail] shows failiure states in the UI. diff --git a/lib/blocs/shared_file/shared_file_cubit.dart b/lib/blocs/shared_file/shared_file_cubit.dart index cfcd841654..110a94c4d1 100644 --- a/lib/blocs/shared_file/shared_file_cubit.dart +++ b/lib/blocs/shared_file/shared_file_cubit.dart @@ -519,15 +519,18 @@ class SharedFileCubit extends Cubit { // The link names the exact revision that was shared, so the key is // tried against it directly: one lookup by transaction id instead of an // owner probe followed by a latest-revision query. - final shared = await _fetchSharedRevision(metadataTxId, fileKey); + final shared = await _bounded( + _fetchSharedRevision(metadataTxId, fileKey), + 'trying the key against the revision the link names', + ); if (shared == null) { // The link's metadata transaction is not on the network. The key may // still be perfectly good, so fall back to resolving the file the // long way rather than blaming the key. - final file = await _arweave.getLatestFileEntityWithId( - fileId, - fileKey, + final file = await _bounded( + _arweave.getLatestFileEntityWithId(fileId, fileKey), + 'looking up the newest revision to try the key against', ); if (file == null) { diff --git a/lib/components/copyable_share_artifact.dart b/lib/components/copyable_share_artifact.dart index 850e9dabc0..d4a66e83f3 100644 --- a/lib/components/copyable_share_artifact.dart +++ b/lib/components/copyable_share_artifact.dart @@ -18,13 +18,20 @@ import 'package:flutter/material.dart'; /// the key itself, or a link with the key embedded - is masked here until the /// sharer deliberately reveals it. /// -/// Masking never blocks the common path: [text] is copied from the value the -/// caller passed, not from what the field displays, so Copy works while masked. +/// Masking never blocks the common path: Copy puts [text] on the clipboard, not +/// what the field displays, so it works while the field is showing dots. +/// +/// ## Why the controller lives here +/// +/// [text] is the whole input. An earlier version took a controller and left +/// each dialog to fill it from a bloc listener, which silently broke for any +/// cubit that reached its success state synchronously: a listener does not fire +/// for the state a bloc is already in, so the field rendered empty. Owning the +/// controller means the field always shows what it was given. class CopyableShareArtifact extends StatefulWidget { const CopyableShareArtifact({ super.key, required this.label, - required this.controller, required this.text, required this.copyLabel, required this.revealLabel, @@ -32,11 +39,8 @@ class CopyableShareArtifact extends StatefulWidget { }); final String label; - final TextEditingController controller; - /// What the copy button puts on the clipboard. Taken from the caller rather - /// than from [controller], which is only how the value is displayed - and - /// which may be showing dots. + /// The value displayed, and the value Copy puts on the clipboard. final String text; final String copyLabel; @@ -53,8 +57,32 @@ class CopyableShareArtifact extends StatefulWidget { } class _CopyableShareArtifactState extends State { + late final TextEditingController _controller = + TextEditingController(text: widget.text); + bool _isRevealed = false; + @override + void didUpdateWidget(covariant CopyableShareArtifact oldWidget) { + super.didUpdateWidget(oldWidget); + + if (oldWidget.text != widget.text) { + _controller.text = widget.text; + + // A revealed secret stays revealed only for as long as it is the same + // secret. Ticking "include the key in the link" swaps a keyless link for + // one that carries the key, and that new value must not inherit the + // previous one's revealed state. + _isRevealed = false; + } + } + + @override + void dispose() { + _controller.dispose(); + super.dispose(); + } + @override Widget build(BuildContext context) { final typography = ArDriveTypographyNew.of(context); @@ -75,7 +103,7 @@ class _CopyableShareArtifactState extends State { // nothing. key: ValueKey(isMasked), label: widget.label, - controller: widget.controller, + controller: _controller, isEnabled: false, obscureText: isMasked, // Deliberately not the field's own `showObfuscationToggle`: it diff --git a/lib/components/drive_share_dialog.dart b/lib/components/drive_share_dialog.dart index 9c02f9f446..9bba89dc01 100644 --- a/lib/components/drive_share_dialog.dart +++ b/lib/components/drive_share_dialog.dart @@ -36,15 +36,6 @@ class DriveShareDialog extends StatefulWidget { } class DriveShareDialogState extends State { - final shareLinkController = TextEditingController(); - final driveKeyController = TextEditingController(); - - @override - void dispose() { - shareLinkController.dispose(); - driveKeyController.dispose(); - super.dispose(); - } @override void initState() { @@ -53,13 +44,7 @@ class DriveShareDialogState extends State { @override Widget build(BuildContext context) => - BlocConsumer( - listener: (context, state) { - if (state is DriveShareLoadSuccess) { - shareLinkController.text = state.driveShareLink.toString(); - driveKeyController.text = state.driveKeyBase64 ?? ''; - } - }, + BlocBuilder( builder: (context, state) { final typography = ArDriveTypographyNew.of(context); @@ -82,7 +67,6 @@ class DriveShareDialogState extends State { else if (state is DriveShareLoadSuccess) ...{ CopyableShareArtifact( label: appLocalizationsOf(context).shareFileLinkLabel, - controller: shareLinkController, text: state.driveShareLink.toString(), copyLabel: appLocalizationsOf(context).copyLink, revealLabel: @@ -98,7 +82,6 @@ class DriveShareDialogState extends State { CopyableShareArtifact( label: appLocalizationsOf(context) .shareDriveAccessKeyLabel, - controller: driveKeyController, text: state.driveKeyBase64!, copyLabel: appLocalizationsOf(context).copyAccessKey, revealLabel: diff --git a/lib/components/file_share_dialog.dart b/lib/components/file_share_dialog.dart index db51591b41..c314813fd0 100644 --- a/lib/components/file_share_dialog.dart +++ b/lib/components/file_share_dialog.dart @@ -42,28 +42,13 @@ class FileShareDialog extends StatefulWidget { } class FileShareDialogState extends State { - final shareLinkController = TextEditingController(); - final fileKeyController = TextEditingController(); - - @override - void dispose() { - shareLinkController.dispose(); - fileKeyController.dispose(); - super.dispose(); - } @override Widget build(BuildContext context) { final typography = ArDriveTypographyNew.of(context); final colorTokens = ArDriveTheme.of(context).themeData.colorTokens; - return BlocConsumer( - listener: (context, state) { - if (state is FileShareLoadSuccess) { - shareLinkController.text = state.fileShareLink.toString(); - fileKeyController.text = state.fileKeyBase64 ?? ''; - } - }, + return BlocBuilder( builder: (context, state) => ArDriveStandardModalNew( width: kLargeDialogWidth, scrollableContent: true, @@ -133,7 +118,6 @@ class FileShareDialogState extends State { ), CopyableShareArtifact( label: appLocalizationsOf(context).shareFileLinkLabel, - controller: shareLinkController, text: state.fileShareLink.toString(), copyLabel: appLocalizationsOf(context).copyLink, revealLabel: appLocalizationsOf(context).shareDriveRevealLink, @@ -155,7 +139,6 @@ class FileShareDialogState extends State { const SizedBox(height: 16), CopyableShareArtifact( label: appLocalizationsOf(context).shareFileAccessKeyLabel, - controller: fileKeyController, text: state.fileKeyBase64!, copyLabel: appLocalizationsOf(context).copyAccessKey, revealLabel: appLocalizationsOf(context).shareFileRevealKey, diff --git a/lib/pages/drive_detail/components/drive_explorer_item_tile.dart b/lib/pages/drive_detail/components/drive_explorer_item_tile.dart index 2d441817ef..1008143eb8 100644 --- a/lib/pages/drive_detail/components/drive_explorer_item_tile.dart +++ b/lib/pages/drive_detail/components/drive_explorer_item_tile.dart @@ -489,6 +489,22 @@ class _DriveExplorerItemTileTrailingState ), if (isOwner) hideFileDropdownItem(context, item), ], + ArDriveDropdownItem( + onClick: () { + promptToShareDrive( + context: context, + drive: widget.drive, + folderId: item.id, + ); + }, + content: _buildItem( + appLocalizationsOf(context).shareFolder, + ArDriveIcons.share( + size: defaultIconSize, + ), + height: height, + ), + ), ArDriveDropdownItem( onClick: () { final bloc = context.read(); diff --git a/test/components/copyable_share_artifact_test.dart b/test/components/copyable_share_artifact_test.dart index f10aebb9f6..168c2bffd0 100644 --- a/test/components/copyable_share_artifact_test.dart +++ b/test/components/copyable_share_artifact_test.dart @@ -11,10 +11,10 @@ void main() { child: MaterialApp(home: Scaffold(body: child)), ); - Widget artifact({required bool isSecret}) => CopyableShareArtifact( + Widget artifact({required bool isSecret, String text = secret}) => + CopyableShareArtifact( label: 'Access key', - controller: TextEditingController(text: secret), - text: secret, + text: text, copyLabel: 'Copy', revealLabel: 'Show access key', isSecret: isSecret, @@ -70,6 +70,39 @@ void main() { expect(isObscured(tester), isTrue); }); + testWidgets('a synchronously available value is shown, not swallowed', + (tester) async { + // The component owns its controller precisely so this holds. When each + // dialog filled a controller from a bloc listener instead, a cubit that + // reached success synchronously - which the public drive path does - + // never fired the listener, and the field rendered empty. + await tester.pumpWidget(wrap(artifact(isSecret: false, text: 'a-link'))); + + expect( + tester.widget(find.byType(EditableText)).controller.text, + 'a-link', + ); + }); + + testWidgets('revealing one secret does not reveal the next', + (tester) async { + // Ticking "include the key in the link" swaps the value underneath a + // revealed field. The new value must start masked again. + await tester.pumpWidget(wrap(artifact(isSecret: true))); + + await tester.tap(find.byTooltip('Show access key')); + await tester.pump(); + + expect(isObscured(tester), isFalse); + + await tester.pumpWidget( + wrap(artifact(isSecret: true, text: 'a-different-secret')), + ); + await tester.pump(); + + expect(isObscured(tester), isTrue); + }); + testWidgets('the value stays intact underneath the mask', (tester) async { // Masking is a display concern. If it ever reached the controller the // sharer would hand out a string of dots. diff --git a/test/pages/shared_file/shared_file_page_test.dart b/test/pages/shared_file/shared_file_page_test.dart index a242917bd0..10828872f3 100644 --- a/test/pages/shared_file/shared_file_page_test.dart +++ b/test/pages/shared_file/shared_file_page_test.dart @@ -72,7 +72,7 @@ void main() { name: name, parentFolderId: 'parent-folder-id', size: size, - lastModifiedDate: DateTime.utc(2024, 3, 3), + lastModifiedDate: DateTime.utc(2023, 11, 9), dataContentType: dataContentType, metadataTxId: 'metadata-tx', dataTxId: dataTxId, @@ -507,11 +507,19 @@ void main() { expect(find.text('File type'), findsOneWidget); expect(find.text('application/pdf'), findsOneWidget); + // Distinct dates, so each row is pinned to its own field rather than + // both matching one string. expect(find.text('Date created'), findsOneWidget); expect( find.text(formatDateToUtcString(DateTime.utc(2024, 3, 3))), findsWidgets, ); + + expect(find.text('Last updated'), findsOneWidget); + expect( + find.text(formatDateToUtcString(DateTime.utc(2023, 11, 9))), + findsOneWidget, + ); }); testWidgets('asks for the version history only when it is opened', From 6ea535adf6933bcf25a816f750f65fa7ff572217 Mon Sep 17 00:00:00 2001 From: vilenarios Date: Tue, 18 Aug 2026 19:15:44 -0400 Subject: [PATCH 05/13] test: scope the details drawer date assertions to their own rows PE-9210 The finders searched the whole tree, so created and modified could have been swapped between rows and the test would still have passed. Each value is now found under its own label's row, and the expected strings are literal rather than produced by formatDateToUtcString - deriving them from the formatter under test let a format change rewrite the production output and the expectation together. --- .../shared_file/shared_file_page_test.dart | 30 +++++++++++-------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/test/pages/shared_file/shared_file_page_test.dart b/test/pages/shared_file/shared_file_page_test.dart index 10828872f3..7389e76a1d 100644 --- a/test/pages/shared_file/shared_file_page_test.dart +++ b/test/pages/shared_file/shared_file_page_test.dart @@ -4,7 +4,6 @@ import 'package:ardrive/blocs/blocs.dart'; import 'package:ardrive/models/models.dart'; import 'package:ardrive/pages/shared_file/shared_file_key_session.dart'; import 'package:ardrive/pages/shared_file/shared_file_page.dart'; -import 'package:ardrive/utils/format_date.dart'; import 'package:ardrive/pages/shared_file/shared_file_ready_view.dart'; import 'package:ardrive/utils/filesize.dart'; import 'package:ardrive/utils/session_key_value_store.dart'; @@ -60,6 +59,15 @@ void main() { late MockSharedFileCubit cubit; late StreamController states; + /// The value rendered beside [label] in the details drawer, scoped to that + /// label's own row. + Finder detailRowValue(String label, String value) => find.descendant( + of: find + .ancestor(of: find.text(label), matching: find.byType(Row)) + .first, + matching: find.text(value), + ); + FileRevision fileRevision({ String name = 'Q3 Report.pdf', String dataTxId = 'data-tx-newest', @@ -504,20 +512,18 @@ void main() { await tester.tap(find.byType(ExpansionTile).first); await tester.pumpAndSettle(); - expect(find.text('File type'), findsOneWidget); - expect(find.text('application/pdf'), findsOneWidget); - - // Distinct dates, so each row is pinned to its own field rather than - // both matching one string. - expect(find.text('Date created'), findsOneWidget); + // Each value is scoped to the row its label is in, so two rows cannot + // satisfy each other's assertion - a swap between created and modified + // would otherwise still pass. The expected strings are literal rather + // than run through `formatDateToUtcString`, which would let a formatter + // change rewrite the production output and the expectation together. + expect(detailRowValue('File type', 'application/pdf'), findsOneWidget); expect( - find.text(formatDateToUtcString(DateTime.utc(2024, 3, 3))), - findsWidgets, + detailRowValue('Date created', '2024-03-03 00:00:00 GMT+0'), + findsOneWidget, ); - - expect(find.text('Last updated'), findsOneWidget); expect( - find.text(formatDateToUtcString(DateTime.utc(2023, 11, 9))), + detailRowValue('Last updated', '2023-11-09 00:00:00 GMT+0'), findsOneWidget, ); }); From 7fc8539afc1a47085b967ed697c9811d70760580 Mon Sep 17 00:00:00 2001 From: vilenarios Date: Tue, 18 Aug 2026 19:40:27 -0400 Subject: [PATCH 06/13] fix: four defects found reviewing the sharing changes line by line PE-9210 1970 in the details drawer. A v2 link carries no timestamps, so the revision the page paints from holds an epoch placeholder until the metadata resolves - a contract the cubit states explicitly ("rendered as unknown, never as 1970"). The new date rows ignored it and put 1970-01-01 in front of the recipient as though it were the upload date. They are now gated on detailsAreResolved, with a test that pumps the unresolved state. Copy that no longer matched the product. "Anyone can access this private drive using the link above" was written when the key was always embedded; for the keyless default it is simply false, and it is the sharer's only cue that they still have to send the key. A keyless private link now says so instead. The dialog never showed what was being shared. ArDriveStandardModalNew renders `description` only when `content` is null, and this dialog always has content, so the drive name passed to it was never on screen - and a folder share would have confirmed the drive's name rather than the folder's anyway. The name is now rendered in the content, and folders carry their own. Ghost folders offered a share action. Their metadata was never found, so a link naming one points the recipient at nothing. Also adds the drive share dialog's first widget tests. Its absence is why the empty public-drive link field reached review at all, and why the modal's dead `description` went unnoticed - both were found by writing them. --- lib/blocs/drive_share/drive_share_cubit.dart | 5 + lib/blocs/drive_share/drive_share_state.dart | 5 + lib/components/details_panel.dart | 1 + lib/components/drive_share_dialog.dart | 41 +++- lib/l10n/app_en.arb | 4 + .../components/drive_explorer_item_tile.dart | 35 ++-- .../shared_file/shared_file_ready_view.dart | 33 +++- test/blocs/drive_share_cubit_test.dart | 34 +++- test/components/drive_share_dialog_test.dart | 180 ++++++++++++++++++ .../shared_file/shared_file_page_test.dart | 38 +++- 10 files changed, 338 insertions(+), 38 deletions(-) create mode 100644 test/components/drive_share_dialog_test.dart diff --git a/lib/blocs/drive_share/drive_share_cubit.dart b/lib/blocs/drive_share/drive_share_cubit.dart index 4ec430feec..1545ad1819 100644 --- a/lib/blocs/drive_share/drive_share_cubit.dart +++ b/lib/blocs/drive_share/drive_share_cubit.dart @@ -18,6 +18,9 @@ class DriveShareCubit extends Cubit { /// The folder being shared, or `null` when the whole drive is. final FolderID? folderId; + /// The folder's own name, for the dialog to confirm what is being shared. + final String? folderName; + final ProfileCubit _profileCubit; final DriveDao _driveDao; @@ -35,6 +38,7 @@ class DriveShareCubit extends Cubit { DriveShareCubit({ required this.drive, this.folderId, + this.folderName, required DriveDao driveDao, required ProfileCubit profileCubit, }) : _driveDao = driveDao, @@ -103,6 +107,7 @@ class DriveShareCubit extends Cubit { drive: drive, driveShareLink: driveShareLink, isFolder: folderId != null, + folderName: folderName, keyIsInLink: keyIsInLink, // The key is offered as its own artifact so the sharer can send it // through a different channel than the link. diff --git a/lib/blocs/drive_share/drive_share_state.dart b/lib/blocs/drive_share/drive_share_state.dart index 121b57295e..4680a5a916 100644 --- a/lib/blocs/drive_share/drive_share_state.dart +++ b/lib/blocs/drive_share/drive_share_state.dart @@ -21,6 +21,9 @@ class DriveShareLoadSuccess extends DriveShareState { /// Whether the link points at one folder rather than the whole drive. final bool isFolder; + /// The shared folder's name, when a folder is what is being shared. + final String? folderName; + /// Whether the drive key is embedded in [driveShareLink]. final bool keyIsInLink; @@ -33,6 +36,7 @@ class DriveShareLoadSuccess extends DriveShareState { required this.drive, required this.driveShareLink, this.isFolder = false, + this.folderName, this.keyIsInLink = false, this.driveKeyBase64, }); @@ -45,6 +49,7 @@ class DriveShareLoadSuccess extends DriveShareState { drive, driveShareLink, isFolder, + folderName, keyIsInLink, driveKeyBase64, ]; diff --git a/lib/components/details_panel.dart b/lib/components/details_panel.dart index d1018dab2b..f7b1589b96 100644 --- a/lib/components/details_panel.dart +++ b/lib/components/details_panel.dart @@ -1211,6 +1211,7 @@ class DetailsPanelToolbar extends StatelessWidget { context: context, drive: drive, folderId: item.id, + folderName: item.name, ); } else if (item is DriveDataItem) { promptToShareDrive( diff --git a/lib/components/drive_share_dialog.dart b/lib/components/drive_share_dialog.dart index 9bba89dc01..b9c8c7327c 100644 --- a/lib/components/drive_share_dialog.dart +++ b/lib/components/drive_share_dialog.dart @@ -9,10 +9,15 @@ import 'package:flutter/material.dart'; import 'package:flutter_bloc/flutter_bloc.dart'; /// Shares [drive], or - when [folderId] is given - one folder inside it. +/// +/// [folderName] is what the dialog shows the sharer they are sharing. Without +/// it a folder share is described by its *drive's* name, which is the wrong +/// thing to confirm before copying a link. Future promptToShareDrive({ required BuildContext context, required Drive drive, String? folderId, + String? folderName, }) => showArDriveDialog( context, @@ -20,6 +25,7 @@ Future promptToShareDrive({ create: (_) => DriveShareCubit( drive: drive, folderId: folderId, + folderName: folderName, driveDao: context.read(), profileCubit: context.read(), ), @@ -36,12 +42,6 @@ class DriveShareDialog extends StatefulWidget { } class DriveShareDialogState extends State { - - @override - void initState() { - super.initState(); - } - @override Widget build(BuildContext context) => BlocBuilder( @@ -53,8 +53,6 @@ class DriveShareDialogState extends State { title: state is DriveShareLoadSuccess && state.isFolder ? appLocalizationsOf(context).shareFolderWithOthers : appLocalizationsOf(context).shareDriveWithOthers, - description: - state is DriveShareLoadSuccess ? state.drive.name : null, scrollableContent: true, content: SizedBox( width: kLargeDialogWidth, @@ -65,6 +63,23 @@ class DriveShareDialogState extends State { if (state is DriveShareLoadInProgress) const Center(child: CircularProgressIndicator()) else if (state is DriveShareLoadSuccess) ...{ + // What is being shared, confirmed before the sharer copies + // anything. Not the modal's `description`, which it only + // renders when there is no `content` - so this dialog never + // showed one. + Padding( + padding: const EdgeInsets.only(bottom: 16), + child: Text( + state.folderName ?? state.drive.name, + style: typography.paragraphNormal( + fontWeight: ArFontWeight.semiBold, + color: ArDriveTheme.of(context) + .themeData + .colorTokens + .textHigh, + ), + ), + ), CopyableShareArtifact( label: appLocalizationsOf(context).shareFileLinkLabel, text: state.driveShareLink.toString(), @@ -140,11 +155,17 @@ class DriveShareDialogState extends State { }, const SizedBox(height: 16), Text( + // A keyless private link does *not* grant access on its + // own, so it must not be described as though it does - + // that copy predates the key ever being optional. state.drive.isPublic ? appLocalizationsOf(context) .anyoneCanAccessThisDrivePublic - : appLocalizationsOf(context) - .anyoneCanAccessThisDrivePrivate, + : state.keyIsInLink + ? appLocalizationsOf(context) + .anyoneCanAccessThisDrivePrivate + : appLocalizationsOf(context) + .shareDriveKeylessNotice, style: typography.paragraphLarge(), ), } else if (state is DriveShareLoadFail) diff --git a/lib/l10n/app_en.arb b/lib/l10n/app_en.arb index 7b48e3ab8e..9a2e51a45e 100644 --- a/lib/l10n/app_en.arb +++ b/lib/l10n/app_en.arb @@ -2342,6 +2342,10 @@ "@shareFolder": { "description": "The action of sharing a link to a folder" }, + "shareDriveKeylessNotice": "The link alone won’t open this drive. The person you send it to also needs the access key above.", + "@shareDriveKeylessNotice": { + "description": "Explains that a private drive link without the key embedded needs the key sent separately" + }, "shareDriveAccessKeyLabel": "Drive access key", "@shareDriveAccessKeyLabel": { "description": "Label of the field holding the drive key handed over alongside a private drive link" diff --git a/lib/pages/drive_detail/components/drive_explorer_item_tile.dart b/lib/pages/drive_detail/components/drive_explorer_item_tile.dart index 1008143eb8..777132665c 100644 --- a/lib/pages/drive_detail/components/drive_explorer_item_tile.dart +++ b/lib/pages/drive_detail/components/drive_explorer_item_tile.dart @@ -489,22 +489,26 @@ class _DriveExplorerItemTileTrailingState ), if (isOwner) hideFileDropdownItem(context, item), ], - ArDriveDropdownItem( - onClick: () { - promptToShareDrive( - context: context, - drive: widget.drive, - folderId: item.id, - ); - }, - content: _buildItem( - appLocalizationsOf(context).shareFolder, - ArDriveIcons.share( - size: defaultIconSize, + // Not offered for a ghost folder: its metadata was never found, so a + // link naming it points the recipient at nothing. + if (!item.isGhostFolder) + ArDriveDropdownItem( + onClick: () { + promptToShareDrive( + context: context, + drive: widget.drive, + folderId: item.id, + folderName: item.name, + ); + }, + content: _buildItem( + appLocalizationsOf(context).shareFolder, + ArDriveIcons.share( + size: defaultIconSize, + ), + height: height, ), - height: height, ), - ), ArDriveDropdownItem( onClick: () { final bloc = context.read(); @@ -913,13 +917,14 @@ class EntityActionsMenu extends StatelessWidget { ], // Guarded rather than assumed: this menu is also built in places that // do not know the drive, and a folder link needs it for the key. - if (drive != null) + if (drive != null && !item.isGhostFolder) ArDriveDropdownItem( onClick: () { promptToShareDrive( context: context, drive: drive!, folderId: item.id, + folderName: item.name, ); }, content: _buildItem( diff --git a/lib/pages/shared_file/shared_file_ready_view.dart b/lib/pages/shared_file/shared_file_ready_view.dart index 9ad81b78a1..d658ff2a66 100644 --- a/lib/pages/shared_file/shared_file_ready_view.dart +++ b/lib/pages/shared_file/shared_file_ready_view.dart @@ -298,6 +298,7 @@ class _SharedFileReadyViewState extends State { revision: revision, ownerAddress: state.ownerAddress ?? payload?.ownerAddress, licenseName: state.latestLicense?.meta.nameWithShortName, + detailsAreResolved: state.detailsAreResolved, ), SharedFileVersionsDrawer( revisions: state.activityRevisions, @@ -838,12 +839,22 @@ class SharedFileDetailsDrawer extends StatelessWidget { required this.revision, this.ownerAddress, this.licenseName, + this.detailsAreResolved = true, }); final FileRevision revision; final String? ownerAddress; final String? licenseName; + /// Whether [revision] holds the file's own record rather than what the link + /// claimed. + /// + /// A v2 link carries no timestamps, so until the metadata resolves the dates + /// on [revision] are the epoch placeholder. Showing those would put + /// "1970-01-01" in front of a recipient as though it were the upload date, + /// which is worse than showing nothing. + final bool detailsAreResolved; + @override Widget build(BuildContext context) { final ownerAddress = this.ownerAddress; @@ -866,16 +877,18 @@ class SharedFileDetailsDrawer extends StatelessWidget { value: contentType, canCopy: false, ), - _SharedFileDetailRow( - label: appLocalizationsOf(context).dateCreated, - value: formatDateToUtcString(revision.dateCreated), - canCopy: false, - ), - _SharedFileDetailRow( - label: appLocalizationsOf(context).lastUpdated, - value: formatDateToUtcString(revision.lastModifiedDate), - canCopy: false, - ), + if (detailsAreResolved) ...[ + _SharedFileDetailRow( + label: appLocalizationsOf(context).dateCreated, + value: formatDateToUtcString(revision.dateCreated), + canCopy: false, + ), + _SharedFileDetailRow( + label: appLocalizationsOf(context).lastUpdated, + value: formatDateToUtcString(revision.lastModifiedDate), + canCopy: false, + ), + ], _SharedFileDetailRow( label: appLocalizationsOf(context).fileID, value: revision.fileId, diff --git a/test/blocs/drive_share_cubit_test.dart b/test/blocs/drive_share_cubit_test.dart index 4422b48821..3b74012d4e 100644 --- a/test/blocs/drive_share_cubit_test.dart +++ b/test/blocs/drive_share_cubit_test.dart @@ -41,9 +41,11 @@ void main() { Future drive() => driveDao.driveById(driveId: driveId).getSingle(); - DriveShareCubit cubit(Drive d, {String? folderId}) => DriveShareCubit( + DriveShareCubit cubit(Drive d, {String? folderId, String? folderName}) => + DriveShareCubit( drive: d, folderId: folderId, + folderName: folderName, driveDao: driveDao, profileCubit: profileCubit, ); @@ -242,6 +244,36 @@ void main() { expect(link, contains('driveKey=')); }); + test('a folder share carries the folder\'s own name', () async { + // The dialog confirms what is being shared. Described by the drive's + // name, a folder share would confirm the wrong thing. + const folderId = 'c4d9dc2c-5d4c-6e3d-ac4f-8f3c2d5e6f70'; + + await insertDrive(isPrivate: false); + + final success = await settled( + cubit(await drive(), folderId: folderId, folderName: 'Q4 Photos'), + ) as DriveShareLoadSuccess; + + expect(success.folderName, 'Q4 Photos'); + expect(success.isFolder, isTrue); + }); + + test('the drive key never reaches the state\'s stringification', () async { + // Equatable stringifies props in debug builds, and one of them is a key. + when(() => profileCubit.state).thenReturn(ProfilePromptAdd()); + + await insertDrive(isPrivate: true); + await driveDao.putDriveKeyInMemory(driveID: driveId, driveKey: driveKey); + + final success = + await settled(cubit(await drive())) as DriveShareLoadSuccess; + + expect(success.driveKeyBase64, isNotNull); + expect(success.toString(), contains('')); + expect(success.toString(), isNot(contains(success.driveKeyBase64!))); + }); + test('a private drive resolves to a link carrying its key', () async { // Signed out, so the key comes from the in-memory vault - the path a // drive attached this session but never persisted takes. diff --git a/test/components/drive_share_dialog_test.dart b/test/components/drive_share_dialog_test.dart new file mode 100644 index 0000000000..2cdfc93cc8 --- /dev/null +++ b/test/components/drive_share_dialog_test.dart @@ -0,0 +1,180 @@ +import 'package:ardrive/blocs/blocs.dart'; +import 'package:ardrive/components/drive_share_dialog.dart'; +import 'package:ardrive/models/models.dart'; +import 'package:ardrive_utils/ardrive_utils.dart'; +import 'package:ardrive_ui/ardrive_ui.dart'; +import 'package:bloc_test/bloc_test.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter_bloc/flutter_bloc.dart'; +import 'package:flutter_gen/gen_l10n/app_localizations.dart'; +import 'package:flutter_localizations/flutter_localizations.dart'; +import 'package:flutter_test/flutter_test.dart'; + +class MockDriveShareCubit extends MockCubit + implements DriveShareCubit {} + +/// What the share drive dialog actually puts on screen. +/// +/// These exist because the dialog had no test at all, and the gap showed: it +/// filled its fields from a bloc listener, which never fires for the state a +/// bloc is already in, so a public drive - whose link is built synchronously - +/// rendered an empty link field. +void main() { + const driveId = 'a2b7ba0a-3b2a-4c1b-8a2f-6d1a0b3c4d5e'; + const driveKeyBase64 = 'X123YZAB-CD4e5fgHIjKlmN6O7pqrStuVwxYzaBcd8E'; + + late MockDriveShareCubit cubit; + + Drive drive({required bool isPrivate}) => Drive( + id: driveId, + name: 'My Drive', + ownerAddress: 'owner', + rootFolderId: 'root', + privacy: + isPrivate ? DrivePrivacyTag.private : DrivePrivacyTag.public, + isHidden: false, + dateCreated: DateTime.utc(2026, 1, 1), + lastUpdated: DateTime.utc(2026, 1, 1), + ); + + Widget wrap(Widget child) => ArDriveTheme( + themeData: lightTheme(), + child: MaterialApp( + localizationsDelegates: const [ + AppLocalizations.delegate, + GlobalMaterialLocalizations.delegate, + GlobalWidgetsLocalizations.delegate, + GlobalCupertinoLocalizations.delegate, + ], + supportedLocales: const [Locale('en', '')], + home: Scaffold(body: child), + ), + ); + + Future pumpState(WidgetTester tester, DriveShareState state) async { + tester.view.physicalSize = const Size(1200, 1600); + tester.view.devicePixelRatio = 1.0; + addTearDown(tester.view.reset); + + whenListen(cubit, const Stream.empty(), + initialState: state); + + await tester.pumpWidget( + wrap( + BlocProvider.value( + value: cubit, + child: const DriveShareDialog(), + ), + ), + ); + await tester.pump(); + } + + /// The value shown in a field, whether or not it is masked. + String fieldText(WidgetTester tester, int index) => + tester.widgetList(find.byType(EditableText)).toList()[index] + .controller + .text; + + setUp(() => cubit = MockDriveShareCubit()); + + group('DriveShareDialog', () { + testWidgets('a public drive shows its link', (tester) async { + // The regression that started this file: the public path builds its link + // synchronously, so the dialog was already in the success state before + // it could listen for one, and the field came up blank. + await pumpState( + tester, + DriveShareLoadSuccess( + drive: drive(isPrivate: false), + driveShareLink: + Uri.parse('https://app.ardrive.io/#/drives/$driveId?name=My+Drive'), + ), + ); + + expect( + fieldText(tester, 0), + 'https://app.ardrive.io/#/drives/$driveId?name=My+Drive', + ); + }); + + testWidgets('a keyless private drive does not claim the link is enough', + (tester) async { + // The old copy - "Anyone can access this private drive using the link + // above" - was written when the key was always embedded. For the keyless + // default it is simply false, and it is the sharer's only cue about what + // they still have to send. + await pumpState( + tester, + DriveShareLoadSuccess( + drive: drive(isPrivate: true), + driveShareLink: + Uri.parse('https://app.ardrive.io/#/drives/$driveId'), + driveKeyBase64: driveKeyBase64, + ), + ); + + expect( + find.textContaining('Anyone can access this private drive'), + findsNothing, + ); + expect(find.textContaining('The link alone'), findsOneWidget); + + // And the key is offered as its own artifact to send separately. + expect(find.text('Drive access key'), findsOneWidget); + expect(fieldText(tester, 1), driveKeyBase64); + }); + + testWidgets('embedding the key retires the separate artifact', + (tester) async { + await pumpState( + tester, + DriveShareLoadSuccess( + drive: drive(isPrivate: true), + driveShareLink: Uri.parse( + 'https://app.ardrive.io/#/drives/$driveId?driveKey=$driveKeyBase64', + ), + driveKeyBase64: driveKeyBase64, + keyIsInLink: true, + ), + ); + + expect(find.text('Drive access key'), findsNothing); + expect( + find.textContaining('Anyone can access this private drive'), + findsOneWidget, + ); + }); + + testWidgets('a folder share is named after the folder, not the drive', + (tester) async { + await pumpState( + tester, + DriveShareLoadSuccess( + drive: drive(isPrivate: false), + driveShareLink: Uri.parse( + 'https://app.ardrive.io/#/drives/$driveId/folders/folder-id', + ), + isFolder: true, + folderName: 'Q4 Photos', + ), + ); + + expect(find.text('Share folder with others'), findsOneWidget); + expect(find.text('Q4 Photos'), findsOneWidget); + expect(find.text('My Drive'), findsNothing); + }); + + testWidgets('a failure is shown, with a way out of it', (tester) async { + // The state that used to be unreachable: the dialog spun forever instead. + await pumpState(tester, const DriveShareLoadFail()); + + expect(find.byType(CircularProgressIndicator), findsNothing); + expect( + find.textContaining('couldn’t create a share link'), + findsOneWidget, + ); + expect(find.text('Try Again'), findsOneWidget); + }); + }); +} diff --git a/test/pages/shared_file/shared_file_page_test.dart b/test/pages/shared_file/shared_file_page_test.dart index 7389e76a1d..a95b977c77 100644 --- a/test/pages/shared_file/shared_file_page_test.dart +++ b/test/pages/shared_file/shared_file_page_test.dart @@ -73,6 +73,8 @@ void main() { String dataTxId = 'data-tx-newest', int size = 4821133, String? dataContentType = 'application/pdf', + DateTime? lastModifiedDate, + DateTime? dateCreated, }) { return FileRevision( fileId: fileId, @@ -80,11 +82,11 @@ void main() { name: name, parentFolderId: 'parent-folder-id', size: size, - lastModifiedDate: DateTime.utc(2023, 11, 9), + lastModifiedDate: lastModifiedDate ?? DateTime.utc(2023, 11, 9), dataContentType: dataContentType, metadataTxId: 'metadata-tx', dataTxId: dataTxId, - dateCreated: DateTime.utc(2024, 3, 3), + dateCreated: dateCreated ?? DateTime.utc(2024, 3, 3), action: RevisionAction.create, isHidden: false, ); @@ -607,6 +609,38 @@ void main() { expect(find.text('Preview'), findsNothing); }); + testWidgets('shows no dates until the file\'s own record has them', + (tester) async { + // A v2 link carries no timestamps, so the revision it paints from holds + // the epoch placeholder until the metadata resolves. Rendering that puts + // "1970-01-01" in front of a recipient as though it were the upload + // date - worse than showing nothing, and worse than the placeholder was + // ever meant to be. + await pumpPage( + tester, + SharedFileLoadSuccess( + fileRevisions: [ + fileRevision( + lastModifiedDate: DateTime.fromMillisecondsSinceEpoch(0), + dateCreated: DateTime.fromMillisecondsSinceEpoch(0), + ), + ], + verification: LinkVerification.pending, + detailsAreResolved: false, + ), + ); + + await tester.tap(find.byType(ExpansionTile).first); + await tester.pumpAndSettle(); + + expect(find.text('Date created'), findsNothing); + expect(find.text('Last updated'), findsNothing); + expect(find.textContaining('1970'), findsNothing); + + // The type still shows: the link really did carry it. + expect(find.text('File type'), findsOneWidget); + }); + testWidgets('shows the verification badge the link earned', (tester) async { await pumpPage(tester, success(verification: LinkVerification.verified)); From b3aa88b68f708a9c5ff500658a3ec17ce1484a81 Mon Sep 17 00:00:00 2001 From: vilenarios Date: Tue, 18 Aug 2026 20:11:52 -0400 Subject: [PATCH 07/13] perf: stop the share dialogs doing work the sharer has to wait through PE-9210 Toggling "include the key in the link" re-ran the whole load: it re-announced DriveShareLoadInProgress, which the dialog renders as a centred spinner, so the dialog blanked and the checkbox the sharer had just clicked left the screen and came back. It also went back to the database for a drive key that cannot have changed. The key is now resolved once and the toggle rebuilds the link with no loading state, which is what the file share dialog has always done. The sharer's one network read - the cipher tags that fill `c`/`iv` - is now bounded. GraphQLRetry retries a call that throws but sets no timeout, so a gateway that hung left the dialog saying "finishing your link" for as long as it stayed open, telling the sharer to wait for something already done. The link is complete and copyable without those two fields; they only save the recipient a lookup. --- lib/blocs/drive_share/drive_share_cubit.dart | 31 ++++++++++++++++++-- lib/blocs/file_share/file_share_cubit.dart | 14 ++++++++- test/blocs/drive_share_cubit_test.dart | 31 ++++++++++++++++++++ 3 files changed, 72 insertions(+), 4 deletions(-) diff --git a/lib/blocs/drive_share/drive_share_cubit.dart b/lib/blocs/drive_share/drive_share_cubit.dart index 1545ad1819..dc46647a68 100644 --- a/lib/blocs/drive_share/drive_share_cubit.dart +++ b/lib/blocs/drive_share/drive_share_cubit.dart @@ -35,6 +35,17 @@ class DriveShareCubit extends Cubit { /// [SharedFileCubit] uses. int _generation = 0; + /// The drive's key, resolved once. + /// + /// Toggling the checkbox rebuilds the link from what is already in hand: it + /// must not go back to the database for a key that cannot have changed, and + /// it must not blank the dialog to a spinner - which would take the very + /// control the sharer just clicked off the screen. The file share dialog + /// rebuilds its link with no loading state at all, and this one should feel + /// the same. Only populated on success, so a retry after a failure resolves + /// afresh. + DriveKey? _driveKeyCache; + DriveShareCubit({ required this.drive, this.folderId, @@ -55,7 +66,8 @@ class DriveShareCubit extends Cubit { _keyIsInLink = value; - await loadDriveShareDetails(); + // No progress state: this is a rebuild, not a load. + await _buildShareDetails(announceProgress: false); } /// Builds the share link for [drive], or fails in a way the dialog can show. @@ -65,7 +77,10 @@ class DriveShareCubit extends Cubit { /// throws becomes an unhandled asynchronous error: the cubit would stay in /// [DriveShareLoadInProgress] and the dialog would spin forever, which is /// exactly what a missing drive key used to do. - Future loadDriveShareDetails() async { + Future loadDriveShareDetails() => + _buildShareDetails(announceProgress: true); + + Future _buildShareDetails({required bool announceProgress}) async { final generation = ++_generation; // Captured once. Read twice - once to build the link and once to describe @@ -74,7 +89,9 @@ class DriveShareCubit extends Cubit { // render a key-bearing link unmasked. final keyIsInLink = _keyIsInLink; - emit(DriveShareLoadInProgress()); + if (announceProgress) { + emit(DriveShareLoadInProgress()); + } try { final driveKey = drive.isPrivate ? await _driveKey() : null; @@ -138,6 +155,12 @@ class DriveShareCubit extends Cubit { /// Neither is guaranteed, and a [StateError] here is a real outcome rather /// than a should-never-happen: it lands on the failure state above. Future _driveKey() async { + final cached = _driveKeyCache; + + if (cached != null) { + return cached; + } + final profileState = _profileCubit.state; final driveKey = profileState is ProfileLoggedIn @@ -148,6 +171,8 @@ class DriveShareCubit extends Cubit { throw StateError('Drive key not found'); } + _driveKeyCache = driveKey; + return driveKey; } } diff --git a/lib/blocs/file_share/file_share_cubit.dart b/lib/blocs/file_share/file_share_cubit.dart index fb98153c8c..19f0ce0e4f 100644 --- a/lib/blocs/file_share/file_share_cubit.dart +++ b/lib/blocs/file_share/file_share_cubit.dart @@ -184,6 +184,10 @@ class FileShareCubit extends Cubit { _emitLoadSuccess(); } + /// How long the one network read on this side may take before the link is + /// presented as final without `c`/`iv`. + static const _cipherDetailsTimeout = Duration(seconds: 10); + /// Fetches `c`/`iv` - the only two link fields that are not in the local /// database - and folds them into the link when they arrive. /// @@ -192,7 +196,15 @@ class FileShareCubit extends Cubit { /// link built before this schema does today (§1.2). Future _loadCipherDetails(String dataTxId) async { try { - final dataTx = await _arweave.getTransactionDetails(dataTxId); + // Bounded. `GraphQLRetry` retries a call that throws but sets no + // timeout, so a gateway that simply hangs would leave the dialog saying + // "finishing your link" for as long as it stayed open - telling the + // sharer to wait for something that is already done. The link is + // complete and copyable without `c`/`iv`; those two fields only save the + // recipient one lookup. + final dataTx = await _arweave + .getTransactionDetails(dataTxId) + .timeout(_cipherDetailsTimeout); if (isClosed) { return; diff --git a/test/blocs/drive_share_cubit_test.dart b/test/blocs/drive_share_cubit_test.dart index 3b74012d4e..9d38d1bc35 100644 --- a/test/blocs/drive_share_cubit_test.dart +++ b/test/blocs/drive_share_cubit_test.dart @@ -244,6 +244,37 @@ void main() { expect(link, contains('driveKey=')); }); + test('toggling the key rebuilds the link without a loading state', + ( + ) async { + // The dialog renders a centred spinner for DriveShareLoadInProgress, so + // re-announcing it here would blank the dialog and take the checkbox the + // sharer just clicked off the screen. The file share dialog rebuilds its + // link with no loading state; this one matches. + when(() => profileCubit.state).thenReturn(ProfilePromptAdd()); + + await insertDrive(isPrivate: true); + await driveDao.putDriveKeyInMemory(driveID: driveId, driveKey: driveKey); + + final c = cubit(await drive()); + + await settled(c); + + final states = []; + final subscription = c.stream.listen(states.add); + + await c.setKeyIsInLink(true); + await Future.delayed(Duration.zero); + await subscription.cancel(); + + expect(states, isNot(contains(isA()))); + expect(states.last, isA()); + expect( + (states.last as DriveShareLoadSuccess).driveShareLink.toString(), + contains('driveKey='), + ); + }); + test('a folder share carries the folder\'s own name', () async { // The dialog confirms what is being shared. Described by the drive's // name, a folder share would confirm the wrong thing. From 65c5c24f162f9156f2e4b097cffabc7232492144 Mon Sep 17 00:00:00 2001 From: vilenarios Date: Tue, 18 Aug 2026 20:30:17 -0400 Subject: [PATCH 08/13] fix: gate the third folder share entry point on ghost folders too PE-9210 The two dropdown menus refuse to share a ghost folder - its metadata was never found, so a link naming it points the recipient at nothing - but the details panel's share icon, the third entry point, did not. Now all three agree. Hoisting `item` to a local for the promotion this needs also made four existing casts in the same method redundant; they are gone. --- lib/components/details_panel.dart | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/lib/components/details_panel.dart b/lib/components/details_panel.dart index f7b1589b96..2ec973ba54 100644 --- a/lib/components/details_panel.dart +++ b/lib/components/details_panel.dart @@ -1170,6 +1170,9 @@ class DetailsPanelToolbar extends StatelessWidget { @override Widget build(BuildContext context) { final drive = driveDetailLoadSuccess.currentDrive; + // A local so that type promotion works on it: a widget field cannot be + // promoted, and the share icon has to ask a folder whether it is a ghost. + final item = this.item; return Container( padding: const EdgeInsets.symmetric(vertical: 12), @@ -1192,7 +1195,10 @@ class DetailsPanelToolbar extends StatelessWidget { ), if (item is FileDataTableItem || item is DriveDataItem || - item is FolderDataTableItem) + // Not a ghost folder: its metadata was never found, so a link + // naming it points the recipient at nothing. The two dropdown + // menus gate this the same way. + (item is FolderDataTableItem && !item.isGhostFolder)) _buildActionIcon( tooltip: _getShareTooltip(item, context), icon: ArDriveIcons.share(size: defaultIconSize), @@ -1228,15 +1234,15 @@ class DetailsPanelToolbar extends StatelessWidget { if (item is FileDataTableItem) { promptToDownloadProfileFile( context: context, - file: item as FileDataTableItem, + file: item, ); } else if (item is FolderDataTableItem) { promptToDownloadMultipleFiles(context, - selectedItems: [item as FolderDataTableItem], + selectedItems: [item], zipName: item.name); } else if (item is DriveDataItem) { promptToDownloadMultipleFiles(context, - selectedItems: [item as DriveDataItem], + selectedItems: [item], zipName: item.name); } }), @@ -1246,7 +1252,7 @@ class DetailsPanelToolbar extends StatelessWidget { icon: ArDriveIcons.newWindow(size: defaultIconSize), onTap: () { final bloc = context.read(); - bloc.launchPreview((item as FileDataTableItem).dataTxId); + bloc.launchPreview(item.dataTxId); }, ), if (isDriveOwner(context.read(), drive.ownerAddress)) From 9401f030d3bbb2f3ece26a51c7037c59ff9c77a3 Mon Sep 17 00:00:00 2001 From: vilenarios Date: Tue, 18 Aug 2026 21:34:44 -0400 Subject: [PATCH 09/13] fix: make a folder link open the folder, and a bad keyed link say so PE-9210 A folder link only ever opened the folder for someone who already had the drive. Everyone else - which is every stranger a public folder link is sent to - went through the attach flow, which ends by clearing driveId so the prompt cannot re-fire; the drive is then selected fresh, the folder reads as belonging to a different drive, and it is dropped. A folder link was a drive link with extra characters. The delegate now holds the drive a folder link named until that drive is the selected one, and releases it immediately after, so ordinary navigation still discards a folder as it always has. The attach form no longer goes silent on a link whose key is wrong. Auto-attach was gated on having a name as well as a key, so a link that could not resolve one just sat there with the key filled in and nothing said why. It is gated on the key alone now, and submit() surfaces the real outcome through the states it already emits - DriveAttachInvalidDriveKey, DriveAttachDriveNotFound. What made that gate necessary was submit() freezing the name before the loaders that resolve it, so an empty one would have been persisted. It now reads the typed name up front and falls back to the resolved one when nothing was typed: a hand-typed name still wins, an absent one is no longer mistaken for a choice. That also drops the extra lookup the previous commit added, since submit() resolves the name on its way through anyway. --- .../drive_attach/drive_attach_cubit.dart | 52 +++++++------------ lib/pages/app_router_delegate.dart | 29 ++++++++++- 2 files changed, 47 insertions(+), 34 deletions(-) diff --git a/lib/blocs/drive_attach/drive_attach_cubit.dart b/lib/blocs/drive_attach/drive_attach_cubit.dart index 0c9668fe0c..4dc25e00b1 100644 --- a/lib/blocs/drive_attach/drive_attach_cubit.dart +++ b/lib/blocs/drive_attach/drive_attach_cubit.dart @@ -90,39 +90,16 @@ class DriveAttachCubit extends Cubit { ); } - // A private share link no longer carries the drive's name - it is a - // secret, and it is recoverable from the drive's own record once the - // key is in hand. Resolve it here so the auto-attach below still - // has one. + // The key is the whole precondition. A private share link no longer + // carries the drive's name - it is a secret, and `submit()` resolves + // the real one from the drive's own record on its way through. // - // Deliberately resolved *before* `submit()` rather than inside it: - // `submit()` reads the name controller up front so that a name the - // user typed by hand is the one the drive is attached under, and - // moving that read later would silently discard it. - if (driveNameController.text.isEmpty && - driveKeyController.text.isNotEmpty) { - // Guarded: this runs inside a microtask started from the - // constructor whose future nobody awaits, so a network or decode - // failure in here would otherwise surface as an unhandled - // asynchronous error. A name that will not resolve is not fatal - - // the auto-submit below is simply skipped and the form stays open - // for the recipient to act on, which is what a keyless link - // already does. - try { - await driveNameLoader(); - } catch (e, stacktrace) { - logger.e( - 'Failed to resolve the name of the shared drive', - e, - stacktrace, - ); - } - - if (isClosed) return; - } - - if (driveNameController.text.isNotEmpty && - driveKeyController.text.isNotEmpty) { + // Gating this on the name as well used to mean that a link whose key + // was wrong, or whose drive could not be found, simply sat there: + // auto-attach was skipped and nothing said why. Letting `submit()` + // run surfaces those through the states it already emits - + // `DriveAttachInvalidDriveKey` and `DriveAttachDriveNotFound`. + if (driveKeyController.text.isNotEmpty) { submit(); } } @@ -136,7 +113,12 @@ class DriveAttachCubit extends Cubit { void submit() async { final driveId = driveIdController.text; - final driveName = driveNameController.text; + + // Read before the loaders below overwrite the field with the drive's own + // name. A name typed by hand is the one the drive is attached under; an + // empty one is not a preference, so it falls back to whatever the chain + // says once the loaders have run. + final typedDriveName = driveNameController.text; try { final previousState = state; @@ -186,6 +168,10 @@ class DriveAttachCubit extends Cubit { return; } + final driveName = typedDriveName.isNotEmpty + ? typedDriveName + : driveNameController.text; + await _driveDao.writeDriveEntity( name: driveName, entity: driveEntity, diff --git a/lib/pages/app_router_delegate.dart b/lib/pages/app_router_delegate.dart index 7648337be8..458e210dab 100644 --- a/lib/pages/app_router_delegate.dart +++ b/lib/pages/app_router_delegate.dart @@ -44,6 +44,19 @@ class AppRouterDelegate extends RouterDelegate String? driveName; String? driveFolderId; + /// The drive a folder link named, held until that drive is the selected one. + /// + /// A folder link opened by someone who does not have the drive yet goes + /// through the attach flow, which ends by clearing [driveId] so the prompt + /// cannot re-fire. The drive is then selected fresh, [driveFolderId] reads as + /// belonging to a different drive, and the folder the link named is dropped - + /// so every recipient who was not already in the drive landed at its root, + /// which made a folder link a drive link with extra characters. + /// + /// One shot: cleared as soon as that drive is selected, so ordinary + /// navigation away from the drive still discards the folder as it always has. + String? _pendingFolderDriveId; + DriveKey? sharedDriveKey; String? sharedRawDriveKey; @@ -198,10 +211,21 @@ class AppRouterDelegate extends RouterDelegate if (state is DrivesLoadSuccess) { final selectedDriveChanged = driveId != state.selectedDriveId; - if (selectedDriveChanged) { + + // The drive a folder link named has just become the + // selected one, so its folder is not stale - it is the + // whole point of the link. + final isTheLinkedDrive = _pendingFolderDriveId != null && + _pendingFolderDriveId == state.selectedDriveId; + + if (selectedDriveChanged && !isTheLinkedDrive) { driveFolderId = null; } + if (isTheLinkedDrive) { + _pendingFolderDriveId = null; + } + driveId = state.selectedDriveId; notifyListeners(); } @@ -386,6 +410,8 @@ class AppRouterDelegate extends RouterDelegate driveId = configuration.driveId; driveName = configuration.driveName; driveFolderId = configuration.driveFolderId; + _pendingFolderDriveId = + configuration.driveFolderId == null ? null : configuration.driveId; sharedDriveKey = configuration.sharedDriveKey; sharedRawDriveKey = configuration.sharedRawDriveKey; sharedFileId = configuration.sharedFileId; @@ -404,6 +430,7 @@ class AppRouterDelegate extends RouterDelegate driveId = null; driveName = null; driveFolderId = null; + _pendingFolderDriveId = null; sharedDriveKey = null; sharedRawDriveKey = null; sharedFileId = null; From 24457272ef1bcab4319cd4810d50b09bcdbfbf8e Mon Sep 17 00:00:00 2001 From: vilenarios Date: Tue, 18 Aug 2026 23:38:35 -0400 Subject: [PATCH 10/13] test: cover the folder a link names surviving the drive being attached PE-9210 The reconciliation lived in an inline BlocListener closure, which is why it shipped untested while everything else here has coverage. It is a named method on the delegate now, called by that listener, so it can be exercised without standing up the whole app shell. Seven tests: a folder link opens its folder both for someone who already had the drive and for someone the attach flow ran for; the pending folder is released once honored, so returning to that drive later lands at its root; it never follows the user to a different drive; a drive link holds nothing back; and ordinary navigation discards or keeps the folder exactly as before. Verified to bite - reverting the fix fails the first of them. The other six pass either way by design: they fence the change in rather than prove it. --- lib/pages/app_router_delegate.dart | 48 +++++---- test/pages/app_router_delegate_test.dart | 122 +++++++++++++++++++++++ 2 files changed, 152 insertions(+), 18 deletions(-) create mode 100644 test/pages/app_router_delegate_test.dart diff --git a/lib/pages/app_router_delegate.dart b/lib/pages/app_router_delegate.dart index 458e210dab..2af9cd216b 100644 --- a/lib/pages/app_router_delegate.dart +++ b/lib/pages/app_router_delegate.dart @@ -57,6 +57,35 @@ class AppRouterDelegate extends RouterDelegate /// navigation away from the drive still discards the folder as it always has. String? _pendingFolderDriveId; + /// Reconciles the folder in view with the drive that has just been selected. + /// + /// Selecting a different drive discards the folder, which is what ordinary + /// navigation wants - a folder from the drive you just left is stale. The one + /// exception is the drive a folder link named: it becomes selected *because* + /// of that link, so its folder is the whole point rather than a leftover. + /// + /// Extracted from the listener that calls it so it can be tested without + /// standing up the whole app shell. + @visibleForTesting + void onDriveSelected(String? selectedDriveId) { + final selectedDriveChanged = driveId != selectedDriveId; + + final isTheLinkedDrive = _pendingFolderDriveId != null && + _pendingFolderDriveId == selectedDriveId; + + if (selectedDriveChanged && !isTheLinkedDrive) { + driveFolderId = null; + } + + // One shot. Released as soon as it is honored, so navigating away from the + // drive and back lands at its root rather than jumping to the old folder. + if (isTheLinkedDrive) { + _pendingFolderDriveId = null; + } + + driveId = selectedDriveId; + } + DriveKey? sharedDriveKey; String? sharedRawDriveKey; @@ -209,24 +238,7 @@ class AppRouterDelegate extends RouterDelegate shell = BlocListener( listener: (context, state) { if (state is DrivesLoadSuccess) { - final selectedDriveChanged = - driveId != state.selectedDriveId; - - // The drive a folder link named has just become the - // selected one, so its folder is not stale - it is the - // whole point of the link. - final isTheLinkedDrive = _pendingFolderDriveId != null && - _pendingFolderDriveId == state.selectedDriveId; - - if (selectedDriveChanged && !isTheLinkedDrive) { - driveFolderId = null; - } - - if (isTheLinkedDrive) { - _pendingFolderDriveId = null; - } - - driveId = state.selectedDriveId; + onDriveSelected(state.selectedDriveId); notifyListeners(); } }, diff --git a/test/pages/app_router_delegate_test.dart b/test/pages/app_router_delegate_test.dart new file mode 100644 index 0000000000..4f133b8f4b --- /dev/null +++ b/test/pages/app_router_delegate_test.dart @@ -0,0 +1,122 @@ +import 'package:ardrive/pages/app_route_path.dart'; +import 'package:ardrive/pages/app_router_delegate.dart'; +import 'package:flutter_test/flutter_test.dart'; + +/// How a folder link survives the drive being attached under it. +/// +/// A folder link only ever opened the folder for someone who already had the +/// drive. Everyone else - which is every stranger a public folder link is sent +/// to - went through the attach flow, which ends by clearing the drive id so +/// the prompt cannot re-fire. The drive was then selected fresh, the folder +/// read as belonging to a different drive, and it was dropped. A folder link +/// was a drive link with extra characters. +void main() { + const driveId = 'a2b7ba0a-3b2a-4c1b-8a2f-6d1a0b3c4d5e'; + const otherDriveId = 'b3c8cb1b-4c3b-5d2c-9b3f-7e2b1c4d5e6f'; + const folderId = 'c4d9dc2c-5d4c-6e3d-ac4f-8f3c2d5e6f70'; + + late AppRouterDelegate delegate; + + setUp(() => delegate = AppRouterDelegate()); + + /// What the attach flow does when it finishes: the drive id is cleared so the + /// attach prompt cannot fire again, and the drive is then selected fresh. + void attachCompletes() => delegate.driveId = null; + + group('a folder link', () { + test('opens its folder once the drive it names is selected', () async { + await delegate.setNewRoutePath( + AppRoutePath.folderDetail(driveId: driveId, driveFolderId: folderId), + ); + + attachCompletes(); + delegate.onDriveSelected(driveId); + + expect(delegate.driveFolderId, folderId); + expect(delegate.driveId, driveId); + }); + + test('opens its folder for someone who already had the drive', () async { + // No attach in between: the drive is already selected. + await delegate.setNewRoutePath( + AppRoutePath.folderDetail(driveId: driveId, driveFolderId: folderId), + ); + + delegate.onDriveSelected(driveId); + + expect(delegate.driveFolderId, folderId); + }); + + test('is released after it has been honored', () async { + // Otherwise every later visit to that drive would jump back into the + // folder the link named, long after the link was opened. + await delegate.setNewRoutePath( + AppRoutePath.folderDetail(driveId: driveId, driveFolderId: folderId), + ); + + attachCompletes(); + delegate.onDriveSelected(driveId); + + // Away to another drive, and back. + delegate.onDriveSelected(otherDriveId); + delegate.onDriveSelected(driveId); + + expect(delegate.driveFolderId, isNull); + }); + + test('does not follow the user to a different drive', () async { + await delegate.setNewRoutePath( + AppRoutePath.folderDetail(driveId: driveId, driveFolderId: folderId), + ); + + delegate.onDriveSelected(otherDriveId); + + expect(delegate.driveFolderId, isNull); + expect(delegate.driveId, otherDriveId); + }); + }); + + group('a drive link', () { + test('holds nothing back, since it names no folder', () async { + await delegate.setNewRoutePath( + AppRoutePath.driveDetail(driveId: driveId), + ); + + // The folder in view at the time belongs to wherever the user was. + delegate.driveFolderId = folderId; + + attachCompletes(); + delegate.onDriveSelected(driveId); + + expect(delegate.driveFolderId, isNull); + }); + }); + + group('ordinary navigation', () { + test('still discards the folder when the drive changes', () async { + await delegate.setNewRoutePath( + AppRoutePath.driveDetail(driveId: driveId), + ); + + delegate.onDriveSelected(driveId); + delegate.driveFolderId = folderId; + + delegate.onDriveSelected(otherDriveId); + + expect(delegate.driveFolderId, isNull); + }); + + test('keeps the folder while the drive stays the same', () async { + await delegate.setNewRoutePath( + AppRoutePath.driveDetail(driveId: driveId), + ); + + delegate.onDriveSelected(driveId); + delegate.driveFolderId = folderId; + + delegate.onDriveSelected(driveId); + + expect(delegate.driveFolderId, folderId); + }); + }); +} From e24bdc42ec8a02eaee327939f7871b2ae55197c8 Mon Sep 17 00:00:00 2001 From: vilenarios Date: Wed, 19 Aug 2026 00:39:55 -0400 Subject: [PATCH 11/13] fix: version history budget, and the share drive modal's layout PE-9210 Version history stopped loading, and that was mine. The resilience pass put the 15s first-paint budget on `loadActivity`'s read - the one query on the page that walks the file's entire revision history, is asked for only when the recipient opens the drawer, and blocks nothing while it runs. A file with real history, or a slow gateway, then reported "version history unavailable" where it used to simply take a while. It has its own two minute budget now. The share drive modal's layout: - The reveal control only exists on a secret row, so it stole width from the key field and not the link field: two boxes of visibly different widths, stacked. Its slot is now reserved on every row, and the field widths are asserted equal. - The access notice was `paragraphLarge` - the largest text in a dialog whose subject is the two fields above it. It is `paragraphNormal`, muted, now. - Helper lines use the same treatment as the file share dialog's rather than an ad hoc padding, so the two dialogs read as one design. - The reveal button no longer carries Material's default 48px padding, which was what left it floating between the field and the copy action. --- lib/blocs/shared_file/shared_file_cubit.dart | 37 +++++++-- lib/components/copyable_share_artifact.dart | 48 +++++++---- lib/components/drive_share_dialog.dart | 80 +++++++++---------- .../copyable_share_artifact_test.dart | 17 ++++ 4 files changed, 119 insertions(+), 63 deletions(-) diff --git a/lib/blocs/shared_file/shared_file_cubit.dart b/lib/blocs/shared_file/shared_file_cubit.dart index 110a94c4d1..50948b6519 100644 --- a/lib/blocs/shared_file/shared_file_cubit.dart +++ b/lib/blocs/shared_file/shared_file_cubit.dart @@ -114,20 +114,40 @@ class SharedFileCubit extends Cubit { /// offers Retry. static const defaultReadTimeout = Duration(seconds: 15); + /// The budget for the version history, which is a different kind of read. + /// + /// Every other read here fetches one thing and gates what the recipient sees; + /// this one walks the file's whole revision history, is asked for only when + /// the recipient opens the drawer, and blocks nothing while it runs. Holding + /// it to the first-paint budget made a file with real history - or a slow + /// gateway - report "version history unavailable" where it used to simply + /// take a while. + static const defaultHistoryTimeout = Duration(minutes: 2); + final Duration _readTimeout; + final Duration _historyTimeout; + /// Bounds [future], naming [what] so a timeout is legible in the log. /// /// A [TimeoutException] is deliberately left to propagate: every caller on /// the critical path already handles a failed read, either by degrading to /// another resolution path or by emitting the failure state. - Future _bounded(Future future, String what) => future.timeout( - _readTimeout, - onTimeout: () => throw TimeoutException( - 'Timed out after ${_readTimeout.inSeconds}s while $what', - _readTimeout, - ), - ); + Future _bounded( + Future future, + String what, { + Duration? timeout, + }) { + final budget = timeout ?? _readTimeout; + + return future.timeout( + budget, + onTimeout: () => throw TimeoutException( + 'Timed out after ${budget.inSeconds}s while $what', + budget, + ), + ); + } SharedFileCubit({ required this.fileId, @@ -139,11 +159,13 @@ class SharedFileCubit extends Cubit { ArDriveCrypto? crypto, Duration propagationRetryDelay = const Duration(seconds: 3), Duration readTimeout = defaultReadTimeout, + Duration historyTimeout = defaultHistoryTimeout, }) : _arweave = arweave, _licenseService = licenseService, _crypto = crypto ?? ArDriveCrypto(), _propagationRetryDelay = propagationRetryDelay, _readTimeout = readTimeout, + _historyTimeout = historyTimeout, // A v2 link can paint its skeleton with the real name and size before // a single byte has been fetched. super(SharedFileLoadInProgress(payload: linkPayload)) { @@ -310,6 +332,7 @@ class SharedFileCubit extends Cubit { final entities = await _bounded( _arweave.getAllFileEntitiesWithId(fileId, fileKey), 'reading the file\'s version history', + timeout: _historyTimeout, ); if (_isStale(resolution)) { diff --git a/lib/components/copyable_share_artifact.dart b/lib/components/copyable_share_artifact.dart index d4a66e83f3..4bcf66afb4 100644 --- a/lib/components/copyable_share_artifact.dart +++ b/lib/components/copyable_share_artifact.dart @@ -57,6 +57,14 @@ class CopyableShareArtifact extends StatefulWidget { } class _CopyableShareArtifactState extends State { + /// The reveal control's footprint, reserved on every row. + /// + /// Only a secret has something to reveal, but the slot is held open either + /// way: without it, the field beside a reveal button is narrower than the one + /// without, so a dialog showing a link above a key rendered two boxes of + /// visibly different widths. + static const _revealSlotWidth = 36.0; + late final TextEditingController _controller = TextEditingController(text: widget.text); @@ -113,22 +121,30 @@ class _CopyableShareArtifactState extends State { showObfuscationToggle: false, ), ), - if (widget.isSecret) ...[ - const SizedBox(width: 8), - // Material's `IconButton` rather than the `GestureDetector` + - // `ArDriveClickArea` pair used elsewhere: this control has no visible - // label, so it needs the keyboard focus and the announced name that - // a raw gesture detector does not provide. - IconButton( - icon: isMasked - ? ArDriveIcons.eyeClosed(color: colorTokens.textMid) - : ArDriveIcons.eyeOpen(color: colorTokens.textMid), - onPressed: () => setState(() => _isRevealed = !_isRevealed), - tooltip: widget.revealLabel, - splashRadius: 20, - ), - ], - const SizedBox(width: 16), + const SizedBox(width: 8), + SizedBox( + width: _revealSlotWidth, + child: widget.isSecret + // Material's `IconButton` rather than the `GestureDetector` + + // `ArDriveClickArea` pair used elsewhere: this control has no + // visible label, so it needs the keyboard focus and the announced + // name that a raw gesture detector does not provide. + ? IconButton( + icon: isMasked + ? ArDriveIcons.eyeClosed(color: colorTokens.textMid) + : ArDriveIcons.eyeOpen(color: colorTokens.textMid), + onPressed: () => setState(() => _isRevealed = !_isRevealed), + tooltip: widget.revealLabel, + splashRadius: 18, + padding: EdgeInsets.zero, + constraints: const BoxConstraints.tightFor( + width: _revealSlotWidth, + height: _revealSlotWidth, + ), + ) + : null, + ), + const SizedBox(width: 12), CopyButton( positionX: 4, positionY: 40, diff --git a/lib/components/drive_share_dialog.dart b/lib/components/drive_share_dialog.dart index b9c8c7327c..959526da88 100644 --- a/lib/components/drive_share_dialog.dart +++ b/lib/components/drive_share_dialog.dart @@ -47,6 +47,7 @@ class DriveShareDialogState extends State { BlocBuilder( builder: (context, state) { final typography = ArDriveTypographyNew.of(context); + final colorTokens = ArDriveTheme.of(context).themeData.colorTokens; return ArDriveStandardModalNew( width: kLargeDialogWidth, @@ -73,10 +74,7 @@ class DriveShareDialogState extends State { state.folderName ?? state.drive.name, style: typography.paragraphNormal( fontWeight: ArFontWeight.semiBold, - color: ArDriveTheme.of(context) - .themeData - .colorTokens - .textHigh, + color: colorTokens.textHigh, ), ), ), @@ -103,22 +101,13 @@ class DriveShareDialogState extends State { appLocalizationsOf(context).shareFileRevealKey, isSecret: true, ), - Padding( - padding: const EdgeInsets.only(top: 4), - child: Text( - appLocalizationsOf(context) - .shareDriveSendKeySeparately, - style: typography.paragraphSmall( - color: ArDriveTheme.of(context) - .themeData - .colorTokens - .textLow, - ), - ), + _HelperText( + appLocalizationsOf(context).shareDriveSendKeySeparately, + color: colorTokens.textLow, ), }, if (state.drive.isPrivate) ...{ - const SizedBox(height: 16), + const SizedBox(height: 20), ArDriveCheckBox( // The checkbox only reads `checked` when it is first // built, so the key forces a fresh one whenever the @@ -127,11 +116,8 @@ class DriveShareDialogState extends State { checked: state.keyIsInLink, title: appLocalizationsOf(context) .shareDriveIncludeKeyInLink, - titleStyle: typography.paragraphSmall( - color: ArDriveTheme.of(context) - .themeData - .colorTokens - .textMid, + titleStyle: typography.paragraphNormal( + color: colorTokens.textMid, ), onChange: (value) => context.read().setKeyIsInLink( @@ -139,21 +125,13 @@ class DriveShareDialogState extends State { ), ), if (state.keyIsInLink) - Padding( - padding: const EdgeInsets.only(top: 4), - child: Text( - appLocalizationsOf(context) - .shareDriveKeyInLinkWarning, - style: typography.paragraphSmall( - color: ArDriveTheme.of(context) - .themeData - .colorTokens - .strokeRed, - ), - ), + _HelperText( + appLocalizationsOf(context) + .shareDriveKeyInLinkWarning, + color: colorTokens.strokeRed, ), }, - const SizedBox(height: 16), + const SizedBox(height: 20), Text( // A keyless private link does *not* grant access on its // own, so it must not be described as though it does - @@ -166,16 +144,18 @@ class DriveShareDialogState extends State { .anyoneCanAccessThisDrivePrivate : appLocalizationsOf(context) .shareDriveKeylessNotice, - style: typography.paragraphLarge(), + // Guidance, not the headline. As `paragraphLarge` it was + // the largest text in a dialog whose actual subject is + // the two fields above it. + style: typography.paragraphNormal( + color: colorTokens.textMid, + ), ), } else if (state is DriveShareLoadFail) Text( appLocalizationsOf(context).shareDriveFailure, style: typography.paragraphNormal( - color: ArDriveTheme.of(context) - .themeData - .colorTokens - .textMid, + color: colorTokens.textMid, ), ), ], @@ -203,3 +183,23 @@ class DriveShareDialogState extends State { }, ); } + +/// A line of guidance under the control it belongs to. +/// +/// Same treatment as the file share dialog's, so the two dialogs read as one +/// design rather than two. +class _HelperText extends StatelessWidget { + const _HelperText(this.text, {required this.color}); + + final String text; + final Color color; + + @override + Widget build(BuildContext context) => Padding( + padding: const EdgeInsets.only(top: 8), + child: Text( + text, + style: ArDriveTypographyNew.of(context).paragraphSmall(color: color), + ), + ); +} diff --git a/test/components/copyable_share_artifact_test.dart b/test/components/copyable_share_artifact_test.dart index 168c2bffd0..6425a0527d 100644 --- a/test/components/copyable_share_artifact_test.dart +++ b/test/components/copyable_share_artifact_test.dart @@ -103,6 +103,23 @@ void main() { expect(isObscured(tester), isTrue); }); + testWidgets('a secret and a plain field are the same width', + (tester) async { + // A dialog stacks a link over a key. The reveal control only exists on + // the secret one, so unless its slot is held open on both, the two boxes + // render at visibly different widths. + double fieldWidth(WidgetTester tester) => + tester.getSize(find.byType(ArDriveTextFieldNew)).width; + + await tester.pumpWidget(wrap(artifact(isSecret: false))); + final plain = fieldWidth(tester); + + await tester.pumpWidget(wrap(artifact(isSecret: true))); + final secret = fieldWidth(tester); + + expect(secret, plain); + }); + testWidgets('the value stays intact underneath the mask', (tester) async { // Masking is a display concern. If it ever reached the controller the // sharer would hand out a string of dots. From 818b3128c88213a1c7caf68833ca23974e80b193 Mon Sep 17 00:00:00 2001 From: vilenarios Date: Wed, 19 Aug 2026 01:03:19 -0400 Subject: [PATCH 12/13] perf: stop the download re-fetching the cipher the link already carried PE-9210 `c` and `iv` are in the link schema so that a private download does not have to ask the network what its own bytes are encrypted with. Nothing ever read them: `SharedFileLinkPayload.hasCipherDetails` was defined and referenced nowhere, and every private download issued a `getTransactionDetails` for tags the link had already delivered. The sharer even pays a lookup to populate those fields, so they were pure cost - longer links, an extra call on the way out, nothing saved on the way in. On a rate-limited connection that is not waste but a call that can fail outright. The page now hands the link's cipher to the download, and the download uses what it is given. Guarded on the link naming a *bundled* data item and the *current* target: `verifyDownload` turns on the arweave client's chunk check for L1 transactions and is decided by a tag on that same lookup, so skipping it for something that might be L1 would drop a check silently. A bundled item is not an L1 transaction, so there is nothing to drop. A recipient who has moved the page to a newer revision gets the lookup, since the link never described those bytes. Attach drive says what is happening. Entering an id that resolves to a private drive showed nothing but a key field appearing; it now says the drive was found and what is still needed. The lookup progress line was hardcoded English and is localized. --- .../shared_file_download_cubit.dart | 45 +++++++++++++++---- lib/components/drive_attach_form.dart | 17 ++++++- lib/components/file_download_dialog.dart | 4 ++ lib/l10n/app_en.arb | 17 +++++++ .../shared_file/shared_file_ready_view.dart | 21 +++++++++ .../shared_file_download_cubit_test.dart | 41 +++++++++++++++-- 6 files changed, 132 insertions(+), 13 deletions(-) diff --git a/lib/blocs/file_download/shared_file_download_cubit.dart b/lib/blocs/file_download/shared_file_download_cubit.dart index 068aab7797..5e68a7af61 100644 --- a/lib/blocs/file_download/shared_file_download_cubit.dart +++ b/lib/blocs/file_download/shared_file_download_cubit.dart @@ -9,8 +9,19 @@ class SharedFileDownloadCubit extends FileDownloadCubit { final ArDriveDownloader _arDriveDownloader; final DownloadPolicy _downloadPolicy; + /// The cipher the share link named, when it named one. + /// + /// Supplied only for a transaction the caller knows is bundled - see + /// `_downloadFile` for why that qualification is load bearing. + final String? cipher; + + /// The cipher IV the share link named. Travels with [cipher] or not at all. + final String? cipherIv; + SharedFileDownloadCubit({ this.fileKey, + this.cipher, + this.cipherIv, required this.revision, required ArweaveService arweave, required ArDriveCrypto crypto, @@ -85,17 +96,33 @@ class SharedFileDownloadCubit extends FileDownloadCubit { } if (fileKey != null && !isPinFile) { - // Private/encrypted files need cipher/IV tags from the data transaction - final dataTx = await _arweave.getTransactionDetails(dataTxId); + if (cipher != null && cipherIv != null) { + // The link already said what this transaction is encrypted with, so + // the lookup that would have asked is skipped entirely. Carrying `c` + // and `iv` in a share link only pays for itself here - before this, + // every private download re-fetched tags the link had already + // delivered, which on a rate-limited connection is a call that can + // fail outright. + // + // `verifyDownload` stays false, and correctly so: it turns on the + // arweave client's chunk check for L1 transactions, and the caller + // only supplies these tags for a data item it knows is bundled - + // which is not an L1 transaction and has no chunks to check. + cipherTag = cipher; + cipherIvTag = cipherIv; + } else { + // Private/encrypted files need cipher/IV tags from the data transaction + final dataTx = await _arweave.getTransactionDetails(dataTxId); + + if (dataTx == null) { + throw StateError( + 'Data transaction not found for file ${revision.id} with txId $dataTxId from gateway ${_arweave.client.api.gatewayUrl.origin}'); + } - if (dataTx == null) { - throw StateError( - 'Data transaction not found for file ${revision.id} with txId $dataTxId from gateway ${_arweave.client.api.gatewayUrl.origin}'); + cipherTag = dataTx.getTag(EntityTag.cipher); + cipherIvTag = dataTx.getTag(EntityTag.cipherIv); + verifyDownload = dataTx.getTag(EntityTag.appName) == 'ArDrive-CLI'; } - - cipherTag = dataTx.getTag(EntityTag.cipher); - cipherIvTag = dataTx.getTag(EntityTag.cipherIv); - verifyDownload = dataTx.getTag(EntityTag.appName) == 'ArDrive-CLI'; } logger.d('File size: ${revision.size}'); diff --git a/lib/components/drive_attach_form.dart b/lib/components/drive_attach_form.dart index f1d8d0c046..81db655e42 100644 --- a/lib/components/drive_attach_form.dart +++ b/lib/components/drive_attach_form.dart @@ -155,7 +155,7 @@ class _DriveAttachFormState extends State { const LinearProgressIndicator(), const SizedBox(height: 4), Text( - 'Looking up drive...', + appLocalizationsOf(context).attachDriveLookingUp, style: ArDriveTypographyNew.of(context) .paragraphSmall( color: ArDriveTheme.of(context) @@ -169,6 +169,21 @@ class _DriveAttachFormState extends State { ); }, ), + if (state is DriveAttachPrivate) ...[ + const SizedBox(height: 8), + Align( + alignment: Alignment.centerLeft, + child: Text( + appLocalizationsOf(context).attachDrivePrivateFound, + style: ArDriveTypographyNew.of(context).paragraphSmall( + color: ArDriveTheme.of(context) + .themeData + .colorTokens + .textLow, + ), + ), + ), + ], const SizedBox(height: 16), if (state is DriveAttachPrivate) ArDriveTextFieldNew( diff --git a/lib/components/file_download_dialog.dart b/lib/components/file_download_dialog.dart index 103ca20c65..b05933efe8 100644 --- a/lib/components/file_download_dialog.dart +++ b/lib/components/file_download_dialog.dart @@ -103,6 +103,8 @@ Future promptToDownloadSharedFile({ required BuildContext context, SecretKey? fileKey, required ARFSFileEntity revision, + String? cipher, + String? cipherIv, }) { final cubit = SharedFileDownloadCubit( arDriveDownloader: ArDriveDownloader( @@ -113,6 +115,8 @@ Future promptToDownloadSharedFile({ crypto: ArDriveCrypto(), revision: revision, fileKey: fileKey, + cipher: cipher, + cipherIv: cipherIv, arweave: context.read(), ); return showArDriveDialog( diff --git a/lib/l10n/app_en.arb b/lib/l10n/app_en.arb index 9a2e51a45e..8e408e7ca7 100644 --- a/lib/l10n/app_en.arb +++ b/lib/l10n/app_en.arb @@ -2362,6 +2362,23 @@ "@shareDriveSendKeySeparately": { "description": "Helper text under the drive key field explaining the two-artifact handover" }, + "attachDriveLookingUp": "Looking up drive…", + "@attachDriveLookingUp": { + "description": "Shown while the entered drive ID is being resolved" + }, + "attachDrivePrivateFound": "This is a private drive. Enter its access key to continue.", + "@attachDrivePrivateFound": { + "description": "Shown once a drive ID resolves to a private drive, so the user knows it was found and what is still needed" + }, + "attachDriveNameResolved": "Drive found: {driveName}", + "@attachDriveNameResolved": { + "description": "Confirms the drive whose name has been read off the chain", + "placeholders": { + "driveName": { + "type": "String" + } + } + }, "shareDriveFailure": "We couldn’t create a share link for this drive. Check your connection and try again.", "@shareDriveFailure": { "description": "Shown in the share drive dialog when the link could not be built, most often because the drive key could not be read" diff --git a/lib/pages/shared_file/shared_file_ready_view.dart b/lib/pages/shared_file/shared_file_ready_view.dart index d658ff2a66..e73221a7ae 100644 --- a/lib/pages/shared_file/shared_file_ready_view.dart +++ b/lib/pages/shared_file/shared_file_ready_view.dart @@ -500,11 +500,32 @@ class _SharedFileReadyViewState extends State { Future _download(BuildContext context, FileRevision revision) async { setState(() => _isDownloading = true); + final payload = widget.state.payload; + + // The link's own cipher, handed to the download so it does not re-fetch + // tags the link already delivered - the reason `c` and `iv` are in the + // schema at all. + // + // Only for a data item the link says is bundled. `verifyDownload` turns on + // the arweave client's chunk check for L1 transactions and is decided by a + // tag on that same lookup, so skipping the lookup for something that might + // be L1 would drop a check without saying so. A bundled item is not an L1 + // transaction, so for those there is nothing to drop. + // + // It must also be *this* revision's cipher: the recipient may have moved + // the page to a newer one, whose bytes the link never described. + final linkDescribesTarget = payload != null && + payload.hasCipherDetails && + payload.bundledInTxId != null && + payload.dataTxId == revision.dataTxId; + try { await promptToDownloadSharedFile( revision: ARFSFactory().getARFSFileFromFileRevision(revision), context: context, fileKey: widget.state.fileKey, + cipher: linkDescribesTarget ? payload.cipher : null, + cipherIv: linkDescribesTarget ? payload.cipherIv : null, ); } finally { if (mounted) { diff --git a/test/blocs/shared_file_download_cubit_test.dart b/test/blocs/shared_file_download_cubit_test.dart index 8bcee49394..37d41c563b 100644 --- a/test/blocs/shared_file_download_cubit_test.dart +++ b/test/blocs/shared_file_download_cubit_test.dart @@ -10,8 +10,10 @@ import 'package:cryptography/cryptography.dart'; // lib/download/limits.dart. // ignore: depend_on_referenced_packages import 'package:fake_async/fake_async.dart'; +import 'package:ardrive/services/services.dart'; import 'package:flutter/services.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:mocktail/mocktail.dart'; import '../test_utils/mocks.dart'; @@ -152,16 +154,49 @@ class _RecordingDownloader implements ArDriveDownloader { /// The recipient's download. void main() { - SharedFileDownloadCubit cubitFor(_RecordingDownloader downloader) { + SharedFileDownloadCubit cubitFor( + _RecordingDownloader downloader, { + ArweaveService? arweave, + SecretKey? fileKey, + String? cipher, + String? cipherIv, + DownloadPolicy policy = const _UnavailablePolicy(), + }) { return SharedFileDownloadCubit( revision: _Revision(), - arweave: MockArweaveService(), + arweave: arweave ?? MockArweaveService(), crypto: MockArDriveCrypto(), arDriveDownloader: downloader, - downloadPolicy: const _UnavailablePolicy(), + fileKey: fileKey, + cipher: cipher, + cipherIv: cipherIv, + downloadPolicy: policy, ); } + test( + 'a private download uses the cipher the link named, without asking for ' + 'it again', () async { + // `c` and `iv` are in the link schema precisely so this lookup does not + // have to happen. Before this the download re-fetched tags the link had + // already delivered - wasted on a good connection, and on a rate limited + // one a call that can fail outright. + final arweave = MockArweaveService(); + final downloader = _RecordingDownloader(); + + final cubit = cubitFor( + downloader, + arweave: arweave, + fileKey: SecretKey(List.filled(32, 1)), + cipher: 'AES256-GCM', + cipherIv: 'an-iv', + ); + + await cubit.stream.firstWhere((s) => s is! FileDownloadStarting); + + verifyNever(() => arweave.getTransactionDetails(any())); + }); + test('a size check that throws costs the check, not the download', () async { final downloader = _RecordingDownloader(); From 16f4aad169716611587a632de2db83c2d0118ca0 Mon Sep 17 00:00:00 2001 From: vilenarios Date: Wed, 19 Aug 2026 12:45:39 -0400 Subject: [PATCH 13/13] fix: a folder link could open another drive's folder under this drive PE-9210 The pending marker preserved whatever folder was in view when its drive was finally selected, rather than restoring the one the link named. Between the link opening and that drive arriving the recipient may have been somewhere else entirely - so opening a folder link for drive A, wandering into a folder of drive B, and then landing on A showed B's folder under A's name. The folder is now held with the drive it belongs to and restored by id, which cannot name a folder from anywhere else. Verified to bite: reverting the restore fails the new test. --- lib/pages/app_router_delegate.dart | 28 +++++++++++++++++++----- test/pages/app_router_delegate_test.dart | 22 +++++++++++++++++++ 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/lib/pages/app_router_delegate.dart b/lib/pages/app_router_delegate.dart index 2af9cd216b..04f7f9d223 100644 --- a/lib/pages/app_router_delegate.dart +++ b/lib/pages/app_router_delegate.dart @@ -57,6 +57,17 @@ class AppRouterDelegate extends RouterDelegate /// navigation away from the drive still discards the folder as it always has. String? _pendingFolderDriveId; + /// The folder that link named, held with the drive it belongs to. + /// + /// Kept alongside [_pendingFolderDriveId] rather than read off + /// [driveFolderId] when the drive arrives: between the link opening and that + /// drive being selected the recipient may have been somewhere else entirely, + /// and [driveFolderId] would by then hold a folder from whatever drive they + /// were last in. Restoring *this* is the difference between opening the + /// folder the link named and opening another drive's folder under this + /// drive's name. + String? _pendingFolderId; + /// Reconciles the folder in view with the drive that has just been selected. /// /// Selecting a different drive discards the folder, which is what ordinary @@ -73,14 +84,17 @@ class AppRouterDelegate extends RouterDelegate final isTheLinkedDrive = _pendingFolderDriveId != null && _pendingFolderDriveId == selectedDriveId; - if (selectedDriveChanged && !isTheLinkedDrive) { - driveFolderId = null; - } - - // One shot. Released as soon as it is honored, so navigating away from the - // drive and back lands at its root rather than jumping to the old folder. if (isTheLinkedDrive) { + // The folder the link named, not the one in view - see [_pendingFolderId]. + driveFolderId = _pendingFolderId; + + // One shot. Released as soon as it is honored, so navigating away from + // the drive and back lands at its root rather than jumping to the old + // folder. _pendingFolderDriveId = null; + _pendingFolderId = null; + } else if (selectedDriveChanged) { + driveFolderId = null; } driveId = selectedDriveId; @@ -424,6 +438,7 @@ class AppRouterDelegate extends RouterDelegate driveFolderId = configuration.driveFolderId; _pendingFolderDriveId = configuration.driveFolderId == null ? null : configuration.driveId; + _pendingFolderId = configuration.driveFolderId; sharedDriveKey = configuration.sharedDriveKey; sharedRawDriveKey = configuration.sharedRawDriveKey; sharedFileId = configuration.sharedFileId; @@ -443,6 +458,7 @@ class AppRouterDelegate extends RouterDelegate driveName = null; driveFolderId = null; _pendingFolderDriveId = null; + _pendingFolderId = null; sharedDriveKey = null; sharedRawDriveKey = null; sharedFileId = null; diff --git a/test/pages/app_router_delegate_test.dart b/test/pages/app_router_delegate_test.dart index 4f133b8f4b..78d50ee94b 100644 --- a/test/pages/app_router_delegate_test.dart +++ b/test/pages/app_router_delegate_test.dart @@ -64,6 +64,28 @@ void main() { expect(delegate.driveFolderId, isNull); }); + test('never carries another drive\'s folder in under its name', () async { + // The pending marker used to preserve whatever folder was in view when + // its drive arrived, rather than restoring the one the link named. A + // recipient who wandered off before the drive was selected therefore + // landed on drive A showing a folder that belongs to drive B. + const otherFolderId = 'd5eaed3d-6e5d-7f4e-bd5f-9f4d3e6f7a81'; + + await delegate.setNewRoutePath( + AppRoutePath.folderDetail(driveId: driveId, driveFolderId: folderId), + ); + + // Away to another drive, and into a folder there. + delegate.onDriveSelected(otherDriveId); + delegate.driveFolderId = otherFolderId; + + // Now the linked drive finally arrives. + delegate.onDriveSelected(driveId); + + expect(delegate.driveFolderId, folderId); + expect(delegate.driveFolderId, isNot(otherFolderId)); + }); + test('does not follow the user to a different drive', () async { await delegate.setNewRoutePath( AppRoutePath.folderDetail(driveId: driveId, driveFolderId: folderId),