feat(secure_storage): handle package uninstall/re-install on Linux and Windows #2044

Jordan-Nelson · 2022-08-17T20:10:52Z

Issue #, if available:

Changes:

General / Utils

Add FileKeyValueStore for file simple key/value storage in the file system
Add path_provider in as dependency in amplify_secure_storage to get the application support directory
Tests for App re-install and utils

Linux

Add required gio bindings to get app ID on linux
Add App ID as a namespace for all keys since Linux does not namespace secrets by app (unlike iOS, Android, MacOS)
Add accessGroup to linux config as an optional override to App ID to allow for sharing of data across apps
Add initialization logic in flutter to clear all data for a given scope upon first initialization of that scope, set flags in file storage to keep track of scopes using FileKeyValueStore

Windows

Pass application support dir from flutter package to dart only package
Add config option for customer to optionally override the storage location
Add Win32 Data Protection API bindings
Use the Data Protection APIs to encrypt/decrypt data and FileKeyValueStore to store the data

Motivation:

If a user uninstalls and re-installs an app installed via snap or the Windows app store, the expectation would be that data from their previous install would not persist.

Implementation:

Linux: Data is cleared immediately after the app is launched for the first time (after a re-install). Flags are set in application file storage to track the apps first launch post re-install.
Windows: Encrypted data is now stored in application file storage. This removes the need to clear the data as it is removed automatically on app uninstall.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

packages/secure_storage/amplify_secure_storage_dart/lib/src/ffi/utils/linux_utils.dart

packages/secure_storage/amplify_secure_storage_dart/lib/src/utils/file_key_value_store.dart

dnys1 · 2022-08-17T20:25:15Z

packages/secure_storage/amplify_secure_storage_test/test/linux_test.dart

+        directory: appDirectory,
+        fileName: 'amplify_secure_storage_scopes.json',
+      );
+      fileStore.writeAll({});


Should this use the in-mem fs?

It could be updated to use in-mem. AmplifySecureStorageDart would also have to be updated to use in-mem for these tests in that case.

This is more of an integration tests since it isn't mocking libsecret / secret service, so I think it is fine as is.

packages/secure_storage/amplify_secure_storage_dart/lib/src/utils/file_key_value_store.dart

codecov-commenter · 2022-08-17T20:56:06Z

Codecov Report

Merging #2044 (829e82b) into next (60210e7) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             next    #2044   +/-   ##
=======================================
  Coverage   42.86%   42.86%           
=======================================
  Files         114      114           
  Lines        7502     7502           
=======================================
  Hits         3216     3216           
  Misses       4286     4286

