BIT-1686: Limit syncing to every 30 minutes if account has changes (#433)

This commit is contained in:
Matt Czech 2024-01-31 22:33:08 -06:00 committed by GitHub
parent 78699f7f92
commit c3205f19d6
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
19 changed files with 334 additions and 19 deletions

View File

@ -0,0 +1,24 @@
import Foundation
import Networking
// MARK: - AccountRevisionDateResponseModel
/// API response model for the account revision date request.
///
struct AccountRevisionDateResponseModel: Response {
// MARK: Properties
/// The account's last revision date.
let date: Date?
// MARK: Initialization
init(response: HTTPResponse) throws {
let bodyString = String(data: response.body, encoding: .utf8)
guard let bodyString, let epochMilliseconds = TimeInterval(bodyString) else {
date = nil
return
}
date = Date(timeIntervalSince1970: epochMilliseconds / 1000)
}
}

View File

@ -0,0 +1,21 @@
import XCTest
@testable import BitwardenShared
class AccountRevisionDateResponseModelTests: BitwardenTestCase {
// MARK: Tests
/// `init(response:)` sets the date to `nil` if a date isn't able to be parsed from the response.
func test_init_invalidDate() throws {
let subject = try AccountRevisionDateResponseModel(response: .success(body: Data()))
XCTAssertNil(subject.date)
}
/// `init(response:)` parses the date from the plain text response.
func test_init_validDate() throws {
let subject = try AccountRevisionDateResponseModel(
response: .success(body: Data("1704067200000".utf8))
)
XCTAssertEqual(subject.date, Date(timeIntervalSince1970: 1_704_067_200))
}
}

View File

@ -7,6 +7,12 @@ import Networking
/// A protocol for an API service used to make account requests.
///
protocol AccountAPIService {
/// Performs the account revision date request and returns the date of the account's last revision.
///
/// - Returns: The account's last revision date.
///
func accountRevisionDate() async throws -> Date?
/// Checks if the user's entered password has been found in a data breach.
///
/// - Parameter password: The user's entered password.
@ -48,6 +54,10 @@ protocol AccountAPIService {
// MARK: - APIService
extension APIService: AccountAPIService {
func accountRevisionDate() async throws -> Date? {
try await apiService.send(AccountRevisionDateRequest()).date
}
func checkDataBreaches(password: String) async throws -> Int {
// Generate a SHA1 hash value for the password.
let fullPasswordHash = Data(password.utf8).generatedHash(using: Insecure.SHA1.self)

View File

@ -26,6 +26,23 @@ class AccountAPIServiceTests: BitwardenTestCase {
// MARK: Tests
/// `accountRevisionDate()` returns the account's last revision date.
func test_accountRevisionDate() async throws {
client.result = .httpSuccess(testData: .accountRevisionDate())
let date = try await subject.accountRevisionDate()
XCTAssertEqual(date, Date(timeIntervalSince1970: 1_704_067_200))
}
/// `accountRevisionDate()` throws an error if the request fails.
func test_accountRevisionDate_httpFailure() async throws {
client.result = .httpFailure(BitwardenTestError.example)
await assertAsyncThrows(error: BitwardenTestError.example) {
_ = try await subject.accountRevisionDate()
}
}
/// `createNewAccount(email:masterPasswordHash)` throws an error if the request fails.
func test_create_account_httpFailure() async {
client.result = .httpFailure()

View File

@ -1,4 +1,14 @@
import Foundation
extension APITestData {
// MARK: Account Revision Date
static func accountRevisionDate( // swiftlint:disable:this type_contents_order
_ date: Date = Date(timeIntervalSince1970: 1_704_067_200)
) -> APITestData {
APITestData(data: Data(String(date.timeIntervalSince1970 * 1000).utf8))
}
// MARK: Create Account
static let createAccountAccountAlreadyExists = loadFromJsonBundle(resource: "CreateAccountAccountAlreadyExists")

View File

@ -0,0 +1,15 @@
import Networking
// MARK: - AccountRevisionDateRequest
/// Data model for fetching the account's last revision date.
///
struct AccountRevisionDateRequest: Request {
typealias Response = AccountRevisionDateResponseModel
/// The HTTP method for this request.
let method = HTTPMethod.get
/// The URL path for this request.
let path = "/accounts/revision-date"
}

View File

@ -0,0 +1,18 @@
import Networking
import XCTest
@testable import BitwardenShared
class AccountRevisionDateRequestTests: BitwardenTestCase {
// MARK: Tests
/// Validate that the method is correct.
func test_method() {
XCTAssertEqual(AccountRevisionDateRequest().method, .get)
}
/// Validate that the path is correct.
func test_path() {
XCTAssertEqual(AccountRevisionDateRequest().path, "/accounts/revision-date")
}
}

View File

@ -175,7 +175,7 @@ extension DefaultSettingsRepository: SettingsRepository {
}
func fetchSync() async throws {
try await syncService.fetchSync()
try await syncService.fetchSync(forceSync: true)
}
func getAllowSyncOnRefresh() async throws -> Bool {

View File

@ -178,11 +178,11 @@ class DefaultNotificationService: NotificationService {
case .syncCiphers,
.syncSettings,
.syncVault:
try await syncService.fetchSync()
try await syncService.fetchSync(forceSync: true)
case .syncOrgKeys:
// TODO: BIT-1528 call api to refresh token
// try await authAPIService.refreshIdentityToken(refreshToken: ???)
try await syncService.fetchSync()
try await syncService.fetchSync(forceSync: true)
case .logOut:
break
case .syncSendCreate,

View File

@ -295,6 +295,7 @@ public class ServiceContainer: Services { // swiftlint:disable:this type_body_le
)
let syncService = DefaultSyncService(
accountAPIService: apiService,
cipherService: cipherService,
collectionService: collectionService,
folderService: folderService,

View File

@ -154,6 +154,13 @@ protocol StateService: AnyObject {
///
func getLastActiveTime(userId: String?) async throws -> Date?
/// Gets the time of the last sync for a user.
///
/// - Parameter userId: The user ID associated with the last sync time.
/// - Returns: The user's last sync time.
///
func getLastSyncTime(userId: String?) async throws -> Date?
/// The last value of the connect to watch setting, ignoring the user id. Used for
/// sending the status to the watch if the user is logged out.
///
@ -618,6 +625,15 @@ extension StateService {
try await getLastActiveTime(userId: nil)
}
/// Gets the time of the last sync for a user.
///
/// - Parameter userId: The user ID associated with the last sync time.
/// - Returns: The user's last sync time.
///
func getLastSyncTime() async throws -> Date? {
try await getLastSyncTime(userId: nil)
}
/// Gets the master password hash for the active account.
///
/// - Returns: The user's master password hash.
@ -1031,6 +1047,11 @@ actor DefaultStateService: StateService { // swiftlint:disable:this type_body_le
return appSettingsStore.lastActiveTime(userId: userId)
}
func getLastSyncTime(userId: String?) async throws -> Date? {
let userId = try userId ?? getActiveAccountUserId()
return appSettingsStore.lastSyncTime(userId: userId)
}
func getLastUserShouldConnectToWatch() async -> Bool {
appSettingsStore.lastUserShouldConnectToWatch
}

View File

@ -430,6 +430,19 @@ class StateServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body
)
}
/// `getLastSyncTime(userId:)` gets the user's last sync time.
func test_getLastSyncTime() async throws {
await subject.addAccount(.fixture(profile: .fixture(userId: "1")))
let noTime = try await subject.getLastSyncTime(userId: "1")
XCTAssertNil(noTime)
let date = Date(timeIntervalSince1970: 1_704_067_200)
appSettingsStore.lastSyncTimeByUserId["1"] = date
let lastSyncTime = try await subject.getLastSyncTime(userId: "1")
XCTAssertEqual(lastSyncTime, date)
}
/// `getLoginRequest()` gets any pending login requests.
func test_getLoginRequest() async {
let loginRequest = LoginRequestNotification(id: "1", userId: "10")

View File

@ -175,6 +175,11 @@ class MockStateService: StateService { // swiftlint:disable:this type_body_lengt
return lastActiveTime[userId]
}
func getLastSyncTime(userId: String?) async throws -> Date? {
let userId = try unwrapUserId(userId)
return lastSyncTimeByUserId[userId]
}
func getLoginRequest() async -> LoginRequestNotification? {
loginRequest
}

View File

@ -55,6 +55,9 @@ enum Constants {
/// A default value for the minimum number of characters required when creating a password.
static let minimumPasswordCharacters = 12
/// The minimum number of minutes before allowing the vault to sync again.
static let minimumSyncInterval: TimeInterval = 30 * 60 // 30 minutes
/// The default number of KDF iterations to perform.
static let pbkdf2Iterations = 600_000

View File

@ -224,7 +224,7 @@ class DefaultSendRepository: SendRepository {
func fetchSync(isManualRefresh: Bool) async throws {
let allowSyncOnRefresh = try await stateService.getAllowSyncOnRefresh()
if !isManualRefresh || allowSyncOnRefresh {
try await syncService.fetchSync()
try await syncService.fetchSync(forceSync: isManualRefresh)
}
}

View File

@ -646,7 +646,7 @@ extension DefaultVaultRepository: VaultRepository {
func fetchSync(isManualRefresh: Bool) async throws {
let allowSyncOnRefresh = try await stateService.getAllowSyncOnRefresh()
if !isManualRefresh || allowSyncOnRefresh {
try await syncService.fetchSync()
try await syncService.fetchSync(forceSync: isManualRefresh)
}
}

View File

@ -11,7 +11,10 @@ protocol SyncService: AnyObject {
/// Performs an API request to sync the user's vault data.
///
func fetchSync() async throws
/// - Parameter forceSync: Whether syncing should be forced, bypassing the account revision and
/// minimum sync interval checks.
///
func fetchSync(forceSync: Bool) async throws
}
// MARK: - DefaultSyncService
@ -21,6 +24,9 @@ protocol SyncService: AnyObject {
class DefaultSyncService: SyncService {
// MARK: Properties
/// The services used by the application to make account related API requests.
private let accountAPIService: AccountAPIService
/// The service for managing the ciphers for the user.
private let cipherService: CipherService
@ -53,6 +59,7 @@ class DefaultSyncService: SyncService {
/// Initializes a `DefaultSyncService`.
///
/// - Parameters:
/// - accountAPIService: The services used by the application to make account related API requests.
/// - cipherService: The service for managing the ciphers for the user.
/// - collectionService: The service for managing the collections for the user.
/// - folderService: The service for managing the folders for the user.
@ -64,6 +71,7 @@ class DefaultSyncService: SyncService {
/// - syncAPIService: The API service used to perform sync API requests.
///
init(
accountAPIService: AccountAPIService,
cipherService: CipherService,
collectionService: CollectionService,
folderService: FolderService,
@ -74,6 +82,7 @@ class DefaultSyncService: SyncService {
stateService: StateService,
syncAPIService: SyncAPIService
) {
self.accountAPIService = accountAPIService
self.cipherService = cipherService
self.collectionService = collectionService
self.folderService = folderService
@ -84,12 +93,45 @@ class DefaultSyncService: SyncService {
self.stateService = stateService
self.syncAPIService = syncAPIService
}
// MARK: Private
/// Determine if a full sync is necessary.
///
/// - Parameters:
/// - forceSync: Whether syncing should be forced, bypassing the account revision and minimum
/// sync interval checks.
/// - userId: The user ID of the account to sync.
/// - Returns: Whether a sync should be performed.
///
private func needsSync(forceSync: Bool, userId: String) async throws -> Bool {
guard let lastSyncTime = try await stateService.getLastSyncTime(userId: userId), !forceSync else { return true }
guard lastSyncTime.addingTimeInterval(Constants.minimumSyncInterval) < Date.now else { return false }
do {
guard let accountRevisionDate = try await accountAPIService.accountRevisionDate()
else { return true }
if lastSyncTime < accountRevisionDate {
return true
} else {
// No updates to the account since the last sync. Update the last sync time but
// don't do a full sync.
try await stateService.setLastSyncTime(Date(), userId: userId)
return false
}
} catch {
return false
}
}
}
extension DefaultSyncService {
func fetchSync() async throws {
func fetchSync(forceSync: Bool) async throws {
let userId = try await stateService.getActiveAccountId()
guard try await needsSync(forceSync: forceSync, userId: userId) else { return }
let response = try await syncAPIService.getSync()
if let organizations = response.profile?.organizations {

View File

@ -34,6 +34,7 @@ class SyncServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body_
stateService = MockStateService()
subject = DefaultSyncService(
accountAPIService: APIService(client: client),
cipherService: cipherService,
collectionService: collectionService,
folderService: folderService,
@ -68,7 +69,7 @@ class SyncServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body_
client.result = .httpSuccess(testData: .syncWithCiphers)
stateService.activeAccount = .fixture()
try await subject.fetchSync()
try await subject.fetchSync(forceSync: false)
XCTAssertEqual(client.requests.count, 1)
XCTAssertEqual(client.requests[0].method, .get)
@ -81,12 +82,124 @@ class SyncServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body_
)
}
/// `fetchSync()` with `forceSync: true` performs the sync API request regardless of the
/// account revision or sync interval.
func test_fetchSync_forceSync() async throws {
client.result = .httpSuccess(testData: .syncWithCiphers)
stateService.activeAccount = .fixture()
stateService.lastSyncTimeByUserId["1"] = .now
try await subject.fetchSync(forceSync: true)
XCTAssertEqual(client.requests.count, 1)
XCTAssertEqual(client.requests[0].method, .get)
XCTAssertEqual(client.requests[0].url.absoluteString, "https://example.com/api/sync")
try XCTAssertEqual(
XCTUnwrap(stateService.lastSyncTimeByUserId["1"]).timeIntervalSince1970,
Date().timeIntervalSince1970,
accuracy: 1
)
}
/// `fetchSync()` syncs if the last sync time is greater than 30 minutes ago and the account has
/// newer revisions.
func test_fetchSync_needsSync_lastSyncTime_older30MinsWithRevisions() async throws {
client.results = [
.httpSuccess(testData: .accountRevisionDate(Date.now)),
.httpSuccess(testData: .syncWithCipher),
]
stateService.activeAccount = .fixture()
stateService.lastSyncTimeByUserId["1"] = try XCTUnwrap(
Calendar.current.date(byAdding: .minute, value: -31, to: .now)
)
try await subject.fetchSync(forceSync: false)
XCTAssertEqual(client.requests.count, 2)
XCTAssertNotNil(cipherService.replaceCiphersCiphers)
try XCTAssertEqual(
XCTUnwrap(stateService.lastSyncTimeByUserId["1"]?.timeIntervalSinceNow),
0,
accuracy: 1
)
}
/// `fetchSync()` doesn't sync if the last sync time is greater than 30 minutes but fetching
/// the account revision date fails.
func test_fetchSync_needsSync_lastSyncTime_older30Mins_revisionsError() async throws {
let lastSyncTime = try XCTUnwrap(
Calendar.current.date(byAdding: .minute, value: -31, to: .now)
)
client.result = .httpFailure(BitwardenTestError.example)
stateService.activeAccount = .fixture()
stateService.lastSyncTimeByUserId["1"] = lastSyncTime
try await subject.fetchSync(forceSync: false)
XCTAssertEqual(client.requests.count, 1)
XCTAssertNil(cipherService.replaceCiphersCiphers)
XCTAssertEqual(stateService.lastSyncTimeByUserId["1"], lastSyncTime)
}
/// `fetchSync()` doesn't syncs if the last sync time is greater than 30 minutes ago but the
/// account doesn't have newer revisions.
func test_fetchSync_needsSync_lastSyncTime_older30MinsWithoutRevisions() async throws {
let lastRevision = try XCTUnwrap(Calendar.current.date(byAdding: .day, value: -1, to: .now))
client.results = [
.httpSuccess(testData: .accountRevisionDate(lastRevision)),
.httpSuccess(testData: .syncWithCipher),
]
stateService.activeAccount = .fixture()
stateService.lastSyncTimeByUserId["1"] = try XCTUnwrap(
Calendar.current.date(byAdding: .minute, value: -31, to: .now)
)
try await subject.fetchSync(forceSync: false)
XCTAssertEqual(client.requests.count, 1)
XCTAssertNil(cipherService.replaceCiphersCiphers)
try XCTAssertEqual(
XCTUnwrap(stateService.lastSyncTimeByUserId["1"]?.timeIntervalSinceNow),
0,
accuracy: 1
)
}
/// `fetchSync()` doesn't sync if the last sync time is within the last 30 minutes.
func test_fetchSync_needsSync_lastSyncTime_newer30Mins() async throws {
client.result = .httpSuccess(testData: .syncWithCipher)
stateService.activeAccount = .fixture()
stateService.lastSyncTimeByUserId["1"] = try XCTUnwrap(
Calendar.current.date(byAdding: .minute, value: -29, to: .now)
)
try await subject.fetchSync(forceSync: false)
XCTAssertTrue(client.requests.isEmpty)
XCTAssertNil(cipherService.replaceCiphersCiphers)
}
/// `fetchSync()` syncs if there's no existing last sync time.
func test_fetchSync_needsSync_noLastSyncTime() async throws {
client.result = .httpSuccess(testData: .syncWithCipher)
stateService.activeAccount = .fixture()
try await subject.fetchSync(forceSync: false)
XCTAssertFalse(client.requests.isEmpty)
XCTAssertNotNil(cipherService.replaceCiphersCiphers)
}
/// `fetchSync()` replaces the list of the user's ciphers.
func test_fetchSync_ciphers() async throws {
client.result = .httpSuccess(testData: .syncWithCipher)
stateService.activeAccount = .fixture()
try await subject.fetchSync()
try await subject.fetchSync(forceSync: false)
let date = Date(year: 2023, month: 8, day: 10, hour: 8, minute: 33, second: 45, nanosecond: 345_000_000)
XCTAssertEqual(
@ -119,7 +232,7 @@ class SyncServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body_
client.result = .httpSuccess(testData: .syncWithCiphersCollections)
stateService.activeAccount = .fixture()
try await subject.fetchSync()
try await subject.fetchSync(forceSync: false)
XCTAssertEqual(
collectionService.replaceCollectionsCollections,
@ -144,7 +257,7 @@ class SyncServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body_
client.result = .httpSuccess(testData: .syncWithProfile)
stateService.activeAccount = .fixture()
try await subject.fetchSync()
try await subject.fetchSync(forceSync: false)
XCTAssertEqual(
stateService.updateProfileResponse,
@ -166,7 +279,7 @@ class SyncServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body_
client.result = .httpSuccess(testData: .syncWithSends)
stateService.activeAccount = .fixture()
try await subject.fetchSync()
try await subject.fetchSync(forceSync: false)
XCTAssertEqual(
sendService.replaceSendsSends,
@ -209,7 +322,7 @@ class SyncServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body_
client.result = .httpSuccess(testData: .syncWithDomains)
stateService.activeAccount = .fixture()
try await subject.fetchSync()
try await subject.fetchSync(forceSync: false)
XCTAssertEqual(
settingsService.replaceEquivalentDomainsDomains,
@ -227,7 +340,7 @@ class SyncServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body_
client.result = .httpSuccess(testData: .syncWithCiphers)
stateService.activeAccount = .fixture()
try await subject.fetchSync()
try await subject.fetchSync(forceSync: false)
XCTAssertEqual(
folderService.replaceFoldersFolders,
@ -247,7 +360,7 @@ class SyncServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body_
client.result = .httpSuccess(testData: .syncWithProfileOrganizations)
stateService.activeAccount = .fixture()
try await subject.fetchSync()
try await subject.fetchSync(forceSync: false)
XCTAssertTrue(organizationService.initializeOrganizationCryptoWithOrgsCalled)
XCTAssertEqual(organizationService.replaceOrganizationsOrganizations?.count, 2)
@ -261,7 +374,7 @@ class SyncServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body_
client.result = .httpSuccess(testData: .syncWithPolicies)
stateService.activeAccount = .fixture()
try await subject.fetchSync()
try await subject.fetchSync(forceSync: false)
XCTAssertEqual(policyService.replacePoliciesPolicies.count, 4)
XCTAssertEqual(
@ -309,7 +422,7 @@ class SyncServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body_
stateService.activeAccount = .fixture()
await assertAsyncThrows {
try await subject.fetchSync()
try await subject.fetchSync(forceSync: false)
}
}
}
} // swiftlint:disable:this file_length

View File

@ -4,10 +4,12 @@ import Combine
class MockSyncService: SyncService {
var didFetchSync = false
var fetchSyncForceSync: Bool?
var fetchSyncResult: Result<Void, Error> = .success(())
func fetchSync() async throws {
func fetchSync(forceSync: Bool) async throws {
didFetchSync = true
fetchSyncForceSync = forceSync
try fetchSyncResult.get()
}
}