@W-22355537: [MSDK Android] App Attestation Public API#2880
Conversation
| mainActivity: Class<out Activity>, | ||
| private val loginActivity: Class<out Activity>? = null, | ||
| internal val nativeLoginActivity: Class<out Activity>? = null, | ||
| googleCloudProjectId: Long? = null, |
There was a problem hiding this comment.
@wmathurin, here's the relocated googleCloudProjectId in the SalesforceSDKManager constructor that subclasses use. I noticed that our RestExplorerApp sample subclasses, so this gives that app a convenient way to pass this value.
Are there other constructor use cases we'd need to expose? I've yet to do a complete inventory of all our samples and templates to see what they are up to and wanted to get your feedback early.
|
|
||
| /** Lock object for synchronized access to the app Attestation Client */ | ||
| private val appAttestationClientLock = Any() | ||
| val appAttestationClient: AppAttestationClient? by lazy { |
There was a problem hiding this comment.
@wmathurin - The AppAttestationClient initialization is once at construction now.
Generated by 🚫 Danger |
| ) | ||
|
|
||
| // Disable Salesforce App Attestation for login servers that are not My Domain servers. | ||
| appAttestationClient?.apiHostName = null |
There was a problem hiding this comment.
@wmathurin - I went without the app-provided callback to determine the Challenge API Host. Here, we seem to know that we're not using a My Domain and that means App Attestation will be disabled. Is that correct?
There was a problem hiding this comment.
(I'm still hoping we can move to attesting on code exchange only - but that will only fly if user agent flow can be blocked for a given ECA which is not possible today --- stay tuned).
| ) | ||
|
|
||
| // Consider enabling Salesforce App Attestation for login servers that are My Domain servers. | ||
| appAttestationClient?.apiHostName = loginServer.toUri().host |
There was a problem hiding this comment.
Similar to the comment a few lines before, here we seem to know we are using a My Domain host and enable App Attestation. How's that look?
| // Add Salesforce Mobile App Attestation parameter to authorization URL if applicable. | ||
| val additionalParams = appAttestationClient?.run { | ||
| val challenge = fetchMobileAppAttestationChallenge() | ||
| val challenge = fetchMobileAppAttestationChallenge() ?: return@run null |
There was a problem hiding this comment.
It's convenient I'd previously separated the challenge and attestation fetch at the call site, so here it's just a second guard when the Challenge Host is not set.
Job Summary for GradlePull Request :: test-android
|
| context = appContext, | ||
| deviceId = deviceId, | ||
| googleCloudProjectId = appAttestationGoogleCloudProjectId, | ||
| remoteAccessConsumerKey = getBootConfig(appContext).remoteAccessConsumerKey, |
There was a problem hiding this comment.
Since 13.2, we allow apps to dynamically change the consumer key at runtime. So having a AppAttestationClient only works with the bootconfig's consumer key will not work in all cases. Why does the consumer key need to be a property of the app attestation client?
There was a problem hiding this comment.
That's a good point. It was a property for dependency injection, which aids in testability. I'll find a way to accommodate the dynamic value 👍🏻
There was a problem hiding this comment.
@wmathurin - Take a look at 0cd9064 where I gave our tools a chance to recommend a way to keep App Attestation Client updated with the latest remote consumer key while not coupling it with a singleton boot config. The new RemoteAccessConsumerKeyProvider looks like a win for that.
I have been judiciously requesting our tools to avoid coupling new code with fixed, untestable singletons.
| ) | ||
|
|
||
| // Disable Salesforce App Attestation for login servers that are not My Domain servers. | ||
| appAttestationClient?.apiHostName = null |
There was a problem hiding this comment.
(I'm still hoping we can move to attesting on code exchange only - but that will only fly if user agent flow can be blocked for a given ECA which is not possible today --- stay tuned).
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (17.64%) 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 #2880 +/- ##
============================================
- Coverage 55.05% 55.03% -0.03%
- Complexity 2495 2500 +5
============================================
Files 226 226
Lines 17752 17759 +7
Branches 2316 2322 +6
============================================
Hits 9774 9774
- Misses 6978 6984 +6
- Partials 1000 1001 +1
🚀 New features to boost your workflow:
|
JohnsonEricAtSalesforce
force-pushed
the
feature/w-22355537_msdk-android-app-attestation-public-api
branch
from
bf6dc67 to
98fccc8
Compare
JohnsonEricAtSalesforce
force-pushed
the
feature/w-22355537_msdk-android-app-attestation-public-api
branch
from
98fccc8 to
35651b4
Compare
I'm attempting to stabilize some of the unreliable tests inherited from upstream dev. However, that might not be practical without a dedicated follow-up work effort. For record-keeping when the checks can complete here's a snapshot illustrated the code coverage for the patch. |
Generated by 🚫 Danger |
…Unit Test Updates)
…testationClient remoteAccessConsumerKey To A Computed String)
…estationClient To Get Current Remote Access Consumer Key)
…dated Ignored Test Inventory)
…SyncSDKManager And SmartStoreSDKManager Constructors)
…fety And Documentation Improvements)
…ent Test Failure In SalesforceSDKManagerTests)
…Quality: Fix Exception Handling And Implement Strict Mocking) This commit resolves 3 of 5 code review issues in SalesforceSDKManagerTests: 1. Improved exception handling in @before setup: - Changed from catching all Exception to specific RuntimeException - Added message validation to only catch expected initialization errors - Re-throws unexpected exceptions to prevent masking real problems 2. Implemented strict mocking for better regression detection: - Removed 'relaxed = true' from all HTTP mocks - Tests will now fail if unexpected methods are called - All 16 tests pass with strict mocking 3. Added comprehensive code review documentation: - Documented all 5 issues identified in code review - Tracked resolution status and implementation details - Identified 2 remaining medium-priority improvements Test Results: All 16/16 tests passing
…ation With LoginServerManager Property Override) - Changed loginServerManager from factory method pattern to direct property override - Simplified dependency injection approach while maintaining test isolation - Removed SharedPreferences race condition in SalesforceSDKManagerTests - Tests now inject LoginServerManager via property override instead of factory method - Cleaner, more direct implementation with identical functionality
…Actions_ReturnsAllActions_ForNonLoginActivity)
JohnsonEricAtSalesforce
force-pushed
the
feature/w-22355537_msdk-android-app-attestation-public-api
branch
from
f294d9e to
febface
Compare
JohnsonEricAtSalesforce
force-pushed
the
feature/w-22355537_msdk-android-app-attestation-public-api
branch
from
8c7f7db to
d7fb7d5
Compare
| context = appContext, | ||
| deviceId = deviceId, | ||
| googleCloudProjectId = appAttestationGoogleCloudProjectId, | ||
| remoteAccessConsumerKeyProvider = RemoteAccessConsumerKeyProvider { |
There was a problem hiding this comment.
Where/how is the remoteAccessConsumerKeyProvider set up when an appConfigForLoginHost was provided ??
There was a problem hiding this comment.
How does this look? 4f9d02c
This models the remote consumer key look up after what's currently in login view model.
If that's a winner, should we create a reusable routine for this? Perhaps on the manager?
Note, the tests will fail on this commit. I figured to give you a chance to review the production code first.
There was a problem hiding this comment.
Agreed on having a reusable routine somewhere
… Consumer Key Look Up In Create App Attestation Client)
Job Summary for GradlePull Request :: test-android
|
…h Config Resolution Into Common Method) Eliminates code duplication by extracting OAuth configuration resolution logic into a reusable internal method on SalesforceSDKManager. This follows DRY principles and provides a single source of truth for the resolution order: debug override → app config → boot config. Changes: - Added internal resolveOAuthConfigForLoginServer() method to SalesforceSDKManager - Updated LoginViewModel.generateAuthorizationUrl to use new common method - Updated AppAttestationClient creation to use new common method via RemoteAccessConsumerKeyProvider - Added comprehensive test coverage in SalesforceSDKManagerOAuthConfigResolverTest (7 tests, 100% code coverage) - Fixed test mocking patterns to properly use coEvery for suspend functions in: - AppAttestationClientTest (5 tests updated) - LoginViewModelTest (mock setup corrected) - NativeLoginManagerTest (2 helper methods updated) - IDPAuthCodeHelperTest (removed 6 unnecessary advanceUntilIdle calls, fixed mock setup) - OAuth2MockTests unchanged (correctly uses blocking version) All tests pass: 71 LoginViewModelTest, 20 NativeLoginManagerTest, 6 OAuth2MockTests, 7 SalesforceSDKManagerOAuthConfigResolverTest.
Job Summary for GradlePull Request :: test-android
|
Job Summary for GradlePull Request :: test-android
|
Job Summary for GradlePull Request :: test-android
|
… Annotations) Restores @ignore annotations that were removed in commit d7fb7d5. These tests are ignored due to: - Timeouts or long execution times - Dependency on external test fixtures or test orgs - Known flakiness in Firebase Test Lab environment - Missing test assets (bootconfig JSON files) This commit restores test stability by re-ignoring tests that are not yet ready for CI execution. Includes missing `import org.junit.Ignore` statements for all affected test files to ensure compilation. Test files affected: - SalesforceSDK: 15 test files (9 Kotlin, 4 Java, 2 class-level) - SmartStore: 3 test files (all Java) - AuthFlowTester: 4 test files (all Kotlin) Total: 22 test files, 32 @ignore annotations restored No functional changes to production code. Tests compile successfully.
JohnsonEricAtSalesforce
force-pushed
the
feature/w-22355537_msdk-android-app-attestation-public-api
branch
from
f9e49e6 to
1e7fbef
Compare
…Blocking Challenge Fetch Wrapper) Adds minimal test coverage for `fetchMobileAppAttestationChallengeBlocking()` method which previously had zero coverage in CodeCov. The blocking method is a simple `runBlocking` wrapper around the suspend function. Since all functionality is thoroughly tested by the existing suspend function tests, only one test is needed to achieve 100% code coverage of the wrapper itself. New test: - appAttestationClient_fetchMobileAppAttestationChallengeBlocking_DelegatesToSuspendFunction Verifies the blocking wrapper delegates correctly to the suspend function. All edge cases (failures, nulls, exceptions) are already covered by the 5 existing suspend function tests. Coverage: 100% (12 instructions, 2 lines, 1 method) Test results: All 20 tests pass (19 existing + 1 new) Verified with local JaCoCo coverage report.
…ant Null Check From Fetch Mobile App Attestation Challenge)
…rage For Release Build OAuth Config Resolution)
|
Excluding the tests from upstream dev that are failing and blocking the CI run, a successful run shows the new tests on this branch at 100% coverage. 
|
|
Sorry for the delay in merging since yesterday. Google Play was not deploying my updated Integrity Test app to my Internal Track. It just came through and passed my App Attestation regression test. ✅ |
JohnsonEricAtSalesforce
merged commit d526b86
into
forcedotcom:dev
JohnsonEricAtSalesforce
deleted the
feature/w-22355537_msdk-android-app-attestation-public-api
branch
2 participants
🎸 Ready For Review 🥁
This update to App Attestation separates the two key data points required to enable or disable the feature from one combined API to a simpler public API for one method and an internal API for the other.
The first is the Google Cloud Project Id, which is now an optional parameter for the app to set once on initialization.
The second is the Salesforce Challenge API Host, which is managed by the well-known authentication configuration fetch internally by MSDK.
All the tests have been updated to reflect this update and I'll deploy my test app to a device for another round of regression testing.