-
Notifications
You must be signed in to change notification settings - Fork 391
@W-17964655: [Android] WSC discovery endpoint to postpone OAuth flow #2736
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Generated by 🚫 Danger |
@@ -1,6 +1,6 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<servers> | |||
<server name="Production" url="https://login.salesforce.com" /> | |||
<server name="Welcome" url="https://welcome.salesforce.com" /> | |||
<server name="Welcome" url="https://welcome.salesforce.com/discovery" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wmathurin - I've set up the MSDK logic to distinguish Salesforce Welcome Discovery URLs by the path element. According to our convention, that works and doesn't require us to add an UX for the user to specify if they're entering discovery URL in the "Add Connection" bottom sheet.
Alternately, we could add some way for the user to check-off that the scheme and host they've entered is for discovery and then add this path in the MSDK logic. That seemed more intrusive to the user, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to other ideas. This seemed to keep it simple, at first blush.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The end user wouldn't know if the app developer is using a scheme and host for discovery, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. We also don't have a specification for what this will look like beyond internal use, so it might change.
import java.security.PrivateKey | ||
import java.security.cert.X509Certificate | ||
|
||
/** | ||
* Login activity authenticates a user. Authorization happens inside a web view. | ||
* | ||
* Support for Salesforce Welcome (WSC) Discovery is provided. If the activity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's some light code documentation for login activity's welcome discovery support.
@@ -180,6 +194,7 @@ open class LoginActivity : FragmentActivity() { | |||
webViewClient = [email protected] | |||
webChromeClient = [email protected] | |||
setBackgroundColor(Color.Transparent.toArgb()) | |||
settings.domStorageEnabled = true /* Salesforce Welcome Discovery requires this */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We knew this was coming back based on earlier support discussions.
// Set the Salesforce Welcome Login hint and host for the OAuth authorize URL, if applicable. | ||
useLoginHint(intent) | ||
// Apply the intent extras' Salesforce Welcome Login hint and host for use in the OAuth authorize URL, if applicable. | ||
applySalesforceWelcomeLoginHintAndHost(intent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worked hard to think of login activity's use cases and intuitive semantics in naming conventions. That way I could really extract Salesforce Welcome Discovery into readable method calls that kept that logic separate from the existing logic branches, which are more in-line with each other.
} | ||
|
||
// Use the URL to switch between default or Salesforce Welcome Discovery log in, if applicable. | ||
if (switchDefaultOrSalesforceWelcomeDiscoveryLogin(selectedServerUri)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's another location where I worked hard to extract the WSC logic from existing logic.
* Uses the Salesforce Welcome login hint and host in the Intent, if | ||
* applicable. | ||
*/ | ||
private fun useLoginHint(intent: Intent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This moved into a region with other WSC code that made more sense with the existing region markers in this source.
@@ -881,6 +892,150 @@ open class LoginActivity : FragmentActivity() { | |||
) | |||
} | |||
|
|||
// region Salesforce Welcome Login Private Implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each of the new WSC specific methods in this region should hopefully be self-explanatory with their source commentary as well.
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (30.76%) 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 #2736 +/- ##
============================================
- Coverage 56.18% 56.01% -0.18%
- Complexity 2469 2476 +7
============================================
Files 210 210
Lines 17116 17193 +77
Branches 2381 2397 +16
============================================
+ Hits 9617 9631 +14
- Misses 6467 6517 +50
- Partials 1032 1045 +13
🚀 New features to boost your workflow:
|
} | ||
|
||
// Use the URL to switch between default or Salesforce Welcome Discovery log in, if applicable. | ||
if (switchDefaultOrSalesforceWelcomeDiscoveryLogin(selectedServerUri)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually, this is step #1 where the user selects welcome from the select server or has entered it in add connection. This'll stop the default login activity and start an instance for welcome just as if the app had started an instance in response to an external link. I like that the activity never "switches" between default or welcome log in modes except by finishing and starting again. That keeps the runtime testing really simple.
|
||
// Apply the URL as the initial login URL if it's a valid Salesforce Welcome Discovery URL. | ||
intent.data?.let { uri -> | ||
useSalesforceWelcomeDiscoveryMobileUrl(uri) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is step #2 where the activity detects a welcome URL and applies it instead of default login. This is something like what we did for front door URL, but the code is more concise this time.
@@ -890,6 +1045,12 @@ open class LoginActivity : FragmentActivity() { | |||
*/ | |||
open inner class AuthWebViewClient : WebViewClient() { | |||
override fun shouldOverrideUrlLoading(view: WebView, request: WebResourceRequest): Boolean { | |||
|
|||
// Use the request's Salesforce Welcome Discovery mobile callback URL, if applicable. | |||
if (useSalesforceWelcomeDiscoveryMobileCallbackUrlForDefaultLogin(request.url)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Step #3 is detecting welcome's mobile callback after an environment selection and using the login hint to start default login with the My Domain host.
// Set the Salesforce Welcome Login hint and host for the OAuth authorize URL, if applicable. | ||
useLoginHint(intent) | ||
// Apply the intent extras' Salesforce Welcome Login hint and host for use in the OAuth authorize URL, if applicable. | ||
applySalesforceWelcomeLoginHintAndHost(intent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final step #4 is using login hint and host in default log in after the user completes the welcome flow.
private fun isSwitchFromSalesforceWelcomeDiscoveryToDefaultLogin( | ||
proposedSelectedServerUrl: Uri | ||
) = viewModel.loginUrl.value?.toUri()?.let { | ||
isSalesforceWelcomeDiscoveryMobileCallbackUrl(proposedSelectedServerUrl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't that be:
private fun isSwitchFromSalesforceWelcomeDiscoveryToDefaultLogin(
proposedSelectedServerUrl: Uri
) = viewModel.loginUrl.value?.toUri()?.let {
isSalesforceWelcomeDiscoveryMobileUrl(it)
} ?: false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are exactly correct and I'd been looking for that while I was testing and seeing something a little off. It's nice to know someone (if not me 🥴) followed my naming convention and code comments so well. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wmathurin - Here's the commit with the fix. f36cbd8
loginServerManager.setSelectedLoginServer( | ||
LoginServer(loginHost, loginUrl, true) | ||
) | ||
loginServerManager.addCustomLoginServer(loginHost, loginUrl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be added before being selected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. It works in this order. That's probably because the user doesn't return to the picker until after both updates are done. I'll try switching it just to see how it goes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the order in f36cbd8
…(Correct Logic Of `isSwitchFromSalesforceWelcomeDiscoveryToDefaultLogin`)
eeb31be
to
f36cbd8
Compare
@@ -167,7 +168,11 @@ public void setSelectedLoginServer(LoginServer server) { | |||
edit.putString(SERVER_URL, server.url); | |||
edit.putBoolean(IS_CUSTOM, server.isCustom); | |||
edit.apply(); | |||
selectedServer.postValue(server); | |||
if (Looper.myLooper() == Looper.getMainLooper()) { | |||
selectedServer.setValue(server); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This optimization uses the synchronous setValue
on the main thread, when applicable, and falls back to the asynchronous postValue
when off main. In LoginActivity
, this makes the UX smoother since we're already on the main thread.
…(Correct Login Activity Lifecycle For Salesforce Welcome Discovery Use To Match Other Login Activity Use Cases)
@wmathurin - One more update to this one. 1b4d8ee updates |
loginServerManager.setSelectedLoginServer( | ||
LoginServer(loginHost, loginUrl, true) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't addCustomLoginServer
already set the LoginServer it creates as the selected option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does, indeed. I'll update that.
* @param salesforceWelcomeDiscoveryHostAndPathUrl The Salesforce Welcome | ||
* Discovery host and path URL | ||
* @return A Salesforce Welcome Discovery mobile URL with all required | ||
* parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT and Unrelated to this PR: I found out recently that the official Kotlin style guide actually discourages using @param
and @return
in documentation comments 🤯.
If we wanted to go that route it would obviously be quite a large change if we wanted to be consistent everywhere, so perhaps we should have a team discussion about it in the near-ish future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is really interesting and feels like a loss of helpful formatting based on how Android Studio displays documentation. They don't seem to offer an explanation, either. We'll have to ponder that one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated what little code documentation we have on the other project and found I really like the new style. The doc definitely became far more readable without all the boiler plate. Doesn't seem like any information is lost, but things like return value might be a little less glanceable.
if (isSalesforceWelcomeDiscoveryUrlPath(uri)) { | ||
|
||
// Navigate to Salesforce Welcome Discovery. | ||
startActivity( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to avoid this? Since we are using single top it shouldn't create multiple LoginActivity's simultaneously and mess up the back stack but it just feels a heavy handed way of changing the login url (tbf I've done similar things in the past). Maybe there are other reasons I am not seeing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They key driver was that the app can receive an external link and will need a way to start a "Welcome" LoginActivity
according to the app's own lifecycle and timing. It's possible that the "Default" LoginActivity
is already resumed and will then switch to "Welcome." Or, the app could directly start a new "Welcome" LoginActivity
at any time, perhaps from an external link and kind of like QR Code Log In. This gives the app and MSDK one Intent and a consistent approach to start that adjusts to the current state of things on the fly.
I test this two ways. One is by using the server picker. The second is by emulating an external link being received in the app's MainActivity
. The external link may or may not be for the default, production WSC even. Before this is used externally, a lot about WSC may change. So we have some future unknowns to adapt to.
Now that I've seen how Welcome played out like this, I'm looking back at QR Code Log In and wondering if it would be a beneficial way to clean that up as well. The logic there is not this organized.
It's also kind of like the way the Chrome callback is handled as well, at least from what I saw in onNewIntent
.
Maybe there's other ways. Though, now that we have QR Code Log In and Welcome exposing non-default paths to LoginActivity
via its Intent
seems to be working really and follows Android basics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-reading the original question and answering it more directly, this actually doesn't create multiple activities and preserves the back-stack plus the app's main activity really neatly. Single top will re-use the existing instance with a call to onNewItent
. I watched it closely in the debugger to be sure as well 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay! I wasn't thinking about the deep linking but it makes a lot of sense to have a single code path/point of entry to start Welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JohnsonEricAtSalesforce Speaking of onNewIntent
I forgot to comment yesterday and ask if you had tested Advanced Auth? I don't see a reason why it wouldn't work as expected but it is something we should confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you DM instructions on how to test that to me? I agree, it should work but does require testing.
// region Salesforce Welcome Login Private Implementation | ||
|
||
/** The default Salesforce Welcome Discovery mobile callback URL. This value is fixed until future Salesforce Welcome updates */ | ||
private const val SALESFORCE_WELCOME_DISCOVERY_MOBILE_CALLBACK_URL = "sfdc://discocallback" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: These all feel a bit wordy for me personally. I don't think they really need Salesforce in the name and I think they would be find with either "WELCOME" or "DISCOVERY", but that's just my 2 cents.
@@ -1,6 +1,6 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<servers> | |||
<server name="Production" url="https://login.salesforce.com" /> | |||
<server name="Welcome" url="https://welcome.salesforce.com" /> | |||
<server name="Welcome" url="https://welcome.salesforce.com/discovery" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The end user wouldn't know if the app developer is using a scheme and host for discovery, right?
…(Light Code Review Cleanup)
What happens if a non-supported client app uses disco? I'm guessing the redirect never takes place. Could we/should we handle that case gracefully? |
You're correct that |
Where is that logic? |
SALESFORCE_WELCOME_DISCOVERY_MOBILE_URL_QUERY_PARAMETER_KEY_CLIENT_ID | ||
) != null && uri.queryParameterNames?.contains( | ||
SALESFORCE_WELCOME_DISCOVERY_MOBILE_URL_QUERY_PARAMETER_KEY_CALLBACK_URL | ||
) != null && uri.getQueryParameter(SALESFORCE_WELCOME_DISCOVERY_MOBILE_URL_QUERY_PARAMETER_KEY_CLIENT_ID) == getBootConfig(context).remoteAccessConsumerKey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wmathurin - This is the validation logic matching your comment a moment ago.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The server currently will only allow discovery for a handful of consumer keys (see https://git.soma.salesforce.com/globalIdentity/LoginService/blob/domain-disco-2/ui/lwr/src/modules/login/constants/myDomainDiscoveryConstants.ts#L13). I don't really like the idea of hard-coding that in the client side code. Do you know if the server errors out if you attempt discovery with a non-supported client id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The server does not error if given an unknown client id. For instance, you could try this URL in your web browser.
https://gidruntime-cell2.sfproxy.gid.dev1-uswest2.aws.sfdc.cl/?client_id=SfdcMobileChatterXXX&client_version=3.6.0&callback_url=sfdc%3A%2F%2Fdiscocallback
What it seems to do it ignore the three parameters entirely. This means it will prompt for the user's password when a selection is made in the Environment Switcher. That sort of makes sense, but what's odd is that when a valid password is entered the user gets redirected to stage.welcome.com
instead of authenticated with the instance. Perhaps that's a bug?
I don't have any logic in MSDK other than ensuring the client id in an externally linked WSC Discovery URL matches what's in the app's boot config plus making sure that WSC Discovery URLs generated by MSDK use the app's boot config.
What could we do here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe in the check above, you could see if the client id is one of those? Maybe put some big TODO to remove that logic once the server supports all client ids?
If it's not, then you would let them do a login with WSC without the discovery step, which will work as long as they don't expect advanced authentication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wmathurin - How does this look? e437641
…(Lock In Supported Client Id Values For Salesforce Welcome Discovery)
) != null && uri.queryParameterNames?.contains( | ||
SALESFORCE_WELCOME_DISCOVERY_MOBILE_URL_QUERY_PARAMETER_KEY_CLIENT_VERSION | ||
) != null && uri.queryParameterNames?.contains( | ||
SALESFORCE_WELCOME_DISCOVERY_MOBILE_URL_QUERY_PARAMETER_KEY_CALLBACK_URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment to indicate that this should be kept in sync with the corresponding client ids list on the server and also have a todo to remove that check once discovery is made available to all client ids?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a look at ff78c26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
SALESFORCE_WELCOME_DISCOVERY_MOBILE_URL_QUERY_PARAMETER_KEY_CALLBACK_URL | ||
) != null && uri.getQueryParameter(SALESFORCE_WELCOME_DISCOVERY_MOBILE_URL_QUERY_PARAMETER_KEY_CLIENT_ID) == getBootConfig(context).remoteAccessConsumerKey | ||
): Boolean { | ||
val clientIdParameter = uri.getQueryParameter(SALESFORCE_WELCOME_DISCOVERY_MOBILE_URL_QUERY_PARAMETER_KEY_CLIENT_ID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What did you think of @brandonpage suggestion to shorten constant names:
- SALESFORCE_WELCOME_DISCOVERY_MOBILE_URL_QUERY_PARAMETER_KEY_CLIENT_ID could just be DISCOVERY_MOBILE_URL_QUERY_PARAMETER_KEY_CLIENT_ID
- etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to toss it out there, the reason I thought the longer names is helpful is so they key really, really well to the names of the documents and other resources where I (being the newer developer) know where to find help. Even being a developer for Mobile SDK, our support across so many aspects of the product can be difficult to keep organized.
Let me know what we want to do either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense... but by that same logic you could literally prefix everything we do with SALESFORCE
, which wouldn't be helpful.
…(Lock In Supported Client Id Values For Salesforce Welcome Discovery - Commentary Update)
395a7f9
into
forcedotcom:dev
🎸 Ready For Review 🥁
LoginActivity
is updated to support selecting Salesforce Welcome Discovery URLs plus responding to their callback URL to initiate log in.During the discussion in Slack with @wmathurin and @Crebs, we leaned towards having WSC behave and look as much like the existing default login activity as possible. In particular, app bar and overflow menu are identical. With that in mind, it was really easy to fold the new logic into our existing activity.
Here's a video that shows the new login/welcome flow in action. This updated version of the video today shows the more polished version.
Screen_recording_20250721_140629.webm