Flag	Coverage Δ
android-unit-tests	`∅ <ø> (∅)`
flutter-unit-tests	`25.08% <ø> (ø)`
ios-unit-tests	`89.06% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

…at/package-uninstall

Jordan-Nelson · 2022-08-26T22:20:49Z

packages/amplify/amplify_flutter/example/macos/Flutter/GeneratedPluginRegistrant.swift

@@ -5,6 +5,8 @@
 import FlutterMacOS
 import Foundation

+import path_provider_macos


We don't need path_provider_macos, but it is brought in as part of path_provider. We could bring in path_provider_linux and path_provider_windows. They are unlisted packages though. Not sure if we want to bring in unlisted packages or not.

Jordan-Nelson · 2022-08-26T22:22:06Z

packages/secure_storage/amplify_secure_storage/example/integration_test/main_test.dart

-    // Android is already run on a background thread via pigeon
-    skip: !zIsWeb && Platform.isAndroid,
-  );
+  group('Secure Storage', () => runTests(storageFactory));


This change is just clean up. It isn't related to this PR. AmplifySecureStorage uses AmplifySecureStorageWorker by default, so these tests were redundant.

Jordan-Nelson · 2022-09-06T22:29:05Z

packages/secure_storage/amplify_secure_storage_test/lib/amplify_secure_storage_test.dart

@@ -148,31 +155,26 @@ void runStandardTests(
    });
  });

-  group('write', () {


Note: These tests were duplicates.

dnys1

Minor comments. Looks good, though. Thanks!

dnys1 · 2022-09-08T21:31:07Z

...re_storage/amplify_secure_storage_dart/lib/src/platforms/amplify_secure_storage_windows.dart

+    if (directory != null) {
+      return FileKeyValueStore(fileName: _fileName, path: directory);
+    }
+    return FileKeyValueStore(


What's the use case for this? Just testing?

Yeah, tests/examples. Could also be a reasonable fallback if/when we support non-flutter use cases for amplify.

dnys1 · 2022-09-08T21:36:14Z

...storage/amplify_secure_storage_dart/lib/src/interfaces/amplify_secure_storage_interface.dart

+  ///
+  /// Used to store encrypted data on Windows. If no value
+  /// is provided, data will be stored in memory on Windows.
+  final String? applicationDirectory;


Should this be part of the config?

Yeah. This was unnecessary. I had added storagePath to WindowsSecureStorageOptions already. I was passing both to the _dart package, with storagePath taking priority over applicationDirectory. This made sense when I was using applicationDirectory in the _dart package on linux as well to read/write scope names, but with that happening on the flutter side, passing two values for the same thing didn't make sense.

Updated 👍

dnys1 · 2022-09-13T18:47:43Z

packages/secure_storage/amplify_secure_storage/lib/src/amplify_secure_storage.vm.dart

+  Future<void> _initializeScope(String? accessGroup) async {
+    // if accessGroup is set, do not clear data on initialization
+    // since the data can be shared across applications.
+    if (accessGroup != null) return;


Can we reformat this to not call _initalizeScope if accessGroup is set since the only reason this method has a parameter is to check if it's not null which is kind of confusing.

dnys1 · 2022-09-13T18:52:39Z

...re_storage/amplify_secure_storage_dart/lib/src/platforms/amplify_secure_storage_windows.dart

+    }
+    final Uint8List encrypted;
+    try {
+      encrypted = Uint8List.fromList(List<int>.from(rawData as List));


I think this makes two copies of the list (List.from, then Uint8List.fromList). I would do Uint8List.fromList((rawData as List).cast()); to avoid that. Is the only failure because of the cast? It's probably more reasonable to do an is check if that's the case.

Updated to use .cast() over List.from.

I kept the try/catch block. rawData could not be a list, or it could be a List that contains data other than integers. Either of those would be problematic. I could do an is check to confirm it is a list, but I would still need to cast to get it to List rather than List.

dnys1 · 2022-09-13T18:54:36Z

packages/secure_storage/amplify_secure_storage_test/lib/amplify_secure_storage_test.dart

-      expect(value2, 'test_update');
-    });
+  group('read/write/delete can handle key value pairs of varying length', () {
+    for (var entry in keyValuePairs.entries) {


Suggested change

for (var entry in keyValuePairs.entries) {

for (final entry in keyValuePairs.entries) {

Don't necessarily need to change since I don't care as much about the style of test code but I thought this was a lint

…at/package-uninstall

Jordan-Nelson added 7 commits August 10, 2022 21:36

feat(secure_storage): clear data on initial of new scope

e8275e3

chore: add empty attributes

b05ab35

chore: update doc comment

c9112b3

chore: fix async await in tests

d784164

chore: clean up unused code in gio

2ba3841

chore: clean up schema and label names

c0a4de2

chore: fix typo in gio config

8e9224b

Jordan-Nelson requested a review from a team as a code owner August 17, 2022 20:10

chore: move @internal to lib

3ed3bef

dnys1 reviewed Aug 17, 2022

View reviewed changes

Jordan-Nelson added 13 commits August 18, 2022 08:53

chore: add tests for FileKeyValueStore

e669d60

chore: use async methods for file and dir operations

dfbccaa

chore: ignore internal member use in test

0ce3cd1

fix: do not clear keys if accessGroup is provided

bfcbb83

chore: update linux tests for appId and accessGroup

79ff0e1

chore: add remove API for testing

9189fd3

chore: refactor to use path_provider

ca524ab

Merge branch 'next' of github.com:aws-amplify/amplify-flutter into fe…

357b1d6

…at/package-uninstall

chore: remove dep on dart:io

fbb6951

chore: rename appSupportPathProvider

652cf2c

chore: add uninstall integ test

25121af

chore: move linux tests to new file

6ff292a

chore: move init to flutter, add worker_bee back

857eae4

Jordan-Nelson force-pushed the feat/package-uninstall branch from d256334 to 857eae4 Compare August 26, 2022 18:47

Jordan-Nelson added 5 commits August 26, 2022 14:51

chore: remove unintended formatting change

10fe18c

fix: add removeAll to mixins, update tests

72f3366

chore: remove test dir from format check

310a3b2

chore: test on vm only

571a83a

chore: fix formatting

1694148

Jordan-Nelson added 7 commits August 26, 2022 18:17

chore: fix linting issues

7c62f9d

chore: add back removed remote tests

0d24566

chore: use package_info_plus to get packageId

7972308

chore: update getters, comments

5748693

chore: pass applicationDir to dart package

6e071ed

chore: use crypt32 fns on windows

cc896d8

chore: use gio to get packageId on linux

907cbf1

Jordan-Nelson changed the title ~~feat(secure_storage): clear keys after reinstall on linux~~ feat(secure_storage): handle package uninstall/re-install on Linux and Windows Sep 1, 2022

Jordan-Nelson added 7 commits September 1, 2022 15:30

chore: fix typos in crypt32 bindings

2409c13

chore: update comments

50189c2

chore: free memory for dp fns

3593bdf

chore: add storage path to windows options

68e635e

chore: update defaultNamespace comment

25d0def

Merge branch 'next' into feat/package-uninstall

d2fd190

chore: fix windows impl, add tests

b19103e

Jordan-Nelson commented Sep 6, 2022

View reviewed changes

Jordan-Nelson added 3 commits September 7, 2022 11:01

chore: update comments in tests

b79bfa3

chore: update comments in tests

3475521

chore: update tests, remove unused code

221bf93

haverchuck self-requested a review September 8, 2022 00:08

haverchuck approved these changes Sep 8, 2022

View reviewed changes

Merge branch 'next' into feat/package-uninstall

4afd081

Jordan-Nelson mentioned this pull request Sep 13, 2022

feat(secure_storage): remove items on re-install for iOS & MacOS #2118

Merged

dnys1 approved these changes Sep 13, 2022

View reviewed changes

Jordan-Nelson added 4 commits September 14, 2022 16:31

chore: cleanup, pr comments

6ff5743

Merge branch 'next' of github.com:aws-amplify/amplify-flutter into fe…

3489d1a

…at/package-uninstall

Merge branch 'next' of github.com:aws-amplify/amplify-flutter into fe…

71ef926

…at/package-uninstall

chore: regenerate mono_repo

829e82b

Jordan-Nelson merged commit 3632e36 into aws-amplify:next Sep 15, 2022

Jordan-Nelson deleted the feat/package-uninstall branch September 15, 2022 13:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(secure_storage): handle package uninstall/re-install on Linux and Windows #2044

feat(secure_storage): handle package uninstall/re-install on Linux and Windows #2044

Jordan-Nelson commented Aug 17, 2022 •

edited

dnys1 Aug 17, 2022

Jordan-Nelson Aug 17, 2022

codecov-commenter commented Aug 17, 2022 •

edited

Jordan-Nelson Aug 26, 2022

Jordan-Nelson Aug 26, 2022

Jordan-Nelson Sep 6, 2022

dnys1 left a comment

dnys1 Sep 8, 2022

Jordan-Nelson Sep 14, 2022

dnys1 Sep 8, 2022

Jordan-Nelson Sep 14, 2022

dnys1 Sep 13, 2022

dnys1 Sep 13, 2022

Jordan-Nelson Sep 14, 2022

dnys1 Sep 13, 2022

	for (var entry in keyValuePairs.entries) {
	for (final entry in keyValuePairs.entries) {

feat(secure_storage): handle package uninstall/re-install on Linux and Windows #2044

feat(secure_storage): handle package uninstall/re-install on Linux and Windows #2044

Conversation

Jordan-Nelson commented Aug 17, 2022 • edited

Changes:

General / Utils

Linux

Windows

Motivation:

Implementation:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 17, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnys1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jordan-Nelson commented Aug 17, 2022 •

edited

codecov-commenter commented Aug 17, 2022 •

edited