From 65c9dd3b4009aa4bfbd4350d7d7b365f3ff8400d Mon Sep 17 00:00:00 2001 From: Andy Uhnak Date: Tue, 14 Feb 2023 17:03:25 +0000 Subject: [PATCH 1/2] CryptoSDK phased rollout feature --- Config/CommonConfiguration.swift | 10 +- Config/CryptoSDKConfiguration.swift | 42 -------- Podfile | 2 +- Podfile.lock | 8 +- Riot/Experiments/CryptoSDKFeature.swift | 97 +++++++++++++++++++ Riot/Experiments/Experiment.swift | 50 ++++++++++ Riot/Experiments/PhasedRolloutFeature.swift | 46 +++++++++ .../RemoteFeaturesClientProtocol.swift | 22 +++++ Riot/Modules/Analytics/Analytics.swift | 2 +- .../Analytics/PostHogAnalyticsClient.swift | 8 ++ Riot/Modules/Application/LegacyAppDelegate.m | 2 +- .../Modules/Settings/SettingsViewController.m | 30 +++--- RiotNSE/NotificationService.swift | 17 ++-- RiotNSE/target.yml | 1 + RiotShareExtension/target.yml | 1 + .../Experiments/CryptoSDKFeatureTests.swift | 79 +++++++++++++++ RiotTests/Experiments/ExperimentTests.swift | 71 ++++++++++++++ .../PhasedRolloutFeatureTests.swift | 55 +++++++++++ RiotTests/target.yml | 2 - SiriIntents/target.yml | 1 + changelog.d/pr-7374.change | 1 + 21 files changed, 463 insertions(+), 84 deletions(-) delete mode 100644 Config/CryptoSDKConfiguration.swift create mode 100644 Riot/Experiments/CryptoSDKFeature.swift create mode 100644 Riot/Experiments/Experiment.swift create mode 100644 Riot/Experiments/PhasedRolloutFeature.swift create mode 100644 Riot/Experiments/RemoteFeaturesClientProtocol.swift create mode 100644 RiotTests/Experiments/CryptoSDKFeatureTests.swift create mode 100644 RiotTests/Experiments/ExperimentTests.swift create mode 100644 RiotTests/Experiments/PhasedRolloutFeatureTests.swift create mode 100644 changelog.d/pr-7374.change diff --git a/Config/CommonConfiguration.swift b/Config/CommonConfiguration.swift index 35001b1e4..b00f18831 100644 --- a/Config/CommonConfiguration.swift +++ b/Config/CommonConfiguration.swift @@ -92,14 +92,8 @@ class CommonConfiguration: NSObject, Configurable { sdkOptions.enableNewClientInformationFeature = RiotSettings.shared.enableClientInformationFeature - if sdkOptions.isCryptoSDKAvailable { - let isEnabled = RiotSettings.shared.enableCryptoSDK - MXLog.debug("[CommonConfiguration] Crypto SDK is \(isEnabled ? "enabled" : "disabled")") - sdkOptions.enableCryptoSDK = isEnabled - sdkOptions.enableStartupProgress = isEnabled - } else { - MXLog.debug("[CommonConfiguration] Crypto SDK is not available)") - } + // Configure Crypto SDK feature deciding which crypto module to use + sdkOptions.cryptoSDKFeature = CryptoSDKFeature.shared } private func makeASCIIUserAgent() -> String? { diff --git a/Config/CryptoSDKConfiguration.swift b/Config/CryptoSDKConfiguration.swift deleted file mode 100644 index 3c922e547..000000000 --- a/Config/CryptoSDKConfiguration.swift +++ /dev/null @@ -1,42 +0,0 @@ -// -// Copyright 2023 New Vector Ltd -// -// 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 - -/// Configuration for enabling / disabling Matrix Crypto SDK -@objcMembers class CryptoSDKConfiguration: NSObject { - static let shared = CryptoSDKConfiguration() - - func enable() { - guard MXSDKOptions.sharedInstance().isCryptoSDKAvailable else { - return - } - - RiotSettings.shared.enableCryptoSDK = true - MXSDKOptions.sharedInstance().enableCryptoSDK = true - MXSDKOptions.sharedInstance().enableStartupProgress = true - - MXLog.debug("[CryptoSDKConfiguration] enabling Crypto SDK") - } - - func disable() { - RiotSettings.shared.enableCryptoSDK = false - MXSDKOptions.sharedInstance().enableCryptoSDK = false - MXSDKOptions.sharedInstance().enableStartupProgress = false - - MXLog.debug("[CryptoSDKConfiguration] disabling Crypto SDK") - } -} diff --git a/Podfile b/Podfile index 50655ecd5..74132d85f 100644 --- a/Podfile +++ b/Podfile @@ -70,7 +70,7 @@ abstract_target 'RiotPods' do pod 'WeakDictionary', '~> 2.0' # PostHog for analytics - pod 'PostHog', '~> 1.4.4' + pod 'PostHog', '~> 2.0.0' pod 'Sentry', '~> 7.15.0' pod 'AnalyticsEvents', :git => 'https://github.com/matrix-org/matrix-analytics-events.git', :branch => 'release/swift', :inhibit_warnings => false # pod 'AnalyticsEvents', :path => '../matrix-analytics-events/AnalyticsEvents.podspec' diff --git a/Podfile.lock b/Podfile.lock index eb134a7ee..d5eb10f7d 100644 --- a/Podfile.lock +++ b/Podfile.lock @@ -57,7 +57,7 @@ PODS: - OLMKit/olmcpp (= 3.2.12) - OLMKit/olmc (3.2.12) - OLMKit/olmcpp (3.2.12) - - PostHog (1.4.4) + - PostHog (2.0.0) - ReadMoreTextView (3.0.1) - Realm (10.27.0): - Realm/Headers (= 10.27.0) @@ -105,7 +105,7 @@ DEPENDENCIES: - MatrixSDK (= 0.25.2) - MatrixSDK/JingleCallStack (= 0.25.2) - OLMKit - - PostHog (~> 1.4.4) + - PostHog (~> 2.0.0) - ReadMoreTextView (~> 3.0.1) - Reusable (~> 4.1) - Sentry (~> 7.15.0) @@ -199,7 +199,7 @@ SPEC CHECKSUMS: MatrixSDK: 354274127d163af37bdc55093ab96deea1be6a40 MatrixSDKCrypto: e1ef22aae76b5a6f030ace21a47be83864f4ff44 OLMKit: da115f16582e47626616874e20f7bb92222c7a51 - PostHog: 4b6321b521569092d4ef3a02238d9435dbaeb99f + PostHog: 660ec6c9d80cec17b685e148f17f6785a88b597d ReadMoreTextView: 19147adf93abce6d7271e14031a00303fe28720d Realm: 9ca328bd7e700cc19703799785e37f77d1a130f2 Reusable: 6bae6a5e8aa793c9c441db0213c863a64bce9136 @@ -217,6 +217,6 @@ SPEC CHECKSUMS: zxcvbn-ios: fef98b7c80f1512ff0eec47ac1fa399fc00f7e3c ZXingObjC: fdbb269f25dd2032da343e06f10224d62f537bdb -PODFILE CHECKSUM: 20544e99d9acfdfbc4bf98b21d20c1496b9d6fe9 +PODFILE CHECKSUM: a160b10da6c4728f70275f07b53ef1502c794d4e COCOAPODS: 1.11.3 diff --git a/Riot/Experiments/CryptoSDKFeature.swift b/Riot/Experiments/CryptoSDKFeature.swift new file mode 100644 index 000000000..fd73ce975 --- /dev/null +++ b/Riot/Experiments/CryptoSDKFeature.swift @@ -0,0 +1,97 @@ +// +// Copyright 2023 New Vector Ltd +// +// 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 + +/// An implementation of `MXCryptoV2Feature` which uses `UserDefaults` to persist the enabled status +/// of `CryptoSDK`, and which uses feature flags to control rollout availability. +/// +/// The implementation uses both remote and local feature flags to control the availability of `CryptoSDK`. +/// Whilst remote is more convenient in that it allows changes to the rollout without new app releases, +/// it is not available to all users because it requires data tracking user consent. Remote therefore +/// represents the safer, albeit limited rollout strategy, whereas the local feature flags allows eventually +/// targetting all users, but each target change requires new app release. +/// +/// Additionally users can manually enable this feature from the settings if they are not already in the +/// feature group. +@objc class CryptoSDKFeature: NSObject, MXCryptoV2Feature { + @objc static let shared = CryptoSDKFeature() + + var isEnabled: Bool { + RiotSettings.shared.enableCryptoSDK + } + + private static let FeatureName = "ios-crypto-sdk" + private let remoteFeature: RemoteFeaturesClientProtocol + private let localFeature: PhasedRolloutFeature + + init(remoteFeature: RemoteFeaturesClientProtocol = PostHogAnalyticsClient.shared) { + self.remoteFeature = remoteFeature + self.localFeature = PhasedRolloutFeature( + name: Self.FeatureName, + // Local feature is currently set to 0% target, and all availability is fully controlled + // by the remote feature. Once the remote is fully rolled out, target for local feature will + // be gradually increased. + targetPercentage: 0.0 + ) + } + + func enable() { + RiotSettings.shared.enableCryptoSDK = true + Analytics.shared.trackCryptoSDKEnabled() + + MXLog.debug("[CryptoSDKFeature] Crypto SDK enabled") + } + + func enableIfAvailable(forUserId userId: String!) { + guard !isEnabled else { + MXLog.debug("[CryptoSDKFeature] enableIfAvailable: Feature is already enabled") + return + } + + guard let userId else { + MXLog.failure("[CryptoSDKFeature] enableIfAvailable: Missing user id") + return + } + + guard isFeatureEnabled(userId: userId) else { + MXLog.debug("[CryptoSDKFeature] enableIfAvailable: Feature is currently not available for this user") + return + } + + MXLog.debug("[CryptoSDKFeature] enableIfAvailable: Feature has become available for this user and will be enabled") + enable() + } + + @objc func canManuallyEnable(forUserId userId: String!) -> Bool { + guard let userId else { + MXLog.failure("[CryptoSDKFeature] canManuallyEnable: Missing user id") + return false + } + + // User can manually enable only if not already within the automatic feature group + return !isFeatureEnabled(userId: userId) + } + + @objc func reset() { + RiotSettings.shared.enableCryptoSDK = false + MXLog.debug("[CryptoSDKFeature] Crypto SDK disabled") + } + + private func isFeatureEnabled(userId: String) -> Bool { + remoteFeature.isFeatureEnabled(Self.FeatureName) || localFeature.isEnabled(userId: userId) + } +} diff --git a/Riot/Experiments/Experiment.swift b/Riot/Experiments/Experiment.swift new file mode 100644 index 000000000..2a1531afc --- /dev/null +++ b/Riot/Experiments/Experiment.swift @@ -0,0 +1,50 @@ +// +// Copyright 2023 New Vector Ltd +// +// 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 CryptoKit + +/// Object encapsulating an experiment with an arbitrary number of variants, and a method to deterministically +/// and uniformly assign a variant to user. Variants do not carry any implicit semantics, they are plain numbers +/// to be interpreted by the caller of the experiment. +struct Experiment { + let name: String + let variants: UInt + + /// Get the assigned variant from the total number of variants and for a given `userId` + /// + /// This variant is chosen deterministically (the same `userId` and experiment `name` will yield the same variant) + /// and uniformly (multiple users are distributed roughly evenly among the variants). + func variant(userId: String) -> UInt { + // Combine user id with experiment name to avoid identical variant + // for the same user in different experiments + let data = (userId + name).data(using: .utf8) ?? Data() + + // Get the first 8 bytes and map to decimal number (UInt64 = 8 bytes) + let decimal = digest(for: data) + .prefix(8) + .reduce(0) { $0 << 8 | UInt64($1) } + + // Compress the decimal into a set number of variants using modulo + return UInt(decimal % UInt64(variants)) + } + + private func digest(for data: Data) -> SHA256.Digest { + var sha = SHA256() + sha.update(data: data) + return sha.finalize() + } +} diff --git a/Riot/Experiments/PhasedRolloutFeature.swift b/Riot/Experiments/PhasedRolloutFeature.swift new file mode 100644 index 000000000..67025fd3e --- /dev/null +++ b/Riot/Experiments/PhasedRolloutFeature.swift @@ -0,0 +1,46 @@ +// +// Copyright 2023 New Vector Ltd +// +// 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 + +/// Object enabling a phased rollout of features depending on the `userId` and `targetPercentage`. +/// +/// The feature uses an experiment under the hood with 100 variants representing 100%. +/// Each userId is deterministically and uniformly assigned a variant, and depending +/// on whether this falls below or above the `targetPercentage` threshold, the feature +/// is considered enabled or disabled. +struct PhasedRolloutFeature { + private let experiment: Experiment + private let targetPercentage: Double + + init(name: String, targetPercentage: Double) { + self.experiment = .init( + name: name, + // 100 variants where each variant represents a single percentage + variants: 100 + ) + self.targetPercentage = targetPercentage + } + + func isEnabled(userId: String) -> Bool { + // Get a bucket number between 0-99 + let variant = experiment.variant(userId: userId) + // Convert to a percentage + let percentage = Double(variant) / 100 + // Consider enabled if falls below rollout target + return percentage < targetPercentage + } +} diff --git a/Riot/Experiments/RemoteFeaturesClientProtocol.swift b/Riot/Experiments/RemoteFeaturesClientProtocol.swift new file mode 100644 index 000000000..95ca8393e --- /dev/null +++ b/Riot/Experiments/RemoteFeaturesClientProtocol.swift @@ -0,0 +1,22 @@ +// +// Copyright 2023 New Vector Ltd +// +// 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 + +/// A protocol representing a remote features client +protocol RemoteFeaturesClientProtocol { + func isFeatureEnabled(_ feature: String) -> Bool +} diff --git a/Riot/Modules/Analytics/Analytics.swift b/Riot/Modules/Analytics/Analytics.swift index 60e1560b9..8592bd5bf 100644 --- a/Riot/Modules/Analytics/Analytics.swift +++ b/Riot/Modules/Analytics/Analytics.swift @@ -44,7 +44,7 @@ import AnalyticsEvents static let shared = Analytics() /// The analytics client to send events with. - private var client: AnalyticsClientProtocol = PostHogAnalyticsClient() + private var client: AnalyticsClientProtocol = PostHogAnalyticsClient.shared /// The monitoring client to track crashes, issues and performance private var monitoringClient = SentryMonitoringClient() diff --git a/Riot/Modules/Analytics/PostHogAnalyticsClient.swift b/Riot/Modules/Analytics/PostHogAnalyticsClient.swift index 6b27affea..5bee879d2 100644 --- a/Riot/Modules/Analytics/PostHogAnalyticsClient.swift +++ b/Riot/Modules/Analytics/PostHogAnalyticsClient.swift @@ -25,6 +25,8 @@ class PostHogAnalyticsClient: AnalyticsClientProtocol { /// Any user properties to be included with the next captured event. private(set) var pendingUserProperties: AnalyticsEvent.UserProperties? + static let shared = PostHogAnalyticsClient() + var isRunning: Bool { postHog?.enabled ?? false } func start() { @@ -102,3 +104,9 @@ class PostHogAnalyticsClient: AnalyticsClientProtocol { return properties } } + +extension PostHogAnalyticsClient: RemoteFeaturesClientProtocol { + func isFeatureEnabled(_ feature: String) -> Bool { + postHog?.isFeatureEnabled(feature) == true + } +} diff --git a/Riot/Modules/Application/LegacyAppDelegate.m b/Riot/Modules/Application/LegacyAppDelegate.m index 5944cccdb..208cb46eb 100644 --- a/Riot/Modules/Application/LegacyAppDelegate.m +++ b/Riot/Modules/Application/LegacyAppDelegate.m @@ -2184,7 +2184,7 @@ NSString *const AppDelegateUniversalLinkDidChangeNotification = @"AppDelegateUni [self clearCache]; // Reset Crypto SDK configuration (labs flag for which crypto module to use) - [CryptoSDKConfiguration.shared disable]; + [CryptoSDKFeature.shared reset]; // Reset key backup banner preferences [SecureBackupBannerPreferences.shared reset]; diff --git a/Riot/Modules/Settings/SettingsViewController.m b/Riot/Modules/Settings/SettingsViewController.m index ea8b57b22..023679125 100644 --- a/Riot/Modules/Settings/SettingsViewController.m +++ b/Riot/Modules/Settings/SettingsViewController.m @@ -588,7 +588,7 @@ ChangePasswordCoordinatorBridgePresenterDelegate> if (BuildSettings.settingsScreenShowLabSettings) { Section *sectionLabs = [Section sectionWithTag:SECTION_TAG_LABS]; - if (MXSDKOptions.sharedInstance.isCryptoSDKAvailable) + if ([CryptoSDKFeature.shared canManuallyEnableForUserId:self.mainSession.myUserId]) { [sectionLabs addRowWithTag:LABS_ENABLE_CRYPTO_SDK]; } @@ -2589,20 +2589,17 @@ ChangePasswordCoordinatorBridgePresenterDelegate> cell = labelAndSwitchCell; } - else + else if (row == LABS_ENABLE_CRYPTO_SDK) { - if (row == LABS_ENABLE_CRYPTO_SDK) - { - MXKTableViewCellWithLabelAndSwitch *labelAndSwitchCell = [self getLabelAndSwitchCell:tableView forIndexPath:indexPath]; - BOOL isEnabled = MXSDKOptions.sharedInstance.enableCryptoSDK; - labelAndSwitchCell.mxkLabel.text = isEnabled ? VectorL10n.settingsLabsDisableCryptoSdk : VectorL10n.settingsLabsEnableCryptoSdk; - labelAndSwitchCell.mxkSwitch.on = isEnabled; - [labelAndSwitchCell.mxkSwitch setEnabled:!isEnabled]; - labelAndSwitchCell.mxkSwitch.onTintColor = ThemeService.shared.theme.tintColor; - [labelAndSwitchCell.mxkSwitch addTarget:self action:@selector(enableCryptoSDKFeature:) forControlEvents:UIControlEventTouchUpInside]; - - cell = labelAndSwitchCell; - } + MXKTableViewCellWithLabelAndSwitch *labelAndSwitchCell = [self getLabelAndSwitchCell:tableView forIndexPath:indexPath]; + BOOL isEnabled = MXSDKOptions.sharedInstance.enableCryptoSDK; + labelAndSwitchCell.mxkLabel.text = isEnabled ? VectorL10n.settingsLabsDisableCryptoSdk : VectorL10n.settingsLabsEnableCryptoSdk; + labelAndSwitchCell.mxkSwitch.on = isEnabled; + [labelAndSwitchCell.mxkSwitch setEnabled:!isEnabled]; + labelAndSwitchCell.mxkSwitch.onTintColor = ThemeService.shared.theme.tintColor; + [labelAndSwitchCell.mxkSwitch addTarget:self action:@selector(enableCryptoSDKFeature:) forControlEvents:UIControlEventTouchUpInside]; + + cell = labelAndSwitchCell; } } else if (section == SECTION_TAG_SECURITY) @@ -3391,10 +3388,7 @@ ChangePasswordCoordinatorBridgePresenterDelegate> }]]; [confirmationAlert addAction:[UIAlertAction actionWithTitle:[VectorL10n continue] style:UIAlertActionStyleDefault handler:^(UIAlertAction * action) { - - [CryptoSDKConfiguration.shared enable]; - [Analytics.shared trackCryptoSDKEnabled]; - + [CryptoSDKFeature.shared enable]; [[AppDelegate theDelegate] reloadMatrixSessions:YES]; }]]; diff --git a/RiotNSE/NotificationService.swift b/RiotNSE/NotificationService.swift index f0c0b1fab..084140e60 100644 --- a/RiotNSE/NotificationService.swift +++ b/RiotNSE/NotificationService.swift @@ -200,11 +200,14 @@ class NotificationService: UNNotificationServiceExtension { MXLog.debug("[NotificationService] setup: MXBackgroundSyncService init: BEFORE") self.logMemory() - NotificationService.backgroundSyncService = MXBackgroundSyncService(withCredentials: userAccount.mxCredentials, persistTokenDataHandler: { persistTokenDataHandler in - MXKAccountManager.shared().readAndWriteCredentials(persistTokenDataHandler) - }, unauthenticatedHandler: { error, softLogout, refreshTokenAuth, completion in - userAccount.handleUnauthenticatedWithError(error, isSoftLogout: softLogout, isRefreshTokenAuth: refreshTokenAuth, andCompletion: completion) - }) + NotificationService.backgroundSyncService = MXBackgroundSyncService( + withCredentials: userAccount.mxCredentials, + isCryptoSDKEnabled: isCryptoSDKEnabled, + persistTokenDataHandler: { persistTokenDataHandler in + MXKAccountManager.shared().readAndWriteCredentials(persistTokenDataHandler) + }, unauthenticatedHandler: { error, softLogout, refreshTokenAuth, completion in + userAccount.handleUnauthenticatedWithError(error, isSoftLogout: softLogout, isRefreshTokenAuth: refreshTokenAuth, andCompletion: completion) + }) MXLog.debug("[NotificationService] setup: MXBackgroundSyncService init: AFTER") self.logMemory() } @@ -219,10 +222,10 @@ class NotificationService: UNNotificationServiceExtension { /// Determine whether we have switched from using crypto v1 to v2 or vice versa which will require /// rebuilding `MXBackgroundSyncService` private func hasChangedCryptoSDK() -> Bool { - guard isCryptoSDKEnabled != RiotSettings.shared.enableCryptoSDK else { + guard isCryptoSDKEnabled != MXSDKOptions.sharedInstance().enableCryptoSDK else { return false } - isCryptoSDKEnabled = RiotSettings.shared.enableCryptoSDK + isCryptoSDKEnabled = MXSDKOptions.sharedInstance().enableCryptoSDK return true } diff --git a/RiotNSE/target.yml b/RiotNSE/target.yml index 21c5f2864..9d9047ba8 100644 --- a/RiotNSE/target.yml +++ b/RiotNSE/target.yml @@ -45,6 +45,7 @@ targets: - path: ../Config/BuildSettings.swift - path: ../Riot/Utils/DataProtectionHelper.swift - path: ../Config/CommonConfiguration.swift + - path: ../Riot/Experiments/ - path: ../Riot/Managers/PushNotification/PushNotificationStore.swift - path: ../Riot/Modules/SetPinCode/PinCodePreferences.swift - path: ../Riot/Managers/KeyValueStorage/Extensions/Keychain.swift diff --git a/RiotShareExtension/target.yml b/RiotShareExtension/target.yml index 8601ce85f..6543c688f 100644 --- a/RiotShareExtension/target.yml +++ b/RiotShareExtension/target.yml @@ -52,6 +52,7 @@ targets: - path: ../Riot/Categories/MXRoom+Riot.m - path: ../Config/Configurable.swift - path: ../Config/CommonConfiguration.swift + - path: ../Riot/Experiments/ - path: ../Riot/Utils/UserNameColorGenerator.swift - path: ../Riot/Categories/MXRoomSummary+Riot.m - path: ../Riot/Managers/EncryptionKeyManager/EncryptionKeyManager.swift diff --git a/RiotTests/Experiments/CryptoSDKFeatureTests.swift b/RiotTests/Experiments/CryptoSDKFeatureTests.swift new file mode 100644 index 000000000..ffcf04521 --- /dev/null +++ b/RiotTests/Experiments/CryptoSDKFeatureTests.swift @@ -0,0 +1,79 @@ +// +// Copyright 2023 New Vector Ltd +// +// 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 XCTest +@testable import Element + +class CryptoSDKFeatureTests: XCTestCase { + class RemoteFeatureClient: RemoteFeaturesClientProtocol { + var isEnabled = false + func isFeatureEnabled(_ feature: String) -> Bool { + isEnabled + } + } + + var remote: RemoteFeatureClient! + var feature: CryptoSDKFeature! + + override func setUp() { + RiotSettings.shared.enableCryptoSDK = false + remote = RemoteFeatureClient() + feature = CryptoSDKFeature(remoteFeature: remote) + } + + override func tearDown() { + RiotSettings.shared.enableCryptoSDK = false + } + + func test_disabledByDefault() { + XCTAssertFalse(feature.isEnabled) + } + + func test_enable() { + feature.enable() + XCTAssertTrue(feature.isEnabled) + } + + func test_enableIfAvailable_remainsEnabledWhenRemoteClientDisabled() { + feature.enable() + remote.isEnabled = false + + feature.enableIfAvailable(forUserId: "alice") + + XCTAssertTrue(feature.isEnabled) + } + + func test_enableIfAvailable_notEnabledIfRemoteFeatureDisabled() { + remote.isEnabled = false + feature.enableIfAvailable(forUserId: "alice") + XCTAssertFalse(feature.isEnabled) + } + + func test_canManuallyEnable() { + remote.isEnabled = false + XCTAssertTrue(feature.canManuallyEnable(forUserId: "alice")) + + remote.isEnabled = true + XCTAssertFalse(feature.canManuallyEnable(forUserId: "alice")) + } + + func test_reset() { + feature.enable() + feature.reset() + XCTAssertFalse(RiotSettings.shared.enableCryptoSDK) + } +} diff --git a/RiotTests/Experiments/ExperimentTests.swift b/RiotTests/Experiments/ExperimentTests.swift new file mode 100644 index 000000000..e831f781f --- /dev/null +++ b/RiotTests/Experiments/ExperimentTests.swift @@ -0,0 +1,71 @@ +// +// Copyright 2023 New Vector Ltd +// +// 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 XCTest +@testable import Element + +class ExperimentTests: XCTestCase { + + private func randomUserId() -> String { + return "user_" + UUID().uuidString.prefix(6) + } + + func test_singleVariant() { + let experiment = Experiment(name: "single", variants: 1) + for _ in 0 ..< 1000 { + let variant = experiment.variant(userId: randomUserId()) + XCTAssertEqual(variant, 0) + } + } + + func test_twoVariants() { + let experiment = Experiment(name: "two", variants: 2) + + var variants = Set() + for _ in 0 ..< 1000 { + let variant = experiment.variant(userId: randomUserId()) + variants.insert(variant) + } + + // We perform the test by collecting all assigned variants for 1000 users + // and ensuring we only encounter variants 0 and 1 + XCTAssertEqual(variants.count, 2) + XCTAssertTrue(variants.contains(0)) + XCTAssertTrue(variants.contains(1)) + XCTAssertFalse(variants.contains(2)) + } + + func test_manyVariants() { + let experiment = Experiment(name: "many", variants: 5) + + var variants = Set() + for _ in 0 ..< 10000 { + let variant = experiment.variant(userId: randomUserId()) + variants.insert(variant) + } + + // We perform the test by collecting all assigned variants for 10000 users + // and ensuring we only encounter variants between 0 and 4 + XCTAssertEqual(variants.count, 5) + XCTAssertTrue(variants.contains(0)) + XCTAssertTrue(variants.contains(1)) + XCTAssertTrue(variants.contains(2)) + XCTAssertTrue(variants.contains(3)) + XCTAssertTrue(variants.contains(4)) + XCTAssertFalse(variants.contains(5)) + } +} diff --git a/RiotTests/Experiments/PhasedRolloutFeatureTests.swift b/RiotTests/Experiments/PhasedRolloutFeatureTests.swift new file mode 100644 index 000000000..3c50078d3 --- /dev/null +++ b/RiotTests/Experiments/PhasedRolloutFeatureTests.swift @@ -0,0 +1,55 @@ +// +// Copyright 2023 New Vector Ltd +// +// 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 XCTest +@testable import Element + +class PhasedRolloutFeatureTests: XCTestCase { + + private func randomUserId() -> String { + return "user_" + UUID().uuidString.prefix(6) + } + + func test_allDisabled() { + let feature = PhasedRolloutFeature(name: "disabled", targetPercentage: 0) + for _ in 0 ..< 1000 { + let isEnabled = feature.isEnabled(userId: randomUserId()) + XCTAssertFalse(isEnabled) + } + } + + func test_allEnabled() { + let feature = PhasedRolloutFeature(name: "enabled", targetPercentage: 1) + for _ in 0 ..< 1000 { + let isEnabled = feature.isEnabled(userId: randomUserId()) + XCTAssertTrue(isEnabled) + } + } + + func test_someEnabled() { + let feature = PhasedRolloutFeature(name: "some", targetPercentage: 0.5) + var statuses = Set() + for _ in 0 ..< 1000 { + let isEnabled = feature.isEnabled(userId: randomUserId()) + statuses.insert(isEnabled) + } + + // We test by checking that we encountered both enabled and disabled cases + XCTAssertTrue(statuses.contains(true)) + XCTAssertTrue(statuses.contains(false)) + } +} diff --git a/RiotTests/target.yml b/RiotTests/target.yml index dd7736b6f..ecce8f700 100644 --- a/RiotTests/target.yml +++ b/RiotTests/target.yml @@ -60,8 +60,6 @@ targets: - path: . - path: ../Config/Configurable.swift - path: ../Config/BuildSettings.swift - - path: ../Config/AppConfiguration.swift - - path: ../Config/CommonConfiguration.swift - path: ../Riot/Categories/Bundle.swift - path: ../Riot/Managers/AppInfo/AppInfo.swift - path: ../Riot/Managers/AppInfo/AppVersion.swift diff --git a/SiriIntents/target.yml b/SiriIntents/target.yml index 82f7a89da..57fcaa53b 100644 --- a/SiriIntents/target.yml +++ b/SiriIntents/target.yml @@ -45,6 +45,7 @@ targets: - path: ../Riot/Categories/Bundle.swift - path: ../Riot/Categories/MXEvent.swift - path: ../Config/CommonConfiguration.swift + - path: ../Riot/Experiments/ - path: ../Config/BuildSettings.swift - path: ../Config/Configurable.swift - path: ../Riot/Managers/Settings/RiotSettings.swift diff --git a/changelog.d/pr-7374.change b/changelog.d/pr-7374.change new file mode 100644 index 000000000..40d1e3294 --- /dev/null +++ b/changelog.d/pr-7374.change @@ -0,0 +1 @@ +CryptoV2: CryptoSDK phased rollout feature From 6d51197214079439186745a652e13970af1db456 Mon Sep 17 00:00:00 2001 From: Andy Uhnak Date: Wed, 22 Feb 2023 11:23:05 +0000 Subject: [PATCH 2/2] Update tests --- RiotTests/Experiments/ExperimentTests.swift | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/RiotTests/Experiments/ExperimentTests.swift b/RiotTests/Experiments/ExperimentTests.swift index e831f781f..ea0b8af2b 100644 --- a/RiotTests/Experiments/ExperimentTests.swift +++ b/RiotTests/Experiments/ExperimentTests.swift @@ -60,12 +60,8 @@ class ExperimentTests: XCTestCase { // We perform the test by collecting all assigned variants for 10000 users // and ensuring we only encounter variants between 0 and 4 - XCTAssertEqual(variants.count, 5) - XCTAssertTrue(variants.contains(0)) - XCTAssertTrue(variants.contains(1)) - XCTAssertTrue(variants.contains(2)) - XCTAssertTrue(variants.contains(3)) - XCTAssertTrue(variants.contains(4)) + XCTAssertTrue(variants.count >= 2 && variants.count <= 5) + XCTAssertTrue(variants.isSubset(of: .init([0, 1, 2, 3, 4]))) XCTAssertFalse(variants.contains(5)) } }