From f34b0a63938df11ef471aa3301dcc0de09b0d31b Mon Sep 17 00:00:00 2001 From: Federico Cappelli Date: Wed, 8 May 2024 08:57:46 +0100 Subject: [PATCH 1/7] Pixels automatic naming prefixing fixed (#810) Task/Issue URL: https://app.asana.com/0/1201037661562251/1207238594885159/f macOS PR: https://github.com/duckduckgo/macos-browser/pull/2751 *Description* This PR contains few improvements: - Added a check on every Pixel name, if the name contains a `.` an assertionError is fired (in Debug) - Added a special pixel event called `NonStandardEvent` for those legacy Pixels non following the new PixelKit naming conventions, these kinds of pixels will maintain the name without automatic alterations. - New unit tests for covering the new event type and Debug event --- Sources/PixelKit/DebugEvent.swift | 92 ++++++++++++ Sources/PixelKit/NonStandardEvent.swift | 41 ++++++ Sources/PixelKit/PixelKit.swift | 23 ++- Sources/PixelKit/PixelKitEvent.swift | 74 ---------- .../XCTestCase+PixelKit.swift | 31 ++-- Tests/PixelKitTests/PixelKitTests.swift | 139 ++++++++++++++++-- 6 files changed, 288 insertions(+), 112 deletions(-) create mode 100644 Sources/PixelKit/DebugEvent.swift create mode 100644 Sources/PixelKit/NonStandardEvent.swift diff --git a/Sources/PixelKit/DebugEvent.swift b/Sources/PixelKit/DebugEvent.swift new file mode 100644 index 000000000..5aff63d17 --- /dev/null +++ b/Sources/PixelKit/DebugEvent.swift @@ -0,0 +1,92 @@ +// +// DebugEvent.swift +// +// Copyright Β© 2024 DuckDuckGo. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +import Foundation + +/// Implementation of ``PixelKitEvent`` with specific logic for debug events. +public final class DebugEvent: PixelKitEvent { + public enum EventType { + case assertionFailure(message: String, file: StaticString, line: UInt) + case custom(_ event: PixelKitEvent) + } + + public let eventType: EventType + public let error: Error? + + public init(eventType: EventType, error: Error? = nil) { + self.eventType = eventType + self.error = error + } + + public init(_ event: PixelKitEvent, error: Error? = nil) { + self.eventType = .custom(event) + self.error = error + } + + public var name: String { + switch eventType { + case .assertionFailure: + return "assertion_failure" + case .custom(let event): + return event.name + } + } + + public var parameters: [String: String]? { + var params: [String: String] + + if case let .custom(event) = eventType, + let eventParams = event.parameters { + params = eventParams + } else { + params = [String: String]() + } + + if let errorWithUserInfo = error as? ErrorWithPixelParameters { + params = errorWithUserInfo.errorParameters + } + + if case let .assertionFailure(message, file, line) = eventType { + params[PixelKit.Parameters.assertionMessage] = message + params[PixelKit.Parameters.assertionFile] = String(file) + params[PixelKit.Parameters.assertionLine] = String(line) + } + + if let error = error { + let nsError = error as NSError + + params[PixelKit.Parameters.errorCode] = "\(nsError.code)" + params[PixelKit.Parameters.errorDomain] = nsError.domain + + if let underlyingError = nsError.userInfo["NSUnderlyingError"] as? NSError { + params[PixelKit.Parameters.underlyingErrorCode] = "\(underlyingError.code)" + params[PixelKit.Parameters.underlyingErrorDomain] = underlyingError.domain + } + + if let sqlErrorCode = nsError.userInfo["SQLiteResultCode"] as? NSNumber { + params[PixelKit.Parameters.underlyingErrorSQLiteCode] = "\(sqlErrorCode.intValue)" + } + + if let sqlExtendedErrorCode = nsError.userInfo["SQLiteExtendedResultCode"] as? NSNumber { + params[PixelKit.Parameters.underlyingErrorSQLiteExtendedCode] = "\(sqlExtendedErrorCode.intValue)" + } + } + + return params + } +} diff --git a/Sources/PixelKit/NonStandardEvent.swift b/Sources/PixelKit/NonStandardEvent.swift new file mode 100644 index 000000000..274b68e64 --- /dev/null +++ b/Sources/PixelKit/NonStandardEvent.swift @@ -0,0 +1,41 @@ +// +// NonStandardEvent.swift +// +// Copyright Β© 2024 DuckDuckGo. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +import Foundation + +/// This custom event is used for special cases, like pixels with non-standard names and uses, these pixels are sent as is and the names remain unchanged +public final class NonStandardEvent: PixelKitEventV2 { + + let event: PixelKitEventV2 + + public init(_ event: PixelKitEventV2) { + self.event = event + } + + public var name: String { + event.name + } + + public var parameters: [String: String]? { + event.parameters + } + + public var error: Error? { + event.error + } +} diff --git a/Sources/PixelKit/PixelKit.swift b/Sources/PixelKit/PixelKit.swift index d647b2883..6af7381c1 100644 --- a/Sources/PixelKit/PixelKit.swift +++ b/Sources/PixelKit/PixelKit.swift @@ -186,6 +186,9 @@ public final class PixelKit { headers[Header.moreInfo] = "See " + Self.duckDuckGoMorePrivacyInfo.absoluteString headers[Header.client] = "macOS" + // The event name can't contain `.` + reportErrorIf(pixel: pixelName, contains: ".") + switch frequency { case .standard: reportErrorIf(pixel: pixelName, endsWith: "_u") @@ -204,6 +207,7 @@ public final class PixelKit { reportErrorIf(pixel: pixelName, endsWith: "_d") guard pixelName.hasSuffix("_u") else { assertionFailure("Unique pixel: must end with _u") + onComplete(false, nil) return } if !pixelHasBeenFiredEver(pixelName) { @@ -253,6 +257,14 @@ public final class PixelKit { } } + /// If the pixel name contains the forbiddenString then an error is logged or an assertion failure is fired in debug + func reportErrorIf(pixel: String, contains forbiddenString: String) { + if pixel.contains(forbiddenString) { + logger.error("Pixel \(pixel, privacy: .public) must not contain \(forbiddenString, privacy: .public)") + assertionFailure("Pixel \(pixel) must not contain \(forbiddenString)") + } + } + private func printDebugInfo(pixelName: String, frequency: Frequency, parameters: [String: String], skipped: Bool = false) { let params = parameters.filter { key, _ in !["test"].contains(key) } logger.debug("πŸ‘Ύ[\(frequency.description, privacy: .public)-\(skipped ? "Skipped" : "Fired", privacy: .public)] \(pixelName, privacy: .public) \(params, privacy: .public)") @@ -279,11 +291,14 @@ public final class PixelKit { private func prefixedName(for event: Event) -> String { if event.name.hasPrefix("m_mac_") { + // Can be a debug event or not, if already prefixed the name remains unchanged return event.name - } - - if let debugEvent = event as? DebugEvent { + } else if let debugEvent = event as? DebugEvent { + // Is a Debug event not already prefixed return "m_mac_debug_\(debugEvent.name)" + } else if let nonStandardEvent = event as? NonStandardEvent { + // Special kind of pixel event that don't follow the standard naming conventions + return nonStandardEvent.name } else { return "m_mac_\(event.name)" } @@ -328,7 +343,7 @@ public final class PixelKit { let error = event.error { // For v2 events we only consider the error specified in the event - // and purposedly ignore the parameter in this call. + // and purposely ignore the parameter in this call. // This is to encourage moving the error over to the protocol error // instead of still relying on the parameter of this call. newError = error diff --git a/Sources/PixelKit/PixelKitEvent.swift b/Sources/PixelKit/PixelKitEvent.swift index ca352f334..04573f0e7 100644 --- a/Sources/PixelKit/PixelKitEvent.swift +++ b/Sources/PixelKit/PixelKitEvent.swift @@ -24,77 +24,3 @@ public protocol PixelKitEvent { var name: String { get } var parameters: [String: String]? { get } } - -/// Implementation of ``PixelKitEvent`` with specific logic for debug events. -/// -public final class DebugEvent: PixelKitEvent { - public enum EventType { - case assertionFailure(message: String, file: StaticString, line: UInt) - case custom(_ event: PixelKitEvent) - } - - public let eventType: EventType - public let error: Error? - - public init(eventType: EventType, error: Error? = nil) { - self.eventType = eventType - self.error = error - } - - public init(_ event: PixelKitEvent, error: Error? = nil) { - self.eventType = .custom(event) - self.error = error - } - - public var name: String { - switch eventType { - case .assertionFailure: - return "assertion_failure" - case .custom(let event): - return event.name - } - } - - public var parameters: [String: String]? { - var params: [String: String] - - if case let .custom(event) = eventType, - let eventParams = event.parameters { - params = eventParams - } else { - params = [String: String]() - } - - if let errorWithUserInfo = error as? ErrorWithPixelParameters { - params = errorWithUserInfo.errorParameters - } - - if case let .assertionFailure(message, file, line) = eventType { - params[PixelKit.Parameters.assertionMessage] = message - params[PixelKit.Parameters.assertionFile] = String(file) - params[PixelKit.Parameters.assertionLine] = String(line) - } - - if let error = error { - let nsError = error as NSError - - params[PixelKit.Parameters.errorCode] = "\(nsError.code)" - params[PixelKit.Parameters.errorDomain] = nsError.domain - - if let underlyingError = nsError.userInfo["NSUnderlyingError"] as? NSError { - params[PixelKit.Parameters.underlyingErrorCode] = "\(underlyingError.code)" - params[PixelKit.Parameters.underlyingErrorDomain] = underlyingError.domain - } - - if let sqlErrorCode = nsError.userInfo["SQLiteResultCode"] as? NSNumber { - params[PixelKit.Parameters.underlyingErrorSQLiteCode] = "\(sqlErrorCode.intValue)" - } - - if let sqlExtendedErrorCode = nsError.userInfo["SQLiteExtendedResultCode"] as? NSNumber { - params[PixelKit.Parameters.underlyingErrorSQLiteExtendedCode] = "\(sqlExtendedErrorCode.intValue)" - } - } - - return params - } -} diff --git a/Sources/PixelKitTestingUtilities/XCTestCase+PixelKit.swift b/Sources/PixelKitTestingUtilities/XCTestCase+PixelKit.swift index 78766b559..c12e95213 100644 --- a/Sources/PixelKitTestingUtilities/XCTestCase+PixelKit.swift +++ b/Sources/PixelKitTestingUtilities/XCTestCase+PixelKit.swift @@ -51,14 +51,6 @@ public extension XCTestCase { } } - static var pixelPlatformPrefix: String { -#if os(macOS) - return "m_mac_" -#elseif os(iOS) - return "m_" -#endif - } - /// These parameters are known to be expected just based on the event definition. /// /// They're not a complete list of parameters for the event, as the fire call may contain extra information @@ -122,10 +114,14 @@ public extension XCTestCase { let firedParameters = Self.filterStandardPixelParameters(from: firedParameters) // Internal validations - XCTAssertTrue(expectedPixelNames.contains(firedPixelName), file: file, line: line) + var found = false + for expectedNameSuffix in expectedPixelNames where firedPixelName.hasSuffix(expectedNameSuffix) { + found = true + } + XCTAssertTrue(found, file: file, line: line) XCTAssertTrue(knownExpectedParameters.allSatisfy { (key, value) in firedParameters[key] == value - }) + }, file: file, line: line) if frequency == .dailyAndCount { XCTAssertTrue(firedPixelName.hasPrefix(expectations.pixelName)) @@ -146,23 +142,22 @@ public extension XCTestCase { } func expectedPixelNames(originalName: String, frequency: PixelKit.Frequency) -> [String] { - let expectedPixelNameWithoutSuffix = originalName.hasPrefix(Self.pixelPlatformPrefix) ? originalName : Self.pixelPlatformPrefix + originalName var expectedPixelNames: [String] = [] switch frequency { case .standard: - expectedPixelNames.append(expectedPixelNameWithoutSuffix) + expectedPixelNames.append(originalName) case .legacyInitial: - expectedPixelNames.append(expectedPixelNameWithoutSuffix) + expectedPixelNames.append(originalName) case .unique: - expectedPixelNames.append(expectedPixelNameWithoutSuffix) + expectedPixelNames.append(originalName) case .legacyDaily: - expectedPixelNames.append(expectedPixelNameWithoutSuffix) + expectedPixelNames.append(originalName) case .daily: - expectedPixelNames.append(expectedPixelNameWithoutSuffix.appending("_d")) + expectedPixelNames.append(originalName.appending("_d")) case .dailyAndCount: - expectedPixelNames.append(expectedPixelNameWithoutSuffix.appending("_d")) - expectedPixelNames.append(expectedPixelNameWithoutSuffix.appending("_c")) + expectedPixelNames.append(originalName.appending("_d")) + expectedPixelNames.append(originalName.appending("_c")) } return expectedPixelNames } diff --git a/Tests/PixelKitTests/PixelKitTests.swift b/Tests/PixelKitTests/PixelKitTests.swift index cf51b5031..51d1834b7 100644 --- a/Tests/PixelKitTests/PixelKitTests.swift +++ b/Tests/PixelKitTests/PixelKitTests.swift @@ -27,23 +27,38 @@ final class PixelKitTests: XCTestCase { } /// Test events for convenience - /// + private enum TestEvent: String, PixelKitEvent { + + case testEventPrefixed = "m_mac_testEventPrefixed" + case testEvent + + var name: String { + return rawValue + } + + var parameters: [String: String]? { + return nil + } + + var error: Error? { + return nil + } + } + + private enum TestEventV2: String, PixelKitEventV2 { + case testEvent case testEventWithoutParameters case dailyEvent case dailyEventWithoutParameters case dailyAndContinuousEvent case dailyAndContinuousEventWithoutParameters - case uniqueEvent + case uniqueEvent = "uniqueEvent_u" + case nameWithDot = "test.pixel.with.dot" var name: String { - switch self { - case .uniqueEvent: - return "\(rawValue)_u" - default: - return rawValue - } + return rawValue } var parameters: [String: String]? { @@ -53,14 +68,18 @@ final class PixelKitTests: XCTestCase { "eventParam1": "eventParamValue1", "eventParam2": "eventParamValue2" ] - case .testEventWithoutParameters, .dailyEventWithoutParameters, .dailyAndContinuousEventWithoutParameters: + default: return nil } } + var error: Error? { + return nil + } + var frequency: PixelKit.Frequency { switch self { - case .testEvent, .testEventWithoutParameters: + case .testEvent, .testEventWithoutParameters, .nameWithDot: return .standard case .uniqueEvent: return .unique @@ -83,7 +102,95 @@ final class PixelKitTests: XCTestCase { XCTFail("This callback should not be executed when doing a dry run") } - pixelKit.fire(TestEvent.testEvent) + pixelKit.fire(TestEventV2.testEvent) + } + + func testNonStandardEvent() { + func testReportBrokenSitePixel() { + fire(NonStandardEvent(TestEventV2.testEvent), + frequency: .standard, + and: .expect(pixelName: TestEventV2.testEvent.name), + file: #filePath, + line: #line) + } + } + + func testDebugEventPrefixed() { + let appVersion = "1.0.5" + let headers = ["a": "2", "b": "3", "c": "2000"] + let event = DebugEvent(TestEvent.testEventPrefixed) + let userDefaults = userDefaults() + + // Set expectations + let expectedPixelName = TestEvent.testEventPrefixed.name + let fireCallbackCalled = expectation(description: "Expect the pixel firing callback to be called") + + // Prepare mock to validate expectations + let pixelKit = PixelKit(dryRun: false, + appVersion: appVersion, + defaultHeaders: headers, + dailyPixelCalendar: nil, + defaults: userDefaults) { firedPixelName, firedHeaders, parameters, _, _, _ in + + fireCallbackCalled.fulfill() + XCTAssertEqual(expectedPixelName, firedPixelName) + } + // Run test + pixelKit.fire(event) + // Wait for expectations to be fulfilled + wait(for: [fireCallbackCalled], timeout: 0.5) + } + + func testDebugEventNotPrefixed() { + let appVersion = "1.0.5" + let headers = ["a": "2", "b": "3", "c": "2000"] + let event = DebugEvent(TestEvent.testEvent) + let userDefaults = userDefaults() + + // Set expectations + let expectedPixelName = "m_mac_debug_\(TestEvent.testEvent.name)" + let fireCallbackCalled = expectation(description: "Expect the pixel firing callback to be called") + + // Prepare mock to validate expectations + let pixelKit = PixelKit(dryRun: false, + appVersion: appVersion, + defaultHeaders: headers, + dailyPixelCalendar: nil, + defaults: userDefaults) { firedPixelName, firedHeaders, parameters, _, _, _ in + + fireCallbackCalled.fulfill() + XCTAssertEqual(expectedPixelName, firedPixelName) + } + // Run test + pixelKit.fire(event) + // Wait for expectations to be fulfilled + wait(for: [fireCallbackCalled], timeout: 0.5) + } + + func testDebugEventDaily() { + let appVersion = "1.0.5" + let headers = ["a": "2", "b": "3", "c": "2000"] + let event = DebugEvent(TestEvent.testEvent) + let userDefaults = userDefaults() + + // Set expectations + let expectedPixelName = "m_mac_debug_\(TestEvent.testEvent.name)_d" + let fireCallbackCalled = expectation(description: "Expect the pixel firing callback to be called") + + // Prepare mock to validate expectations + let pixelKit = PixelKit(dryRun: false, + appVersion: appVersion, + defaultHeaders: headers, + dailyPixelCalendar: nil, + defaults: userDefaults) { firedPixelName, firedHeaders, parameters, _, _, _ in + + fireCallbackCalled.fulfill() + XCTAssertEqual(expectedPixelName, firedPixelName) + } + // Run test + pixelKit.fire(event, frequency: .daily) + // Wait for expectations to be fulfilled + wait(for: [fireCallbackCalled], timeout: 0.5) } /// Tests firing a sample pixel and ensuring that all fields are properly set in the fire request callback. @@ -92,7 +199,7 @@ final class PixelKitTests: XCTestCase { // Prepare test parameters let appVersion = "1.0.5" let headers = ["a": "2", "b": "3", "c": "2000"] - let event = TestEvent.testEvent + let event = TestEventV2.testEvent let userDefaults = userDefaults() // Set expectations @@ -136,7 +243,7 @@ final class PixelKitTests: XCTestCase { // Prepare test parameters let appVersion = "1.0.5" let headers = ["a": "2", "b": "3", "c": "2000"] - let event = TestEvent.dailyEvent + let event = TestEventV2.dailyEvent let userDefaults = userDefaults() // Set expectations @@ -180,7 +287,7 @@ final class PixelKitTests: XCTestCase { // Prepare test parameters let appVersion = "1.0.5" let headers = ["a": "2", "b": "3", "c": "2000"] - let event = TestEvent.dailyEvent + let event = TestEventV2.dailyEvent let userDefaults = userDefaults() // Set expectations @@ -226,7 +333,7 @@ final class PixelKitTests: XCTestCase { // Prepare test parameters let appVersion = "1.0.5" let headers = ["a": "2", "b": "3", "c": "2000"] - let event = TestEvent.dailyEvent + let event = TestEventV2.dailyEvent let userDefaults = userDefaults() let timeMachine = TimeMachine() @@ -270,7 +377,7 @@ final class PixelKitTests: XCTestCase { // Prepare test parameters let appVersion = "1.0.5" let headers = ["a": "2", "b": "3", "c": "2000"] - let event = TestEvent.uniqueEvent + let event = TestEventV2.uniqueEvent let userDefaults = userDefaults() let timeMachine = TimeMachine() From e3280911e427e8a60d5766d38602361ab78d5e66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jacek=20=C5=81yp?= Date: Wed, 8 May 2024 16:09:39 +0200 Subject: [PATCH 2/7] Bat.js fix (fix for installing content blocking rules multiple times) (#779) Task/Issue URL: https://app.asana.com/0/1177771139624306/1205259925028360/f --- .../ContentBlockerRulesIdentifier.swift | 25 +- .../ContentBlockerRulesManager.swift | 24 +- .../ContentBlockingRulesCompilationTask.swift | 9 +- .../UserContentController.swift | 102 ++++-- Sources/Common/Logging.swift | 2 + .../UserContentControllerTests.swift | 308 ++++++++++++++++++ 6 files changed, 442 insertions(+), 28 deletions(-) create mode 100644 Tests/BrowserServicesKitTests/ContentBlocker/UserContentControllerTests.swift diff --git a/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesIdentifier.swift b/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesIdentifier.swift index 245fcbac2..f90211ef7 100644 --- a/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesIdentifier.swift +++ b/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesIdentifier.swift @@ -30,7 +30,7 @@ public class ContentBlockerRulesIdentifier: Equatable, Codable { return name + tdsEtag + tempListId + allowListId + unprotectedSitesHash } - public struct Difference: OptionSet { + public struct Difference: OptionSet, CustomDebugStringConvertible { public let rawValue: Int public init(rawValue: Int) { @@ -43,6 +43,29 @@ public class ContentBlockerRulesIdentifier: Equatable, Codable { public static let unprotectedSites = Difference(rawValue: 1 << 3) public static let all: Difference = [.tdsEtag, .tempListId, .allowListId, .unprotectedSites] + + public var debugDescription: String { + if self == .all { + return "all" + } + var result = "[" + for i in 0...Int(log2(Double(max(self.rawValue, Self.all.rawValue)))) where self.contains(Self(rawValue: 1 << i)) { + if result.count > 1 { + result += ", " + } + result += { + switch Self(rawValue: 1 << i) { + case .tdsEtag: ".tdsEtag" + case .tempListId: ".tempListId" + case .allowListId: ".allowListId" + case .unprotectedSites: ".unprotectedSites" + default: "1<<\(i)" + } + }() + } + result += "]" + return result + } } private class func normalize(identifier: String?) -> String { diff --git a/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesManager.swift b/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesManager.swift index b980a60d9..6588035c7 100644 --- a/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesManager.swift +++ b/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesManager.swift @@ -96,7 +96,7 @@ public class ContentBlockerRulesManager: CompiledRuleListsSource { private let cache: ContentBlockerRulesCaching? public let exceptionsSource: ContentBlockerRulesExceptionsSource - public struct UpdateEvent { + public struct UpdateEvent: CustomDebugStringConvertible { public let rules: [ContentBlockerRulesManager.Rules] public let changes: [String: ContentBlockerRulesIdentifier.Difference] public let completionTokens: [ContentBlockerRulesManager.CompletionToken] @@ -108,6 +108,14 @@ public class ContentBlockerRulesManager: CompiledRuleListsSource { self.changes = changes self.completionTokens = completionTokens } + + public var debugDescription: String { + """ + rules: \(rules.map { "\($0.name):\($0.identifier) – \($0.rulesList) (\($0.etag))" }.joined(separator: ", ")) + changes: \(changes) + completionTokens: \(completionTokens) + """ + } } private let updatesSubject = PassthroughSubject() public var updatesPublisher: AnyPublisher { @@ -193,6 +201,7 @@ public class ContentBlockerRulesManager: CompiledRuleListsSource { @discardableResult public func scheduleCompilation() -> CompletionToken { let token = UUID().uuidString + os_log("Scheduling compilation with %{public}s", log: log, type: .default, token) workQueue.async { let shouldStartCompilation = self.updateCompilationState(token: token) if shouldStartCompilation { @@ -228,12 +237,17 @@ public class ContentBlockerRulesManager: CompiledRuleListsSource { Returns true if rules were found, false otherwise. */ private func lookupCompiledRules() -> Bool { + os_log("Lookup compiled rules", log: log, type: .debug) prepareSourceManagers() let initialCompilationTask = LookupRulesTask(sourceManagers: Array(sourceManagers.values)) let mutex = DispatchSemaphore(value: 0) - Task { - try? await initialCompilationTask.lookupCachedRulesLists() + Task { [log] in + do { + try await initialCompilationTask.lookupCachedRulesLists() + } catch { + os_log("❌ Lookup failed: %{public}s", log: log, type: .debug, error.localizedDescription) + } mutex.signal() } // We want to confine Compilation work to WorkQueue, so we wait to come back from async Task @@ -241,6 +255,7 @@ public class ContentBlockerRulesManager: CompiledRuleListsSource { if let result = initialCompilationTask.result { let rules = result.map(Rules.init(compilationResult:)) + os_log("🟩 Found %{public}d rules", log: log, type: .debug, rules.count) applyRules(rules) return true } @@ -252,6 +267,8 @@ public class ContentBlockerRulesManager: CompiledRuleListsSource { Returns true if rules were found, false otherwise. */ private func fetchLastCompiledRules(with lastCompiledRules: [LastCompiledRules]) { + os_log("Fetch last compiled rules: %{public}d", log: log, type: .debug, lastCompiledRules.count) + let initialCompilationTask = LastCompiledRulesLookupTask(sourceRules: rulesSource.contentBlockerRulesLists, lastCompiledRules: lastCompiledRules) let mutex = DispatchSemaphore(value: 0) @@ -294,6 +311,7 @@ public class ContentBlockerRulesManager: CompiledRuleListsSource { } private func startCompilationProcess() { + os_log("Starting compilataion process", log: log, type: .debug) prepareSourceManagers() // Prepare compilation tasks based on the sources diff --git a/Sources/BrowserServicesKit/ContentBlocking/ContentBlockingRulesCompilationTask.swift b/Sources/BrowserServicesKit/ContentBlocking/ContentBlockingRulesCompilationTask.swift index 74cf980ae..3d7ecc0b1 100644 --- a/Sources/BrowserServicesKit/ContentBlocking/ContentBlockingRulesCompilationTask.swift +++ b/Sources/BrowserServicesKit/ContentBlocking/ContentBlockingRulesCompilationTask.swift @@ -53,12 +53,14 @@ extension ContentBlockerRulesManager { func start(ignoreCache: Bool = false, completionHandler: @escaping Completion) { self.workQueue.async { guard let model = self.sourceManager.makeModel() else { + os_log("❌ compilation impossible", log: self.log, type: .default) self.compilationImpossible = true completionHandler(self, false) return } guard !ignoreCache else { + os_log("❗️ ignoring cache", log: self.log, type: .default) self.workQueue.async { self.compile(model: model, completionHandler: completionHandler) } @@ -68,8 +70,10 @@ extension ContentBlockerRulesManager { // Delegate querying to main thread - crashes were observed in background. DispatchQueue.main.async { let identifier = model.rulesIdentifier.stringValue + os_log("Lookup CBR with %{public}s", log: self.log, type: .default, identifier) WKContentRuleListStore.default()?.lookUpContentRuleList(forIdentifier: identifier) { ruleList, _ in if let ruleList = ruleList { + os_log("🟒 CBR loaded from cache: %{public}s", log: self.log, type: .default, self.rulesList.name) self.compilationSucceeded(with: ruleList, model: model, completionHandler: completionHandler) } else { self.workQueue.async { @@ -94,7 +98,7 @@ extension ContentBlockerRulesManager { with error: Error, completionHandler: @escaping Completion) { workQueue.async { - os_log("Failed to compile %{public}s rules %{public}s", + os_log("❌ Failed to compile %{public}s rules %{public}s", log: self.log, type: .error, self.rulesList.name, @@ -125,7 +129,7 @@ extension ContentBlockerRulesManager { do { data = try JSONEncoder().encode(rules) } catch { - os_log("Failed to encode content blocking rules %{public}s", log: log, type: .error, rulesList.name) + os_log("❌ Failed to encode content blocking rules %{public}s", log: log, type: .error, rulesList.name) compilationFailed(for: model, with: error, completionHandler: completionHandler) return } @@ -136,6 +140,7 @@ extension ContentBlockerRulesManager { encodedContentRuleList: ruleList) { ruleList, error in if let ruleList = ruleList { + os_log("🟒 CBR compilation for %{public}s succeeded", log: self.log, type: .default, self.rulesList.name) self.compilationSucceeded(with: ruleList, model: model, completionHandler: completionHandler) } else if let error = error { self.compilationFailed(for: model, with: error, completionHandler: completionHandler) diff --git a/Sources/BrowserServicesKit/ContentScopeScript/UserContentController.swift b/Sources/BrowserServicesKit/ContentScopeScript/UserContentController.swift index 6468fcb3f..762f036c1 100644 --- a/Sources/BrowserServicesKit/ContentScopeScript/UserContentController.swift +++ b/Sources/BrowserServicesKit/ContentScopeScript/UserContentController.swift @@ -16,9 +16,11 @@ // limitations under the License. // -import WebKit import Combine +import Common import UserScript +import WebKit +import QuartzCore public protocol UserContentControllerDelegate: AnyObject { @MainActor @@ -37,12 +39,13 @@ public protocol UserContentControllerNewContent { var makeUserScripts: @MainActor (SourceProvider) -> UserScripts { get } } +@objc(UserContentController) final public class UserContentController: WKUserContentController { public let privacyConfigurationManager: PrivacyConfigurationManaging @MainActor public weak var delegate: UserContentControllerDelegate? - public struct ContentBlockingAssets { + public struct ContentBlockingAssets: CustomDebugStringConvertible { public let globalRuleLists: [String: WKContentRuleList] public let userScripts: UserScriptsProvider public let wkUserScripts: [WKUserScript] @@ -58,32 +61,52 @@ final public class UserContentController: WKUserContentController { self.wkUserScripts = await userScripts.loadWKUserScripts() } + + public var debugDescription: String { + """ + + """ + } } @Published @MainActor public private(set) var contentBlockingAssets: ContentBlockingAssets? { willSet { self.removeAllContentRuleLists() self.removeAllUserScripts() + + if let contentBlockingAssets = newValue { + os_log(.debug, log: .contentBlocking, "\(self): πŸ“š installing \(contentBlockingAssets)") + self.installGlobalContentRuleLists(contentBlockingAssets.globalRuleLists) + os_log(.debug, log: .userScripts, "\(self): πŸ“œ installing user scripts") + self.installUserScripts(contentBlockingAssets.wkUserScripts, handlers: contentBlockingAssets.userScripts.userScripts) + os_log(.debug, log: .contentBlocking, "\(self): βœ… installing content blocking assets done") + } } } @MainActor private func installContentBlockingAssets(_ contentBlockingAssets: ContentBlockingAssets) { // donβ€˜t install ContentBlockingAssets (especially Message Handlers retaining `self`) after cleanUpBeforeClosing was called guard assetsPublisherCancellable != nil else { return } - + // installation should happen in `contentBlockingAssets.willSet` + // so the $contentBlockingAssets subscribers receive an update only after everything is set self.contentBlockingAssets = contentBlockingAssets - self.installGlobalContentRuleLists(contentBlockingAssets.globalRuleLists) - self.installUserScripts(contentBlockingAssets.wkUserScripts, handlers: contentBlockingAssets.userScripts.userScripts) - delegate?.userContentController(self, didInstallContentRuleLists: contentBlockingAssets.globalRuleLists, userScripts: contentBlockingAssets.userScripts, updateEvent: contentBlockingAssets.updateEvent) } + enum ContentRuleListIdentifier: Hashable { + case global(String), local(String) + } @MainActor - private var localRuleLists = [String: WKContentRuleList]() + private var contentRuleLists = [ContentRuleListIdentifier: WKContentRuleList]() @MainActor private var assetsPublisherCancellable: AnyCancellable? @MainActor @@ -96,7 +119,8 @@ final public class UserContentController: WKUserContentController { self.privacyConfigurationManager = privacyConfigurationManager super.init() - assetsPublisherCancellable = assetsPublisher.sink { [weak self] content in + assetsPublisherCancellable = assetsPublisher.sink { [weak self, selfDescr=self.debugDescription] content in + os_log(.debug, log: .contentBlocking, "\(selfDescr): πŸ“š received content blocking assets") Task.detached { [weak self] in let contentBlockingAssets = await ContentBlockingAssets(content: content) await self?.installContentBlockingAssets(contentBlockingAssets) @@ -116,50 +140,73 @@ final public class UserContentController: WKUserContentController { } @MainActor - private func installGlobalContentRuleLists(_ contentRuleLists: [String: WKContentRuleList]) { + private func installGlobalContentRuleLists(_ globalContentRuleLists: [String: WKContentRuleList]) { + assert(contentRuleLists.isEmpty, "installGlobalContentRuleLists should be called after removing all Content Rule Lists") guard self.privacyConfigurationManager.privacyConfig.isEnabled(featureKey: .contentBlocking) else { + os_log(.debug, log: .contentBlocking, "\(self): ❗️ content blocking disabled, removing all content rule lists") removeAllContentRuleLists() return } - contentRuleLists.values.forEach(self.add) + os_log(.debug, log: .contentBlocking, "\(self): ❇️ installing global rule lists: \(globalContentRuleLists))") + contentRuleLists = globalContentRuleLists.reduce(into: [:]) { + $0[.global($1.key)] = $1.value + } + globalContentRuleLists.values.forEach(self.add) } public struct ContentRulesNotFoundError: Error {} @MainActor public func enableGlobalContentRuleList(withIdentifier identifier: String) throws { - guard let ruleList = self.contentBlockingAssets?.globalRuleLists[identifier] else { + guard let ruleList = contentBlockingAssets?.globalRuleLists[identifier] + // when enabling from a $contentBlockingAssets subscription, the ruleList gets + // to contentRuleLists before contentBlockingAssets value is set + ?? contentRuleLists[.global(identifier)] else { + os_log(.debug, log: .contentBlocking, "\(self): ❗️ canβ€˜t enable rule list `\(identifier)` as itβ€˜s not available") throw ContentRulesNotFoundError() } - self.add(ruleList) + guard contentRuleLists[.global(identifier)] == nil else { return /* already enabled */ } + + os_log(.debug, log: .contentBlocking, "\(self): 🟩 enabling rule list `\(identifier)`") + contentRuleLists[.global(identifier)] = ruleList + add(ruleList) } public struct ContentRulesNotEnabledError: Error {} @MainActor public func disableGlobalContentRuleList(withIdentifier identifier: String) throws { - guard let ruleList = self.contentBlockingAssets?.globalRuleLists[identifier] else { + guard let ruleList = contentRuleLists[.global(identifier)] else { + os_log(.debug, log: .contentBlocking, "\(self): ❗️ canβ€˜t disable rule list `\(identifier)` as itβ€˜s not enabled") throw ContentRulesNotEnabledError() } - self.remove(ruleList) + + os_log(.debug, log: .contentBlocking, "\(self): πŸ”» disabling rule list `\(identifier)`") + contentRuleLists[.global(identifier)] = nil + remove(ruleList) } @MainActor public func installLocalContentRuleList(_ ruleList: WKContentRuleList, identifier: String) { - localRuleLists[identifier] = ruleList - self.add(ruleList) + // replace if already installed + removeLocalContentRuleList(withIdentifier: identifier) + + os_log(.debug, log: .contentBlocking, "\(self): πŸ”Έ installing local rule list `\(identifier)`") + contentRuleLists[.local(identifier)] = ruleList + add(ruleList) } @MainActor public func removeLocalContentRuleList(withIdentifier identifier: String) { - guard let ruleList = localRuleLists.removeValue(forKey: identifier) else { - return - } - self.remove(ruleList) + guard let ruleList = contentRuleLists.removeValue(forKey: .local(identifier)) else { return } + + os_log(.debug, log: .contentBlocking, "\(self): πŸ”» removing local rule list `\(identifier)`") + remove(ruleList) } @MainActor public override func removeAllContentRuleLists() { - localRuleLists = [:] + os_log(.debug, log: .contentBlocking, "\(self): 🧹 removing all content rule lists") + contentRuleLists.removeAll(keepingCapacity: true) super.removeAllContentRuleLists() } @@ -171,6 +218,8 @@ final public class UserContentController: WKUserContentController { @MainActor public func cleanUpBeforeClosing() { + os_log(.debug, log: .contentBlocking, "\(self): πŸ’€ cleanUpBeforeClosing") + self.removeAllUserScripts() if #available(macOS 11.0, *) { @@ -222,7 +271,9 @@ public extension UserContentController { @MainActor var awaitContentBlockingAssetsInstalled: () async -> Void { guard !contentBlockingAssetsInstalled else { return {} } - return { [weak self] in + os_log(.debug, log: .contentBlocking, "\(self): πŸ›‘ will wait for content blocking assets installed") + let startTime = CACurrentMediaTime() + return { [weak self, selfDescr=self.description] in // merge $contentBlockingAssets with Task cancellation completion event publisher let taskCancellationSubject = PassthroughSubject() guard let assetsPublisher = self?.$contentBlockingAssets else { return } @@ -237,14 +288,21 @@ public extension UserContentController { try? await withTaskCancellationHandler { try await withCheckedThrowingContinuation { c in var cancellable: AnyCancellable! + var elapsedTime: String { + String(format: "%.2fs.", CACurrentMediaTime() - startTime) + } cancellable = throwingPublisher.sink /* completion: */ { _ in withExtendedLifetime(cancellable) { + os_log(.debug, log: .contentBlocking, "\(selfDescr): ❌ wait cancelled after \(elapsedTime)") + c.resume(with: .failure(CancellationError())) cancellable.cancel() } } receiveValue: { assets in guard assets != nil else { return } withExtendedLifetime(cancellable) { + os_log(.debug, log: .contentBlocking, "\(selfDescr): 🏁 content blocking assets installed (\(elapsedTime))") + c.resume(with: .success( () )) cancellable.cancel() } diff --git a/Sources/Common/Logging.swift b/Sources/Common/Logging.swift index 1d83e3322..8347ac6c4 100644 --- a/Sources/Common/Logging.swift +++ b/Sources/Common/Logging.swift @@ -36,6 +36,7 @@ extension OSLog { }() public enum Categories: String, CaseIterable { + case contentBlocking = "Content Blocking" case userScripts = "User Scripts" case passwordManager = "Password Manager" case remoteMessaging = "Remote Messaging" @@ -51,6 +52,7 @@ extension OSLog { static var debugCategories: Set = [ /*.autofill*/ ] #endif + @OSLogWrapper(.contentBlocking) public static var contentBlocking @OSLogWrapper(.userScripts) public static var userScripts @OSLogWrapper(.passwordManager) public static var passwordManager @OSLogWrapper(.remoteMessaging) public static var remoteMessaging diff --git a/Tests/BrowserServicesKitTests/ContentBlocker/UserContentControllerTests.swift b/Tests/BrowserServicesKitTests/ContentBlocker/UserContentControllerTests.swift new file mode 100644 index 000000000..3caa3d016 --- /dev/null +++ b/Tests/BrowserServicesKitTests/ContentBlocker/UserContentControllerTests.swift @@ -0,0 +1,308 @@ +// +// UserContentControllerTests.swift +// +// Copyright Β© 2024 DuckDuckGo. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +import Combine +import Common +import Foundation +import UserScript +import WebKit +import XCTest + +@testable import BrowserServicesKit + +final class UserContentControllerTests: XCTestCase { + + struct MockScriptSourceProvider { + } + class MockScriptProvider: UserScriptsProvider { + var userScripts: [UserScript] { [] } + func loadWKUserScripts() async -> [WKUserScript] { + [] + } + } + + struct NewContent: UserContentControllerNewContent { + let rulesUpdate: ContentBlockerRulesManager.UpdateEvent + let sourceProvider: MockScriptSourceProvider + + var makeUserScripts: @MainActor (MockScriptSourceProvider) -> MockScriptProvider { + { sourceProvider in + MockScriptProvider() + } + } + } + let assetsSubject = PassthroughSubject() + + var ucc: UserContentController! + typealias Assets = (contentRuleLists: [String: WKContentRuleList], userScripts: any UserScriptsProvider, updateEvent: ContentBlockerRulesManager.UpdateEvent) + var onAssetsInstalled: ((Assets) -> Void)? + + @MainActor + override func setUp() async throws { + _=WKUserContentController.swizzleContentRuleListsMethodsOnce + ucc = UserContentController(assetsPublisher: assetsSubject, privacyConfigurationManager: MockPrivacyConfigurationManager()) + ucc.delegate = self + } + + func assetsInstalledExpectation(onAssetsInstalled: ((Assets) -> Void)? = nil) -> XCTestExpectation { + let e = expectation(description: "assets installed") + self.onAssetsInstalled = { + onAssetsInstalled?($0) + e.fulfill() + } + return e + } + + // MARK: - Tests + + @MainActor + func testWhenContentBlockingAssetsPublished_contentRuleListsAreInstalled() async throws { + let rules1 = await ContentBlockingRulesHelper().makeFakeRules(name: "list1")! + let rules2 = await ContentBlockingRulesHelper().makeFakeRules(name: "list2")! + + let e = assetsInstalledExpectation { + XCTAssertEqual($0.contentRuleLists, [rules1.name: rules1.rulesList, rules2.name: rules2.rulesList]) + } + assetsSubject.send(NewContent(rulesUpdate: .init(rules: [rules1, rules2], changes: [rules1.name: .all, rules2.name: .all], completionTokens: ["1"]), sourceProvider: MockScriptSourceProvider())) + + await fulfillment(of: [e], timeout: 1) + XCTAssertEqual(ucc.installedContentRuleLists.sorted(by: { $0.identifier < $1.identifier }), [rules1.rulesList, rules2.rulesList]) + } + + @MainActor + func testWhenLocalContentRuleListInstalled_contentRuleListIsInstalled() async throws { + let rules1 = await ContentBlockingRulesHelper().makeFakeRules(name: "list1")! + let rules2 = await ContentBlockingRulesHelper().makeFakeRules(name: "list1")! + let rules3 = await ContentBlockingRulesHelper().makeFakeRules(name: "list2")! + + // initial publish + let e = assetsInstalledExpectation() + assetsSubject.send(NewContent(rulesUpdate: .init(rules: [rules1], changes: [rules1.name: .all], completionTokens: ["1"]), sourceProvider: MockScriptSourceProvider())) + await fulfillment(of: [e], timeout: 1) + + // install 2 local lists + ucc.installLocalContentRuleList(rules2.rulesList, identifier: rules2.name) + ucc.installLocalContentRuleList(rules3.rulesList, identifier: rules3.name) + XCTAssertEqual(ucc.installedContentRuleLists, [rules1.rulesList, rules2.rulesList, rules3.rulesList]) + } + + @MainActor + func testWhenLocalContentRuleListWithExistingIdInstalled_contentRuleListIsReplaced() async throws { + let rules1 = await ContentBlockingRulesHelper().makeFakeRules(name: "list1")! + let rules2 = await ContentBlockingRulesHelper().makeFakeRules(name: "list1")! + let rules3 = await ContentBlockingRulesHelper().makeFakeRules(name: "list1")! + + // initial publish + let e = assetsInstalledExpectation() + assetsSubject.send(NewContent(rulesUpdate: .init(rules: [rules1], changes: [rules1.name: .all], completionTokens: ["1"]), sourceProvider: MockScriptSourceProvider())) + await fulfillment(of: [e], timeout: 1) + + // install 2 local lists with same id + ucc.installLocalContentRuleList(rules2.rulesList, identifier: rules2.name) + ucc.installLocalContentRuleList(rules3.rulesList, identifier: rules2.name) + XCTAssertEqual(ucc.installedContentRuleLists, [rules1.rulesList, rules3.rulesList]) + } + + @MainActor + func testWhenLocalContentRuleListRemoved_contentRuleListIsRemoved() async throws { + let rules1 = await ContentBlockingRulesHelper().makeFakeRules(name: "list1")! + let rules2 = await ContentBlockingRulesHelper().makeFakeRules(name: "list2")! + let rules3 = await ContentBlockingRulesHelper().makeFakeRules(name: "list3")! + + // initial publish + let e = assetsInstalledExpectation { [unowned self] _ in + // install 2 local lists + ucc.installLocalContentRuleList(rules2.rulesList, identifier: rules2.name) + ucc.installLocalContentRuleList(rules3.rulesList, identifier: rules3.name) + } + assetsSubject.send(NewContent(rulesUpdate: .init(rules: [rules1], changes: [rules1.name: .all], completionTokens: ["1"]), sourceProvider: MockScriptSourceProvider())) + await fulfillment(of: [e], timeout: 1) + + ucc.removeLocalContentRuleList(withIdentifier: rules2.name) + ucc.removeLocalContentRuleList(withIdentifier: rules3.name) + + XCTAssertEqual(ucc.installedContentRuleLists, [rules1.rulesList]) + } + + @MainActor + func testWhenGlobalContentRuleListDisabled_itIsRemoved() async throws { + let rules1 = await ContentBlockingRulesHelper().makeFakeRules(name: "list1")! + let rules2 = await ContentBlockingRulesHelper().makeFakeRules(name: "list1")! + + let e = assetsInstalledExpectation { [unowned self] _ in + // install local rule list during the delegate call + ucc.installLocalContentRuleList(rules2.rulesList, identifier: rules2.rulesList.identifier) + // disable global content rule list + try! ucc.disableGlobalContentRuleList(withIdentifier: "list1") + } + assetsSubject.send(NewContent(rulesUpdate: .init(rules: [rules1, rules2], changes: [rules1.name: .all], completionTokens: ["1"]), sourceProvider: MockScriptSourceProvider())) + + await fulfillment(of: [e], timeout: 1) + XCTAssertEqual(ucc.installedContentRuleLists, [rules2.rulesList]) + } + + @MainActor + func testWhenGlobalContentRuleListEnabled_itIsAdded() async throws { + let rules1 = await ContentBlockingRulesHelper().makeFakeRules(name: "list1")! + let rules2 = await ContentBlockingRulesHelper().makeFakeRules(name: "list2")! + + let e = assetsInstalledExpectation { [unowned self] _ in + // disable global content rule lists + try! ucc.disableGlobalContentRuleList(withIdentifier: "list2") + } + assetsSubject.send(NewContent(rulesUpdate: .init(rules: [rules1, rules2], changes: [rules1.name: .all, rules2.name: .all], completionTokens: ["1"]), sourceProvider: MockScriptSourceProvider())) + + await fulfillment(of: [e], timeout: 1) + + // re-enable global content rule list + try ucc.enableGlobalContentRuleList(withIdentifier: "list2") + XCTAssertEqual(ucc.installedContentRuleLists, [rules1.rulesList, rules2.rulesList]) + } + + @MainActor + func testWhenContentBlockingAssetsUpdated_allContentRuleListsAreReistalled() async throws { + let rules1 = await ContentBlockingRulesHelper().makeFakeRules(name: "list1")! + let rules2 = await ContentBlockingRulesHelper().makeFakeRules(name: "list2")! + let rules3 = await ContentBlockingRulesHelper().makeFakeRules(name: "list3")! + let rules4 = await ContentBlockingRulesHelper().makeFakeRules(name: "list4")! + + // initial publish + let e = assetsInstalledExpectation { [unowned self] _ in + ucc.installLocalContentRuleList(rules3.rulesList, identifier: rules3.name) + } + assetsSubject.send(NewContent(rulesUpdate: .init(rules: [rules1, rules2], changes: [rules1.name: .all], completionTokens: ["1"]), sourceProvider: MockScriptSourceProvider())) + await fulfillment(of: [e], timeout: 1) + ucc.installLocalContentRuleList(rules4.rulesList, identifier: rules4.name) + + XCTAssertEqual(ucc.installedContentRuleLists.sorted(by: { $0.identifier < $1.identifier }), [rules1.rulesList, rules2.rulesList, rules3.rulesList, rules4.rulesList]) + + let rules1_1 = await ContentBlockingRulesHelper().makeFakeRules(name: "list1")! + let rules2_1 = await ContentBlockingRulesHelper().makeFakeRules(name: "list2_1")! + let rules3_1 = await ContentBlockingRulesHelper().makeFakeRules(name: "list3")! + let rules4_1 = await ContentBlockingRulesHelper().makeFakeRules(name: "list4_1")! + + let e2 = assetsInstalledExpectation { [unowned self] _ in + ucc.installLocalContentRuleList(rules3_1.rulesList, identifier: rules3_1.name) + } + assetsSubject.send(NewContent(rulesUpdate: .init(rules: [rules1_1, rules2_1], changes: [rules1_1.name: .all], completionTokens: ["1"]), sourceProvider: MockScriptSourceProvider())) + await fulfillment(of: [e2], timeout: 1) + ucc.installLocalContentRuleList(rules4_1.rulesList, identifier: rules4_1.name) + + XCTAssertEqual(ucc.installedContentRuleLists.sorted(by: { $0.identifier < $1.identifier }), [rules1_1.rulesList, rules2_1.rulesList, rules3_1.rulesList, rules4_1.rulesList]) + } + +} + +extension WKUserContentController { + + private static let contentRuleListsKey = UnsafeRawPointer(bitPattern: "contentRuleListsKey".hashValue)! + var installedContentRuleLists: [WKContentRuleList] { + get { + objc_getAssociatedObject(self, Self.contentRuleListsKey) as? [WKContentRuleList] ?? [] + } + set { + objc_setAssociatedObject(self, Self.contentRuleListsKey, newValue, .OBJC_ASSOCIATION_RETAIN) + } + } + + static var swizzleContentRuleListsMethodsOnce: Void = { + let originalAddMethod = class_getInstanceMethod(WKUserContentController.self, NSSelectorFromString("addContentRuleList:"))! + let swizzledAddMethod = class_getInstanceMethod(WKUserContentController.self, #selector(swizzled_addContentRuleList))! + method_exchangeImplementations(originalAddMethod, swizzledAddMethod) + + let originalRemoveMethod = class_getInstanceMethod(WKUserContentController.self, NSSelectorFromString("removeContentRuleList:"))! + let swizzledRemoveMethod = class_getInstanceMethod(WKUserContentController.self, #selector(swizzled_removeContentRuleList))! + method_exchangeImplementations(originalRemoveMethod, swizzledRemoveMethod) + + let originalRemoveAllMethod = class_getInstanceMethod(WKUserContentController.self, #selector(removeAllContentRuleLists))! + let swizzledRemoveAllMethod = class_getInstanceMethod(WKUserContentController.self, #selector(swizzled_removeAllContentRuleLists))! + method_exchangeImplementations(originalRemoveAllMethod, swizzledRemoveAllMethod) + }() + + @objc dynamic private func swizzled_addContentRuleList(_ contentRuleList: WKContentRuleList) { + installedContentRuleLists.append(contentRuleList) + self.swizzled_addContentRuleList(contentRuleList) // call the original + } + + @objc dynamic private func swizzled_removeContentRuleList(_ contentRuleList: WKContentRuleList) { + installedContentRuleLists.remove(at: installedContentRuleLists.firstIndex(of: contentRuleList)!) + self.swizzled_removeContentRuleList(contentRuleList) // call the original + } + + @objc dynamic private func swizzled_removeAllContentRuleLists() { + installedContentRuleLists.removeAll() + self.swizzled_removeAllContentRuleLists() // call the original + } +} + +extension UserContentControllerTests: UserContentControllerDelegate { + func userContentController(_ userContentController: UserContentController, didInstallContentRuleLists contentRuleLists: [String: WKContentRuleList], userScripts: any UserScriptsProvider, updateEvent: ContentBlockerRulesManager.UpdateEvent) { + onAssetsInstalled?((contentRuleLists, userScripts, updateEvent)) + } +} + +class MockPrivacyConfigurationManager: PrivacyConfigurationManaging { + var currentConfig: Data = .init() + var updatesSubject = PassthroughSubject() + let updatesPublisher: AnyPublisher + var privacyConfig: PrivacyConfiguration = MockPrivacyConfiguration() + let internalUserDecider: InternalUserDecider = DefaultInternalUserDecider() + var toggleProtectionsCounter = ToggleProtectionsCounter(eventReporting: EventMapping { _, _, _, _ in }) + func reload(etag: String?, data: Data?) -> PrivacyConfigurationManager.ReloadResult { + .downloaded + } + + init() { + updatesPublisher = updatesSubject.eraseToAnyPublisher() + } +} + +class MockPrivacyConfiguration: PrivacyConfiguration { + + func isEnabled(featureKey: PrivacyFeature, versionProvider: AppVersionProvider) -> Bool { true } + + func stateFor(featureKey: BrowserServicesKit.PrivacyFeature, versionProvider: BrowserServicesKit.AppVersionProvider) -> BrowserServicesKit.PrivacyConfigurationFeatureState { + return .enabled + } + + func isSubfeatureEnabled( + _ subfeature: any PrivacySubfeature, + versionProvider: AppVersionProvider, + randomizer: (Range) -> Double + ) -> Bool { + true + } + + func stateFor(_ subfeature: any PrivacySubfeature, versionProvider: AppVersionProvider, randomizer: (Range) -> Double) -> PrivacyConfigurationFeatureState { + return .enabled + } + + var identifier: String = "abcd" + var userUnprotectedDomains: [String] = [] + var tempUnprotectedDomains: [String] = [] + var trackerAllowlist: PrivacyConfigurationData.TrackerAllowlist = .init(json: ["state": "disabled"])! + func exceptionsList(forFeature featureKey: PrivacyFeature) -> [String] { [] } + func isFeature(_ feature: PrivacyFeature, enabledForDomain: String?) -> Bool { true } + func isProtected(domain: String?) -> Bool { false } + func isUserUnprotected(domain: String?) -> Bool { false } + func isTempUnprotected(domain: String?) -> Bool { false } + func isInExceptionList(domain: String?, forFeature featureKey: PrivacyFeature) -> Bool { false } + func settings(for feature: PrivacyFeature) -> PrivacyConfigurationData.PrivacyFeature.FeatureSettings { .init() } + func userEnabledProtection(forDomain: String) {} + func userDisabledProtection(forDomain: String) {} +} From 9df0476d399a34e4a35b3fe605a91d446fdee1e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jacek=20=C5=81yp?= Date: Wed, 8 May 2024 17:48:37 +0200 Subject: [PATCH 3/7] Rename tests to avoid conflicts (#813) * Rename tests to avoid conflicts --- .../ContentBlocker/UserContentControllerTests.swift | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Tests/BrowserServicesKitTests/ContentBlocker/UserContentControllerTests.swift b/Tests/BrowserServicesKitTests/ContentBlocker/UserContentControllerTests.swift index 3caa3d016..622385ba3 100644 --- a/Tests/BrowserServicesKitTests/ContentBlocker/UserContentControllerTests.swift +++ b/Tests/BrowserServicesKitTests/ContentBlocker/UserContentControllerTests.swift @@ -55,7 +55,7 @@ final class UserContentControllerTests: XCTestCase { @MainActor override func setUp() async throws { _=WKUserContentController.swizzleContentRuleListsMethodsOnce - ucc = UserContentController(assetsPublisher: assetsSubject, privacyConfigurationManager: MockPrivacyConfigurationManager()) + ucc = UserContentController(assetsPublisher: assetsSubject, privacyConfigurationManager: PrivacyConfigurationManagerMock()) ucc.delegate = self } @@ -256,11 +256,11 @@ extension UserContentControllerTests: UserContentControllerDelegate { } } -class MockPrivacyConfigurationManager: PrivacyConfigurationManaging { +class PrivacyConfigurationManagerMock: PrivacyConfigurationManaging { var currentConfig: Data = .init() var updatesSubject = PassthroughSubject() let updatesPublisher: AnyPublisher - var privacyConfig: PrivacyConfiguration = MockPrivacyConfiguration() + var privacyConfig: PrivacyConfiguration = PrivacyConfigurationMock() let internalUserDecider: InternalUserDecider = DefaultInternalUserDecider() var toggleProtectionsCounter = ToggleProtectionsCounter(eventReporting: EventMapping { _, _, _, _ in }) func reload(etag: String?, data: Data?) -> PrivacyConfigurationManager.ReloadResult { @@ -272,7 +272,7 @@ class MockPrivacyConfigurationManager: PrivacyConfigurationManaging { } } -class MockPrivacyConfiguration: PrivacyConfiguration { +class PrivacyConfigurationMock: PrivacyConfiguration { func isEnabled(featureKey: PrivacyFeature, versionProvider: AppVersionProvider) -> Bool { true } From 8b8092db4bcca19c0fa959a517c004900753644e Mon Sep 17 00:00:00 2001 From: Christopher Brind Date: Thu, 9 May 2024 11:27:41 +0100 Subject: [PATCH 4/7] on iOS allow bookmarks in top hits (#812) Required: Task/Issue URL: https://app.asana.com/0/0/1207251449708624/f iOS PR: duckduckgo/iOS#2835 macOS PR: duckduckgo/macos-browser#2754 What kind of version bump will this require?: Major/Minor/Patch Optional: Tech Design URL: CC: Description: Bookmarks are added to top hits on iOS, but the behaviour is unchanged on macOS. Steps to test this PR: iOS On iOS launch the app (use the md variant by editing the scheme > run > check environment variable VARIANT is md and enabled Add some bookmarks Start typing in the address bar The bookmarks should appear in the top hits section (ie at the top of the suggestions list) Check that launching the app clean with no variant still works as expected. macOS Launch the app and confirm that suggestions works as it did previously. ie Bookmarks should not be in top hits unless they have been visited several times. --- Sources/Suggestions/Suggestion.swift | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Sources/Suggestions/Suggestion.swift b/Sources/Suggestions/Suggestion.swift index 349bf8aea..cca036f4c 100644 --- a/Sources/Suggestions/Suggestion.swift +++ b/Sources/Suggestions/Suggestion.swift @@ -70,10 +70,17 @@ extension Suggestion { init?(bookmark: Bookmark) { guard let urlObject = URL(string: bookmark.url) else { return nil } + #if os(macOS) self = .bookmark(title: bookmark.title, url: urlObject, isFavorite: bookmark.isFavorite, allowedInTopHits: bookmark.isFavorite) + #else + self = .bookmark(title: bookmark.title, + url: urlObject, + isFavorite: bookmark.isFavorite, + allowedInTopHits: true) + #endif } init(historyEntry: HistorySuggestion) { From 6ceabf1d257ff1d1164afb5b9139f9f20baf0c6e Mon Sep 17 00:00:00 2001 From: amddg44 Date: Thu, 9 May 2024 15:48:14 +0200 Subject: [PATCH 5/7] Password manager survey update (#811) Task/Issue URL: https://app.asana.com/0/0/1206567467430495/f iOS PR: duckduckgo/iOS#2834 macOS PR: duckduckgo/macos-browser#2753 What kind of version bump will this require?: Patch Description: Updates related to temporary survey in the clients apps passwords screens --- Package.resolved | 4 +-- Package.swift | 2 +- .../AutofillDatabaseProvider.swift | 8 +++++ .../SecureVault/AutofillSecureVault.swift | 29 +++++++++++++++++++ .../MockAutofillDatabaseProvider.swift | 4 +++ 5 files changed, 44 insertions(+), 3 deletions(-) diff --git a/Package.resolved b/Package.resolved index 491a98c2c..dadbd9a5c 100644 --- a/Package.resolved +++ b/Package.resolved @@ -23,8 +23,8 @@ "kind" : "remoteSourceControl", "location" : "https://github.com/duckduckgo/duckduckgo-autofill.git", "state" : { - "revision" : "6053999d6af384a716ab0ce7205dbab5d70ed1b3", - "version" : "11.0.1" + "revision" : "10aeff1ec7f533d1705233a9b14f9393a699b1c0", + "version" : "11.0.2" } }, { diff --git a/Package.swift b/Package.swift index 725131db6..8cd51ddc5 100644 --- a/Package.swift +++ b/Package.swift @@ -38,7 +38,7 @@ let package = Package( .library(name: "PixelKitTestingUtilities", targets: ["PixelKitTestingUtilities"]), ], dependencies: [ - .package(url: "https://github.com/duckduckgo/duckduckgo-autofill.git", exact: "11.0.1"), + .package(url: "https://github.com/duckduckgo/duckduckgo-autofill.git", exact: "11.0.2"), .package(url: "https://github.com/duckduckgo/GRDB.swift.git", exact: "2.3.0"), .package(url: "https://github.com/duckduckgo/TrackerRadarKit", exact: "2.0.0"), .package(url: "https://github.com/duckduckgo/sync_crypto", exact: "0.2.0"), diff --git a/Sources/BrowserServicesKit/SecureVault/AutofillDatabaseProvider.swift b/Sources/BrowserServicesKit/SecureVault/AutofillDatabaseProvider.swift index a03e551f8..73470a359 100644 --- a/Sources/BrowserServicesKit/SecureVault/AutofillDatabaseProvider.swift +++ b/Sources/BrowserServicesKit/SecureVault/AutofillDatabaseProvider.swift @@ -26,6 +26,7 @@ import SecureStorage public protocol AutofillDatabaseProvider: SecureStorageDatabaseProvider { func accounts() throws -> [SecureVaultModels.WebsiteAccount] + func accountsCount() throws -> Int func hasAccountFor(username: String?, domain: String?) throws -> Bool @discardableResult @@ -119,6 +120,13 @@ public final class DefaultAutofillDatabaseProvider: GRDBSecureStorageDatabasePro } } + public func accountsCount() throws -> Int { + let count = try db.read { + try SecureVaultModels.WebsiteAccount.fetchCount($0) + } + return count + } + public func hasAccountFor(username: String?, domain: String?) throws -> Bool { let account = try db.read { try SecureVaultModels.WebsiteAccount diff --git a/Sources/BrowserServicesKit/SecureVault/AutofillSecureVault.swift b/Sources/BrowserServicesKit/SecureVault/AutofillSecureVault.swift index 865c54268..498a7a2b7 100644 --- a/Sources/BrowserServicesKit/SecureVault/AutofillSecureVault.swift +++ b/Sources/BrowserServicesKit/SecureVault/AutofillSecureVault.swift @@ -54,6 +54,8 @@ public protocol AutofillSecureVault: SecureVault { func resetL2Password(oldPassword: Data?, newPassword: Data) throws func accounts() throws -> [SecureVaultModels.WebsiteAccount] + func accountsCount() throws -> Int + func accountsCountBucket() throws -> String func accountsFor(domain: String) throws -> [SecureVaultModels.WebsiteAccount] func accountsWithPartialMatchesFor(eTLDplus1: String) throws -> [SecureVaultModels.WebsiteAccount] func hasAccountFor(username: String?, domain: String?) throws -> Bool @@ -228,6 +230,33 @@ public class DefaultAutofillSecureVault: AutofillSe } } + public func accountsCount() throws -> Int { + lock.lock() + defer { + lock.unlock() + } + + do { + return try self.providers.database.accountsCount() + } catch { + throw SecureStorageError.databaseError(cause: error) + } + } + + public func accountsCountBucket() throws -> String { + let accountsCount = try accountsCount() + + if accountsCount < 3 { + return "none" + } else if accountsCount < 11 { + return "some" + } else if accountsCount < 50 { + return "many" + } else { + return "lots" + } + } + public func accountsFor(domain: String) throws -> [SecureVaultModels.WebsiteAccount] { lock.lock() defer { diff --git a/Tests/BrowserServicesKitTests/SecureVault/MockAutofillDatabaseProvider.swift b/Tests/BrowserServicesKitTests/SecureVault/MockAutofillDatabaseProvider.swift index de4613553..7e674a1bf 100644 --- a/Tests/BrowserServicesKitTests/SecureVault/MockAutofillDatabaseProvider.swift +++ b/Tests/BrowserServicesKitTests/SecureVault/MockAutofillDatabaseProvider.swift @@ -96,6 +96,10 @@ internal class MockAutofillDatabaseProvider: AutofillDatabaseProvider { return _accounts } + func accountsCount() throws -> Int { + return _accounts.count + } + func notes() throws -> [SecureVaultModels.Note] { return _notes } From 49d68429a3b8679d2ddc836e8eb0522bc68b46a2 Mon Sep 17 00:00:00 2001 From: Sean Barag Date: Fri, 10 May 2024 03:11:03 -0700 Subject: [PATCH 6/7] autofill: send current language in runtime config (#808) Task/Issue URL: https://app.asana.com/0/11984721910118/1206933877192910/f iOS PR: N/A macOS PR: N/A What kind of version bump will this require?: Patch. This is backwards-compatible, and the autofill codebase falls back to 'en' when it doesn't receive a language. Description: The HTML autofill UI will be translated shortly, but doing so is only possible once that UI knows which language to use. Send the current locale's two-character language code as part of the privacy config (aka 'runtime config' in the autofill codebase). This is a backwards-compatible change, since 'language' is currently an optional input to the autofill HTML pages. --- .../ContentScopeScript/ContentScopeUserScript.swift | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/Sources/BrowserServicesKit/ContentScopeScript/ContentScopeUserScript.swift b/Sources/BrowserServicesKit/ContentScopeScript/ContentScopeUserScript.swift index ad7c306d7..32a51d285 100644 --- a/Sources/BrowserServicesKit/ContentScopeScript/ContentScopeUserScript.swift +++ b/Sources/BrowserServicesKit/ContentScopeScript/ContentScopeUserScript.swift @@ -27,16 +27,29 @@ public final class ContentScopeProperties: Encodable { public let globalPrivacyControlValue: Bool public let debug: Bool = false public let sessionKey: String + public let languageCode: String public let platform = ContentScopePlatform() public let features: [String: ContentScopeFeature] public init(gpcEnabled: Bool, sessionKey: String, featureToggles: ContentScopeFeatureToggles) { self.globalPrivacyControlValue = gpcEnabled self.sessionKey = sessionKey + languageCode = Locale.current.languageCode ?? "en" features = [ "autofill": ContentScopeFeature(featureToggles: featureToggles) ] } + + enum CodingKeys: String, CodingKey { + // Rename 'languageCode' to 'language' to conform to autofill.js's interface. + case languageCode = "language" + + case globalPrivacyControlValue + case debug + case sessionKey + case platform + case features + } } public struct ContentScopeFeature: Encodable { From 72be4e73360989af170399bc063fd5c628e1e84c Mon Sep 17 00:00:00 2001 From: Dominik Kapusta Date: Fri, 10 May 2024 14:41:56 +0200 Subject: [PATCH 7/7] Enable gzip compression for Sync PATCH payloads (#803) Task/Issue URL: https://app.asana.com/0/0/1206919211758354/f Description: This change adds gzip compression to all Sync data PATCH requests. --- Package.resolved | 9 +++ Package.swift | 2 + Sources/DDGSync/SyncError.swift | 4 + .../internal/ProductionDependencies.swift | 2 + .../DDGSync/internal/SyncDependencies.swift | 1 + .../internal/SyncGzipPayloadCompressor.swift | 51 +++++++++++++ Sources/DDGSync/internal/SyncOperation.swift | 12 ++- Sources/DDGSync/internal/SyncQueue.swift | 4 +- .../DDGSync/internal/SyncRequestMaker.swift | 24 +++++- Tests/DDGSyncTests/Mocks/Mocks.swift | 41 ++++++++++ Tests/DDGSyncTests/SyncOperationTests.swift | 74 +++++++++++++++++-- Tests/DDGSyncTests/SyncQueueTests.swift | 31 +++++++- 12 files changed, 239 insertions(+), 16 deletions(-) create mode 100644 Sources/DDGSync/internal/SyncGzipPayloadCompressor.swift diff --git a/Package.resolved b/Package.resolved index dadbd9a5c..2a423b3a6 100644 --- a/Package.resolved +++ b/Package.resolved @@ -36,6 +36,15 @@ "version" : "2.3.0" } }, + { + "identity" : "gzipswift", + "kind" : "remoteSourceControl", + "location" : "https://github.com/1024jp/GzipSwift.git", + "state" : { + "revision" : "731037f6cc2be2ec01562f6597c1d0aa3fe6fd05", + "version" : "6.0.1" + } + }, { "identity" : "privacy-dashboard", "kind" : "remoteSourceControl", diff --git a/Package.swift b/Package.swift index 8cd51ddc5..ebf53737c 100644 --- a/Package.swift +++ b/Package.swift @@ -48,6 +48,7 @@ let package = Package( .package(url: "https://github.com/httpswift/swifter.git", exact: "1.5.0"), .package(url: "https://github.com/duckduckgo/bloom_cpp.git", exact: "3.0.0"), .package(url: "https://github.com/duckduckgo/wireguard-apple", exact: "1.1.3"), + .package(url: "https://github.com/1024jp/GzipSwift.git", exact: "6.0.1") ], targets: [ .target( @@ -153,6 +154,7 @@ let package = Package( "BrowserServicesKit", "Common", .product(name: "DDGSyncCrypto", package: "sync_crypto"), + .product(name: "Gzip", package: "GzipSwift"), "Networking", ], resources: [ diff --git a/Sources/DDGSync/SyncError.swift b/Sources/DDGSync/SyncError.swift index c1cc51f61..ef3faad38 100644 --- a/Sources/DDGSync/SyncError.swift +++ b/Sources/DDGSync/SyncError.swift @@ -57,6 +57,7 @@ public enum SyncError: Error, Equatable { case settingsMetadataNotPresent case unauthenticatedWhileLoggedIn + case patchPayloadCompressionFailed(_ errorCode: Int) public var isServerError: Bool { switch self { @@ -137,6 +138,8 @@ public enum SyncError: Error, Equatable { return [syncErrorString: "settingsMetadataNotPresent"] case .unauthenticatedWhileLoggedIn: return [syncErrorString: "unauthenticatedWhileLoggedIn"] + case .patchPayloadCompressionFailed: + return [syncErrorString: "patchPayloadCompressionFailed"] } } } @@ -183,6 +186,7 @@ extension SyncError: CustomNSError { case .emailProtectionUsernamePresentButTokenMissing: return 26 case .settingsMetadataNotPresent: return 27 case .unauthenticatedWhileLoggedIn: return 28 + case .patchPayloadCompressionFailed: return 29 } } diff --git a/Sources/DDGSync/internal/ProductionDependencies.swift b/Sources/DDGSync/internal/ProductionDependencies.swift index 66724039e..720b91ce3 100644 --- a/Sources/DDGSync/internal/ProductionDependencies.swift +++ b/Sources/DDGSync/internal/ProductionDependencies.swift @@ -27,6 +27,7 @@ struct ProductionDependencies: SyncDependencies { let endpoints: Endpoints let account: AccountManaging let api: RemoteAPIRequestCreating + let payloadCompressor: SyncPayloadCompressing var keyValueStore: KeyValueStoring let secureStore: SecureStoring let crypter: CryptingInternal @@ -72,6 +73,7 @@ struct ProductionDependencies: SyncDependencies { self.getLog = log api = RemoteAPIRequestCreator(log: log()) + payloadCompressor = SyncGzipPayloadCompressor() crypter = Crypter(secureStore: secureStore) account = AccountManager(endpoints: endpoints, api: api, crypter: crypter) diff --git a/Sources/DDGSync/internal/SyncDependencies.swift b/Sources/DDGSync/internal/SyncDependencies.swift index 6e5740c0d..06c1bd390 100644 --- a/Sources/DDGSync/internal/SyncDependencies.swift +++ b/Sources/DDGSync/internal/SyncDependencies.swift @@ -31,6 +31,7 @@ protocol SyncDependencies: SyncDependenciesDebuggingSupport { var endpoints: Endpoints { get } var account: AccountManaging { get } var api: RemoteAPIRequestCreating { get } + var payloadCompressor: SyncPayloadCompressing { get } var keyValueStore: KeyValueStoring { get } var secureStore: SecureStoring { get } var crypter: CryptingInternal { get } diff --git a/Sources/DDGSync/internal/SyncGzipPayloadCompressor.swift b/Sources/DDGSync/internal/SyncGzipPayloadCompressor.swift new file mode 100644 index 000000000..fa295e9e8 --- /dev/null +++ b/Sources/DDGSync/internal/SyncGzipPayloadCompressor.swift @@ -0,0 +1,51 @@ +// +// SyncGzipPayloadCompressor.swift +// +// Copyright Β© 2024 DuckDuckGo. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +import Foundation +import Gzip + +protocol SyncPayloadCompressing { + func compress(_ payload: Data) throws -> Data +} + +struct SyncGzipPayloadCompressor: SyncPayloadCompressing { + func compress(_ payload: Data) throws -> Data { + try payload.gzipped() + } +} + +extension GzipError { + /// Mapping is taken from `GzipError.Kind` documentation which maps zlib error codes to enum cases, + /// and we're effectively reversing that mapping here. + var errorCode: Int { + switch kind { + case .stream: + return -2 + case .data: + return -3 + case .memory: + return -4 + case .buffer: + return -5 + case .version: + return -6 + case .unknown(let code): + return code + } + } +} diff --git a/Sources/DDGSync/internal/SyncOperation.swift b/Sources/DDGSync/internal/SyncOperation.swift index b4e84e756..ee1fa9669 100644 --- a/Sources/DDGSync/internal/SyncOperation.swift +++ b/Sources/DDGSync/internal/SyncOperation.swift @@ -19,6 +19,7 @@ import Foundation import Combine import Common +import Gzip final class SyncOperation: Operation { @@ -143,7 +144,7 @@ final class SyncOperation: Operation { try checkCancellation() let syncRequest = try await self.makeSyncRequest(for: dataProvider, fetchOnly: fetchOnly) let clientTimestamp = Date() - let httpRequest = try self.makeHTTPRequest(with: syncRequest, timestamp: clientTimestamp) + let httpRequest = try self.makeHTTPRequest(for: dataProvider, with: syncRequest, timestamp: clientTimestamp) try checkCancellation() let httpResult: HTTPResult = try await httpRequest.execute() @@ -211,10 +212,15 @@ final class SyncOperation: Operation { return SyncRequest(feature: dataProvider.feature, previousSyncTimestamp: dataProvider.lastSyncTimestamp, sent: localChanges) } - private func makeHTTPRequest(with syncRequest: SyncRequest, timestamp: Date) throws -> HTTPRequesting { + private func makeHTTPRequest(for dataProvider: DataProviding, with syncRequest: SyncRequest, timestamp: Date) throws -> HTTPRequesting { let hasLocalChanges = !syncRequest.sent.isEmpty if hasLocalChanges { - return try requestMaker.makePatchRequest(with: syncRequest, clientTimestamp: timestamp) + do { + return try requestMaker.makePatchRequest(with: syncRequest, clientTimestamp: timestamp, isCompressed: true) + } catch let error as GzipError { + dataProvider.handleSyncError(SyncError.patchPayloadCompressionFailed(error.errorCode)) + return try requestMaker.makePatchRequest(with: syncRequest, clientTimestamp: timestamp, isCompressed: false) + } } return try requestMaker.makeGetRequest(with: syncRequest) } diff --git a/Sources/DDGSync/internal/SyncQueue.swift b/Sources/DDGSync/internal/SyncQueue.swift index fb6e4d22f..9bd4b9042 100644 --- a/Sources/DDGSync/internal/SyncQueue.swift +++ b/Sources/DDGSync/internal/SyncQueue.swift @@ -69,6 +69,7 @@ final class SyncQueue { crypter: dependencies.crypter, api: dependencies.api, endpoints: dependencies.endpoints, + payloadCompressor: dependencies.payloadCompressor, log: dependencies.log ) } @@ -79,13 +80,14 @@ final class SyncQueue { crypter: Crypting, api: RemoteAPIRequestCreating, endpoints: Endpoints, + payloadCompressor: SyncPayloadCompressing, log: @escaping @autoclosure () -> OSLog = .disabled ) { self.dataProviders = dataProviders self.storage = storage self.crypter = crypter self.getLog = log - requestMaker = SyncRequestMaker(storage: storage, api: api, endpoints: endpoints) + requestMaker = SyncRequestMaker(storage: storage, api: api, endpoints: endpoints, payloadCompressor: payloadCompressor) syncDidFinishPublisher = syncDidFinishSubject.eraseToAnyPublisher() syncHTTPRequestErrorPublisher = syncHTTPRequestErrorSubject.eraseToAnyPublisher() isSyncInProgressPublisher = Publishers diff --git a/Sources/DDGSync/internal/SyncRequestMaker.swift b/Sources/DDGSync/internal/SyncRequestMaker.swift index 12382e667..b814c16e5 100644 --- a/Sources/DDGSync/internal/SyncRequestMaker.swift +++ b/Sources/DDGSync/internal/SyncRequestMaker.swift @@ -17,16 +17,18 @@ // import Foundation +import Gzip protocol SyncRequestMaking { func makeGetRequest(with result: SyncRequest) throws -> HTTPRequesting - func makePatchRequest(with result: SyncRequest, clientTimestamp: Date) throws -> HTTPRequesting + func makePatchRequest(with result: SyncRequest, clientTimestamp: Date, isCompressed: Bool) throws -> HTTPRequesting } struct SyncRequestMaker: SyncRequestMaking { let storage: SecureStoring let api: RemoteAPIRequestCreating let endpoints: Endpoints + let payloadCompressor: SyncPayloadCompressing let dateFormatter = ISO8601DateFormatter() func makeGetRequest(with result: SyncRequest) throws -> HTTPRequesting { @@ -41,7 +43,7 @@ struct SyncRequestMaker: SyncRequestMaking { return api.createAuthenticatedGetRequest(url: url, authToken: try getToken(), parameters: parameters) } - func makePatchRequest(with result: SyncRequest, clientTimestamp: Date) throws -> HTTPRequesting { + func makePatchRequest(with result: SyncRequest, clientTimestamp: Date, isCompressed: Bool) throws -> HTTPRequesting { var json = [String: Any]() let modelPayload: [String: Any?] = [ "updates": result.sent.map(\.payload), @@ -55,7 +57,23 @@ struct SyncRequestMaker: SyncRequestMaking { } let body = try JSONSerialization.data(withJSONObject: json, options: []) - return api.createAuthenticatedJSONRequest(url: endpoints.syncPatch, method: .PATCH, authToken: try getToken(), json: body) + + guard isCompressed else { + return api.createAuthenticatedJSONRequest( + url: endpoints.syncPatch, + method: .PATCH, + authToken: try getToken(), + json: body + ) + } + + let compressedBody = try payloadCompressor.compress(body) + return api.createAuthenticatedJSONRequest( + url: endpoints.syncPatch, + method: .PATCH, + authToken: try getToken(), + json: compressedBody, + headers: ["Content-Encoding": "gzip"]) } private func getToken() throws -> String { diff --git a/Tests/DDGSyncTests/Mocks/Mocks.swift b/Tests/DDGSyncTests/Mocks/Mocks.swift index 301e7cd7e..b4cd4e896 100644 --- a/Tests/DDGSyncTests/Mocks/Mocks.swift +++ b/Tests/DDGSyncTests/Mocks/Mocks.swift @@ -20,6 +20,7 @@ import BrowserServicesKit import Combine import Common import Foundation +import Gzip import Persistence import TestUtils @@ -199,6 +200,7 @@ struct MockSyncDependencies: SyncDependencies, SyncDependenciesDebuggingSupport var endpoints: Endpoints = Endpoints(baseURL: URL(string: "https://dev.null")!) var account: AccountManaging = AccountManagingMock() var api: RemoteAPIRequestCreating = RemoteAPIRequestCreatingMock() + var payloadCompressor: SyncPayloadCompressing = SyncGzipPayloadCompressorMock() var secureStore: SecureStoring = SecureStorageStub() var crypter: CryptingInternal = CryptingMock() var scheduler: SchedulingInternal = SchedulerMock() @@ -276,6 +278,45 @@ class RemoteAPIRequestCreatingMock: RemoteAPIRequestCreating { } } +class InspectableSyncRequestMaker: SyncRequestMaking { + + struct MakePatchRequestCallArgs { + let result: SyncRequest + let clientTimestamp: Date + let isCompressed: Bool + } + + func makeGetRequest(with result: SyncRequest) throws -> HTTPRequesting { + try requestMaker.makeGetRequest(with: result) + } + + func makePatchRequest(with result: SyncRequest, clientTimestamp: Date, isCompressed: Bool) throws -> HTTPRequesting { + makePatchRequestCallCount += 1 + makePatchRequestCallArgs.append(.init(result: result, clientTimestamp: clientTimestamp, isCompressed: isCompressed)) + return try requestMaker.makePatchRequest(with: result, clientTimestamp: clientTimestamp, isCompressed: isCompressed) + } + + let requestMaker: SyncRequestMaker + + init(requestMaker: SyncRequestMaker) { + self.requestMaker = requestMaker + } + + var makePatchRequestCallCount = 0 + var makePatchRequestCallArgs: [MakePatchRequestCallArgs] = [] +} + +class SyncGzipPayloadCompressorMock: SyncPayloadCompressing { + var error: Error? + + func compress(_ payload: Data) throws -> Data { + if let error { + throw error + } + return try payload.gzipped() + } +} + struct CryptingMock: CryptingInternal { var _encryptAndBase64Encode: (String) throws -> String = { "encrypted_\($0)" } var _base64DecodeAndDecrypt: (String) throws -> String = { $0.dropping(prefix: "encrypted_") } diff --git a/Tests/DDGSyncTests/SyncOperationTests.swift b/Tests/DDGSyncTests/SyncOperationTests.swift index e76df782d..0a90466e8 100644 --- a/Tests/DDGSyncTests/SyncOperationTests.swift +++ b/Tests/DDGSyncTests/SyncOperationTests.swift @@ -18,15 +18,17 @@ import XCTest +@testable import Gzip @testable import DDGSync class SyncOperationTests: XCTestCase { var apiMock: RemoteAPIRequestCreatingMock! var request: HTTPRequestingMock! var endpoints: Endpoints! + var payloadCompressor: SyncGzipPayloadCompressorMock! var storage: SecureStorageStub! var crypter: CryptingMock! - var requestMaker: SyncRequestMaking! + var requestMaker: InspectableSyncRequestMaker! override func setUpWithError() throws { try super.setUpWithError() @@ -35,6 +37,7 @@ class SyncOperationTests: XCTestCase { request = HTTPRequestingMock() apiMock.request = request endpoints = Endpoints(baseURL: URL(string: "https://example.com")!) + payloadCompressor = SyncGzipPayloadCompressorMock() storage = SecureStorageStub() crypter = CryptingMock() try storage.persistAccount( @@ -50,7 +53,9 @@ class SyncOperationTests: XCTestCase { ) ) - requestMaker = SyncRequestMaker(storage: storage, api: apiMock, endpoints: endpoints) + requestMaker = InspectableSyncRequestMaker( + requestMaker: .init(storage: storage, api: apiMock, endpoints: endpoints, payloadCompressor: payloadCompressor) + ) } func testWhenThereAreNoChangesThenGetRequestIsFired() async throws { @@ -167,16 +172,16 @@ class SyncOperationTests: XCTestCase { for body in bodies.compactMap({$0}) { do { - let payload = try JSONDecoder.snakeCaseKeys.decode(BookmarksPayload.self, from: body) + let payload = try JSONDecoder.snakeCaseKeys.decode(BookmarksPayload.self, from: body.gunzipped()) XCTAssertEqual(payload, bookmarks) payloadCount -= 1 } catch { do { - let payload = try JSONDecoder.snakeCaseKeys.decode(SettingsPayload.self, from: body) + let payload = try JSONDecoder.snakeCaseKeys.decode(SettingsPayload.self, from: body.gunzipped()) XCTAssertEqual(payload, settings) payloadCount -= 1 } catch { - let payload = try JSONDecoder.snakeCaseKeys.decode(AutofillPayload.self, from: body) + let payload = try JSONDecoder.snakeCaseKeys.decode(AutofillPayload.self, from: body.gunzipped()) XCTAssertEqual(payload, autofill) payloadCount -= 1 } @@ -245,6 +250,65 @@ class SyncOperationTests: XCTestCase { XCTAssertTrue(try sentModels.isJSONRepresentationEquivalent(to: objectsToSync)) } + + func testWhenPatchPayloadCompressionSucceedsThenPayloadIsSentCompressed() async throws { + let objectsToSync = [ + Syncable(jsonObject: ["id": "1", "name": "bookmark1", "url": "https://example.com"]), + ] + let dataProvider = DataProvidingMock(feature: .init(name: "bookmarks")) + var sentModels: [Syncable] = [] + var errors: [any Error] = [] + dataProvider.updateSyncTimestamps(server: "1234", local: nil) + dataProvider._fetchChangedObjects = { _ in objectsToSync } + dataProvider._handleSyncError = { errors.append($0) } + dataProvider.handleSyncResponse = { sent, _, _, _, _ in + sentModels = sent + } + + let syncOperation = SyncOperation(dataProviders: [dataProvider], storage: storage, crypter: crypter, requestMaker: requestMaker) + request.result = .init(data: nil, response: HTTPURLResponse(url: URL(string: "https://example.com")!, statusCode: 304, httpVersion: nil, headerFields: nil)!) + + try await syncOperation.sync(fetchOnly: false) + + XCTAssertTrue(try sentModels.isJSONRepresentationEquivalent(to: objectsToSync)) + XCTAssertEqual(requestMaker.makePatchRequestCallCount, 1) + XCTAssertEqual(requestMaker.makePatchRequestCallArgs[0].isCompressed, true) + XCTAssertTrue(errors.isEmpty) + } + + func testWhenPatchPayloadCompressionFailsThenPayloadIsSentUncompressed() async throws { + let errorCode = 100200300 + payloadCompressor.error = GzipError(code: Int32(errorCode), msg: nil) + + let objectsToSync = [ + Syncable(jsonObject: ["id": "1", "name": "bookmark1", "url": "https://example.com"]), + ] + let dataProvider = DataProvidingMock(feature: .init(name: "bookmarks")) + var sentModels: [Syncable] = [] + var errors: [any Error] = [] + dataProvider.updateSyncTimestamps(server: "1234", local: nil) + dataProvider._fetchChangedObjects = { _ in objectsToSync } + dataProvider._handleSyncError = { errors.append($0) } + dataProvider.handleSyncResponse = { sent, _, _, _, _ in + sentModels = sent + } + + let syncOperation = SyncOperation(dataProviders: [dataProvider], storage: storage, crypter: crypter, requestMaker: requestMaker) + request.result = .init(data: nil, response: HTTPURLResponse(url: URL(string: "https://example.com")!, statusCode: 304, httpVersion: nil, headerFields: nil)!) + + try await syncOperation.sync(fetchOnly: false) + + XCTAssertTrue(try sentModels.isJSONRepresentationEquivalent(to: objectsToSync)) + XCTAssertEqual(requestMaker.makePatchRequestCallCount, 2) + XCTAssertEqual(requestMaker.makePatchRequestCallArgs[0].isCompressed, true) + XCTAssertEqual(requestMaker.makePatchRequestCallArgs[1].isCompressed, false) + XCTAssertEqual(errors.count, 1) + let error = try XCTUnwrap(errors.first) + guard case SyncError.patchPayloadCompressionFailed(errorCode) = error else { + XCTFail("Unexpected error thrown: \(error)") + return + } + } } private extension Array where Element == Syncable { diff --git a/Tests/DDGSyncTests/SyncQueueTests.swift b/Tests/DDGSyncTests/SyncQueueTests.swift index 202e3bd06..dafe96736 100644 --- a/Tests/DDGSyncTests/SyncQueueTests.swift +++ b/Tests/DDGSyncTests/SyncQueueTests.swift @@ -27,6 +27,7 @@ class SyncQueueTests: XCTestCase { var storage: SecureStorageStub! var crypter: CryptingMock! var requestMaker: SyncRequestMaking! + var payloadCompressor: SyncPayloadCompressing! override func setUpWithError() throws { try super.setUpWithError() @@ -34,6 +35,7 @@ class SyncQueueTests: XCTestCase { apiMock = RemoteAPIRequestCreatingMock() request = HTTPRequestingMock() apiMock.request = request + payloadCompressor = SyncGzipPayloadCompressorMock() endpoints = Endpoints(baseURL: URL(string: "https://example.com")!) storage = SecureStorageStub() crypter = CryptingMock() @@ -50,11 +52,18 @@ class SyncQueueTests: XCTestCase { ) ) - requestMaker = SyncRequestMaker(storage: storage, api: apiMock, endpoints: endpoints) + requestMaker = SyncRequestMaker(storage: storage, api: apiMock, endpoints: endpoints, payloadCompressor: payloadCompressor) } func testWhenDataSyncingFeatureFlagIsDisabledThenNewOperationsAreNotEnqueued() async { - let syncQueue = SyncQueue(dataProviders: [], storage: storage, crypter: crypter, api: apiMock, endpoints: endpoints) + let syncQueue = SyncQueue( + dataProviders: [], + storage: storage, + crypter: crypter, + api: apiMock, + endpoints: endpoints, + payloadCompressor: payloadCompressor + ) XCTAssertFalse(syncQueue.operationQueue.isSuspended) var syncDidStartEvents = [Bool]() @@ -81,7 +90,14 @@ class SyncQueueTests: XCTestCase { func testThatInProgressPublisherEmitsValuesWhenSyncStartsAndEndsWithSuccess() async throws { let feature = Feature(name: "bookmarks") let dataProvider = DataProvidingMock(feature: feature) - let syncQueue = SyncQueue(dataProviders: [dataProvider], storage: storage, crypter: crypter, api: apiMock, endpoints: endpoints) + let syncQueue = SyncQueue( + dataProviders: [dataProvider], + storage: storage, + crypter: crypter, + api: apiMock, + endpoints: endpoints, + payloadCompressor: payloadCompressor + ) var isInProgressEvents = [Bool]() @@ -99,7 +115,14 @@ class SyncQueueTests: XCTestCase { func testThatInProgressPublisherEmitsValuesWhenSyncStartsAndEndsWithError() async throws { let feature = Feature(name: "bookmarks") let dataProvider = DataProvidingMock(feature: feature) - let syncQueue = SyncQueue(dataProviders: [dataProvider], storage: storage, crypter: crypter, api: apiMock, endpoints: endpoints) + let syncQueue = SyncQueue( + dataProviders: [dataProvider], + storage: storage, + crypter: crypter, + api: apiMock, + endpoints: endpoints, + payloadCompressor: payloadCompressor + ) var isInProgressEvents = [Bool]()