fix(secure_storage): clean up unused data (#150)

tsutsu3 · Feb 22, 2025 · 6a77cae · 6a77cae
1 parent 89af50b
commit 6a77cae
Show file tree

Hide file tree

Showing 7 changed files with 145 additions and 8 deletions.
diff --git a/lib/repository/database.dart b/lib/repository/database.dart
@@ -54,6 +54,8 @@ class DatabaseRepository {
     _servers = piHoleClientData.servers;
     _appConfig = piHoleClientData.appConfig;
     _dbInstance = piHoleClientData.dbInstance;
+    await cleanUpSecureStorage();
+    logger.d('Secure storage: ${await _secureStorage.readAll()}');
   }
 
   Map<String, dynamic> toDict() {
@@ -149,9 +151,6 @@ class DatabaseRepository {
         final passCode = await _secureStorage.getValue('passCode');
         appConfig = AppDbData.withSecrets(appConfig!, passCode);
 
-        // _secureStorage.readAll()
-        logger.d((await _secureStorage.readAll()).toString());
-
         if (servers != null && servers!.isNotEmpty) {
           for (var i = 0; i < servers!.length; i++) {
             final server = servers![i];
@@ -512,6 +511,38 @@ class DatabaseRepository {
     }
   }
 
+  /// Delete and clean up unused secure storage data for servers not present in the database.
+  /// This method is intended to be called during application startup.
+  Future<void> cleanUpSecureStorage() async {
+    final serverAddresses = servers.map((e) => e.address).toList();
+    final keys = await _secureStorage.readAll();
+
+    // Collect keys to be deleted
+    final keysToDelete = <String>[];
+
+    for (final key in keys.keys) {
+      if (key.endsWith('_token') ||
+          key.endsWith('_basicAuthUser') ||
+          key.endsWith('_basicAuthPassword') ||
+          key.endsWith('_password') ||
+          key.endsWith('_sid')) {
+        final address = key.split('_').first;
+        if (!serverAddresses.contains(address)) {
+          keysToDelete.add(key);
+        }
+      }
+    }
+
+    // Delete collected keys separately
+    for (final key in keysToDelete) {
+      await _secureStorage.deleteValue(key);
+    }
+  }
+
+  // ==========================================================================
+  // MIGRATION METHODS
+  // ==========================================================================
+
   // Migration methods
   Future<dynamic> _upgradeToV2(Database db) async {
     // await db.execute('ALTER TABLE appConfig DROP COLUMN oneColumnLegend');

diff --git a/lib/screens/servers/add_server_fullscreen.dart b/lib/screens/servers/add_server_fullscreen.dart
@@ -278,6 +278,10 @@ class _AddServerFullscreenState extends State<AddServerFullscreen> {
               isConnecting = false;
               _restoreSecrets();
             });
+
+            await serverObj.sm.deletePassword();
+            await serverObj.sm.deleteToken();
+
             if (result?.result == APiResponseType.socket) {
               showErrorSnackBar(
                 context: context,

diff --git a/lib/services/secret_manager.dart b/lib/services/secret_manager.dart
@@ -71,4 +71,22 @@ class SecretManager {
       return false;
     }
   }
+
+  Future<bool> deletePassword() async {
+    try {
+      await _storage.deleteValue('${_address}_password');
+      return true;
+    } catch (e) {
+      return false;
+    }
+  }
+
+  Future<bool> deleteToken() async {
+    try {
+      await _storage.deleteValue('${_address}_token');
+      return true;
+    } catch (e) {
+      return false;
+    }
+  }
 }
diff --git a/test/full_coverage_test.dart b/test/full_coverage_test.dart
@@ -31,6 +31,12 @@ import 'package:pi_hole_client/gateways/api_gateway_factory.dart';
 import 'package:pi_hole_client/gateways/api_gateway_interface.dart';
 import 'package:pi_hole_client/gateways/v5/api_gateway_v5.dart';
 import 'package:pi_hole_client/gateways/v6/api_gateway_v6.dart';
+import 'package:pi_hole_client/l10n/generated/app_localizations.dart';
+import 'package:pi_hole_client/l10n/generated/app_localizations_de.dart';
+import 'package:pi_hole_client/l10n/generated/app_localizations_en.dart';
+import 'package:pi_hole_client/l10n/generated/app_localizations_es.dart';
+import 'package:pi_hole_client/l10n/generated/app_localizations_ja.dart';
+import 'package:pi_hole_client/l10n/generated/app_localizations_pl.dart';
 import 'package:pi_hole_client/main.dart';
 import 'package:pi_hole_client/models/api/v6/auth/auth.dart';
 import 'package:pi_hole_client/models/api/v6/dns/dns.dart';

diff --git a/test/ut/gateways/v6/api_gateway_v6_test.dart b/test/ut/gateways/v6/api_gateway_v6_test.dart
@@ -67,6 +67,18 @@ class SecretManagerMock implements SecretManager {
     _sid = token;
     return true;
   }
+
+  @override
+  Future<bool> deletePassword() async {
+    _password = null;
+    return true;
+  }
+
+  @override
+  Future<bool> deleteToken() async {
+    _sid = null;
+    return true;
+  }
 }
 
 @GenerateMocks([http.Client])

diff --git a/test/ut/repository/database_test.dart b/test/ut/repository/database_test.dart
@@ -58,7 +58,6 @@ void main() async {
       expect(appConfig.passCode, isNull);
       expect(appConfig.useBiometricAuth, 0);
       expect(appConfig.sendCrashReports, 0);
-      // TODO: check logger messages
     });
 
     test('should open database', () async {
@@ -83,7 +82,6 @@ void main() async {
       expect(appConfig.passCode, isNull);
       expect(appConfig.useBiometricAuth, 0);
       expect(appConfig.sendCrashReports, 0);
-      // TODO: check logger messages
     });
 
     test('should return one registered server without passcode', () async {
@@ -822,4 +820,75 @@ void main() async {
       },
     );
   });
+
+  group('DatabaseRepository.cleanUpSecureStorage', () {
+    late DbHelper dbHelper;
+
+    setUp(() async {
+      await deleteDatabase(testDb);
+      FlutterSecureStorage.setMockInitialValues({
+        'http://localhost_password': 'test123',
+        'http://localhost_token': '',
+        'http://localhost_sid': 'XXXXXxxx1234Q=',
+      });
+    });
+
+    tearDown(() async {
+      await deleteDatabase(testDb);
+    });
+
+    test('should not cleanup secure storage', () async {
+      final secureStorageRepository = SecureStorageRepository();
+      final server = Server(
+        address: 'http://localhost',
+        alias: 'test v6',
+        defaultServer: false,
+        apiVersion: 'v6',
+        sm: SecretManager(
+          secureStorageRepository,
+          'http://localhost',
+        ),
+      );
+
+      dbHelper = DbHelper(testDb);
+      await dbHelper.loadDb();
+      await dbHelper.saveDb(server);
+      await dbHelper.closeDb();
+
+      final databaseRepository = DatabaseRepository(secureStorageRepository);
+      await databaseRepository.initialize(path: testDb);
+
+      final actual = await secureStorageRepository.readAll();
+      expect(actual['http://localhost_password'], 'test123');
+      expect(actual['http://localhost_token'], '');
+      expect(actual['http://localhost_sid'], 'XXXXXxxx1234Q=');
+    });
+
+    test('should cleanup secure storage', () async {
+      final secureStorageRepository = SecureStorageRepository();
+      final server = Server(
+        address: 'http://localhost:8080',
+        alias: 'test v6',
+        defaultServer: false,
+        apiVersion: 'v6',
+        sm: SecretManager(
+          secureStorageRepository,
+          'http://localhost:8080',
+        ),
+      );
+
+      dbHelper = DbHelper(testDb);
+      await dbHelper.loadDb();
+      await dbHelper.saveDb(server);
+      await dbHelper.closeDb();
+
+      final databaseRepository = DatabaseRepository(secureStorageRepository);
+      await databaseRepository.initialize(path: testDb);
+
+      final actual = await secureStorageRepository.readAll();
+      expect(actual['http://localhost_password'], null);
+      expect(actual['http://localhost_token'], null);
+      expect(actual['http://localhost_sid'], null);
+    });
+  });
 }
diff --git a/test/widgets/screens/statistics/statistics_test.dart b/test/widgets/screens/statistics/statistics_test.dart
@@ -9,7 +9,6 @@ import 'package:pi_hole_client/screens/statistics/statistics_triple_column.dart'
 import 'package:pie_chart/pie_chart.dart';
 
 import '../../helpers.dart';
-import '../utils.dart';
 
 void main() async {
   await initializeApp();
@@ -113,7 +112,6 @@ void main() async {
           // Switch to Domains tab
           await tester.tap(find.text('Domains'));
           await tester.pump();
-          showText();
           expect(find.text('Loading stats...'), findsOneWidget);
           expect(find.byType(StatisticsListContent), findsNothing);
         },
@@ -147,7 +145,6 @@ void main() async {
           // Switch to Domains tab
           await tester.tap(find.text('Domains'));
           await tester.pump();
-          showText();
           expect(find.text('Stats could not be loaded'), findsOneWidget);
           expect(find.byType(StatisticsListContent), findsNothing);
         },