Multithreaded Correctness and Callback support #14
Conversation
…n Vault. Possible synchronization issue... special note to look at the clearing of the mCachedSecretKey without a lock
…izes more methods that access the cached secret key.
… adds extra infromation for apply() calls
…Null TypedArray, and Malformed Key
Feature/callback
|
|
There was a problem hiding this comment.
Nice job overall! After addressing the review comments, my only other comment is to break up the multithreaded correctness and callback support into two separate PRs, since they are independent of each other. My apologies for not mentioning that sooner but it should be relatively trivial :)
| editor.putString(TEST_STRING_KEY, TEST_STRING_VALUE).apply(); | ||
| } | ||
|
|
||
| public void testNullTypedBundleFailureCommit() { |
There was a problem hiding this comment.
@lazysp333 This test case should be removed since you can't simulate a null typed bundle
| editor.commit(); | ||
| } | ||
|
|
||
| public void testNullTypedBundleFailureApply() { |
There was a problem hiding this comment.
@lazysp333 This test case should be removed since you can't simulate a null typed bundle
| }) | ||
| ).edit(); | ||
|
|
||
| editor.putString(TEST_STRING_KEY, TEST_STRING_VALUE).commit(); |
There was a problem hiding this comment.
@lazysp333 Should this be editor.putString(null, TEST_STRING_VALUE).commit();
There was a problem hiding this comment.
@dev777 This is the null secret key, AKA the user is trying to encrypt data without having a secret key set. That is why I clear the storage before.
If you would also like, I can add a test case for a attempt to set a null shared pref key as well
There was a problem hiding this comment.
@lazysp333 I think that renaming this test case to testNullSecretKeyFailureCommit and then adding the test case for null shared pref key makes sense. Same recommendation for the testNullKeyFailureApply comment as well
There was a problem hiding this comment.
@dev777 Another thing, putting a null value in this IS acceptable in its current state, the editor.commit() returns true (same with apply). Is this expected behavior?
There was a problem hiding this comment.
These have been explained and fixed
| }) | ||
| ).edit(); | ||
|
|
||
| editor.putString(TEST_STRING_KEY, TEST_STRING_VALUE).apply(); |
There was a problem hiding this comment.
@lazysp333 Should this be editor.putString(null, TEST_STRING_VALUE).apply();
There was a problem hiding this comment.
These have been explained and fixed
README.md
Outdated
| The vault can be rekeyed at any time. This will delete all values in the shared preference file. This is completely irreversible. | ||
|
|
||
| #### API Changes to Commit and Apply | ||
| Typical results of the SharedPreferenc commit and apply are slightly modified in Vault resulting of the extra encryption that is taking place. Because of this, commit and apply can both fail for reasons other than the SharedPreference write failing. The return type for commit is now more important. The recommendation is to **enable exceptions** when creating a SharedPreferenceVault. |
There was a problem hiding this comment.
@lazysp333 Suggested changes:
Typical behavior of the SharedPreference commit and apply methods are slightly modified in Vault as a result of the added encryption. Because of this, commit and apply can both fail for reasons other than the SharedPreference write failing. This means that you should pay attention to the return value for commit and react appropriately in the case where it returns false. It is recommended to enable exceptions when creating a SharedPreferenceVault if you want to see more information on failures (by catching the thrown exception yourself)
README.md
Outdated
| 1. If using `commit()` consider **enabling exceptions** to allow handling of these errors. Also, pay attention to the return type from `commit()` to make sure data was successfully written. | ||
| 2. If using `apply()`, there is a new `SharedPrefVaultWriteListener` interface that can be used to handle callback should any of the errors mentioned above. The `SharedPreferenceVault` interface now supports a new method to add this Listener to the Vault. `setSharedPrefVaultWriteListener(SharedPrefVaultWriteListener listener)` This method can be chained but must be called **before** an editor is created. | ||
| * The actual result of the write is still ignored when using the new `apply()` with a listener. The listener is only to show potential encryption issues that happen *before* the write. | ||
| * Setting no listener will behave like the standard `SharedPreference.apply()`. All error are ignored. |
There was a problem hiding this comment.
@lazysp333 nit: Typo. Change last sentence to All errors are ignored.
README.md
Outdated
| 2. If using `apply()`, there is a new `SharedPrefVaultWriteListener` interface that can be used to handle callback should any of the errors mentioned above. The `SharedPreferenceVault` interface now supports a new method to add this Listener to the Vault. `setSharedPrefVaultWriteListener(SharedPrefVaultWriteListener listener)` This method can be chained but must be called **before** an editor is created. | ||
| * The actual result of the write is still ignored when using the new `apply()` with a listener. The listener is only to show potential encryption issues that happen *before* the write. | ||
| * Setting no listener will behave like the standard `SharedPreference.apply()`. All error are ignored. | ||
| * The listener will only provide basic information on success and fail, it is still recommended that if more information is required, to **enable exceptions**. |
There was a problem hiding this comment.
@lazysp333 nit: Suggested changes:
- The listener will only provide basic information on success and fail. It is recommended to enable exceptions if more information on the failure is required.
| @@ -171,6 +210,6 @@ Automated testing tools are often built to trigger on potential cryptographic mi | |||
| This project must be built with gradle. | |||
|
|
|||
| * Version Numbering - The version name should end with "-SNAPSHOT" for non release builds. This will cause the resulting binary, source, and javadoc files to be uploaded to the snapshot repository in Maven as a snapshot build. Removing snapshot from the version name will publish the build on jcenter. If that version is already published, it will not overwrite it. | |||
| * Execution - To build this libarary, associated tasks are dynamically generated by Android build tools in conjunction with Gradle. Example command for the production flavor of the release build type: | |||
| * Execution - To build this library, associated tasks are dynamically generated by Android build tools in conjunction with Gradle. Example command for the production flavor of the release build type: | |||
…NullSharedPrefKey
Feature/callback
Fixes issues found by Find-Bugs associated with multithreaded correctness relating to SecretKeys.
Also adds support for adding a callback listener to
commit()andapply()inside theSharedPreferenceVaultto improve their functionality.commit()andapply()could fail during encryption and results would become useless. This attempts to fix this issue by giving the user more control over the failure of these two methods.