8000 PM-12565: Don't fail login if setting account setup progress fails by matt-livefront · Pull Request #988 · bitwarden/ios · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

PM-12565: Don't fail login if setting account setup progress fails #988

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 30, 2024
Merged

PM-12565: Don't fail login if setting account setup progress fails #988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions BitwardenShared/Core/Auth/Services/AuthService.swift
View file
Open in desktop
10000
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,9 @@ class DefaultAuthService: AuthService { // swiftlint:disable:this type_body_leng
/// The service used by the application to manage the environment settings.
private let environmentService: EnvironmentService

/// The service used by the application to report non-fatal errors.
private let errorReporter: ErrorReporter

/// The repository used to manages keychain items.
private let keychainRepository: KeychainRepository

Expand Down Expand Up @@ -314,6 +317,7 @@ class DefaultAuthService: AuthService { // swiftlint:disable:this type_body_leng
/// - credentialIdentityStore: The store which makes credential identities available to the
/// system for AutoFill suggestions.
/// - environmentService: The service used by the application to manage the environment settings.
/// - errorReporter: The service used by the application to report non-fatal errors.
/// - keychainRepository: The repository used to manages keychain items.
/// - policyService: The service used by the application to manage the policy.
/// - stateService: The object used by the application to retrieve information about this device.
Expand All @@ -328,6 +332,7 @@ class DefaultAuthService: AuthService { // swiftlint:disable:this type_body_leng
configService: ConfigService,
credentialIdentityStore: CredentialIdentityStore = ASCredentialIdentityStore.shared,
environmentService: EnvironmentService,
errorReporter: ErrorReporter,
keychainRepository: KeychainRepository,
policyService: PolicyService,
stateService: StateService,
Expand All @@ -341,6 +346,7 @@ class DefaultAuthService: AuthService { // swiftlint:disable:this type_body_leng
self.configService = configService
self.credentialIdentityStore = credentialIdentityStore
self.environmentService = environmentService
self.errorReporter = errorReporter
self.keychainRepository = keychainRepository
self.policyService = policyService
self.stateService = stateService
Expand Down Expand Up @@ -593,9 +599,13 @@ class DefaultAuthService: AuthService { // swiftlint:disable:this type_body_leng
}

if isNewAccount, await configService.getFeatureFlag(.nativeCreateAccountFlow) {
let isAutofillEnabled = await credentialIdentityStore.isAutofillEnabled()
try await stateService.setAccountSetupAutofill(isAutofillEnabled ? .complete : .incomplete)
try await stateService.setAccountSetupVaultUnlock(.incomplete)
do {
let isAutofillEnabled = await credentialIdentityStore.isAutofillEnabled()
try await stateService.setAccountSetupAutofill(isAutofillEnabled ? .complete : .incomplete)
try await stateService.setAccountSetupVaultUnlock(.incomplete)
} catch {
errorReporter.log(error: error)
}
}
}

Expand Down
34 changes: 34 additions & 0 deletions BitwardenShared/Core/Auth/Services/AuthServiceTests.swift
View file
Open in desktop
8000
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ class AuthServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body_
var configService: MockConfigService!
var credentialIdentityStore: MockCredentialIdentityStore!
var environmentService: MockEnvironmentService!
var errorReporter: MockErrorReporter!
var keychainRepository: MockKeychainRepository!
var stateService: MockStateService!
var policyService: MockPolicyService!
Expand Down Expand Up @@ -47,6 +48,7 @@ class AuthServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body_
))
credentialIdentityStore = MockCredentialIdentityStore()
environmentService = MockEnvironmentService()
errorReporter = MockErrorReporter()
keychainRepository = MockKeychainRepository()
policyService = MockPolicyService()
stateService = MockStateService()
Expand All @@ -61,6 +63,7 @@ class AuthServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body_
configService: configService,
credentialIdentityStore: credentialIdentityStore,
environmentService: environmentService,
errorReporter: errorReporter,
keychainRepository: keychainRepository,
policyService: policyService,
stateService: stateService,
Expand All @@ -80,6 +83,7 @@ class AuthServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body_
configService = nil
credentialIdentityStore = nil
environmentService = nil
errorReporter = nil
keychainRepository = nil
stateService = nil
subject = nil
Expand Down Expand Up @@ -457,6 +461,36 @@ class AuthServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body_
assertGetConfig()
}

