fix(app): keys an earlier build kept go too, and a locked keyring stops nothing
On Apple's systems a keychain read finds only the entries of its own accessibility, so forgetting everyone's keys but one person's missed those builds before had kept with the default or first_unlock_this_device: they stayed after signing out, a switch of person or removing the instance (the short reading's L5). forgetAllBut now also reads the keyring with every accessibility a build used and deletes each entry found by name, which a delete finds whatever its accessibility; one that can't be read now holds back none of the others. The repository's forgetting, run as the person changes and not awaited, catches a keyring that can't be read (a locked device), where its error went unhandled. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Tizian 「ティツィアーン」 <raisondetredev@pm.me>
This commit is contained in:
parent
1ca4e083bb
commit
efc1c7ffd6
4 changed files with 173 additions and 11 deletions
|
|
@ -1,6 +1,8 @@
|
|||
import 'dart:convert';
|
||||
import 'dart:developer';
|
||||
import 'dart:io';
|
||||
|
||||
import 'package:flutter/services.dart';
|
||||
import 'package:flutter_secure_storage/flutter_secure_storage.dart';
|
||||
import 'package:halkyon_app/src/password/model/unlocked_account_keys.dart';
|
||||
|
||||
|
|
@ -22,6 +24,23 @@ const accountKeyStorage = FlutterSecureStorage(
|
|||
),
|
||||
);
|
||||
|
||||
/// The keyring as builds before kept the account's keys in it on Apple's
|
||||
/// systems, with other accessibilities: the default (`unlocked`), then
|
||||
/// `first_unlock_this_device`. There a read finds only the entries of its
|
||||
/// own accessibility, so those are found through these, and deleted by
|
||||
/// name ([SecureAccountKeyStore.forgetAllBut]).
|
||||
const formerAccountKeyStorages = [
|
||||
FlutterSecureStorage(),
|
||||
FlutterSecureStorage(
|
||||
iOptions: IOSOptions(
|
||||
accessibility: KeychainAccessibility.first_unlock_this_device,
|
||||
),
|
||||
mOptions: MacOsOptions(
|
||||
accessibility: KeychainAccessibility.first_unlock_this_device,
|
||||
),
|
||||
),
|
||||
];
|
||||
|
||||
/// Where the device keeps the account's private keys once unlocked (ADR
|
||||
/// 0017): in the system's keyring, as sessions are, one person at a time
|
||||
/// per instance, gone once they sign out or another person signs in. Tests
|
||||
|
|
@ -44,7 +63,16 @@ abstract interface class AccountKeyStore() {
|
|||
class SecureAccountKeyStore(
|
||||
final String _prefix, {
|
||||
final FlutterSecureStorage _storage = accountKeyStorage,
|
||||
|
||||
/// The keyring as builds before kept keys in it; by default
|
||||
/// [formerAccountKeyStorages] on Apple's systems, none elsewhere, where a
|
||||
/// read finds every entry anyway.
|
||||
final List<FlutterSecureStorage>? formerly,
|
||||
}) implements AccountKeyStore {
|
||||
List<FlutterSecureStorage> get _formerly =>
|
||||
formerly ??
|
||||
(Platform.isIOS || Platform.isMacOS ? formerAccountKeyStorages : []);
|
||||
|
||||
String _keyOf(String person) => '$_prefix:$person';
|
||||
|
||||
/// Deletes what the keyring held for [person] first: on Apple's systems
|
||||
|
|
@ -78,10 +106,24 @@ class SecureAccountKeyStore(
|
|||
}
|
||||
}
|
||||
|
||||
/// Finds the entries kept with every accessibility a build ever used and
|
||||
/// deletes each by name, which finds it whatever its accessibility (the
|
||||
/// short reading's L5): read as kept now alone, those an earlier build
|
||||
/// kept stayed after signing out, a switch of person or removing the
|
||||
/// instance. An accessibility that can't be read now (an earlier one
|
||||
/// while the device is locked) holds back none of the others; one that
|
||||
/// fails as kept now fails it, for the caller to know.
|
||||
@override
|
||||
Future<void> forgetAllBut(String? person) async {
|
||||
final kept = await _storage.readAll();
|
||||
for (final key in kept.keys) {
|
||||
final found = {...(await _storage.readAll()).keys};
|
||||
for (final storage in _formerly) {
|
||||
try {
|
||||
found.addAll((await storage.readAll()).keys);
|
||||
} on PlatformException catch (error) {
|
||||
log('Keys kept before unreadable now: ${error.code}', name: 'Keys');
|
||||
}
|
||||
}
|
||||
for (final key in found) {
|
||||
if (key.startsWith('$_prefix:') &&
|
||||
(person == null || key != _keyOf(person))) {
|
||||
await _storage.delete(key: key);
|
||||
|
|
|
|||
|
|
@ -106,13 +106,22 @@ class PasswordRepository(
|
|||
/// minutes; a little less leaves room for the trip.
|
||||
static const _challengeFor = Duration(seconds: 90);
|
||||
|
||||
/// Forgets every person's keys but [status]'s.
|
||||
void _keepOnlyKeysOf(AuthStatus status) => unawaited(
|
||||
_keys.forgetAllBut(switch (status) {
|
||||
SignedIn(:final authUserId) => authUserId,
|
||||
SignedOut() => null,
|
||||
}),
|
||||
);
|
||||
/// Forgets every person's keys but [status]'s; a keyring that can't be
|
||||
/// read now (a locked device) is left as it is, and tried again at the
|
||||
/// next change of person (the short reading's L5).
|
||||
void _keepOnlyKeysOf(AuthStatus status) => unawaited(_forgetAllBut(status));
|
||||
|
||||
Future<void> _forgetAllBut(AuthStatus status) async {
|
||||
try {
|
||||
await _keys.forgetAllBut(switch (status) {
|
||||
SignedIn(:final authUserId) => authUserId,
|
||||
SignedOut() => null,
|
||||
});
|
||||
} on Object catch (error) {
|
||||
// Its type only: never what the keyring held.
|
||||
log('Forgetting keys failed: ${error.runtimeType}', name: 'Keys');
|
||||
}
|
||||
}
|
||||
|
||||
/// Stops following who is signed in; the owner calls it as it closes.
|
||||
Future<void> dispose() async {
|
||||
|
|
|
|||
|
|
@ -1,5 +1,4 @@
|
|||
import 'dart:typed_data';
|
||||
|
||||
import 'package:flutter/services.dart';
|
||||
import 'package:flutter_secure_storage/flutter_secure_storage.dart';
|
||||
import 'package:flutter_test/flutter_test.dart';
|
||||
import 'package:halkyon_app/halkyon_app.dart';
|
||||
|
|
@ -32,6 +31,45 @@ final class _Recording() extends Fake implements FlutterSecureStorage {
|
|||
}) async => calls.add('write $key');
|
||||
}
|
||||
|
||||
/// One accessibility's view of an Apple device's keychain ([entries]: each
|
||||
/// key's accessibility and value): a read finds only the entries of its
|
||||
/// own accessibility, a delete an entry of any; [locked] where it can't be
|
||||
/// read now.
|
||||
final class _AppleKeychain(
|
||||
final Map<String, (String, String)> entries,
|
||||
final String accessibility, {
|
||||
final bool locked = false,
|
||||
}) extends Fake implements FlutterSecureStorage {
|
||||
@override
|
||||
Future<Map<String, String>> readAll({
|
||||
AppleOptions? iOptions,
|
||||
AndroidOptions? aOptions,
|
||||
LinuxOptions? lOptions,
|
||||
WebOptions? webOptions,
|
||||
AppleOptions? mOptions,
|
||||
WindowsOptions? wOptions,
|
||||
}) async {
|
||||
if (locked) {
|
||||
throw PlatformException(code: 'Unexpected security result code');
|
||||
}
|
||||
return {
|
||||
for (final MapEntry(:key, value: (kind, value)) in entries.entries)
|
||||
if (kind == accessibility) key: value,
|
||||
};
|
||||
}
|
||||
|
||||
@override
|
||||
Future<void> delete({
|
||||
required String key,
|
||||
AppleOptions? iOptions,
|
||||
AndroidOptions? aOptions,
|
||||
LinuxOptions? lOptions,
|
||||
WebOptions? webOptions,
|
||||
AppleOptions? mOptions,
|
||||
WindowsOptions? wOptions,
|
||||
}) async => entries.remove(key);
|
||||
}
|
||||
|
||||
void main() {
|
||||
test("the account's keys open only while an Apple device is unlocked, and "
|
||||
'stay on it: never in a backup restored elsewhere, nor in iCloud '
|
||||
|
|
@ -66,4 +104,52 @@ void main() {
|
|||
|
||||
expect(keyring.calls, ['delete learn:anna', 'write learn:anna']);
|
||||
});
|
||||
|
||||
group('forgetting the keys of everyone but one (the short reading, L5)', () {
|
||||
Map<String, (String, String)> keychain() => {
|
||||
// Kept by builds before, with the accessibilities they kept them with.
|
||||
'learn:anna': ('first_unlock_this_device', 'a'),
|
||||
'learn:ben': ('unlocked', 'b'),
|
||||
'learn:carl': ('unlocked_this_device', 'c'),
|
||||
'learn:dana': ('unlocked_this_device', 'd'),
|
||||
'todo:emil': ('first_unlock_this_device', 'e'),
|
||||
};
|
||||
|
||||
test('deletes those kept with another accessibility too, by name, and '
|
||||
"nothing of another instance's", () async {
|
||||
final entries = keychain();
|
||||
final store = SecureAccountKeyStore(
|
||||
'learn',
|
||||
storage: _AppleKeychain(entries, 'unlocked_this_device'),
|
||||
formerly: [
|
||||
_AppleKeychain(entries, 'first_unlock_this_device'),
|
||||
_AppleKeychain(entries, 'unlocked'),
|
||||
],
|
||||
);
|
||||
|
||||
await store.forgetAllBut('carl');
|
||||
|
||||
expect(entries.keys, unorderedEquals(['learn:carl', 'todo:emil']));
|
||||
});
|
||||
|
||||
test('an accessibility that cannot be read now holds back nothing '
|
||||
'else', () async {
|
||||
final entries = keychain();
|
||||
final store = SecureAccountKeyStore(
|
||||
'learn',
|
||||
storage: _AppleKeychain(entries, 'unlocked_this_device'),
|
||||
formerly: [
|
||||
_AppleKeychain(entries, 'first_unlock_this_device', locked: true),
|
||||
_AppleKeychain(entries, 'unlocked'),
|
||||
],
|
||||
);
|
||||
|
||||
await store.forgetAllBut(null);
|
||||
|
||||
expect(
|
||||
entries.keys,
|
||||
unorderedEquals(['learn:anna', 'todo:emil']),
|
||||
);
|
||||
});
|
||||
});
|
||||
}
|
||||
|
|
|
|||
|
|
@ -17,6 +17,14 @@ final class _UnreadableKeys() implements AccountKeyStore {
|
|||
Future<void> forgetAllBut(String? person) async {}
|
||||
}
|
||||
|
||||
/// A keyring that can't be read while the device is locked: forgetting
|
||||
/// fails.
|
||||
final class _LockedKeyring() extends MemoryAccountKeyStore {
|
||||
@override
|
||||
Future<void> forgetAllBut(String? person) =>
|
||||
Future.error(PlatformException(code: 'locked'));
|
||||
}
|
||||
|
||||
Matcher failsWith(PasswordProblem problem) => throwsA(
|
||||
isA<PasswordFailed>().having((failed) => failed.problem, 'problem', problem),
|
||||
);
|
||||
|
|
@ -42,6 +50,23 @@ void main() {
|
|||
/// Lets the keys' forgetting, which follows the status, run.
|
||||
Future<void> settle() => Future<void>.delayed(Duration.zero);
|
||||
|
||||
test('a keyring locked as someone signs in or out is left as it is: the '
|
||||
'app goes on (the short reading, L5)', () async {
|
||||
final locked = PasswordRepository(
|
||||
gateway,
|
||||
crypto,
|
||||
_LockedKeyring(),
|
||||
auth,
|
||||
addresses,
|
||||
);
|
||||
addTearDown(locked.dispose);
|
||||
|
||||
await auth.signInAnonymously();
|
||||
await settle();
|
||||
await auth.signOut();
|
||||
await settle();
|
||||
});
|
||||
|
||||
group('signing in', () {
|
||||
late FakePasswordAccount anna;
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue