From 22a6602f08bfbb2f3914900ad7e278bc3b25dae1 Mon Sep 17 00:00:00 2001 From: Ayushya Amitabh Date: Tue, 6 Oct 2026 20:07:28 -0400 Subject: [PATCH] Security locks work independently; other-account avatars flush right in the avatar menu Login lock, lock account switching and lock hidden files no longer depend on each other: passGate prompts on its own flag, the two sub-locks can be set with login lock off, and disabling login lock leaves them alone. Turning any lock on or off asks for device auth (on also checks the device can). The iOS Share Extension's unlock rules follow. The avatar dropdown's additional accounts lose the 44px spacer so their avatars sit at the right edge. Co-Authored-By: Claude Sonnet 5.5 --- .claude/context/architecture.md | 4 +- .claude/context/server.md | 23 ++- ios/RunnerTests/RunnerTests.swift | 11 +- ios/Shared/SharedAccount.swift | 9 +- lib/providers/session_controller.dart | 67 ++++++--- lib/services/app_lock_service.dart | 10 ++ lib/widgets/avatar_menu.dart | 1 - lib/widgets/settings/settings_security.dart | 51 +++++-- test/providers/security_locks_test.dart | 156 ++++++++++++++++++++ test/widgets/avatar_menu_test.dart | 46 ++++++ 10 files changed, 323 insertions(+), 55 deletions(-) create mode 100644 test/providers/security_locks_test.dart diff --git a/.claude/context/architecture.md b/.claude/context/architecture.md index 0ac3fad..7a18964 100644 --- a/.claude/context/architecture.md +++ b/.claude/context/architecture.md @@ -57,8 +57,8 @@ new provider instance): saved-accounts list, which one is active, `sessionGeneration` — see below), auth/login-flow state (`isLoggedIn`, `isRestoringSession`, `loginFlowStatus`), the active `NextcloudService` instance, and login lock - (`loginLockEnabled`/`lockAccountSwitching`/`lockHiddenFiles`/ - `needsUnlock`/`passGate`). Exposes `addAccountClearedListener`/ + (`loginLockEnabled`/`lockAccountSwitching`/`lockHiddenFiles` - three + independent locks - plus `needsUnlock`/`passGate`). Exposes `addAccountClearedListener`/ `addAccountActivatedListener` (plain `List`) so sibling controllers — constructed after `SessionController` and unable to hold a forward reference to it — can react to login/logout/account-switch diff --git a/.claude/context/server.md b/.claude/context/server.md index ddac4dc..74f0e99 100644 --- a/.claude/context/server.md +++ b/.claude/context/server.md @@ -798,6 +798,19 @@ plugin's own manifest via merge, but kept explicit here too). own requirement) via `maxOf(24, flutter.minSdkVersion)` rather than trusting Flutter's own default to already be high enough. +**The three locks are independent** (Settings -> Security; +`SessionController`): `loginLockEnabled` (unlock to open the app - the only one +`needsUnlock`/the lock screen look at), `lockAccountSwitching` (unlock to +switch accounts) and `lockHiddenFiles` (unlock to turn on showing hidden +files). Each gate works with the others off - `passGate(gate, reason)` +prompts whenever its own `gate` is on, whether or not login lock is. Turning +any of them on **or off** needs a successful `AppLockService.authenticate` +(on also needs `isDeviceSupported`, so a gate nobody can pass can't be set; +off needs it so someone with a momentarily unlocked phone can't remove it), +and `disableLoginLock` leaves the other two untouched. The old master/sub-toggle +dependency is gone. Tests replace the prompt through +`AppLockService.debugAuthenticate`/`debugIsDeviceSupported`. + `isRestoringSession` still gates the splash screen until the above resolves — see `standards.md` for why widget tests must mock both storage channels rather than relying on this async path throwing naturally. @@ -903,16 +916,16 @@ Reminders/Notes - the destination is picked *inside* the sheet: setting of their own. `hide` (default) drops dot-folders, `only` lists just them, `include` lists everything; a folder is hidden when it *or any ancestor* starts with a dot (`HiddenFilter`, same rule as the app). - When the filter isn't `hide` and the app has login lock + "lock hidden - files" on, the sheet asks for Face ID/passcode first (`DeviceAuth`, the + When the filter isn't `hide` and the app's "lock hidden files" is on, + the sheet asks for Face ID/passcode first (`DeviceAuth`, the `.deviceOwnerAuthentication` policy - biometrics with passcode fallback, like `local_auth` with `biometricOnly: false`); cancelling falls back to hiding them, with a note. - **Account switching**: choosing any account other than the app's - active one asks for the same unlock when login lock + "lock account - switching" are on. One successful unlock covers the rest of that sheet. + active one asks for the same unlock when "lock account switching" is on. One successful unlock covers the rest of that sheet. - Opening the sheet itself is not gated - only these two actions are - (as in the app, where login lock guards launch and these toggles). + (as in the app, where login lock guards launch and these are separate + locks). 3. **Upload** calls `ShareUpload.enqueue`: one `PUT` per file on a *background* `URLSession` (`dev.ayushya.noo.transfers.share`, with `sharedContainerIdentifier`) that outlives the extension, and posts diff --git a/ios/RunnerTests/RunnerTests.swift b/ios/RunnerTests/RunnerTests.swift index ff1b899..f390cdd 100644 --- a/ios/RunnerTests/RunnerTests.swift +++ b/ios/RunnerTests/RunnerTests.swift @@ -204,18 +204,17 @@ class RunnerTests: XCTestCase { XCTAssertEqual(shared.active?.id, "alice") } - func testUnlockRulesNeedTheMasterLockAndTheirOwnToggle() { + func testUnlockRulesAreIndependentOfLoginLock() { func rules(master: Bool, switching: Bool, hidden: Bool) -> (Bool, Bool) { let shared = SharedAccounts( accounts: [account("a")], activeId: "a", loginLockEnabled: master, lockAccountSwitching: switching, lockHiddenFiles: hidden) return (shared.needsUnlockToSwitchAccount, shared.needsUnlockForHidden) } - XCTAssertTrue(rules(master: true, switching: true, hidden: false) == (true, false)) - XCTAssertTrue(rules(master: true, switching: false, hidden: true) == (false, true)) - XCTAssertTrue( - rules(master: false, switching: true, hidden: true) == (false, false), - "the sub-toggles mean nothing while the login lock is off, as in the app") + XCTAssertTrue(rules(master: false, switching: true, hidden: false) == (true, false)) + XCTAssertTrue(rules(master: false, switching: false, hidden: true) == (false, true)) + XCTAssertTrue(rules(master: true, switching: false, hidden: false) == (false, false)) + XCTAssertTrue(rules(master: true, switching: true, hidden: true) == (true, true)) } // MARK: - TransferBatchStore diff --git a/ios/Shared/SharedAccount.swift b/ios/Shared/SharedAccount.swift index bf98259..2423ff8 100644 --- a/ios/Shared/SharedAccount.swift +++ b/ios/Shared/SharedAccount.swift @@ -23,8 +23,9 @@ struct SharedAccount: Codable, Equatable, Identifiable { struct SharedAccounts: Codable, Equatable { var accounts: [SharedAccount] var activeId: String? - /// Settings -> Security: the master "login lock" and its two sub-toggles. - /// The sub-toggles only count while the master one is on (as in the app). + /// Settings -> Security: the three independent locks. `loginLockEnabled` + /// (unlock to open the app) is carried for completeness; the extension's own + /// gates are the other two. var loginLockEnabled: Bool var lockAccountSwitching: Bool var lockHiddenFiles: Bool @@ -36,11 +37,11 @@ struct SharedAccounts: Codable, Equatable { /// Uploading to an account other than the active one is "switching" in /// the app's terms, so it needs the same unlock. - var needsUnlockToSwitchAccount: Bool { loginLockEnabled && lockAccountSwitching } + var needsUnlockToSwitchAccount: Bool { lockAccountSwitching } /// Showing hidden folders needs the same unlock the app asks for when you /// turn hidden files on. - var needsUnlockForHidden: Bool { loginLockEnabled && lockHiddenFiles } + var needsUnlockForHidden: Bool { lockHiddenFiles } } /// Keeps the [SharedAccounts] in a Keychain access group both the app and the diff --git a/lib/providers/session_controller.dart b/lib/providers/session_controller.dart index 2504d80..42f8636 100644 --- a/lib/providers/session_controller.dart +++ b/lib/providers/session_controller.dart @@ -516,7 +516,7 @@ class SessionController extends ChangeNotifier with WidgetsBindingObserver { /// session-restore that failed, e.g. transient network trouble at cold /// start), this still retries rather than no-op, since that's exactly the /// case LoginView's "Continue as" tile exists to recover from. Gated - /// behind login lock when [lockAccountSwitching] is on. Returns whether + /// behind its own unlock when [lockAccountSwitching] is on. Returns whether /// the account ended up logged in, so callers (LoginView's saved-account /// tile) can surface a failure - e.g. a stored app password that no /// longer works and needs the account removed/re-added. @@ -640,10 +640,10 @@ class SessionController extends ChangeNotifier with WidgetsBindingObserver { return true; } - /// Turns login lock off, along with both of its sub-toggles (meaningless - /// once the base lock is gone). Requires a successful auth first, same as - /// turning it on - otherwise anyone with momentary access to an unlocked - /// phone could just switch it off. + /// Turns login lock off. Requires a successful auth first, same as turning + /// it on - otherwise anyone with momentary access to an unlocked phone + /// could just switch it off. Only this lock: the account-switching and + /// hidden-files locks are independent and stay as they are. Future disableLoginLock() async { if (!_loginLockEnabled) return true; final confirmed = await AppLockService.authenticate( @@ -651,30 +651,55 @@ class SessionController extends ChangeNotifier with WidgetsBindingObserver { ); if (!confirmed) return false; _loginLockEnabled = false; - _lockAccountSwitching = false; - _lockHiddenFiles = false; _isUnlocked = false; notifyListeners(); - prefsFuture.then((p) { - p.setBool(_prefLoginLockEnabled, false); - p.setBool(_prefLockAccountSwitching, false); - p.setBool(_prefLockHiddenFiles, false); - }); + prefsFuture.then((p) => p.setBool(_prefLoginLockEnabled, false)); return true; } - void setLockAccountSwitching(bool value) { - if (!_loginLockEnabled) return; + /// Turns the "unlock to switch accounts" gate on or off. Independent of + /// login lock. Both directions need a successful auth - enabling proves the + /// device can authenticate at all (a gate nobody can pass would lock the + /// user out of switching), disabling stops anyone with momentary access to + /// an unlocked phone from just removing it. Returns whether it changed. + Future setLockAccountSwitching(bool value) async { + if (value == _lockAccountSwitching) return true; + if (!await _confirmLockChange( + value, + 'Confirm to lock account switching', + 'Confirm to unlock account switching', + )) { + return false; + } _lockAccountSwitching = value; notifyListeners(); prefsFuture.then((p) => p.setBool(_prefLockAccountSwitching, value)); + return true; } - void setLockHiddenFiles(bool value) { - if (!_loginLockEnabled) return; + /// Same as [setLockAccountSwitching], for revealing hidden files. + Future setLockHiddenFiles(bool value) async { + if (value == _lockHiddenFiles) return true; + if (!await _confirmLockChange( + value, + 'Confirm to lock hidden files', + 'Confirm to unlock hidden files', + )) { + return false; + } _lockHiddenFiles = value; notifyListeners(); prefsFuture.then((p) => p.setBool(_prefLockHiddenFiles, value)); + return true; + } + + Future _confirmLockChange( + bool enabling, + String enableReason, + String disableReason, + ) async { + if (enabling && !await AppLockService.isDeviceSupported()) return false; + return AppLockService.authenticate(enabling ? enableReason : disableReason); } /// Called by the lock screen. Returns whether it actually unlocked. @@ -687,12 +712,12 @@ class SessionController extends ChangeNotifier with WidgetsBindingObserver { return success; } - /// Prompts for auth if [gate] is on and login lock is configured; - /// returns true immediately (no prompt) otherwise. Shared by the - /// account-switching gate above and [FilesController]'s/`PhotosController`'s - /// hidden-files gates. + /// Prompts for auth if [gate] is on; returns true immediately (no prompt) + /// otherwise. Each gate stands on its own - it doesn't depend on login lock + /// being on. Shared by the account-switching gate above and + /// [FilesController]'s/`PhotosController`'s hidden-files gates. Future passGate(bool gate, String reason) async { - if (!_loginLockEnabled || !gate) return true; + if (!gate) return true; return AppLockService.authenticate(reason); } diff --git a/lib/services/app_lock_service.dart b/lib/services/app_lock_service.dart index 8a205d7..7a0cda6 100644 --- a/lib/services/app_lock_service.dart +++ b/lib/services/app_lock_service.dart @@ -1,3 +1,4 @@ +import 'package:flutter/foundation.dart'; import 'package:local_auth/local_auth.dart'; /// Thin wrapper around `local_auth`. Deliberately doesn't implement its own @@ -8,9 +9,17 @@ import 'package:local_auth/local_auth.dart'; class AppLockService { static final LocalAuthentication _auth = LocalAuthentication(); + /// Replace the platform prompt in tests (null = the real one). + @visibleForTesting + static Future Function(String reason)? debugAuthenticate; + + @visibleForTesting + static Future Function()? debugIsDeviceSupported; + /// Whether this device can do *some* form of local auth - biometric /// enrolled, or at minimum a device PIN/pattern/password set up. static Future isDeviceSupported() async { + if (debugIsDeviceSupported != null) return debugIsDeviceSupported!(); try { final canCheckBiometrics = await _auth.canCheckBiometrics; if (canCheckBiometrics) return true; @@ -24,6 +33,7 @@ class AppLockService { /// throws) on cancellation, failure, or any platform error, so callers /// can treat every non-true result the same way: stay locked/blocked. static Future authenticate(String reason) async { + if (debugAuthenticate != null) return debugAuthenticate!(reason); try { return await _auth.authenticate( localizedReason: reason, diff --git a/lib/widgets/avatar_menu.dart b/lib/widgets/avatar_menu.dart index 3c13dcc..898c88e 100644 --- a/lib/widgets/avatar_menu.dart +++ b/lib/widgets/avatar_menu.dart @@ -332,7 +332,6 @@ class _OtherAccountRow extends StatelessWidget { ), const SizedBox(width: 12), NooAvatar(initials: accountInitial(name), current: false, size: 40), - const SizedBox(width: 44), ], ), ), diff --git a/lib/widgets/settings/settings_security.dart b/lib/widgets/settings/settings_security.dart index 341ab93..e9de0de 100644 --- a/lib/widgets/settings/settings_security.dart +++ b/lib/widgets/settings/settings_security.dart @@ -26,26 +26,28 @@ String biometricLabel(TargetPlatform platform) { } } -/// Settings section 3: login lock, which gates opening the app, switching -/// accounts, and revealing hidden files behind the device's own PIN/ +/// Settings section 3: three independent locks - opening the app, switching +/// accounts, and revealing hidden files - each behind the device's own PIN/ /// biometric credential (see `AppLockService` - this app never stores or -/// handles a PIN itself). +/// handles a PIN itself). Turning one on or off asks for that credential, and +/// none of them requires or implies another. class SettingsSecuritySection extends StatelessWidget { const SettingsSecuritySection({super.key}); - Future _handleLoginLockChanged( - BuildContext context, - SessionController session, - bool value, - ) async { - final success = value ? await session.setupLoginLock() : await session.disableLoginLock(); + Future _handleLockChanged( + BuildContext context, { + required Future Function() change, + required bool turningOn, + required String name, + }) async { + final success = await change(); if (!success && context.mounted) { ScaffoldMessenger.of(context).showSnackBar( SnackBar( content: Text( - value - ? "Could not set up login lock - make sure this device has a PIN, pattern, password, or biometric configured" - : 'Could not turn off login lock', + turningOn + ? "Could not turn on $name - make sure this device has a PIN, pattern, password, or biometric configured" + : 'Could not turn off $name', ), behavior: SnackBarBehavior.floating, ), @@ -64,10 +66,17 @@ class SettingsSecuritySection extends StatelessWidget { NooSettingsRow( icon: LucideIcons.lock, label: Text(biometricLabel(platform)), - subtitle: const Text("Require this device's PIN or biometric to open Noo"), + subtitle: const Text( + "Require this device's PIN or biometric to open Noo", + ), trailing: NooToggle( checked: session.loginLockEnabled, - onChanged: (value) => _handleLoginLockChanged(context, session, value), + onChanged: (value) => _handleLockChanged( + context, + change: value ? session.setupLoginLock : session.disableLoginLock, + turningOn: value, + name: 'login lock', + ), ), ), NooSettingsRow( @@ -76,7 +85,12 @@ class SettingsSecuritySection extends StatelessWidget { subtitle: const Text('Unlock to switch between saved accounts'), trailing: NooToggle( checked: session.lockAccountSwitching, - onChanged: session.loginLockEnabled ? session.setLockAccountSwitching : null, + onChanged: (value) => _handleLockChanged( + context, + change: () => session.setLockAccountSwitching(value), + turningOn: value, + name: 'account switching lock', + ), ), ), NooSettingsRow( @@ -85,7 +99,12 @@ class SettingsSecuritySection extends StatelessWidget { subtitle: const Text('Unlock to reveal hidden files and folders'), trailing: NooToggle( checked: session.lockHiddenFiles, - onChanged: session.loginLockEnabled ? session.setLockHiddenFiles : null, + onChanged: (value) => _handleLockChanged( + context, + change: () => session.setLockHiddenFiles(value), + turningOn: value, + name: 'hidden files lock', + ), ), ), ], diff --git a/test/providers/security_locks_test.dart b/test/providers/security_locks_test.dart new file mode 100644 index 0000000..be09cab --- /dev/null +++ b/test/providers/security_locks_test.dart @@ -0,0 +1,156 @@ +import 'package:flutter/services.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:shared_preferences/shared_preferences.dart'; +import 'package:noo/providers/connectivity_controller.dart'; +import 'package:noo/providers/session_controller.dart'; +import 'package:noo/services/app_lock_service.dart'; + +/// The three Settings -> Security locks (open the app, switch accounts, reveal +/// hidden files) each work on their own - none needs or implies another. The +/// device prompt is replaced with a recorder so the gates can be exercised. +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + setUpAll(() { + final messenger = + TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger; + messenger.setMockMethodCallHandler( + const MethodChannel('plugins.it_nomads.com/flutter_secure_storage'), + (call) async => call.method == 'readAll' ? {} : null, + ); + messenger.setMockMethodCallHandler( + const MethodChannel('dev.fluttercommunity.plus/connectivity'), + (call) async => call.method == 'check' ? ['none'] : null, + ); + messenger.setMockStreamHandler( + const EventChannel('dev.fluttercommunity.plus/connectivity_status'), + MockStreamHandler.inline(onListen: (arguments, events) {}), + ); + }); + + late List prompts; + var authSucceeds = true; + var deviceSupported = true; + + setUp(() { + SharedPreferences.setMockInitialValues({}); + prompts = []; + authSucceeds = true; + deviceSupported = true; + AppLockService.debugAuthenticate = (reason) async { + prompts.add(reason); + return authSucceeds; + }; + AppLockService.debugIsDeviceSupported = () async => deviceSupported; + }); + + tearDown(() { + AppLockService.debugAuthenticate = null; + AppLockService.debugIsDeviceSupported = null; + }); + + SessionController build() => SessionController(ConnectivityController()); + + test( + 'hidden files and account switching can be locked without login lock', + () async { + final session = build(); + + expect(await session.setLockHiddenFiles(true), isTrue); + expect(await session.setLockAccountSwitching(true), isTrue); + + expect(session.lockHiddenFiles, isTrue); + expect(session.lockAccountSwitching, isTrue); + expect(session.loginLockEnabled, isFalse); + expect( + session.needsUnlock, + isFalse, + reason: 'opening the app stays open', + ); + }, + ); + + test('a gate prompts on its own, with login lock off', () async { + final session = build(); + + expect(await session.passGate(false, 'unused'), isTrue); + expect(prompts, isEmpty, reason: 'a gate that is off never prompts'); + + expect(await session.passGate(true, 'Unlock to show hidden files'), isTrue); + expect(prompts, ['Unlock to show hidden files']); + + authSucceeds = false; + expect(await session.passGate(true, 'again'), isFalse); + }); + + test( + 'turning a lock on needs a capable device and a successful auth', + () async { + final session = build(); + + deviceSupported = false; + expect(await session.setLockHiddenFiles(true), isFalse); + expect(session.lockHiddenFiles, isFalse); + + deviceSupported = true; + authSucceeds = false; + expect(await session.setLockHiddenFiles(true), isFalse); + expect(session.lockHiddenFiles, isFalse); + + authSucceeds = true; + expect(await session.setLockHiddenFiles(true), isTrue); + expect(session.lockHiddenFiles, isTrue); + }, + ); + + test( + 'turning a lock off needs auth, so it cannot just be flipped away', + () async { + final session = build(); + await session.setLockAccountSwitching(true); + prompts.clear(); + + authSucceeds = false; + expect(await session.setLockAccountSwitching(false), isFalse); + expect(session.lockAccountSwitching, isTrue); + + authSucceeds = true; + expect(await session.setLockAccountSwitching(false), isTrue); + expect(session.lockAccountSwitching, isFalse); + expect(prompts, hasLength(2)); + }, + ); + + test('setting a lock to its current value does not prompt', () async { + final session = build(); + expect(await session.setLockHiddenFiles(false), isTrue); + expect(prompts, isEmpty); + }); + + test('turning login lock off leaves the other two locks alone', () async { + final session = build(); + await session.setupLoginLock(); + await session.setLockAccountSwitching(true); + await session.setLockHiddenFiles(true); + expect(session.loginLockEnabled, isTrue); + + expect(await session.disableLoginLock(), isTrue); + + expect(session.loginLockEnabled, isFalse); + expect(session.lockAccountSwitching, isTrue); + expect(session.lockHiddenFiles, isTrue); + }); + + test('each lock is saved on its own and restored', () async { + final first = build(); + await first.setLockHiddenFiles(true); + // Let the fire-and-forget pref write land. + await Future.delayed(const Duration(milliseconds: 50)); + + final second = build(); + await Future.delayed(const Duration(milliseconds: 100)); + expect(second.lockHiddenFiles, isTrue); + expect(second.lockAccountSwitching, isFalse); + expect(second.loginLockEnabled, isFalse); + }); +} diff --git a/test/widgets/avatar_menu_test.dart b/test/widgets/avatar_menu_test.dart index 2d703fc..6fc1dd9 100644 --- a/test/widgets/avatar_menu_test.dart +++ b/test/widgets/avatar_menu_test.dart @@ -15,6 +15,8 @@ import 'package:noo/providers/settings_controller.dart'; import 'package:noo/providers/sync_status_controller.dart'; import 'package:noo/providers/trash_controller.dart'; import 'package:noo/theme/app_theme.dart'; +import 'package:noo/theme/design_tokens.dart'; +import 'package:noo/widgets/noo/core/noo_avatar.dart'; import 'package:noo/widgets/app_top_bar.dart'; import 'package:noo/widgets/noo/nav/noo_top_bar.dart'; @@ -174,4 +176,48 @@ void main() { expect(find.text('Add account'), findsOneWidget); }, ); + + testWidgets("other accounts' avatars sit flush right in the dropdown", ( + tester, + ) async { + SharedPreferences.setMockInitialValues({ + 'account_migration_v1_done': true, + 'accounts_list': jsonEncode([ + const SavedAccount( + id: accountId, + serverUrl: 'https://server.example.com', + username: 'alice', + ).toJson(), + const SavedAccount( + id: 'other_example_org__bob', + serverUrl: 'https://other.example.org', + username: 'bob', + ).toJson(), + ]), + 'active_account_id': accountId, + }); + await pumpTopBar(tester, navMenuStyle: NooNavMenuStyle.avatarMenu); + + await tester.tap(find.byTooltip('Menu')); + await tester.pump(); + await tester.pump(const Duration(milliseconds: 200)); + // Expand the account list from the header. + await tester.tap(find.text('alice')); + await tester.pump(); + await tester.pump(const Duration(milliseconds: 400)); + + final row = find.ancestor( + of: find.text('bob'), + matching: find.byType(InkWell), + ); + final avatar = find.descendant(of: row, matching: find.byType(NooAvatar)); + expect(avatar, findsOneWidget); + + // Only the row's own padding between the avatar and the card's edge - + // no extra gutter pushing it in from the right. + expect( + tester.getTopRight(row).dx - tester.getTopRight(avatar).dx, + NooSpace.md, + ); + }); }