@W-21933885: [MSDK Android] App Attestation Implementation#2868
Conversation
Generated by 🚫 Danger |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (5.88%) 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.87% 40.45% -24.43%
+ Complexity 2979 858 -2121
=============================================
Files 223 126 -97
Lines 17507 9542 -7965
Branches 2497 1438 -1059
=============================================
- Hits 11358 3860 -7498
- Misses 4937 5080 +143
+ Partials 1212 602 -610
🚀 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 |
JohnsonEricAtSalesforce
force-pushed
the
feature/w-21933885_msdk-android-app-attestation-implementation
branch
9 times, most recently
from
fa97a67 to
4076ab2
Compare
|
The last mile of code coverage is taking some time to wrap up today. While I'm waiting on the last couple of CodeCov reports, I'll add some walk-through commentary even though I'd still recommend holding reviews until I mark the pull request Ready For Review. |
JohnsonEricAtSalesforce
force-pushed
the
feature/w-21933885_msdk-android-app-attestation-implementation
branch
3 times, most recently
from
973e1dd to
e374d58
Compare
|
|
||
| if $IS_PR ; then | ||
| LEVELS_TO_TEST=$PR_API_VERSION | ||
| RETRIES=1 |
There was a problem hiding this comment.
@brandonpage, a little later you'll see a fix in the RestClientTest.java when I had generated by our tools to resolve unreliable behavior I saw in that test. I've been marking it @Ignore a lot since it's so unpredictable. The fix looks solid - It's so solid that our tools believe we don't need retry anymore. I'd love to see a "faster fail" on pull request runs since they can take a very long time. Thoughts?
| * TODO: Make this Kotlin-internal once it is no longer referenced by Java. ECJ20260420 | ||
| */ | ||
| @Volatile | ||
| var appAttestationClient: AppAttestationClient? = null |
There was a problem hiding this comment.
A crux change is creating a new object to encapsulate all the things for the new Salesforce "Challenge" API, the Integrity Token Provider, the Token and providing that in the "Attestation" format the auth and token refresh endpoints now expect. That's here.
Our tools had some great suggestion around making this property thread safe, so I added @volatile, the private setter and a dedicated lock object based on tool feedback.
| * @param googleCloudProjectId The Google Cloud Project ID or null to | ||
| * disable Salesforce App Attestation | ||
| */ | ||
| fun updateAppAttestationClient( |
There was a problem hiding this comment.
If one reads the description of this pull request, this is the entry point for an app to actually enable App Attestation.
| * @param restClient The REST client, usually provided by the Salesforce SDK | ||
| * Manager's unauthenticated REST client | ||
| */ | ||
| class AppAttestationClient( |
There was a problem hiding this comment.
This object is the heart 'n soul of App Attestation.
| internal var integrityTokenProvider: StandardIntegrityTokenProvider? = null | ||
|
|
||
| init { | ||
| prepareIntegrityTokenProvider() |
There was a problem hiding this comment.
This is the "warm up" for Google Play Integrity API, as we often call it in internal discussion and docs.
| * @return The "attestation" value usable in Salesforce OAuth authorization | ||
| * and token refresh requests or null if the value cannot be created | ||
| */ | ||
| internal suspend fun createSalesforceOAuthAuthorizationAppAttestation( |
There was a problem hiding this comment.
The doc comment says it all and having detailed comments has really been adding context for our tools. This is where our auth and token refresh flows get the new attestation value.
| integrityTokenProvider: StandardIntegrityTokenProvider? = this.integrityTokenProvider, | ||
| ): String? { | ||
| // Guard to ensure the Google Play Integrity API Integrity Provider was asynchronously resolved or do so synchronously now. | ||
| val integrityTokenProviderResolved = integrityTokenProvider ?: prepareIntegrityTokenProvider().result |
There was a problem hiding this comment.
This guard helps resolve the case where LoginActivity generates the authorization URL faster than the integrity token provider can be warmed up. Our login flow gets to this point fast compared to what I believe Google Integrity API expected according to their documentation.
| }.getOrElse { e -> | ||
| // If the Google Play Integrity API failed due to the Integrity Token Provider being expired, re-prepare it once for an inline retry. | ||
| if ((e as? IntegrityServiceException)?.errorCode == INTEGRITY_TOKEN_PROVIDER_INVALID) { | ||
| createSalesforceOAuthAuthorizationAppAttestation( |
There was a problem hiding this comment.
This is a second way we guard by checking for Google Play Integrity API's documented "expired" code and issuing an inline retry.
| * Client App (ECA) Plug-In. | ||
| * @return The Salesforce App Attestation ECA Plug-In's "Challenge" | ||
| */ | ||
| internal fun fetchSalesforceMobileAppAttestationChallenge(): String { |
There was a problem hiding this comment.
This fetches the new "Challenge" value from the org at the very beginning of the flow in our internal docs. If needed, I can send those internally.
| ) | ||
| val attestationValue = getInstance().appAttestationClient?.createSalesforceOAuthAuthorizationAppAttestation() | ||
| val authRequestBody = createRequestBody( | ||
| ATTESTATION to attestationValue, |
There was a problem hiding this comment.
Here's the attestation parameter in use in Native Login.
…ests In ScreenLockActivityScenarioTest.kt)
JohnsonEricAtSalesforce
force-pushed
the
feature/w-21933885_msdk-android-app-attestation-implementation
branch
from
f86e876 to
a8352aa
Compare
|
The is rebased on current |
Job Summary for GradlePull Request :: test-android
|
Job Summary for GradlePull Request :: test-android
|
…ginViewModel Tests)
…read-Unsafe Local Member References And Force-Unwrap From LoginViewModel.generateAuthorizationUrl)
JohnsonEricAtSalesforce
force-pushed
the
feature/w-21933885_msdk-android-app-attestation-implementation
branch
from
a6a2c03 to
b610e69
Compare
…de Coverage For IDPAuthCodeHelper.kt)
…de Coverage For LoginViewModel.kt)
…NativeLoginManager.kt For Full Code Coverage)
…LoginViewModel.kt For Full Code Coverage And Add Comprehensive Tests)
…: Updated Ignored Test Inventory)
| val useHybridAuthentication = SalesforceSDKManager.getInstance().useHybridAuthentication | ||
|
|
||
| // Add Salesforce Mobile App Attestation parameter to authorization URL if applicable. | ||
| val additionalParams = appAttestationClient?.run { |
There was a problem hiding this comment.
For client side IDP-SP flow, how should app attestation work? Right now it looks like an attested IDP app could get a refresh token for a non-attested SP app. That could be problematic.
There was a problem hiding this comment.
The part of the "attestation" that is specific to an org is the "challenge" that's generated before requesting the "token" from Google Play Integrity API. That said, the "attestation" parameter is ignored if it is not needed by the org. Would that make this a non-issue? What analysis do we have of the cross-matrix between IDP and App Attestation?
There was a problem hiding this comment.
I believe during a IDP / SP flow, the IDP gets the code and then the SP does the code to token exchange. As long as both use their own attestation values, we should be okay (both app would be attested).
There was a problem hiding this comment.
Any call to /authorize or /token will know include attestation so long as the host app has configured the Google Cloud Project Id and Challenge API Host. With that in mind, this should be solid. Do we have a test environment ready to try this out? If so, @wmathurin, send that to my direct message.
There was a problem hiding this comment.
I'll also update the internal test plan documentation with a task to cover this flow.
| REDIRECT_URI to redirectUri, | ||
| CODE_CHALLENGE to codeChallenge, | ||
| ) | ||
| val queryString = if (attestationValue != null) "?$ATTESTATION=$attestationValue" else "" |
There was a problem hiding this comment.
Why not have it in the request body with the other params?
There was a problem hiding this comment.
The server didn't see the parameter in the request body when I tested a while back. It seems to be specifically coded to receive the parameter in the query parameters. Should we request a change to look in both locations?
There was a problem hiding this comment.
It feels weird to have it in the query string. IMHO we should request a change to look for it in the body.
| * @param googleCloudProjectId The Google Cloud Project ID or null to | ||
| * disable Salesforce App Attestation | ||
| */ | ||
| fun updateAppAttestationClient( |
There was a problem hiding this comment.
How is it supposed to work? Is the app supposed to register the googleCloudProjectId before first login? Why is it apiHostName specific?
If it needs to be dynamic and decided at runtime, should we use the approach we did for dynamic consumer key where the app can register a block/lambda to provide a googleCloudProjectId during a login flow once the login server is known?
There was a problem hiding this comment.
Based on our conversation last Friday, I created follow-up W-22355537 to cover this. That will separate the Google Cloud Project Id into a new and optional Salesforce SDK Manager initialization parameter. When set, that will allow the manager to immediately begin preparing the Google Play Integrity API Token Provider warm up. The Salesforce Challenge API Hostname will be resolved by a new callback function which the app registers as a separate Salesforce SDK Manager property. That callback will be called with the resolved My Domain host after the well-known authentication configuration is fetched. Only when both are present will the attestation parameter be generated.
| val jwtFlow = !jwt.isNullOrBlank() && !authCodeForJwtFlow.isNullOrBlank() | ||
| val currentJwt = jwt | ||
| val currentAuthCode = authCodeForJwtFlow | ||
| val jwtFlow = if (currentJwt != null && currentAuthCode != null) { | ||
| currentJwt.isNotBlank() && currentAuthCode.isNotBlank() | ||
| } else { | ||
| false | ||
| } |
There was a problem hiding this comment.
@JohnsonEricAtSalesforce I don't understand. Does this not do the exact same thing in 7 lines of code that was originally 1 line?
There was a problem hiding this comment.
You're exactly correct - with the exception that this generates to simpler byte code with fewer logic branches. This came as a suggestion from our tools and gives us a higher coverage score, at the least. I'm on the fence much of the time when it comes to cosmetic Kotlin changes that have logic/coverage impacts that improve our coverage. For this one, I was game to get the coverage win. If anyone's particularly adverse to it, I'm flexible.
There were two cases like this. The other, however, remained a one-liner.
There was a problem hiding this comment.
@brandonpage, just checking if you do or don't want to roll this back. I thought it over a little more and still like the cosmetic code change in exchange for fewer logic branches in the generated byte code and higher coverage. It's just a preference though, so feel free to make the call.
There was a problem hiding this comment.
@JohnsonEricAtSalesforce I am very much in favor of keeping the code as easy as possible for humans to read and digest. I think the 1 line removed is easier to quickly understand than the 7 that replaced it. And when have we ever cared about byte code?
As for code coverage, I am very happy we are pushing to increase it, but this feels artificial. Adding more code just to have more coverage doesn't accomplish much IMO.
There was a problem hiding this comment.
That works. I'll just roll it back. It's just a neat experiment in balancing our myriad of expectations 😊
There was a problem hiding this comment.
That commit is pushed 👍🏻
|
Before making any updates which might intentionally reduce code coverage, here's a screenshot of the "peak" coverage possible on this pull request, some of which was possible largely due to our new tools. Also, future coverage reports may not generate properly once known unstable tests from upstream are restored to this change set prior to merge. 
|
…ew: Move `attestation` Parameter To Request Body In `NativeLoginManager.kt`)
JohnsonEricAtSalesforce
force-pushed
the
feature/w-21933885_msdk-android-app-attestation-implementation
branch
from
c270bee to
cc5f7db
Compare
…EMPORARY: Updated Ignored Test Inventory)" This reverts commit 8387168.
| jwtFlow -> null | ||
| jwtFlow -> mutableMapOf() |
There was a problem hiding this comment.
This probably doesn't matter, but I think we need to remove jwtFlow in 14.0 since getFrontdoorUrl is being removed. I believe it is the (very) old "Magic Links" flow that was simply carried over when we re-wrote the login flows in 13.
| import java.net.URI | ||
|
|
||
| @RunWith(AndroidJUnit4::class) | ||
| class OAuth2MockTests { |
…de Coverage Updates For `jwtFlow` Logic In `LoginViewModel.kt`)
JohnsonEricAtSalesforce
merged commit 3b19ac6
into
forcedotcom:dev
JohnsonEricAtSalesforce
deleted the
feature/w-21933885_msdk-android-app-attestation-implementation
branch
3 participants
🎸 Ready For Final Review - Test Failures Appear Unrelated 🥁
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.