Skip to content

@W-19461869: [M1][MSDK13.1] Enable Flexible Server Matching for QR Code Login (Android) #2762

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.

Sign up for GitHub

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

Closed
JohnsonEricAtSalesforce wants to merge 8 commits into from
Closed

@W-19461869: [M1][MSDK13.1] Enable Flexible Server Matching for QR Code Login (Android) #2762

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a92e929
@W-19461869: [M1][MSDK13.1] Enable Flexible Server Matching for QR Co…
JohnsonEricAtSalesforce Sep 10, 2025
250fe94
@W-19461869: [M1][MSDK13.1] Enable Flexible Server Matching for QR Co…
JohnsonEricAtSalesforce Sep 10, 2025
f4ba7a9
@W-19461869: [M1][MSDK13.1] Enable Flexible Server Matching for QR Co…
JohnsonEricAtSalesforce Sep 10, 2025
de898b2
@W-19461869: [M1][MSDK13.1] Enable Flexible Server Matching for QR Co…
JohnsonEricAtSalesforce Sep 10, 2025
aa5ea4d
@W-19461869: [M1][MSDK13.1] Enable Flexible Server Matching for QR Co…
JohnsonEricAtSalesforce Sep 10, 2025
6717f3a
@W-19461869: [M1][MSDK13.1] Enable Flexible Server Matching for QR Co…
JohnsonEricAtSalesforce Sep 11, 2025
2577239
@W-19461869: [M1][MSDK13.1] Enable Flexible Server Matching for QR Co…
JohnsonEricAtSalesforce Sep 12, 2025
d9a6f19
@W-19461869: [M1][MSDK13.1] Enable Flexible Server Matching for QR Co…
JohnsonEricAtSalesforce Sep 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion libs/SalesforceSDK/src/com/salesforce/androidsdk/config/RuntimeConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,14 @@
*/
package com.salesforce.androidsdk.config;

import static androidx.annotation.VisibleForTesting.PACKAGE_PRIVATE;

import android.content.Context;
import android.content.RestrictionsManager;
import android.os.Bundle;

import androidx.annotation.VisibleForTesting;

