@W-21148602: Internal Server List does not respect changes made to Servers.xml file

[JohnsonEricAtSalesforce]

🚧 Draft For Conceptual Review. Please Review Commentary For Questions 🧱

[JohnsonEricAtSalesforce]

@wmathurin, @brandonpage and @bbirman, before I update the tests and move this into ready-for-review I wanted to run the concept of the change past you all.

I chatted briefly with @bbirman to review the iOS behavior since the work item specifically hails that logic as working compared to Android's. Currently, MSDK Android loads the servers.xml only once on install. The work item mentions upgrade, but I didn't see that logic. More, when MDM provides servers they're additive only. When MDM drops a server, the app doesn't reflect that.

What I'm trying here and seems to manually test well is:

1. Drop both sets of non-custom servers on startup, including non-custom servers from servers.xml and MDM.
2. Load the current set of non-custom servers from either servers.xml or MDM according to the existing logic, as much as possible.
3. When MDM is not enabled, preserve the user's custom login servers at the end of the list

I'm doing a little more testing around the behavior of the only show authorized hosts parameter. I want to be sure the MDM flow still respects the user's custom servers.

The question to answer is: Does this seem like a reasonable update to the existing logic to resolve this work item? I can manipulate servers.xml and the MDM data and the app seems to keep the servers list adds, updates or removes non-custom servers just as we'd want. The logic here is older and the methods are not as self-contained as our newer logic, so I wanted to be sure we all got to review any potential side-effects of this before moving further!

[bbirman]

For #3 (and pending your authorized hosts testing), on iOS the custom login servers would be preserved if MDM was enabled as long as "onlyShowAuthorizedHosts" is false

[brandonpage]

This sounds good to me.

One thought though: Should we only remove MDM servers when network is available? Security wise, it would be bad it:

1. Put device in airplane mode
2. Open app. MDM servers are deleted and updated servers cannot be retrieved.
3. Turn airplane mode off.
4. LoginServerManager constructor does not run again. MDM server list is not respected.

Is that scenario possible?

[JohnsonEricAtSalesforce]

Thanks and that thought about network is a great topic. I'll do some research!

[JohnsonEricAtSalesforce]

I took a manual look and also asked our agents. RuntimeConfig does cache the policy locally and will give MSDK those values when no network is present ✅

[codecov]

Codecov Report

❌ Patch coverage is 99.25373% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 64.82%. Comparing base (9e8338d) to head (51d888c).
⚠️ Report is 3 commits behind head on dev.

Files with missing lines	Patch %	Lines
...lesforce/androidsdk/config/LoginServerManager.java	99.25%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##                dev    #2845      +/-   ##
============================================
+ Coverage     64.55%   64.82%   +0.27%     
- Complexity     2925     2961      +36     
============================================
  Files           222      222              
  Lines         17323    17364      +41     
  Branches       2471     2477       +6     
============================================
+ Hits          11182    11257      +75     
+ Misses         4934     4906      -28     
+ Partials       1207     1201       -6     

Components	Coverage Δ
Analytics	47.92% <ø> (ø)
SalesforceSDK	59.57% <97.31%> (+0.54%)	⬆️
Hybrid	59.05% <ø> (ø)
SmartStore	78.20% <ø> (ø)
MobileSync	81.68% <ø> (ø)
React	52.36% <ø> (ø)

Files with missing lines	Coverage Δ
...lesforce/androidsdk/config/LoginServerManager.java	96.94% <99.25%> (+21.83%)	⬆️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

	1 Warning
⚠️	Big PR, try to keep changes smaller if you can.

Generated by 🚫 Danger