diff --git a/Xcodes/Backend/AppState+Install.swift b/Xcodes/Backend/AppState+Install.swift index 02850d70..7f3dce1c 100644 --- a/Xcodes/Backend/AppState+Install.swift +++ b/Xcodes/Backend/AppState+Install.swift @@ -373,7 +373,9 @@ extension AppState { try await xcodePostInstallWorkflowService.performPostInstallSteps(for: xcode) } catch { Logger.appState.error("Performing post-install steps failed: \(error.legibleLocalizedDescription)") - throw InstallationError.postInstallStepsNotPerformed(version: xcode.version, helperInstallState: helperInstallState) + // Keep the underlying reason (unless the user simply declined), so the alert can say why it failed. + let reason: String? = if case InstallationError.postInstallStepsNotPerformed = error { nil } else { error.legibleLocalizedDescription } + throw InstallationError.postInstallStepsNotPerformed(version: xcode.version, helperInstallState: helperInstallState, reason: reason) } } @@ -411,7 +413,8 @@ extension AppState { helperConsent.resume( throwing: InstallationError.postInstallStepsNotPerformed( version: version, - helperInstallState: self.helperInstallState + helperInstallState: self.helperInstallState, + reason: nil ) ) } @@ -511,7 +514,8 @@ public enum InstallationError: LocalizedError, Equatable { case versionAlreadyInstalled(InstalledXcode) case invalidVersion(String) case versionNotInstalled(Version) - case postInstallStepsNotPerformed(version: Version, helperInstallState: HelperInstallState) + /// `reason` describes the underlying failure, if there was one besides the user declining to install the helper + case postInstallStepsNotPerformed(version: Version, helperInstallState: HelperInstallState, reason: String?) public var errorDescription: String? { switch self { @@ -545,13 +549,16 @@ public enum InstallationError: LocalizedError, Equatable { return String(format: localizeString("InstallationError.InvalidVersion"), version) case let .versionNotInstalled(version): return String(format: localizeString("InstallationError.VersionNotInstalled"), version.appleDescription) - case let .postInstallStepsNotPerformed(version, helperInstallState): + case let .postInstallStepsNotPerformed(version, helperInstallState, reason): + let message: String switch helperInstallState { case .installed: - return String(format: localizeString("InstallationError.PostInstallStepsNotPerformed.Installed"), version.appleDescription) + message = String(format: localizeString("InstallationError.PostInstallStepsNotPerformed.Installed"), version.appleDescription) case .notInstalled, .unknown: - return String(format: localizeString("InstallationError.PostInstallStepsNotPerformed.NotInstalled"), version.appleDescription) + message = String(format: localizeString("InstallationError.PostInstallStepsNotPerformed.NotInstalled"), version.appleDescription) } + guard let reason else { return message } + return "\(message)\n\n\(reason)" } } } diff --git a/Xcodes/Backend/AppState.swift b/Xcodes/Backend/AppState.swift index 6ac509bb..6d0e4d50 100644 --- a/Xcodes/Backend/AppState.swift +++ b/Xcodes/Backend/AppState.swift @@ -92,6 +92,8 @@ class AppState: ObservableObject { @Published var presentedAlert: XcodesAlert? @Published var presentedPreferenceAlert: XcodesPreferencesAlert? @Published var helperInstallState: HelperInstallState = .notInstalled + /// Delay between attempts to reach a newly installed helper + var helperInstallRetryDelay: Duration = .milliseconds(500) /// Whether the user is being prepared for the helper installation alert with an explanation. /// This closure will be performed after the user chooses whether or not to proceed. @Published var isPreparingUserForActionRequiringHelper: ((Bool) -> Void)? @@ -546,15 +548,37 @@ class AppState: ObservableObject { try Task.checkCancellation() try await Current.helper.install() try Task.checkCancellation() - await checkIfHelperIsInstalled() + // launchd starts a newly blessed helper asynchronously, so give it a moment to answer. + let connectionError = await checkIfHelperIsInstalled(attempts: 5) + try Task.checkCancellation() + guard helperInstallState == .installed else { + throw HelperClientError.unreachableAfterInstall(underlyingError: connectionError) + } } } - private func checkIfHelperIsInstalled() async { + /// Asks the helper for its version, retrying up to `attempts` times. + /// - Returns: The last connection error, if the helper couldn't be reached. + @discardableResult + private func checkIfHelperIsInstalled(attempts: Int = 1) async -> Error? { helperInstallState = .unknown - let installed = (try? await Current.helper.checkIfLatestHelperIsInstalledAsync()) ?? false - helperInstallState = installed ? .installed : .notInstalled + var lastError: Error? + for attempt in 1...max(1, attempts) { + do { + if try await Current.helper.checkIfLatestHelperIsInstalledAsync() { + helperInstallState = .installed + return nil + } + lastError = nil + } catch { + lastError = error + } + guard attempt < attempts, !Task.isCancelled else { break } + try? await Task.sleep(for: helperInstallRetryDelay) + } + helperInstallState = .notInstalled + return lastError } @discardableResult diff --git a/Xcodes/Backend/HelperClient.swift b/Xcodes/Backend/HelperClient.swift index 1fb73d8b..a9efaa53 100644 --- a/Xcodes/Backend/HelperClient.swift +++ b/Xcodes/Backend/HelperClient.swift @@ -1,4 +1,5 @@ import Foundation +import LegibleError import os.log import ServiceManagement import XcodesKit @@ -235,7 +236,20 @@ final class HelperClient { let authRef = try authorizationRef(&authRights, nil, [.interactionAllowed, .extendRights, .preAuthorize]) var cfError: Unmanaged? SMJobBless(kSMDomainSystemLaunchd, machServiceName as CFString, authRef, &cfError) - if let error = cfError?.takeRetainedValue() { throw error } + if let error = cfError?.takeRetainedValue() { + // kSMErrorDomainLaunchd is deprecated, but SMJobBless still reports its errors in that domain + if CFErrorGetDomain(error) as String == "CFErrorDomainLaunchd" { + switch CFErrorGetCode(error) { + case kSMErrorInvalidSignature: + throw HelperClientError.invalidSignature(underlyingError: error) + case kSMErrorAuthorizationFailure: + throw HelperClientError.authorizationFailed(underlyingError: error) + default: + break + } + } + throw error + } self.connection?.invalidate() self.connection = nil @@ -271,6 +285,12 @@ final class HelperClient { enum HelperClientError: LocalizedError { case failedToCreateRemoteObjectProxy case message(String) + /// SMJobBless rejected the helper because its signature doesn't satisfy the app's SMPrivilegedExecutables requirement + case invalidSignature(underlyingError: Error) + /// SMJobBless wasn't authorized, e.g. the administrator prompt was cancelled or couldn't be shown + case authorizationFailed(underlyingError: Error) + /// The helper was blessed but doesn't answer, e.g. it rejects this app's signature via SMAuthorizedClients + case unreachableAfterInstall(underlyingError: Error?) var errorDescription: String? { switch self { @@ -278,6 +298,17 @@ enum HelperClientError: LocalizedError { return localizeString("HelperClient.error") case let .message(message): return message + case let .invalidSignature(underlyingError): + return Self.withDetails(localizeString("HelperClient.error.InvalidSignature"), underlyingError) + case let .authorizationFailed(underlyingError): + return Self.withDetails(localizeString("HelperClient.error.AuthorizationFailed"), underlyingError) + case let .unreachableAfterInstall(underlyingError): + return Self.withDetails(localizeString("HelperClient.error.UnreachableAfterInstall"), underlyingError) } } + + private static func withDetails(_ message: String, _ underlyingError: Error?) -> String { + guard let underlyingError else { return message } + return "\(message)\n\n\(underlyingError.legibleLocalizedDescription)" + } } diff --git a/Xcodes/Resources/Localizable.xcstrings b/Xcodes/Resources/Localizable.xcstrings index 6608a149..820bc2b9 100644 --- a/Xcodes/Resources/Localizable.xcstrings +++ b/Xcodes/Resources/Localizable.xcstrings @@ -11157,6 +11157,39 @@ } } }, + "HelperClient.error.AuthorizationFailed" : { + "extractionState" : "manual", + "localizations" : { + "en" : { + "stringUnit" : { + "state" : "translated", + "value" : "macOS didn't authorize installing the privileged helper. Try again and enter an administrator's name and password when asked; the helper can also be installed from Settings > Advanced." + } + } + } + }, + "HelperClient.error.InvalidSignature" : { + "extractionState" : "manual", + "localizations" : { + "en" : { + "stringUnit" : { + "state" : "translated", + "value" : "macOS refused to install the privileged helper because its code signature doesn't match what Xcodes expects. The app and helper must be signed by the same developer team." + } + } + } + }, + "HelperClient.error.UnreachableAfterInstall" : { + "extractionState" : "manual", + "localizations" : { + "en" : { + "stringUnit" : { + "state" : "translated", + "value" : "The privileged helper was installed, but Xcodes couldn't connect to it. This usually means the helper and Xcodes are signed by different developer teams, for example a local development build running alongside a helper installed by the released app." + } + } + } + }, "HelperInstalled" : { "localizations" : { "ar" : { diff --git a/XcodesTests/AppStateTests.swift b/XcodesTests/AppStateTests.swift index 2945b06c..271d0c03 100644 --- a/XcodesTests/AppStateTests.swift +++ b/XcodesTests/AppStateTests.swift @@ -81,6 +81,49 @@ class AppStateTests: XCTestCase { subject = AppState() } + func test_InstallHelper_RetriesUntilNewlyInstalledHelperAnswers() async throws { + subject.helperInstallState = .notInstalled + subject.helperInstallRetryDelay = .zero + let checks = AppStateTestsCounter() + Current.helper.install = { } + Current.helper.checkIfLatestHelperIsInstalledAsync = { + // launchd hasn't started the helper for the first couple of checks + checks.increment() >= 3 + } + + try await subject.installHelperIfNecessaryAsync() + + XCTAssertEqual(subject.helperInstallState, .installed) + XCTAssertEqual(checks.value, 3) + } + + func test_InstallHelper_ThrowsWithReasonWhenInstalledHelperIsUnreachable() async throws { + subject.helperInstallState = .notInstalled + subject.helperInstallRetryDelay = .zero + Current.helper.install = { } + Current.helper.checkIfLatestHelperIsInstalledAsync = { + throw NSError(domain: NSCocoaErrorDomain, code: NSXPCConnectionInvalid) + } + + do { + try await subject.installHelperIfNecessaryAsync() + XCTFail("Expected an unreachable helper to throw") + } catch let HelperClientError.unreachableAfterInstall(underlyingError) { + XCTAssertEqual((underlyingError as NSError?)?.code, NSXPCConnectionInvalid) + } + XCTAssertEqual(subject.helperInstallState, .notInstalled) + } + + func test_PostInstallStepsError_IncludesUnderlyingReason() { + let error = InstallationError.postInstallStepsNotPerformed( + version: Version("27.2.0")!, + helperInstallState: .notInstalled, + reason: HelperClientError.unreachableAfterInstall(underlyingError: nil).localizedDescription + ) + + XCTAssertTrue(error.errorDescription?.contains(localizeString("HelperClient.error.UnreachableAfterInstall")) == true) + } + func test_InstallError_Network401IsUnauthorized() { let error = NetworkError.non200StatusCode(statusCode: 401, data: Data()) @@ -1046,3 +1089,19 @@ private extension HTTPCookie { ])) } } + +private final class AppStateTestsCounter: @unchecked Sendable { + private let lock = NSLock() + private var count = 0 + + var value: Int { + lock.withLock { count } + } + + func increment() -> Int { + lock.withLock { + count += 1 + return count + } + } +}