From cd63f67b3e7083782eaa257af049a6ea4610d0bc Mon Sep 17 00:00:00 2001 From: Harry <109690906+harry-signal@users.noreply.github.com> Date: Fri, 17 Feb 2023 14:50:54 -0800 Subject: [PATCH] Retry most network failures in RegistrationCoordinator * Retry most network failures in RegistrationCoordinator * Update the way we identify network errors to be more flexible * Retry remaining requests * Automatically retry registration requests if time intervals are short * fix error types not compiling on kbs * Add test for reg recovery password reg with retries * fix extra t --- .../RegistrationCoodinatorShims.swift | 4 + .../RegistrationCoordinatorImpl+Service.swift | 33 +- .../RegistrationCoordinatorImpl.swift | 302 +++++++++++++++--- .../RegistrationCoordinatorTest.swift | 131 ++++++++ .../KeyBackupService/KeyBackupService.swift | 17 +- .../KeyBackupServiceProtocol.swift | 5 +- .../TSRequestOWSURLSessionMock.swift | 43 ++- .../RegistrationSessionManagerImpl.swift | 6 +- 8 files changed, 473 insertions(+), 68 deletions(-) diff --git a/Signal/Registration/RegistrationCoodinatorShims.swift b/Signal/Registration/RegistrationCoodinatorShims.swift index 4823a3a9b7..b88b801f67 100644 --- a/Signal/Registration/RegistrationCoodinatorShims.swift +++ b/Signal/Registration/RegistrationCoodinatorShims.swift @@ -191,6 +191,7 @@ extension Registration { public enum SyncPushTokensResult { case success case pushUnsupported(description: String) + case networkError case genericError } } @@ -238,6 +239,9 @@ public class _RegistrationCoordinator_PushRegistrationManagerWrapper: _Registrat return job.run() .map(on: SyncScheduler()) { return .success } .recover(on: SyncScheduler()) { error -> Guarantee in + if error.isNetworkConnectivityFailure { + return .value(.networkError) + } switch error { case PushRegistrationError.pushNotSupported(let description): return .value(.pushUnsupported(description: description)) diff --git a/Signal/Registration/RegistrationCoordinatorImpl+Service.swift b/Signal/Registration/RegistrationCoordinatorImpl+Service.swift index a88dc97ba5..6e2b6f28d4 100644 --- a/Signal/Registration/RegistrationCoordinatorImpl+Service.swift +++ b/Signal/Registration/RegistrationCoordinatorImpl+Service.swift @@ -258,10 +258,22 @@ extension RegistrationCoordinatorImpl { public static func makeEnableReglockRequest( reglockToken: String, signalService: OWSSignalServiceProtocol, - schedulers: Schedulers + schedulers: Schedulers, + retriesLeft: Int = RegistrationCoordinatorImpl.Constants.networkErrorRetries ) -> Promise { let request = OWSRequestFactory.enableRegistrationLockV2Request(token: reglockToken) return signalService.urlSessionForMainSignalService().promiseForTSRequest(request).asVoid() + .recover(on: schedulers.sync) { error in + if error.isNetworkConnectivityFailure, retriesLeft > 0 { + return makeEnableReglockRequest( + reglockToken: reglockToken, + signalService: signalService, + schedulers: schedulers, + retriesLeft: retriesLeft - 1 + ) + } + return .init(error: error) + } } /// Returns nil error if success. @@ -270,7 +282,8 @@ extension RegistrationCoordinatorImpl { authUsername: String, authPassword: String, signalService: OWSSignalServiceProtocol, - schedulers: Schedulers + schedulers: Schedulers, + retriesLeft: Int = RegistrationCoordinatorImpl.Constants.networkErrorRetries ) -> Guarantee { let request = RegistrationRequestFactory.updatePrimaryDeviceAccountAttributesRequest( attributes, @@ -286,6 +299,16 @@ extension RegistrationCoordinatorImpl { return nil } .recover(on: schedulers.sync) { error in + if error.isNetworkConnectivityFailure, retriesLeft > 0 { + return makeUpdateAccountAttributesRequest( + attributes, + authUsername: authUsername, + authPassword: authPassword, + signalService: signalService, + schedulers: schedulers, + retriesLeft: retriesLeft - 1 + ) + } return .value(error) } } @@ -307,12 +330,12 @@ extension RegistrationCoordinatorImpl { ) } .recover(on: schedulers.sharedBackground) { (error: Error) -> Guarantee in + if error.isNetworkConnectivityFailure { + return .value(networkFailureError) + } guard let error = error as? OWSHTTPError else { return .value(fallbackError) } - if case .networkFailure = error { - return .value(networkFailureError) - } let response = handler( error.responseStatusCode, error.responseHeaders?.value(forHeader: Constants.retryAfterHeader), diff --git a/Signal/Registration/RegistrationCoordinatorImpl.swift b/Signal/Registration/RegistrationCoordinatorImpl.swift index 0dcce645b2..e4fddab6f3 100644 --- a/Signal/Registration/RegistrationCoordinatorImpl.swift +++ b/Signal/Registration/RegistrationCoordinatorImpl.swift @@ -512,6 +512,13 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { } // Update the account attributes once, now, at the end. + return updateAccountAttributesAndFinish(accountIdentity: accountIdentity) + } + + private func updateAccountAttributesAndFinish( + accountIdentity: AccountIdentity, + retriesLeft: Int = Constants.networkErrorRetries + ) -> Guarantee { return Service .makeUpdateAccountAttributesRequest( makeAccountAttributes(authToken: accountIdentity.authToken), @@ -534,7 +541,13 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { self.storageServiceManager.backupPendingChanges() return .value(.done) } - // TODO[Registration]: handle account attributes update error? + if error.isNetworkConnectivityFailure, retriesLeft > 0 { + return self.updateAccountAttributesAndFinish( + accountIdentity: accountIdentity, + retriesLeft: retriesLeft - 1 + ) + } + // TODO[Registration]: what should we do with a non-transiest account attributes update error? Logger.error("Failed to register due to failed account attributes update: \(error)") return .value(.showGenericError) } @@ -689,7 +702,11 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { ) } - private func registerForRegRecoveryPwPath(regRecoveryPw: String, e164: String) -> Guarantee { + private func registerForRegRecoveryPwPath( + regRecoveryPw: String, + e164: String, + retriesLeft: Int = Constants.networkErrorRetries + ) -> Guarantee { if inMemoryState.pinFromUser == nil { // We need the user to confirm their pin. // TODO[Registration]: set state that tells the pin entry controller @@ -703,14 +720,18 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { ).then(on: schedulers.main) { [weak self] accountResponse in return self?.handleCreateAccountResponseFromRegRecoveryPassword( accountResponse, - e164: e164 + regRecoveryPw: regRecoveryPw, + e164: e164, + retriesLeft: retriesLeft ) ?? .value(.showGenericError) } } private func handleCreateAccountResponseFromRegRecoveryPassword( _ response: AccountResponse, - e164: String + regRecoveryPw: String, + e164: String, + retriesLeft: Int ) -> Guarantee { // TODO[Registration] handle error case for rejected e164. switch response { @@ -764,8 +785,20 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { // Now we have to start a session; its the only way to recover. return self.startSession(e164: e164) - case .retryAfter: - // TODO[Registration] handle retries, possibly automatically if short enough. + case .retryAfter(let timeInterval): + if timeInterval < Constants.autoRetryInterval { + return Guarantee + .after(on: self.schedulers.sharedBackground, seconds: timeInterval) + .then(on: self.schedulers.sync) { [weak self] in + guard let self else { + return .value(.showGenericError) + } + return self.registerForRegRecoveryPwPath( + regRecoveryPw: regRecoveryPw, + e164: e164 + ) + } + } return .value(.showGenericError) case .deviceTransferPossible: @@ -774,7 +807,13 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { return nextStep() case .networkError: - // TODO[Registration]: handle retries + if retriesLeft > 0 { + return registerForRegRecoveryPwPath( + regRecoveryPw: regRecoveryPw, + e164: e164, + retriesLeft: retriesLeft - 1 + ) + } return .value(.showGenericError) case .genericError: @@ -824,7 +863,8 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { private func restoreKBSMasterSecretForAuthCredentialPath( pin: String, - credential: KBSAuthCredential + credential: KBSAuthCredential, + retriesLeft: Int = Constants.networkErrorRetries ) -> Guarantee { kbs.restoreKeysAndBackup(pin: pin, authMethod: .kbsAuth(credential, backup: nil)) .then(on: schedulers.main) { [weak self] result -> Guarantee in @@ -847,6 +887,15 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { // recover. Give it all up and wipe our KBS info. self.wipeInMemoryStateToPreventKBSPathAttempts() return self.nextStep() + case .networkError: + if retriesLeft > 0 { + return self.restoreKBSMasterSecretForAuthCredentialPath( + pin: pin, + credential: credential, + retriesLeft: retriesLeft - 1 + ) + } + return .value(.showGenericError) case .genericError: return .value(.showGenericError) } @@ -873,6 +922,17 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { return .value(.phoneNumberEntry) } // Check the candidates. + return makeKBSAuthCredentialCheckRequest( + kbsAuthCredentialCandidates: kbsAuthCredentialCandidates, + e164: e164 + ) + } + + private func makeKBSAuthCredentialCheckRequest( + kbsAuthCredentialCandidates: [KBSAuthCredential], + e164: String, + retriesLeft: Int = Constants.networkErrorRetries + ) -> Guarantee { return Service.makeKBSAuthCheckRequest( e164: e164, candidateCredentials: kbsAuthCredentialCandidates, @@ -885,7 +945,8 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { return self.handleKBSAuthCredentialCheckResponse( response, kbsAuthCredentialCandidates: kbsAuthCredentialCandidates, - e164: e164 + e164: e164, + retriesLeft: retriesLeft ) } } @@ -893,13 +954,20 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { private func handleKBSAuthCredentialCheckResponse( _ response: Service.KBSAuthCheckResponse, kbsAuthCredentialCandidates: [KBSAuthCredential], - e164: String + e164: String, + retriesLeft: Int ) -> Guarantee { var matchedCredential: KBSAuthCredential? var credentialsToDelete = [KBSAuthCredential]() switch response { case .networkError: - // TODO[Registration]: handle retries + if retriesLeft > 0 { + return makeKBSAuthCredentialCheckRequest( + kbsAuthCredentialCandidates: kbsAuthCredentialCandidates, + e164: e164, + retriesLeft: retriesLeft - 1 + ) + } self.inMemoryState.kbsAuthCredentialCandidates = nil return self.nextStep() case .genericError: @@ -937,15 +1005,7 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { private func nextStepForSessionPath(_ session: RegistrationSession) -> Guarantee { if session.verified { // We have to complete registration. - return self.makeRegisterOrChangeNumberRequest( - .sessionId(session.id), - e164: session.e164 - ).then(on: schedulers.main) { [weak self] response in - guard let self = self else { - return .value(.showGenericError) - } - return self.handleCreateAccountResponseFromSession(response) - } + return self.makeRegisterOrChangeNumberRequestFromSession(session) } if inMemoryState.needsToAskForDeviceTransfer { @@ -1047,8 +1107,29 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { self.sessionManager.completeSession() } + private func makeRegisterOrChangeNumberRequestFromSession( + _ session: RegistrationSession, + retriesLeft: Int = Constants.networkErrorRetries + ) -> Guarantee { + return self.makeRegisterOrChangeNumberRequest( + .sessionId(session.id), + e164: session.e164 + ).then(on: schedulers.main) { [weak self] response in + guard let self = self else { + return .value(.showGenericError) + } + return self.handleCreateAccountResponseFromSession( + response, + sessionFromBeforeRequest: session, + retriesLeft: retriesLeft + ) + } + } + private func handleCreateAccountResponseFromSession( - _ response: AccountResponse + _ response: AccountResponse, + sessionFromBeforeRequest: RegistrationSession, + retriesLeft: Int ) -> Guarantee { switch response { case .success(let identityResponse): @@ -1081,14 +1162,31 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { resetSession() return nextStep() - case .retryAfter: - // TODO[Registration] handle retries, possibly automatically if short enough. + case .retryAfter(let timeInterval): + if timeInterval < Constants.autoRetryInterval { + return Guarantee + .after(on: schedulers.sharedBackground, seconds: timeInterval) + .then(on: schedulers.sync) { [weak self] in + guard let self else { + return .value(.showGenericError) + } + return self.makeRegisterOrChangeNumberRequestFromSession( + sessionFromBeforeRequest + ) + } + } + // TODO[Registration] bubble up the error to the ui properly. return .value(.showGenericError) case .deviceTransferPossible: inMemoryState.needsToAskForDeviceTransfer = true return .value(.transferSelection) case .networkError: - // TODO[Registration] handle retries + if retriesLeft > 0 { + return makeRegisterOrChangeNumberRequestFromSession( + sessionFromBeforeRequest, + retriesLeft: retriesLeft - 1 + ) + } return .value(.showGenericError) case .genericError: return .value(.showGenericError) @@ -1096,7 +1194,8 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { } private func startSession( - e164: String + e164: String, + retriesLeft: Int = Constants.networkErrorRetries ) -> Guarantee { return pushRegistrationManager.requestPushToken() .then(on: schedulers.sharedBackground) { [weak self] apnsToken -> Guarantee in @@ -1119,11 +1218,27 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { case .invalidArgument: // TODO[Registration] populate error state for phone number entry. return .value(.phoneNumberEntry) - case .retryAfter: - // TODO[Registration] handle retries, possibly automatically if short enough. + case .retryAfter(let timeInterval): + if timeInterval < Constants.autoRetryInterval { + return Guarantee + .after(on: strongSelf.schedulers.sharedBackground, seconds: timeInterval) + .then(on: strongSelf.schedulers.sync) { [weak self] in + guard let self else { + return .value(.showGenericError) + } + return self.startSession( + e164: e164 + ) + } + } return .value(.phoneNumberEntry) case .networkFailure: - // TODO[Registration] handle retries + if retriesLeft > 0 { + return strongSelf.startSession( + e164: e164, + retriesLeft: retriesLeft - 1 + ) + } return .value(.showGenericError) case .genericError: return .value(.showGenericError) @@ -1134,7 +1249,8 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { private func requestSessionCode( session: RegistrationSession, - transport: Registration.CodeTransport + transport: Registration.CodeTransport, + retriesLeft: Int = Constants.networkErrorRetries ) -> Guarantee { return sessionManager.requestVerificationCode( for: session, @@ -1174,14 +1290,39 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { return .value(.showGenericError) } case .retryAfterTimeout(let session): - // TODO[Registration]: just auto retry if short enough? - // Clear the pending code; we want the user to press again - // once the timeout expires. - self.inMemoryState.pendingCodeTransport = nil self.processSession(session) - return self.nextStep() + + let timeInterval: TimeInterval? + switch transport { + case .sms: + timeInterval = session.nextSMS + case .voice: + timeInterval = session.nextCall + } + if let timeInterval, timeInterval < Constants.autoRetryInterval { + return Guarantee + .after(on: self.schedulers.sharedBackground, seconds: timeInterval) + .then(on: self.schedulers.sync) { [weak self] in + guard let self else { + return .value(.showGenericError) + } + return self.requestSessionCode( + session: session, + transport: transport + ) + } + } else { + self.inMemoryState.pendingCodeTransport = nil + return self.nextStep() + } case .networkFailure: - // TODO[Registration] handle retries + if retriesLeft > 0 { + return self.requestSessionCode( + session: session, + transport: transport, + retriesLeft: retriesLeft - 1 + ) + } return .value(.showGenericError) case .genericError: self.inMemoryState.pendingCodeTransport = nil @@ -1192,7 +1333,8 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { private func submitCaptchaChallengeFulfillment( session: RegistrationSession, - token: String + token: String, + retriesLeft: Int = Constants.networkErrorRetries ) -> Guarantee { return sessionManager.fulfillChallenge( for: session, @@ -1227,14 +1369,20 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { return .value(.showGenericError) } case .retryAfterTimeout(let session): - // TODO[Registration]: just auto retry if short enough? + Logger.error("Should not have to retry a captcha challenge request") // Clear the pending code; we want the user to press again // once the timeout expires. self.inMemoryState.pendingCodeTransport = nil self.processSession(session) return self.nextStep() case .networkFailure: - // TODO[Registration] handle retries + if retriesLeft > 0 { + return self.submitCaptchaChallengeFulfillment( + session: session, + token: token, + retriesLeft: retriesLeft - 1 + ) + } return .value(.showGenericError) case .genericError: return .value(.showGenericError) @@ -1244,7 +1392,8 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { private func submitSessionCode( session: RegistrationSession, - code: String + code: String, + retriesLeft: Int = Constants.networkErrorRetries ) -> Guarantee { return sessionManager.submitVerificationCode( for: session, @@ -1283,11 +1432,30 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { return .value(.showGenericError) } case .retryAfterTimeout(let session): - // TODO[Registration]: need to wait out timeout; show error, or retry if short enough. self.processSession(session) + if let timeInterval = session.nextVerificationAttempt, timeInterval < Constants.autoRetryInterval { + return Guarantee + .after(on: self.schedulers.sharedBackground, seconds: timeInterval) + .then(on: self.schedulers.sync) { [weak self] in + guard let self else { + return .value(.showGenericError) + } + return self.submitSessionCode( + session: session, + code: code + ) + } + } + // TODO[Registration]: show some kind of error. return self.nextStep() case .networkFailure: - // TODO[Registration] handle retries + if retriesLeft > 0 { + return self.submitSessionCode( + session: session, + code: code, + retriesLeft: retriesLeft - 1 + ) + } return .value(.showGenericError) case .genericError: return .value(.showGenericError) @@ -1359,7 +1527,10 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { return exportAndWipeState(accountIdentity: accountIdentity) } - private func syncPushTokens(_ accountIdentity: AccountIdentity) -> Guarantee { + private func syncPushTokens( + _ accountIdentity: AccountIdentity, + retriesLeft: Int = Constants.networkErrorRetries + ) -> Guarantee { pushRegistrationManager .syncPushTokensForcingUpload( authUsername: accountIdentity.authUsername, @@ -1391,13 +1562,25 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { } } return self.nextStep() + case .networkError: + if retriesLeft > 0 { + return self.syncPushTokens( + accountIdentity, + retriesLeft: retriesLeft - 1 + ) + } + return .value(.showGenericError) case .genericError: return .value(.showGenericError) } } } - private func restoreKBSBackupPostRegistration(pin: String, accountIdentity: AccountIdentity) -> Guarantee { + private func restoreKBSBackupPostRegistration( + pin: String, + accountIdentity: AccountIdentity, + retriesLeft: Int = Constants.networkErrorRetries + ) -> Guarantee { let backupAuthMethod = KBS.AuthMethod.chatServerAuth(username: accountIdentity.authUsername, password: accountIdentity.authPassword) let authMethod: KBS.AuthMethod if let kbsAuthCredential = inMemoryState.kbsAuthCredentialForPostRegRestoration { @@ -1428,13 +1611,26 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { // recover. Keep going like if nothing happened. self.inMemoryState.shouldRestoreKBSMasterKeyAfterRegistration = false return self.nextStep() + case .networkError: + if retriesLeft > 0 { + return self.restoreKBSBackupPostRegistration( + pin: pin, + accountIdentity: accountIdentity, + retriesLeft: retriesLeft - 1 + ) + } + return .value(.showGenericError) case .genericError: return .value(.showGenericError) } } } - private func backupToKBS(pin: String, accountIdentity: AccountIdentity) -> Guarantee { + private func backupToKBS( + pin: String, + accountIdentity: AccountIdentity, + retriesLeft: Int = Constants.networkErrorRetries + ) -> Guarantee { let authMethod: KBS.AuthMethod let backupAuthMethod = KBS.AuthMethod.chatServerAuth( username: accountIdentity.authUsername, @@ -1473,8 +1669,14 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { guard let self else { return .value(.showGenericError) } - // TODO[Registration]: retry network failures. - if case OWSHTTPError.networkFailure = error { + if error.isNetworkConnectivityFailure { + if retriesLeft > 0 { + return self.backupToKBS( + pin: pin, + accountIdentity: accountIdentity, + retriesLeft: retriesLeft - 1 + ) + } return .value(.showGenericError) } Logger.error("Failed to back up to KBS with error: \(error)") @@ -1658,5 +1860,13 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { enum Constants { static let persistedStateKey = "state" + + // how many times we will retry network errors. + static let networkErrorRetries = 1 + + // If a request that can be retried has a timeout below this + // threshold, we will auto-retry it. + // (e.g. you try sending an sms code and the nextSMS is less than this.) + static let autoRetryInterval: TimeInterval = 0.5 } } diff --git a/Signal/test/Registration/RegistrationCoordinatorTest.swift b/Signal/test/Registration/RegistrationCoordinatorTest.swift index cc90cf287e..2b04ad2a44 100644 --- a/Signal/test/Registration/RegistrationCoordinatorTest.swift +++ b/Signal/test/Registration/RegistrationCoordinatorTest.swift @@ -492,6 +492,137 @@ public class RegistrationCoordinatorTest: XCTestCase { XCTAssertFalse(kbs.hasMasterKey) } + func testRegRecoveryPwPath_retryNetworkError() throws { + // Set profile info so we skip those steps. + setupDefaultAccountAttributes() + + // Set a PIN on disk. + ows2FAManagerMock.pinCodeMock = { Stubs.pinCode } + + // Make KBS give us back a reg recovery password. + kbs.dataGenerator = { + switch $0 { + case .registrationRecoveryPassword: + return Stubs.regRecoveryPwData + case .registrationLock: + return Stubs.reglockData + default: + return nil + } + } + kbs.hasMasterKey = true + + // Run the scheduler for a bit; we don't care about timing these bits. + scheduler.start() + + // We haven't set a phone number so it should ask for that. + XCTAssertEqual(coordinator.nextStep().value, .phoneNumberEntry) + + // Give it a phone number, which should show the PIN entry step. + var nextStep = coordinator.submitE164(Stubs.e164) + // Now it should ask for the PIN to confirm the user knows it. + XCTAssertEqual(nextStep.value, .pinEntry) + + // Now we want to control timing so we can verify things happened in the right order. + scheduler.stop() + scheduler.adjustTime(to: 0) + + // Give it the pin code, which should make it try and register. + nextStep = coordinator.submitPINCode(Stubs.pinCode) + + let expectedRecoveryPwRequest = RegistrationRequestFactory.createAccountRequest( + verificationMethod: .recoveryPassword(Stubs.regRecoveryPw), + e164: Stubs.e164, + accountAttributes: Stubs.accountAttributes(), + skipDeviceTransfer: true + ) + + // Fail the request at t=2 with a network error. + let failResponse = TSRequestOWSURLSessionMock.Response.networkError(url: expectedRecoveryPwRequest.url!) + mockURLSession.addResponse(failResponse, atTime: 2, on: scheduler) + + let identityResponse = Stubs.accountIdentityResponse() + let authUsername = identityResponse.aci.uuidString + var authPassword: String! + + // Once the first request fails, at t=2, it should retry. + scheduler.run(atTime: 1) { + // Resolve with success at t=3 + let expectedRequest = RegistrationRequestFactory.createAccountRequest( + verificationMethod: .recoveryPassword(Stubs.regRecoveryPw), + e164: Stubs.e164, + accountAttributes: Stubs.accountAttributes(), + skipDeviceTransfer: true + ) + + self.mockURLSession.addResponse( + TSRequestOWSURLSessionMock.Response( + matcher: { request in + // The password is generated internally by RegistrationCoordinator. + // Extract it so we can check that the same password sent to the server + // to register is used later for other requests. + authPassword = Self.attributesFromCreateAccountRequest(request).authKey + return request.url == expectedRequest.url + }, + statusCode: 200, + bodyData: try! JSONEncoder().encode(identityResponse) + ), + atTime: 3, + on: self.scheduler + ) + } + + // When registered at t=3, it should try and sync push tokens. Succeed at t=4 + pushRegistrationManagerMock.syncPushTokensForcingUploadMock = { username, password in + XCTAssertEqual(self.scheduler.currentTime, 3) + XCTAssertEqual(username, authUsername) + XCTAssertEqual(password, authPassword) + return self.scheduler.guarantee(resolvingWith: .success, atTime: 4) + } + + // We haven't done a kbs backup; that should happen at t=4. Succeed at t=5. + kbs.generateAndBackupKeysMock = { pin, authMethod, rotateMasterKey in + XCTAssertEqual(self.scheduler.currentTime, 4) + XCTAssertEqual(pin, Stubs.pinCode) + // We don't have a kbs auth credential, it should use chat server creds. + XCTAssertEqual(authMethod, .chatServerAuth(username: authUsername, password: authPassword)) + XCTAssertFalse(rotateMasterKey) + self.kbs.hasMasterKey = true + return self.scheduler.promise(resolvingWith: (), atTime: 5) + } + + // Once we sync push tokens at t=5, we should restore from storage service. + // Succeed at t=6. + accountManagerMock.performInitialStorageServiceRestoreMock = { + XCTAssertEqual(self.scheduler.currentTime, 5) + return self.scheduler.promise(resolvingWith: (), atTime: 6) + } + + // Once we do the storage service restore at t=6, + // we will sync account attributes and then we are finished! + let expectedAttributesRequest = RegistrationRequestFactory.updatePrimaryDeviceAccountAttributesRequest( + Stubs.accountAttributes(), + authUsername: "", // doesn't matter for url matching + authPassword: "" // doesn't matter for url matching + ) + self.mockURLSession.addResponse( + TSRequestOWSURLSessionMock.Response( + matcher: { request in + return request.url == expectedAttributesRequest.url + }, + statusCode: 200, + bodyData: nil + ), + atTime: 7, + on: scheduler + ) + + scheduler.runUntilIdle() + XCTAssertEqual(scheduler.currentTime, 7) + + XCTAssertEqual(nextStep.value, .done) + } + // MARK: - KBS Auth Credential Path func testKBSAuthCredentialPath_happyPath() { diff --git a/SignalServiceKit/KeyBackupService/KeyBackupService.swift b/SignalServiceKit/KeyBackupService/KeyBackupService.swift index 55befd9d2a..45074a7a6a 100644 --- a/SignalServiceKit/KeyBackupService/KeyBackupService.swift +++ b/SignalServiceKit/KeyBackupService/KeyBackupService.swift @@ -175,8 +175,10 @@ public class KeyBackupService: KeyBackupServiceProtocol { throw KBS.KBSError.invalidPin(triesRemaining: UInt32(remainingAttempts)) case .backupMissing: throw KBS.KBSError.backupMissing - case .genericError: - throw KBS.KBSError.assertion + case .networkError(let error): + throw error + case .genericError(let error): + throw error } } } @@ -198,7 +200,7 @@ public class KeyBackupService: KeyBackupServiceProtocol { ) -> Guarantee { guard let enclave = enclavesToCheck.first else { owsFailDebug("Unexpectedly tried to restore keys with no specified enclaves") - return .value(.genericError) + return .value(.genericError(KBS.KBSError.assertion)) } return restoreKeysAndBackup( pin: pin, @@ -206,7 +208,7 @@ public class KeyBackupService: KeyBackupServiceProtocol { enclave: enclave ).then(on: schedulers.sync) { result -> Guarantee in switch result { - case .success, .invalidPin, .genericError: + case .success, .invalidPin, .networkError, .genericError: return .value(result) case .backupMissing: if enclavesToCheck.count > 1 { @@ -304,14 +306,17 @@ public class KeyBackupService: KeyBackupServiceProtocol { return .success } .recover(on: schedulers.global()) { error -> Guarantee in + if error.isNetworkConnectivityFailure { + return .value(.networkError(error)) + } guard let kbsError = error as? KBS.KBSError else { owsFailDebug("Unexpectedly surfacing a non KBS error \(error)") - return .value(.genericError) + return .value(.genericError(error)) } switch kbsError { case .assertion: - return .value(.genericError) + return .value(.genericError(error)) case .invalidPin(let remainingAttempts): return .value(.invalidPin(remainingAttempts: Int(remainingAttempts))) case .backupMissing: diff --git a/SignalServiceKit/KeyBackupService/KeyBackupServiceProtocol.swift b/SignalServiceKit/KeyBackupService/KeyBackupServiceProtocol.swift index fd32e2cb75..6fb8b00e88 100644 --- a/SignalServiceKit/KeyBackupService/KeyBackupServiceProtocol.swift +++ b/SignalServiceKit/KeyBackupService/KeyBackupServiceProtocol.swift @@ -88,8 +88,9 @@ public enum KBS { // This could mean there was never a backup, or it's been // deleted due to using up all pin attempts. case backupMissing - // Network or other issue. - case genericError + case networkError(Error) + // Some other issue. + case genericError(Error) } } diff --git a/SignalServiceKit/Mocks/OWSSignalService/TSRequestOWSURLSessionMock.swift b/SignalServiceKit/Mocks/OWSSignalService/TSRequestOWSURLSessionMock.swift index 906c93175b..f718f85ae7 100644 --- a/SignalServiceKit/Mocks/OWSSignalService/TSRequestOWSURLSessionMock.swift +++ b/SignalServiceKit/Mocks/OWSSignalService/TSRequestOWSURLSessionMock.swift @@ -20,6 +20,8 @@ public class TSRequestOWSURLSessionMock: BaseOWSURLSessionMock { let headers: OWSHttpHeaders let bodyData: Data? + let error: Error? + public init( matcher: @escaping (TSRequest) -> Bool, statusCode: Int = 200, @@ -30,6 +32,7 @@ public class TSRequestOWSURLSessionMock: BaseOWSURLSessionMock { self.statusCode = statusCode self.headers = headers self.bodyData = bodyData + self.error = nil } public init( @@ -50,6 +53,7 @@ public class TSRequestOWSURLSessionMock: BaseOWSURLSessionMock { self.statusCode = statusCode self.headers = OWSHttpHeaders(httpHeaders: headers, overwriteOnConflict: true) self.bodyData = bodyData + self.error = nil } public init( @@ -65,6 +69,30 @@ public class TSRequestOWSURLSessionMock: BaseOWSURLSessionMock { bodyJson: bodyJson ) } + + private init( + matcher: @escaping (TSRequest) -> Bool, + error: Error + ) { + self.matcher = matcher + self.statusCode = 0 + self.headers = .init() + self.bodyData = nil + self.error = error + } + + public static func networkError( + url: URL + ) -> Self { + Self.init(matcher: { $0.url == url }, error: OWSHTTPError.networkFailure(requestUrl: url)) + } + + public static func networkError( + matcher: @escaping (TSRequest) -> Bool, + url: URL + ) -> Self { + Self.init(matcher: matcher, error: OWSHTTPError.networkFailure(requestUrl: url)) + } } public var responses = [(Response, Guarantee)]() @@ -113,13 +141,16 @@ public class TSRequestOWSURLSessionMock: BaseOWSURLSessionMock { fatalError("Got a request with no response set up!") } let response = responses.remove(at: responseIndex) - return response.1.map(on: SyncScheduler()) { - return HTTPResponseImpl( + return response.1.then(on: SyncScheduler()) { response throws -> Promise in + if let error = response.error { + throw error + } + return .value(HTTPResponseImpl( requestUrl: rawRequest.url!, - status: $0.statusCode, - headers: $0.headers, - bodyData: $0.bodyData - ) + status: response.statusCode, + headers: response.headers, + bodyData: response.bodyData + )) } } } diff --git a/SignalServiceKit/Registration/RegistrationSessionManagerImpl.swift b/SignalServiceKit/Registration/RegistrationSessionManagerImpl.swift index 1304a76dab..07537fe965 100644 --- a/SignalServiceKit/Registration/RegistrationSessionManagerImpl.swift +++ b/SignalServiceKit/Registration/RegistrationSessionManagerImpl.swift @@ -544,12 +544,12 @@ public class RegistrationSessionManagerImpl: RegistrationSessionManager { ) } .recover(on: schedulers.sync) { (error: Error) -> Guarantee in + if error.isNetworkConnectivityFailure { + return .value(networkFailureError) + } guard let error = error as? OWSHTTPError else { return .value(fallbackError) } - if case .networkFailure = error { - return .value(networkFailureError) - } let response = handler( e164, error.responseStatusCode,