[iOS] Validate login host input, recover to previous host on failure#4088
[iOS] Validate login host input, recover to previous host on failure#4088sfdctaka wants to merge 2 commits into
Conversation
- Reject empty/whitespace/syntactically-invalid host input in NewLoginHostView with an inline error. - Snapshot the previous login host on user-initiated host change so recovery can return there. - On HostConnectionErrorHandler dismissal: remove the failing host from storage if deletable, then restart auth on the previous host (falls back to index-0; guarded against empty storage). - New LOGIN_INVALID_HOST string in en.lproj — translation needed for other locales. Adds unit tests for the new validator and recovery handler.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #4088 +/- ##
==========================================
- Coverage 70.73% 68.64% -2.09%
==========================================
Files 246 246
Lines 21541 21595 +54
==========================================
- Hits 15237 14824 -413
- Misses 6304 6771 +467
🚀 New features to boost your workflow:
|
Clang Static Analysis Issues
Generated by 🚫 Danger |
|
||||||||||||||
|
||||||||||||||||||
|
||||||||||||||||
| struct NewLoginHostView: View { | ||
| @State var host = "" | ||
| @State var hostLabel = "" | ||
| @State var hostError: String? |
There was a problem hiding this comment.
Minor: hostError is internal view state — should be @State private var hostError: String? to prevent external mutation.
| .autocapitalization(.none) | ||
| .listRowSeparator(.hidden) | ||
|
|
||
| if let hostError { |
There was a problem hiding this comment.
The error persists while the user edits after a failed save. Add .onChange(of: host) { _ in hostError = nil } on the host TextField above so the inline error clears as soon as the user starts correcting input.
| } | ||
| } | ||
| } else if (!failing) { | ||
| [SFSDKCoreLogger d:[strongSelf class] format:@"Failing host not found in storage; skipping removal."]; |
There was a problem hiding this comment.
Log level: .d (debug) is too quiet here. A host missing from storage during a connection failure is unexpected — prefer .w so it surfaces in production log collection.
| strongSelf.loginHost = recoveryHost; | ||
| [strongSelf restartAuthentication:session]; | ||
| } else { | ||
| [SFSDKCoreLogger d:[strongSelf class] format:@"No recovery host available; skipping restart."]; |
There was a problem hiding this comment.
Log level: .d (debug) is too quiet for this outcome. When there is no recovery host, authentication silently stops with no user feedback — this is a degraded UX path that warrants .e (error) or at minimum .w (warning) so it surfaces in crash/log collection.
| } | ||
| } | ||
|
|
||
| func testSaveRejectsEmpty() { |
There was a problem hiding this comment.
These rejection tests verify that saveAction was not called (savedHost/savedLabel are nil), but don't verify that an inline error was actually surfaced to the user. Since @State is struct-level in SwiftUI (not directly observable from outside), testing hostError requires a different approach — e.g. extracting the validation logic into a pure testable function, passing hostError as @Binding, or using ViewInspector. At minimum, consider a testSaveErrorClearedOnSuccess case to verify the happy path clears a prior error. Applies to all five rejection tests.
| /// Drives the error handler block and waits until the alert OK completion has run. | ||
| /// Replaces alertDisplayBlock with a fake that immediately fires actionOneCompletion, | ||
| /// so we never present a real alert and the recovery logic runs deterministically. | ||
| - (void)fireHandlerBlockForSession:(SFSDKAuthSession *)session { |
There was a problem hiding this comment.
fireHandlerBlockForSession: stubs alertDisplayBlock but lets restartAuthentication: call through to the real implementation. With a minimal SFSDKAuthRequest (test client-id, test://callback redirect), this may trigger async coordinator state changes that race with the loginHost assertions. Consider stubbing or swizzling restartAuthentication: so recovery tests isolate the decision logic from the actual OAuth restart.
| [self waitForExpectations:@[completionRan] timeout:5.0]; | ||
| } | ||
|
|
||
| #pragma mark - Tests |
There was a problem hiding this comment.
Missing edge-case test: the PR description and the inline comment in SFUserAccountManager.m specifically call out the empty-storage guard (numberOfLoginHosts == 0 → log and bail rather than raising NSRangeException). That guard path has no test here. Consider adding test_givenEmptyStorage_when_handlerCompletionRuns_then_noRangeException — drain storage, fire the handler, and assert no crash and no host assignment occurs.
| B75233DE1F4DF0B700040B6E /* SFSDKAuthPreferences.h in Headers */ = {isa = PBXBuildFile; fileRef = B75233DC1F4DF0B700040B6E /* SFSDKAuthPreferences.h */; }; | ||
| B75233E01F4DF0B700040B6E /* SFSDKAuthPreferences.m in Sources */ = {isa = PBXBuildFile; fileRef = B75233DD1F4DF0B700040B6E /* SFSDKAuthPreferences.m */; }; | ||
| B759CD891F8BDBAC0081AA87 /* SDSDKAlertMessageTest.m in Sources */ = {isa = PBXBuildFile; fileRef = B759CD881F8BDBAC0081AA87 /* SDSDKAlertMessageTest.m */; }; | ||
| B7FF237400000002 /* SFUserAccountManagerLoginHostRecoveryTests.m in Sources */ = {isa = PBXBuildFile; fileRef = B7FF237400000001 /* SFUserAccountManagerLoginHostRecoveryTests.m */; }; |
There was a problem hiding this comment.
Hand-authored GUIDs (B7FF237400000001 / B7FF237400000002). Real Xcode GUIDs are random 12-byte hex values (e.g. B759CD891F8BDBAC0081AA87). Sequential zero-padded IDs risk collision if Xcode or a future contributor auto-generates a GUID in the same range. Recommend letting Xcode add these entries via "Add Files to target" to get proper random UUIDs.
| .listRowSeparator(.hidden) | ||
|
|
||
| if let hostError { | ||
| Text(hostError) |
There was a problem hiding this comment.
nit: the spacing feels off to me (but I'm not UX :P), wondering if this could/should go into the same VStack that NewLoginHostField creates so that the error message seems more attached to the host field than in the middle of the two
| if (failing && failing.isDeletable) { | ||
| for (NSUInteger i = 0; i < [storage numberOfLoginHosts]; i++) { | ||
| if ([[storage loginHostAtIndex:i].host isEqualToString:failingHost]) { | ||
| [storage removeLoginHostAtIndex:i]; |
There was a problem hiding this comment.
Is there any other info we could use to determine if deleting is the right path for the scenario? I'm worried it might be too broad for things like a time out or a connectivity issue vs having a bad URL
There was a problem hiding this comment.
You're right. The previous gate (isDeletable) was too broad. The dispatch hook upstream (SFSDKAuthErrorManager.m:141) routes anything with _kCFStreamError* userInfo into this handler, so transient codes like timeout (-1001), cannot-connect (-1004), and not-connected-to-internet (-1009) were also triggering removal. A user on flaky Wi-Fi could lose their legit custom host.
Tightened the auto-remove to fire only on strong "host is unusable" signals:
- kSFOAuthErrorDomain / kSFOAuthErrorInvalidURL
- NSURLErrorBadURL (-1000)
- NSURLErrorUnsupportedURL (-1002)
- NSURLErrorCannotFindHost (-1003, DNS NXDOMAIN)
- NSURLErrorDNSLookupFailed (-1006)
- NSURLErrorAppTransportSecurityRequiresSecureConnection (-1022)
All other codes that reach this handler now leave the host in storage but still recover (revert to previous host or fall back to index 0). Added a test (test_givenDeletableFailingHostAndAmbiguousSignal...) that fires a timeout and asserts the host stays.
SwiftUI (NewLoginHostView.swift): - Use dynamic systemRed for the error text (adapts to light/dark mode). - Move the error message into the host field's VStack so it sits attached to the host input instead of as its own List row between fields. - Clear the error as soon as the user edits the host so stale text does not linger after they correct their input. Auto-delete gate (SFUserAccountManager.m): - Restrict the host-removal branch in hostConnectionErrorHandlerBlock to errors that strongly indicate the host itself is unusable: OAuth invalid URL, bad URL, unsupported URL, cannot-find-host (DNS NXDOMAIN), DNS lookup failed, and ATS-requires-secure-connection. - Ambiguous transport codes (timeout, cannot-connect, not-connected-to- internet, connection-lost, roaming-off, data-not-allowed) no longer auto-remove a deletable host; recovery to the previous host still runs. - Added a recovery-test case asserting that an ambiguous timeout keeps a deletable host in storage while still restoring the previous host.
| } | ||
| } | ||
| if (failing && failing.isDeletable && strongBadHostSignal) { | ||
| for (NSUInteger i = 0; i < [storage numberOfLoginHosts]; i++) { |
There was a problem hiding this comment.
one more small one here, could you use indexOfLoginHost: instead of the for loop?
3 participants
Summary
Three coordinated fixes in the SalesforceSDKCore iOS login flow:
NewLoginHostView.save(host:hostLabel:)rejects empty/whitespace/syntactically-invalid input (non-empty after trim, contains., no internal whitespace, parses viaURL(string:)) and shows an inline red error below the host field. No persistence, no dismiss, nosaveActioncall on rejection.SFUserAccountManager.hostListViewController:didChangeLoginHost:now records the priorloginHostto a new internal-onlypreviousLoginHostproperty before applying the change.hostConnectionErrorHandlerBlock's OK completion fires, the SDK now: (a) removes the failing host fromSFSDKLoginHostStorageif it isisDeletable == YES(built-in/MDM hosts are never removed), (b) restarts auth onpreviousLoginHostif still in storage, falling back to index-0 otherwise. The fallback is guarded against an empty-storage edge case (MDMonlyShowAuthorizedHosts == YESwith empty MDM list, or the deleted bad host being the only entry) — logs and bails rather than raisingNSRangeException.The alert UX is unchanged: single OK button, same title/message format keys.
Files changed
Classes/Login/LoginHost/NewLoginHostView.swift— validation predicate +hostErrorstate + inline SwiftUITextrendering.Classes/UserAccount/SFUserAccountManager+Internal.h— newpreviousLoginHostinternal property.Classes/UserAccount/SFUserAccountManager.m— snapshot indidChangeLoginHost:; rewrite ofhostConnectionErrorHandlerBlockOK completion.SalesforceSDKCoreTests/NewLoginHostTests.swift— 6 new test cases (empty, whitespace, no-dot, internal whitespace, invalid scheme, my-domain accept).SalesforceSDKCoreTests/SFUserAccountManagerLoginHostRecoveryTests.m— new XCTest fixture with 6 handler-recovery scenarios (snapshot, prev-in-storage, prev-nil, prev-missing, deletable-removed, non-deletable-kept).SalesforceSDKCore.xcodeproj/project.pbxproj— 4 entries wiring the new.minto the test target.shared/resources/SalesforceSDKResources.bundle/en.lproj/Localizable.strings— addsLOGIN_INVALID_HOST.Localization
New user-facing string added:
LOGIN_INVALID_HOST="Please enter a valid host (for example, login.salesforce.com)."— translation needed for all non-English locales.Public API
No public API changes.
previousLoginHostlives inSFUserAccountManager+Internal.honly. No public header outside+Internal.hwas touched.Test plan
xcodebuild test -scheme SalesforceSDKCore -only-testing:SalesforceSDKCoreTests/NewLoginHostTests— 10/10 passxcodebuild test -scheme SalesforceSDKCore -only-testing:SalesforceSDKCoreTests/SFUserAccountManagerLoginHostRecoveryTests— 6/6 passfooblocked inline with red error;mobliesdk...salesforce.com(typo) → OAuth fails → tap OK → typo host is gone from picker AND active host is back to the working host.Reviewer attention
Localizable.strings— escalation triggers perCLAUDE.md. Please double-check the recovery branching inhostConnectionErrorHandlerBlock.previousLoginHostis a singleton property. Theoretically races with a host change in a second scene while a first scene's failure alert is open. The alert OK completion runs on the main queue, so the practical risk is low, but flag if multi-window iPad is in scope.user:pass@evil.com/ IDN homographs.