@W-20161958: [MSDK 13.1][Android] Cannot login GUS using Welcome endpoint

[JohnsonEricAtSalesforce]

🎸 Ready For Review 20151213 🥁

This resolves an issue where any Welcome Discovery selection that requires browser-based authentication will skip the login server observer that would ordinarily fetch the authentication configuration and start the new intent. Instead, the web view is used which does not have the correct client certificate. One consequence of this is log in will fail as documented in the issue.

The gist of the change is that LoginActivity observes the current value of the selected login server once in onCreate and each distinct value thereafter. One value is consumed in the switch back from WSC Discovery to Default Login which starts a new LoginActivity intent. Since that reuses the activity instance, onCreate is not called and the current value of the selected login server is not consumed. The change moves the observer from onCreate to onNewIntent so that both new and reused instances will consume the current value in the same manner.

The transition to onNewIntent has been useful for a number of recent changes for WSC and QR Code Login. It helps LoginActivity have more multiple stages in its workflow rather than being a one shot activity.

🗞️_20251202 Update_🗞️: I found another issue and corrected it since my initial notes. This one has a subtle logic change I'd like the team to review before we finalize this change. It looks and runs really well, so I hope we'll like it. Currently, LoginViewModel has one observable named selectedServer that is set immediately when the user makes a server selection (by proxy of the LoginServerManager). This asynchronously starts the LoginActivity fetch of the authentication configuration (if needed) and the web view loading the login URL. For Welcome Discovery we need the authentication configuration to drive browser-based authentication before the web view starts loading the login URL (which it doesn't need to do for browser-based authentication). Having the authentication configuration in advance also makes the switch between default and WSC Discovery login smoother.

To accomplish the above without any public API change, I added a new and internal pendingServer observable. This gets set first in the flow while default/WSC Discovery login choices are made and the authentication configuration is fetched. Once the internals are worked out and the authentication configuration is available, the pendingServer is set as the selectedServer just as it is today.

The visible effect of this is that browser-based authentication works as my video shows. A less visible yet positive benefit is that some unneeded web view loading is avoided which I only saw due to logging and several error toasts that would display when testing without the correct client certificates.

This change is very difficult to test since most devices seem to have restrictions preventing debug builds on the needed profiles. I'm working with one of the app teams since they are able to perform this test for us.

I was able to record this demonstration of how the update works in the app: Uploading 20251202 Salesforce App Welcome Discovery Demo.mp4…

🗞️20251213 Update🗞️: Returning to this after travel I've worked hard to incorporate last week's feedback, a very comprehensive set of unit tests and code coverage plus a complete end-to-end manual test in our sample apps. Here's a new video that walks through the tests. Note, the app's boot configuration has to match Salesforce app's for this to work because of backend restrictions.

Functionally, the only change since last review is the restoration of loginUrl to be exclusively focused on the internal web view while a new customTabUrl is only for the external browser custom tabs activity that's used for browser-based authentication. This keeps my goal of separating observers of the two flows while matching the naming we wanted in our team discussion.

I've updated the login activity and login view model tests with lots of great new tests I'm really happy with. They're very comprehensive and will catch issues such we've seen recently with parameter values being switched during code edits for other authentication features. They also achieve 🚀_100% code coverage on this patch_, which will greatly protect WSC Discovery from regression. This did require re-factoring some of the existing login view and activity code to make it functionally testable, which is a great gain in the long term.

It's a bit of reading, so do be patient. I'm here to answer questions and help plus I'm really confident this is a big step forward for WSC and many other parts of the authentication flow. Thanks!

P.S., if you're still reading at this point I really enjoyed reading https://developer.android.com/kotlin/coroutines/test#injecting-test-dispatchers again and applying that to many of the new concurrent tests.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.93%. Comparing base (347388f) to head (82e7b98).
⚠️ Report is 3 commits behind head on dev.

Additional details and impacted files

@@             Coverage Diff              @@
##                dev    #2807      +/-   ##
============================================
+ Coverage     62.15%   62.93%   +0.77%     
- Complexity     2790     2829      +39     
============================================
  Files           215      215              
  Lines         16993    17052      +59     
  Branches       2474     2469       -5     
============================================
+ Hits          10562    10731     +169     
+ Misses         5254     5153     -101     
+ Partials       1177     1168       -9     

Components	Coverage Δ
Analytics	47.92% <ø> (ø)
SalesforceSDK	55.79% <100.00%> (+1.52%)	⬆️
Hybrid	59.05% <ø> (ø)
SmartStore	78.20% <ø> (ø)
MobileSync	81.68% <ø> (ø)
React	52.36% <ø> (ø)

Files with missing lines	Coverage Δ
...lesforce/androidsdk/config/LoginServerManager.java	70.70% <ø> (ø)
.../src/com/salesforce/androidsdk/ui/LoginActivity.kt	43.77% <100.00%> (+18.82%)	⬆️
...src/com/salesforce/androidsdk/ui/LoginViewModel.kt	90.52% <100.00%> (+6.97%)	⬆️

... and 5 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.

[JohnsonEricAtSalesforce]

While still in draft, there are some significant changes coming to this today. Please hold reviewing. Thanks!

[github-actions]

Job Summary for Gradle

Pull Request :: test-android

Gradle Root Project	Requested Tasks	Gradle Version	Build Outcome	Build Scan®
SalesforceMobileSDK-Android	libs:SalesforceSDK:lint	8.14.3	❌
SalesforceMobileSDK-Android	libs:SalesforceSDK:assembleAndroidTest	8.14.3	❌
SalesforceMobileSDK-Android	libs:SalesforceSDK:convertCodeCoverage	8.14.3	✅

[JohnsonEricAtSalesforce]

@wmathurin and @brandonpage, I'm still regression testing this now that I've cherry-picked it from the app's repo plus reviewing tests and coverage. So, it still qualifies as a draft. That said, can you take a preview look and see how the approach looks? Watching the code run via the debugger, logs and what I see in the app the new pendingServer observable really helps. See how you like it and we could investigate other options it there's something I didn't think of where it doesn't work. The goal is just to give the view model and its observers a way to pre-flight the server while we're waiting on the asynchronous fetch of the authentication configuration (if needed).

[brandonpage]

Overall I like the direction. It has a similar theme to my open PR about allowing a different auth config per server. Because the auth config values are used to generate the login url on server change now come from an async function I avoid reloading the WebView until we have the data. Seems very similar to not loading the WebView until we know we aren't using a CustomTab instead.

I don't quite understand (or remember?) why we need to re-launch the LoginActivity for Welcome but we can take that offline.

[JohnsonEricAtSalesforce]

Overall I like the direction. It has a similar theme to my open PR about allowing a different auth config per server. Because the auth config values are used to generate the login url on server change now come from an async function I avoid reloading the WebView until we have the data. Seems very similar to not loading the WebView until we know we aren't using a CustomTab instead.

I don't quite understand (or remember?) why we need to re-launch the LoginActivity for Welcome but we can take that offline.

Thanks for the direction feedback 💡 I at least wanted vet that before going too much further.

The activity life-cycle is a good topic to refresh our shared awareness of since it's evolved over the course of several code reviews of QR Code Log In, WSC Deep-Link and now a few iterations of WSC Discovery. Each of these has ways the app can start LoginActivity and need to pass parameters such as the QR Code Log In URL or the WSC Login Hint. Those suit well to Intent extras and data. Where MSDK had internal paths to pass this data to LoginActivity, we'd tried creating a "internal" set of methods or properties on LoginActivity or SalesforceSDKManger, but that become a second path to the same logic in some cases. Having two ways (Intent or internal methods) created a maintenance burden. That was where I believe @wmathurin and I centralized on using the LoginActivity Intent as a single path to QR Code Login and WSC Login Hint/WSC Discovery use cases. Plus, since we're re-using the Activity instance it's very efficient.

Without being so wordy [sorry], I could just have said since QR Code Log In/WSC Log In Hint/WSC Welcome Discovery are all available to the app to invoke when starting a LoginActivity, they all present a single public API via its Intent data and extras.

[JohnsonEricAtSalesforce]

I just added a commit e3a3fc4 that addresses the original issue reported by the app plus improves the way the view model observables are used to improve the readability, runtime flow and has options for better tests (I'll start those once we vet this approach).

The app's issue was that the authentication configuration wasn't getting fetched at the correct time when selecting WSC Discovery tiles for servers that needed it. The code-style issue I wanted to solve is making sure the activity's view model observers respond correctly to the WSC Discovery flow.

The gist of accomplishing all this compared to our state on dev is:

1. Creating the new pendingServer observable so a new value from LoginServerManager can let the activity fetch the authentication configuration (if needed) before setting selectedServer and continuing the flow
2. Separating the pendingServer observer into two parts so the the logic related to WSC Discovery can be called as needed, not just on a new value
3. Splitting the activity's custom tab logic into a new observer on the loginUrl

This really does a nice start to separating all the logic conditions that are currently in that one, big selectedServer observer on dev. It works in my app testing also. I'm still testing in the app and in our template apps, but the functionality all seems to be working.

I'd love to get early feedback on the idea of adding a few new observables like this to make smaller, more manageable steps out of the auth flow.

[JohnsonEricAtSalesforce]

This has been an issue for some time. The constructor has blocked nulls but the nullability of the backing fields was never updated. This improves the tests on this pull request greatly plus it probably should have been done for integrity long ago.

[brandonpage]

Is there any way the app could still have a null name that was entered prior to MSDK 13.0?

[JohnsonEricAtSalesforce]

The app would have to be doing something very deliberate to achieve that plus ignoring the non-null constructor parameters. Interestingly, an AI analysis also notes that since URL and name are treated as non-null in a lot of code throughout MSDK, they'd already be triggering null pointer exceptions in many places. Take a look at the number of non-null safe calls to "trim" there are on "name".

So, this looks like a real win to help prevent future NPEs.

[JohnsonEricAtSalesforce]

Expanding on my earlier work in this pull request the code related to the custom tab activity is fully extracted from onCreate and polished into discrete code components that are 100% covered by tests.

[JohnsonEricAtSalesforce]

It's worth repeating from my notes last week that so much code has been lifted out of onCreate and into 100% code covered testable code. That's really going to help prevent regression.

[JohnsonEricAtSalesforce]

This is a great example of the kind of regression I'm hoping the newer, higher coverage tests will prevent 💯 coverage 🤘🏻

[JohnsonEricAtSalesforce]

Creating named classes for activity result callback and observer lets them be instantiated in tests with fully mocked dependencies. That's been super helpful in lifting out untestable in-line code and getting it to 100% coverage.

[JohnsonEricAtSalesforce]

Here's the new browser custom tab URL observer. I did add additional documentation around this and other properties. Their use is too difficult to infer simply from the code, especially now that many new authentication flows are in use.

[JohnsonEricAtSalesforce]

It was very subtle to find, but the logic branching in this method was much higher with the URI constructor check at the start.

[JohnsonEricAtSalesforce]

To make concurrent code testable, scope and dispatcher injection really helps. See https://developer.android.com/kotlin/coroutines/test#injecting-test-dispatchers

[JohnsonEricAtSalesforce]

We really should avoid force unwraps whenever possible. This was the cause of occasional crashes in tests. Note the new local variable is thread safe. Any time multiple references are made to an external value in a method, using a local will guard against concurrency issues.

[JohnsonEricAtSalesforce]

I've successfully updated the app which reported this issue from 13.1.0 to this branch so I can regression test. Thus far, I've completed a lot of regression testing and all looks well. I have to wait one day for the app to be testable with the required client certificates (due to my test device's settings). I'll report then on the final test.

If there _is any feedback to incorporate to this pull request, it would be most efficient if I could do that before running that test. There's a one day delay before I can re-test each change.

[wmathurin]

The approach makes sense. I wish there was a way to keep things simpler.
Could you draft a PR for the change for 13.1.1 (against master) so we can see what the changes would look like there?
Also, once we have auth UI tests, we need to have tests for welcome scenarios also. @brandonpage

[JohnsonEricAtSalesforce]

The approach makes sense. I wish there was a way to keep things simpler. Could you draft a PR for the change for 13.1.1 (against master) so we can see what the changes would look like there? Also, once we have auth UI tests, we need to have tests for welcome scenarios also. @brandonpage

Thanks, @wmathurin. I'll have that 13.1.1 patch pull request ready soon.

[brandonpage]

Is there any way the app could still have a null name that was entered prior to MSDK 13.0?

[brandonpage]

I still don't understand why pendingServer (and now browserCustomTabUrl) need to be added.

If we want to make sure the web view is not loaded when it shouldn't be, I would very much prefer the approach of:

1. Fetch the auth config when selectedServer changes.
2. Generate the Authorization URL.
3. Either set loginUrl or load the auth url into the custom tab.

[JohnsonEricAtSalesforce]

I see the single loginUrl observable becomes unwieldy in point #3. The trouble is that loginUrl is a single view model observable for two view elements (in different classes for two different use cases). When loginUrl is set, the view model has to use conditional logic to determine if the web view should be used and the activity has to use separate conditional logic to determine if the custom tab is used. One of the two observers "ignores" each value. Splitting these two concerns into separate observables loginUrl (web view) and customTabUrl really simplifies that logic compared to "overloading" one observable for multiple purposes.

For point #1, MSDK doesn't always fetch the authentication configuration. There's a dedicated property named singleServerCustomTabActivity that specifically by-passes the whole routine. We could analyze why that's there, but it's part of the flow to consider.

More specifically to pendingServer: As soon as selectedServer is set the view model's observer for the web view considers that immediately usable and loads the web view. Meanwhile, the activity is potentially fetching the auth config and loading the custom tab. These two observers are not interconnected or aware of each other's actions. This async behavior (plus the auth config fetch being anonymous inside the selectedServer observer) were the root cause of the bug here. Having a server value to observe for the "pending authentication configuration" step helps coordinate this.

[brandonpage]

One thing I didn't consider is that we cannot directly launch the custom tab from the view model. A trigger is needed, so in that regard you are correct. browserCustomTabUrl absolutely servers a valid purpose.

singleServerCustomTabActivity is a mode I added for Trailhead GO. It simply always uses a custom tab. Salesforce login is one option they support so if it is triggered from their launcher activity it is used like you would expect a custom tab to be in any other app.

In the steps above I was trying to describe something like this:

loginUrl.addSource(selectedServer) { newServer: String? ->
   if (!isUsingFrontDoorBridge && newServer != null) {
      val isNewServer = loginUrl.value?.startsWith(newServer) != true
         if (isNewServer) {
            viewModelScope.launch {
               /* -------- New code below ---------  */

               // Show loading indicator because appConfigForLoginHost could take a noticeable amount of time.
              loading. value = true
              val deferredAuthConfig = async { fetchAuthenticationConfiguration(newServer) } 
              val deferredUrl = async { getAuthorizationUrl(newServer) }
              if (deferredAuthConfig.await()?.isBrowserLoginEnabled == true) {
                 // this launches the custom tab in the activity without setting loginUrl
                 browserCustomTabUrl.value = deferredUrl.await() + "&prompt=login"
              } else {
                 loginUrl.value = deferredUrl.await()
             }  
         }
      }
   }
}

For the above to work fetchAuthenticationConfiguration needs to move to the view model but I think that would be a good chance. It also opens us up to moving isBrowserLoginEnabled and isShareBrowserSessionEnabled to the view model in the future, which I think makes a lot of sense.

[JohnsonEricAtSalesforce]

Thanks for considering browserCustomTab and how that separation could help. That note about singleServerCustomTabActivity really helps add historical context as well. I appreciate that.

I also believe moving more logic into our relatively new view model is great.

Thanks for sharing some code. I am hoping to change one aspect. In this snippet, setting selectedServer triggers a logic switch between setting loginUrl or browserCustomTab but the observer is attached to the "source" of loginUrl. Couldn't that simply be an observer on selectedServer? Also, this doesn't examine singleServerCustomTabActivity and moves to always fetching the auth config. Is that intended since that's a shift from the logic that's on dev?

[brandonpage]

Yeah, sorry that was some quick pseudocode, should probably observe selectedServer directly.

singleServerCustomTabActivity should probably be used in fetchAuthenticationConfiguration. We can check that at the same time we check if the server is login.salesforce.com or test.salesforce.com and skip the actual fetch.

I only added comments

[brandonpage]

What does switchDefaultOrSalesforceWelcomeDiscoveryLogin mean? Maybe something like useWelcomeDiscoveryIfNeeded would be better?

[JohnsonEricAtSalesforce]

I did want to include the notion that it will return to the default mode from WSC as well if needed. It's a combination check.

[brandonpage]

@JohnsonEricAtSalesforce Can you (or @wmathurin) explain again why we would need to restart the activity in this scenario? Considering we have live data, I do not understand why that would ever be necessary.

[JohnsonEricAtSalesforce]

It actually doesn't restart the activity. We made some changes in the first pass of WSC so that the new intent is received by the existing instance, which then updates the view model. Triggering the new intent to update the view model mirrors how WSC deep links update the view model by starting login activity from other intents. The benefit is that each entry point to the various parts of WSC can be started by the app just by starting the activity intent like a deep link.

[brandonpage]

I don't really like the structure of most of the additions here. IMO pendingServer is unnecessary. And I really thing all of the logic in PendingServerObserver and BrowserCustomTabUrlObserver should be in the view model and not the activity.

But I do not want to to block the PR over my opinion if the code is functional and well tested.

[JohnsonEricAtSalesforce]

I don't really like the structure of most of the additions here. IMO pendingServer is unnecessary. And I really thing all of the logic in PendingServerObserver and BrowserCustomTabUrlObserver should be in the view model and not the activity.

But I do not want to to block the PR over my opinion if the code is functional and well tested.

Perhaps, for a constructive path forward, we can schedule some time to analyze the goals the existing code is trying to achieve and coordinate a new pattern together for the next major release. We can see there are short comings in much of the legacy code that was incorporated as-is during the recent introduction of the view model, so it will require time and a coordinated effort to work together on its evolution while also adding significant new features.