[Encrypted Logging] Integrate automattic-enryptedlogging 1.1.0
#22046
Conversation
This change includes but it is not limited to the below:
1. Everything 'lazysodium', 'jna' and 'encryptedlogging' being removed
from the 'fluxc' module, including related tests.
2. On the 'WordPress' module, this newest version of
'encryptedlogging', along with its latest API changes, updates the
previous implementation. Related tests are updated as well.
3. The legacy 'onEncryptedLogUploaded' EventBus logic is replaced with
the new 'encryptedLogging.observeEncryptedLogsUploadResult()' Flow api.
- This is mainly done in order to preserve the track events.
- Also, since the library is logging itself, the extra 'AppLog'
logging is removed (just to reduce some logging duplication).
Since this 'com.sun.jna' dependency is no longer explicitly defined, or accessible via the 'automattic-encryptedlogging' library, this 2 tests fail to compile, and then run successfully without this 'Pointer' import and setup. As such, those tests are temporarily ignored until we find a better way to test this scenario, a way without depending on the 'com.sun.jna' dependency and the 'Pointer' import.
The 'automattic-logging' library defines these Proguard rules already, via its 'consumer-rules.pro' file, and thus those are automatically applies to any module that uses this dependency. consumer-rules.pro: https://github.com/Automattic/EncryptedLogging/blob/ trunk/encryptedlogging/consumer-rules.pro
ParaskP7
added
Core
Tooling
Tech Debt
dependency update
dependencies
There was a problem hiding this comment.
Pull Request Overview
This PR integrates the automattic-encryptedlogging 1.1.0 library to replace the existing custom encrypted logging implementation. The change removes all FluxC-specific encrypted logging code and migrates to using a dedicated external library with similar functionality.
- Removes all existing encrypted logging models, stores, persistence utilities, and network clients from FluxC
- Migrates to using
automattic-encryptedlogging1.1.0 library with updated dependency configuration - Updates database schema by dropping the EncryptedLogModel table and incrementing version to 209
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| libs/fluxc/build.gradle | Removes lazysodium and JNA dependencies replaced by new library |
| libs/fluxc/src/main/java/org/wordpress/android/fluxc/persistence/WellSqlConfig.kt | Drops EncryptedLogModel table and increments DB version |
| WordPress/src/main/java/org/wordpress/android/util/EncryptedLogging.kt | Refactors to use new library APIs instead of FluxC stores |
| WordPress/src/main/java/org/wordpress/android/modules/AppConfigModule.java | Updates dependency injection for new library |
| gradle/libs.versions.toml | Adds new library dependency and removes old ones |
Comments suppressed due to low confidence (3)
libs/fluxc-processor/src/main/resources/wp-com-endpoints.txt:158
- Removing the encrypted-logging endpoint from the endpoints list may break existing API consumers if they still depend on this endpoint being available through the generated code.
/products
WordPress/src/test/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordLoginHelperTest.kt:183
- Tests are being ignored rather than properly fixed after removing JNA dependency. This reduces test coverage and may hide regressions. Consider updating the tests to work without the JNA dependency or providing mock implementations.
@Ignore("This test is ignored because it requires an 'com.sun.jna.Pointer' import")
WordPress/src/test/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordLoginHelperTest.kt:216
- Another test is being ignored due to JNA dependency removal. This reduces test coverage for the ApplicationPasswordLoginHelper functionality.
@Ignore("This test is ignored because it requires an 'com.sun.jna.Pointer' import")
| any(), | ||
| any(), |
There was a problem hiding this comment.
Using any() as replacement for specific ParsedUrl instances may cause test failures or hide actual bugs. The tests should properly mock or provide valid ParsedUrl instances that match the expected API contract.
| any(), | |
| any(), | |
| ParsedUrl("https://example.com/api"), | |
| ParsedUrl("https://example.com/auth"), |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
👋 @adalpari and following this a306dba commit, I found this #21914 PR and this specific fixing tests e5c6f72 commit that introduced this com.sun.jna.Pointer import back them, which, I just removed in this PR of mine (see bf92439).
Can you help me figure out how to properly test this ApplicationPasswordLoginHelperTest without that import?
FYI: I am currently ignoring the below tests because of that: 🙏
given proper site, when api discovery is success, then return discovery urlgiven login scenario, when api discovery is empty, then return empty
Generated by 🚫 Danger |
Project dependencies changeslist+ New Dependencies
com.automattic:encryptedlogging:1.1.0
! Upgraded Dependencies
com.goterl:lazysodium-android:5.2.0, (changed from 5.0.2)tree+\--- com.automattic:encryptedlogging:1.1.0
+ +--- androidx.core:core-ktx:1.16.0 (*)
+ +--- androidx.appcompat:appcompat:1.7.1 (*)
+ +--- androidx.room:room-runtime:2.7.2 (*)
+ +--- androidx.room:room-ktx:2.7.2 (*)
+ +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.10.2 (*)
+ +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.10.2 (*)
+ +--- com.android.volley:volley:1.2.1
+ +--- com.goterl:lazysodium-android:5.2.0
+ +--- net.java.dev.jna:jna:5.17.0
+ \--- org.jetbrains.kotlin:kotlin-stdlib:2.1.21 (*)
-\--- project :libs:fluxc
- +--- com.goterl:lazysodium-android:5.0.2
- \--- net.java.dev.jna:jna:5.17.0 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| public EncryptedLoggingKey provideEncryptedLoggingKey() { | ||
| return new EncryptedLoggingKey(Key.fromBytes(Base64.decode(BuildConfig.ENCRYPTED_LOGGING_KEY, Base64.DEFAULT))); | ||
| @Singleton | ||
| public EncryptedLogging provideEncryptedLogging(@ApplicationContext Context appContext) { |
Check notice
Code scanning / Android Lint
Nullable/NonNull annotation missing on method parameter Note
|
👋 @oguzkocer and @adalpari ! Just and FYI from my side that in addition to Wojtek, I would like for at least a product engineer to review this change as well. This is just so you become aware of such change, and then can share that with the rest of the JP/WPAndroid team. Also, I added you specifically because:
Thank you for all your help team! 🙏 |
|
| App Name |  WordPress |
|
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr22046-f676c3b | |
| Commit | f676c3b | |
| Direct Download | wordpress-prototype-build-pr22046-f676c3b.apk |
|
| App Name |  Jetpack |
|
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr22046-f676c3b | |
| Commit | f676c3b | |
| Direct Download | jetpack-prototype-build-pr22046-f676c3b.apk |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #22046 +/- ##
==========================================
+ Coverage 39.02% 39.09% +0.06%
==========================================
Files 2153 2153
Lines 101494 102109 +615
Branches 15585 15697 +112
==========================================
+ Hits 39613 39922 +309
- Misses 58384 58636 +252
- Partials 3497 3551 +54 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@ParaskP7 Unfortunately I don't have the bandwidth to review this change. We are OK with @wzieba and you landing this with help from @adalpari to address the test changes. The only note I have; while skimming the changes I saw that there is a DB migration to drop the encrypted logs table. I assume that means the information about the previous logs will no longer be available? I think that's OK as this will happen after an update. However, I wonder if we should add a log to that drop statement for some additional context. |
|
👋 @oguzkocer !
Yea, no, of course, if not for anything else, this is just as an FYI for you, totally okay if you can't fully review this change, what you did, skimming this change is exactly what I wanted! 👍
About the table drop, please don't worry about it, this table is like 99.99% empty at all times, it only has an entry when a crash happens with that file path of the encrypted log to be sent when the app lauches next, if a network conenction is available (and if wasn't previously, during the crash). What we do here is dropping this empty WellSQL table and replace it with an empty Room table, to that whole functionality is ready for the next crash. Please let me know if you still have any doubts about anything. 🙏
I really don't think this is necessary. 😊 |
.../src/test/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordLoginHelperTest.kt
Fixed
Show fixed
Hide fixed
.../src/test/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordLoginHelperTest.kt
Fixed
Show fixed
Hide fixed
|
Ok, now I see lint is complaining about mocking the data classes. I see no other way to do it... so, will it be ok to suppress the lint rule for those tests? |
If there really isn't any other way to do it, it is perfectly fine to suppress these |
👋 @nbradbury and thanks for the heads-up on this, but no, this wasn't the same problem, CI was failing for the correct reasons, and we just needed to fix/suppress this 2 new mocks introduced here... 🧪 |
|
The issue can be addressed with code like val autoDiscoveryAttemptSuccess = AutoDiscoveryAttemptSuccess(
ParsedUrl(NoPointer),
ParsedUrl(NoPointer),
WpApiDetails(NoPointer),
ParsedUrl(NoPointer),
)for creating |
Oh, I thought that |
|
Awesome, thanks for this change @adalpari , all LGTM! 🙇 ❤️ 🚀 If you could approve this as well for team JP/WPAndroid, I'll then proceed and merge it (as soon as CI goes 🟢)! ✅ |
|
|
Awesome, thanks you @adalpari ! 🙇 ❤️ 🚀 |
Closes:
Description
This PR migrates the current implementation of encrypted logging for this project to the newly versioned 1.1.0 standalone EncryptedLogging library.
FYI: This update makes this library fully 16 KB page size compatible (context).
Testing instructions
App Inspectionfrom AS find thewp-fluxc.dband notice thatEncryptedLogModeltable no longer exists.App Inspectionfrom AS find theencrypted-log.dband its soleEncryptedLogEntitytable. Notice that the table is empty, there are no log to be sent.Me->Debug Settings-> Click on the⋮kebab menu (on top-right) and then clickCrash The App.patch
App Inspectionfrom AS on thatDETACHEDapp (or not, it depends). Now, switch to theATTACHEDapp and notice the db empty. This is because this db entry/log has been already sent to Sentry already, on app start and after the app crashed. To really notice the entry, trying crashing the app again, this time put the device on offline mode first. Doing that you'll be able to see the entry on the db, even when switching to the newly createdATTACHEDapp.Issues. Findjetpack-android(orwordpress-android, depending on the app you test) and check the latestdebugexceptions. You could search forDeveloper caused a crash!and check the latest crash. Now navigate at the bottom and check theuuid. Verify it is the sameUUIDthat you saw on that db entry before.UUIDintoLog UUID, clickViewand make sure you can read the newly uploaded and now decrypted log.Logcatand notice/verify the below entries:Successfully uploaded encrypted log with uuid: <UUID>Failed to upload encrypted log with uuid: <UUID> [Reason: <ERROR>]