/// `loginWithMasterPassword(_:username:captchaToken:)` logs the user in with the password for
/// a newly created account and logs an error instead of throwing if setting the account setup
/// progress fails.
func test_loginWithMasterPassword_isNewAccount_accountSetupError() async throws {
client.results = [
.httpSuccess(testData: .preLoginSuccess),
.httpSuccess(testData: .identityTokenSuccess),
]
appSettingsStore.appId = "App id"
clientService.mockAuth.hashPasswordResult = .success("hashed password")
configService.featureFlagsBool[.nativeCreateAccountFlow] = true
credentialIdentityStore.state.mockIsEnabled = true
stateService.accountSetupAutofillError = BitwardenTestError.example
stateService.preAuthEnvironmentUrls = EnvironmentUrlData(base: URL(string: "https://vault.bitwarden.com"))
systemDevice.modelIdentifier = "Model id"

// Attempt to login.
try await subject.loginWithMasterPassword(
"Password1234!",
username: "email@example.com",
captchaToken: nil,
isNewAccount: true
)

XCTAssertEqual(errorReporter.errors as? [BitwardenTestError], [.example])

XCTAssertNil(stateService.accountSetupAutofill["13512467-9cfe-43b0-969f-07534084764b"])
XCTAssertNil(stateService.accountSetupVaultUnlock["13512467-9cfe-43b0-969f-07534084764b"])
}

