Refactor secure storage and push request repository

This commit refactors the code by introducing a new class called SecureStorageMutexed. This class provides a protected way to access the FlutterSecureStorage, ensuring that calls to read, write, delete, and deleteAll methods are executed serially. The SecurePushRequestRepository class now uses the SecureStorageMutexed class to handle the storage of push request state securely.

The changes include:
- Adding the SecureStorageMutexed class to handle protected access to FlutterSecureStorage.
- Updating the SecurePushRequestRepository class to use the SecureStorageMutexed class for storage operations.
- Removing the protect method from the SecurePushRequestRepository class.

These changes improve the code structure and ensure thread-safe access to the secure storage.

lib/repo/secure_push_request_repository.dart

@@ -1,84 +1,88 @@
+/*
+ * privacyIDEA Authenticator
+ *
+ * Author: Frank Merkel <[email protected]>
+ *
+ * Copyright (c) 2024 NetKnights GmbH
+ *
+ * Licensed under the Apache License, Version 2.0 (the 'License');
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an 'AS IS' BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
 // ignore_for_file: constant_identifier_names
 
 import 'dart:convert';
 
-import 'package:flutter_secure_storage/flutter_secure_storage.dart';
-import 'package:mutex/mutex.dart';
-
 import '../interfaces/repo/push_request_repository.dart';
 import '../model/push_request.dart';
 import '../model/states/push_request_state.dart';
 import '../utils/custom_int_buffer.dart';
+import 'secure_storage_mutexed.dart';
 
 class SecurePushRequestRepository implements PushRequestRepository {
   const SecurePushRequestRepository();
 
   // Use this to lock critical sections of code.
-  static final Mutex _m = Mutex();
-
-  /// Function [f] is executed, protected by Mutex [_m].
-  /// That means, that calls of this method will always be executed serial.
-  static Future<T> protect<T>(Future<T> Function() f) => _m.protect<T>(f);
+  final SecureStorageMutexed _storage = const SecureStorageMutexed();
 
-  static const FlutterSecureStorage _storage = FlutterSecureStorage();
   static const String _securePushRequestKey = 'app_v3_pr_state';
 
-  @override
-
   /// Save the state to the secure storage.
   /// This is a critical section, so it is protected by Mutex.
-  Future<void> saveState(PushRequestState pushRequestState) => protect(() => _saveState(pushRequestState));
-  Future<void> _saveState(PushRequestState pushRequestState) async {
+  @override
+  Future<void> saveState(PushRequestState pushRequestState) async {
     final stateJson = jsonEncode(pushRequestState.toJson());
     await _storage.write(key: _securePushRequestKey, value: stateJson);
   }
 
-  @override
-
   /// Load the state from the secure storage.
   /// If no state is found, an empty state is returned.
   /// This is a critical section, so it is protected by Mutex.
-  Future<PushRequestState> loadState() => protect<PushRequestState>(_loadState);
-  Future<PushRequestState> _loadState() async {
+  @override
+  Future<PushRequestState> loadState() async {
     final String? stateJson = await _storage.read(key: _securePushRequestKey);
     if (stateJson == null) {
       return PushRequestState(pushRequests: [], knownPushRequests: CustomIntBuffer(list: []));
     }
     return PushRequestState.fromJson(jsonDecode(stateJson));
   }
 
-  @override
-
   /// Adds a push request in the given state if it is not already known.
   /// If no state is given, the current state is loaded from the secure storage.
   /// This is a critical section, so it is protected by Mutex.
-  Future<PushRequestState> add(PushRequest pushRequest, {PushRequestState? state}) => protect<PushRequestState>(() async {
-        state ??= await _loadState();
-        if (state!.knowsRequest(pushRequest)) {
-          return state!;
-        }
-        final newState = state!.withRequest(pushRequest);
-        await _saveState(newState);
-        return newState;
-      });
-
   @override
+  Future<PushRequestState> add(PushRequest pushRequest, {PushRequestState? state}) async {
+    state ??= await loadState();
+    if (state.knowsRequest(pushRequest)) {
+      return state;
+    }
+    final newState = state.withRequest(pushRequest);
+    await saveState(newState);
+    return newState;
+  }
 
   /// Remove a push request from the state.
   /// If no state is given, the current state is loaded from the secure storage.
   /// This is a critical section, so it is protected by Mutex.
-  Future<PushRequestState> remove(PushRequest pushRequest, {PushRequestState? state}) => protect<PushRequestState>(() async {
-        state ??= await _loadState();
-        final newState = state!.withoutRequest(pushRequest);
-        await _saveState(newState);
-        return newState;
-      });
-
   @override
+  Future<PushRequestState> remove(PushRequest pushRequest, {PushRequestState? state}) async {
+    state ??= await loadState();
+    final newState = state.withoutRequest(pushRequest);
+    await saveState(newState);
+    return newState;
+  }
 
   /// Removes all push requests from the repository.
   /// If no state is saved, nothing will happen.
   /// This is a critical section, so it is protected by Mutex.
-  Future<void> clearState() => protect<void>(_clearState);
-  Future<void> _clearState() => _storage.delete(key: _securePushRequestKey);
+  @override
+  Future<void> clearState() => _storage.delete(key: _securePushRequestKey);
 }

lib/repo/secure_storage_mutexed.dart

@@ -0,0 +1,38 @@
+/*
+ * privacyIDEA Authenticator
+ *
+ * Author: Frank Merkel <[email protected]>
+ *
+ * Copyright (c) 2024 NetKnights GmbH
+ *
+ * Licensed under the Apache License, Version 2.0 (the 'License');
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an 'AS IS' BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+import 'package:flutter_secure_storage/flutter_secure_storage.dart';
+import 'package:mutex/mutex.dart';
+
+class SecureStorageMutexed {
+  const SecureStorageMutexed();
+
+  static final Mutex _m = Mutex();
+  static final FlutterSecureStorage storage = const FlutterSecureStorage();
+
+  /// Function [f] is executed, protected by Mutex [_m].
+  /// That means, that calls of this method will always be executed serial.
+  Future<T> _protect<T>(Future<T> Function() f) => _m.protect<T>(f);
+
+  Future<void> write({required String key, required String value}) => _protect(() => storage.write(key: key, value: value));
+  Future<String?> read({required String key}) => _protect(() => storage.read(key: key));
+  Future<Map<String, String>> readAll() => _protect(() => storage.readAll());
+  Future<void> delete({required String key}) => _protect(() => storage.delete(key: key));
+  Future<void> deleteAll() => _protect(() => storage.deleteAll());
+}

lib/repo/secure_token_repository.dart

@@ -5,7 +5,7 @@
 
   Authors: Timo Sturm <[email protected]>
            Frank Merkel <[email protected]>
-  Copyright (c) 2017-2023 NetKnights GmbH
+  Copyright (c) 2017-2024 NetKnights GmbH
 
   Licensed under the Apache License, Version 2.0 (the 'License');
   you may not use this file except in compliance with the License.
@@ -25,8 +25,6 @@ import 'dart:convert';
 
 import 'package:flutter/material.dart';
 import 'package:flutter/services.dart';
-import 'package:flutter_secure_storage/flutter_secure_storage.dart';
-import 'package:mutex/mutex.dart';
 
 import '../interfaces/repo/token_repository.dart';
 import '../l10n/app_localizations.dart';
@@ -38,30 +36,25 @@ import '../utils/view_utils.dart';
 import '../views/settings_view/settings_view_widgets/send_error_dialog.dart';
 import '../widgets/dialog_widgets/default_dialog.dart';
 import '../widgets/dialog_widgets/default_dialog_button.dart';
+import 'secure_storage_mutexed.dart';
 
 // TODO How to test the behavior of this class?
 class SecureTokenRepository implements TokenRepository {
   const SecureTokenRepository();
 
-  // Use this to lock critical sections of code.
-  static final Mutex _m = Mutex();
-
-  /// Function [f] is executed, protected by Mutex [_m].
-  /// That means, that calls of this method will always be executed serial.
-  static Future<T> _protect<T>(Future<T> Function() f) => _m.protect<T>(f);
-  static const FlutterSecureStorage _storage = FlutterSecureStorage();
+  static const SecureStorageMutexed _storage = SecureStorageMutexed();
   static const String _TOKEN_PREFIX = GLOBAL_SECURE_REPO_PREFIX;
 
   // ###########################################################################
   // TOKENS
   // ###########################################################################
 
   @override
-  Future<Token?> loadToken(String id) => _protect(() => _loadToken(id));
-  Future<Token?> _loadToken(String id) async {
+  Future<Token?> loadToken(String id) async {
     final token = await _storage.read(key: _TOKEN_PREFIX + id);
+    Logger.info('Loading token from secure storage: $id');
     if (token == null) {
-      Logger.warning('Token not found in secure storage', name: 'secure_token_repository.dart#loadToken');
+      Logger.warning('Token not found in secure storage');
       return null;
     }
     return Token.fromJson(jsonDecode(token));
@@ -71,13 +64,12 @@ class SecureTokenRepository implements TokenRepository {
   /// this device.
   /// If [loadLegacy] is set to true, will attempt to load old android and ios tokens.
   @override
-  Future<List<Token>> loadTokens() => _protect(() => _loadTokens());
-  Future<List<Token>> _loadTokens() async {
+  Future<List<Token>> loadTokens() async {
     late Map<String, String> keyValueMap;
     try {
       keyValueMap = await _storage.readAll();
     } on PlatformException catch (e, s) {
-      Logger.warning("Token found, but could not be decrypted.", name: 'secure_token_repository.dart#loadTokens', error: e, stackTrace: s, verbose: true);
+      Logger.warning("Token found, but could not be decrypted.", error: e, stackTrace: s, verbose: true);
       _decryptErrorDialog();
       return [];
     }
@@ -98,21 +90,28 @@ class SecureTokenRepository implements TokenRepository {
         valueJson = jsonDecode(value);
       } on FormatException catch (_) {
         // Value should be a json. Skip everything that is not a json.
+        Logger.info('Value is not a json.. skipping');
         continue;
       }
 
-      if (valueJson == null || !valueJson.containsKey('type')) {
+      if (valueJson == null) {
+        // If valueJson is null or does not contain a type, it can't be a token. Skip it.
+        Logger.info('Value Json is null.. skipping');
+        continue;
+      }
+      if (!valueJson.containsKey('type')) {
         // If valueJson is null or does not contain a type, it can't be a token. Skip it.
+        Logger.info('Value Json does not contain a type.. skipping');
         continue;
       }
 
       // TODO token.version might be deprecated, is there a reason to use it?
       // TODO when the token version (token.version) changed handle this here.
-      Logger.info('Loading token from secure storage: ${valueJson['id']}', name: 'secure_token_repository.dart#loadTokens');
+      Logger.info('Loading token from secure storage: ${valueJson['id']}');
       try {
         tokenList.add(Token.fromJson(valueJson));
       } catch (e, s) {
-        Logger.error('Could not load token from secure storage', name: 'secure_token_repository.dart#loadTokens', error: e, stackTrace: s);
+        Logger.error('Could not load token from secure storage', error: e, stackTrace: s);
       }
     }
 
@@ -124,62 +123,58 @@ class SecureTokenRepository implements TokenRepository {
   /// in the storage the existing value is overwritten.
   /// Returns all tokens that could not be saved.
   @override
-  Future<List<T>> saveOrReplaceTokens<T extends Token>(List<T> tokens) => _protect<List<T>>(() => _saveOrReplaceTokens(tokens));
-  Future<List<T>> _saveOrReplaceTokens<T extends Token>(List<T> tokens) async {
+  Future<List<T>> saveOrReplaceTokens<T extends Token>(List<T> tokens) async {
+    if (tokens.isEmpty) return [];
     final failedTokens = <T>[];
     for (var element in tokens) {
-      if (!await _saveOrReplaceToken(element)) {
+      if (!await saveOrReplaceToken(element)) {
         failedTokens.add(element);
       }
     }
     if (failedTokens.isNotEmpty) {
       Logger.error(
         'Could not save all tokens (${tokens.length - failedTokens.length}/${tokens.length}) to secure storage',
-        name: 'secure_token_repository.dart#saveOrReplaceTokens',
         stackTrace: StackTrace.current,
       );
     } else {
-      Logger.info('Saved ${tokens.length}/${tokens.length} tokens to secure storage', name: 'secure_token_repository.dart#saveOrReplaceTokens');
+      Logger.info('Saved ${tokens.length}/${tokens.length} tokens to secure storage');
     }
     return failedTokens;
   }
 
   @override
-  Future<bool> saveOrReplaceToken(Token token) => _protect<bool>(() => _saveOrReplaceToken(token));
-  Future<bool> _saveOrReplaceToken(Token token) async {
+  Future<bool> saveOrReplaceToken(Token token) async {
     try {
-      await _storage.write(key: _TOKEN_PREFIX + token.id, value: jsonEncode(token));
-    } catch (_) {
+      await _storage.write(key: _TOKEN_PREFIX + token.id, value: jsonEncode(token.toJson()));
+    } catch (e) {
+      Logger.warning('Could not save token to secure storage', error: e, verbose: true);
       return false;
     }
     return true;
   }
 
   /// Deletes the saved jsons of [tokens] from the secure storage.
   @override
-  Future<List<T>> deleteTokens<T extends Token>(List<T> tokens) => _protect<List<T>>(() => _deleteTokens(tokens));
-  Future<List<T>> _deleteTokens<T extends Token>(List<T> tokens) async {
+  Future<List<T>> deleteTokens<T extends Token>(List<T> tokens) async {
     final failedTokens = <T>[];
     for (var element in tokens) {
-      if (!await _deleteToken(element)) {
+      if (!await deleteToken(element)) {
         failedTokens.add(element);
       }
     }
     if (failedTokens.isNotEmpty) {
-      Logger.warning('Could not delete all tokens from secure storage',
-          name: 'secure_token_repository.dart#deleteTokens', error: 'Failed tokens: $failedTokens', stackTrace: StackTrace.current);
+      Logger.warning('Could not delete all tokens from secure storage', error: 'Failed tokens: $failedTokens', stackTrace: StackTrace.current);
     }
     return failedTokens;
   }
 
   /// Deletes the saved json of [token] from the secure storage.
   @override
-  Future<bool> deleteToken(Token token) => _protect<bool>(() => _deleteToken(token));
-  Future<bool> _deleteToken(Token token) async {
+  Future<bool> deleteToken(Token token) async {
     try {
       _storage.delete(key: _TOKEN_PREFIX + token.id);
     } catch (e, s) {
-      Logger.warning('Could not delete token from secure storage', name: 'secure_token_repository.dart#deleteToken', error: e, stackTrace: s);
+      Logger.warning('Could not delete token from secure storage', error: e, stackTrace: s);
       return false;
     }
     Logger.info('Token deleted from secure storage');
@@ -213,7 +208,7 @@ class SecureTokenRepository implements TokenRepository {
             DefaultDialogButton(
                 child: Text(AppLocalizations.of(context)!.decryptErrorButtonSendError),
                 onPressed: () async {
-                  Logger.info('Sending error report', name: 'secure_token_repository.dart#_decryptErrorDialog');
+                  Logger.info('Sending error report');
                   await showDialog(
                     context: context,
                     builder: (context) => const SendErrorDialog(),
@@ -229,7 +224,7 @@ class SecureTokenRepository implements TokenRepository {
                     child: SizedBox(
                       height: 50,
                       width: 50,
-                      child: CircularProgressIndicator(),
+                      child: CircularProgressIndicator.adaptive(),
                     ),
                   ),
                 );
@@ -261,7 +256,6 @@ class SecureTokenRepository implements TokenRepository {
               onPressed: () async {
                 Logger.info(
                   'Deleting all tokens from secure storage',
-                  name: 'secure_token_repository.dart#_decryptErrorDeleteTokenConfirmationDialog',
                   verbose: true,
                 );
                 Navigator.pop(context, true);