[iOS] Fix Login for Admin and Welcome Discovery incompatibility#4078
[iOS] Fix Login for Admin and Welcome Discovery incompatibility#4078brandonpage wants to merge 4 commits into
Conversation
Clang Static Analysis Issues
Generated by 🚫 Danger |
|
||||||||||||||||||
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #4078 +/- ##
==========================================
- Coverage 70.83% 68.38% -2.46%
==========================================
Files 246 246
Lines 21495 21535 +40
==========================================
- Hits 15227 14726 -501
- Misses 6268 6809 +541
🚀 New features to boost your workflow:
|
|
||||||||||||||||||
|
||||||||||||||
| // Login for Admin - forces browser-based (advanced) authentication to support phishing-resistant MFA. | ||
| [menuActions addObject:[UIAction actionWithTitle:[SFSDKResourceUtils localizedString:@"LOGIN_FOR_ADMIN"] | ||
| // Wrapped in an uncached UIDeferredMenuElement so the show/hide predicate is re-evaluated each | ||
| // time the menu opens. The entry is hidden during phase 1 of Welcome Discovery (i.e. before the |
There was a problem hiding this comment.
So we are hiding the menu during phase 1, do we expect admin to look for that menu in phase 2 or do we expect that to configure the correct login server manually if they need to login as admin?
There was a problem hiding this comment.
Hide in phase 1 so we don't have to deal with the discovery callback and the experience can be the same per platform (easier to document).
When the customer selects the org they want either the My Domain is configured to trigger Adv Auth for everyone or it loads in the WebView and the option to "Login for Admin" is in the menu.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
brandonpage
force-pushed
the
fix-login-for-admin-welcome-discovery
branch
from
b3dc482 to
cba3d81
Compare
| } | ||
|
|
||
| NSString *loginHost = session.oauthRequest.loginHost; | ||
| SFDomainDiscoveryCoordinator *discoveryCoordinator = [[SFDomainDiscoveryCoordinator alloc] init]; |
There was a problem hiding this comment.
A fresh SFDomainDiscoveryCoordinator is allocated here purely to call isDiscoveryDomain:, and the same pattern is repeated in SFUserAccountManager.m at the loginViewControllerDidSelectLoginForAdmin: guard. If isDiscoveryDomain: is stateless (just a string predicate, no mutable coordinator state needed), consider extracting it to a class method — e.g. +[SFDomainDiscoveryCoordinator isDiscoveryDomain:] — so callers don't need to alloc a throw-away instance. Minor, but two independent allocs for the same check at independent call sites looks slightly odd to a reader.
There was a problem hiding this comment.
Done. This adds new public API, but I don't think that is too big of a deal.
| // MARK: - SFSDKAuthRequest loginAsAdmin Property | ||
|
|
||
| func testGivenNewAuthRequest_whenCreated_thenLoginAsAdminIsFalse() { | ||
| func testGivenNewAuthRequest_whenCreated_thenloginAsAdminIsFalse() { |
There was a problem hiding this comment.
Nit: the renamed test methods have a lowercase l in the middle of the method name (e.g. testGivenloginAsAdmin_..., testGivenNologinAsAdmin_...), which reads as a typo. The intent seems to be matching the ObjC property name casing, but LoginAsAdmin with a capital L would be more legible in a test name. Up to you, but worth a pass to recapitalize if you agree.
There was a problem hiding this comment.
Good catch. I originally replaced loginAsAdmin to loginForAdmin in a bunch of places but later changed it back to make cherry-picking cleaner.
- Extract the stateless isDiscoveryDomain check to a class method on SFDomainDiscoveryCoordinator so callers no longer alloc a throwaway instance. The instance method delegates to it; existing callers are unchanged. Converts the LFA menu-visibility check, the LFA phase-1 guard, and the pre-existing setCurrentUser discovery check. - Add class-method + instance/class parity tests. - Capitalize "LoginAsAdmin"/"LoginForAdmin" in test method names (property/variable references stay lowercase). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| @@ -100,6 +100,11 @@ public class DomainDiscoveryCoordinator: NSObject { | |||
|
|
|||
| @objc | |||
| public func isDiscoveryDomain(_ domain: String?) -> Bool { | |||
There was a problem hiding this comment.
I like the class one! Could we deprecate this one too?
The class method must be @objc so ObjC callers (SFUserAccountManager.m, SFLoginViewController.m) can resolve +isDiscoveryDomain: through the generated -Swift.h header. Without it CI failed with "no known class method for selector 'isDiscoveryDomain:'". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The stateless discovery-host check now lives on the class method, so the instance method is redundant. Mark it @available deprecated (removed in 15.0) and migrate the last production caller (SFOAuthCoordinator) to the class method. Annotate the instance-method tests as exercising deprecated API so the build stays warning-free while coverage of the deprecated path is retained until removal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3 participants
Summary
The 13.2.1 Login for Admin (LFA) settings menu action fundamentally did not work with the 2-phase Welcome Discovery login flow. Tapping LFA on the discovered My Domain (phase 2) launched
ASWebAuthenticationSessionagainstwelcome.salesforce.cominstead of the resolved My Domain — a non-functional UX. Tapping it on the discovery page itself (phase 1) made things worse.This PR is a narrow fix that:
Hides the LFA menu entry during phase 1 of Welcome Discovery. A
UIDeferredMenuElement.elementWithUncachedProvider:re-evaluates visibility every time the menu opens, keyed off+[SFLoginViewController shouldShowLoginForAdminForSession:]. Phase-1 detection:SFDomainDiscoveryCoordinator.isDiscoveryDomainAND!coordinator.domainUpdated. Per-scene scoping viaview.window.windowScene.session.persistentIdentifier.Also no-ops the public
loginViewControllerDidSelectLoginForAdmin:API in phase 1, so external hosts that call it directly cannot bypass the visibility gate. Logs a warning. Documented on the public header.Routes the LFA browser session to the resolved My Domain in phase 2 with the captured
login_hintby introducing two LFA-scoped overrides onSFSDKAuthRequest:loginAsAdminMyDomain— the resolved My DomainloginAsAdminLoginHint— the username surfaced by the discovery pageConsulted in
authenticateWithRequest:loginHint:to setcredentials.domain, the coordinator'sloginHint, and theappConfigForLoginHost:lookup. The request's permanentloginHostis left unchanged.Clears all three LFA fields on cancel (
loginAsAdmin,loginAsAdminMyDomain,loginAsAdminLoginHint) so the post-cancelrestartAuthentication:re-runs against the originally configured login host.Why LFA-scoped overrides instead of mutating
oauthRequest.loginHostAn earlier version of this fix copied
coordinator.credentials.domainontosession.oauthRequest.loginHostin-memory. That looked clean but produced an asymmetry: after a cancelled LFA, Reload and Clear Cache kept the WebView pinned to the My Domain instead of returning towelcome.salesforce.com/discovery. Putting the override in dedicated LFA-scoped fields keepsloginHostreflecting "the configured login server" at all times, so every other settings action stays correct by definition.No persistence — Welcome Discovery returns after logout
The discovered My Domain is held only in memory on
SFSDKAuthRequest. It is never written tosetLoginHost:,SFSDKLoginHostStorage, orNSUserDefaults. After logout, the app correctly reloads the Welcome Discovery host and the user goes through phase 1 again — preserving the existing intentional behavior.Behavior change: phase-2 LFA cancel
After tapping Cancel in the system browser sheet, the embedded WebView returns to phase 1 of Welcome Discovery (
welcome.salesforce.com/discovery) and the user re-picks the account. This is intentionally different from Android, where the equivalent flow keeps the WebView on the resolved My Domain. The iOS choice matches existing iOS cancel behavior on every other login flow (cancel routes throughrestartAuthentication:against the configured login host) and avoids the Reload/Clear-Cache asymmetry described above. The cost is one extra account re-pick after a cancel; the benefit is symmetric, easy-to-reason behavior across all settings actions.Backport / cherry-pick notes
Welcome Discovery ships only to a few internal apps, so this fix is not being patched into 13.2.x. However, those apps may want to cherry-pick this change onto a pre-14.0 base before 14.0 ships, so the commit is deliberately structured to apply with minimal conflict:
loginAsAdminproperty name (no rename), so it touches none of the ~70loginAsAdminreferences that pre-14.0 branches carry.SFSDKAuthSession.m(theuseBrowserAuth || loginAsAdminline already exists on those branches), avoiding the deprecated-userAgentForAuthblock that differs across releases.v13.2.1: 5 of 6 files apply cleanly; the only conflict is a one-line placement nudge in the test bridging header (SalesforceSDKCoreTests-Bridging-Header.h), where adjacent unrelated test categories that exist ondevare absent on 13.2.1. Trivial to resolve by keeping the newSFLoginViewController (LoginForAdminTesting)category.Test plan
Automated tests
SalesforceSDKCoreTests/LoginForAdminTests— 34/34 passSalesforceSDKCoreTests/WelcomeDiscoveryLoginHostTests— 6/6 passSalesforceSDKCoreTests/DomainDiscoveryCoordinatorTests— passManual verification (AuthFlowTester)
login_hint=<username>. Does NOT open againstwelcome.salesforce.com.welcome.salesforce.com/discovery(phase 1) — intentionally different from Android (see Behavior change section above).login.salesforce.com): LFA still present and works as before.useWebServerFlow=NO, phase-2 LFA: authorize URL still usesresponse_type=codewith PKCE.loginViewControllerDidSelectLoginForAdmin:called externally during phase 1: no-op, warning logged, no browser opens.SFSDKLoginHostStorage/NSUserDefaultsinspection after phase-2 LFA + logout: My Domain is NOT persisted.Review checklist
SFSDKAuthRequest.his internal (not inpublic_header_files); the two new properties are not exposed. No symbols renamed.Localizable.stringschange.Podfile/.podspec/.xcodeproj/.xcworkspacechange.