/// `loginWithMasterPassword(_:username:captchaToken:)` logs the user in with the password for
/// a newly created account and sets their autofill account setup progress to complete if
/// autofill is already enabled.
Expand Down
1 change: 1 addition & 0 deletions BitwardenShared/Core/Platform/Services/ServiceContainer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,7 @@ public class ServiceContainer: Services { // swiftlint:disable:this type_body_le
clientService: clientService,
configService: configService,
environmentService: environmentService,
errorReporter: errorReporter,
keychainRepository: keychainRepository,
policyService: policyService,
stateService: stateService,
Expand Down
30 changes: 14 additions & 16 deletions BitwardenShared/Core/Platform/Services/StateService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,14 @@ protocol StateService: AnyObject {
/// - Parameter userId: The user ID associated with the autofill setup progress.
/// - Returns: The user's autofill setup progress.
///
func getAccountSetupAutofill(userId: String?) async throws -> AccountSetupProgress?
func getAccountSetupAutofill(userId: String) async -> AccountSetupProgress?
Copy link
Collaborator Author
@matt-livefront matt-livefront Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a slight change to the pattern that exists in StateService.

Our current pattern is that the protocol defines a throwing function which takes an optional userId. If the user ID parameter is nil, then we get the active user ID. And then we have an extension method which doesn't accept any parameters to essentially provide a default value of nil as the user ID to get the active user ID.

If we instead move the "default to active user ID" logic into the extension method, we could make the protocol method require a user ID and in most cases make it not throw.

This won't affect most places where we call the extension method without a user ID. But in cases where we already have the user ID (some service/repository methods), we could avoid throwing. If we like this pattern, I can update the rest of StateService to match.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! I really like the approach but I wouldn't perform the changes right now. We're working on a proposal to reduce most of the boilerplate in StateService and AppSettingsStore, so reducing the effort to maintain the code and tests. So I'll add this there and share the proposal with you as well to revise it if you agree.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, thanks!


/// Gets the user's progress for setting up vault unlock.
///
/// - Parameter userId: The user ID associated with the vault unlock setup progress.
/// - Returns: The user's vault unlock setup progress.
///
func getAccountSetupVaultUnlock(userId: String?) async throws -> AccountSetupProgress?
func getAccountSetupVaultUnlock(userId: String) async -> AccountSetupProgress?

/// Gets all accounts.
///
Expand Down Expand Up @@ -725,15 +725,15 @@ extension StateService {
/// - Returns: The user's autofill setup progress.
///
func getAccountSetupAutofill() async throws -> AccountSetupProgress? {
try await getAccountSetupAutofill(userId: nil)
try await getAccountSetupAutofill(userId: getActiveAccountId())
}

/// Gets the active user's progress for setting up vault unlock.
///
/// - Returns: The user's vault unlock setup progress.
///
func getAccountSetupVaultUnlock() async throws -> AccountSetupProgress? {
try await getAccountSetupVaultUnlock(userId: nil)
try await getAccountSetupVaultUnlock(userId: getActiveAccountId())
}

/// Gets the active account id.
Expand Down Expand Up @@ -1277,14 +1277,12 @@ actor DefaultStateService: StateService { // swiftlint:disable:this type_body_le
return accountVolatileData[userId]?.hasBeenUnlockedInteractively == true
}

func getAccountSetupAutofill(userId: String?) async throws -> AccountSetupProgress? {
let userId = try userId ?? getActiveAccountUserId()
return appSettingsStore.accountSetupAutofill(userId: userId)
func getAccountSetupAutofill(userId: String) async -> AccountSetupProgress? {
appSettingsStore.accountSetupAutofill(userId: userId)
}

func getAccountSetupVaultUnlock(userId: String?) async throws -> AccountSetupProgress? {
let userId = try userId ?? getActiveAccountUserId()
return appSettingsStore.accountSetupVaultUnlock(userId: userId)
func getAccountSetupVaultUnlock(userId: String) async -> AccountSetupProgress? {
appSettingsStore.accountSetupVaultUnlock(userId: userId)
}

func getAccounts() throws -> [Account] {
Expand Down Expand Up @@ -1522,13 +1520,13 @@ actor DefaultStateService: StateService { // swiftlint:disable:this type_body_le
func setAccountSetupAutofill(_ autofillSetup: AccountSetupProgress?, userId: String?) async throws {
let userId = try userId ?? getActiveAccountUserId()
appSettingsStore.setAccountSetupAutofill(autofillSetup, userId: userId)
try await updateSettingsBadgePublisher(userId: userId)
await updateSettingsBadgePublisher(userId: userId)
}

func setAccountSetupVaultUnlock(_ vaultUnlockSetup: AccountSetupProgress?, userId: String?) async throws {
let userId = try userId ?? getActiveAccountUserId()
appSettingsStore.setAccountSetupVaultUnlock(vaultUnlockSetup, userId: userId)
try await updateSettingsBadgePublisher(userId: userId)
await updateSettingsBadgePublisher(userId: userId)
}

func setActiveAccount(userId: String) async throws {
Expand Down Expand Up @@ -1774,7 +1772,7 @@ actor DefaultStateService: StateService { // swiftlint:disable:this type_body_le

func settingsBadgePublisher() async throws -> AnyPublisher<SettingsBadgeState, Never> {
let userId = try getActiveAccountUserId()
try await updateSettingsBadgePublisher(userId: userId)
await updateSettingsBadgePublisher(userId: userId)
return settingsBadgeByUserIdSubject.compactMap { $0[userId] }.eraseToAnyPublisher()
}

Expand Down Expand Up @@ -1812,9 +1810,9 @@ actor DefaultStateService: StateService { // swiftlint:disable:this type_body_le
///
/// - Parameter userId: The user ID whose settings badge count should be updated.
///
private func updateSettingsBadgePublisher(userId: String) async throws {
let autofillSetupProgress = try await getAccountSetupAutofill(userId: userId)
let vaultUnlockSetupProgress = try await getAccountSetupVaultUnlock(userId: userId)
private func updateSettingsBadgePublisher(userId: String) async {
let autofillSetupProgress = await getAccountSetupAutofill(userId: userId)
let vaultUnlockSetupProgress = await getAccountSetupVaultUnlock(userId: userId)
let badgeCount = [autofillSetupProgress, vaultUnlockSetupProgress]
.compactMap { $0 }
.filter { $0 != .complete }
Expand Down
10 changes: 4 additions & 6 deletions BitwardenShared/Core/Platform/Services/TestHelpers/MockStateService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,14 +153,12 @@ class MockStateService: StateService { // swiftlint:disable:this type_body_lengt
return accounts
}

func getAccountSetupAutofill(userId: String?) async throws -> AccountSetupProgress? {
let userId = try unwrapUserId(userId)
return accountSetupAutofill[userId]
func getAccountSetupAutofill(userId: String) async -> AccountSetupProgress? {
accountSetupAutofill[userId]
}

func getAccountSetupVaultUnlock(userId: String?) async throws -> AccountSetupProgress? {
let userId = try unwrapUserId(userId)
return accountSetupVaultUnlock[userId]
func getAccountSetupVaultUnlock(userId: String) async -> AccountSetupProgress? {
accountSetupVaultUnlock[userId]
}

func getAccountIdOrActiveId(userId: String?) async throws -> String {
Expand Down
Loading
0