import com.salesforce.androidsdk.analytics.EventBuilderHelper;
import com.salesforce.androidsdk.app.Features;
import com.salesforce.androidsdk.app.SalesforceSDKManager;
Expand Down Expand Up @@ -69,7 +73,8 @@ public enum ConfigKey {

private static RuntimeConfig INSTANCE = null;

RuntimeConfig(Context ctx) {
@VisibleForTesting(otherwise = PACKAGE_PRIVATE)
Copy link
Contributor

@brandonpage brandonpage Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not know you could could use the Kotlin style named parameter syntax in a Java file. Must be because it is an annotation?

Copy link
Contributor Author

@JohnsonEricAtSalesforce JohnsonEricAtSalesforce Sep 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recall using named parameters with Java annotations all the way back to Java 5 and Hibernate ORM (2005), which used it a lot. That's an interesting place where named parameters snuck in early and I'd never thought about it 🙂

public RuntimeConfig(Context ctx) {
configurations = getRestrictions(ctx);
isManaged = hasRestrictionsProvider(ctx);

Expand Down
6 changes: 3 additions & 3 deletions libs/SalesforceSDK/src/com/salesforce/androidsdk/ui/FrontdoorBridgeLoginOverride.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,11 +124,11 @@ internal class FrontdoorBridgeLoginOverride(
appLoginServer = frontdoorBridgeUrl.host
}

appLoginServer?.let { appLoginServer ->
appLoginServer?.let { server ->
matchesLoginHost = true
// Set the login appLoginServer on the server manager
// Set the login server on the server manager
val loginServerManager = SalesforceSDKManager.getInstance().loginServerManager
val loginUrl = "https://$appLoginServer"
val loginUrl = "https://$server"
loginServerManager.addCustomLoginServer(loginUrl, loginUrl)
}
}
Expand Down
10 changes: 8 additions & 2 deletions libs/SalesforceSDK/src/com/salesforce/androidsdk/ui/FrontdoorBridgeUrlAppLoginServerMatch.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ internal data class FrontdoorBridgeUrlAppLoginServerMatch(
}

if (frontdoorBridgeUrl.isMyDomain()) {
val frontdoorBridgeUrlMyDomainSuffix = frontdoorBridgeUrlHost.split("\\.my\\.").last()
val frontdoorBridgeUrlMyDomainSuffix = "my.${frontdoorBridgeUrlHost.split(".my.").last()}"
if (frontdoorBridgeUrlMyDomainSuffix.isNotEmpty()) {
Copy link
Contributor

@brandonpage brandonpage Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am surprised the compiler did not complain about this check as it can never be false. The suffix is always "my." + something.

Copy link
Contributor Author

@JohnsonEricAtSalesforce JohnsonEricAtSalesforce Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. We can simplify that.

for (eligibleAppLoginServer in eligibleAppLoginServers) {
if (eligibleAppLoginServer.endsWith(frontdoorBridgeUrlMyDomainSuffix)) {
Copy link
Contributor

@brandonpage brandonpage Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this just match any My Domain? By this logic "one.my.salesforce.com" matches "two.my.salesforce.com".

Copy link
Contributor

@wmathurin wmathurin Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brandonpage is right a bit of the proposed logic has been lost:

[Soft match] Look at part of the hostname in the QR code that comes after .my. and make sure it appears in the currently selected login server also as long as the currently selected login server does not have .my, itself. When the currently login server has a .my. the whole hostname should match. So mydomain.my.salesforce.com would be allowed if login.salesforce.com is currently selected but not if myotherdomain.my.salesforce.com is selected.

So if we have a my domain front door url, and we did not find an exact match, we should look among the eligible login hosts that are not my domain themselves.

The idea is that if the QR says one.my.salesforce.com and you have login.salesforce.com, then you should be let through.
The only weakness is that if you have login.salesforce.com and test.salesforce.com we won't know which one to the my domain "belongs" to. That would however only be a problem if somehow the app disallowed login.salesforce.com and the QR code is for a production my domain (or vice versa with Sandbox).

Copy link
Contributor Author

@JohnsonEricAtSalesforce JohnsonEricAtSalesforce Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wmathurin, that issue with the "belongs to" note was causing the Salesforce app to fail for me though. The test orgs and users I have would create a QR Code for the specific My Domain host, but the app ships (in test, at least) with preset "login" hosts that the QR Code cannot be used with.

I had these notes in the story and the soft-match as we have it on this branch do work in the app with the test users and orgs I have at the moment.

  • If QR is not a my domain, existing login server must match exactly
  • If QR is a my domain, existing login server must either match exactly or match everything after the .my.
    1. If adding and switching are disallowed, only let the QR through if its login server "soft-matches" the currently selected login server.
    2. If adding is disallowed but switching is allowed, let the QR through if its login server "soft-matches" any of the login server and switch to it.
    3. If adding is allowed and switching is allowed, try (b) first, but if no match are found add the QR login server and switch to it.

There's definitely complexity to go around on this logic 😅 We might need to break this into a pairing session so we can demonstrate the behavior the app is seeing.

Copy link
Contributor Author

@JohnsonEricAtSalesforce JohnsonEricAtSalesforce Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's schedule some time to review this interactively in the app 🚀

Expand Down Expand Up @@ -100,7 +100,13 @@ internal data class FrontdoorBridgeUrlAppLoginServerMatch(
}
}
} else {
results.add(selectedAppLoginHost)
try {
val url = URL(selectedAppLoginHost)
results.add(url.host)
} catch (_: Exception) {
// If parsing fails, try to use as-is (might already be just a host)
Copy link
Contributor

@wmathurin wmathurin Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When do you think it would not parse but should still be used?

Copy link
Contributor Author

@JohnsonEricAtSalesforce JohnsonEricAtSalesforce Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of all the interesting things the AI tooling did that worked and didn't, this was the oddest choice - Or so I thought! 🥴. Nothing in my prompt or the existing logic suggested this, from what I saw. I was already planning on removing this before merging.

results.add(selectedAppLoginHost)
Copy link
Contributor

@brandonpage brandonpage Sep 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is a string that throws an exception when parsed as a URL ever going to be a valid URL host?

Copy link
Contributor Author

@JohnsonEricAtSalesforce JohnsonEricAtSalesforce Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wmathurin noticed this oddity injected by the generator as well. I'm still really curious where it got this notion since that wasn't in the prompt or really even hinted at by any code in context. 🤨 I've removed it.

}
}
return results
}
Expand Down
Loading
Loading