From e7ec0f4386d9b258432c0850b84dc519fd73b76a Mon Sep 17 00:00:00 2001 From: Peter Jankuliak Date: Tue, 17 Sep 2024 14:47:40 +0200 Subject: [PATCH 1/6] Add `operator==` and `toString` for `Option` class --- lib/app/utils/option.dart | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/lib/app/utils/option.dart b/lib/app/utils/option.dart index 85294f24..6855a5a1 100644 --- a/lib/app/utils/option.dart +++ b/lib/app/utils/option.dart @@ -5,6 +5,21 @@ sealed class Option { const Option(); T? get value; + + @override + bool operator ==(Object other) { + if (other is! Option) return false; + switch ((this, other)) { + case (Some(value: var lv), Some(value: var rv)): + return lv == rv; + case (None, None): + return true; + case (_, _): + return false; + } + } + + String toString(); } class Some extends Option { @@ -12,6 +27,9 @@ class Some extends Option { @override final T value; + + @override + String toString() => "Some($value)"; } class None extends Option { @@ -19,4 +37,7 @@ class None extends Option { @override T? get value => null; + + @override + String toString() => "None"; } From 613fb1c673c8efa237c1c702c194271be49be9cb Mon Sep 17 00:00:00 2001 From: Peter Jankuliak Date: Tue, 17 Sep 2024 15:34:22 +0200 Subject: [PATCH 2/6] Security screen fix: password Fix a bug where after "updating" a password in the repository screen and then leaving there was a popup about unsaved changes. This was because the password was still in the password field and the old logic evaluated it as a pending change. Fixed by introducing another field `updatedLocalPassword` to signify the latest set password and checking if the current password in the password field is equal to that. --- lib/app/cubits/repo_security.dart | 37 +++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/lib/app/cubits/repo_security.dart b/lib/app/cubits/repo_security.dart index 43c80826..8c7c23e2 100644 --- a/lib/app/cubits/repo_security.dart +++ b/lib/app/cubits/repo_security.dart @@ -20,6 +20,7 @@ class RepoSecurityState { bool? store, bool? secureWithBiometrics, this.localPassword = const None(), + this.updatedLocalPassword = const None(), this.isBiometricsAvailable = false, }) : origin = origin ?? oldLocalSecretMode.origin, store = store ?? _initialStore(oldLocalSecretMode), @@ -32,6 +33,11 @@ class RepoSecurityState { final bool store; final bool secureWithBiometrics; final Option localPassword; + // This is set to the above `localPassword` once the user clicks the "UPDATE" + // button. It is used to check whether the `localPassword` has changed + // between the user clicking the "UPDATE" button and leaving the security + // page. + final Option updatedLocalPassword; final bool isBiometricsAvailable; RepoSecurityState copyWith({ @@ -41,6 +47,7 @@ class RepoSecurityState { bool? store, bool? secureWithBiometrics, Option? localPassword, + Option? updatedLocalPassword, bool? isBiometricsAvailable, }) => RepoSecurityState( @@ -50,6 +57,7 @@ class RepoSecurityState { store: store ?? this.store, secureWithBiometrics: secureWithBiometrics ?? this.secureWithBiometrics, localPassword: localPassword ?? this.localPassword, + updatedLocalPassword: updatedLocalPassword ?? this.updatedLocalPassword, isBiometricsAvailable: isBiometricsAvailable ?? this.isBiometricsAvailable, ); @@ -71,12 +79,13 @@ class RepoSecurityState { bool get isValid => newLocalSecretInput != null; - bool get hasPendingChanges => - origin != oldLocalSecretMode.origin || - store != oldLocalSecretMode.store.isStored || - secureWithBiometrics != - oldLocalSecretMode.store.isSecuredWithBiometrics || - localPassword is Some; + bool get hasPendingChanges { + return origin != oldLocalSecretMode.origin || + store != oldLocalSecretMode.store.isStored || + secureWithBiometrics != + oldLocalSecretMode.store.isSecuredWithBiometrics || + localPassword != updatedLocalPassword; + } LocalSecretInput? get newLocalSecretInput => switch ((localPassword, origin, store, secureWithBiometrics)) { @@ -170,7 +179,17 @@ class RepoSecurityCubit extends Cubit with AppLogger { // Save the new auth mode try { await repoCubit.setAuthMode(newAuthMode); - emit(state.copyWith(oldLocalSecretMode: newAuthMode.localSecretMode)); + + Option newLocalPassword = + newLocalSecretInput is LocalSecretManual + ? Some(newLocalSecretInput.password) + : None(); + + emit(state.copyWith( + oldLocalSecretMode: newAuthMode.localSecretMode, + updatedLocalPassword: newLocalPassword, + )); + loggy.debug('Repo auth mode updated: $newAuthMode'); } catch (e, st) { loggy.error( @@ -216,8 +235,8 @@ class RepoSecurityCubit extends Cubit with AppLogger { //} } -// We want store to be explicitly opt-in so the switch must be initially off even if the -// initial origin is random which is implicitly stored. +// We want `store` to be explicitly opt-in so the switch must be initially off even if the +// initial `origin` is random which is implicitly stored. bool _initialStore(LocalSecretMode localSecretMode) => switch (localSecretMode) { LocalSecretMode.manualStored || From fb207c69f43d8fb69ba1c4cf5efec7fb01a4b92f Mon Sep 17 00:00:00 2001 From: Peter Jankuliak Date: Wed, 18 Sep 2024 09:34:46 +0200 Subject: [PATCH 3/6] Also don't count no password or mismatched password as unsaved change --- lib/app/cubits/repo_security.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/app/cubits/repo_security.dart b/lib/app/cubits/repo_security.dart index 8c7c23e2..d2b1e694 100644 --- a/lib/app/cubits/repo_security.dart +++ b/lib/app/cubits/repo_security.dart @@ -84,7 +84,7 @@ class RepoSecurityState { store != oldLocalSecretMode.store.isStored || secureWithBiometrics != oldLocalSecretMode.store.isSecuredWithBiometrics || - localPassword != updatedLocalPassword; + (localPassword is Some && localPassword != updatedLocalPassword); } LocalSecretInput? get newLocalSecretInput => From 03ee18c245fdebdc9d1e5e01be315ea2e70a48fb Mon Sep 17 00:00:00 2001 From: Peter Jankuliak Date: Wed, 18 Sep 2024 09:46:48 +0200 Subject: [PATCH 4/6] Change action text from "Accept" to "Discard" When the user is asked whether to discard unsaved changes in the security screen. --- lib/app/pages/repo_security_page.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/app/pages/repo_security_page.dart b/lib/app/pages/repo_security_page.dart index 1a65f8ec..1e398595 100644 --- a/lib/app/pages/repo_security_page.dart +++ b/lib/app/pages/repo_security_page.dart @@ -81,7 +81,7 @@ class RepoSecurityPage extends StatelessWidget { child: Text(S.current.actionCancel), onPressed: () => Navigator.of(context).pop(false)), TextButton( - child: Text(S.current.actionAccept), + child: Text(S.current.actionDiscard), onPressed: () => Navigator.of(context).pop(true)) ], ).then((pop) { From 434e3d9efa4a844c3b6d79ed437261587d00b779 Mon Sep 17 00:00:00 2001 From: Peter Jankuliak Date: Wed, 18 Sep 2024 09:48:16 +0200 Subject: [PATCH 5/6] Change text "{Do you want -> Would you like} to discard them?" When leaving the security screen with unsaved changes. --- lib/generated/intl/messages_all.dart | 27 ++++++++++++++------------- lib/generated/intl/messages_en.dart | 2 +- lib/generated/l10n.dart | 4 ++-- lib/l10n/intl_en.arb | 4 ++-- 4 files changed, 19 insertions(+), 18 deletions(-) diff --git a/lib/generated/intl/messages_all.dart b/lib/generated/intl/messages_all.dart index 5d4060f8..a2443ef6 100644 --- a/lib/generated/intl/messages_all.dart +++ b/lib/generated/intl/messages_all.dart @@ -11,6 +11,7 @@ import 'dart:async'; +import 'package:flutter/foundation.dart'; import 'package:intl/intl.dart'; import 'package:intl/message_lookup_by_library.dart'; import 'package:intl/src/intl_helpers.dart'; @@ -27,15 +28,15 @@ import 'messages_zh-TW.dart' as messages_zh_tw; typedef Future LibraryLoader(); Map _deferredLibraries = { - 'en': () => new Future.value(null), - 'es': () => new Future.value(null), - 'fa': () => new Future.value(null), - 'fr': () => new Future.value(null), - 'my': () => new Future.value(null), - 'ru': () => new Future.value(null), - 'uk': () => new Future.value(null), - 'zh_CN': () => new Future.value(null), - 'zh_TW': () => new Future.value(null), + 'en': () => new SynchronousFuture(null), + 'es': () => new SynchronousFuture(null), + 'fa': () => new SynchronousFuture(null), + 'fr': () => new SynchronousFuture(null), + 'my': () => new SynchronousFuture(null), + 'ru': () => new SynchronousFuture(null), + 'uk': () => new SynchronousFuture(null), + 'zh_CN': () => new SynchronousFuture(null), + 'zh_TW': () => new SynchronousFuture(null), }; MessageLookupByLibrary? _findExact(String localeName) { @@ -64,18 +65,18 @@ MessageLookupByLibrary? _findExact(String localeName) { } /// User programs should call this before using [localeName] for messages. -Future initializeMessages(String localeName) async { +Future initializeMessages(String localeName) { var availableLocale = Intl.verifiedLocale( localeName, (locale) => _deferredLibraries[locale] != null, onFailure: (_) => null); if (availableLocale == null) { - return new Future.value(false); + return new SynchronousFuture(false); } var lib = _deferredLibraries[availableLocale]; - await (lib == null ? new Future.value(false) : lib()); + lib == null ? new SynchronousFuture(false) : lib(); initializeInternalMessageLookup(() => new CompositeMessageLookup()); messageLookup.addLocale(availableLocale, _findGeneratedMessagesFor); - return new Future.value(true); + return new SynchronousFuture(true); } bool _messagesExistFor(String locale) { diff --git a/lib/generated/intl/messages_en.dart b/lib/generated/intl/messages_en.dart index e42de1e3..d9861160 100644 --- a/lib/generated/intl/messages_en.dart +++ b/lib/generated/intl/messages_en.dart @@ -753,7 +753,7 @@ class MessageLookup extends MessageLookupByLibrary { "messageUnlockUsingBiometrics": MessageLookupByLibrary.simpleMessage("Unlock using biometrics"), "messageUnsavedChanges": MessageLookupByLibrary.simpleMessage( - "You have unsaved changes.\n\nDo you want to discard them?"), + "You have unsaved changes.\n\nWould you like to discard them?"), "messageUpdateLocalPasswordConfirmation": MessageLookupByLibrary.simpleMessage( "Update this repository localpassword?"), diff --git a/lib/generated/l10n.dart b/lib/generated/l10n.dart index 0ab311bc..be684983 100644 --- a/lib/generated/l10n.dart +++ b/lib/generated/l10n.dart @@ -2750,10 +2750,10 @@ class S { ); } - /// `You have unsaved changes.\n\nDo you want to discard them?` + /// `You have unsaved changes.\n\nWould you like to discard them?` String get messageUnsavedChanges { return Intl.message( - 'You have unsaved changes.\n\nDo you want to discard them?', + 'You have unsaved changes.\n\nWould you like to discard them?', name: 'messageUnsavedChanges', desc: '', args: [], diff --git a/lib/l10n/intl_en.arb b/lib/l10n/intl_en.arb index 730b6e8c..fc132951 100644 --- a/lib/l10n/intl_en.arb +++ b/lib/l10n/intl_en.arb @@ -426,7 +426,7 @@ "messageVPN":"VPN", "messageNone":"None", "messagePeerExchange":"Peer Exchange", - "messageUnsavedChanges":"You have unsaved changes.\n\nDo you want to discard them?", + "messageUnsavedChanges":"You have unsaved changes.\n\nWould you like to discard them?", "messageAccessingSecureStorage":"Accessing secure storage", "messageSavingChanges":"Do you want to save the current changes?", "messageUpdateLocalSecretFailed":"Updating security properties of the repository failed.", @@ -704,4 +704,4 @@ "dokanUrl": {} } } -} \ No newline at end of file +} From 1569b51dd6ceaa249dfed311015b64f8f4efc1bc Mon Sep 17 00:00:00 2001 From: Peter Jankuliak Date: Wed, 18 Sep 2024 11:29:16 +0200 Subject: [PATCH 6/6] Fix incorrect "unsaved changes" popup when leaving the security screen Happened even if no changes in the security screen were made. The precondition was that the repo had the "Store password" option set to false. --- lib/app/cubits/repo_security.dart | 97 +++++++++++++++++++----------- lib/app/widgets/repo_security.dart | 2 +- 2 files changed, 62 insertions(+), 37 deletions(-) diff --git a/lib/app/cubits/repo_security.dart b/lib/app/cubits/repo_security.dart index d2b1e694..d4fc82cc 100644 --- a/lib/app/cubits/repo_security.dart +++ b/lib/app/cubits/repo_security.dart @@ -17,20 +17,21 @@ class RepoSecurityState { required this.oldLocalSecretMode, required this.oldLocalSecret, SecretKeyOrigin? origin, - bool? store, + // Reflects the user's preference on storing a password for + // `LocalSecretMode`s where storing is not implicit. + this.userPrefersToStoreSecret = const None(), bool? secureWithBiometrics, this.localPassword = const None(), this.updatedLocalPassword = const None(), this.isBiometricsAvailable = false, }) : origin = origin ?? oldLocalSecretMode.origin, - store = store ?? _initialStore(oldLocalSecretMode), secureWithBiometrics = secureWithBiometrics ?? oldLocalSecretMode.store.isSecuredWithBiometrics; final LocalSecretMode oldLocalSecretMode; final LocalSecret oldLocalSecret; final SecretKeyOrigin origin; - final bool store; + final Option userPrefersToStoreSecret; final bool secureWithBiometrics; final Option localPassword; // This is set to the above `localPassword` once the user clicks the "UPDATE" @@ -39,12 +40,13 @@ class RepoSecurityState { // page. final Option updatedLocalPassword; final bool isBiometricsAvailable; + final bool _defaultStoreIfOriginIsManual = false; RepoSecurityState copyWith({ LocalSecretMode? oldLocalSecretMode, LocalSecret? oldLocalSecret, SecretKeyOrigin? origin, - bool? store, + bool? userPrefersToStoreSecret, bool? secureWithBiometrics, Option? localPassword, Option? updatedLocalPassword, @@ -54,7 +56,9 @@ class RepoSecurityState { oldLocalSecretMode: oldLocalSecretMode ?? this.oldLocalSecretMode, oldLocalSecret: oldLocalSecret ?? this.oldLocalSecret, origin: origin ?? this.origin, - store: store ?? this.store, + userPrefersToStoreSecret: userPrefersToStoreSecret != null + ? Some(userPrefersToStoreSecret) + : this.userPrefersToStoreSecret, secureWithBiometrics: secureWithBiometrics ?? this.secureWithBiometrics, localPassword: localPassword ?? this.localPassword, updatedLocalPassword: updatedLocalPassword ?? this.updatedLocalPassword, @@ -75,41 +79,61 @@ class RepoSecurityState { }; bool get isSecureWithBiometricsEnabled => - isBiometricsAvailable && (origin == SecretKeyOrigin.random || store); + isBiometricsAvailable && secretWillBeStored; bool get isValid => newLocalSecretInput != null; + bool get secretWillBeStored => + origin == SecretKeyOrigin.random || + switch (userPrefersToStoreSecret) { + Some(value: final store) => store, + None() => _defaultStoreIfOriginIsManual + }; + bool get hasPendingChanges { - return origin != oldLocalSecretMode.origin || - store != oldLocalSecretMode.store.isStored || - secureWithBiometrics != - oldLocalSecretMode.store.isSecuredWithBiometrics || - (localPassword is Some && localPassword != updatedLocalPassword); + final originChanged = origin != oldLocalSecretMode.origin; + + final localPasswordChanged = + localPassword is Some && localPassword != updatedLocalPassword; + + final storeChanged = + secretWillBeStored != oldLocalSecretMode.store.isStored; + + final biometricsChanged = secureWithBiometrics != + oldLocalSecretMode.store.isSecuredWithBiometrics; + + return originChanged || + storeChanged || + biometricsChanged || + localPasswordChanged; } - LocalSecretInput? get newLocalSecretInput => - switch ((localPassword, origin, store, secureWithBiometrics)) { - (Some(value: final password), SecretKeyOrigin.manual, false, _) => - LocalSecretManual( - password: password, - store: SecretKeyStore.notStored, - ), - (Some(value: final password), SecretKeyOrigin.manual, true, false) => - LocalSecretManual( - password: password, - store: SecretKeyStore.stored, - ), - (Some(value: final password), SecretKeyOrigin.manual, true, true) => - LocalSecretManual( - password: password, - store: SecretKeyStore.securedWithBiometrics, - ), - (None(), SecretKeyOrigin.manual, _, _) => null, - (_, SecretKeyOrigin.random, _, false) => - LocalSecretRandom(secureWithBiometrics: false), - (_, SecretKeyOrigin.random, _, true) => - LocalSecretRandom(secureWithBiometrics: true), - }; + LocalSecretInput? get newLocalSecretInput { + final willStore = secretWillBeStored; + + return switch ((localPassword, origin, willStore, secureWithBiometrics)) { + (Some(value: final password), SecretKeyOrigin.manual, false, _) => + LocalSecretManual( + password: password, + store: SecretKeyStore.notStored, + ), + (Some(value: final password), SecretKeyOrigin.manual, true, false) => + LocalSecretManual( + password: password, + store: SecretKeyStore.stored, + ), + (Some(value: final password), SecretKeyOrigin.manual, true, true) => + LocalSecretManual( + password: password, + store: SecretKeyStore.securedWithBiometrics, + ), + (None(), SecretKeyOrigin.manual, _, _) => null, + (_, SecretKeyOrigin.random, _, false) => + LocalSecretRandom(secureWithBiometrics: false), + (_, SecretKeyOrigin.random, _, true) => + LocalSecretRandom(secureWithBiometrics: true), + }; + } LocalPassword? get newLocalPassword => switch ((localPassword, origin)) { (Some(value: final value), SecretKeyOrigin.manual) => value, @@ -117,7 +141,8 @@ class RepoSecurityState { }; @override - String toString() => '$runtimeType(origin: $origin, store: $store, ...)'; + String toString() => + '$runtimeType(origin: $origin, userPrefersToStoreSecret: $userPrefersToStoreSecret, ...)'; } class RepoSecurityCubit extends Cubit with AppLogger { @@ -143,7 +168,7 @@ class RepoSecurityCubit extends Cubit with AppLogger { } void setStore(bool value) { - emit(state.copyWith(store: value)); + emit(state.copyWith(userPrefersToStoreSecret: value)); } void setSecureWithBiometrics(bool value) { diff --git a/lib/app/widgets/repo_security.dart b/lib/app/widgets/repo_security.dart index 8a53a7b4..aad05fea 100644 --- a/lib/app/widgets/repo_security.dart +++ b/lib/app/widgets/repo_security.dart @@ -62,7 +62,7 @@ class RepoSecurity extends StatelessWidget { Widget _buildStoreSwitch(RepoSecurityState state) => switch (state.origin) { SecretKeyOrigin.manual => _buildSwitch( - value: state.store, + value: state.secretWillBeStored, title: S.current.labelRememberPassword, onChanged: cubit.setStore, ),