@W-21933885: [MSDK Android] App Attestation Implementation#2868
Open
JohnsonEricAtSalesforce wants to merge 97 commits intoforcedotcom:devfrom
Conversation
Generated by 🚫 Danger |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (50.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## dev #2868 +/- ##
============================================
- Coverage 64.92% 62.77% -2.16%
+ Complexity 2976 2904 -72
============================================
Files 223 226 +3
Lines 17467 17599 +132
Branches 2490 2508 +18
============================================
- Hits 11341 11048 -293
- Misses 4922 5346 +424
- Partials 1204 1205 +1
🚀 New features to boost your workflow:
|
JohnsonEricAtSalesforce
force-pushed
the
feature/w-21933885_msdk-android-app-attestation-implementation
branch
2 times, most recently
from
145a8a9 to
6990e6f
Compare
Generated by 🚫 Danger |
…Refactored Salesforce Org)
…o New App Attestation Client Classes)
…ndroid Jetpack Data Store Use, Initialize Unit Tests And Code Coverage)
…reate App Attestation Retry)
…ly Ignore Unrelated Test Suites)
…est Naming, Resolve To-Dos)
…hallenge API Hostname)
…aky RestClientTest)
…etry For Expired Integrity Token Provider)
…ll For Unhandled Exceptions Getting Integrity Token)
… Salesforce App Attestation ECA Plugin Challenge API Error Type)
…aky LoginServerManagerTest.kt)
…Test Code Coverage For AppAttestationClient)
…Test Code Coverage For AppAttestationChallengeApiClient)
…Test Code Coverage For NativeLoginManager.createRequestBody)
…empt At Testing App Attestation In NativeLoginManager.login())
…Current Iteration Of Testing App Attestation In NativeLoginManager.login() While Deferring A Comprehensive Test For That Method)
…overage For OAuthAuthorizationAttestation)
…rage For SalesforceSDKManager.updateAppAttestationClient)
JohnsonEricAtSalesforce
force-pushed
the
feature/w-21933885_msdk-android-app-attestation-implementation
branch
from
2be39b2 to
8129ef1
Compare
…sts For Temporary Test Performance)
JohnsonEricAtSalesforce
force-pushed
the
feature/w-21933885_msdk-android-app-attestation-implementation
branch
from
8129ef1 to
a2a451f
Compare
…inManager.login Code Coverage Test)
…dency Injection To OAuth2 And IDPAuthCodeHelper For Testing) - Add dependency injection for SalesforceSDKManager to OAuth2.getAuthorizationUrl() and getBrandedLoginPath() - Create backward-compatible overloads that default to SalesforceSDKManager.getInstance() - Update IDPAuthCodeHelper.getAuthorizationPathForSP() to accept and pass injected SDKManager - Fix IDPAuthCodeHelperTest to properly mock the 11-parameter OAuth2.getAuthorizationUrl() method signature - Add OAuth2.TIMESTAMP_FORMAT access before mockkStatic() to force class initialization - Update OAuth2Test to test new overloaded methods with SalesforceSDKManager parameter All IDPAuthCodeHelperTest tests now pass (10/10, 100% success rate).
…PAuthCodeHelperTest.idpAuthCodeHelper_getAuthorizationPathForSP_whenAttestationClientReturnsAttestation_includesAttestationInQuery)
…thCodeHelperTest Attestation Tests Missing OAuth2 Mock) Fixed two tests that were timing out with UncompletedCoroutinesError: - idpAuthCodeHelper_getAuthorizationPathForSP_whenAttestationClientReturnsAttestation_includesAttestationInQuery - idpAuthCodeHelper_getAuthorizationPathForSP_whenCreateAppAttestationReturnsNull_excludesAttestationFromQuery Root cause: Both tests were calling the real OAuth2.getAuthorizationUrl() static method instead of a mock, causing the test coroutine to hang and timeout after 1 minute. Resolution: Added stubOAuthAuthorizationUrl() calls in both tests to properly mock OAuth2.getAuthorizationUrl() before calling getAuthorizationPathForSP(). The first test now returns a URI with attestation parameter, the second returns a URI without. This follows the same pattern used in other tests in this file that mock the OAuth2 static method before invoking code that depends on it.
…ging Notes For Test Investigation) Added commented debugging notes to track test failures and crashes encountered during attestation test development: - LoginViewModelTest.kt: Added NullPointerException note for jwtFlow_Changes_loginUrl test showing SalesforceKeyGenerator.getSHA256Hash crash with null string - IDPAuthCodeHelperTest.kt: Added ANR crash note and removed unused Ignore import - LoginActivityScenarioTest.kt: Added native crash stack trace from Bluetooth service (system/gd/stack_manager.cc) affecting testWebviewSettings test These notes document issues encountered during test execution and will aid in future debugging and test stabilization efforts.
…DPAuthCodeHelperTest)
JohnsonEricAtSalesforce
force-pushed
the
feature/w-21933885_msdk-android-app-attestation-implementation
branch
3 times, most recently
from
7d09391 to
b9ddfc0
Compare
…DPAuthCodeHelperTest)
JohnsonEricAtSalesforce
force-pushed
the
feature/w-21933885_msdk-android-app-attestation-implementation
branch
from
b9ddfc0 to
5f968fa
Compare
…nt Update To Avoid mockkStatic In IDPAuthCodeHelperTest.kt)
JohnsonEricAtSalesforce
force-pushed
the
feature/w-21933885_msdk-android-app-attestation-implementation
branch
from
9919599 to
d526e06
Compare
…e Unit Test Configuration And Code Coverage Report For IDPAuthCodeHelperTest.kt)
Generated by 🚫 Danger |
…related BootConfigTest From Blocking Unit Test Prototype)
…nit Test Coverage Source Directories)
…stable Test Inventory)
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -23,6 +23,7 @@ dependencies { | |||
| api("androidx.browser:browser:1.8.0") // Update requires API 36 compileSdk | |||
There was a problem hiding this comment.
| A newer version of androidx.browser:browser than 1.8.0 is available: 1.10.0 |
| @@ -23,6 +23,7 @@ dependencies { | |||
| api("androidx.browser:browser:1.8.0") // Update requires API 36 compileSdk | |||
| api("androidx.work:work-runtime-ktx:2.10.3") | |||
There was a problem hiding this comment.
| A newer version of androidx.work:work-runtime-ktx than 2.10.3 is available: 2.11.2 |
| @@ -55,6 +56,14 @@ dependencies { | |||
| androidTestImplementation("androidx.arch.core:core-testing:2.2.0") | |||
| androidTestImplementation("androidx.compose.ui:ui-test-junit4:$composeVersion") | |||
There was a problem hiding this comment.
| A newer version of androidx.compose.ui:ui-test-junit4 than 1.8.2 is available: 1.11.0 |
| @@ -55,6 +56,14 @@ dependencies { | |||
| androidTestImplementation("androidx.arch.core:core-testing:2.2.0") | |||
| androidTestImplementation("androidx.compose.ui:ui-test-junit4:$composeVersion") | |||
| androidTestImplementation("io.mockk:mockk-android:1.14.0") // Update requires Kotlin 2 | |||
There was a problem hiding this comment.
| A newer version of io.mockk:mockk-android than 1.14.0 is available: 1.14.9 |
| androidTestImplementation("io.mockk:mockk-android:1.14.0") // Update requires Kotlin 2 | ||
|
|
||
| testImplementation("junit:junit:4.13.2") | ||
| testImplementation("io.mockk:mockk:1.14.0") // Update requires Kotlin 2 |
There was a problem hiding this comment.
| A newer version of io.mockk:mockk than 1.14.0 is available: 1.14.9 |
|
|
||
| testImplementation("junit:junit:4.13.2") | ||
| testImplementation("io.mockk:mockk:1.14.0") // Update requires Kotlin 2 | ||
| testImplementation("org.jetbrains.kotlinx:kotlinx-coroutines-test:1.8.1") // Update requires Kotlin 2 |
There was a problem hiding this comment.
| A newer version of org.jetbrains.kotlinx:kotlinx-coroutines-test than 1.8.1 is available: 1.10.2 |
| testImplementation("androidx.test:core:1.7.0") | ||
| testImplementation("androidx.test.ext:junit:1.3.0") | ||
| testImplementation("androidx.arch.core:core-testing:2.2.0") | ||
| testImplementation("org.robolectric:robolectric:4.14.1") |
There was a problem hiding this comment.
| A newer version of org.robolectric:robolectric than 4.14.1 is available: 4.16.1 |
Updated sourceDirectories in both convertCodeCoverage and unitTestCoverageReport tasks to use the correct source root path. Previously, JaCoCo was looking for source files at incorrect paths, causing coverage data for files like IDPAuthCodeHelper.kt to be excluded from XML reports sent to CodeCov. Changes: - convertCodeCoverage: Changed from "src/main/java" to "src" and "src/debug" - unitTestCoverageReport: Changed from "src/com" to "src" and "src/debug" This ensures JaCoCo can properly map compiled class files to their source files, allowing CodeCov to receive complete coverage data in CI runs.
| assertTrue(viewModel.loginUrl.value!!.contains(newCodeChallenge)) | ||
| } | ||
|
|
||
| @Ignore |
There was a problem hiding this comment.
| Test is ignored without giving any explanation |
| import org.junit.Test | ||
| import org.junit.runner.RunWith | ||
|
|
||
| @Ignore |
There was a problem hiding this comment.
| Test is ignored without giving any explanation |
…stable Test Inventory)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🧱 Under Construction: Please Hold Reviews Pending Significant Changes And Tests 🚧
This adds Mobile SDK Android support for:
1.1. The "Challenge" API creates challenges for use as the "hash" when creating Google Play App Integrity API "tokens"
2.1. Tokens can be created using the "Challenge" and then passed to authorization and token refresh requests for validation by the App Attestation ECA Plug-In
The app's entry point is calling a new "updateAppAttestationClient" method on the SalesforceSDKManager with the relevant Google Cloud Project ID. Once that's provided, the manager and the new AppAttestationClient manage the interaction with the Challenge API and Google Play Integrity API. The AppAttestationClient provides "Attestation" values to the existing authorization and token refresh logic when applicable.
Most of the new code follows our existing patterns. I'll include a source code walk-though in the differences as well. None of this is set in stone and we can incorporate feedback here or in follow-up work items.
I've tested this by creating a new test app from the REST Explorer sample. That app is configured for deployment to a Google Play Developer account that is configured for App Integrity with a matching Google Cloud project. I've been able to deploy the app to my internal release track. When installed from there, I'm able to successfully authenticate to the internal MSDK App Attestation Test org I've been given access to.
Test coverage is nearing 100% which is great given authentication's critical priority and high availability threshold.
The Google Play App Integrity API has public documentation here https://developer.android.com/google/play/integrity/overview
There's no user interface component for this change in either mobile or web for screenshots or a demo, alas!
Here's an example of how the client app could (re-)initialize app attestation each time the selected login server changes.