Merge branch 'mkirk/cds-feedback'

SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift

@@ -138,7 +138,7 @@ class LegacyContactDiscoveryBatchOperation: OWSOperation {
         let cdsFeedbackOperation = CDSFeedbackOperation(legacyRegisteredRecipientIds: self.registeredRecipientIds)
         cdsFeedbackOperation.addDependency(newCDSBatchOperation)
 
-        CDSFeedbackOperation.operationQueue.addOperations([newCDSBatchOperation, cdsFeedbackOperation], waitUntilFinished: false)
+        CDSBatchOperation.operationQueue.addOperations([newCDSBatchOperation, cdsFeedbackOperation], waitUntilFinished: false)
     }
 
     // Called at most one time.
@@ -190,16 +190,26 @@ class LegacyContactDiscoveryBatchOperation: OWSOperation {
 
 }
 
-public
+enum ContactDiscoveryError: Error {
-class CDSBatchOperation: OWSOperation {
-
-    enum CDSBatchOperationError: Error {
     case parseError(description: String)
     case assertionError(description: String)
+    case attestationError(underlyingError: Error)
+    case clientError(underlyingError: Error)
+    case serverError(underlyingError: Error)
 }
 
+public
+class CDSBatchOperation: OWSOperation {
+
     private let recipientIdsToLookup: [String]
-    var registeredRecipientIds: Set<String>
+    private(set) var registeredRecipientIds: Set<String>
+
+    static let operationQueue: OperationQueue = {
+        let queue = OperationQueue()
+        queue.maxConcurrentOperationCount = 1
+
+        return queue
+    }()
 
     private var networkManager: TSNetworkManager {
         return TSNetworkManager.shared()
@@ -235,7 +245,11 @@ class CDSBatchOperation: OWSOperation {
         contactDiscoveryService.performRemoteAttestation(success: { (remoteAttestation: RemoteAttestation) in
             self.makeContactDiscoveryRequest(remoteAttestation: remoteAttestation)
         },
-                                     failure: self.reportError)
+                                                         failure: self.attestationFailure)
+    }
+
+    private func attestationFailure(error: Error) {
+        self.reportError(ContactDiscoveryError.attestationError(underlyingError: error))
     }
 
     private func makeContactDiscoveryRequest(remoteAttestation: RemoteAttestation) {
@@ -275,18 +289,33 @@ class CDSBatchOperation: OWSOperation {
         },
                                         failure: { (task, error) in
                                             guard let response = task.response as? HTTPURLResponse else {
-                                                let responseError: NSError = OWSErrorMakeUnableToProcessServerResponseError() as NSError
+                                                let responseError = OWSErrorMakeUnableToProcessServerResponseError() as NSError
                                                 responseError.isRetryable = true
                                                 self.reportError(responseError)
                                                 return
                                             }
 
+                                            // TODO CDS ratelimiting
                                             guard response.statusCode != 413 else {
                                                 let rateLimitError = OWSErrorWithCodeDescription(OWSErrorCode.contactsUpdaterRateLimit, "Contacts Intersection Rate Limit")
                                                 self.reportError(rateLimitError)
                                                 return
                                             }
 
+                                            guard response.statusCode / 100 != 4 else {
+                                                let clientError: NSError = ContactDiscoveryError.clientError(underlyingError: error) as NSError
+                                                clientError.isRetryable = (error as NSError).isRetryable
+                                                self.reportError(clientError)
+                                                return
+                                            }
+
+                                            guard response.statusCode / 100 != 5 else {
+                                                let serverError = ContactDiscoveryError.serverError(underlyingError: error) as NSError
+                                                serverError.isRetryable = (error as NSError).isRetryable
+                                                self.reportError(serverError)
+                                                return
+                                            }
+
                                             self.reportError(error)
         })
     }
@@ -299,7 +328,7 @@ class CDSBatchOperation: OWSOperation {
                                                                 additionalAuthenticatedData: remoteAttestation.requestId,
                                                                 key: remoteAttestation.keys.clientKey) else {
 
-            throw CDSBatchOperationError.assertionError(description: "Encryption failure")
+            throw ContactDiscoveryError.assertionError(description: "Encryption failure")
         }
 
         return encryptionResult
@@ -310,14 +339,14 @@ class CDSBatchOperation: OWSOperation {
 
         for recipientId in recipientIds {
             guard recipientId.prefix(1) == "+" else {
-                throw CDSBatchOperationError.assertionError(description: "unexpected id format")
+                throw ContactDiscoveryError.assertionError(description: "unexpected id format")
             }
 
             let numericPortionIndex = recipientId.index(after: recipientId.startIndex)
             let numericPortion = recipientId.suffix(from: numericPortionIndex)
 
             guard let numericIdentifier = UInt64(numericPortion), numericIdentifier > 99 else {
-                throw CDSBatchOperationError.assertionError(description: "unexpectedly short identifier")
+                throw ContactDiscoveryError.assertionError(description: "unexpectedly short identifier")
             }
 
             var bigEndian: UInt64 = CFSwapInt64HostToBig(numericIdentifier)
@@ -331,7 +360,7 @@ class CDSBatchOperation: OWSOperation {
     func handle(response: Any?, remoteAttestation: RemoteAttestation) throws -> Set<String> {
         let isIncludedData: Data = try parseAndDecrypt(response: response, remoteAttestation: remoteAttestation)
         guard let isIncluded: [Bool] = type(of: self).boolArray(data: isIncludedData) else {
-            throw CDSBatchOperationError.assertionError(description: "isIncluded was unexpectedly nil")
+            throw ContactDiscoveryError.assertionError(description: "isIncluded was unexpectedly nil")
         }
 
         return try match(recipientIds: self.recipientIdsToLookup, isIncluded: isIncluded)
@@ -349,7 +378,7 @@ class CDSBatchOperation: OWSOperation {
 
     func match(recipientIds: [String], isIncluded: [Bool]) throws -> Set<String> {
         guard recipientIds.count == isIncluded.count else {
-            throw CDSBatchOperationError.assertionError(description: "length mismatch for isIncluded/recipientIds")
+            throw ContactDiscoveryError.assertionError(description: "length mismatch for isIncluded/recipientIds")
         }
 
         let includedRecipientIds: [String] = (0..<recipientIds.count).compactMap { index in
@@ -362,7 +391,7 @@ class CDSBatchOperation: OWSOperation {
     func parseAndDecrypt(response: Any?, remoteAttestation: RemoteAttestation) throws -> Data {
 
         guard let responseDict = response as? [String: AnyObject] else {
-            throw CDSBatchOperationError.parseError(description: "missing response dict")
+            throw ContactDiscoveryError.parseError(description: "missing response dict")
         }
 
         let cipherText = try responseDict.expectBase64EncodedData(key: "data")
@@ -375,7 +404,7 @@ class CDSBatchOperation: OWSOperation {
                                                           authTag: authTag,
                                                           key: remoteAttestation.keys.serverKey) else {
 
-                                                            throw CDSBatchOperationError.parseError(description: "decryption failed")
+                                                            throw ContactDiscoveryError.parseError(description: "decryption failed")
         }
 
         return plainText
@@ -384,15 +413,21 @@ class CDSBatchOperation: OWSOperation {
 
 class CDSFeedbackOperation: OWSOperation {
 
-    static let operationQueue: OperationQueue = {
+    enum FeedbackResult: String {
-        let queue = OperationQueue()
+        case ok
-        queue.maxConcurrentOperationCount = 1
+        case mismatch
-
+        case serverError = "server-error"
-        return queue
+        case clientError = "client-error"
-    }()
+        case attestationError = "attestation-error"
+        case unexpectedError = "unexpected-error"
+    }
 
     private let legacyRegisteredRecipientIds: Set<String>
 
+    var networkManager: TSNetworkManager {
+        return TSNetworkManager.shared()
+    }
+
     // MARK: Initializers
 
     required init(legacyRegisteredRecipientIds: Set<String>) {
@@ -405,33 +440,57 @@ class CDSFeedbackOperation: OWSOperation {
 
     // MARK: OWSOperation Overrides
 
+    override func checkForPreconditionError() -> Error? {
+        // override super with no-op
+        // In this rare case, we want to proceed even though our dependency might have an
+        // error so we can report the details of that error to the feedback service.
+        return nil
+    }
+
     // Called every retry, this is where the bulk of the operation's work should go.
     override func run() {
+
+        guard !isCancelled else {
+            Logger.info("\(logTag) in \(#function) no work to do, since we were canceled")
+            self.reportCancelled()
+            return
+        }
+
         guard let cdsOperation = dependencies.first as? CDSBatchOperation else {
             let error = OWSErrorMakeAssertionError("\(self.logTag) in \(#function) cdsOperation was unexpectedly nil")
             self.reportError(error)
             return
         }
 
-        let cdsRegisteredRecipientIds = cdsOperation.registeredRecipientIds
+        if let error = cdsOperation.failingError {
+            switch error {
+            case ContactDiscoveryError.serverError:
+                self.makeRequest(result: .serverError)
+            case ContactDiscoveryError.clientError:
+                self.makeRequest(result: .clientError)
+            case ContactDiscoveryError.attestationError:
+                self.makeRequest(result: .attestationError)
+            default:
+                self.makeRequest(result: .unexpectedError)
+            }
 
-        if cdsRegisteredRecipientIds == legacyRegisteredRecipientIds {
+            return
-            Logger.debug("\(logTag) in \(#function) TODO: PUT /v1/directory/feedback/ok")
-        } else {
-            Logger.debug("\(logTag) in \(#function) TODO: PUT /v1/directory/feedback/mismatch")
         }
 
-        self.reportSuccess()
+        if cdsOperation.registeredRecipientIds == legacyRegisteredRecipientIds {
+            self.makeRequest(result: .ok)
+            return
+        } else {
+            self.makeRequest(result: .mismatch)
+            return
+        }
     }
 
-    override func didFail(error: Error) {
+    func makeRequest(result: FeedbackResult) {
-        // dependency failed.
+        let request = OWSRequestFactory.cdsFeedbackRequest(result: result.rawValue)
-        // Depending on error, PUT one of:
+        self.networkManager.makeRequest(request,
-        // /v1/directory/feedback/server-error:
+                                        success: { _, _ in self.reportSuccess() },
-        // /v1/directory/feedback/client-error:
+                                        failure: { _, error in self.reportError(error) })
-        // /v1/directory/feedback/attestation-error:
-        // /v1/directory/feedback/unexpected-error:
-        Logger.debug("\(logTag) in \(#function) TODO: PUT /v1/directory/feedback/*-error")
     }
 }
 

SignalServiceKit/src/Network/API/Requests/OWSRequestFactory.h

@@ -86,6 +86,7 @@ typedef NS_ENUM(NSUInteger, TSVerificationTransport) { TSVerificationTransportVo
                                             cookies:(NSArray<NSHTTPCookie *> *)cookies;
 
 + (TSRequest *)remoteAttestationAuthRequest;
++ (TSRequest *)cdsFeedbackRequestWithResult:(NSString *)result NS_SWIFT_NAME(cdsFeedbackRequest(result:));
 
 @end
 

SignalServiceKit/src/Network/API/Requests/OWSRequestFactory.m

@@ -340,6 +340,12 @@ NS_ASSUME_NONNULL_BEGIN
     return [TSRequest requestWithUrl:[NSURL URLWithString:path] method:@"GET" parameters:@{}];
 }
 
++ (TSRequest *)cdsFeedbackRequestWithResult:(NSString *)result
+{
+    NSString *path = [NSString stringWithFormat:@"/v1/directory/feedback/%@", result];
+    return [TSRequest requestWithUrl:[NSURL URLWithString:path] method:@"PUT" parameters:@{}];
+}
+
 @end
 
 NS_ASSUME_NONNULL_END