feat: Implementing encrypted local storage for user sessions with tests #1211
Conversation
…f the same ParseFile from multiple threads
This reverts commit 8d8a00d
Thanks for opening this pull request!
|
| return store.getAsync() | ||
| .continueWithTask( | ||
| new Continuation<T, Task<T>>() { | ||
| @Override | ||
| public Task<T> then(Task<T> task) throws Exception { | ||
| if (task.getResult() != null) return task; | ||
| return legacy.getAsync() | ||
| .continueWithTask( | ||
| new Continuation<T, Task<T>>() { | ||
| @Override | ||
| public Task<T> then(Task<T> task) | ||
| throws Exception { | ||
| T object = task.getResult(); | ||
| if (object == null) return task; | ||
| return legacy.deleteAsync() | ||
| .continueWith( | ||
| task1 -> | ||
| ParseTaskUtils.wait( | ||
| store | ||
| .setAsync( | ||
| object))) | ||
| .onSuccess(task1 -> object); | ||
| } | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Is there a better way to structure this code?
| @@ -46,7 +46,9 @@ public boolean isCancellationRequested() { | |||
| } | |||
| } | |||
|
|
|||
| /** @return the token that can be passed to asynchronous method to control cancellation. */ | |||
There was a problem hiding this comment.
Please revert these unrelated changes in bolts, twitter and wherever you find these comment-only refactors.
|
As this PR is a continuation of #1191, did you consider the PR review comments and discussion feedback there when creating this PR? |
Yes, I did. Did i missed something during the discussion feedback? I will again look into the discussion. Thanks for the attention. |
Noting specific, I didn't check the discussion there, only noticed that there has been extensive discussion. |
📝 WalkthroughWalkthroughThis change introduces encrypted storage for cached user session files using Android Jetpack Security, preventing session data from being easily copied on rooted devices. It adds an Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant ParseCorePlugins
participant EncryptedFileObjectStore
participant LegacyStore
participant ParseObjectStoreMigrator
App->>ParseCorePlugins: getCurrentUserController()
ParseCorePlugins->>ParseObjectStoreMigrator: getAsync()
ParseObjectStoreMigrator->>EncryptedFileObjectStore: getAsync()
alt Encrypted store has data
EncryptedFileObjectStore-->>ParseObjectStoreMigrator: Return user/session
else Encrypted store empty
ParseObjectStoreMigrator->>LegacyStore: getAsync()
alt Legacy has data
LegacyStore-->>ParseObjectStoreMigrator: Return user/session
ParseObjectStoreMigrator->>EncryptedFileObjectStore: setAsync(user)
ParseObjectStoreMigrator->>LegacyStore: deleteAsync()
else Legacy empty
LegacyStore-->>ParseObjectStoreMigrator: Return null
end
end
ParseObjectStoreMigrator-->>ParseCorePlugins: Return user/session
ParseCorePlugins-->>App: Return user/session
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. All functional code changes are directly related to encrypted session storage, migration, supporting utilities, and test infrastructure as described in the linked issue. Javadoc reformatting and build file updates are trivial and do not affect functionality. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🔭 Outside diff range comments (1)
parse/src/main/java/com/parse/ParseCorePlugins.java (1)
131-143:⚠️ Potential issueCritical: File path conflict between plain and encrypted stores
Both
FileObjectStoreandEncryptedFileObjectStoreare using the same file path, which will cause data corruption. The plain store expects unencrypted data while the encrypted store expects encrypted data in a different format.Consider using a different file path for the encrypted store:
File file = new File(Parse.getParseFilesDir(), FILENAME_CURRENT_USER); FileObjectStore<ParseUser> fileStore = new FileObjectStore<>(ParseUser.class, file, ParseUserCurrentCoder.get()); ParseObjectStore<ParseUser> store = Parse.isLocalDatastoreEnabled() ? new OfflineObjectStore<>(ParseUser.class, PIN_CURRENT_USER, fileStore) : fileStore; +File encryptedFile = new File(Parse.getParseFilesDir(), FILENAME_CURRENT_USER + ".encrypted"); EncryptedFileObjectStore<ParseUser> encryptedFileObjectStore = new EncryptedFileObjectStore<>( - ParseUser.class, file, ParseUserCurrentCoder.get()); + ParseUser.class, encryptedFile, ParseUserCurrentCoder.get()); ParseObjectStoreMigrator<ParseUser> storeMigrator = new ParseObjectStoreMigrator<>(encryptedFileObjectStore, store); ParseCurrentUserController controller = new CachedCurrentUserController(storeMigrator);
♻️ Duplicate comments (8)
bolts-tasks/src/main/java/com/parse/boltsinternal/Task.java (4)
544-551: Revert these unrelated Javadoc formatting changes.As mentioned in the previous review feedback, these comment-only refactors in bolts files should be reverted since they are unrelated to the PR's main objective of implementing encrypted storage.
553-560: Revert these unrelated Javadoc formatting changes.As mentioned in the previous review feedback, these comment-only refactors in bolts files should be reverted since they are unrelated to the PR's main objective of implementing encrypted storage.
562-569: Revert these unrelated Javadoc formatting changes.As mentioned in the previous review feedback, these comment-only refactors in bolts files should be reverted since they are unrelated to the PR's main objective of implementing encrypted storage.
571-585: Revert these unrelated Javadoc formatting changes.As mentioned in the previous review feedback, these comment-only refactors in bolts files should be reverted since they are unrelated to the PR's main objective of implementing encrypted storage.
bolts-tasks/src/main/java/com/parse/boltsinternal/CancellationTokenSource.java (1)
49-51: Revert these unrelated Javadoc formatting changes.As you acknowledged in your previous response, these comment-only refactors in bolts files should be reverted since they are unrelated to the PR's main objective.
parse/src/main/java/com/parse/ManifestInfo.java (1)
153-155: Revert these unrelated Javadoc formatting changes.Per the previous review feedback requesting to revert comment-only refactors "wherever you find these", this Javadoc formatting change should also be reverted as it's unrelated to the encrypted storage implementation.
parse/src/main/java/com/parse/ParseUser.java (1)
720-722: Revert these unrelated Javadoc formatting changes.Per the previous review feedback requesting to revert comment-only refactors "wherever you find these", this Javadoc formatting change should also be reverted as it's unrelated to the encrypted storage implementation.
parse/src/main/java/com/parse/ParseObjectStoreMigrator.java (1)
24-49: 🛠️ Refactor suggestionSimplify deeply nested anonymous classes
The current implementation uses deeply nested anonymous classes that make the code difficult to read and maintain.
Consider using Java 8+ lambdas or extracting methods to improve readability:
@Override public Task<T> getAsync() { return store.getAsync() - .continueWithTask( - new Continuation<T, Task<T>>() { - @Override - public Task<T> then(Task<T> task) throws Exception { - if (task.getResult() != null) return task; - return legacy.getAsync() - .continueWithTask( - new Continuation<T, Task<T>>() { - @Override - public Task<T> then(Task<T> task) - throws Exception { - T object = task.getResult(); - if (object == null) return task; - return legacy.deleteAsync() - .continueWith( - task1 -> - ParseTaskUtils.wait( - store - .setAsync( - object))) - .onSuccess(task1 -> object); - } - }); - } - }); + .continueWithTask(task -> { + if (task.getResult() != null) return task; + return migrateFromLegacy(); + }); +} + +private Task<T> migrateFromLegacy() { + return legacy.getAsync() + .continueWithTask(task -> { + T object = task.getResult(); + if (object == null) return task; + return legacy.deleteAsync() + .continueWithTask(ignored -> store.setAsync(object)) + .onSuccess(ignored -> object); + }); }
🧹 Nitpick comments (2)
parse/src/test/java/com/parse/EncryptedFileObjectStoreTest.java (1)
43-43: Consider renaminggetSetup()to follow Java naming conventions.The method name
getSetup()is misleading as it suggests a getter that returns a value, but it appears to perform initialization. Consider renaming it tosetup()orinitialize().parse/src/test/java/com/parse/AndroidKeyStoreProvider.kt (1)
165-169: Incomplete RSA key pair storage implementation.The RSA key pair is generated but not stored in the AndroidKeyStore. The comment suggests certificate generation might be needed.
Would you like me to help implement the complete RSA key pair storage with certificate generation, or create an issue to track this TODO?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
bolts-tasks/src/main/java/com/parse/boltsinternal/CancellationTokenSource.java(1 hunks)bolts-tasks/src/main/java/com/parse/boltsinternal/Task.java(1 hunks)bolts-tasks/src/main/java/com/parse/boltsinternal/TaskCompletionSource.java(1 hunks)build.gradle(1 hunks)parse/build.gradle(2 hunks)parse/src/main/java/com/parse/EncryptedFileObjectStore.java(1 hunks)parse/src/main/java/com/parse/ManifestInfo.java(1 hunks)parse/src/main/java/com/parse/Parse.java(1 hunks)parse/src/main/java/com/parse/ParseClassName.java(1 hunks)parse/src/main/java/com/parse/ParseCorePlugins.java(1 hunks)parse/src/main/java/com/parse/ParseFileUtils.java(7 hunks)parse/src/main/java/com/parse/ParseObjectStoreMigrator.java(1 hunks)parse/src/main/java/com/parse/ParseQuery.java(1 hunks)parse/src/main/java/com/parse/ParseSession.java(1 hunks)parse/src/main/java/com/parse/ParseUser.java(1 hunks)parse/src/test/java/com/parse/AlgorithmParameterSpec.kt(1 hunks)parse/src/test/java/com/parse/AndroidKeyStoreProvider.kt(1 hunks)parse/src/test/java/com/parse/AndroidOpenSSLProvider.kt(1 hunks)parse/src/test/java/com/parse/EncryptedFileObjectStoreTest.java(1 hunks)parse/src/test/java/com/parse/ParseObjectStoreMigratorTest.kt(1 hunks)parse/src/test/java/com/parse/RobolectricKeyStore.kt(1 hunks)twitter/src/main/java/com/parse/twitter/ParseTwitterUtils.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
parse/src/test/java/com/parse/AndroidOpenSSLProvider.kt (1)
parse/src/test/java/com/parse/AndroidKeyStoreProvider.kt (1)
put(38-177)
parse/src/test/java/com/parse/ParseObjectStoreMigratorTest.kt (1)
parse/src/main/java/com/parse/ParseObjectStoreMigrator.java (1)
ParseObjectStoreMigrator(8-82)
parse/src/main/java/com/parse/ParseObjectStoreMigrator.java (3)
bolts-tasks/src/main/java/com/parse/boltsinternal/Task.java (1)
Task(28-993)parse/src/test/java/com/parse/ParseObjectStoreMigratorTest.kt (1)
store(15-101)parse/src/main/java/com/parse/ParseTaskUtils.java (1)
ParseTaskUtils(17-137)
parse/src/main/java/com/parse/ParseFileUtils.java (1)
parse/src/main/java/com/parse/ParseIOUtils.java (1)
ParseIOUtils(27-347)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Gradle Build
🔇 Additional comments (11)
bolts-tasks/src/main/java/com/parse/boltsinternal/TaskCompletionSource.java (1)
27-29: Approve JavaDoc formatting update
The new multi-line Javadoc forgetTask()is well-formed and consistent with the project’s comment style.parse/src/main/java/com/parse/Parse.java (1)
291-293: Approve JavaDoc formatting update
The expanded Javadoc forisInitialized()aligns with the repository’s documentation conventions without altering functionality.parse/src/main/java/com/parse/ParseSession.java (1)
125-127: Approve JavaDoc formatting update
The new multi-line Javadoc forgetSessionToken()accurately describes its return behavior and matches the style used across the codebase.twitter/src/main/java/com/parse/twitter/ParseTwitterUtils.java (1)
111-113: Approve JavaDoc formatting update
The adjusted Javadoc forisLinked(ParseUser)is correctly formatted and consistent with the other API docs.parse/src/main/java/com/parse/ParseClassName.java (1)
24-26: Approve JavaDoc formatting update
The multi-line Javadoc for thevalue()method is properly structured and aligns with the established annotation documentation style.parse/src/main/java/com/parse/ParseQuery.java (1)
285-287: JavaDoc formatting standardized
The expanded Javadoc block enhances readability and aligns with the project's JavaDoc conventions. No functional or behavioral changes introduced.parse/build.gradle (1)
2-2: LGTM: Kotlin plugin added correctly.The addition of the
kotlin-androidplugin enables Kotlin support as needed for the new Kotlin test files.parse/src/test/java/com/parse/AndroidOpenSSLProvider.kt (1)
3-17: Verify license compatibility for Appmattus codeThis file contains Appmattus Limited copyright notice. Please ensure that the Apache 2.0 license is compatible with the Parse SDK license and that proper attribution is maintained.
Are you reusing code from an Appmattus project? If so, please ensure you're complying with their license requirements.
parse/src/test/java/com/parse/EncryptedFileObjectStoreTest.java (1)
57-85: Well-structured test for encrypted file writing.The test properly verifies that the ParseUser data is correctly encrypted and saved to disk, with appropriate use of mocks and direct file reading for validation.
parse/src/main/java/com/parse/ParseFileUtils.java (1)
68-85: Well-implemented encrypted file reading with proper resource management.The method correctly handles the EncryptedFile API and follows the established pattern of the existing
readFileToByteArraymethod with proper stream cleanup.parse/src/test/java/com/parse/AndroidKeyStoreProvider.kt (1)
1-17: Verify copyright and licensing compatibility.This file has a different copyright holder (Appmattus Limited) than the other Parse SDK files. Please ensure:
- Proper attribution is maintained
- The Apache 2.0 license is compatible with the Parse SDK license
- Any modifications are properly documented
| @@ -50,6 +51,7 @@ dependencies { | |||
| api "androidx.core:core:1.8.0" | |||
| api "com.squareup.okhttp3:okhttp:$okhttpVersion" | |||
| api project(':bolts-tasks') | |||
| implementation "androidx.security:security-crypto:1.1.0-alpha03" | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the alpha version dependency for production readiness.
Using an alpha version (1.1.0-alpha03) of androidx.security:security-crypto in production raises concerns about stability and security. Alpha versions are typically not recommended for production use.
🌐 Web query:
What is the latest stable version of androidx.security:security-crypto and are there any known security vulnerabilities in version 1.1.0-alpha03?
💡 Result:
The latest stable version of androidx.security:security-crypto is 1.0.0, which was released on April 21, 2021[2][4]. Since then, the library has continued with alpha releases in the 1.1.0 series, with the most recent being version 1.1.0-alpha07, released on April 9, 2025[1][2].
Notably, in version 1.1.0-alpha07, all APIs in the library were deprecated in favor of existing platform APIs and direct use of Android Keystore[1]. This deprecation was officially implemented in April 2025, as mentioned in multiple sources[1][7][9].
Regarding version 1.1.0-alpha03 specifically, it does have a known security vulnerability. According to Maven Repository information, this version has a vulnerability inherited from its dependencies, specifically CVE-2022-24329[6]. This vulnerability is related to the kotlin-stdlib dependency that version 1.1.0-alpha03 uses[6].
Additionally, there have been reports of crashes in certain versions of the library. Some users reported that version 1.1.0-alpha01 was the last properly working version before encountering issues[8]. These crashes were particularly problematic in applications that had allowBackUp enabled[8].
If you're currently using this library, you should consider migrating to platform APIs and Android Keystore directly, as recommended by the deprecation notice in the latest version[1]. For those who still need the functionality, some developers have created forks of the library to provide ongoing support post-deprecation[7].
Citations:
- 1: https://developer.android.com/jetpack/androidx/releases/security
- 2: https://mvnrepository.com/artifact/androidx.security/security-crypto
- 3: https://developer.android.com/jetpack/androidx/versions/all-channel
- 4: https://mvnrepository.com/artifact/androidx.security
- 5: https://www.netguru.com/blog/androidx-security
- 6: https://mvnrepository.com/artifact/androidx.security/security-crypto-ktx/1.1.0-alpha03
- 7: https://github.com/ed-george/encrypted-shared-preferences
- 8: Bug: using EncryptedSharedPreferences, it can cause crashes to users right when initializing it tink-crypto/tink#535
- 9: https://ed-george.github.io/articles/28-05-2024/jetsec-deprecation
- 10: https://www.droidcon.com/2022/06/15/unpacking-android-security-part-2-insecure-data-storage/
Review production readiness of androidx.security:security-crypto:1.1.0-alpha03
Using an alpha release in production introduces stability and security risks. Version 1.1.0-alpha03 is known to inherit CVE-2022-24329 from its Kotlin StdLib dependency and the entire 1.1.0 series has since been deprecated in favor of platform APIs (Android Keystore). The latest stable release is 1.0.0 (Apr 2021).
• File: parse/build.gradle
Line: 54
Suggested changes:
- implementation "androidx.security:security-crypto:1.1.0-alpha03"
+ // Consider migrating to Android Keystore or use the latest stable library
+ implementation "androidx.security:security-crypto:1.0.0"If you require features beyond 1.0.0, either fork a maintained version or switch to direct platform APIs as recommended in the April 2025 deprecation notice.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| implementation "androidx.security:security-crypto:1.1.0-alpha03" | |
| // Consider migrating to Android Keystore or use the latest stable library | |
| implementation "androidx.security:security-crypto:1.0.0" |
🤖 Prompt for AI Agents
In parse/build.gradle at line 54, the dependency on
androidx.security:security-crypto uses the alpha version 1.1.0-alpha03, which
has known security vulnerabilities and is deprecated. Replace this dependency
with the latest stable version 1.0.0 for production use, or alternatively,
remove this dependency and migrate to using direct platform APIs and Android
Keystore as recommended in the April 2025 deprecation notice. If features beyond
1.0.0 are needed, consider using a maintained fork instead of the alpha release.
| @@ -11,7 +11,7 @@ buildscript { | |||
| classpath "org.jacoco:org.jacoco.core:$jacocoVersion" | |||
| classpath "com.dicedmelon.gradle:jacoco-android:0.1.5" | |||
| classpath "io.freefair.gradle:android-gradle-plugins:4.2.0-m1" | |||
| classpath "com.diffplug.spotless:spotless-plugin-gradle:5.17.1" | |||
| classpath "com.diffplug.spotless:spotless-plugin-gradle:6.7.1" | |||
There was a problem hiding this comment.
Fix version inconsistency with Spotless plugin.
The classpath version is updated to 6.7.1, but the plugin declaration on line 20 still uses version 5.17.1. This inconsistency could cause build issues.
Apply this fix to align the plugin version:
plugins {
id "com.github.ben-manes.versions" version "0.28.0"
- id "com.diffplug.spotless" version "5.17.1"
+ id "com.diffplug.spotless" version "6.7.1"
}🤖 Prompt for AI Agents
In build.gradle at line 14 and around line 20, the Spotless plugin version is
inconsistent, with the classpath using 6.7.1 and the plugin declaration using
5.17.1. Update the plugin declaration on line 20 to also use version 6.7.1 to
ensure both references match and prevent build issues.
| internal val AlgorithmParameterSpec.keystoreAlias: String | ||
| get() = this::class.java.getDeclaredMethod("getKeystoreAlias").invoke(this) as String |
There was a problem hiding this comment.
Add error handling for reflection-based property access.
The reflection call lacks error handling and could throw NoSuchMethodException, IllegalAccessException, or InvocationTargetException at runtime.
Apply this fix to add proper error handling:
internal val AlgorithmParameterSpec.keystoreAlias: String
- get() = this::class.java.getDeclaredMethod("getKeystoreAlias").invoke(this) as String
+ get() = try {
+ val method = this::class.java.getDeclaredMethod("getKeystoreAlias")
+ method.isAccessible = true
+ method.invoke(this) as String
+ } catch (e: Exception) {
+ throw IllegalStateException("Failed to access keystoreAlias property", e)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| internal val AlgorithmParameterSpec.keystoreAlias: String | |
| get() = this::class.java.getDeclaredMethod("getKeystoreAlias").invoke(this) as String | |
| internal val AlgorithmParameterSpec.keystoreAlias: String | |
| get() = try { | |
| val method = this::class.java.getDeclaredMethod("getKeystoreAlias") | |
| method.isAccessible = true | |
| method.invoke(this) as String | |
| } catch (e: Exception) { | |
| throw IllegalStateException("Failed to access keystoreAlias property", e) | |
| } |
🤖 Prompt for AI Agents
In parse/src/test/java/com/parse/AlgorithmParameterSpec.kt at lines 5 to 6, the
reflection call to get the "getKeystoreAlias" method lacks error handling and
may throw exceptions like NoSuchMethodException, IllegalAccessException, or
InvocationTargetException. Wrap the reflection invocation in a try-catch block
to catch these exceptions and handle them appropriately, such as by logging the
error or rethrowing a runtime exception with a clear message.
| val setup by lazy { | ||
| Security.removeProvider("AndroidKeyStore") | ||
| Security.removeProvider(BouncyCastleProvider.PROVIDER_NAME) | ||
| Security.removeProvider("AndroidOpenSSL") | ||
|
|
||
| Security.addProvider(AndroidKeyStoreProvider()) | ||
| Security.addProvider(BouncyCastleProvider()) | ||
| Security.addProvider(AndroidOpenSSLProvider()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider thread safety and usage documentation for global security provider manipulation.
The lazy initialization manipulates global JVM security providers, which could cause issues in concurrent test execution environments. Consider adding:
- Thread safety: Use
@Synchronizedor other synchronization mechanisms - Usage documentation: Document when and how this setup should be invoked
- Test isolation: Ensure this doesn't interfere with other tests
object RobolectricKeyStore {
- val setup by lazy {
+ @Synchronized
+ fun setupProviders() {
+ if (isSetup) return
+
Security.removeProvider("AndroidKeyStore")
Security.removeProvider(BouncyCastleProvider.PROVIDER_NAME)
Security.removeProvider("AndroidOpenSSL")
Security.addProvider(AndroidKeyStoreProvider())
Security.addProvider(BouncyCastleProvider())
Security.addProvider(AndroidOpenSSLProvider())
+ isSetup = true
}
+
+ private var isSetup = false
+
+ /**
+ * Call this in test setup to configure security providers for Robolectric tests
+ * that require AndroidKeyStore simulation.
+ */
+ val setup by lazy { setupProviders() }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val setup by lazy { | |
| Security.removeProvider("AndroidKeyStore") | |
| Security.removeProvider(BouncyCastleProvider.PROVIDER_NAME) | |
| Security.removeProvider("AndroidOpenSSL") | |
| Security.addProvider(AndroidKeyStoreProvider()) | |
| Security.addProvider(BouncyCastleProvider()) | |
| Security.addProvider(AndroidOpenSSLProvider()) | |
| } | |
| @Synchronized | |
| fun setupProviders() { | |
| if (isSetup) return | |
| Security.removeProvider("AndroidKeyStore") | |
| Security.removeProvider(BouncyCastleProvider.PROVIDER_NAME) | |
| Security.removeProvider("AndroidOpenSSL") | |
| Security.addProvider(AndroidKeyStoreProvider()) | |
| Security.addProvider(BouncyCastleProvider()) | |
| Security.addProvider(AndroidOpenSSLProvider()) | |
| isSetup = true | |
| } | |
| private var isSetup = false | |
| /** | |
| * Call this in test setup to configure security providers for Robolectric tests | |
| * that require AndroidKeyStore simulation. | |
| */ | |
| val setup by lazy { setupProviders() } |
🤖 Prompt for AI Agents
In parse/src/test/java/com/parse/RobolectricKeyStore.kt around lines 8 to 16,
the lazy initialization block modifies global JVM security providers without
thread safety, risking concurrency issues in tests. To fix this, synchronize the
setup method using @Synchronized or another locking mechanism to ensure only one
thread can modify providers at a time. Additionally, add clear documentation on
when and how to invoke this setup to prevent misuse and interference with other
tests, and consider isolating this setup to avoid impacting concurrent test
executions.
| import org.junit.jupiter.api.Test | ||
| import org.junit.runner.RunWith | ||
| import org.mockito.Mockito.mock | ||
| import org.mockito.Mockito.never | ||
| import org.mockito.Mockito.times | ||
| import org.mockito.Mockito.verify | ||
| import org.mockito.Mockito.`when` | ||
| import org.robolectric.RobolectricTestRunner | ||
| @RunWith(RobolectricTestRunner::class) |
There was a problem hiding this comment.
Fix test framework mismatch between JUnit 4 and JUnit 5
The test uses @RunWith(RobolectricTestRunner::class) which is for JUnit 4, but imports JUnit 5 annotations (org.junit.jupiter.api.Test).
Replace JUnit 5 imports with JUnit 4:
-import org.junit.jupiter.api.Assertions.assertEquals
-import org.junit.jupiter.api.Assertions.assertTrue
-import org.junit.jupiter.api.BeforeEach
-import org.junit.jupiter.api.Test
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertTrue
+import org.junit.Before
+import org.junit.Test
import org.junit.runner.RunWithAlso update the setup method annotation:
- @BeforeEach
+ @Before
fun setUp() {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In parse/src/test/java/com/parse/ParseObjectStoreMigratorTest.kt around lines 7
to 15, there is a mismatch between JUnit 4 and JUnit 5 usage: the test uses
@RunWith(RobolectricTestRunner::class) from JUnit 4 but imports JUnit 5
annotations like org.junit.jupiter.api.Test. To fix this, replace the JUnit 5
imports with their JUnit 4 equivalents, such as importing org.junit.Test instead
of org.junit.jupiter.api.Test, and update any setup method annotations to use
JUnit 4 annotations like @Before instead of JUnit 5's @BeforeEach.
| if (file.exists() && !ParseFileUtils.deleteQuietly(file)) | ||
| throw new RuntimeException("Unable to delete"); | ||
| try { | ||
| saveToDisk(object); | ||
| } catch (GeneralSecurityException e) { | ||
| throw new RuntimeException(e.getMessage()); | ||
| } |
There was a problem hiding this comment.
Improve error messages and exception handling.
The error messages lack context, and exceptions are wrapped without the original cause.
Apply these improvements:
- if (file.exists() && !ParseFileUtils.deleteQuietly(file))
- throw new RuntimeException("Unable to delete");
+ if (file.exists() && !ParseFileUtils.deleteQuietly(file))
+ throw new RuntimeException("Unable to delete file: " + file.getAbsolutePath());
try {
saveToDisk(object);
} catch (GeneralSecurityException e) {
- throw new RuntimeException(e.getMessage());
+ throw new RuntimeException("Failed to encrypt and save object", e);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (file.exists() && !ParseFileUtils.deleteQuietly(file)) | |
| throw new RuntimeException("Unable to delete"); | |
| try { | |
| saveToDisk(object); | |
| } catch (GeneralSecurityException e) { | |
| throw new RuntimeException(e.getMessage()); | |
| } | |
| if (file.exists() && !ParseFileUtils.deleteQuietly(file)) | |
| throw new RuntimeException("Unable to delete file: " + file.getAbsolutePath()); | |
| try { | |
| saveToDisk(object); | |
| } catch (GeneralSecurityException e) { | |
| throw new RuntimeException("Failed to encrypt and save object", e); | |
| } |
🤖 Prompt for AI Agents
In parse/src/main/java/com/parse/EncryptedFileObjectStore.java around lines 108
to 114, improve error handling by enhancing the exception messages with more
context and by wrapping exceptions with their original cause instead of just the
message. Modify the RuntimeException thrown when file deletion fails to include
the file path or name for clarity. When catching GeneralSecurityException, wrap
it in a RuntimeException passing the original exception as the cause to preserve
the stack trace and debugging information.
| if (file.exists() && !ParseFileUtils.deleteQuietly(file)) | ||
| throw new RuntimeException("Unable to delete"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error message with file context.
Apply this improvement:
- if (file.exists() && !ParseFileUtils.deleteQuietly(file))
- throw new RuntimeException("Unable to delete");
+ if (file.exists() && !ParseFileUtils.deleteQuietly(file))
+ throw new RuntimeException("Unable to delete file: " + file.getAbsolutePath());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (file.exists() && !ParseFileUtils.deleteQuietly(file)) | |
| throw new RuntimeException("Unable to delete"); | |
| if (file.exists() && !ParseFileUtils.deleteQuietly(file)) | |
| throw new RuntimeException("Unable to delete file: " + file.getAbsolutePath()); |
🤖 Prompt for AI Agents
In parse/src/main/java/com/parse/EncryptedFileObjectStore.java around lines 129
to 130, the RuntimeException thrown when file deletion fails has a generic
message. Update the exception message to include the file path or name to
provide context about which file could not be deleted, improving error
traceability.
| * Writes a {@link JSONObject} to an encrypted file, will throw an error if the file already | ||
| * exists. |
There was a problem hiding this comment.
Fix incorrect JavaDoc - methods write strings, not JSONObjects.
The JavaDoc mentions writing a JSONObject but these methods write strings.
Fix the documentation:
- * Writes a {@link JSONObject} to an encrypted file, will throw an error if the file already
+ * Writes a string to an encrypted file, will throw an error if the file already
* exists.Also applies to: 657-658
🤖 Prompt for AI Agents
In parse/src/main/java/com/parse/ParseFileUtils.java at lines 641-642 and
657-658, the JavaDoc incorrectly states that the methods write a JSONObject, but
they actually write strings. Update the JavaDoc comments to reflect that these
methods write strings to the encrypted file instead of JSONObject instances.
| override fun engineGenerateKey(): SecretKey = wrapped.generateKey().also { | ||
| AndroidKeyStore.storedKeys[lastSpec!!.keystoreAlias] = KeyStore.SecretKeyEntry(it) | ||
| } |
There was a problem hiding this comment.
Potential NullPointerException with force unwrapping.
The lastSpec!! force unwrapping can cause a crash if engineGenerateKey() is called without prior initialization via engineInit(AlgorithmParameterSpec, SecureRandom).
Add a null check:
override fun engineGenerateKey(): SecretKey = wrapped.generateKey().also {
- AndroidKeyStore.storedKeys[lastSpec!!.keystoreAlias] = KeyStore.SecretKeyEntry(it)
+ lastSpec?.let { spec ->
+ AndroidKeyStore.storedKeys[spec.keystoreAlias] = KeyStore.SecretKeyEntry(it)
+ } ?: throw IllegalStateException("Key generator not properly initialized")
}🤖 Prompt for AI Agents
In parse/src/test/java/com/parse/AndroidKeyStoreProvider.kt around lines 139 to
141, the code force unwraps lastSpec with !! which can cause a
NullPointerException if engineGenerateKey() is called before lastSpec is
initialized. To fix this, add a null check for lastSpec before accessing its
keystoreAlias, and handle the null case appropriately, such as throwing a clear
exception or returning early to avoid the crash.
| override fun engineInit(keysize: Int, random: SecureRandom?) = Unit | ||
| override fun engineGenerateKey(): SecretKey = wrapped.generateKey().also { | ||
| AndroidKeyStore.storedKeys[lastSpec!!.keystoreAlias] = KeyStore.SecretKeyEntry(it) | ||
| } |
There was a problem hiding this comment.
Fix empty implementation and potential NPE.
The engineInit(keysize, random) has an empty implementation, and there's the same NPE risk with lastSpec!!.
- override fun engineInit(keysize: Int, random: SecureRandom?) = Unit
+ override fun engineInit(keysize: Int, random: SecureRandom?) = wrapped.init(keysize, random)
override fun engineGenerateKey(): SecretKey = wrapped.generateKey().also {
- AndroidKeyStore.storedKeys[lastSpec!!.keystoreAlias] = KeyStore.SecretKeyEntry(it)
+ lastSpec?.let { spec ->
+ AndroidKeyStore.storedKeys[spec.keystoreAlias] = KeyStore.SecretKeyEntry(it)
+ } ?: throw IllegalStateException("Key generator not properly initialized")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override fun engineInit(keysize: Int, random: SecureRandom?) = Unit | |
| override fun engineGenerateKey(): SecretKey = wrapped.generateKey().also { | |
| AndroidKeyStore.storedKeys[lastSpec!!.keystoreAlias] = KeyStore.SecretKeyEntry(it) | |
| } | |
| override fun engineInit(keysize: Int, random: SecureRandom?) = | |
| wrapped.init(keysize, random) | |
| override fun engineGenerateKey(): SecretKey = | |
| wrapped.generateKey().also { | |
| lastSpec?.let { spec -> | |
| AndroidKeyStore.storedKeys[spec.keystoreAlias] = | |
| KeyStore.SecretKeyEntry(it) | |
| } ?: throw IllegalStateException("Key generator not properly initialized") | |
| } |
🤖 Prompt for AI Agents
In parse/src/test/java/com/parse/AndroidKeyStoreProvider.kt around lines 154 to
157, the engineInit(keysize, random) method has an empty implementation which
should be properly implemented or safely handled, and the usage of lastSpec!! in
engineGenerateKey() risks a NullPointerException. Fix this by providing a
meaningful implementation for engineInit or documenting why it can be empty, and
replace lastSpec!! with a safe null check or throw a clear exception if lastSpec
is null before accessing its keystoreAlias.
|
@rommansabbir could you look through the feedback comments? |
|
Just a note, reading through this PR again, I wonder what the purpose of this PR is; isn't a rooted device inherently unsafe? If someone roots a device, then they take all the risks, which go far beyond the Parse Session token. |
|
@mtrezza As far as I remember, I was trying to push an existing PR that was originally created by another developer who may have deleted their account. I took over their changes and created a new PR. |
|
Should we proceed with merging this PR, or would it be better to close it? |
|
Do you see any value in it? I assume there was a reason why you picked it up. Just want to confirm it makes sense to merge this. |
New Pull Request Checklist
Issue Description
From an existing PR:
Closes: #1192
Approach
EncryptedFileObjectStoreto enable encryption and support CRUD operations.ParseCorePluginswhengetCurrentUserController()is invoked byParseObjectStoreMigrator.ParseFileUtilsto support CRUD operations for encrypted objects.Testing:
TODOs before merging
gradle clean testSummary by CodeRabbit
New Features
Build System
Bug Fixes
Documentation
Tests